| Message ID | 20200901105445.22277-8-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: switch default MSR behavior | expand |
On 01.09.2020 12:54, Roger Pau Monne wrote: > @@ -3290,11 +3288,6 @@ static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content) > __vmwrite(GUEST_IA32_DEBUGCTL, msr_content); > break; > > - case MSR_IA32_FEATURE_CONTROL: > - case MSR_IA32_VMX_BASIC ... MSR_IA32_VMX_VMFUNC: > - /* None of these MSRs are writeable. */ > - goto gp_fault; I understand Andrew did ask for this (and I didn't look closely when I saw the comment), but ... > @@ -3320,10 +3313,9 @@ static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content) > is_last_branch_msr(msr) ) > break; > > - /* Match up with the RDMSR side; ultimately this should go away. */ > - if ( rdmsr_safe(msr, msr_content) == 0 ) > - break; > - > + gdprintk(XENLOG_WARNING, > + "WRMSR 0x%08x val 0x%016"PRIx64" unimplemented\n", > + msr, msr_content); > goto gp_fault; ... above from here is logic that handling of these MSRs now goes through. I'm particularly worried about vmx_write_guest_msr(), which blindly updates the value of any MSR it can find, i.e. if any r/o MSR (from the set above, or even more generally) ever got added to this vCPU-specific set, the r/o-ness would no longer be maintained. Do we perhaps need to white-list MSRs for which vmx_write_guest_msr() may get called here? Jan
On 04/09/2020 09:53, Jan Beulich wrote: > On 01.09.2020 12:54, Roger Pau Monne wrote: >> @@ -3290,11 +3288,6 @@ static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content) >> __vmwrite(GUEST_IA32_DEBUGCTL, msr_content); >> break; >> >> - case MSR_IA32_FEATURE_CONTROL: >> - case MSR_IA32_VMX_BASIC ... MSR_IA32_VMX_VMFUNC: >> - /* None of these MSRs are writeable. */ >> - goto gp_fault; > I understand Andrew did ask for this (and I didn't look closely > when I saw the comment), but ... > >> @@ -3320,10 +3313,9 @@ static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content) >> is_last_branch_msr(msr) ) >> break; >> >> - /* Match up with the RDMSR side; ultimately this should go away. */ >> - if ( rdmsr_safe(msr, msr_content) == 0 ) >> - break; >> - >> + gdprintk(XENLOG_WARNING, >> + "WRMSR 0x%08x val 0x%016"PRIx64" unimplemented\n", >> + msr, msr_content); >> goto gp_fault; > ... above from here is logic that handling of these MSRs now goes > through. I'm particularly worried about vmx_write_guest_msr(), > which blindly updates the value of any MSR it can find, i.e. if > any r/o MSR (from the set above, or even more generally) ever got > added to this vCPU-specific set, the r/o-ness would no longer be > maintained. Do we perhaps need to white-list MSRs for which > vmx_write_guest_msr() may get called here? If a read-only MSR ever actually gets into the load/save list, we'll take a VMEntry failure (guest load) or SHUTDOWN (host load) as a consequence of microcode takes a #GP fault. However, restricting the content of this catch-all clause to nothing (but the printk) is something I have planned as part of the ongoing MSR cleanup work. ~Andrew
On 04.09.2020 11:44, Andrew Cooper wrote: > On 04/09/2020 09:53, Jan Beulich wrote: >> On 01.09.2020 12:54, Roger Pau Monne wrote: >>> @@ -3290,11 +3288,6 @@ static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content) >>> __vmwrite(GUEST_IA32_DEBUGCTL, msr_content); >>> break; >>> >>> - case MSR_IA32_FEATURE_CONTROL: >>> - case MSR_IA32_VMX_BASIC ... MSR_IA32_VMX_VMFUNC: >>> - /* None of these MSRs are writeable. */ >>> - goto gp_fault; >> I understand Andrew did ask for this (and I didn't look closely >> when I saw the comment), but ... >> >>> @@ -3320,10 +3313,9 @@ static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content) >>> is_last_branch_msr(msr) ) >>> break; >>> >>> - /* Match up with the RDMSR side; ultimately this should go away. */ >>> - if ( rdmsr_safe(msr, msr_content) == 0 ) >>> - break; >>> - >>> + gdprintk(XENLOG_WARNING, >>> + "WRMSR 0x%08x val 0x%016"PRIx64" unimplemented\n", >>> + msr, msr_content); >>> goto gp_fault; >> ... above from here is logic that handling of these MSRs now goes >> through. I'm particularly worried about vmx_write_guest_msr(), >> which blindly updates the value of any MSR it can find, i.e. if >> any r/o MSR (from the set above, or even more generally) ever got >> added to this vCPU-specific set, the r/o-ness would no longer be >> maintained. Do we perhaps need to white-list MSRs for which >> vmx_write_guest_msr() may get called here? > > If a read-only MSR ever actually gets into the load/save list, we'll > take a VMEntry failure (guest load) or SHUTDOWN (host load) as a > consequence of microcode takes a #GP fault. Oh, of course. In order to surface a value different from the hardware's one has to intercept such MSRs. > However, restricting the content of this catch-all clause to nothing > (but the printk) is something I have planned as part of the ongoing MSR > cleanup work. Good to know. Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
On Fri, Sep 04, 2020 at 10:53:26AM +0200, Jan Beulich wrote: > On 01.09.2020 12:54, Roger Pau Monne wrote: > > @@ -3290,11 +3288,6 @@ static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content) > > __vmwrite(GUEST_IA32_DEBUGCTL, msr_content); > > break; > > > > - case MSR_IA32_FEATURE_CONTROL: > > - case MSR_IA32_VMX_BASIC ... MSR_IA32_VMX_VMFUNC: > > - /* None of these MSRs are writeable. */ > > - goto gp_fault; > > I understand Andrew did ask for this (and I didn't look closely > when I saw the comment), but ... > > > @@ -3320,10 +3313,9 @@ static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content) > > is_last_branch_msr(msr) ) > > break; > > > > - /* Match up with the RDMSR side; ultimately this should go away. */ > > - if ( rdmsr_safe(msr, msr_content) == 0 ) > > - break; > > - > > + gdprintk(XENLOG_WARNING, > > + "WRMSR 0x%08x val 0x%016"PRIx64" unimplemented\n", > > + msr, msr_content); > > goto gp_fault; > > ... above from here is logic that handling of these MSRs now goes > through. I'm particularly worried about vmx_write_guest_msr(), > which blindly updates the value of any MSR it can find, i.e. if > any r/o MSR (from the set above, or even more generally) ever got > added to this vCPU-specific set, the r/o-ness would no longer be > maintained. But those MSRs need to be added to the list explicitly (using vmx_add_guest_msr) in order for the guest to be able to update them, and they are supposed to be owned by the guest? I understand the concern, but AFAICT none of the MSRs handled by VMX_MSR_GUEST require such handling. Maybe it's worth adding something like VMX_MSR_GUEST_RO in the future if such need arises? > Do we perhaps need to white-list MSRs for which > vmx_write_guest_msr() may get called here? When such MSRs are added such addition should make sure they are not allowed write permissions? You would have to do that now anyway AFAICT. Roger.
> From: Roger Pau Monne <roger.pau@citrix.com> > Sent: Tuesday, September 1, 2020 6:55 PM > > From: Andrew Cooper <andrew.cooper3@citrix.com> > > Change the catch-all behavior for MSR not explicitly handled. Instead > of allow full read-access to the MSR space and silently dropping > writes return an exception when the MSR is not explicitly handled. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > [remove rdmsr_safe from default case in svm_msr_read_intercept] > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Reviewed-by: Kevin Tian <kevin.tian@intel.com> > --- > Changes since v1: > - Fold chunk to remove explicit write handling of VMX MSRs just to > #GP. > - Remove catch-all rdmsr_safe in svm_msr_read_intercept. > --- > xen/arch/x86/hvm/svm/svm.c | 10 ++++------ > xen/arch/x86/hvm/vmx/vmx.c | 16 ++++------------ > 2 files changed, 8 insertions(+), 18 deletions(-) > > diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c > index 0e43154c7e..66b22efdab 100644 > --- a/xen/arch/x86/hvm/svm/svm.c > +++ b/xen/arch/x86/hvm/svm/svm.c > @@ -1964,8 +1964,7 @@ static int svm_msr_read_intercept(unsigned int > msr, uint64_t *msr_content) > break; > > default: > - if ( rdmsr_safe(msr, *msr_content) == 0 ) > - break; > + gdprintk(XENLOG_WARNING, "RDMSR 0x%08x unimplemented\n", > msr); > goto gpf; > } > > @@ -2150,10 +2149,9 @@ static int svm_msr_write_intercept(unsigned int > msr, uint64_t msr_content) > break; > > default: > - /* Match up with the RDMSR side; ultimately this should go away. */ > - if ( rdmsr_safe(msr, msr_content) == 0 ) > - break; > - > + gdprintk(XENLOG_WARNING, > + "WRMSR 0x%08x val 0x%016"PRIx64" unimplemented\n", > + msr, msr_content); > goto gpf; > } > > diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c > index f6657af923..9cc9d81c41 100644 > --- a/xen/arch/x86/hvm/vmx/vmx.c > +++ b/xen/arch/x86/hvm/vmx/vmx.c > @@ -3015,9 +3015,7 @@ static int vmx_msr_read_intercept(unsigned int > msr, uint64_t *msr_content) > break; > } > > - if ( rdmsr_safe(msr, *msr_content) == 0 ) > - break; > - > + gdprintk(XENLOG_WARNING, "RDMSR 0x%08x unimplemented\n", > msr); > goto gp_fault; > } > > @@ -3290,11 +3288,6 @@ static int vmx_msr_write_intercept(unsigned int > msr, uint64_t msr_content) > __vmwrite(GUEST_IA32_DEBUGCTL, msr_content); > break; > > - case MSR_IA32_FEATURE_CONTROL: > - case MSR_IA32_VMX_BASIC ... MSR_IA32_VMX_VMFUNC: > - /* None of these MSRs are writeable. */ > - goto gp_fault; > - > case MSR_IA32_MISC_ENABLE: > /* Silently drop writes that don't change the reported value. */ > if ( vmx_msr_read_intercept(msr, &tmp) != X86EMUL_OKAY || > @@ -3320,10 +3313,9 @@ static int vmx_msr_write_intercept(unsigned int > msr, uint64_t msr_content) > is_last_branch_msr(msr) ) > break; > > - /* Match up with the RDMSR side; ultimately this should go away. */ > - if ( rdmsr_safe(msr, msr_content) == 0 ) > - break; > - > + gdprintk(XENLOG_WARNING, > + "WRMSR 0x%08x val 0x%016"PRIx64" unimplemented\n", > + msr, msr_content); > goto gp_fault; > } > > -- > 2.28.0
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c index 0e43154c7e..66b22efdab 100644 --- a/xen/arch/x86/hvm/svm/svm.c +++ b/xen/arch/x86/hvm/svm/svm.c @@ -1964,8 +1964,7 @@ static int svm_msr_read_intercept(unsigned int msr, uint64_t *msr_content) break; default: - if ( rdmsr_safe(msr, *msr_content) == 0 ) - break; + gdprintk(XENLOG_WARNING, "RDMSR 0x%08x unimplemented\n", msr); goto gpf; } @@ -2150,10 +2149,9 @@ static int svm_msr_write_intercept(unsigned int msr, uint64_t msr_content) break; default: - /* Match up with the RDMSR side; ultimately this should go away. */ - if ( rdmsr_safe(msr, msr_content) == 0 ) - break; - + gdprintk(XENLOG_WARNING, + "WRMSR 0x%08x val 0x%016"PRIx64" unimplemented\n", + msr, msr_content); goto gpf; } diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index f6657af923..9cc9d81c41 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -3015,9 +3015,7 @@ static int vmx_msr_read_intercept(unsigned int msr, uint64_t *msr_content) break; } - if ( rdmsr_safe(msr, *msr_content) == 0 ) - break; - + gdprintk(XENLOG_WARNING, "RDMSR 0x%08x unimplemented\n", msr); goto gp_fault; } @@ -3290,11 +3288,6 @@ static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content) __vmwrite(GUEST_IA32_DEBUGCTL, msr_content); break; - case MSR_IA32_FEATURE_CONTROL: - case MSR_IA32_VMX_BASIC ... MSR_IA32_VMX_VMFUNC: - /* None of these MSRs are writeable. */ - goto gp_fault; - case MSR_IA32_MISC_ENABLE: /* Silently drop writes that don't change the reported value. */ if ( vmx_msr_read_intercept(msr, &tmp) != X86EMUL_OKAY || @@ -3320,10 +3313,9 @@ static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content) is_last_branch_msr(msr) ) break; - /* Match up with the RDMSR side; ultimately this should go away. */ - if ( rdmsr_safe(msr, msr_content) == 0 ) - break; - + gdprintk(XENLOG_WARNING, + "WRMSR 0x%08x val 0x%016"PRIx64" unimplemented\n", + msr, msr_content); goto gp_fault; }