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

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.

We're going to get additional kvm CPUs on mac and hvf CPUs on Linux and that will break qemucapabilitiestest.

In fact they will be the same accelCPUs of the supported accelerator but with hostCPU's type attribute of the other accelerator.

If you wish I can try to rework the patchset. Instead of generalizing kvmCPU, I'd just add hvfCPU to qemuCaps. It might have a good side effect that libvirt will be able to support multiple accelerators on the same platform.