| Message ID | 20200129202034.15052-13-liuwe@microsoft.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | More Hyper-V infrastructures | expand |
> -----Original Message----- > From: Wei Liu <wei.liu.xen@gmail.com> On Behalf Of Wei Liu > Sent: 29 January 2020 20:21 > To: Xen Development List <xen-devel@lists.xenproject.org> > Cc: Durrant, Paul <pdurrant@amazon.co.uk>; Michael Kelley > <mikelley@microsoft.com>; Wei Liu <liuwe@microsoft.com>; Wei Liu > <wl@xen.org>; Jan Beulich <jbeulich@suse.com>; Andrew Cooper > <andrew.cooper3@citrix.com>; Roger Pau Monné <roger.pau@citrix.com> > Subject: [PATCH v5 12/12] x86/hyperv: setup VP assist page > > VP assist page is rather important as we need to toggle some bits in it > for efficient nested virtualisation. > > Signed-off-by: Wei Liu <liuwe@microsoft.com> > --- > v5: > 1. Deal with error properly instead of always panicking > 2. Swap percpu variables declarations' location > > v4: > 1. Use private.h > 2. Prevent leak > > v3: > 1. Use xenheap page > 2. Drop set_vp_assist > > v2: > 1. Use HV_HYP_PAGE_SHIFT instead > --- > xen/arch/x86/guest/hyperv/hyperv.c | 44 ++++++++++++++++++++++++++++- > xen/arch/x86/guest/hyperv/private.h | 1 + > 2 files changed, 44 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/x86/guest/hyperv/hyperv.c > b/xen/arch/x86/guest/hyperv/hyperv.c > index af0d6ed692..bc40a3d338 100644 > --- a/xen/arch/x86/guest/hyperv/hyperv.c > +++ b/xen/arch/x86/guest/hyperv/hyperv.c > @@ -31,6 +31,7 @@ > > struct ms_hyperv_info __read_mostly ms_hyperv; > DEFINE_PER_CPU_READ_MOSTLY(void *, hv_pcpu_input_page); > +DEFINE_PER_CPU_READ_MOSTLY(void *, hv_vp_assist); > DEFINE_PER_CPU_READ_MOSTLY(unsigned int, hv_vp_index); > > static uint64_t generate_guest_id(void) > @@ -155,16 +156,57 @@ static int setup_hypercall_pcpu_arg(void) > return 0; > } > > +static int setup_vp_assist(void) > +{ > + void *mapping; > + uint64_t val; > + > + mapping = this_cpu(hv_vp_assist); > + > + if ( !mapping ) > + { > + mapping = alloc_xenheap_page(); > + if ( !mapping ) > + { > + printk("Failed to allocate vp_assist page for CPU%u\n", > + smp_processor_id()); > + return -ENOMEM; > + } > + > + clear_page(mapping); > + this_cpu(hv_vp_assist) = mapping; > + } > + > + val = (virt_to_mfn(mapping) << HV_HYP_PAGE_SHIFT) > + | HV_X64_MSR_VP_ASSIST_PAGE_ENABLE; Perhaps it would be neater to put the viridian_page_msr union into hyperv-tlfs.h and then use that. Paul > + wrmsrl(HV_X64_MSR_VP_ASSIST_PAGE, val); > + > + return 0; > +} > + > static void __init setup(void) > { > setup_hypercall_page(); > + > if ( setup_hypercall_pcpu_arg() ) > panic("Hypercall percpu arg setup failed\n"); > + > + if ( setup_vp_assist() ) > + panic("VP assist page setup failed\n"); > } > > static int ap_setup(void) > { > - return setup_hypercall_pcpu_arg(); > + int rc; > + > + rc = setup_hypercall_pcpu_arg(); > + if ( rc ) > + goto out; > + > + rc = setup_vp_assist(); > + > + out: > + return rc; > } > > static const struct hypervisor_ops ops = { > diff --git a/xen/arch/x86/guest/hyperv/private.h > b/xen/arch/x86/guest/hyperv/private.h > index c1c2431eff..fcddc47544 100644 > --- a/xen/arch/x86/guest/hyperv/private.h > +++ b/xen/arch/x86/guest/hyperv/private.h > @@ -25,6 +25,7 @@ > #include <xen/percpu.h> > > DECLARE_PER_CPU(void *, hv_pcpu_input_page); > +DECLARE_PER_CPU(void *, hv_vp_assist); > DECLARE_PER_CPU(unsigned int, hv_vp_index); > > #endif /* __XEN_HYPERV_PRIVIATE_H__ */ > -- > 2.20.1
On Wed, Jan 29, 2020 at 08:20:34PM +0000, Wei Liu wrote: > VP assist page is rather important as we need to toggle some bits in it > for efficient nested virtualisation. > > Signed-off-by: Wei Liu <liuwe@microsoft.com> > --- > v5: > 1. Deal with error properly instead of always panicking > 2. Swap percpu variables declarations' location > > v4: > 1. Use private.h > 2. Prevent leak > > v3: > 1. Use xenheap page > 2. Drop set_vp_assist > > v2: > 1. Use HV_HYP_PAGE_SHIFT instead > --- > xen/arch/x86/guest/hyperv/hyperv.c | 44 ++++++++++++++++++++++++++++- > xen/arch/x86/guest/hyperv/private.h | 1 + > 2 files changed, 44 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/x86/guest/hyperv/hyperv.c b/xen/arch/x86/guest/hyperv/hyperv.c > index af0d6ed692..bc40a3d338 100644 > --- a/xen/arch/x86/guest/hyperv/hyperv.c > +++ b/xen/arch/x86/guest/hyperv/hyperv.c > @@ -31,6 +31,7 @@ > > struct ms_hyperv_info __read_mostly ms_hyperv; > DEFINE_PER_CPU_READ_MOSTLY(void *, hv_pcpu_input_page); > +DEFINE_PER_CPU_READ_MOSTLY(void *, hv_vp_assist); > DEFINE_PER_CPU_READ_MOSTLY(unsigned int, hv_vp_index); > > static uint64_t generate_guest_id(void) > @@ -155,16 +156,57 @@ static int setup_hypercall_pcpu_arg(void) > return 0; > } > > +static int setup_vp_assist(void) > +{ > + void *mapping; > + uint64_t val; > + > + mapping = this_cpu(hv_vp_assist); You could also avoid the usage of the local mapping variable here. > + > + if ( !mapping ) > + { > + mapping = alloc_xenheap_page(); > + if ( !mapping ) > + { > + printk("Failed to allocate vp_assist page for CPU%u\n", > + smp_processor_id()); > + return -ENOMEM; > + } > + > + clear_page(mapping); > + this_cpu(hv_vp_assist) = mapping; > + } > + > + val = (virt_to_mfn(mapping) << HV_HYP_PAGE_SHIFT) There's virt_to_maddr which would avoid the shift. > + | HV_X64_MSR_VP_ASSIST_PAGE_ENABLE; > + wrmsrl(HV_X64_MSR_VP_ASSIST_PAGE, val); > + > + return 0; > +} > + > static void __init setup(void) > { > setup_hypercall_page(); > + > if ( setup_hypercall_pcpu_arg() ) > panic("Hypercall percpu arg setup failed\n"); > + > + if ( setup_vp_assist() ) > + panic("VP assist page setup failed\n"); > } > > static int ap_setup(void) > { > - return setup_hypercall_pcpu_arg(); > + int rc; > + > + rc = setup_hypercall_pcpu_arg(); > + if ( rc ) > + goto out; No need for a label, as just returning here would make the function shorter: rc = setup_hypercall_pcpu_arg(); if ( rc ) return rc; return setup_vp_assist(); Thanks, Roger.
On Thu, Jan 30, 2020 at 10:34:29AM +0000, Durrant, Paul wrote: > > + > > + val = (virt_to_mfn(mapping) << HV_HYP_PAGE_SHIFT) > > + | HV_X64_MSR_VP_ASSIST_PAGE_ENABLE; > > Perhaps it would be neater to put the viridian_page_msr union into > hyperv-tlfs.h and then use that. Done. Now this series is one patch longer... Wei.
On Thu, Jan 30, 2020 at 01:42:29PM +0100, Roger Pau Monné wrote: > On Wed, Jan 29, 2020 at 08:20:34PM +0000, Wei Liu wrote: > > VP assist page is rather important as we need to toggle some bits in it > > for efficient nested virtualisation. > > > > Signed-off-by: Wei Liu <liuwe@microsoft.com> > > --- > > v5: > > 1. Deal with error properly instead of always panicking > > 2. Swap percpu variables declarations' location > > > > v4: > > 1. Use private.h > > 2. Prevent leak > > > > v3: > > 1. Use xenheap page > > 2. Drop set_vp_assist > > > > v2: > > 1. Use HV_HYP_PAGE_SHIFT instead > > --- > > xen/arch/x86/guest/hyperv/hyperv.c | 44 ++++++++++++++++++++++++++++- > > xen/arch/x86/guest/hyperv/private.h | 1 + > > 2 files changed, 44 insertions(+), 1 deletion(-) > > > > diff --git a/xen/arch/x86/guest/hyperv/hyperv.c b/xen/arch/x86/guest/hyperv/hyperv.c > > index af0d6ed692..bc40a3d338 100644 > > --- a/xen/arch/x86/guest/hyperv/hyperv.c > > +++ b/xen/arch/x86/guest/hyperv/hyperv.c > > @@ -31,6 +31,7 @@ > > > > struct ms_hyperv_info __read_mostly ms_hyperv; > > DEFINE_PER_CPU_READ_MOSTLY(void *, hv_pcpu_input_page); > > +DEFINE_PER_CPU_READ_MOSTLY(void *, hv_vp_assist); > > DEFINE_PER_CPU_READ_MOSTLY(unsigned int, hv_vp_index); > > > > static uint64_t generate_guest_id(void) > > @@ -155,16 +156,57 @@ static int setup_hypercall_pcpu_arg(void) > > return 0; > > } > > > > +static int setup_vp_assist(void) > > +{ > > + void *mapping; > > + uint64_t val; > > + > > + mapping = this_cpu(hv_vp_assist); > > You could also avoid the usage of the local mapping variable here. this_cpu(...) is longer than mapping. But since you ask for the change, I have made the change. > > > + > > + if ( !mapping ) > > + { > > + mapping = alloc_xenheap_page(); > > + if ( !mapping ) > > + { > > + printk("Failed to allocate vp_assist page for CPU%u\n", > > + smp_processor_id()); > > + return -ENOMEM; > > + } > > + > > + clear_page(mapping); > > + this_cpu(hv_vp_assist) = mapping; > > + } > > + > > + val = (virt_to_mfn(mapping) << HV_HYP_PAGE_SHIFT) > > There's virt_to_maddr which would avoid the shift. This line is gone. > > > + | HV_X64_MSR_VP_ASSIST_PAGE_ENABLE; > > + wrmsrl(HV_X64_MSR_VP_ASSIST_PAGE, val); > > + > > + return 0; > > +} > > + > > static void __init setup(void) > > { > > setup_hypercall_page(); > > + > > if ( setup_hypercall_pcpu_arg() ) > > panic("Hypercall percpu arg setup failed\n"); > > + > > + if ( setup_vp_assist() ) > > + panic("VP assist page setup failed\n"); > > } > > > > static int ap_setup(void) > > { > > - return setup_hypercall_pcpu_arg(); > > + int rc; > > + > > + rc = setup_hypercall_pcpu_arg(); > > + if ( rc ) > > + goto out; > > No need for a label, as just returning here would make the function > shorter: > > rc = setup_hypercall_pcpu_arg(); > if ( rc ) > return rc; > > return setup_vp_assist(); Done. Wei. > > Thanks, Roger.
diff --git a/xen/arch/x86/guest/hyperv/hyperv.c b/xen/arch/x86/guest/hyperv/hyperv.c index af0d6ed692..bc40a3d338 100644 --- a/xen/arch/x86/guest/hyperv/hyperv.c +++ b/xen/arch/x86/guest/hyperv/hyperv.c @@ -31,6 +31,7 @@ struct ms_hyperv_info __read_mostly ms_hyperv; DEFINE_PER_CPU_READ_MOSTLY(void *, hv_pcpu_input_page); +DEFINE_PER_CPU_READ_MOSTLY(void *, hv_vp_assist); DEFINE_PER_CPU_READ_MOSTLY(unsigned int, hv_vp_index); static uint64_t generate_guest_id(void) @@ -155,16 +156,57 @@ static int setup_hypercall_pcpu_arg(void) return 0; } +static int setup_vp_assist(void) +{ + void *mapping; + uint64_t val; + + mapping = this_cpu(hv_vp_assist); + + if ( !mapping ) + { + mapping = alloc_xenheap_page(); + if ( !mapping ) + { + printk("Failed to allocate vp_assist page for CPU%u\n", + smp_processor_id()); + return -ENOMEM; + } + + clear_page(mapping); + this_cpu(hv_vp_assist) = mapping; + } + + val = (virt_to_mfn(mapping) << HV_HYP_PAGE_SHIFT) + | HV_X64_MSR_VP_ASSIST_PAGE_ENABLE; + wrmsrl(HV_X64_MSR_VP_ASSIST_PAGE, val); + + return 0; +} + static void __init setup(void) { setup_hypercall_page(); + if ( setup_hypercall_pcpu_arg() ) panic("Hypercall percpu arg setup failed\n"); + + if ( setup_vp_assist() ) + panic("VP assist page setup failed\n"); } static int ap_setup(void) { - return setup_hypercall_pcpu_arg(); + int rc; + + rc = setup_hypercall_pcpu_arg(); + if ( rc ) + goto out; + + rc = setup_vp_assist(); + + out: + return rc; } static const struct hypervisor_ops ops = { diff --git a/xen/arch/x86/guest/hyperv/private.h b/xen/arch/x86/guest/hyperv/private.h index c1c2431eff..fcddc47544 100644 --- a/xen/arch/x86/guest/hyperv/private.h +++ b/xen/arch/x86/guest/hyperv/private.h @@ -25,6 +25,7 @@ #include <xen/percpu.h> DECLARE_PER_CPU(void *, hv_pcpu_input_page); +DECLARE_PER_CPU(void *, hv_vp_assist); DECLARE_PER_CPU(unsigned int, hv_vp_index); #endif /* __XEN_HYPERV_PRIVIATE_H__ */
VP assist page is rather important as we need to toggle some bits in it for efficient nested virtualisation. Signed-off-by: Wei Liu <liuwe@microsoft.com> --- v5: 1. Deal with error properly instead of always panicking 2. Swap percpu variables declarations' location v4: 1. Use private.h 2. Prevent leak v3: 1. Use xenheap page 2. Drop set_vp_assist v2: 1. Use HV_HYP_PAGE_SHIFT instead --- xen/arch/x86/guest/hyperv/hyperv.c | 44 ++++++++++++++++++++++++++++- xen/arch/x86/guest/hyperv/private.h | 1 + 2 files changed, 44 insertions(+), 1 deletion(-)