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(-)
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@amd.com>
Cc: Alexander M. Merritt <alexander@edera.dev>
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
@@ -16,6 +16,7 @@
#include "private.h"
+void __read_mostly *hv_hcall_page;
struct ms_hyperv_info __read_mostly ms_hyperv;
DEFINE_PER_CPU_READ_MOSTLY(void *, hv_input_page);
DEFINE_PER_CPU_READ_MOSTLY(void *, hv_vp_assist);
@@ -80,13 +81,11 @@ const struct hypervisor_ops *__init hyperv_probe(void)
return &ops;
}
-static void __init setup_hypercall_page(void)
+static int __init setup_hypercall_page(void)
{
union hv_x64_msr_hypercall_contents hypercall_msr;
union hv_guest_os_id guest_id;
- unsigned long mfn;
-
- BUILD_BUG_ON(HV_HYP_PAGE_SHIFT != PAGE_SHIFT);
+ unsigned long mfn, start;
rdmsrl(HV_X64_MSR_GUEST_OS_ID, guest_id.raw);
if ( !guest_id.raw )
@@ -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");
+ 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;
@@ -109,9 +120,9 @@ static void __init setup_hypercall_page(void)
rdmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
BUG_ON(!hypercall_msr.enable);
- set_fixmap_x(FIX_X_HYPERV_HCALL, mfn << PAGE_SHIFT);
-
hcall_page_ready = true;
+
+ return 0;
}
static int setup_hypercall_pcpu_arg(void)
@@ -165,9 +176,8 @@ static int setup_vp_assist(void)
static void __init cf_check setup(void)
{
- ASM_CONSTANT(HV_HCALL_PAGE, __fix_x_to_virt(FIX_X_HYPERV_HCALL));
-
- setup_hypercall_page();
+ if ( setup_hypercall_page() )
+ panic("Hyper-V hypercall page setup failed\n");
if ( setup_hypercall_pcpu_arg() )
panic("Hyper-V hypercall percpu arg setup failed\n");
@@ -187,14 +197,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 +213,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/fixmap.h b/xen/arch/x86/include/asm/fixmap.h
index 516ec3fa6c..7384a2d07c 100644
--- a/xen/arch/x86/include/asm/fixmap.h
+++ b/xen/arch/x86/include/asm/fixmap.h
@@ -98,9 +98,6 @@ static inline unsigned long virt_to_fix(const unsigned long vaddr)
enum fixed_addresses_x {
/* Index 0 is reserved since fix_x_to_virt(0) == FIXADDR_X_TOP. */
FIX_X_RESERVED,
-#ifdef CONFIG_HYPERV_GUEST
- FIX_X_HYPERV_HCALL,
-#endif
__end_of_fixed_addresses_x
};
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"
+ "call *%[target_addr]"
: "=a" (status), "+c" (control),
"+d" (input_addr) ASM_CALL_CONSTRAINT
- : "r" (r8)
+ : [output_addr] "r" (output_addr), [target_addr] "r" (hv_hcall_page)
: "memory" );
return status;
@@ -37,13 +37,13 @@ static inline uint64_t hv_do_fast_hypercall(uint16_t code,
{
uint64_t status;
uint64_t control = code | HV_HYPERCALL_FAST_BIT;
- register unsigned long r8 asm ( "r8" ) = input2;
/* See TLFS for volatile registers */
- asm volatile ( "call hv_hcall_page"
+ asm volatile ( "mov %[input2], %%r8\n"
+ "call *[target_addr]"
: "=a" (status), "+c" (control),
"+d" (input1) ASM_CALL_CONSTRAINT
- : "r" (r8) );
+ : [input2] "r" (input2), [target_addr] "r" (hv_hcall_page) );
return status;
}
diff --git a/xen/arch/x86/include/asm/guest/hyperv-tlfs.h b/xen/arch/x86/include/asm/guest/hyperv-tlfs.h
index 79cfc90dd8..01fc76f20c 100644
--- a/xen/arch/x86/include/asm/guest/hyperv-tlfs.h
+++ b/xen/arch/x86/include/asm/guest/hyperv-tlfs.h
@@ -13,6 +13,8 @@
#include <xen/page-size.h>
#include <xen/types.h>
+extern void *hv_hcall_page;
+
/*
* While not explicitly listed in the TLFS, Hyper-V always runs with a page size
* of 4096. These definitions are used when communicating with Hyper-V using
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:
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 53bafc98a5..3d0c8e6ffc 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -359,10 +359,6 @@ SECTIONS
DWARF2_DEBUG_SECTIONS
-#ifdef CONFIG_HYPERV_GUEST
- hv_hcall_page = ABSOLUTE(HV_HCALL_PAGE - XEN_VIRT_START + __XEN_VIRT_START);
-#endif
-
DISCARD_SECTIONS
#ifndef EFI
--
2.39.5 (Apple Git-154)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@amd.com>
> Cc: Alexander M. Merritt <alexander@edera.dev>
> 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?
> + 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.
> + "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
Generally looks good.
Thanks,
Jason
> : "=a" (status), "+c" (control),
> "+d" (input_addr) ASM_CALL_CONSTRAINT
> - : "r" (r8)
> + : [output_addr] "r" (output_addr), [target_addr] "r" (hv_hcall_page)
> : "memory" );
>
> return status;
Hi,
> On Apr 25, 2025, at 2:02 PM, Jason Andryuk <jason.andryuk@amd.com> 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@amd.com>
>> Cc: Alexander M. Merritt <alexander@edera.dev>
>> 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
On 25/04/2025 5:51 pm, Ariadne Conill wrote:
> 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");
> + return -ENOMEM;
> + }
> +
> + printk("Hyper-V: Allocated hypercall page @ %p.\n", hv_hcall_page);
> +
> + mfn = virt_to_mfn(hv_hcall_page);
Up to here is ok; this is just choosing a different page, but...
> 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);
... this and ...
> }
> else
> mfn = hypercall_msr.guest_physical_address;
> @@ -109,9 +120,9 @@ static void __init setup_hypercall_page(void)
> rdmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
> BUG_ON(!hypercall_msr.enable);
>
> - set_fixmap_x(FIX_X_HYPERV_HCALL, mfn << PAGE_SHIFT);
... this break the case where the overlay is already chosen and cannot move.
It really needs to stay using set_fixmap_x(), which in turn means you
can ...
> 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"
> + "call *%[target_addr]"
> : "=a" (status), "+c" (control),
> "+d" (input_addr) ASM_CALL_CONSTRAINT
> - : "r" (r8)
> + : [output_addr] "r" (output_addr), [target_addr] "r" (hv_hcall_page)
> : "memory" );
... undo this speculative security vulnerability you've got by not using
INDIRECT_CALL.
The point of FIXMAP_X is to provide a virtual address in the main 1G
range for .text/.data/.rodata/.bss, which can point to an arbitrary
location, and can be regularly CALL'd.
~Andrew
© 2016 - 2025 Red Hat, Inc.