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
