[libvirt] [PATCH v4 3/8] libxl: error out on not supported CPU mode, instead of silently ignoring

[Marek Marczykowski-Górecki]

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?

-- 
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?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180215/ab1636ec/attachment-0001.sig>