[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 3/4] x86: Allow non-faulting accesses to non-emulated MSRs if policy permits this
On 07.01.2021 21:34, Boris Ostrovsky wrote: > Starting with commit 84e848fd7a16 ("x86/hvm: disallow access to unknown MSRs") > accesses to unhandled MSRs result in #GP sent to the guest. This caused a > regression for Solaris who tries to acccess MSR_RAPL_POWER_UNIT and (unlike, Nit: One c too many. > for example, Linux) does not catch exceptions when accessing MSRs that > potentially may not be present. Just to re-raise the question raised by Andrew already earlier on: Has Solaris been fixed in the meantime, or is this at least firmly planned to happen? > --- a/xen/arch/x86/hvm/svm/svm.c > +++ b/xen/arch/x86/hvm/svm/svm.c > @@ -1965,8 +1965,8 @@ static int svm_msr_read_intercept(unsigned int msr, > uint64_t *msr_content) > break; > > default: > - gdprintk(XENLOG_WARNING, "RDMSR 0x%08x unimplemented\n", msr); > - goto gpf; > + if ( guest_unhandled_msr(v, msr, msr_content, false) ) > + goto gpf; > } > > HVM_DBG_LOG(DBG_LEVEL_MSR, "returns: ecx=%x, msr_value=%"PRIx64, > @@ -2151,10 +2151,8 @@ static int svm_msr_write_intercept(unsigned int msr, > uint64_t msr_content) > break; > > default: > - gdprintk(XENLOG_WARNING, > - "WRMSR 0x%08x val 0x%016"PRIx64" unimplemented\n", > - msr, msr_content); > - goto gpf; > + if ( guest_unhandled_msr(v, msr, &msr_content, true) ) > + goto gpf; > } > > return X86EMUL_OKAY; > --- a/xen/arch/x86/hvm/vmx/vmx.c > +++ b/xen/arch/x86/hvm/vmx/vmx.c > @@ -3017,8 +3017,8 @@ static int vmx_msr_read_intercept(unsigned int msr, > uint64_t *msr_content) > break; > } > > - gdprintk(XENLOG_WARNING, "RDMSR 0x%08x unimplemented\n", msr); > - goto gp_fault; > + if ( guest_unhandled_msr(curr, msr, msr_content, false) ) > + goto gp_fault; > } > > done: > @@ -3319,10 +3319,8 @@ static int vmx_msr_write_intercept(unsigned int msr, > uint64_t msr_content) > is_last_branch_msr(msr) ) > break; > > - gdprintk(XENLOG_WARNING, > - "WRMSR 0x%08x val 0x%016"PRIx64" unimplemented\n", > - msr, msr_content); > - goto gp_fault; > + if ( guest_unhandled_msr(v, msr, &msr_content, true) ) > + goto gp_fault; > } > > return X86EMUL_OKAY; These functions also get used from the insn emulator, when it needs to fetch an MSR value (not necessarily in the context of emulating RDMSR or WRMSR). I wonder whether applying this behavior in that case is actually correct. It would only be if we would settle on it being a requirement that any such MSRs have to have emulation present in one of the handlers. > --- a/xen/arch/x86/msr.c > +++ b/xen/arch/x86/msr.c > @@ -164,6 +164,26 @@ 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) > +{ > + const struct msr_policy *mp = v->domain->arch.msr; > + > + if ( unlikely(mp->ignore_msrs != MSR_UNHANDLED_NEVER) && !is_write ) > + *val = 0; I'd recommend to drop the left side of the && - there's no harm in storing zero in this extra case. > + if ( likely(mp->ignore_msrs != MSR_UNHANDLED_SILENT) ) { Nit: Brace on its own line please. > --- a/xen/include/asm-x86/msr.h > +++ b/xen/include/asm-x86/msr.h > @@ -345,5 +345,6 @@ int init_vcpu_msr_policy(struct vcpu *v); > */ > int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val); > int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val); > - > +bool guest_unhandled_msr(const struct vcpu *v, uint32_t msr, > + uint64_t *val, bool is_write); > #endif /* __ASM_MSR_H */ Please retain the blank line that was there. Jan
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |