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

[Philippe Mathieu-Daudé]

On 10/1/24 15:29, 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

"execution"

> 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 <thuth@redhat.com>
> ---
>   v2: Print the error in s390_machine_protect() instead of doing it
>       in s390_pv_set_sec_parms(), report the text via Error **errp
>
>   hw/s390x/ipl.h             |  2 +-
>   target/s390x/kvm/pv.h      |  2 +-
>   hw/s390x/ipl.c             |  5 ++---
>   hw/s390x/s390-virtio-ccw.c |  5 ++++-
>   target/s390x/kvm/pv.c      | 25 ++++++++++++++++++++-----
>   5 files changed, 28 insertions(+), 11 deletions(-)


> diff --git a/target/s390x/kvm/pv.c b/target/s390x/kvm/pv.c
> index 6a69be7e5c..7ca7faec73 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;

Changing __s390_pv_cmd() is a bit more than improving error reporting,
so I'd rather see a preliminary commit. Either ways:

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> +    }
>       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);                       \
>       }                                  \