On 30/01/2018 18:50, KarimAllah Ahmed wrote: > On 01/30/2018 11:49 PM, Jim Mattson wrote: >> On Tue, Jan 30, 2018 at 1:00 PM, KarimAllah Ahmed >> <karahmed@xxxxxxxxxx> wrote: >>> Ooops! I did not think at all about nested :) >>> >>> This should be addressed now, I hope: >>> >>> http://git.infradead.org/linux-retpoline.git/commitdiff/f7f0cbba3e0cffcee050a8a5a9597a162d57e572 >>> >> >> + if (cpu_has_vmx_msr_bitmap() && data && >> + !vmx->save_spec_ctrl_on_exit) { >> + vmx->save_spec_ctrl_on_exit = true; >> + >> + msr_bitmap = is_guest_mode(vcpu) ? >> vmx->nested.vmcs02.msr_bitmap : >> + >> vmx->vmcs01.msr_bitmap; >> + vmx_disable_intercept_for_msr(msr_bitmap, >> + MSR_IA32_SPEC_CTRL, >> + MSR_TYPE_RW); >> + } >> >> There are two ways to get to this point in vmx_set_msr while >> is_guest_mode(vcpu) is true: >> 1) L0 is processing vmcs12's VM-entry MSR load list on emulated >> VM-entry (see enter_vmx_non_root_mode). >> 2) L2 tried to execute WRMSR, writes to the MSR are intercepted in >> vmcs02's MSR permission bitmap, and writes to the MSR are not >> intercepted in vmcs12's MSR permission bitmap. >> >> In the first case, disabling the intercepts for the MSR in >> vmx->nested.vmcs02.msr_bitmap is incorrect, because we haven't yet >> determined that the intercepts are clear in vmcs12's MSR permission >> bitmap. >> In the second case, disabling *both* of the intercepts for the MSR in >> vmx->nested.vmcs02.msr_bitmap is incorrect, because we don't know that >> the read intercept is clear in vmcs12's MSR permission bitmap. >> Furthermore, disabling the write intercept for the MSR in >> vmx->nested.vmcs02.msr_bitmap is somewhat fruitless, because >> nested_vmx_merge_msr_bitmap is just going to undo that change on the >> next emulated VM-entry. > > Okay, I took a second look at the code (specially > nested_vmx_merge_msr_bitmap). > > This means that I simply should not touch the MSR bitmap in set_msr in > case of nested, I just need to properly update the l02 msr_bitmap in > nested_vmx_merge_msr_bitmap. As in here: > > http://git.infradead.org/linux-retpoline.git/commitdiff/d90eedebdd16bb00741a2c93bc13c5e444c99c2b > > > or am I still missing something? (sorry, did not actually look at the > nested code before!) The new code in nested_vmx_merge_msr_bitmap should be conditional on vmx->save_spec_ctrl_on_exit. Also, guest_cpuid_has is pretty slow (because of kvm_find_cpuid_entry); calling it once or twice on each and every nested vmexit is probably not a good idea. Apart from this, it looks good to me. Paolo
