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

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 :)