[PATCH 0/2] error-reporting for query-sev-capabilities

[PATCH 0/2] error-reporting for query-sev-capabilities

[Paolo Bonzini]

In some cases, such as if the kvm-amd "sev" module parameter is set
to 0, SEV will be unavailable but query-sev-capabilities will still
return all the information.  This tricks libvirt into erroneously
reporting that SEV is available.  This series checks for the actual
usability of the feature and returns the appropriate error if QEMU
cannot use KVM or KVM cannot use SEV.

Because query-sev-capabilities's error reporting was abysmal, we
first have to fix it up (patch 1).  Curiously that removes more
code than it adds.

Paolo


Paolo Bonzini (2):
  target-i386: sev: provide proper error reporting for query-sev-capabilities
  target-i386: sev: fail query-sev-capabilities if QEMU cannot use SEV

 target/i386/monitor.c  | 10 +---------
 target/i386/sev-stub.c |  3 ++-
 target/i386/sev.c      | 27 ++++++++++++++++++---------
 target/i386/sev_i386.h |  2 +-
 4 files changed, 22 insertions(+), 20 deletions(-)

-- 
2.26.2

[PATCH 1/2] target-i386: sev: provide proper error reporting for query-sev-capabilities

[Paolo Bonzini]

The query-sev-capabilities was reporting errors through error_report;
change it to use Error** so that the cause of the failure is clearer.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target/i386/monitor.c  | 10 +---------
 target/i386/sev-stub.c |  3 ++-
 target/i386/sev.c      | 18 +++++++++---------
 target/i386/sev_i386.h |  2 +-
 4 files changed, 13 insertions(+), 20 deletions(-)

diff --git a/target/i386/monitor.c b/target/i386/monitor.c
index 27ebfa3ad2..7abae3c8df 100644
--- a/target/i386/monitor.c
+++ b/target/i386/monitor.c
@@ -726,13 +726,5 @@ SevLaunchMeasureInfo *qmp_query_sev_launch_measure(Error **errp)
 
 SevCapability *qmp_query_sev_capabilities(Error **errp)
 {
-    SevCapability *data;
-
-    data = sev_get_capabilities();
-    if (!data) {
-        error_setg(errp, "SEV feature is not available");
-        return NULL;
-    }
-
-    return data;
+    return sev_get_capabilities(errp);
 }
diff --git a/target/i386/sev-stub.c b/target/i386/sev-stub.c
index e5ee13309c..88e3f39a1e 100644
--- a/target/i386/sev-stub.c
+++ b/target/i386/sev-stub.c
@@ -44,7 +44,8 @@ char *sev_get_launch_measurement(void)
     return NULL;
 }
 
-SevCapability *sev_get_capabilities(void)
+SevCapability *sev_get_capabilities(Error **errp)
 {
+    error_setg(errp, "SEV is not available in this QEMU");
     return NULL;
 }
