Re: [libvirt] [PATCH 3/4] qemu: Fix probing of AMD SEV support
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
> > ---
> > 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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/4] qemu: Fix probing of AMD SEV support
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
> ---
> 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.
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 3/4] qemu: Fix probing of AMD SEV support
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
---
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 */
+if (qemuMonitorJSONHasError(reply, "GenericError"))
goto cleanup;
-}
if (qemuMonitorJSONCheckError(cmd, reply) < 0)
goto cleanup;
diff --git a/tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml
b/tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml
index efddcbc6a5..2b47337449 100644
--- a/tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml
@@ -211,7 +211,6 @@
-
--
2.14.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
