On Mon, 17 Apr 2017 14:22:20 -0600
Jim Fehlig <jfehlig@xxxxxxxx> wrote:
> On 03/24/2017 03:02 PM, Wim Ten Have wrote:
> > From: Wim ten Have <wim.ten.have@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > Signed-off-by: Wim ten Have <wim.ten.have@xxxxxxxxxx>
> > ---
> > 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 &&
> >
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
