Re: [PATCH v2] x86/hyperv: use dynamically allocated page for hypercalls

[Alejandro Vallejo <agarciav@xxxxxxx>]

On Mon Apr 28, 2025 at 12:07 PM BST, Andrew Cooper wrote:
> On 28/04/2025 11:55 am, Alejandro Vallejo wrote:
>> On Mon Apr 28, 2025 at 10:41 AM BST, Roger Pau Monné wrote:
>>> On Fri, Apr 25, 2025 at 04:43:31PM -0700, Ariadne Conill wrote:
>>>> Previously Xen placed the hypercall page at the highest possible MFN,
>>>> but this caused problems on systems where there is more than 36 bits
>>>> of physical address space.
>>>>
>>>> In general, it also seems unreliable to assume that the highest possible
>>>> MFN is not already reserved for some other purpose.
>>>>
>>>> Changes from v1:
>>>> - Continue to use fixmap infrastructure
>>>> - Use panic in Hyper-V setup() function instead of returning -ENOMEM
>>>>   on hypercall page allocation failure
>>>>
>>>> Fixes: 620fc734f854 ("x86/hyperv: setup hypercall page")
>>>> Cc: Alejandro Vallejo <agarciav@xxxxxxx>
>>>> Cc: Alexander M. Merritt <alexander@xxxxxxxxx>
>>>> Signed-off-by: Ariadne Conill <ariadne@ariadne.space>
>>>> ---
>>>>  xen/arch/x86/guest/hyperv/hyperv.c      | 17 +++++++----------
>>>>  xen/arch/x86/include/asm/guest/hyperv.h |  3 ---
>>>>  2 files changed, 7 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/xen/arch/x86/guest/hyperv/hyperv.c 
>>>> b/xen/arch/x86/guest/hyperv/hyperv.c
>>>> index 6989af38f1..0305374a06 100644
>>>> --- a/xen/arch/x86/guest/hyperv/hyperv.c
>>>> +++ b/xen/arch/x86/guest/hyperv/hyperv.c
>>>> @@ -98,7 +98,13 @@ static void __init setup_hypercall_page(void)
>>>>      rdmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
>>>>      if ( !hypercall_msr.enable )
>>>>      {
>>>> -        mfn = HV_HCALL_MFN;
>>>> +        void *hcall_page = alloc_xenheap_page();
>>>> +        if ( !hcall_page )
>>>> +            panic("Hyper-V: Failed to allocate hypercall trampoline 
>>>> page");
>>>> +
>>>> +        printk("Hyper-V: Allocated hypercall page @ %p.\n", hcall_page);
>>> This likely wants to be a dprintk, and possibly also print the
>>> physical address of the used page?  And no period at the end of the
>>> sentence IMO.
>>>
>>> I think Xen might have used the last page in the physical address
>>> range to prevent HyperV from possibly shattering a superpage in the
>>> second stage translation page-tables if normal RAM was used?
>>>
>>> However I don't know whether HyperV will shatter super-pages if a
>>> sub-page of it is used to contain the hypercall page (I don't think it
>>> should?)
>> I think it's quite unlikely.
>
> It will shatter superpages.
>
> The overlay is not part of guest memory, and will hide whatever is
> behind it while it is mapped, which will force a 4k PTE in EPT/NPT.

That's an implementation detail. They can very well copy the trampoline
to guest memory when there is such (and save the previous contents
elsewhere) and restore them when disabling the trampoline. It's a
trivial optimisation that would prevent shattering while being fully
compliant with the TLFS.

The actual physical location of the trampoline is fully undefined. It
is defined to be an overlay; but that's a specification, not an
implementation.

>
> Thinking about it, a better position would be adjacent to the APIC MMIO
> window, so at 0xfee01000.  The APIC MMIO window is forced to be a 4k
> mapping too, and the rest of the 2M window is normally empty.
>

Sounds like an assumption waiting to be broken. Just like the last page
of guest-physical was.

Cheers,
Alejandro