[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2 3/4] x86: Allow non-faulting accesses to non-emulated MSRs if policy permits this
On 20.01.2021 23:49, Boris Ostrovsky wrote: > --- a/xen/arch/x86/msr.c > +++ b/xen/arch/x86/msr.c > @@ -164,6 +164,34 @@ int init_vcpu_msr_policy(struct vcpu *v) > return 0; > } > > +/* Returns true if policy requires #GP to the guest. */ > +bool guest_unhandled_msr(const struct vcpu *v, uint32_t msr, uint64_t *val, > + bool is_write, bool is_guest_msr_access) Would you mind dropping the "_msr" infix from the last parameter's name? We're anyway aware we're talking about MSR accesses here, from the function name. As a nit - while I'm okay with the uint64_t *, I think the uint32_t would better be unsigned int - see ./CODING_STYLE. Finally, this being a policy function and policy being per- domain here, I think the first parameter wants to be const struct domain *. > +{ > + u8 ignore_msrs = v->domain->arch.msr->ignore_msrs; uint8_t please or, as per above, even better unsigned int. > + > + /* > + * Accesses to unimplemented MSRs as part of emulation of instructions > + * other than guest's RDMSR/WRMSR should never succeed. > + */ > + if ( !is_guest_msr_access ) > + ignore_msrs = MSR_UNHANDLED_NEVER; Wouldn't you better "return true" here? Such accesses also shouldn't be logged imo (albeit I agree that's a change from current behavior). > + if ( unlikely(ignore_msrs != MSR_UNHANDLED_NEVER) ) > + *val = 0; I don't understand the conditional here, even more so with the respective changelog entry. In any event you don't want to clobber the value ahead of ... > + if ( likely(ignore_msrs != MSR_UNHANDLED_SILENT) ) > + { > + if ( is_write ) > + gdprintk(XENLOG_WARNING, "WRMSR 0x%08x val 0x%016"PRIx64 > + " unimplemented\n", msr, *val); ... logging it. > --- a/xen/arch/x86/x86_emulate/x86_emulate.h > +++ b/xen/arch/x86/x86_emulate/x86_emulate.h > @@ -850,4 +850,10 @@ static inline void x86_emul_reset_event(struct > x86_emulate_ctxt *ctxt) > ctxt->event = (struct x86_event){}; > } > > +static inline bool x86_emul_guest_msr_access(struct x86_emulate_ctxt *ctxt) The parameter wants to be pointer-to-const. In addition I wonder whether this wouldn't better be a sibling to x86_insn_is_cr_access() (without a "state" parameter, which would be unused and unavailable to the callers), which may end up finding further uses down the road. > +{ > + return ctxt->opcode == X86EMUL_OPC(0x0f, 0x32) || /* RDMSR */ > + ctxt->opcode == X86EMUL_OPC(0x0f, 0x30); /* WRMSR */ > +} Personally I'd prefer if this was a single comparison: return (ctxt->opcode | 2) == X86EMUL_OPC(0x0f, 0x32); But maybe nowadays' compilers are capable of this transformation? I notice you use this function only from PV priv-op emulation. What about the call paths through hvmemul_{read,write}_msr()? (It's also questionable whether the write paths need this - the only MSR written outside of WRMSR emulation is MSR_SHADOW_GS_BASE, which can't possibly reach the "unhandled" logic anywhere. But maybe better to be future proof here in case new MSR writes appear in the emulator, down the road.) Jan
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |