On 18.07.2018 00:39, Collin Walling wrote:
> On 07/17/2018 05:01 PM, David Hildenbrand wrote:
>> On 13.07.2018 18:00, Jiri Denemark wrote:
>>> On Mon, Jul 09, 2018 at 22:56:55 -0500, Chris Venteicher wrote:
>>>> Transient S390 configurations require using QEMU to compute CPU Model
>>>> Baseline and to do CPU Feature Expansion.
>>>>
>>>> Start and use a single QEMU instance to do both the baseline and
>>>> expansion transactions required by BaselineHypervisorCPU.
>>>>
>>>> CPU Feature Expansion uses true / false to indicate if property is/isn't
>>>> included in model. Baseline only returns property list where all
>>>> enumerated properties are included.
>>>
>>> So are you saying on s390 there's no chance there would be a CPU model
>>> with some feature which is included in the CPU model disabled for some
>>> reason? Sounds too good to be true :-) (This is the question I referred
>>> to in one of my replies to the other patches.)
>>
>> Giving some background information: When we expand/baseline CPU models,
>> we always expand them to the "-base" variants of our CPU models, which
>> contain some set of features we expect to be around in all sane
>> configurations ("minimal feature set").
>>
>> It is very unlikely that we ever have something like
>> "z14-base,featx=off", but it could happen
>>  - When using an emulator (TCG)
>>  - When running nested and the guest hypervisor is started with such a
>>    strange CPU model
>>  - When something in the HW is very wrong or eventually removed in the
>>    future (unlikely but possible)
>>
>> On some very weird inputs to a baseline request, such a strange model
>> can also be the result. But it is very unusual.
>>
>> I assume something like "baseline z14-base,featx=off with z14-base" will
>> result in "z14-base,featx=off", too.
>>
>>
> 
> That's correct. A CPU model with a feature disabled that is baseline with a 
> CPU 
> model with that same feature enabled will omit that feature in the QMP 
> response.
> 
> e.g. if z14-base has features x, y, z then
> 
> "baseline z14-base,featx=off with z14-base" will result in 
> "z14-base,featy=on,featz=on"

Usually we try to not chose a model with stripped off base features ("we
try to produce a model that looks sane"), but instead fallback to some
very ancient CPU model. E.g.

{ "execute": "query-cpu-model-baseline", "arguments" : { "modela": {
"name": "z14-base", "props": {"msa" : false}}, "modelb": { "name": "z14"}} }

-> {"return": {"model": {"name": "z800-base", "props": {"etf2": true,
"ldisp": true}}}}

We might want to change that behavior in the future however (or maybe it
already is like this for some corner cases) - assume some base feature
gets dropped by HW in a new CPU generation. We don't always want to
fallback to a z900 or so when baselining. So one should assume that we
can have disabled features here.

Especially as there is a BUG in QEMU I'll have to fix:

{ "execute": "query-cpu-model-baseline", "arguments" : { "modela": {
"name": "z14-base", "props": {"esan3" : false}}, "modelb": { "name":
"z14"}} }

-> Segmentation fault

This would have to produce a model with esan3 disabled (very very
unlikely to ever happen in real life :) )

The result should be something like {"model": {"name": "z900-base",
"props": {"esan3": false}}} or an error that they cannot be baselined.



> 
> Since baseline will just report a base cpu and its minimal feature set... I 
> wonder if it
> makes sense for libvirt to just report the features resulting from the 
> baseline command 
> instead of later calling expansion?

Yes it does and the docs say:

"Baseline two CPU models, creating a compatible third model. The created
model will always be a static, migration-safe CPU model (see "static"
CPU model expansion for details)"

> 
>>>
>>> Most of the code you added in this patch is indented by three spaces
>>> while we use four spaces in libvirt.
>>>
>>>> ---
>>>>  src/qemu/qemu_driver.c | 74 +++++++++++++++++++++++++++++++++++++-----
>>>>  1 file changed, 65 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>>>> index 9a35e04a85..6c6107f077 100644
>>>> --- a/src/qemu/qemu_driver.c
>>>> +++ b/src/qemu/qemu_driver.c
>>>> @@ -13400,10 +13400,13 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr 
>>>> conn,
>>>>      virArch arch;
>>>>      virDomainVirtType virttype;
>>>>      virDomainCapsCPUModelsPtr cpuModels;
>>>> -    bool migratable;
>>>> +    bool migratable_only;
>>>
>>> Why? The bool says the user specified
>>> VIR_CONNECT_BASELINE_CPU_MIGRATABLE which means they want a migratable
>>> CPU back. What does the "_only" part mean? This API does not return
>>> several CPUs, it only returns a single one and it's either migratable or
>>> not.
>>>
>>>>      virCPUDefPtr cpu = NULL;
>>>>      char *cpustr = NULL;
>>>>      char **features = NULL;
>>>> +    virQEMUCapsInitQMPCommandPtr cmd = NULL;
>>>> +    bool forceTCG = false;
>>>> +    qemuMonitorCPUModelInfoPtr modelInfo = NULL;
>>>>  
>>>>      virCheckFlags(VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES |
>>>>                    VIR_CONNECT_BASELINE_CPU_MIGRATABLE, NULL);
>>>> @@ -13411,8 +13414,6 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr 
>>>> conn,
>>>>      if (virConnectBaselineHypervisorCPUEnsureACL(conn) < 0)
>>>>          goto cleanup;
>>>>  
>>>> -    migratable = !!(flags & VIR_CONNECT_BASELINE_CPU_MIGRATABLE);
>>>> -
>>>>      if (!(cpus = virCPUDefListParse(xmlCPUs, ncpus, VIR_CPU_TYPE_AUTO)))
>>>>          goto cleanup;
>>>>  
>>>> @@ -13425,6 +13426,19 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr 
>>>> conn,
>>>>      if (!qemuCaps)
>>>>          goto cleanup;
>>>>  
>>>> +    /* QEMU can enumerate non-migratable cpu model features for some 
>>>> archs like x86
>>>> +     * migratable_only == true:  ask for and include only migratable 
>>>> features
>>>> +     * migratable_only == false: ask for and include all features
>>>> +     */
>>>> +    migratable_only = !!(flags & VIR_CONNECT_BASELINE_CPU_MIGRATABLE);
>>>> +
>>>> +    if (ARCH_IS_S390(arch)) {
>>>> +       /* QEMU for S390 arch only enumerates migratable features
>>>> +        * No reason to explicitly ask QEMU for or include non-migratable 
>>>> features
>>>> +        */
>>>> +       migratable_only = true;
>>>> +    }
>>>> +
>>>
>>> And what if they come up with some features which are not migratable in
>>> the future? I don't think there's any reason for this API to mess with
>>> the value. The code should just provide the same CPU in both cases for
>>> s390.
>>
>> s390x usually only provides features if they are migratable. Could it
>> happen it the future that we have something that cannot be migrated?
>> Possible but very very unlikely. We cared about migration (even for
>> nested support) right from the beginning. As of now, we have no features
>> that are supported by QEMU that cannot be migrated - in contrast to e.g.
>> x86 (e.g. VMX/SVM). Every new feature has to be whitelisted in QEMU -
>> and we only allow to do so if migration is in place for it.
>>
> 
> You're a gold mine on this kind of information.  I may have to pester you 
> about some 
> CPU model related stuff in the future :)

Sure, just CC or ask me and I'm happy to help.


-- 

Thanks,

David / dhildenb

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Reply via email to