On Fri, Feb 12, 2021, Sean Christopherson wrote: > On Fri, Feb 12, 2021, Makarand Sonare wrote: > > >> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > > >> index 777177ea9a35e..eb6639f0ee7eb 100644 > > >> --- a/arch/x86/kvm/vmx/vmx.c > > >> +++ b/arch/x86/kvm/vmx/vmx.c > > >> @@ -4276,7 +4276,7 @@ static void > > >> vmx_compute_secondary_exec_control(struct vcpu_vmx *vmx) > > >> */ > > >> exec_control &= ~SECONDARY_EXEC_SHADOW_VMCS; > > >> > > >> - if (!enable_pml) > > >> + if (!enable_pml || !vcpu->kvm->arch.pml_enabled) > > >> exec_control &= ~SECONDARY_EXEC_ENABLE_PML; > > > > > > The checks are unnecessary if PML is dynamically toggled, i.e. this > > > snippet can unconditionally clear PML. When setting SECONDARY_EXEC > > > (below snippet), PML will be preserved in the current controls, which is > > > what we want. > > > > Assuming a new VCPU can be added at a later time after PML is already > > enabled, should we clear > > PML in VMCS for the new VCPU. If yes what will be the trigger for > > setting PML for the new VCPU? > > Ah, didn't consider that. Phooey. I remember why I thought this could be unconditional. Adding PML to the list of dynamic bits in vmcs_set_secondary_exec_control() effectively makes this code unconditional, because it means that current bit will be preserved, including the case where PML=0 when a vCPU is created. I believe the fix is simply to not mark PML as fully dynamic.
