On Tue, Oct 22, 2019 at 12:51:01PM +0200, Paolo Bonzini wrote: > On 22/10/19 02:08, Sean Christopherson wrote: > > Remove the code to initialize IA32_FEATURE_CONTROL MSR when KVM is > > loaded now that the MSR is initialized during boot on all CPUs that > > support VMX, i.e. can possibly load kvm_intel. > > > > Reviewed-by: Jim Mattson <jmattson@xxxxxxxxxx> > > Signed-off-by: Sean Christopherson <sean.j.christopherson@xxxxxxxxx> > > --- > > arch/x86/kvm/vmx/vmx.c | 48 +++++++++++++++++------------------------- > > 1 file changed, 19 insertions(+), 29 deletions(-) > > I am still not sure about this... Enabling VMX is adding a possible > attack vector for the kernel, we should not do it unless we plan to do a > VMXON. An attacker would need arbitrary cpl0 access to toggle CR4.VMXE and do VMXON (and VMLAUNCH), would an extra WRMSR really slow them down? And practically speaking, how often do you encounter systems whose firmware leaves IA32_FEATURE_CONTROL unlocked? > Why is it so important to operate with locked > IA32_FEATURE_CONTROL (so that KVM can enable VMX and the kernel can > still enable SGX if desired). For simplicity. The alternative that comes to mind is to compute the desired MSR value and write/lock the MSR on demand, e.g. add a sequence similar to KVM's hardware_enable_all() for SGX, but that's a fair amount of complexity for marginal benefit (IMO). If a user really doesn't want VMX enabled, they can clear the feature bit via the clearcpuid kernel param. That being said, enabling VMX in IA32_FEATURE_CONTROL if and only if IS_ENABLED(CONFIG_KVM) is true would be an easy enhancement.
