Re: [Xen-devel] [PATCH 5/8] viridian: separate interrupt related enlightenment implementations...

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Roger Pau Monne
> Sent: 30 October 2018 16:52
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Andrew Cooper
> <Andrew.Cooper3@xxxxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>; Jan Beulich
> <jbeulich@xxxxxxxx>
> Subject: Re: [Xen-devel] [PATCH 5/8] viridian: separate interrupt related
> enlightenment implementations...
> 
> On Mon, Oct 29, 2018 at 06:02:08PM +0000, Paul Durrant wrote:
> > ...into new 'synic' module.
> >
> > The SynIC (synthetic interrupt controller) is specified [1] to be a
> super-
> > set of a virtualized LAPIC, and its definition encompasses all
> > enlightenments related to virtual interrupt control.
> >
> > This patch reduces the size of the main viridian source module by giving
> > these enlightenments their own module. This is done in anticipation of
> > implementation of more such enlightenments and a desire not to further
> > lengthen then main source module when this work is done.
> >
> > Whilst moving the code:
> >
> > - Fix various style issues.
> > - Move the MSR definitions into the header (since they are now needed in
> >   more than one source module).
> >
> > [1] https://github.com/MicrosoftDocs/Virtualization-
> Documentation/raw/live/tlfs/Hypervisor%20Top%20Level%20Functional%20Specif
> ication%20v5.0C.pdf
> >
> > Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> > ---
> > Cc: Jan Beulich <jbeulich@xxxxxxxx>
> > Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> > ---
> >  xen/arch/x86/hvm/viridian/Makefile   |   1 +
> >  xen/arch/x86/hvm/viridian/synic.c    | 224
> ++++++++++++++++++++++++++++++++++
> >  xen/arch/x86/hvm/viridian/viridian.c | 229 ++--------------------------
> -------
> >  xen/include/asm-x86/hvm/viridian.h   |  76 ++++++++++++
> >  4 files changed, 311 insertions(+), 219 deletions(-)
> >  create mode 100644 xen/arch/x86/hvm/viridian/synic.c
> >
> > diff --git a/xen/arch/x86/hvm/viridian/Makefile
> b/xen/arch/x86/hvm/viridian/Makefile
> > index 09fd0a5f3c..fca8e16e20 100644
> > --- a/xen/arch/x86/hvm/viridian/Makefile
> > +++ b/xen/arch/x86/hvm/viridian/Makefile
> > @@ -1 +1,2 @@
> > +obj-y += synic.o
> >  obj-y += viridian.o
> > diff --git a/xen/arch/x86/hvm/viridian/synic.c
> b/xen/arch/x86/hvm/viridian/synic.c
> > new file mode 100644
> > index 0000000000..3f043f34ee
> > --- /dev/null
> > +++ b/xen/arch/x86/hvm/viridian/synic.c
> > @@ -0,0 +1,224 @@
> >
> +/************************************************************************
> ***
> > + * synic.c
> > + *
> > + * An implementation of some interrupt related Viridian enlightenments.
> > + * See Microsoft's Hypervisor Top Level Functional Specification.
> > + * for more information.
> > + */
> > +
> > +#include <xen/sched.h>
> > +#include <xen/version.h>
> > +#include <xen/hypercall.h>
> > +#include <xen/domain_page.h>
> 
> Could you sort the above alphabetically?
> 
> And I usually add a newline between xen/ and asm/ includes, but I
> don't think that's coding style.
> 

Sure. It'll look better.

> > @@ -989,14 +786,13 @@ HVM_REGISTER_SAVE_RESTORE(VIRIDIAN_DOMAIN,
> viridian_save_domain_ctxt,
> >
> >  static int viridian_save_vcpu_ctxt(struct vcpu *v, hvm_domain_context_t
> *h)
> >  {
> > -    struct hvm_viridian_vcpu_context ctxt = {
> > -        .vp_assist_msr = v->arch.hvm.viridian.vp_assist.msr.raw,
> > -        .vp_assist_pending = v->arch.hvm.viridian.vp_assist.pending,
> > -    };
> > +    struct hvm_viridian_vcpu_context ctxt = {};
> >
> >      if ( !is_viridian_domain(v->domain) )
> >          return 0;
> >
> > +    viridian_synic_save_vcpu_ctxt(v, &ctxt);
> > +
> >      return hvm_save_entry(VIRIDIAN_VCPU, v->vcpu_id, h, &ctxt);
> >  }
> >
> > @@ -1020,12 +816,7 @@ static int viridian_load_vcpu_ctxt(struct domain
> *d,
> >      if ( memcmp(&ctxt._pad, zero_page, sizeof(ctxt._pad)) )
> >          return -EINVAL;
> >
> > -    v->arch.hvm.viridian.vp_assist.msr.raw = ctxt.vp_assist_msr;
> > -    if ( v->arch.hvm.viridian.vp_assist.msr.fields.enabled &&
> > -         !v->arch.hvm.viridian.vp_assist.va )
> > -        initialize_vp_assist(v);
> > -
> > -    v->arch.hvm.viridian.vp_assist.pending = !!ctxt.vp_assist_pending;
> > +    viridian_synic_load_vcpu_ctxt(v, &ctxt);
> 
> Seeing that now viridian_{load/save}_vcpu_ctxt is mostly a wrapper
> around viridian_synic_{save/load}_vcpu_ctxt, is there any reason to
> not remove those and declare the save/restore handling of the vidirian
> vcpu in the new synic file?
> 
> Is there more stuff not synic related coming up that needs
> save/restore?
> 

There is per-domain time related save/restore, but synic stuff is the only 
per-cpu data. I'd prefer to keep the wrapper though as there may be other 
non-synic related data... but I don't have any in the pipeline at the moment.

  Paul

> Thanks, Roger.

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