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.
