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"
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?
>>
>> 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 :)
>>
>>> if (!(cpuModels = virQEMUCapsGetCPUDefinitions(qemuCaps, virttype)) ||
>>> cpuModels->nmodels == 0) {
>>> virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
>>> @@ -13437,18 +13451,31 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn,
>>>
>>> if (ARCH_IS_X86(arch)) {
>>> int rc = virQEMUCapsGetCPUFeatures(qemuCaps, virttype,
>>> - migratable, &features);
>>> + migratable_only, &features);
>>> if (rc < 0)
>>> goto cleanup;
>>> if (features && rc == 0) {
>>> /* We got only migratable features from QEMU if we asked for them,
>>> * no further filtering in virCPUBaseline is desired. */
>>> - migratable = false;
>>> + migratable_only = false;
>>> }
>>>
>>> if (!(cpu = virCPUBaseline(arch, cpus, ncpus, cpuModels,
>>> - (const char **)features, migratable)))
>>> + (const char **)features, migratable_only)))
>>> goto cleanup;
>>> + } else if (ARCH_IS_S390(arch)) {
>>> +
>>
>> No need for this extra empty line.
>>
>>> + const char *binary = virQEMUCapsGetBinary(qemuCaps);
>>> + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>>> +
>>> + if (!(cmd = virQEMUCapsNewQMPCommandConnection(binary, cfg->libDir,
>>> + cfg->user, cfg->group,
>>> + forceTCG)))
>>> + goto cleanup;
>>> +
>>> + if ((virQEMUCapsQMPBaselineCPUModel(cmd, cpus, &cpu) < 0) || !cpu)
>>
>> Hmm, I think you should only call this when the user passed more than
>> one CPU. This API is expected to work even with a single CPU in which
>> case it just expands it if the corresponding flag was set.
>
> The QEMU side will bail out if only one model is passed, so this sounds
> like the right thing to do.
>
>>
>>> + goto cleanup; /* Content Error */
>>
>> What did you want to say with the comment? I think you can safely drop
>> it.
>>
>>> +
>>
>> And this one either.
>>
>>> } else {
>>> virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
>>> _("computing baseline hypervisor CPU is not supported "
>>> @@ -13458,9 +13485,36 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn,
>>>
>>> cpu->fallback = VIR_CPU_FALLBACK_FORBID;
>>>
>>> - if ((flags & VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES) &&
>>> - virCPUExpandFeatures(arch, cpu) < 0)
>>> - goto cleanup;
>>> + if (flags & VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES) {
>>> + if (ARCH_IS_X86(arch)) {
>>> + if (virCPUExpandFeatures(arch, cpu) < 0)
>>> + goto cleanup;
>>> + } else if (ARCH_IS_S390(arch)) {
>>> +
>>
>> Extra empty line.
>>
>>> + if (!(modelInfo = virQEMUCapsCPUModelInfoFromCPUDef(cpu)))
>>> + goto cleanup;
>>> +
>>> + virCPUDefFree(cpu); /* Null on failure, repopulated on success */
>>
>> I think it would be better to drop the comment and just do it:
>>
>> cpu = NULL;
>>
>> Oh and since this goes from CPUDef to modelInfo and back, you may
>> actually need to do some extra work to persist some parts of the
>> original CPU definitions which get lost during the translation (e.g.,
>> the CPU vendor) if it's applicable for s390. We have to do similar stuff
>> for x86 too when we translate CPUDef into CPUID bits and back.
>
> My assumption is that there are not such things (e.g. like vendor).
>
Correct. AFAIK and from what I've seen, s390 only cares about the arch, model, and
features data with regards to cpu models and libvirt.
--
Respectfully,
- Collin Walling
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
