[libvirt] [PATCHv2 11/11] qemu_driver: BaselineHypervisorCPU supports S390 using QEMU/QMP
David Hildenbrand
david at redhat.com
Tue Jul 17 21:01:02 UTC 2018
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.
>
> 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.
>
>> 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).
--
Thanks,
David / dhildenb
More information about the libvir-list
mailing list