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

[Thomas Huth <thuth@redhat.com>]

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 <thuth@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