Re: [libvirt] [PATCH v4 3/8] libxl: error out on not supported CPU mode, instead of silently ignoring
On Mon, Feb 26, 2018 at 09:48:14PM +0100, Marek Marczykowski-Górecki wrote: > On Mon, Feb 26, 2018 at 01:20:49PM -0700, Jim Fehlig wrote: > > On 02/15/2018 02:47 PM, Marek Marczykowski-Górecki wrote: > > > On Tue, Feb 13, 2018 at 09:02:35AM -0700, Jim Fehlig wrote: > > > > It looks like we never answered my question from V3, i.e. should we > > > > change > > > > the default mode in the post-parse callback if NUMA or CPU features are > > > > defined within ? > > > > > > Hmm, but this means changing the config behind user's back, no? > > > > Well, sort of. But it is really just making the implicit explicit. > > > > > You have disabled nested HVM, upgrade, now you have enabled. > > > Global switch is some consolation here, but is it enough? > > > > I _think_ so. With a global default that disables nesting, are there any > > scenarios you envision where a previously disabled nesting becomes enabled > > after upgrade? > > Probably also importing VM from older libvirt. Not sure about migration? > > Any other opinion about this? Daniel? Since introduction of global > switch I'm fine with either. As a general rule in libvirt we aim to preserve compatibility such that upgrading either libvirt or the hypervisor stack will not change guest visible ABI. This ensures that migration from old to new libvirt and/or old to new hypervisor will succeed without breaking guests. The caveat is that for QEMU driver this is only guaranteed if QEMU is using a versioned machine type - which means x86, aarch64 and ppc only at this time. If previous versions of the libxl driver defaulted to having svm/vmx enabled by default, and enw versions of libvirt libxl defualt to disabled, that would be a potentially guest visible change. I think you can probably argue that this is none the less OK, because there is a global config parameter to change the default behaviour to regain strict compatibilty 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
Re: [libvirt] [PATCH v4 3/8] libxl: error out on not supported CPU mode, instead of silently ignoring
On 02/26/2018 01:48 PM, Marek Marczykowski-Górecki wrote: On Mon, Feb 26, 2018 at 01:20:49PM -0700, Jim Fehlig wrote: On 02/15/2018 02:47 PM, Marek Marczykowski-Górecki wrote: On Tue, Feb 13, 2018 at 09:02:35AM -0700, Jim Fehlig wrote: It looks like we never answered my question from V3, i.e. should we change the default mode in the post-parse callback if NUMA or CPU features are defined within ? Hmm, but this means changing the config behind user's back, no? Well, sort of. But it is really just making the implicit explicit. You have disabled nested HVM, upgrade, now you have enabled. Global switch is some consolation here, but is it enough? I _think_ so. With a global default that disables nesting, are there any scenarios you envision where a previously disabled nesting becomes enabled after upgrade? Probably also importing VM from older libvirt. Not sure about migration? If importing from an older libvirt, the newer libvirt would have nesting disabled by default. I don't see a problem in that case. The same should be true when migrating from older to new libvirt. The only interesting case is migrating from libvirt containing this work to one without it. And then, only when the config contains and svm|vmx are not disabled. Support for settings under are quite new to the libxl driver, so in practice encountering that case is unlikely. Regards, Jim -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 3/8] libxl: error out on not supported CPU mode, instead of silently ignoring
On Mon, Feb 26, 2018 at 01:20:49PM -0700, Jim Fehlig wrote: > On 02/15/2018 02:47 PM, Marek Marczykowski-Górecki wrote: > > On Tue, Feb 13, 2018 at 09:02:35AM -0700, Jim Fehlig wrote: > > > It looks like we never answered my question from V3, i.e. should we change > > > the default mode in the post-parse callback if NUMA or CPU features are > > > defined within ? > > > > Hmm, but this means changing the config behind user's back, no? > > Well, sort of. But it is really just making the implicit explicit. > > > You have disabled nested HVM, upgrade, now you have enabled. > > Global switch is some consolation here, but is it enough? > > I _think_ so. With a global default that disables nesting, are there any > scenarios you envision where a previously disabled nesting becomes enabled > after upgrade? Probably also importing VM from older libvirt. Not sure about migration? Any other opinion about this? Daniel? Since introduction of global switch I'm fine with either. > BTW, I'm fine with this patch, just playing devil's advocate with handling > such things post-parse vs domain start. It's not the only place where > default xen config is not reflected in libvirt :-/. > > Regards, > Jim > > > > > > It sure feels like such configuration tweaks and error checks should be > > > handled in the parsing phase, similar to qemuDomainDefVcpusPostParse() and > > > qemuDomainDefCPUPostParse() in the qemu driver. I think danpb is vaguely > > > familiar with this series and can help answer that question. Daniel, do > > > you > > > have an opinion? Or a general comment about guidelines for > > > checking/tweaking > > > config in parse phase vs domain start phase? > > > -- 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? signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 3/8] libxl: error out on not supported CPU mode, instead of silently ignoring
On 02/15/2018 02:47 PM, Marek Marczykowski-Górecki wrote:
On Tue, Feb 13, 2018 at 09:02:35AM -0700, Jim Fehlig wrote:
On 02/08/2018 03:58 PM, Marek Marczykowski-Górecki wrote:
This change make libvirt XML with plain element invalid for libxl,
which affect not only upcoming CPUID support, but also NUMA. In fact,
default mode 'custom' does not match what the driver actually does, so
it was a bug. Adjust xenconfig driver accordingly.
But nevertheless this commit break some configurations that were working
before.
---
Changes since v3:
- fix vnuma tests (nested HVM implicitly enabled there)
Changes since v2:
- change separated from 'libxl: add support for CPUID features policy'
---
src/libxl/libxl_conf.c | 10 --
src/xenconfig/xen_xl.c | 1 +-
tests/xlconfigdata/test-fullvirt-vnuma-autocomplete.cfg | 1 +-
tests/xlconfigdata/test-fullvirt-vnuma-autocomplete.xml | 2 +-
tests/xlconfigdata/test-fullvirt-vnuma-nodistances.cfg | 1 +-
tests/xlconfigdata/test-fullvirt-vnuma-nodistances.xml | 2 +-
tests/xlconfigdata/test-fullvirt-vnuma-partialdist.cfg | 1 +-
tests/xlconfigdata/test-fullvirt-vnuma-partialdist.xml | 2 +-
tests/xlconfigdata/test-fullvirt-vnuma.cfg | 1 +-
tests/xlconfigdata/test-fullvirt-vnuma.xml | 2 +-
10 files changed, 17 insertions(+), 6 deletions(-)
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 8cced29..66956a7 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -355,11 +355,17 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
def->features[VIR_DOMAIN_FEATURE_ACPI] ==
VIR_TRISTATE_SWITCH_ON);
-if (caps &&
-def->cpu && def->cpu->mode == (VIR_CPU_MODE_HOST_PASSTHROUGH)) {
+if (caps && def->cpu) {
bool hasHwVirt = false;
bool svm = false, vmx = false;
+if (def->cpu->mode != VIR_CPU_MODE_HOST_PASSTHROUGH) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unsupported cpu mode '%s'"),
+ virCPUModeTypeToString(def->cpu->mode));
+return -1;
+}
It looks like we never answered my question from V3, i.e. should we change
the default mode in the post-parse callback if NUMA or CPU features are
defined within ?
Hmm, but this means changing the config behind user's back, no?
Well, sort of. But it is really just making the implicit explicit.
You have disabled nested HVM, upgrade, now you have enabled.
Global switch is some consolation here, but is it enough?
I _think_ so. With a global default that disables nesting, are there any
scenarios you envision where a previously disabled nesting becomes enabled after
upgrade?
BTW, I'm fine with this patch, just playing devil's advocate with handling such
things post-parse vs domain start. It's not the only place where default xen
config is not reflected in libvirt :-/.
Regards,
Jim
It sure feels like such configuration tweaks and error checks should be
handled in the parsing phase, similar to qemuDomainDefVcpusPostParse() and
qemuDomainDefCPUPostParse() in the qemu driver. I think danpb is vaguely
familiar with this series and can help answer that question. Daniel, do you
have an opinion? Or a general comment about guidelines for checking/tweaking
config in parse phase vs domain start phase?
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 3/8] libxl: error out on not supported CPU mode, instead of silently ignoring
On Tue, Feb 13, 2018 at 09:02:35AM -0700, Jim Fehlig wrote:
> On 02/08/2018 03:58 PM, Marek Marczykowski-Górecki wrote:
> > This change make libvirt XML with plain element invalid for libxl,
> > which affect not only upcoming CPUID support, but also NUMA. In fact,
> > default mode 'custom' does not match what the driver actually does, so
> > it was a bug. Adjust xenconfig driver accordingly.
> >
> > But nevertheless this commit break some configurations that were working
> > before.
> >
> > ---
> > Changes since v3:
> > - fix vnuma tests (nested HVM implicitly enabled there)
> > Changes since v2:
> > - change separated from 'libxl: add support for CPUID features policy'
> > ---
> > src/libxl/libxl_conf.c | 10 --
> > src/xenconfig/xen_xl.c | 1 +-
> > tests/xlconfigdata/test-fullvirt-vnuma-autocomplete.cfg | 1 +-
> > tests/xlconfigdata/test-fullvirt-vnuma-autocomplete.xml | 2 +-
> > tests/xlconfigdata/test-fullvirt-vnuma-nodistances.cfg | 1 +-
> > tests/xlconfigdata/test-fullvirt-vnuma-nodistances.xml | 2 +-
> > tests/xlconfigdata/test-fullvirt-vnuma-partialdist.cfg | 1 +-
> > tests/xlconfigdata/test-fullvirt-vnuma-partialdist.xml | 2 +-
> > tests/xlconfigdata/test-fullvirt-vnuma.cfg | 1 +-
> > tests/xlconfigdata/test-fullvirt-vnuma.xml | 2 +-
> > 10 files changed, 17 insertions(+), 6 deletions(-)
> >
> > diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> > index 8cced29..66956a7 100644
> > --- a/src/libxl/libxl_conf.c
> > +++ b/src/libxl/libxl_conf.c
> > @@ -355,11 +355,17 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
> > def->features[VIR_DOMAIN_FEATURE_ACPI] ==
> > VIR_TRISTATE_SWITCH_ON);
> > -if (caps &&
> > -def->cpu && def->cpu->mode == (VIR_CPU_MODE_HOST_PASSTHROUGH))
> > {
> > +if (caps && def->cpu) {
> > bool hasHwVirt = false;
> > bool svm = false, vmx = false;
> > +if (def->cpu->mode != VIR_CPU_MODE_HOST_PASSTHROUGH) {
> > +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > + _("unsupported cpu mode '%s'"),
> > + virCPUModeTypeToString(def->cpu->mode));
> > +return -1;
> > +}
>
> It looks like we never answered my question from V3, i.e. should we change
> the default mode in the post-parse callback if NUMA or CPU features are
> defined within ?
Hmm, but this means changing the config behind user's back, no?
You have disabled nested HVM, upgrade, now you have enabled.
Global switch is some consolation here, but is it enough?
> It sure feels like such configuration tweaks and error checks should be
> handled in the parsing phase, similar to qemuDomainDefVcpusPostParse() and
> qemuDomainDefCPUPostParse() in the qemu driver. I think danpb is vaguely
> familiar with this series and can help answer that question. Daniel, do you
> have an opinion? Or a general comment about guidelines for checking/tweaking
> config in parse phase vs domain start phase?
--
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?
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 3/8] libxl: error out on not supported CPU mode, instead of silently ignoring
On 02/08/2018 03:58 PM, Marek Marczykowski-Górecki wrote:
This change make libvirt XML with plain element invalid for libxl,
which affect not only upcoming CPUID support, but also NUMA. In fact,
default mode 'custom' does not match what the driver actually does, so
it was a bug. Adjust xenconfig driver accordingly.
But nevertheless this commit break some configurations that were working
before.
---
Changes since v3:
- fix vnuma tests (nested HVM implicitly enabled there)
Changes since v2:
- change separated from 'libxl: add support for CPUID features policy'
---
src/libxl/libxl_conf.c | 10 --
src/xenconfig/xen_xl.c | 1 +-
tests/xlconfigdata/test-fullvirt-vnuma-autocomplete.cfg | 1 +-
tests/xlconfigdata/test-fullvirt-vnuma-autocomplete.xml | 2 +-
tests/xlconfigdata/test-fullvirt-vnuma-nodistances.cfg | 1 +-
tests/xlconfigdata/test-fullvirt-vnuma-nodistances.xml | 2 +-
tests/xlconfigdata/test-fullvirt-vnuma-partialdist.cfg | 1 +-
tests/xlconfigdata/test-fullvirt-vnuma-partialdist.xml | 2 +-
tests/xlconfigdata/test-fullvirt-vnuma.cfg | 1 +-
tests/xlconfigdata/test-fullvirt-vnuma.xml | 2 +-
10 files changed, 17 insertions(+), 6 deletions(-)
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 8cced29..66956a7 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -355,11 +355,17 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
def->features[VIR_DOMAIN_FEATURE_ACPI] ==
VIR_TRISTATE_SWITCH_ON);
-if (caps &&
-def->cpu && def->cpu->mode == (VIR_CPU_MODE_HOST_PASSTHROUGH)) {
+if (caps && def->cpu) {
bool hasHwVirt = false;
bool svm = false, vmx = false;
+if (def->cpu->mode != VIR_CPU_MODE_HOST_PASSTHROUGH) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unsupported cpu mode '%s'"),
+ virCPUModeTypeToString(def->cpu->mode));
+return -1;
+}
It looks like we never answered my question from V3, i.e. should we change the
default mode in the post-parse callback if NUMA or CPU features are defined
within ?
It sure feels like such configuration tweaks and error checks should be handled
in the parsing phase, similar to qemuDomainDefVcpusPostParse() and
qemuDomainDefCPUPostParse() in the qemu driver. I think danpb is vaguely
familiar with this series and can help answer that question. Daniel, do you have
an opinion? Or a general comment about guidelines for checking/tweaking config
in parse phase vs domain start phase?
Regards,
Jim
+
if (ARCH_IS_X86(def->os.arch)) {
vmx = virCPUCheckFeature(caps->host.arch, caps->host.cpu,
"vmx");
svm = virCPUCheckFeature(caps->host.arch, caps->host.cpu,
"svm");
diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c
index 2ef80eb..6cda305 100644
--- a/src/xenconfig/xen_xl.c
+++ b/src/xenconfig/xen_xl.c
@@ -494,6 +494,7 @@ xenParseXLVnuma(virConfPtr conf,
goto cleanup;
}
+cpu->mode = VIR_CPU_MODE_HOST_PASSTHROUGH;
cpu->type = VIR_CPU_TYPE_GUEST;
def->cpu = cpu;
diff --git a/tests/xlconfigdata/test-fullvirt-vnuma-autocomplete.cfg b/tests/xlconfigdata/test-fullvirt-vnuma-autocomplete.cfg
index edba69a..2c9de44 100644
--- a/tests/xlconfigdata/test-fullvirt-vnuma-autocomplete.cfg
+++ b/tests/xlconfigdata/test-fullvirt-vnuma-autocomplete.cfg
@@ -22,5 +22,6 @@ parallel = "none"
serial = "none"
builder = "hvm"
boot = "d"
+nestedhvm = 1
vnuma = [ [ "pnode=0", "size=2048", "vcpus=0,11", "vdistances=10,21,31,41,51,61" ], [ "pnode=1", "size=2048", "vcpus=1,10", "vdistances=21,10,21,31,41,51" ], [ "pnode=2", "size=2048", "vcpus=2,9",
"vdistances=31,21,10,21,31,41" ], [ "pnode=3", "size=2048", "vcpus=3,8", "vdistances=41,31,21,10,21,31" ], [ "pnode=4", "size=2048", "vcpus=4,7", "vdistances=51,41,31,21,10,21" ], [ "pnode=5", "size=2048",
"vcpus=5-6", "vdistances=61,51,41,31,21,10" ] ]
disk = [
"format=raw,vdev=hda,access=rw,backendtype=phy,target=/dev/HostVG/XenGuest2" ]
diff --git a/tests/xlconfigdata/test-fullvirt-vnuma-autocomplete.xml
b/tests/xlconfigdata/test-fullvirt-vnuma-autocomplete.xml
index e3639eb..3c486ad 100644
--- a/tests/xlconfigdata/test-fullvirt-vnuma-autocomplete.xml
+++ b/tests/xlconfigdata/test-fullvirt-vnuma-autocomplete.xml
@@ -14,7 +14,7 @@
-
+
diff --git a/tests/xlconfigdata/test-fullvirt-vnuma-nodistances.cfg
b/tests/xlconfigdata/test-fullvirt-vnuma-nodistances.cfg
index 8186edf..ca8e5b0 100644
--- a/tests/xlconfigdata/test-fullvirt-vnuma-nodistances.cfg
+++ b/tests/xlconfigdata/test-fullvirt-vnuma-nodistances.cfg
@@ -22,5 +22,6 @@ parallel = "none"
serial = "none"
builder = "hvm"
boot = "d"
+nestedh
