| Message ID | 20191229183341.14877-5-liuwe@microsoft.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | More Hyper-V infrastructure | expand |
From: Wei Liu <wei.liu.xen@gmail.com> On Behalf Of Wei Liu Sent: Sunday, December 29, 2019 10:34 AM > > Signed-off-by: Wei Liu <liuwe@microsoft.com> > --- > xen/arch/x86/guest/hyperv/hyperv.c | 41 +++++++++++++++++++++++++++--- > 1 file changed, 38 insertions(+), 3 deletions(-) > > diff --git a/xen/arch/x86/guest/hyperv/hyperv.c b/xen/arch/x86/guest/hyperv/hyperv.c > index c6a26c5453..438910c8cb 100644 > --- a/xen/arch/x86/guest/hyperv/hyperv.c > +++ b/xen/arch/x86/guest/hyperv/hyperv.c > @@ -19,16 +19,17 @@ > * Copyright (c) 2019 Microsoft. > */ > #include <xen/init.h> > +#include <xen/domain_page.h> > > #include <asm/guest.h> > #include <asm/guest/hyperv-tlfs.h> > > struct ms_hyperv_info __read_mostly ms_hyperv; > > -static const struct hypervisor_ops ops = { > - .name = "Hyper-V", > -}; > +void *hv_hypercall; > +static struct page_info *hv_hypercall_page; > > +static const struct hypervisor_ops ops; > const struct hypervisor_ops *__init hyperv_probe(void) > { > uint32_t eax, ebx, ecx, edx; > @@ -71,6 +72,40 @@ const struct hypervisor_ops *__init hyperv_probe(void) > return &ops; > } > > +static void __init setup_hypercall_page(void) > +{ > + union hv_x64_msr_hypercall_contents hypercall_msr; > + > + /* Unfortunately there isn't a really good way to unwind Xen to > + * not use Hyper-V hooks, so panic if anything goes wrong. > + * > + * In practice if page allocation fails this early on it is > + * unlikely we can get a working system later. > + */ > + hv_hypercall_page = alloc_domheap_page(NULL, 0); > + if ( !hv_hypercall_page ) > + panic("Failed to allocate Hyper-V hypercall page\n"); > + > + hv_hypercall = __map_domain_page_global(hv_hypercall_page); > + if ( !hv_hypercall ) > + panic("Failed to map Hyper-V hypercall page\n"); > + > + rdmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64); > + hypercall_msr.enable = 1; > + hypercall_msr.guest_physical_address = page_to_maddr(hv_hypercall_page); The "guest_physical_address" field is actually the guest physical page number. So the physical address needs to be right shifted 12 bits before being stored here. I'd recommend using HV_HYP_PAGE_SHIFT from hyperv-tlfs.h as the shift value; it was introduced to deal with the possibility that the page size used and expected by the Hyper-V interface is different from the page size used by the guest VM (which can happen on ARM64, though not on x86). Michael > + wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64); > +} > + > +static void __init setup(void) > +{ > + setup_hypercall_page(); > +} > + > +static const struct hypervisor_ops ops = { > + .name = "Hyper-V", > + .setup = setup, > +}; > + > /* > * Local variables: > * mode: C > -- > 2.20.1
On Sun, Dec 29, 2019 at 07:54:30PM +0000, Michael Kelley wrote: [...] > > > > +static void __init setup_hypercall_page(void) > > +{ > > + union hv_x64_msr_hypercall_contents hypercall_msr; > > + > > + /* Unfortunately there isn't a really good way to unwind Xen to > > + * not use Hyper-V hooks, so panic if anything goes wrong. > > + * > > + * In practice if page allocation fails this early on it is > > + * unlikely we can get a working system later. > > + */ > > + hv_hypercall_page = alloc_domheap_page(NULL, 0); > > + if ( !hv_hypercall_page ) > > + panic("Failed to allocate Hyper-V hypercall page\n"); > > + > > + hv_hypercall = __map_domain_page_global(hv_hypercall_page); > > + if ( !hv_hypercall ) > > + panic("Failed to map Hyper-V hypercall page\n"); > > + > > + rdmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64); > > + hypercall_msr.enable = 1; > > + hypercall_msr.guest_physical_address = page_to_maddr(hv_hypercall_page); > > The "guest_physical_address" field is actually the guest physical page number. > So the physical address needs to be right shifted 12 bits before being stored > here. I'd recommend using HV_HYP_PAGE_SHIFT from hyperv-tlfs.h as > the shift value; it was introduced to deal with the possibility that the page > size used and expected by the Hyper-V interface is different from the page > size used by the guest VM (which can happen on ARM64, though not on x86). Good catch, and thanks for the tip here. I will fix this in the next version. Wei.
On 29/12/2019 18:33, Wei Liu wrote: > @@ -71,6 +72,40 @@ const struct hypervisor_ops *__init hyperv_probe(void) > return &ops; > } > > +static void __init setup_hypercall_page(void) > +{ > + union hv_x64_msr_hypercall_contents hypercall_msr; > + > + /* Unfortunately there isn't a really good way to unwind Xen to > + * not use Hyper-V hooks, so panic if anything goes wrong. > + * > + * In practice if page allocation fails this early on it is > + * unlikely we can get a working system later. > + */ > + hv_hypercall_page = alloc_domheap_page(NULL, 0); > + if ( !hv_hypercall_page ) > + panic("Failed to allocate Hyper-V hypercall page\n"); > + > + hv_hypercall = __map_domain_page_global(hv_hypercall_page); > + if ( !hv_hypercall ) > + panic("Failed to map Hyper-V hypercall page\n"); I really hope this doesn't actually function correctly. This should result in an NX mapping. See feedback on the next patch for an alternative suggestion. ~Andrew
On Mon, Dec 30, 2019 at 12:55:22PM +0000, Andrew Cooper wrote: > On 29/12/2019 18:33, Wei Liu wrote: > > @@ -71,6 +72,40 @@ const struct hypervisor_ops *__init hyperv_probe(void) > > return &ops; > > } > > > > +static void __init setup_hypercall_page(void) > > +{ > > + union hv_x64_msr_hypercall_contents hypercall_msr; > > + > > + /* Unfortunately there isn't a really good way to unwind Xen to > > + * not use Hyper-V hooks, so panic if anything goes wrong. > > + * > > + * In practice if page allocation fails this early on it is > > + * unlikely we can get a working system later. > > + */ > > + hv_hypercall_page = alloc_domheap_page(NULL, 0); > > + if ( !hv_hypercall_page ) > > + panic("Failed to allocate Hyper-V hypercall page\n"); > > + > > + hv_hypercall = __map_domain_page_global(hv_hypercall_page); > > + if ( !hv_hypercall ) > > + panic("Failed to map Hyper-V hypercall page\n"); > > I really hope this doesn't actually function correctly. This should > result in an NX mapping. > Ah, stupid me. I had actually looked at Xen's implementation and thought "wouldn't it be nice to save one page in the image". I clearly missed that __map_domain_page_global makes the page NX. Wei. > See feedback on the next patch for an alternative suggestion. > > ~Andrew
On 30/12/2019 13:33, Wei Liu wrote: > On Mon, Dec 30, 2019 at 12:55:22PM +0000, Andrew Cooper wrote: >> On 29/12/2019 18:33, Wei Liu wrote: >>> @@ -71,6 +72,40 @@ const struct hypervisor_ops *__init hyperv_probe(void) >>> return &ops; >>> } >>> >>> +static void __init setup_hypercall_page(void) >>> +{ >>> + union hv_x64_msr_hypercall_contents hypercall_msr; >>> + >>> + /* Unfortunately there isn't a really good way to unwind Xen to >>> + * not use Hyper-V hooks, so panic if anything goes wrong. >>> + * >>> + * In practice if page allocation fails this early on it is >>> + * unlikely we can get a working system later. >>> + */ >>> + hv_hypercall_page = alloc_domheap_page(NULL, 0); >>> + if ( !hv_hypercall_page ) >>> + panic("Failed to allocate Hyper-V hypercall page\n"); >>> + >>> + hv_hypercall = __map_domain_page_global(hv_hypercall_page); >>> + if ( !hv_hypercall ) >>> + panic("Failed to map Hyper-V hypercall page\n"); >> I really hope this doesn't actually function correctly. This should >> result in an NX mapping. >> > Ah, stupid me. I had actually looked at Xen's implementation and thought > "wouldn't it be nice to save one page in the image". Its 4k, and there is a lot to be said for not having random tiny critical bits of infrastructure spread dynamically around GFN space. > I clearly missed that __map_domain_page_global makes the page NX. It is hidden in the depths of PAGE_HYPERVISOR. ~Andrew
diff --git a/xen/arch/x86/guest/hyperv/hyperv.c b/xen/arch/x86/guest/hyperv/hyperv.c index c6a26c5453..438910c8cb 100644 --- a/xen/arch/x86/guest/hyperv/hyperv.c +++ b/xen/arch/x86/guest/hyperv/hyperv.c @@ -19,16 +19,17 @@ * Copyright (c) 2019 Microsoft. */ #include <xen/init.h> +#include <xen/domain_page.h> #include <asm/guest.h> #include <asm/guest/hyperv-tlfs.h> struct ms_hyperv_info __read_mostly ms_hyperv; -static const struct hypervisor_ops ops = { - .name = "Hyper-V", -}; +void *hv_hypercall; +static struct page_info *hv_hypercall_page; +static const struct hypervisor_ops ops; const struct hypervisor_ops *__init hyperv_probe(void) { uint32_t eax, ebx, ecx, edx; @@ -71,6 +72,40 @@ const struct hypervisor_ops *__init hyperv_probe(void) return &ops; } +static void __init setup_hypercall_page(void) +{ + union hv_x64_msr_hypercall_contents hypercall_msr; + + /* Unfortunately there isn't a really good way to unwind Xen to + * not use Hyper-V hooks, so panic if anything goes wrong. + * + * In practice if page allocation fails this early on it is + * unlikely we can get a working system later. + */ + hv_hypercall_page = alloc_domheap_page(NULL, 0); + if ( !hv_hypercall_page ) + panic("Failed to allocate Hyper-V hypercall page\n"); + + hv_hypercall = __map_domain_page_global(hv_hypercall_page); + if ( !hv_hypercall ) + panic("Failed to map Hyper-V hypercall page\n"); + + rdmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64); + hypercall_msr.enable = 1; + hypercall_msr.guest_physical_address = page_to_maddr(hv_hypercall_page); + wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64); +} + +static void __init setup(void) +{ + setup_hypercall_page(); +} + +static const struct hypervisor_ops ops = { + .name = "Hyper-V", + .setup = setup, +}; + /* * Local variables: * mode: C
Signed-off-by: Wei Liu <liuwe@microsoft.com> --- xen/arch/x86/guest/hyperv/hyperv.c | 41 +++++++++++++++++++++++++++--- 1 file changed, 38 insertions(+), 3 deletions(-)