RE: [PATCH v3 2/6] x86/tdx: Support vmalloc() for tdx_enc_status_changed()

[Dexuan Cui]

> From: Kirill A. Shutemov <kirill@xxxxxxxxxxxxx>
> Sent: Friday, February 17, 2023 5:20 AM
> To: Dexuan Cui <decui@xxxxxxxxxxxxx>
> > ...
Hi Krill, sorry for my late reply!

> > Hi, Dave, I checked the huge vmalloc mapping code, but still don't know
> > how to get the underlying huge page info (if huge page is in use) and
> > try to use PG_LEVEL_2M/1G in try_accept_page() for vmalloc: I checked
> > is_vm_area_hugepages() and  __vfree() -> __vunmap(), and I think the
> > underlying page allocation info is internal to the mm code, and there
> > is no mm API to for me get the info in tdx_enc_status_changed().
> 
> I also don't obvious way to retrieve this info after vmalloc() is
> complete. split_page() makes all pages independent.
> 
> I think you can try to do this manually: allocate a vmalloc region,
> allocate pages manually, and put into the region. This way you always know
> page sizes and can optimize conversion to shared memory.
> 
> But it is tedious and I'm not sure if it worth the gain.
Thanks, I'll do some research on this idea.

> > Hi, Kirill, the load_unaligned_zeropad() issue is not addressed in
> > this patch. The issue looks like a generic issue that also happens to
> > AMD SNP vTOM mode and C-bit mode. Will need to figure out how to
> > address the issue. If we decide to adjust direct mapping to have the
> > shared bit set, it lools like we need to do the below for each
> > 'start_va' vmalloc page:
> >   pa = slow_virt_to_phys(start_va);
> >   set_memory_decrypted(phys_to_virt(pa), 1); -- this line calls
> > tdx_enc_status_changed() the second time for the same age, which is not
> > great. It looks like we need to find a way to reuse the cpa_flush()
> > related code in __set_memory_enc_pgtable() and make sure we call
> > tdx_enc_status_changed() only once for the same page from vmalloc()?
> 
> Actually, current code will change direct mapping for you. I just
> double-checked: the alias processing in __change_page_attr_set_clr() will
> change direct mapping if you call it on vmalloc()ed memory.
> 
> Splitting direct mapping is still unfortunate, but well.
> 
> >
> > Changes in v3:
> >   No change since v2.
> >
> >  arch/x86/coco/tdx/tdx.c | 69 ++++++++++++++++++++++++++---------------
> >  1 file changed, 44 insertions(+), 25 deletions(-)
> 
> I don't hate what you did here. But I think the code below is a bit
> cleaner.
> 
> Any opinions?
Thanks! Your version looks much better. I'll use it in in v4.