[libvirt] [PATCH 3/4] qemu: Fix probing of AMD SEV support

[Erik Skultety]

On Wed, Aug 15, 2018 at 06:06:26PM +0200, Peter Krempa wrote:
> On Wed, Aug 15, 2018 at 17:02:07 +0200, Erik Skultety wrote:
> > So the procedure to detect SEV support works like this:
> > 1) we detect that sev-guest is among the QOM types and set the cap flag
> > 2) we probe the monitor for SEV support
> >     - this is tricky, because QEMU with compiled SEV support will always
> >     report -object sev-guest and query-sev-capabilities command, that
> >     however doesn't mean SEV is supported
> > 3) depending on what the monitor returned, we either keep or clear the
> > capability flag for SEV
> >
> > Commit a349c6c21c6 added an explicit check for "GenericError" in the
> > monitor reply to prevent libvirtd to spam logs about missing
> > 'query-sev-capabilities' command. At the same time though, it returned
> > success in this case which means that we didn't clear the capability
> > flag afterwards and happily formatted SEV into qemuCaps.
> >
> > Signed-off-by: Erik Skultety <eskultet at redhat.com>
> > ---
> >  src/qemu/qemu_monitor_json.c                     | 9 +++++----
> >  tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml | 1 -
> >  2 files changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> > index 3f99f39120..b0963ed887 100644
> > --- a/src/qemu/qemu_monitor_json.c
> > +++ b/src/qemu/qemu_monitor_json.c
> > @@ -6459,11 +6459,12 @@ qemuMonitorJSONGetSEVCapabilities(qemuMonitorPtr mon,
> >          goto cleanup;
> >
> >      /* Both -object sev-guest and query-sev-capabilities can be present
> > -     * even if SEV is not available */
> > -    if (qemuMonitorJSONHasError(reply, "GenericError")) {
> > -        ret = 0;
> > +     * even if SEV is not available. We have to check for "GenericError" first,
> > +     * in order not to spam libvirtd logs.
> > +     * NOTE: We return failure here too so that the capability gets cleared
> > +     * later */
>
> This should be noted in a comment for the function as for this specific
> case this function will not report an error but will for any other case.
>
> It's also partially weird since the function also reports other errors
> besides allocation errors which any sane caller has to ignore.
>
> ACK if you add a comment block for this function describing this
> weirdness.
>
> You can also come up with a saner detection method e.g. to return a
> boolean via arguments or 1 via return value which says whether it's
> supported or not and reserve -1 for real errors.

^This sounds more plausible to me, so I'll respin with that, along with the
commentary describing the "weirdness".

Thanks,
Erik