Re: [Xen-devel] [PATCH v5 12/12] x86/hyperv: setup VP assist page

[Wei Liu <wl@xen.org>]

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.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel