[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH] x86/msr: add log messages to MSR state load error paths
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |