On Sat, Jun 27, 2020 at 04:24:34PM +0200, Paolo Bonzini wrote: > On 26/06/20 20:18, Sean Christopherson wrote: > >> Btw, would it be more staightforward to check "vcpu->arch.arch_capabilities & > >> ARCH_CAP_TSX_CTRL_MSR" rather than "*ebx | (F(RTM) | F(HLE))" even if we want > >> to have such a fix? > > Not really, That ends up duplicating the check in vmx_get_msr(). From an > > emulation perspective, this really is a "guest" access to the MSR, in the > > sense that it the virtual CPU is in the guest domain, i.e. not a god-like > > entity that gets to break the rules of emulation. > > But if you wrote a guest that wants to read MSR_IA32_TSX_CTRL, there are > two choices: > > 1) check ARCH_CAPABILITIES first > > 2) blindly access it and default to 0. > > Both are fine, because we know MSR_IA32_TSX_CTRL has no > reserved/must-be-one bits. Calling __kvm_get_msr and checking for an > invalid MSR through the return value is not breaking the rules of > emulation, it is "faking" a #GP handler. "guest" was the wrong choice of word. My point was that, IMO, emulation should never set host_initiated=true. To me, accessing MSRs with host_initiated is the equivalent of loading a ucode patch, i.e. it's super duper special stuff that deliberately turns off all safeguards and can change the fundamental behavior of the (virtual) CPU. Emulation on the other handle should either be subject to all the normal rules or have dedicated, intelligent handling for breaking the normal rules, e.g. nested usage of vmx_set_efer(), vmx_set_cr0, vmx_set_cr4, etc... > So I think Peter's patch is fine, but (possibly on top as a third patch) > __must_check should be added to MSR getters and setters. Also one > possibility is to return -EINVAL for invalid MSRs. > > Paolo >
