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

[Roman Bolshakov]

On Wed, Nov 21, 2018 at 05:04:07PM +0100, Jiri Denemark wrote:
> On Wed, Nov 21, 2018 at 17:01:44 +0300, Roman Bolshakov wrote:
> > From: Roman Bolshakov <roolebo at gmail.com>
> > 
> > virQEMUCapsFormatCache/virQEMUCapsLoadCache adds/reads KVM CPUs to/from
> > capabilities cache regardless of QEMU_CAPS_KVM. That can cause undesired
> > side-effects when KVM CPUs are present in the cache on a platform that
> > doesn't support it, e.g. macOS or Linux without KVM support.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange at redhat.com>
> > Signed-off-by: Roman Bolshakov <roolebo at gmail.com>
> 
> This doesn't look like a patch written by Daniel so why did you include
> the Signed-off-by line? Or did I miss anything?

Daniel kindly helped to root cause an issue I had with
qemucapabilitiestest in v1:
https://www.redhat.com/archives/libvir-list/2018-November/msg00740.html

and provided a diff that resolves the issue:
https://www.redhat.com/archives/libvir-list/2018-November/msg00767.html

Should I remove his Signed-off-by tag?

> 
> > ---
> >  src/qemu/qemu_capabilities.c | 18 ++++++++++++------
> >  1 file changed, 12 insertions(+), 6 deletions(-)
> > 
> > diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> > index fde27010e4..4ba8369e3a 100644
> > --- a/src/qemu/qemu_capabilities.c
> > +++ b/src/qemu/qemu_capabilities.c
> > @@ -3467,11 +3467,13 @@ virQEMUCapsLoadCache(virArch hostArch,
> >      }
> >      VIR_FREE(str);
> >  
> > -    if (virQEMUCapsLoadHostCPUModelInfo(qemuCaps, ctxt, VIR_DOMAIN_VIRT_KVM) < 0 ||
> > +    if ((virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM) &&
> > +         virQEMUCapsLoadHostCPUModelInfo(qemuCaps, ctxt, VIR_DOMAIN_VIRT_KVM) < 0) ||
> >          virQEMUCapsLoadHostCPUModelInfo(qemuCaps, ctxt, VIR_DOMAIN_VIRT_QEMU) < 0)
> >          goto cleanup;
> 
> I don't think we should introduce these guards in all the places. All
> the loading and formatting functions should return success if the
> appropriate info is not available, so you should just make sure the
> relevant info is NULL in qemuCaps.
> 

Do you mean the capabilities checks should be moved inside the
functions? Either way they're needed to avoid loading KVM cpus into QEMU
caps cache on the hosts without KVM support.

> > @@ -3584,7 +3586,8 @@ virQEMUCapsLoadCache(virArch hostArch,
> >      if (virQEMUCapsParseSEVInfo(qemuCaps, ctxt) < 0)
> >          goto cleanup;
> >  
> > -    virQEMUCapsInitHostCPUModel(qemuCaps, hostArch, VIR_DOMAIN_VIRT_KVM);
> > +    if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM))
> > +      virQEMUCapsInitHostCPUModel(qemuCaps, hostArch, VIR_DOMAIN_VIRT_KVM);
> 
> Please, follow our coding style, i.e., indent by 4 spaces.
> 
> >      virQEMUCapsInitHostCPUModel(qemuCaps, hostArch, VIR_DOMAIN_VIRT_QEMU);
> >  
> >      ret = 0;
> ...

Will do, thank you for catching this!

Best regards,
Roman