Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 24 Apr 2023 08:51:48 -0700
In-Reply-To: <20230422111834.00003689@gmail.com>
Mime-Version: 1.0
References: <cover.1678643051.git.isaku.yamahata@intel.com>
 <23cc6df6ec2ca776c3efe148e70ba290aa3de319.1678643052.git.isaku.yamahata@intel.com>
 <20230422111834.00003689@gmail.com>
Message-ID: <ZEallJHJnG9HJDNy@google.com>
Subject: Re: [PATCH v13 048/113] KVM: x86/mmu: Disallow dirty logging for x86 TDX
From:   Sean Christopherson <seanjc@google.com>
To:     Zhi Wang <zhi.wang.linux@gmail.com>
Cc:     isaku.yamahata@intel.com, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org, isaku.yamahata@gmail.com,
        Paolo Bonzini <pbonzini@redhat.com>, erdemaktas@google.com,
        Sagi Shahar <sagis@google.com>,
        David Matlack <dmatlack@google.com>,
        Kai Huang <kai.huang@intel.com>,
        Sean Christopherson <sean.j.christopherson@intel.com>,
        Michael Roth <michael.roth@amd.com>
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

On Sat, Apr 22, 2023, Zhi Wang wrote:
> On Sun, 12 Mar 2023 10:56:12 -0700
> isaku.yamahata@intel.com wrote:
> 
> > From: Isaku Yamahata <isaku.yamahata@intel.com>
> > 
> > TDX doesn't support dirty logging.  Report dirty logging isn't supported so
> > that device model, for example qemu, can properly handle it.  Silently
> > ignore on dirty logging on private GFNs of TDX.

...

> > +bool kvm_arch_dirty_log_supported(struct kvm *kvm)
> > +{
> > +	return kvm->arch.vm_type != KVM_X86_PROTECTED_VM;
> > +}
> > +
> 
> Maybe introduce a new x86 ops for SNP/TDX to check this separately as SNP
> might still support it? With the current approach, I think both SNP/TDX
> will be affected. So does the later patch about page-tracking.

This patch is unnecessary, the plan is to disallow dirty logging on memslots that
support private mapping, e.g. we'll end up with something like this:

static int check_memory_region_flags(struct kvm *kvm,
				     const struct kvm_userspace_memory_region2 *mem)
{
	u32 valid_flags = KVM_MEM_LOG_DIRTY_PAGES;

	if (kvm_arch_has_private_mem(kvm))
		valid_flags |= KVM_MEM_PRIVATE;

	/* Dirty logging private memory is not currently supported. */
	if (mem->flags & KVM_MEM_PRIVATE)
		valid_flags &= ~KVM_MEM_LOG_DIRTY_PAGES;

#ifdef __KVM_HAVE_READONLY_MEM
	valid_flags |= KVM_MEM_READONLY;
#endif

	if (mem->flags & ~valid_flags)
		return -EINVAL;

	return 0;
}

> Michael, can you confirm this?

No need to confirm (or deny) at this point, enabling dirty logging for private
memory is not something I want to merge in the initial TDX/SNP series, regardless
of whether or not it's supported by "hardware", a.k.a. trusted firmware.