Re: [Xen-devel] [PATCH 6/8] viridian: add implementation of synthetic interrupt MSRs

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Wei Liu [mailto:wei.liu2@xxxxxxxxxx]
> Sent: 02 January 2019 15:56
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Ian Jackson <Ian.Jackson@xxxxxxxxxx>;
> Wei Liu <wei.liu2@xxxxxxxxxx>; Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>;
> George Dunlap <George.Dunlap@xxxxxxxxxx>; Jan Beulich <jbeulich@xxxxxxxx>;
> Julien Grall <julien.grall@xxxxxxx>; Konrad Rzeszutek Wilk
> <konrad.wilk@xxxxxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; Tim
> (Xen.org) <tim@xxxxxxx>; Roger Pau Monne <roger.pau@xxxxxxxxxx>
> Subject: Re: [PATCH 6/8] viridian: add implementation of synthetic
> interrupt MSRs
> 
> On Thu, Dec 20, 2018 at 04:33:43PM +0000, Paul Durrant wrote:
> > This patch introduces an implementation of the SCONTROL, SVERSION,
> SIEFP,
> > SIMP, EOM and SINT0-15 SynIC MSRs. No message source is added and, as
> such,
> > nothing will yet generate a synthetic interrupt. A subsequent patch will
> > add an implementation of synthetic timers which will need the
> infrastructure
> > added by this patch to deliver expiry messages to the guest.
> >
> > NOTE: A 'synic' option is added to the toolstack viridian enlightenments
> >       enumeration but is deliberately not documented as enabling these
> >       SynIC registers without a message source is only useful for
> >       debugging.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> > ---
> > Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> > Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> > Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > Cc: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
> > Cc: Jan Beulich <jbeulich@xxxxxxxx>
> > Cc: Julien Grall <julien.grall@xxxxxxx>
> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> > Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > Cc: Tim Deegan <tim@xxxxxxx>
> > Cc: "Roger Pau Monné" <roger.pau@xxxxxxxxxx>
> > ---
> >  tools/libxl/libxl.h                    |   6 +
> >  tools/libxl/libxl_dom.c                |   3 +
> >  tools/libxl/libxl_types.idl            |   1 +
> 
> It seems that xl changes are missing?

No xl changes are needed thanks to the magic that is libxl enum parsing :-)

> 
> >  xen/arch/x86/hvm/viridian/synic.c      | 215 +++++++++++++++++++++++++
> >  xen/arch/x86/hvm/viridian/viridian.c   |  16 ++
> >  xen/arch/x86/hvm/vlapic.c              |  16 +-
> >  xen/include/asm-x86/hvm/hvm.h          |   3 +
> >  xen/include/asm-x86/hvm/viridian.h     |  24 +++
> >  xen/include/public/arch-x86/hvm/save.h |   2 +
> >  xen/include/public/hvm/params.h        |   7 +-
> >  10 files changed, 291 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> > index a38e5cdba2..a923a380d3 100644
> > --- a/tools/libxl/libxl.h
> > +++ b/tools/libxl/libxl.h
> > @@ -318,6 +318,12 @@
> >   */
> >  #define LIBXL_HAVE_VIRIDIAN_CRASH_CTL 1
> >
> > +/*
> > + * LIBXL_HAVE_VIRIDIAN_SYNIC indicates that the 'synic' value
> > + * is present in the viridian enlightenment enumeration.
> > + */
> > +#define LIBXL_HAVE_VIRIDIAN_SYNIC 1
> > +
> >  /*
> >   * LIBXL_HAVE_BUILDINFO_HVM_ACPI_LAPTOP_SLATE indicates that
> >   * libxl_domain_build_info has the u.hvm.acpi_laptop_slate field.
> > diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> > index 6160991af3..fb758d2ac3 100644
> > --- a/tools/libxl/libxl_dom.c
> > +++ b/tools/libxl/libxl_dom.c
> > @@ -317,6 +317,9 @@ static int hvm_set_viridian_features(libxl__gc *gc,
> uint32_t domid,
> >      if (libxl_bitmap_test(&enlightenments,
> LIBXL_VIRIDIAN_ENLIGHTENMENT_CRASH_CTL))
> >          mask |= HVMPV_crash_ctl;
> >
> > +    if (libxl_bitmap_test(&enlightenments,
> LIBXL_VIRIDIAN_ENLIGHTENMENT_SYNIC))
> > +        mask |= HVMPV_synic;
> > +
> >      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 51cf06a3a2..ee5eed2945 100644
> > --- a/tools/libxl/libxl_types.idl
> > +++ b/tools/libxl/libxl_types.idl
> > @@ -228,6 +228,7 @@ libxl_viridian_enlightenment =
> Enumeration("viridian_enlightenment", [
> >      (4, "hcall_remote_tlb_flush"),
> >      (5, "apic_assist"),
> >      (6, "crash_ctl"),
> > +    (7, "synic"),
> >      ])
> >
> >  libxl_hdtype = Enumeration("hdtype", [
> > diff --git a/xen/arch/x86/hvm/viridian/synic.c
> b/xen/arch/x86/hvm/viridian/synic.c
> > index 35bd2125fc..8a819c8161 100644
> > --- a/xen/arch/x86/hvm/viridian/synic.c
> > +++ b/xen/arch/x86/hvm/viridian/synic.c
> > @@ -8,11 +8,13 @@
> >
> >  #include <xen/domain_page.h>
> >  #include <xen/hypercall.h>
> > +#include <xen/nospec.h>
> 
> This header is included by array_index_nospec is not used anywhere.

Ah yes, I originally thought I needed it but then changed my mind.

> 
> I'm afraid I will need to shelve reviewing the rest for a while because
> I haven't read the viridian spec.

Sections 10, 11 and 12 are the ones relevant to this series. The spec. is not 
exactly a great read though :-(

  Paul

> 
> Wei.

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