<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.
---
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;
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;
+ nested_hvm = 0;
break;
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;
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);
}
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;
}
- 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;
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;
}
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>
--
git-series 0.9.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Dec 19, 2017 at 01:01:36PM +0000, Joao Martins wrote: > [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'/> Yes, the key point of libvirt is to apply consistent semantics across different drivers, so we should not diverge betweeen QEMU & Xen in this regard. 'host-passthrough' and 'host-model' are supposed to expose *every* feature that the host CPUs support (except for those few which the hypervisor may block due to ability to virtualize them). So 'host-passthrough' is correct to automatically expose vmx/svm, without requiring any extra <feature> element, and I don't think we can accept this patch. This has been the case for KVM for ages, even though it has been considered experimental. The only slight difference is that you can block use of svm/vmx at the host OS level via a kernel arg to the kvm modules. If you want to not expose svm/vmx to the guest, despite it being available in the host, then use feature policy=disble when configuring it. > (I think you propose policy='force' here which is probably better suited as > opposed to policy='require') It depends on what semantics the Xen hypervisor provides. 'require' means expose the feature to the guest if it is supported by the host, but raise an error if the host doesn't support it. 'force' means expose the feature to the guest, even if the host does not support it at all. For HVM Xen guests there's no real distinction between these, as you can't run an HVM Xen guest without having hardware virt in your host. So for 'vmx' / 'svm' force/require are basically the same thing. For other CPU feature bits they are definitely different. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 12/19/2017 01:13 PM, Daniel P. Berrange wrote: > On Tue, Dec 19, 2017 at 01:01:36PM +0000, Joao Martins wrote: >> [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'/> > > Yes, the key point of libvirt is to apply consistent semantics across different > drivers, so we should not diverge betweeen QEMU & Xen in this regard. > /nods > 'host-passthrough' and 'host-model' are supposed to expose *every* feature that > the host CPUs support (except for those few which the hypervisor may block due > to ability to virtualize them). > > So 'host-passthrough' is correct to automatically expose vmx/svm, without > requiring any extra <feature> element, and I don't think we can accept > this patch. > > This has been the case for KVM for ages, even though it has been considered > experimental. The only slight difference is that you can block use of svm/vmx > at the host OS level via a kernel arg to the kvm modules. > Ah that's where Xen falls off a little in which there's only libxl nested_hvm field to control it, even though is still marked Experimental. There's no global parameter to block it. > If you want to not expose svm/vmx to the guest, despite it being available > in the host, then use feature policy=disble when configuring it. > >> (I think you propose policy='force' here which is probably better suited as >> opposed to policy='require') > > It depends on what semantics the Xen hypervisor provides. > > 'require' means expose the feature to the guest if it is supported > by the host, but raise an error if the host doesn't support it. > > 'force' means expose the feature to the guest, even if the host does > not support it at all. > > For HVM Xen guests there's no real distinction between these, as you > can't run an HVM Xen guest without having hardware virt in your > host. So for 'vmx' / 'svm' force/require are basically the same > thing. For other CPU feature bits they are definitely different. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Dec 19, 2017 at 01:43:24PM +0000, Joao Martins wrote: > On 12/19/2017 01:13 PM, Daniel P. Berrange wrote: > > On Tue, Dec 19, 2017 at 01:01:36PM +0000, Joao Martins wrote: > >> [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'/> > > > > Yes, the key point of libvirt is to apply consistent semantics across different > > drivers, so we should not diverge betweeen QEMU & Xen in this regard. > > > > /nods > > > 'host-passthrough' and 'host-model' are supposed to expose *every* feature that > > the host CPUs support (except for those few which the hypervisor may block due > > to ability to virtualize them). > > > > So 'host-passthrough' is correct to automatically expose vmx/svm, without > > requiring any extra <feature> element, and I don't think we can accept > > this patch. > > > > This has been the case for KVM for ages, even though it has been considered > > experimental. The only slight difference is that you can block use of svm/vmx > > at the host OS level via a kernel arg to the kvm modules. > > > Ah that's where Xen falls off a little in which there's only libxl nested_hvm > field to control it, even though is still marked Experimental. There's no global > parameter to block it. You could conceivably replicate the host-level control KVM has by using an /etc/libvirt/libxl.conf driver level config option to indicate whether nested-virt is permitted or not. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Dec 19, 2017 at 01:45:57PM +0000, Daniel P. Berrange wrote:
> On Tue, Dec 19, 2017 at 01:43:24PM +0000, Joao Martins wrote:
> > On 12/19/2017 01:13 PM, Daniel P. Berrange wrote:
> > > On Tue, Dec 19, 2017 at 01:01:36PM +0000, Joao Martins wrote:
> > >> [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'/>
> > >
> > > Yes, the key point of libvirt is to apply consistent semantics across different
> > > drivers, so we should not diverge betweeen QEMU & Xen in this regard.
> > >
> >
> > /nods
> >
> > > 'host-passthrough' and 'host-model' are supposed to expose *every* feature that
> > > the host CPUs support (except for those few which the hypervisor may block due
> > > to ability to virtualize them).
> > >
> > > So 'host-passthrough' is correct to automatically expose vmx/svm, without
> > > requiring any extra <feature> element, and I don't think we can accept
> > > this patch.
My point is you can use <cpu> element to configure various features,
like mentioned above (NUMA etc). As discussed previously, in libxl
driver only 'host-passthrough' mode makes sense, because that's what
libxl allows (enabled/disable various features, not define the whole
CPU). So, you can use something like:
<cpu mode='host-passthrough'>
<numa>
<cell id='0' cpus='0-3' memory='512000' unit='KiB'/>
<cell id='1' cpus='4-7' memory='512000' unit='KiB' memAccess='shared'/>
</numa>
</cpu>
Now, this is _very not obvious_ you've just enabled potentially
dangerous feature. Quoting
https://wiki.xenproject.org/wiki/Nested_Virtualization_in_Xen:
This means an L1 admin can DOS the L0 hypervisor. This is a
potential security issue; for this reason, we do not recommend running
nested virtualization in production yet.
Enabling potentially harmful features without explicit consent is not
something that I'd expect from a project meant to be used in production
environment...
Generally I think this is bad idea that placing just <cpu
mode='host-passthrough'>, without any specific setting, change anything
(compared to no <cpu/> at all). At least in context of libxl driver.
> > > This has been the case for KVM for ages, even though it has been considered
> > > experimental. The only slight difference is that you can block use of svm/vmx
> > > at the host OS level via a kernel arg to the kvm modules.
> > >
> > Ah that's where Xen falls off a little in which there's only libxl nested_hvm
> > field to control it, even though is still marked Experimental. There's no global
> > parameter to block it.
>
> You could conceivably replicate the host-level control KVM has by using an
> /etc/libvirt/libxl.conf driver level config option to indicate whether
> nested-virt is permitted or not.
That could work. Is 'nestedhvm' ok for parameter name (disabled by
default)?
--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Dec 19, 2017 at 08:44:48PM +0100, Marek Marczykowski-Górecki wrote:
> On Tue, Dec 19, 2017 at 01:45:57PM +0000, Daniel P. Berrange wrote:
> > On Tue, Dec 19, 2017 at 01:43:24PM +0000, Joao Martins wrote:
> > > On 12/19/2017 01:13 PM, Daniel P. Berrange wrote:
> > > > On Tue, Dec 19, 2017 at 01:01:36PM +0000, Joao Martins wrote:
> > > >> [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'/>
> > > >
> > > > Yes, the key point of libvirt is to apply consistent semantics across different
> > > > drivers, so we should not diverge betweeen QEMU & Xen in this regard.
> > > >
> > >
> > > /nods
> > >
> > > > 'host-passthrough' and 'host-model' are supposed to expose *every* feature that
> > > > the host CPUs support (except for those few which the hypervisor may block due
> > > > to ability to virtualize them).
> > > >
> > > > So 'host-passthrough' is correct to automatically expose vmx/svm, without
> > > > requiring any extra <feature> element, and I don't think we can accept
> > > > this patch.
>
> My point is you can use <cpu> element to configure various features,
> like mentioned above (NUMA etc). As discussed previously, in libxl
> driver only 'host-passthrough' mode makes sense, because that's what
> libxl allows (enabled/disable various features, not define the whole
> CPU). So, you can use something like:
>
> <cpu mode='host-passthrough'>
> <numa>
> <cell id='0' cpus='0-3' memory='512000' unit='KiB'/>
> <cell id='1' cpus='4-7' memory='512000' unit='KiB' memAccess='shared'/>
> </numa>
> </cpu>
>
> Now, this is _very not obvious_ you've just enabled potentially
> dangerous feature. Quoting
> https://wiki.xenproject.org/wiki/Nested_Virtualization_in_Xen:
>
> This means an L1 admin can DOS the L0 hypervisor. This is a
> potential security issue; for this reason, we do not recommend running
> nested virtualization in production yet.
>
> Enabling potentially harmful features without explicit consent is not
> something that I'd expect from a project meant to be used in production
> environment...
Whoever wrote that XML *has* given explicit consent, because that is applying
the documented semantics of the 'host-passthrough' CPU mode. This is exactly
the same situation as with the KVM driver.
The mistake here is assuming that mode='host-passthrough' is identical to
not listing CPUJ at all.
If you don't want VMX/SVM added when you define NUMA, then do
<cpu mode='host-passthrough'>
<feature name="vmx" policy="disable"/>
<numa>
<cell id='0' cpus='0-3' memory='512000' unit='KiB'/>
<cell id='1' cpus='4-7' memory='512000' unit='KiB' memAccess='shared'/>
</numa>
</cpu>
In retrospect the <numa> info should not have been inside the <cpu>
element, but that's something we unfortunately have to live with now
for back compatibility.
> Generally I think this is bad idea that placing just <cpu
> mode='host-passthrough'>, without any specific setting, change anything
> (compared to no <cpu/> at all). At least in context of libxl driver.
There's nothing specific about libxl there - it would do the same for
KVM too if the host supports svm/vmx.
> > You could conceivably replicate the host-level control KVM has by using an
> > /etc/libvirt/libxl.conf driver level config option to indicate whether
> > nested-virt is permitted or not.
>
> That could work. Is 'nestedhvm' ok for parameter name (disabled by
> default)?
Sure, whatever parameter name you feel is best - there's no rules about
parameters / naming for the driver specific global config files.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.