RE: [PATCH v2 4/8] x86/sev: Enable PVALIDATE for PFNs without a valid virtual address

[Michael Kelley]

From: Edgecombe, Rick P <rick.p.edgecombe@xxxxxxxxx> Sent: Monday, November 27, 2023 1:39 PM
> 
> On Tue, 2023-11-21 at 13:20 -0800, mhkelley58@xxxxxxxxx wrote:
> > +static int pvalidate_pfn(unsigned long vaddr, unsigned int size,
> > +                        unsigned long pfn, bool validate, int *rc2)
> > +{
> > +       int rc;
> > +       struct page *page = pfn_to_page(pfn);
> > +
> > +       *rc2 = vmap_pages_range(vaddr, vaddr + PAGE_SIZE,
> > +                       PAGE_KERNEL, &page, PAGE_SHIFT);
> 
> Can't this fail and then the pvalidate below would encounter trouble?

Yes.  My intent was to just let the PVALIDATE fail because of operating
on a vaddr that's invalid.  But that would be worth a comment.

> 
> Sort of separately, if those vmalloc objections can't be worked
> through, did you consider doing something like text_poke() does (create
> the temporary mapping in a temporary MM) for pvalidate purposes? I
> don't know enough about what kind of special exceptions might popup
> during that operation though, might be playing with fire...

Interesting idea.  But from a quick glance at the text_poke() code,
such an approach seems somewhat complex, and I suspect it will have 
the same perf issues (or worse) as creating a new vmalloc area for
each PVALIDATE invocation.

At this point, the complexity of creating the temp mapping for
PVALIDATE is seeming excessive.  On balance it seems simpler to
revert to an approach where the use of set_memory_np() and
set_memory_p() is conditional.  It would be necessary when #VC
and #VE exceptions are directed to a paravisor.  (This assumes the
paravisor interface in the hypervisor callbacks does the natural thing
of working with physical addresses, so there's no need for a temp
mapping.)

Optionally, the set_memory_np()/set_memory_p() approach could
be used in other cases where the hypervisor callbacks work with
physical addresses.  But it can't be used with cases where the hypervisor
callbacks need valid virtual addresses.

So on net, set_memory_np()/set_memory_p() would be used in
the Hyper-V cases of TDX and SEV-SNP with a paravisor.   It could
optionally be used with TDX with no paravisor, but my sense is
that Kirill wants to keep TDX "as is" and let the exception handlers
do the load_unaligned_zeropad() fixup.

It could not be used with SEV-SNP with no paravisor.   Additional fixes
may be needed on the SEV-SNP side to properly fixup
load_unaligned_zeropad() accesses to a page that's in transition
between encrypted and decrypted.

I'll work on a patch series that takes the conditional approach.

Michael

> 
> > +       rc = pvalidate(vaddr, size, validate);
> > +       vunmap_range(vaddr, vaddr + PAGE_SIZE);
> > +
> > +       return rc;
> > +}