Re: [PATCH] x86/hvm: Propagate real error information up through hvm_load()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 19/07/2021 13:46, Jan Beulich wrote:
> On 19.07.2021 13:14, Andrew Cooper wrote:
>> hvm_load() is currently a mix of -errno and -1 style error handling, which
>> aliases -EPERM.  This leads to the following confusing diagnostics:
>>
>> From userspace:
>>   xc: info: Restoring domain
>>   xc: error: Unable to restore HVM context (1 = Operation not permitted): 
>> Internal error
>>   xc: error: Restore failed (1 = Operation not permitted): Internal error
>>   xc_domain_restore: [1] Restore failed (1 = Operation not permitted)
>>
>> From Xen:
>>   (XEN) HVM10.0 restore: inconsistent xsave state (feat=0x2ff accum=0x21f 
>> xcr0=0x7 bv=0x3 err=-22)
>>   (XEN) HVM10 restore: failed to load entry 16/0
>>
>> The actual error was a bad backport, but the -EINVAL got converted to -EPERM
>> on the way out of the hypercall.
>>
>> The overwhelming majority of *_load() handlers already use -errno 
>> consistenty.
>> Fix up the rest to be consistent, and fix a few other errors noticed along 
>> the
>> way.
>>
>>  * Failures of hvm_load_entry() indicate a truncated record or other bad data
>>    size.  Use -ENODATA.
> But then ...
>
>> @@ -421,8 +421,8 @@ static int pit_load(struct domain *d, 
>> hvm_domain_context_t *h)
>>  
>>      if ( hvm_load_entry(PIT, h, &pit->hw) )
>>      {
>> -        spin_unlock(&pit->lock);
>> -        return 1;
>> +        rc = -ENODEV;
>> +        goto out;
>>      }
> ... use -ENODATA here as well?

Hmm - that was intended to be ENODATA.  Will fix.

>  Preferably with the adjustment
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Thanks,

> I'll pick this up for backporting once I see it in the tree.

I don't know how much the call tree has changed over time.  Every
handler will need a quick check on each release.

~Andrew