Re: [Xen-devel] [PATCH v3 2/2] x86/hvm/viridian: Enable APIC assist enlightenment

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Wei Liu [mailto:wei.liu2@xxxxxxxxxx]
> Sent: 16 March 2016 18:31
> To: Paul Durrant
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Ian Jackson; Stefano Stabellini; Wei Liu;
> Keir (Xen.org); Jan Beulich; Andrew Cooper
> Subject: Re: [PATCH v3 2/2] x86/hvm/viridian: Enable APIC assist
> enlightenment
> 
> On Wed, Mar 16, 2016 at 05:44:12PM +0000, Paul Durrant wrote:
> > This patch adds code to enable the APIC assist enlightenment which,
> > under certain conditions, means that the guest can avoid an EOI of
> > the local APIC and thereby avoid a VMEXIT.
> >
> > Use of the enlightenment by the hypervisor is under control of the
> > toolstack, and is added to the default set.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> > Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> > Cc: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> > Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> > Cc: Keir Fraser <keir@xxxxxxx>
> > Cc: Jan Beulich <jbeulich@xxxxxxxx>
> > Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > ---
> >
> > v3:
> >  - Re-instated read-modify-write for forwards compatibility
> >  - Fix a coding style issue
> >
> > v2:
> >  - Removed some code duplication and unnecessary read-modify-write
> >    operations on the APIC assist word.
> >  - Stated in the xl.cfg text that the enlightenment has no effect if
> >    posted interrupts are in use.
> >  - Added the enlightenment to the default set.
> > ---
> >  docs/man/xl.cfg.pod.5              | 13 ++++++++-
> >  tools/libxl/libxl_dom.c            |  4 +++
> >  tools/libxl/libxl_types.idl        |  1 +
> >  xen/arch/x86/hvm/viridian.c        | 59
> ++++++++++++++++++++++++++++++++++----
> >  xen/arch/x86/hvm/vlapic.c          | 58
> +++++++++++++++++++++++++++++++++----
> >  xen/include/asm-x86/hvm/viridian.h |  5 ++++
> >  xen/include/public/hvm/params.h    |  7 ++++-
> >  7 files changed, 134 insertions(+), 13 deletions(-)
> >
> > diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> > index 56b1117..1ba026b 100644
> > --- a/docs/man/xl.cfg.pod.5
> > +++ b/docs/man/xl.cfg.pod.5
> > @@ -1484,10 +1484,21 @@ This set incorporates use of hypercalls for
> remote TLB flushing.
> >  This enlightenment may improve performance of Windows guests running
> >  on hosts with higher levels of (physical) CPU contention.
> >
> > +=item B<apic_assist>
> > +
> > +This set incorporates use of the APIC assist page to avoid EOI of
> > +the local APIC.
> > +This enlightenment may improve performance of Windows guests,
> > +particularly those running PV drivers that make use of per-vcpu
> > +event channel upcall vectors.
> 
> I think this can be changed to
> 
>   This enlightenment may improve performance of guests that make use of
>   per-vcpu event channel upcall vectors.
> 
> Surely Linux can benefit from this too, right?
> 

Potentially Linux can benefit, yes. I'll re-word as you suggest.

> > +Note that this enlightenment will have no effect if the guest is
> > +using APICv posted interrupts.
> > +
> >  =item B<defaults>
> >
> >  This is a special value that enables the default set of groups, which
> > -is currently the B<base>, B<freq> and B<time_ref_count> groups.
> > +is currently the B<base>, B<freq>, B<time_ref_count> and B<apic_assist>
> > +groups.
> >
> >  =item B<all>
> >
> > diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> > index b825b98..09d3bca 100644
> > --- a/tools/libxl/libxl_dom.c
> > +++ b/tools/libxl/libxl_dom.c
> > @@ -211,6 +211,7 @@ static int hvm_set_viridian_features(libxl__gc *gc,
> uint32_t domid,
> >          libxl_bitmap_set(&enlightenments,
> LIBXL_VIRIDIAN_ENLIGHTENMENT_BASE);
> >          libxl_bitmap_set(&enlightenments,
> LIBXL_VIRIDIAN_ENLIGHTENMENT_FREQ);
> >          libxl_bitmap_set(&enlightenments,
> LIBXL_VIRIDIAN_ENLIGHTENMENT_TIME_REF_COUNT);
> > +        libxl_bitmap_set(&enlightenments,
> LIBXL_VIRIDIAN_ENLIGHTENMENT_APIC_ASSIST);
> >      }
> >
> >      libxl_for_each_set_bit(v, info->u.hvm.viridian_enable) {
> > @@ -253,6 +254,9 @@ static int hvm_set_viridian_features(libxl__gc *gc,
> uint32_t domid,
> >      if (libxl_bitmap_test(&enlightenments,
> LIBXL_VIRIDIAN_ENLIGHTENMENT_HCALL_REMOTE_TLB_FLUSH))
> >          mask |= HVMPV_hcall_remote_tlb_flush;
> >
> > +    if (libxl_bitmap_test(&enlightenments,
> LIBXL_VIRIDIAN_ENLIGHTENMENT_APIC_ASSIST))
> > +        mask |= HVMPV_apic_assist;
> > +
> >      if (mask != 0 &&
> >          xc_hvm_param_set(CTX->xch,
> >                           domid,
> > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> > index 632c009..e3be957 100644
> > --- a/tools/libxl/libxl_types.idl
> > +++ b/tools/libxl/libxl_types.idl
> > @@ -221,6 +221,7 @@ libxl_viridian_enlightenment =
> Enumeration("viridian_enlightenment", [
> >      (2, "time_ref_count"),
> >      (3, "reference_tsc"),
> >      (4, "hcall_remote_tlb_flush"),
> > +    (5, "apic_assist"),
> >      ])
> >
> 
> 
> Missing a LIBXL_HAVE macro in libxl.h for this new field.
> 

Damn. That always catches me out.

  Paul

> Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel