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

[Jim Fehlig]

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

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