Re: [libvirt] [PATCHv2 11/11] qemu_driver: BaselineHypervisorCPU supports S390 using QEMU/QMP

On Mon, Jul 09, 2018 at 22:56:55 -0500, Chris Venteicher wrote: 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.) Most of the code you added in this patch is indented by three spaces while we use four spaces in libvirt. 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. 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. No need for this extra empty line. 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. 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. > + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION)) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("Feature Expansion not supported with this QEMU binary")); > + goto cleanup; > + } Is this necessary? Can't qemuMonitorGetCPUModelExpansion just report the error when it tries to call an unsupported QMP command? Or is the error message confusing? > + if (qemuMonitorGetCPUModelExpansion(cmd->mon, > + QEMU_MONITOR_CPU_MODEL_EXPANSION_FULL, > + migratable_only, modelInfo) < 0) > + goto cleanup; > + /* Expansion enumerates all features > + * Baseline reply enumerates only in-model (true) features */ > + qemuMonitorCPUModelInfoRemovePropByBoolValue(modelInfo, false); > + if (!(cpu = virQEMUCapsCPUModelInfoToCPUDef(migratable_only, modelInfo))) > + goto cleanup; > + } > + } > > cpustr = virCPUDefFormat(cpu, NULL); > > @@ -13469,6 +13523,8 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn, > virCPUDefFree(cpu); > virObjectUnref(qemuCaps); > virStringListFree(features); > + virQEMUCapsInitQMPCommandFree(cmd); > + qemuMonitorCPUModelInfoFree(modelInfo); > > return cpustr; > } Jirka