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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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%20Specification%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.

> @@ -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?

Thanks, Roger.

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