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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Oct 08, 2024 at 08:29:23AM +0200, Jan Beulich wrote:
> On 07.10.2024 17:32, Roger Pau Monné wrote:
> > On Mon, Oct 07, 2024 at 03:16:47PM +0100, Andrew Cooper wrote:
> >> 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?
> > 
> > Sure, I think that would be XEN_DOMCTL_set_vcpu_msrs?
> > 
> > I've noticed that such hypercall doesn't return an error if the MSR is
> > not handled by Xen (there's no default case returning an error in the
> > switch that processes the entries to load).
> 
> I see
> 
>                 ret = -EINVAL;
>                 ...
>                 switch ( msr.index )
>                 {
>                     ...
>                     if ( guest_wrmsr(v, msr.index, msr.value) != X86EMUL_OKAY 
> )
>                         break;
>                     continue;
>                 }
>                 break;
> 
> which to me means we'll return -EINVAL both when handling an MSR fails (1st
> "break") and when encountering an unhandled MSR (2nd "break").

Oh, I see, I was expecting a construct similar to the one used in
hvm_load_cpu_msrs() and didn't spot that continue.  The logic there is
very obfuscated IMO.

> >>> --- 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.
> > 
> > I also wanted to use %pv here, but it will get out of sync
> > (style-wise) with the rest of messages of the HVM context loading
> > logic?  IOW: my preference would be to switch all in one go.
> 
> I deliberately started using %pv when touching hvm_save() somewhat recently.
> So there is some inconsistency right now anyway, and I guess we'll want to
> move to the new form as we touch code in this area.

Ack, will adjust to use "HVM %pv" then.

Thanks, Roger.