Re: [PATCH 08/13] x86/virt/tdx: Add SEAMCALL wrappers to add TD private pages

[Yan Zhao]

On Wed, Jan 15, 2025 at 12:32:24AM +0100, Paolo Bonzini wrote:
> On 1/6/25 06:50, Yan Zhao wrote:
> > Yeah.
> > So, do you think we need to have tdh_mem_page_aug() to support 4K level page
> > only and ask for Dave's review again for huge page?
> 
> You're right that TDH.MEM.PAGE.AUG is basically the only case in which a
> struct folio is involved; on the other hand that also means that the
> arch/x86/virt part of large page support will be tiny and I don't think it
> will be a problem to review it again (for either Dave or myself).
> 
> > Do we need to add param "level" ?
> > - if yes, "struct page" looks not fit.
> 
> Maybe, but I think adding folio knowledge now would be a bit too
> hypothetical.
> 
> > - if not, hardcode it as 0 in the wrapper and convert "pfn" to "struct page"?
> 
> I think it makes sense to add "int level" now everywhere, even if it is just
> to match the SEPT API and to have the same style for computing the SEAMCALL
> arguments.  I'd rather keep the arguments simple with just "gpa | level"
> (i.e. gpa/level instead of gfn/level) as the computation: that's because gpa
> is more obviously a u64.
> 
> I've pushed to kvm-coco-queue; if you have some time to double check what I
> did that's great, otherwise if I don't hear from you I'll post around noon
> European time the v3 of this series.
For tdh_mem_sept_add(), tdh_mem_page_aug(), tdh_mem_page_add()
- Use tdx_clflush_page() instead of  invoking clflush_cache_range() directly to
  share the common comment of tdx_clflush_page().
- prefer page_to_phy() over page_to_pfn()?
  https://lore.kernel.org/kvm/0070a616-5233-4a8d-8797-eb9f182f074d@xxxxxxxxx/


4d0824a1daba ("x86/virt/tdx: Add SEAMCALL wrappers to add TD private pages")
contains unexpected changes to tdh_mem_sept_add().
u64 tdh_mem_sept_add(struct tdx_td *td, u64 gpa, int level, struct page *page, u64 *ext_err1, u64 *ext_err2)
   {
  +       u64 hpa = page_to_pfn(page) << PAGE_SHIFT;
          struct tdx_module_args args = {
                  .rcx = gpa | level,
                  .rdx = tdx_tdr_pa(td),
  -               .r8 = page_to_pfn(page) << PAGE_SHIFT,
  +               .r8 = hpa,
          };
          u64 ret;

  -       clflush_cache_range(page_to_virt(page), PAGE_SIZE);
  +       clflush_cache_range(__va(hpa), PAGE_SIZE);
          ret = seamcall_ret(TDH_MEM_SEPT_ADD, &args);

          *ext_err1 = args.rcx;
  @@ -1522,6 +1544,26 @@ u64 tdh_vp_addcx(struct tdx_vp *vp, struct page *tdcx_page)
   }
   EXPORT_SYMBOL_GPL(tdh_vp_addcx);

> 
> I have also asked Amazon, since they use KVM without struct page, whether it
> is a problem to have struct page pervasively in the API and they don't care.
> 
> Paolo
>