Re: [RESEND PATCH ] KVM: VMX: Enable/disable PML when dirty logging gets enabled/disabled

[Sean Christopherson]

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.