Re: [Xen-devel] [PATCH 1/8] viridian: add init hooks

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Paul Durrant
> Sent: 03 January 2019 10:38
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; Andrew Cooper
> <Andrew.Cooper3@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Wei Liu <wei.liu2@xxxxxxxxxx>; Jan Beulich <jbeulich@xxxxxxxx>; Roger
> Pau Monne <roger.pau@xxxxxxxxxx>
> Subject: RE: [Xen-devel] [PATCH 1/8] viridian: add init hooks
> 
> > -----Original Message-----
> > From: Xen-devel [mailto:xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx] On
> Behalf
> > Of Paul Durrant
> > Sent: 03 January 2019 09:08
> > To: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; xen-
> > devel@xxxxxxxxxxxxxxxxxxxx
> > Cc: Wei Liu <wei.liu2@xxxxxxxxxx>; Jan Beulich <jbeulich@xxxxxxxx>;
> Roger
> > Pau Monne <roger.pau@xxxxxxxxxx>
> > Subject: Re: [Xen-devel] [PATCH 1/8] viridian: add init hooks
> >
> > > -----Original Message-----
> > > From: Andrew Cooper
> > > Sent: 02 January 2019 17:37
> > > To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; xen-
> > devel@xxxxxxxxxxxxxxxxxxxx
> > > Cc: Wei Liu <wei.liu2@xxxxxxxxxx>; Jan Beulich <jbeulich@xxxxxxxx>;
> > Roger
> > > Pau Monne <roger.pau@xxxxxxxxxx>
> > > Subject: Re: [Xen-devel] [PATCH 1/8] viridian: add init hooks
> > >
> > > On 02/01/2019 16:08, Andrew Cooper wrote:
> > > > On 20/12/2018 16:33, Paul Durrant wrote:
> > > >> This patch adds domain and vcpu init hooks for viridian features.
> The
> > > init
> > > >> hooks do not yet do anything; they will be added to by subsequent
> > > patches.
> > > >>
> > > >> Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> > > > Please can we start by fixing the current, broken, initialisation
> and
> > > > destruction logic ?
> > > >
> > > > To get rid of DOMCTL_setmaxvcpus, we need the *_destroy() logic to
> be
> > > > fully idempotent.  Also, viridian_domain_deinit() should not call
> into
> > > > viridian_vcpu_deinit(), and viridian_vcpu_deinit() shouldn't be
> faking
> > > > up a write to the assist page.
> > > >
> > > > AFAICT, the deinit path is all entirely pointless at the moment and
> > can
> > > > be deleted.
> > >
> > > Given that we are now going to be allocating non-trivial structures
> for
> > > viridian domains, I'd like to float the idea of changing viridian
> > > initialisation to be a domaincreate flag, rather than blindly assuming
> > > that all HVM guests want it.
> >
> > That would certainly be cleaner but sounds non-trivial.
> 
> Looking at this a little more... Viridian is an x86 specific thing, so an
> extra flag in xen_arch_domainconfig would seem most appropriate. This
> would then need to wired into the appropriate place(s) in libxl. I'll have
> a look at how much work this is likely to be.

Would something along the lines of the following (as yet untested and 
incomplete) patch be acceptable?

