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....@citrix.com>
>>> ---
>>> 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
