[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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.

--
Roman


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]