RE: [PATCH v3 1/3] x86/hyperv: Use slow_virt_to_phys() in page transition hypervisor callback

[Michael Kelley]

From: Edgecombe, Rick P <rick.p.edgecombe@xxxxxxxxx> Sent: Thursday, January 11, 2024 5:20 PM
> 
> On Fri, 2024-01-05 at 10:30 -0800, mhkelley58@xxxxxxxxx wrote:
> > + * It is also used in callbacks for CoCo VM page transitions between private
> > + * and shared because it works when the PRESENT bit is not set in the leaf
> > + * PTE. In such cases, the state of the PTEs, including the PFN, is otherwise
> > + * known to be valid, so the returned physical address is correct.  The similar
> > + * function vmalloc_to_pfn() can't be used because it requires the PRESENT bit.
> 
> I'm not sure about this comment. It is mostly about callers far away
> and other functions in vmalloc. Probably a decent chance to get stale.
> It also kind of begs the question of why vmalloc_to_pfn() requires the
> present bit in the leaf.

The comment is Kirill Shutemov's suggestion based on comments in
an earlier version of the patch series.  See [1].   The intent is to prevent
some later revision to slow_virt_to_phys() from adding a check for the
present bit and breaking the CoCo VM hypervisor callback.  Yes, the
comment could get stale, but I'm not sure how else to call out the
implicit dependency.  The idea of creating a private version of
slow_virt_to_phys() for use only in the CoCo VM hypervisor callback
is also discussed in the thread, but that seems worse overall.

As for why vmalloc_to_page() checks the present bit, I don't know.
But vmalloc_to_page() is very widely used, so trying to change it
doesn't seem viable.

> 
> It seems the first part of the comment is about why this is needed when
> __pa() exists. One reason given is that __pa() doesn't work with
> vmalloc memory. Then the next bit talks about another similar function
> that works with vmalloc memory.
> 
> So the comment is a risk to get stale, and leaves me a little confused
> why this function exists.
> 
> I think the reason is because vmalloc_to_pfn() *only* works with
> vmalloc memory and this is needed to work on other alias mappings.

Presumably so.  The first paragraph of the existing comment also
calls out "alloc_remap() areas on 32-bit NUMA systems" as
needing slow_virt_to_phys().

Michael

[1] https://lore.kernel.org/lkml/20230828221333.5blshosyqafbgwlc@xxxxxxxxxxxxxxxxx/