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

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

[Ariadne Conill]

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@amd.com>
Cc: Alexander M. Merritt <alexander@edera.dev>
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);
+
+        mfn = virt_to_mfn(hcall_page);
         hypercall_msr.enable = 1;
         hypercall_msr.guest_physical_address = mfn;
         wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
@@ -187,14 +193,6 @@ static int cf_check ap_setup(void)
     return setup_vp_assist();
 }
 
-static void __init cf_check e820_fixup(void)
-{
-    uint64_t s = HV_HCALL_MFN << PAGE_SHIFT;
-
-    if ( !e820_add_range(s, s + PAGE_SIZE, E820_RESERVED) )
-        panic("Unable to reserve Hyper-V hypercall range\n");
-}
-
 static int cf_check flush_tlb(
     const cpumask_t *mask, const void *va, unsigned int flags)
 {
@@ -211,7 +209,6 @@ static const struct hypervisor_ops __initconst_cf_clobber ops = {
     .name = "Hyper-V",
     .setup = setup,
     .ap_setup = ap_setup,
-    .e820_fixup = e820_fixup,
     .flush_tlb = flush_tlb,
 };
 
diff --git a/xen/arch/x86/include/asm/guest/hyperv.h b/xen/arch/x86/include/asm/guest/hyperv.h
index c05efdce71..5792e77104 100644
--- a/xen/arch/x86/include/asm/guest/hyperv.h
+++ b/xen/arch/x86/include/asm/guest/hyperv.h
@@ -10,9 +10,6 @@
 
 #include <xen/types.h>
 
-/* Use top-most MFN for hypercall page */
-#define HV_HCALL_MFN   (((1ull << paddr_bits) - 1) >> HV_HYP_PAGE_SHIFT)
-
 /*
  * The specification says: "The partition reference time is computed
  * by the following formula:
-- 
2.39.5 (Apple Git-154)

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

[Jan Beulich]

On 26.04.2025 01:43, 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

Nit: This part wants to go ...

> Fixes: 620fc734f854 ("x86/hyperv: setup hypercall page")
> Cc: Alejandro Vallejo <agarciav@amd.com>
> Cc: Alexander M. Merritt <alexander@edera.dev>
> Signed-off-by: Ariadne Conill <ariadne@ariadne.space>
> ---

... somewhere below such a separator.

> --- 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 )

Nit (style): Blank line please between declaration(s) and statement(s). (Both
can probably be taken care of upon committing if no other need for a v3 arises.)

Jan

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

[Roger Pau Monné]

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@amd.com>
> Cc: Alexander M. Merritt <alexander@edera.dev>
> 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?)

Thanks, Roger.

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

[Alejandro Vallejo]

On Sat Apr 26, 2025 at 12:43 AM BST, 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.

Thanks for sending this!

Just one more thing on top of what Jan mentioned.

>
> 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@amd.com>
> Cc: Alexander M. Merritt <alexander@edera.dev>
> 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);

We should be printing the mfn (or maddr) rather than the virtual address
out of the allocator, IMO. Especially since we need to remap it anyway.

With that:

  Reviewed-by: Alejandro Vallejo <agarciav@amd.com>

Cheers,
Alejandro

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

[Alejandro Vallejo]

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@amd.com>
>> Cc: Alexander M. Merritt <alexander@edera.dev>
>> 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. Seeing how Linux simply vmalloc()s and
Microsoft seems to genuinely care about Linux in this day and age. It
seems fair to assume Hyper-V might just copy the old memory out and
rewrite it with the trampoline contents when enabling the MSR, thereby
keeping superpages together in their p2m.

>
> Thanks, Roger.

Cheers,
Alejandro

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

[Andrew Cooper]

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@amd.com>
>>> Cc: Alexander M. Merritt <alexander@edera.dev>
>>> 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.

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.

~Andrew

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

[Andrew Cooper]

On 26/04/2025 12:43 am, Ariadne Conill wrote:
> 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");

\n

~Andrew

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

[Alejandro Vallejo]

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@amd.com>
>>>> Cc: Alexander M. Merritt <alexander@edera.dev>
>>>> 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

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

[Roger Pau Monné]

On Mon, Apr 28, 2025 at 12:50:55PM +0100, Alejandro Vallejo wrote:
> 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@amd.com>
> >>>> Cc: Alexander M. Merritt <alexander@edera.dev>
> >>>> 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.

It's an implementation detail relevant from a guest perspective, as it
impacts guest performance.  IOW: we care about the specific (current)
implementation, as it's meaningful to how the guest-side should be
implemented.

> 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.

As a compromise - could we try to allocate from < 4GB first, and
resort to high memory if that doesn't work?  That would at least limit
shattering (if done) to the low 4GB, which is quite likely fragmented
already:

hcall_page = alloc_xenheap_pages(0, MEMF_bits(32));
if ( !hcall_page )
    hcall_page = alloc_xenheap_page();
if ( !hcall_page )
    panic(...);

That will need a comment to describe what's going on.

Thanks, Roger.

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

[Alejandro Vallejo]

On Tue Apr 29, 2025 at 9:15 AM BST, Roger Pau Monné wrote:
> On Mon, Apr 28, 2025 at 12:50:55PM +0100, Alejandro Vallejo wrote:
>> 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@amd.com>
>> >>>> Cc: Alexander M. Merritt <alexander@edera.dev>
>> >>>> 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.
>
> It's an implementation detail relevant from a guest perspective, as it
> impacts guest performance.  IOW: we care about the specific (current)
> implementation, as it's meaningful to how the guest-side should be
> implemented.

Indeed, I didn't mean otherwise.

My reply had more to do with the assertiveness of "it will shatter". The
reality is that we can't know, short of microbenching a read into the
trampoline after a TLB flush. And even then it may very well be
version-dependent (where old did, and newer doesn't, for instance).

>
>> 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.
>
> As a compromise - could we try to allocate from < 4GB first, and
> resort to high memory if that doesn't work?  That would at least limit
> shattering (if done) to the low 4GB, which is quite likely fragmented
> already:
>
> hcall_page = alloc_xenheap_pages(0, MEMF_bits(32));
> if ( !hcall_page )
>     hcall_page = alloc_xenheap_page();
> if ( !hcall_page )
>     panic(...);
>
> That will need a comment to describe what's going on.

Sounds far more resilient to me. I could get behind that, sure.

Ariadne, thoughts?

Cheers,
Alejandro