[libvirt] [PATCH v2 2/3] xenconfig: add conversions for xen-xl

[Wim ten Have]

On Mon, 17 Apr 2017 14:22:20 -0600
Jim Fehlig <jfehlig at suse.com> wrote:

> On 03/24/2017 03:02 PM, Wim Ten Have wrote:
> > From: Wim ten Have <wim.ten.have at oracle.com>
> >
> > Per xen-xl conversions from and to native under host-passthrough
> > mode we take care for Xen (nestedhvm = mode) applied and inherited
> > settings generating or processing correct feature policy:
> >
> > [On Intel (VT-x) architectures]
> > <feature policy='disable' name='vmx'/>
> >
> > or
> >
> > [On AMD (AMD-V) architectures]
> > <feature policy='disable' name='svm'/>
> >
> > It will then generate (or parse) for nestedhvm=1 in/from xl format.
> >
> > Signed-off-by: Joao Martins <joao.m.martins at oracle.com>
> > Signed-off-by: Wim ten Have <wim.ten.have at oracle.com>
> > ---
> >  src/xenconfig/xen_xl.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 67 insertions(+)
> >
> > diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c
> > index 74f68b3..d3797b8 100644
> > --- a/src/xenconfig/xen_xl.c
> > +++ b/src/xenconfig/xen_xl.c
> > @@ -106,6 +106,7 @@ xenParseXLOS(virConfPtr conf, virDomainDefPtr def, virCapsPtr caps)
> >      if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) {
> >          const char *bios;
> >          const char *boot;
> > +        int val = 0;
> >
> >          if (xenConfigGetString(conf, "bios", &bios, NULL) < 0)
> >              return -1;
> > @@ -164,6 +165,47 @@ xenParseXLOS(virConfPtr conf, virDomainDefPtr def, virCapsPtr caps)
> >              }
> >              def->os.nBootDevs++;
> >          }
> > +
> > +        if (xenConfigGetBool(conf, "nestedhvm", &val, -1) < 0)
> > +            return -1;
> > +
> > +        if (val != -1) {
> > +            virCPUDefPtr cpu = NULL;
> > +
> > +            if (VIR_ALLOC(cpu) < 0)
> > +                return -1;
> > +
> > +            if (val == 0) {
> > +                if (VIR_ALLOC_N(cpu->features, 2) < 0)
> > +                    goto cleanup;
> > +
> > +                /*
> > +                 * Below is pointless in real world but for purpose
> > +                 * of testing let's check features depth holding at
> > +                 * least multiple elements and also check if both
> > +                 * vmx|svm are understood.
> > +                 */
> > +                cpu->features[0].policy = VIR_CPU_FEATURE_DISABLE;
> > +                if (VIR_STRDUP(cpu->features[0].name, "vmx") < 0)
> > +                    goto cleanup;
> > +                cpu->features[1].policy = VIR_CPU_FEATURE_DISABLE;
> > +                if (VIR_STRDUP(cpu->features[1].name, "svm") < 0)
> > +                    goto cleanup;  
> 
> Since caps is passed to this function, can it be used to determine whether to 
> disable vmx or svm?

  Yes :-) ... thanks for enlightening me here as that is actually the correct
  approach if non-test domain (conversion) actions are made under libvirtd.

  There's one minor question here per me.  To process caps for vmx|svm I'll
  bring in virCPUCheckFeature() which at its turn requires me to include "cpu/cpu.h"
  for its prototype.  Unfortunate cfg.mk does not anticipate and raises a
  syntax-check caveat.

	prohibit_cross_inclusion
	src/xenconfig/xen_xl.c:51:#include "cpu/cpu.h"
	maint.mk: unsafe cross-directory include
	cfg.mk:773: recipe for target 'sc_prohibit_cross_inclusion' failed
	make: *** [sc_prohibit_cross_inclusion] Error 1

  That is ... unless I apply below change to cfg.mk.

-           xenapi/ | xenconfig/ ) safe="($$dir|util|conf|xen)";;       \
+           xenapi/ | xenconfig/ ) safe="($$dir|util|conf|xen|cpu)";;   \

  Is that lethal to do?  I tried to reason and fail to see why not, ie. i
  am a bit clueless for its specific reason ... but also new to whole arena.

> > +
> > +                cpu->nfeatures = cpu->nfeatures_max = 2;
> > +            }
> > +            cpu->mode = VIR_CPU_MODE_HOST_PASSTHROUGH;
> > +            cpu->type = VIR_CPU_TYPE_GUEST;
> > +            def->cpu = cpu;
> > +            cpu = NULL;
> > + cleanup:
> > +            if (cpu) {
> > +                VIR_FREE(cpu->features);
> > +                VIR_FREE(cpu);
> > +                return -1;
> > +            }  
> 
> I'm not fond of the cleanup label in the middle of this function. If we can 
> disable only one of vmx or svm, then there will be one less strdup and one less 
> cleanup spot. Cleanup can then occur at the point of error.

  Correct and given former change this will now nicely fold down under its
  eventual failing VIR_STRDUP().

> > +        }
> > +
> >      } else {
> >          if (xenConfigCopyStringOpt(conf, "bootloader", &def->os.bootloader) < 0)
> >              return -1;
> > @@ -897,6 +939,31 @@ xenFormatXLOS(virConfPtr conf, virDomainDefPtr def)
> >          if (xenConfigSetString(conf, "boot", boot) < 0)
> >              return -1;
> >
> > +        if (def->cpu &&
> > +            def->cpu->mode == VIR_CPU_MODE_HOST_PASSTHROUGH) {
> > +            bool hasHwVirt = true;
> > +
> > +            if (def->cpu->nfeatures) {
> > +                for (i = 0; i < def->cpu->nfeatures; i++) {
> > +
> > +                    switch (def->cpu->features[i].policy) {
> > +                        case VIR_CPU_FEATURE_DISABLE:
> > +                        case VIR_CPU_FEATURE_FORBID:
> > +                            if (STREQ(def->cpu->features[i].name, "vmx") ||
> > +                                STREQ(def->cpu->features[i].name, "svm"))
> > +                                hasHwVirt = false;
> > +                            break;
> > +
> > +                        default:
> > +                            break;  
> 
> Similar to patch 1, replace 'default' with the other enum values.

  Sure!

Regards,
- Wim.
 
> > +                    }
> > +                }
> > +            }
> > +
> > +            if (xenConfigSetInt(conf, "nestedhvm", hasHwVirt) < 0)
> > +                return -1;
> > +        }
> > +
> >          /* XXX floppy disks */
> >      } else {
> >          if (def->os.bootloader &&
> >  
>