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

[Jan Beulich <jbeulich@xxxxxxxx>]

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").

>>> --- 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.

Jan