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;
Message-ID: <a0c4d1fdd132fa0767cd27b9c21d3e04857f7598.camel@intel.com>
Subject: Re: [PATCH v7 040/102] KVM: x86/mmu: Zap only leaf SPTEs for
 deleted/moved memslot for private mmu
From:   Kai Huang <kai.huang@intel.com>
To:     isaku.yamahata@intel.com, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org
Cc:     isaku.yamahata@gmail.com, Paolo Bonzini <pbonzini@redhat.com>,
        Sean Christopherson <sean.j.christopherson@intel.com>
Date:   Fri, 01 Jul 2022 22:41:08 +1200
In-Reply-To: <27acc4b2957e1297640d1d8b2a43f7c08e3885d5.1656366338.git.isaku.yamahata@intel.com>
References: <cover.1656366337.git.isaku.yamahata@intel.com>
         <27acc4b2957e1297640d1d8b2a43f7c08e3885d5.1656366338.git.isaku.yamahata@intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.2 (3.44.2-1.fc36) 
MIME-Version: 1.0
Precedence: bulk

On Mon, 2022-06-27 at 14:53 -0700, isaku.yamahata@intel.com wrote:
> From: Sean Christopherson <sean.j.christopherson@intel.com>
>=20
> For kvm mmu that has shared bit mask, zap only leaf SPTEs when
> deleting/moving a memslot.  The existing kvm_mmu_zap_memslot() depends on

Unless I am mistaken, I don't see there's an 'existing' kvm_mmu_zap_memslot=
().

> role.invalid with read lock of mmu_lock so that other vcpu can operate on
> kvm mmu concurrently.=C2=A0
>=20

> Mark the root page table invalid, unlink it from page
> table pointer of CPU, process the page table. =C2=A0
>=20

Are you talking about the behaviour of existing code, or the change you are
going to make?  Looks like you mean the latter but I believe it's the forme=
r.=20

> It doesn't work for private
> page table to unlink the root page table because it requires all SPTE ent=
ry
> to be non-present. =C2=A0
>=20

I don't think we can truly *unlink* the private root page table from secure
EPTP, right?  The EPTP (root table) is fixed (and hidden) during TD's runti=
me.

I guess you are trying to say: removing/unlinking one secure-EPT page requi=
res
removing/unlinking all its children first?=20

So the reason to only zap leaf is we cannot truly unlink the private root p=
age
table, correct?  Sorry your changelog is not obvious to me.

> Instead, with write-lock of mmu_lock and zap only leaf
> SPTEs for kvm mmu with shared bit mask.
>=20
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
> ---
>  arch/x86/kvm/mmu/mmu.c | 35 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 34 insertions(+), 1 deletion(-)
>=20
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 80d7c7709af3..c517c7bca105 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -5854,11 +5854,44 @@ static bool kvm_has_zapped_obsolete_pages(struct =
kvm *kvm)
>  	return unlikely(!list_empty_careful(&kvm->arch.zapped_obsolete_pages));
>  }
> =20
> +static void kvm_mmu_zap_memslot(struct kvm *kvm, struct kvm_memory_slot =
*slot)
> +{
> +	bool flush =3D false;
> +
> +	write_lock(&kvm->mmu_lock);
> +
> +	/*
> +	 * Zapping non-leaf SPTEs, a.k.a. not-last SPTEs, isn't required, worst
> +	 * case scenario we'll have unused shadow pages lying around until they
> +	 * are recycled due to age or when the VM is destroyed.
> +	 */
> +	if (is_tdp_mmu_enabled(kvm)) {
> +		struct kvm_gfn_range range =3D {
> +		      .slot =3D slot,
> +		      .start =3D slot->base_gfn,
> +		      .end =3D slot->base_gfn + slot->npages,
> +		      .may_block =3D false,
> +		};
> +
> +		flush =3D kvm_tdp_mmu_unmap_gfn_range(kvm, &range, flush);


It appears you only unmap private GFNs (because the base_gfn doesn't have s=
hared
bit)?  I think shared mapping in this slot must be zapped too? =C2=A0

How is this done?  Or the kvm_tdp_mmu_unmap_gfn_range() also zaps shared
mappings?

It's hard to review if one patch's behaviour/logic depends on further patch=
es.

> +	} else {
> +		flush =3D slot_handle_level(kvm, slot, kvm_zap_rmapp, PG_LEVEL_4K,
> +					  KVM_MAX_HUGEPAGE_LEVEL, true);
> +	}
> +	if (flush)
> +		kvm_flush_remote_tlbs(kvm);
> +
> +	write_unlock(&kvm->mmu_lock);
> +}
> +
>  static void kvm_mmu_invalidate_zap_pages_in_memslot(struct kvm *kvm,
>  			struct kvm_memory_slot *slot,
>  			struct kvm_page_track_notifier_node *node)
>  {
> -	kvm_mmu_zap_all_fast(kvm);
> +	if (kvm_gfn_shared_mask(kvm))
> +		kvm_mmu_zap_memslot(kvm, slot);
> +	else
> +		kvm_mmu_zap_all_fast(kvm);
>  }
> =20
>  int kvm_mmu_init_vm(struct kvm *kvm)