Re: [PATCH v4 4/7] KVM: x86: Move nested CPU dirty logging logic to common code

["Huang, Kai" <kai.huang@intel.com>]

On Wed, 2025-10-15 at 10:13 +0530, Nikunj A. Dadhania wrote:
> 
> On 10/15/2025 3:07 AM, Huang, Kai wrote:
> > On Tue, 2025-10-14 at 14:24 -0700, Sean Christopherson wrote:
> > > On Tue, Oct 14, 2025, Kai Huang wrote:
> > > > On Tue, 2025-10-14 at 11:34 +0000, Huang, Kai wrote:
> > > > > >   
> > > > > > +static void kvm_vcpu_update_cpu_dirty_logging(struct kvm_vcpu *vcpu)
> > > > > > +{
> > > > > > +	if (WARN_ON_ONCE(!enable_pml))
> > > > > > +		return;
> > > > > 
> > > > > Nit:  
> > > > > 
> > > > > Since kvm_mmu_update_cpu_dirty_logging() checks kvm-
> > > > > > arch.cpu_dirty_log_size to determine whether PML is enabled, maybe it's
> > > > > better to check vcpu->kvm.arch.cpu_dirty_log_size here too to make them
> > > > > consistent.
> > > > 
> > > > After second thought, I think we should just change to checking the vcpu-
> > > > > kvm.arch.cpu_dirty_log_size.
> > > 
> > > +1
> > > 
> > > > > Anyway, the intention of this patch is moving code out of VMX to x86, so
> > > > > if needed, perhaps we can do the change in another patch.
> 
> I will add this as a pre-patch, does it need a fixes tag ?

No I don't think there's any bug in the existing upstream code, since for
VMX guests, checking 'enable_pml' and the per-VM 'cpu_dirty_log_size' is
basically the same thing.

I won't stop you from doing this in a separate patch, but I think we can
just change this patch to check cpu_dirty_log_size with some justification
in changelog (e.g., below, a bit lengthy but not sure how to trim down):

  Note KVM common code has a per-VM kvm->arch.cpu_dirty_log_size to
  indicate whether PML is enabled on VM basis.  It's for supporting
  running both VMX guests and TDX guests with a KVM global 'enable_pml'
  module parameter -- TDX doesn't (yet) support PML, and 'enable_pml' is
  used to enable PML for VMX guests while supporting TDX guests.

  Currently vmx_update_cpu_dirty_logging() has a WARN() to check
  '!enable_pml' to make sure it is not mistakenly called when PML is
  not enabled in KVM.  It's correct for VMX guests.  However such check
  is no longer correct in x86 common since it doesn't apply to TDX guests.
 
  Therefore change to check the per VM cpu_dirty_log_size while moving
  vmx_update_cpu_dirty_logging() to x86 common.  And it's also more 
  consistent with kvm_mmu_update_cpu_dirty_logging(), which checks the
  cpu_dirty_log_size.

Or perhaps Sean has some preference?