diff --git a/target/i386/sev.c b/target/i386/sev.c
index d273174ad3..70f9ee026f 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -399,7 +399,7 @@ sev_get_info(void)
 
 static int
 sev_get_pdh_info(int fd, guchar **pdh, size_t *pdh_len, guchar **cert_chain,
-                 size_t *cert_chain_len)
+                 size_t *cert_chain_len, Error **errp)
 {
     guchar *pdh_data = NULL;
     guchar *cert_chain_data = NULL;
@@ -410,8 +410,8 @@ sev_get_pdh_info(int fd, guchar **pdh, size_t *pdh_len, guchar **cert_chain,
     r = sev_platform_ioctl(fd, SEV_PDH_CERT_EXPORT, &export, &err);
     if (r < 0) {
         if (err != SEV_RET_INVALID_LEN) {
-            error_report("failed to export PDH cert ret=%d fw_err=%d (%s)",
-                         r, err, fw_error_to_str(err));
+            error_setg(errp, "failed to export PDH cert ret=%d fw_err=%d (%s)",
+                       r, err, fw_error_to_str(err));
             return 1;
         }
     }
@@ -423,8 +423,8 @@ sev_get_pdh_info(int fd, guchar **pdh, size_t *pdh_len, guchar **cert_chain,
 
     r = sev_platform_ioctl(fd, SEV_PDH_CERT_EXPORT, &export, &err);
     if (r < 0) {
-        error_report("failed to export PDH cert ret=%d fw_err=%d (%s)",
-                     r, err, fw_error_to_str(err));
+        error_setg(errp, "failed to export PDH cert ret=%d fw_err=%d (%s)",
+                   r, err, fw_error_to_str(err));
         goto e_free;
     }
 
@@ -441,7 +441,7 @@ e_free:
 }
 
 SevCapability *
-sev_get_capabilities(void)
+sev_get_capabilities(Error **errp)
 {
     SevCapability *cap = NULL;
     guchar *pdh_data = NULL;
@@ -452,13 +452,13 @@ sev_get_capabilities(void)
 
     fd = open(DEFAULT_SEV_DEVICE, O_RDWR);
     if (fd < 0) {
-        error_report("%s: Failed to open %s '%s'", __func__,
-                     DEFAULT_SEV_DEVICE, strerror(errno));
+        error_setg_errno(errp, errno, "Failed to open %s",
+                         DEFAULT_SEV_DEVICE);
         return NULL;
     }
 
     if (sev_get_pdh_info(fd, &pdh_data, &pdh_len,
-                         &cert_chain_data, &cert_chain_len)) {
+                         &cert_chain_data, &cert_chain_len, errp)) {
         goto out;
     }
 
diff --git a/target/i386/sev_i386.h b/target/i386/sev_i386.h
index 8eb7de1bef..4db6960f60 100644
--- a/target/i386/sev_i386.h
+++ b/target/i386/sev_i386.h
@@ -34,6 +34,6 @@ extern SevInfo *sev_get_info(void);
 extern uint32_t sev_get_cbit_position(void);
 extern uint32_t sev_get_reduced_phys_bits(void);
 extern char *sev_get_launch_measurement(void);
-extern SevCapability *sev_get_capabilities(void);
+extern SevCapability *sev_get_capabilities(Error **errp);
 
 #endif
-- 
2.26.2

[PATCH 2/2] target-i386: sev: fail query-sev-capabilities if QEMU cannot use SEV

[Paolo Bonzini]

In some cases, such as if the kvm-amd "sev" module parameter is set
to 0, SEV will be unavailable but query-sev-capabilities will still
return all the information.  This tricks libvirt into erroneously
reporting that SEV is available.  Check the actual usability of the
feature and return the appropriate error if QEMU cannot use KVM
or KVM cannot use SEV.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target/i386/sev.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/target/i386/sev.c b/target/i386/sev.c
index 70f9ee026f..22194b3e32 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -450,6 +450,15 @@ sev_get_capabilities(Error **errp)
     uint32_t ebx;
     int fd;
 
+    if (!kvm_enabled()) {
+        error_setg(errp, "KVM not enabled\n");
+        return NULL;
+    }
+    if (kvm_vm_ioctl(kvm_state, KVM_MEMORY_ENCRYPT_OP, NULL) < 0) {
+        error_setg(errp, "SEV is not enabled\n");
+        return NULL;
+    }
+
     fd = open(DEFAULT_SEV_DEVICE, O_RDWR);
     if (fd < 0) {
         error_setg_errno(errp, errno, "Failed to open %s",
-- 
2.26.2

Re: [PATCH 0/2] error-reporting for query-sev-capabilities

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200630154521.552874-1-pbonzini@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [PATCH 0/2] error-reporting for query-sev-capabilities
Type: series
Message-id: 20200630154521.552874-1-pbonzini@redhat.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20200630154521.552874-1-pbonzini@redhat.com -> patchew/20200630154521.552874-1-pbonzini@redhat.com
Switched to a new branch 'test'
a8be190 target-i386: sev: fail query-sev-capabilities if QEMU cannot use SEV
15c69ee target-i386: sev: provide proper error reporting for query-sev-capabilities

=== OUTPUT BEGIN ===
1/2 Checking commit 15c69ee08893 (target-i386: sev: provide proper error reporting for query-sev-capabilities)
2/2 Checking commit a8be190ec95d (target-i386: sev: fail query-sev-capabilities if QEMU cannot use SEV)
ERROR: Error messages should not contain newlines
#26: FILE: target/i386/sev.c:454:
+        error_setg(errp, "KVM not enabled\n");

ERROR: Error messages should not contain newlines
#30: FILE: target/i386/sev.c:458:
+        error_setg(errp, "SEV is not enabled\n");

total: 2 errors, 0 warnings, 15 lines checked

Patch 2/2 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200630154521.552874-1-pbonzini@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH 1/2] target-i386: sev: provide proper error reporting for query-sev-capabilities

[Eric Blake]

On 6/30/20 10:45 AM, Paolo Bonzini wrote:
> The query-sev-capabilities was reporting errors through error_report;
> change it to use Error** so that the cause of the failure is clearer.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 2/2] target-i386: sev: fail query-sev-capabilities if QEMU cannot use SEV

[Eric Blake]

On 6/30/20 10:45 AM, Paolo Bonzini wrote:
> In some cases, such as if the kvm-amd "sev" module parameter is set
> to 0, SEV will be unavailable but query-sev-capabilities will still
> return all the information.  This tricks libvirt into erroneously
> reporting that SEV is available.  Check the actual usability of the
> feature and return the appropriate error if QEMU cannot use KVM
> or KVM cannot use SEV.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   target/i386/sev.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 70f9ee026f..22194b3e32 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -450,6 +450,15 @@ sev_get_capabilities(Error **errp)
>       uint32_t ebx;
>       int fd;
>   
> +    if (!kvm_enabled()) {
> +        error_setg(errp, "KVM not enabled\n");
> +        return NULL;
> +    }
> +    if (kvm_vm_ioctl(kvm_state, KVM_MEMORY_ENCRYPT_OP, NULL) < 0) {
> +        error_setg(errp, "SEV is not enabled\n");

Patchew was correct: drop the two \n.  With that fix,

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 2/2] target-i386: sev: fail query-sev-capabilities if QEMU cannot use SEV

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> In some cases, such as if the kvm-amd "sev" module parameter is set
> to 0, SEV will be unavailable but query-sev-capabilities will still
> return all the information.  This tricks libvirt into erroneously
> reporting that SEV is available.  Check the actual usability of the
> feature and return the appropriate error if QEMU cannot use KVM
> or KVM cannot use SEV.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  target/i386/sev.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 70f9ee026f..22194b3e32 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -450,6 +450,15 @@ sev_get_capabilities(Error **errp)
>      uint32_t ebx;
>      int fd;
>  
> +    if (!kvm_enabled()) {
> +        error_setg(errp, "KVM not enabled\n");
> +        return NULL;
> +    }
> +    if (kvm_vm_ioctl(kvm_state, KVM_MEMORY_ENCRYPT_OP, NULL) < 0) {
> +        error_setg(errp, "SEV is not enabled\n");

Can you make that 'SEV is not enabled in KVM' so it's obvious
that it's the KVM side and not the qemu side (like you've
done in the previous patch).

Dave

> +        return NULL;
> +    }
> +
>      fd = open(DEFAULT_SEV_DEVICE, O_RDWR);
>      if (fd < 0) {
>          error_setg_errno(errp, errno, "Failed to open %s",
> -- 
> 2.26.2
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 2/2] target-i386: sev: fail query-sev-capabilities if QEMU cannot use SEV

[Paolo Bonzini]

On 03/07/20 18:08, Dr. David Alan Gilbert wrote:
>> +        error_setg(errp, "KVM not enabled\n");
>> +        return NULL;
>> +    }
>> +    if (kvm_vm_ioctl(kvm_state, KVM_MEMORY_ENCRYPT_OP, NULL) < 0) {
>> +        error_setg(errp, "SEV is not enabled\n");
> Can you make that 'SEV is not enabled in KVM' so it's obvious
> that it's the KVM side and not the qemu side (like you've
> done in the previous patch).


Sure.

Paolo