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 <cpu> 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 <cpu>? 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?