(I've blindly coded the Ocaml $MAGIC and just hardcoded the flag in the stub)

---8<---
diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
index c0f88a7eaa..17b1e9bf2a 100644
--- a/tools/libxl/libxl_x86.c
+++ b/tools/libxl/libxl_x86.c
@@ -7,9 +7,15 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
                                       libxl_domain_config *d_config,
                                       struct xen_domctl_createdomain *config)
 {
+    bool viridian;
+
+    viridian = libxl_defbool_val(d_config->b_info.u.hvm.viridian);
+
     switch(d_config->c_info.type) {
     case LIBXL_DOMAIN_TYPE_HVM:
         config->arch.emulation_flags = (XEN_X86_EMU_ALL & ~XEN_X86_EMU_VPCI);
+        if (viridian)
+            config->arch.feature_flags = XEN_X86_FEAT_VIRIDIAN;
         break;
     case LIBXL_DOMAIN_TYPE_PVH:
         config->arch.emulation_flags = XEN_X86_EMU_LAPIC;
diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
index a57130a3c3..b8199776a7 100644
--- a/tools/ocaml/libs/xc/xenctrl.ml
+++ b/tools/ocaml/libs/xc/xenctrl.ml
@@ -47,9 +47,13 @@ type x86_arch_emulation_flags =
        | X86_EMU_PIT
        | X86_EMU_USE_PIRQ

+type x86_arch_feature_flags =
+       | X86_FEAT_VIRIDIAN
+
 type xen_x86_arch_domainconfig =
 {
        emulation_flags: x86_arch_emulation_flags list;
+       feature_flags: x86_arch_feature_flags list;
 }

 type arch_domainconfig =
diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli
index 476bbecb90..a851fd115d 100644
--- a/tools/ocaml/libs/xc/xenctrl.mli
+++ b/tools/ocaml/libs/xc/xenctrl.mli
@@ -41,8 +41,12 @@ type x86_arch_emulation_flags =
   | X86_EMU_PIT
   | X86_EMU_USE_PIRQ

+type x86_arch_feature_flags =
+  | X86_FEAT_VIRIDIAN
+
 type xen_x86_arch_domainconfig = {
   emulation_flags: x86_arch_emulation_flags list;
+  feature_flags: x86_arch_feature_flags list;
 }

 type arch_domainconfig =
diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c 
b/tools/ocaml/libs/xc/xenctrl_stubs.c
index c4fdc58b2d..a5ef3bb160 100644
--- a/tools/ocaml/libs/xc/xenctrl_stubs.c
+++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
@@ -166,7 +166,7 @@ CAMLprim value stub_xc_domain_create(value xch, value 
config)
                        cfg.arch.emulation_flags |= 1u << Int_val(Field(l, 0));

 #undef VAL_EMUL_FLAGS
-
+               cfg.arch.feature_flags = XEN_X86_FEAT_VIRIDIAN;
 #else
                caml_failwith("Unhandled: x86");
 #endif
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index ed2a57a8a6..c820cf334d 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -586,7 +586,10 @@ int arch_domain_create(struct domain *d,

     if ( is_hvm_domain(d) )
     {
-        if ( (rc = hvm_domain_initialise(d)) != 0 )
+        bool enable_viridian =
+            config->arch.feature_flags & XEN_X86_FEAT_VIRIDIAN;
+
+        if ( (rc = hvm_domain_initialise(d, enable_viridian)) != 0 )
             goto fail;
     }
     else if ( is_pv_domain(d) )
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 6a1f18d8b5..351994e905 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -575,7 +575,7 @@ static int hvm_print_line(
     return X86EMUL_OKAY;
 }

-int hvm_domain_initialise(struct domain *d)
+int hvm_domain_initialise(struct domain *d, bool enable_viridian)
 {
     unsigned int nr_gsis;
     int rc;
diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index 7892f98c7b..6bde613dd8 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -241,7 +241,7 @@ extern s8 hvm_port80_allowed;
 extern const struct hvm_function_table *start_svm(void);
 extern const struct hvm_function_table *start_vmx(void);

-int hvm_domain_initialise(struct domain *d);
+int hvm_domain_initialise(struct domain *d, bool enable_viridian);
 void hvm_domain_relinquish_resources(struct domain *d);
 void hvm_domain_destroy(struct domain *d);
 void hvm_domain_soft_reset(struct domain *d);
diff --git a/xen/include/public/arch-x86/xen.h 
b/xen/include/public/arch-x86/xen.h
index 629cb2ba40..467aa8bbf0 100644
--- a/xen/include/public/arch-x86/xen.h
+++ b/xen/include/public/arch-x86/xen.h
@@ -304,6 +304,10 @@ struct xen_arch_domainconfig {
                                      XEN_X86_EMU_PIT | XEN_X86_EMU_USE_PIRQ |\
                                      XEN_X86_EMU_VPCI)
     uint32_t emulation_flags;
+
+#define _XEN_X86_FEAT_VIRIDIAN      0
+#define XEN_X86_FEAT_VIRIDIAN       (1U<<_XEN_X86_FEAT_VIRIDIAN)
+    uint32_t feature_flags;
 };

 /* Location of online VCPU bitmap. */
---8<---

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