Re: [PATCH] target/s390x/kvm/pv: Provide some more useful information if decryption fails

[Thomas Huth]

On 09/01/2024 17.51, Cédric Le Goater wrote:


On 1/9/24 15:30, Thomas Huth wrote:
It's a common scenario to copy guest images from one host to another
to run the guest on the other machine. This (of course) does not work
with "secure exection" guests since they are encrypted with one certain
host key. However, if you still (accidentally) do it, you only get a
very user-unfriendly error message that looks like this:

  qemu-system-s390x: KVM PV command 2 (KVM_PV_SET_SEC_PARMS) failed:
   header rc 108 rrc 5 IOCTL rc: -22

Let's provide at least a somewhat nicer hint to the users so that they
are able to figure out what might have gone wrong.

Buglink: https://issues.redhat.com/browse/RHEL-18212
Signed-off-by: Thomas Huth <th...@redhat.com>
---
  target/s390x/kvm/pv.c | 20 ++++++++++++++++----
  1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/target/s390x/kvm/pv.c b/target/s390x/kvm/pv.c
index 6a69be7e5c..2833a255fa 100644
--- a/target/s390x/kvm/pv.c
+++ b/target/s390x/kvm/pv.c
@@ -29,7 +29,8 @@ static bool info_valid;
  static struct kvm_s390_pv_info_vm info_vm;
  static struct kvm_s390_pv_info_dump info_dump;
-static int __s390_pv_cmd(uint32_t cmd, const char *cmdname, void *data)
+static int __s390_pv_cmd(uint32_t cmd, const char *cmdname, void *data,
+                         int *pvrc)
  {
      struct kvm_pv_cmd pv_cmd = {
          .cmd = cmd,
@@ -46,6 +47,9 @@ static int __s390_pv_cmd(uint32_t cmd, const char 
*cmdname, void *data)
                       "IOCTL rc: %d", cmd, cmdname, pv_cmd.rc, pv_cmd.rrc,
                       rc);
      }
+    if (pvrc) {
+        *pvrc = pv_cmd.rc;
+    }
      return rc;
  }
@@ -53,12 +57,13 @@ static int __s390_pv_cmd(uint32_t cmd, const char 
*cmdname, void *data)
   * This macro lets us pass the command as a string to the function so
   * we can print it on an error.
   */
-#define s390_pv_cmd(cmd, data) __s390_pv_cmd(cmd, #cmd, data)
+#define s390_pv_cmd(cmd, data) __s390_pv_cmd(cmd, #cmd, data, NULL)
+#define s390_pv_cmd_pvrc(cmd, data, pvrc) __s390_pv_cmd(cmd, #cmd, data, 
pvrc)
  #define s390_pv_cmd_exit(cmd, data)    \
  {                                      \
      int rc;                            \
                                         \
-    rc = __s390_pv_cmd(cmd, #cmd, data);\
+    rc = __s390_pv_cmd(cmd, #cmd, data, NULL); \
      if (rc) {                          \
          exit(1);                       \
      }                                  \
@@ -144,12 +149,19 @@ bool 
s390_pv_vm_try_disable_async(S390CcwMachineState *ms)
  int s390_pv_set_sec_parms(uint64_t origin, uint64_t length)
  {
+    int ret, pvrc;
      struct kvm_s390_pv_sec_parm args = {
          .origin = origin,
          .length = length,
      };
-    return s390_pv_cmd(KVM_PV_SET_SEC_PARMS, &args);
+    ret = s390_pv_cmd_pvrc(KVM_PV_SET_SEC_PARMS, &args, &pvrc);
+    if (ret && pvrc == 0x108) {

why do we need to test for 0x108 also ? if this sub error code is important,
adding a define would be a plus.

As far as I understood, other codes here could indicate a different failure, 
so I wanted to make sure to limit the text to this scenario only.
And AFAIK the error codes are something internal to the ultravisor, not 
documented publicly, so coming up with a #define here sounds hard to me.

+        error_report("Can't set secure parameters, please check whether "
+                     "the image is correctly encrypted for this host");
The error reporting in s390x_machine_protect() could be improved.

I would add a 'Error *' argument to the routines called by
s390x_machine_protect() and report the error in s390x_machine_protect()
or above. s390_machine_protect() return value is ignored also, could be
replaced by a bool.

Ok, I'll give it a try and send a v2.

 Thomas