On Wed, Jan 20, 2021 at 05:49:11PM -0500, 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,
> 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.ostrov...@oracle.com>
> ---
> Changes in v2:
> * define x86_emul_guest_msr_access() and use it to determine whether emulated
> instruction is rd/wrmsr.
> * Don't use ignore_msrs for MSR accesses that are not guest's rd/wrmsr.
> * Clear @val for writes too in guest_unhandled_msr()
>
> xen/arch/x86/hvm/svm/svm.c | 10 ++++------
> xen/arch/x86/hvm/vmx/vmx.c | 10 ++++------
> xen/arch/x86/msr.c | 28 ++++++++++++++++++++++++++++
> xen/arch/x86/pv/emul-priv-op.c | 10 ++++++----
> xen/arch/x86/x86_emulate/x86_emulate.h | 6 ++++++
> xen/include/asm-x86/msr.h | 3 +++
> 6 files changed, 51 insertions(+), 16 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index b819897a4a9f..7b59885b2619 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, true) )
> + 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, 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..87baca57d33f 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, true) )
> + 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, true) )
> + goto gp_fault;
> }
I think this could be done in hvm_msr_read_intercept instead of having
to call guest_unhandled_msr from each vendor specific handler?
Oh, I see, that's likely done to differentiate between guest MSR
accesses and emulator ones? I'm not sure we really need to make a
difference between guests MSR accesses and emulator ones, surely in
the past they would be treated equally?
Thanks, Roger.
