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(-)
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 */
--
1.8.3.1
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
© 2016 - 2026 Red Hat, Inc.