| Message ID | 1610051698-23675-4-git-send-email-boris.ostrovsky@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Permit fault-less access to non-emulated MSRs | expand |
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
On 1/8/21 10:18 AM, Jan Beulich wrote: > 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? I was told they will open a bug. > 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. Hmm.. Yes, I did not consider this. I am not convinced this will always result in correct behavior for the emulator so I will need to pass down a parameter. Unless there is a way to figure out whether we are running in the emulator (which I don't immediately see) -boris
On 08.01.2021 21:39, boris.ostrovsky@oracle.com wrote: > On 1/8/21 10:18 AM, Jan Beulich wrote: >> 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? > > I was told they will open a bug. "Will", not "did"? >> @@ -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. > > > Hmm.. Yes, I did not consider this. I am not convinced this will > always result in correct behavior for the emulator so I will > need to pass down a parameter. Unless there is a way to figure > out whether we are running in the emulator (which I don't > immediately see) Passing a parameter for this is sort of ugly, but I presume unavoidable. The more that what you need to know is not "running in emulator", but "guest RDMSR/WRMSR" - this can also happen through the emulator. Jan
On 1/11/21 2:48 AM, Jan Beulich wrote: > On 08.01.2021 21:39, boris.ostrovsky@oracle.com wrote: >> On 1/8/21 10:18 AM, Jan Beulich wrote: >> >>> >>> 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? >> I was told they will open a bug. > "Will", not "did"? I can't say for sure, I don't have access to their bugDB, they typically keep bugs private (or so I am told). All I can say is that they are aware of this issue. > >>> @@ -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. >> >> Hmm.. Yes, I did not consider this. I am not convinced this will >> always result in correct behavior for the emulator so I will >> need to pass down a parameter. Unless there is a way to figure >> out whether we are running in the emulator (which I don't >> immediately see) > Passing a parameter for this is sort of ugly, but I presume > unavoidable. The more that what you need to know is not "running > in emulator", but "guest RDMSR/WRMSR" - this can also happen > through the emulator. Right, that's what I meant. -boris
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c index b819897a4a9f..c9a93448f071 100644 --- 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; diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index 2d4475ee3de2..34524c7a6f00 100644 --- 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; diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c index be8e36386250..e624fc8694bf 100644 --- 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; + + if ( likely(mp->ignore_msrs != MSR_UNHANDLED_SILENT) ) { + if ( is_write ) + gdprintk(XENLOG_WARNING, "WRMSR 0x%08x val 0x%016"PRIx64 + " unimplemented\n", msr, *val); + else + gdprintk(XENLOG_WARNING, "RDMSR 0x%08x unimplemented\n", msr); + } + + return (mp->ignore_msrs == MSR_UNHANDLED_NEVER); +} + int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val) { const struct vcpu *curr = current; diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c index dbceed8a05fd..96d90eb30adc 100644 --- a/xen/arch/x86/pv/emul-priv-op.c +++ b/xen/arch/x86/pv/emul-priv-op.c @@ -984,7 +984,8 @@ static int read_msr(unsigned int reg, uint64_t *val, } /* fall through */ default: - gdprintk(XENLOG_WARNING, "RDMSR 0x%08x unimplemented\n", reg); + if ( !guest_unhandled_msr(curr, reg, val, false) ) + return X86EMUL_OKAY; break; normal: @@ -1146,9 +1147,8 @@ static int write_msr(unsigned int reg, uint64_t val, } /* fall through */ default: - gdprintk(XENLOG_WARNING, - "WRMSR 0x%08x val 0x%016"PRIx64" unimplemented\n", - reg, val); + if ( !guest_unhandled_msr(curr, reg, &val, true) ) + return X86EMUL_OKAY; break; invalid: diff --git a/xen/include/asm-x86/msr.h b/xen/include/asm-x86/msr.h index 16f95e734428..43a48e1a50ce 100644 --- 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 */
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, for example, Linux) does not catch exceptions when accessing MSRs that potentially may not be present. Instead of special-casing RAPL registers we decide what to do when any non-emulated MSR is accessed based on ignore_msrs field of msr_policy. Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> --- xen/arch/x86/hvm/svm/svm.c | 10 ++++------ xen/arch/x86/hvm/vmx/vmx.c | 10 ++++------ xen/arch/x86/msr.c | 20 ++++++++++++++++++++ xen/arch/x86/pv/emul-priv-op.c | 8 ++++---- xen/include/asm-x86/msr.h | 3 ++- 5 files changed, 34 insertions(+), 17 deletions(-)