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

[Jan Beulich <jbeulich@xxxxxxxx>]

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? Preferably with the adjustment
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

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

Jan