Re: [Xen-devel] [PATCH v5 12/12] x86/hyperv: setup VP assist page
On Thu, Jan 30, 2020 at 01:42:29PM +0100, Roger Pau Monné wrote:
> On Wed, Jan 29, 2020 at 08:20:34PM +, 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
> > ---
> > 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.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v5 12/12] x86/hyperv: setup VP assist page
On Thu, Jan 30, 2020 at 10:34:29AM +, 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. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v5 12/12] x86/hyperv: setup VP assist page
On Wed, Jan 29, 2020 at 08:20:34PM +, 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
> ---
> 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.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v5 12/12] x86/hyperv: setup VP assist page
> -Original Message-
> From: Wei Liu On Behalf Of Wei Liu
> Sent: 29 January 2020 20:21
> To: Xen Development List
> Cc: Durrant, Paul ; Michael Kelley
> ; Wei Liu ; Wei Liu
> ; Jan Beulich ; Andrew Cooper
> ; Roger Pau Monné
> 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
> ---
> 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
>
> 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
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
