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

[Ariadne Conill <ariadne@ariadne.space>]

Hi,

On Apr 25, 2025, at 2:02 PM, Jason Andryuk <jason.andryuk@xxxxxxx> wrote:

On 2025-04-25 12:51, 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.
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            | 39 ++++++++++---------
 xen/arch/x86/include/asm/fixmap.h             |  3 --
 xen/arch/x86/include/asm/guest/hyperv-hcall.h | 12 +++---
 xen/arch/x86/include/asm/guest/hyperv-tlfs.h  |  2 +
 xen/arch/x86/include/asm/guest/hyperv.h       |  3 --
 xen/arch/x86/xen.lds.S                        |  4 --
 6 files changed, 28 insertions(+), 35 deletions(-)
diff --git a/xen/arch/x86/guest/hyperv/hyperv.c b/xen/arch/x86/guest/hyperv/hyperv.c
index 6989af38f1..637b4bf335 100644
--- a/xen/arch/x86/guest/hyperv/hyperv.c
+++ b/xen/arch/x86/guest/hyperv/hyperv.c

@@ -98,10 +97,22 @@ static void __init setup_hypercall_page(void)
     rdmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
     if ( !hypercall_msr.enable )
     {
-        mfn = HV_HCALL_MFN;
+        hv_hcall_page = alloc_xenheap_page();
+        if ( !hv_hcall_page )
+        {
+            printk("Hyper-V: Failed to allocate hypercall trampoline page\n");

Minor, but maybe panic() here and avoid changing the return type?

Sure, can do that.

+            return -ENOMEM;
+        }
+
+        printk("Hyper-V: Allocated hypercall page @ %p.\n", hv_hcall_page);
+
+        mfn = virt_to_mfn(hv_hcall_page);
         hypercall_msr.enable = 1;
         hypercall_msr.guest_physical_address = mfn;
         wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
+
+        start = (unsigned long) hv_hcall_page;
+        modify_xen_mappings(start, start + PAGE_SIZE, PAGE_HYPERVISOR_RX);
     }
     else
         mfn = hypercall_msr.guest_physical_address;

diff --git a/xen/arch/x86/include/asm/guest/hyperv-hcall.h b/xen/arch/x86/include/asm/guest/hyperv-hcall.h
index b76dbf9ccc..b73edca7c6 100644
--- a/xen/arch/x86/include/asm/guest/hyperv-hcall.h
+++ b/xen/arch/x86/include/asm/guest/hyperv-hcall.h
@@ -20,13 +20,13 @@ static inline uint64_t hv_do_hypercall(uint64_t control, paddr_t input_addr,
                                        paddr_t output_addr)
 {
     uint64_t status;
-    register unsigned long r8 asm ( "r8" ) = output_addr;
       /* See TLFS for volatile registers */
-    asm volatile ( "call hv_hcall_page"
+    asm volatile ( "mov %[output_addr], %%r8\n"

Don't you need to list r8 as clobbered?  Or maybe just retain the r8 handling above and below to avoid this mov.

This can probably mostly be reverted if we get the fixmap working.

The issue we were facing before was a situation where calling hv_hcall_page directly resulted in a page fault due to NX-bit being set on the page.

+                   "call *%[target_addr]"

It might be preferable to retain a direct call which can still be installed with __set_fixmap_x.  Otherwise, __set_fixmap_x can be removed

I think we should use fixmap, I just need to figure out what that looks like.

Will send v2 patch on Monday.

Ariadne