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

[Roman Bolshakov]

On Fri, Nov 23, 2018 at 09:46:36PM +0300, Roman Bolshakov wrote:
> 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
> 

Hi Jiri,

That's a kind reminder.

Thank you,
Roman