[libvirt PATCH 1/5] qemu_capabilities: detect if confidential-guest-support is available

[Pavel Hrdina]

On Mon, Jun 14, 2021 at 04:57:47PM +0200, Peter Krempa wrote:
> On Mon, Jun 14, 2021 at 16:13:40 +0200, Pavel Hrdina wrote:
> > On Fri, Jun 11, 2021 at 04:18:24PM +0200, Peter Krempa wrote:
> > > On Fri, Jun 11, 2021 at 15:24:09 +0200, Pavel Hrdina wrote:
> > > > In libvirt we already use `query-command-line-options` QMP command but
> > > > that is useless as it doesn't provide correct data for `-machine`
> > > > option. So we need a new and better way to get that data.
> > > > 
> > > > We already use `qom-list-properties` to get options for specific machine
> > > > types so we can reuse it to get options for special `none` machine type
> > > > as a generic arch independent machine type.
> > > > 
> > > > Signed-off-by: Pavel Hrdina <phrdina at redhat.com>
> > > 
> > > 
> > > [...]
> > > 
> > > > 
> > > > diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> > > > index 2b9ab9af60..436fe40f65 100644
> > > > --- a/src/qemu/qemu_capabilities.c
> > > > +++ b/src/qemu/qemu_capabilities.c
> > > > @@ -632,6 +632,9 @@ VIR_ENUM_IMPL(virQEMUCaps,
> > > >                "input-linux",
> > > >                "virtio-gpu-gl-pci",
> > > >                "virtio-vga-gl",
> > > > +
> > > > +              /* 405 */
> > > > +              "confidential-guest-support",
> > > >      );
> > > >  
> > > >  
> > > > @@ -1756,6 +1759,10 @@ static struct virQEMUCapsStringFlags virQEMUCapsMachinePropsVirt[] = {
> > > >      { "iommu", QEMU_CAPS_MACHINE_VIRT_IOMMU },
> > > >  };
> > > >  
> > > > +static struct virQEMUCapsStringFlags virQEMUCapsMachinePropsGeneric[] = {
> > > > +    { "confidential-guest-support", QEMU_CAPS_MACHINE_CONFIDENTAL_GUEST_SUPPORT },
> > > 
> > > If possible please include this line and all the detection that happens
> > > from it in a separate commit including the definition of the new
> > > capability flag.
> > > 
> > > > +};
> > > > +
> > > >  static virQEMUCapsObjectTypeProps virQEMUCapsMachineProps[] = {
> > > >      { "pseries", virQEMUCapsMachinePropsPSeries,
> > > >        G_N_ELEMENTS(virQEMUCapsMachinePropsPSeries),
> > > > @@ -1763,6 +1770,9 @@ static virQEMUCapsObjectTypeProps virQEMUCapsMachineProps[] = {
> > > >      { "virt", virQEMUCapsMachinePropsVirt,
> > > >        G_N_ELEMENTS(virQEMUCapsMachinePropsVirt),
> > > >        -1 },
> > > > +    { "none", virQEMUCapsMachinePropsGeneric,
> > > > +      G_N_ELEMENTS(virQEMUCapsMachinePropsGeneric),
> > > > +      -1 },
> > > >  };
> > > >  
> > > >  static void
> > > > @@ -2865,8 +2875,10 @@ virQEMUCapsProbeQMPMachineProps(virQEMUCaps *qemuCaps,
> > > >          const char *canon = virQEMUCapsGetCanonicalMachine(qemuCaps, virtType, props.type);
> > > >          g_autofree char *type = NULL;
> > > >  
> > > > -        if (!virQEMUCapsIsMachineSupported(qemuCaps, virtType, canon))
> > > > +        if (STRNEQ(canon, "none") &&
> > > > +            !virQEMUCapsIsMachineSupported(qemuCaps, virtType, canon)) {
> > > >              continue;
> > > 
> > > This looks fishy since after this patch we'll ever only skip the 'none'
> > > machine, whereas previously we'd let get virQEMUCapsIsMachineSupported
> > > called.
> > 
> > No true, see the following matrix:
> > 
> >     A (canon != "none")
> >     B (!virQEMUCapsIsMachineSupported())
> > 
> >         A   B       A & B
> > 
> >         0   0         0
> >         0   1         0
> >         1   0         0
> >         1   1         1
> > 
> > The current code will skip the call only if `canon` is not `none` and
> > `canon` is not supported.
> > 
> > In all other cases we want to call to QEMU and get the machine type
> > props.
> > 
> > Essentially we want to do the call only if the machine type is "none"
> > or it is supported and skip it in all other cases which can be written
> > like this:
> > 
> >     if (!(STREQ(canon, "none") || virQEMUCapsIsMachineSupported()))
> > 
> > and with a bit of boolean algebra you will get
> > 
> >     if (!STREQ(canon, "none") && !virQEMUCapsIsMachineSupported())
> > 
> > where for !STREQ() we have STRNEQ().
> > 
> > So unless I'm missing something the condition is correct.
> 
> 
> Hmm, yeah. I've probably originally read it as
> 
> STREQ(..."none") && !virQEMUCapsIsMachineSupported
> 
> (STREQ vs STRNEQ)
> 
> In such case we'd only ever check and skip "none" machines which would
> be wrong, but yours is correct.

Sounds like it :) it's easy to miss it. Anyway, thanks for the review,
v2 should be posted to the list.

Pavel

> > 
> > Pavel
> > 
> > > > +        }
> > > >  
> > > >          /* The QOM type for machine types is the machine type name
> > > >           * followed by the -machine suffix */
> > > > diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.replies b/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.replies
> > > > index 884db53152..aa8209cc42 100644
> > > > --- a/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.replies
> > > > +++ b/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.replies
> > > > @@ -17523,9 +17523,122 @@
> > > >    "id": "libvirt-36"
> > > >  }
> > > >  
> > > > +{
> > > > +  "execute": "qom-list-properties",
> > > > +  "arguments": {
> > > > +    "typename": "none-machine"
> > > > +  },
> > > > +  "id": "libvirt-37"
> > > > +}
> > > 
> > > As noted above, I'd prefer if the update of replies is separated from
> > > addition of the capability.
> > > 
> 
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20210614/0bb8633e/attachment-0001.sig>