[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 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.
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |