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
