Re: [PATCH] x86/msr: add log messages to MSR state load error paths

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 07/10/2024 3:03 pm, Roger Pau Monne wrote:
> Some error paths in the MSR state loading logic don't contain error messages,
> which makes debugging them quite hard without adding extra patches to print 
> the
> information.
>
> Add two new log messages to the MSR state load path that print information
> about the entry that failed to load.
>
> No functional change intended.
>
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
>  xen/arch/x86/hvm/hvm.c | 9 +++++++++

Can we fix the PV side at the same time too?

>  1 file changed, 9 insertions(+)
>
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 69a25571db8d..c71087f636c4 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -1598,10 +1598,19 @@ static int cf_check hvm_load_cpu_msrs(struct domain 
> *d, hvm_domain_context_t *h)
>              rc = guest_wrmsr(v, ctxt->msr[i].index, ctxt->msr[i].val);
>  
>              if ( rc != X86EMUL_OKAY )
> +            {
> +                printk(XENLOG_G_ERR
> +                       "HVM%d.%d load MSR %#x with value %#lx failed: %d\n",
> +                       d->domain_id, vcpuid, ctxt->msr[i].index,
> +                       ctxt->msr[i].val, rc);

Just %pv please.  I don't want to propagate the (occasionally ambiguous)
HVM%d form.

Also, rc may not be great to render.  It's an X86EMUL_*, not an errno.

And saying that, we have a discontinuity between PV and HVM.  PV
translates !OKAY into -EINVAL, whereas HVM translates into -ENXIO.  /sigh

~Andrew