Re: [libvirt] [PATCHv2 01/16] qemu: Add KVM CPUs into cache only if KVM is present

On Fri, Nov 23, 2018 at 06:16:46PM +0100, Jiri Denemark wrote: > On Fri, Nov 23, 2018 at 18:55:00 +0300, Roman Bolshakov wrote: > > On Fri, Nov 23, 2018 at 04:30:13PM +0100, Jiri Denemark wrote: > > > On Fri, Nov 23, 2018 at 17:16:12 +0300, Roman Bolshakov wrote: > > > > On Wed, Nov 21, 2018 at 07:43:43PM +0100, Jiri Denemark wrote: > > > > > virQEMUCapsInitHostCPUModel always fills in something and your check > > > > > should probably remain in place for it > > > > > > > > > > virQEMUCapsFormatHostCPUModelInfo does > > > > > virQEMUCapsHostCPUDataPtr cpuData = &qemuCaps->kvmCPU; > > > > > qemuMonitorCPUModelInfoPtr model = cpuData->info; > > > > > if (!model) > > > > > return; > > > > > > > > > > virQEMUCapsFormatCPUModels > > > > > cpus = qemuCaps->kvmCPUModels; > > > > > if (!cpus) > > > > > return; > > > > > > > > > > So to me it looks like all functions are ready to see NULL pointers and > > > > > just do nothing if that's the case. Thus the only thing this patch > > > > > should need to do is to make sure virQEMUCapsInitHostCPUModel does not > > > > > set something non-NULL there. > > > > > > > > Unfortunately, that won't work for the patch series. kvmCPUModels is renamed to > > > > accelCPUModels and kvmCPU is renamed to accelCPU in PATCH 6. > > > > > > And how does different name change the behavior? > > > > > > > So, virQEMUCapsFormatHostCPUModelInfo looks like: > > > > if (virQEMUCapsTypeIsAccelerated(type)) > > > > cpuData = qemuCaps->accelCPU; > > > > else > > > > cpuData = qemuCaps->tcgCPU; > > > > > > > > and virQEMUCapsFormatCPUModels looks like: > > > > if (virQEMUCapsTypeIsAccelerated(type)) > > > > cpus = qemuCaps->accelCPUModels; > > > > else > > > > cpus = qemuCaps->tcgCPUModels; > > > > > > > > Without the check we'd return CPUs for KVM domain on the platform that doesn't > > > > support it. > > > > > > It won't return anything because the code will make sure accelCPUModels > > > and accelCPU will be NULL when no accel method is supported. > > > > But accelCPU is not NULL on macOS with QEMU_CAPS_HVF and on Linux with > > QEMU_CAPS_KVM. That's where the problem arises. > > Right, and that's what I think should be changed. Rather then adding > checks to the formatting and loading code to ignore something which > shouldn't be present in the first place. > > > We're going to get additional kvm CPUs on mac and hvf CPUs on Linux > > and that will break qemucapabilitiestest. > > I think I'm missing something here. There's only one CPU definition > describing the host CPU. There are hosts which have several different > CPUs, but libvirt is not really prepared to see that and I believe this > is not what you're addressing with this series, is it? Or are you > talking about some other CPUs? > > > In fact they will be the same accelCPUs of the supported accelerator > > but with hostCPU's type attribute of the other accelerator. > > How would this happen? We have a single accelerator enabled on a host > and we generate a host CPU model for it (and just for it, there's no > reason to generate a CPU model for something that is not supported on > the host). > accelCPU will be present on a host where an accelerator is avaialable. You said can't have host CPU definitions present twice. I agree with that. But if we call virQEMUCapsFormatCPUModels twice for VIR_DOMAIN_VIRT_KVM and VIR_DOMAIN_VIRT_HVF without the checks, host cpu definitions will be present twice for each accelerator because accelCPU is not NULL. So we need to call it only once for the supported accelerator. The checks help in that. Alternative approach to do only one call is: virDomainVirtType acceleratedDomain = VIR_DOMAIN_VIRT_KVM; if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_HVF)) acceleratedDomain = VIR_DOMAIN_VIRT_HVF; virQEMUCapsFormatHostCPUModelInfo(qemuCaps, &buf, acceleratedDomain); virQEMUCapsFormatHostCPUModelInfo(qemuCaps, &buf, VIR_DOMAIN_VIRT_QEMU); virQEMUCapsFormatCPUModels(qemuCaps, &buf, acceleratedDomain); virQEMUCapsFormatCPUModels(qemuCaps, &buf, VIR_DOMAIN_VIRT_QEMU); Would that work for you? -- Thank you, Roman