[Sorry for double posting, but I mistakenly forgot to include libvirt list) +WimT +Daniel On 12/10/2017 02:10 AM, Marek Marczykowski-Górecki wrote: > <cpu mode='host-passthrough'> element may be used to configure other > features, like NUMA, or CPUID. Do not enable nested HVM (which is in > "preview" state after all) by mere presence of > <cpu mode='host-passthrough'> element, but require explicit <feature > policy='force' name='vmx'/> (or 'svm'). > Also, adjust xenconfig driver to appropriately translate to/from > nestedhvm=1. > > While at it, adjust xenconfig driver to not override def->cpu if already > set elsewhere. This will help with adding cpuid support. > I agree with this and it was what we came up in the first version of nested hvm support[0]. Although Daniel suggested there to use the same semantics of qemu driver such that host-passthrough enables nested hvm without the use of: <feature policy='require' name='vmx'/> (I think you propose policy='force' here which is probably better suited as opposed to policy='require') [0] https://www.redhat.com/archives/libvir-list/2017-March/msg00330.html Some small comments most of them nitpicks that may fall in individuals style preference. > --- > Changes since v2: > - new patch > --- > src/libxl/libxl_conf.c | 10 ++- > src/xenconfig/xen_xl.c | 57 +++++++++---------- > tests/libxlxml2domconfigdata/vnuma-hvm.json | 1 +- > tests/xlconfigdata/test-fullvirt-nestedhvm.xml | 4 +- > 4 files changed, 40 insertions(+), 32 deletions(-) > > diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c > index f39e783..1846109 100644 > --- a/src/libxl/libxl_conf.c > +++ b/src/libxl/libxl_conf.c > @@ -355,6 +355,7 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, > > if (caps && def->cpu) { > bool hasHwVirt = false; > + int nested_hvm = -1; Why not assume nested_hvm set to 0 (replicating libxl behaviour of default disabled) and then [*] > bool svm = false, vmx = false; > > if (def->cpu->mode != (VIR_CPU_MODE_HOST_PASSTHROUGH)) { > @@ -379,18 +380,23 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, > case VIR_CPU_FEATURE_FORBID: > if ((vmx && STREQ(def->cpu->features[i].name, "vmx")) || > (svm && STREQ(def->cpu->features[i].name, "svm"))) > - hasHwVirt = false; You remove this, but it might be needed given that right before this chunk we set hasHwVirt to true depending on whether host cpu supports it. > + nested_hvm = 0; > break; > [*] Just removing the nested_hvm being set to 0 here ... > case VIR_CPU_FEATURE_FORCE: > case VIR_CPU_FEATURE_REQUIRE: > + if ((vmx && STREQ(def->cpu->features[i].name, "vmx")) || > + (svm && STREQ(def->cpu->features[i].name, "svm"))) > + nested_hvm = 1;> + break; ... and instead simply have this one here. > case VIR_CPU_FEATURE_OPTIONAL: > case VIR_CPU_FEATURE_LAST: > break; > } > } > } > - libxl_defbool_set(&b_info->u.hvm.nested_hvm, hasHwVirt); > + if (hasHwVirt && nested_hvm != -1) > + libxl_defbool_set(&b_info->u.hvm.nested_hvm, nested_hvm); An alternative would be the as to always set like previously done: libxl_defbool_set(&b_info->u.hvm.nested_hvm, hasHwVirt && nestedHvm); And thus not needing the if that you are preceding here. > } > > if (def->nsounds > 0) { > diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c > index 9e239a7..317c7a0 100644 > --- a/src/xenconfig/xen_xl.c > +++ b/src/xenconfig/xen_xl.c > @@ -170,16 +170,7 @@ xenParseXLOS(virConfPtr conf, virDomainDefPtr def, virCapsPtr caps) > if (xenConfigGetBool(conf, "nestedhvm", &val, -1) < 0) > return -1; > > - if (val == 1) { > - virCPUDefPtr cpu; > - > - if (VIR_ALLOC(cpu) < 0) > - return -1; > - > - cpu->mode = VIR_CPU_MODE_HOST_PASSTHROUGH; > - cpu->type = VIR_CPU_TYPE_GUEST; > - def->cpu = cpu; > - } else if (val == 0) { > + if (val != -1) { > const char *vtfeature = NULL; > > if (caps && caps->host.cpu && ARCH_IS_X86(def->os.arch)) { > @@ -190,26 +181,29 @@ xenParseXLOS(virConfPtr conf, virDomainDefPtr def, virCapsPtr caps) > } > > if (vtfeature) { > - virCPUDefPtr cpu; > - > - if (VIR_ALLOC(cpu) < 0) > - return -1; > + if (!def->cpu) { > + virCPUDefPtr cpu; > + if (VIR_ALLOC(cpu) < 0) > + return -1; > > - if (VIR_ALLOC(cpu->features) < 0) { > - VIR_FREE(cpu); > - return -1; > + cpu->mode = VIR_CPU_MODE_HOST_PASSTHROUGH; > + cpu->type = VIR_CPU_TYPE_GUEST; > + cpu->nfeatures = 0; > + cpu->nfeatures_max = 0; > + def->cpu = cpu; > } > > - if (VIR_STRDUP(cpu->features->name, vtfeature) < 0) { > - VIR_FREE(cpu->features); > - VIR_FREE(cpu); > - return -1; > + if (val == 0) { > + if (virCPUDefAddFeature(def->cpu, > + vtfeature, > + VIR_CPU_FEATURE_DISABLE) < 0) > + return -1; > + } else if (val == 1) { > + if (virCPUDefAddFeature(def->cpu, > + vtfeature, > + VIR_CPU_FEATURE_FORCE) < 0) > + return -1; > } Aren't you forgetting about VIR_FREE(cpu) if virCPUDefAddFeature fails? > - cpu->features->policy = VIR_CPU_FEATURE_DISABLE; > - cpu->nfeatures = cpu->nfeatures_max = 1; > - cpu->mode = VIR_CPU_MODE_HOST_PASSTHROUGH; > - cpu->type = VIR_CPU_TYPE_GUEST; > - def->cpu = cpu; > } > } > } else { > @@ -1157,6 +1151,7 @@ xenFormatXLOS(virConfPtr conf, virDomainDefPtr def) > if (def->cpu && > def->cpu->mode == VIR_CPU_MODE_HOST_PASSTHROUGH) { > bool hasHwVirt = true; > + int nestedhvm = -1; > Same comment as in the beginning regarding the initialization of nestedhvm to 0. For consistency (since this block very similar to the previous one) we may want to stick to nestedhvm in all similar uses of this variable. IOW using nestedhvm instead nested_hvm in begining of the patch. > if (def->cpu->nfeatures) { > for (i = 0; i < def->cpu->nfeatures; i++) { > @@ -1166,11 +1161,15 @@ xenFormatXLOS(virConfPtr conf, virDomainDefPtr def) > case VIR_CPU_FEATURE_FORBID: > if (STREQ(def->cpu->features[i].name, "vmx") || > STREQ(def->cpu->features[i].name, "svm")) > - hasHwVirt = false; > + nestedhvm = 0; > break; > > case VIR_CPU_FEATURE_FORCE: > case VIR_CPU_FEATURE_REQUIRE: > + if (STREQ(def->cpu->features[i].name, "vmx") || > + STREQ(def->cpu->features[i].name, "svm")) > + nestedhvm = 1; > + break; > case VIR_CPU_FEATURE_OPTIONAL: > case VIR_CPU_FEATURE_LAST: > break; > @@ -1178,7 +1177,9 @@ xenFormatXLOS(virConfPtr conf, virDomainDefPtr def) > } > } > > - if (xenConfigSetInt(conf, "nestedhvm", hasHwVirt) < 0) > + if (hasHwVirt && > + nestedhvm != -1 && > + xenConfigSetInt(conf, "nestedhvm", nestedhvm) < 0) > return -1; If 0 or 1 is used for nestedhvm values you can probably just (hasHwVirt && nestedhvm && ...) > } > > diff --git a/tests/libxlxml2domconfigdata/vnuma-hvm.json b/tests/libxlxml2domconfigdata/vnuma-hvm.json > index 3a5071e..2437a84 100644 > --- a/tests/libxlxml2domconfigdata/vnuma-hvm.json > +++ b/tests/libxlxml2domconfigdata/vnuma-hvm.json > @@ -113,7 +113,6 @@ > "pae": "True", > "apic": "True", > "acpi": "True", > - "nested_hvm": "True", > "vga": { > "kind": "cirrus" > }, > diff --git a/tests/xlconfigdata/test-fullvirt-nestedhvm.xml b/tests/xlconfigdata/test-fullvirt-nestedhvm.xml > index 8c02e7a..8a55bea 100644 > --- a/tests/xlconfigdata/test-fullvirt-nestedhvm.xml > +++ b/tests/xlconfigdata/test-fullvirt-nestedhvm.xml > @@ -14,7 +14,9 @@ > <apic/> > <pae/> > </features> > - <cpu mode='host-passthrough'/> > + <cpu mode='host-passthrough'> > + <feature policy='force' name='vmx'/> > + </cpu> > <clock offset='variable' adjustment='0' basis='utc'/> > <on_poweroff>destroy</on_poweroff> > <on_reboot>restart</on_reboot> > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
