From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 441ACEB64D7 for ; Wed, 21 Jun 2023 02:51:53 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id AFC998D0002; Tue, 20 Jun 2023 22:51:52 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id A853D8D0001; Tue, 20 Jun 2023 22:51:52 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 8FE708D0002; Tue, 20 Jun 2023 22:51:52 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id 7C4BC8D0001 for ; Tue, 20 Jun 2023 22:51:52 -0400 (EDT) Received: from smtpin14.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id 395B04055D for ; Wed, 21 Jun 2023 02:51:52 +0000 (UTC) X-FDA: 80925230064.14.FEC4A64 Received: from mail-ot1-f53.google.com (mail-ot1-f53.google.com [209.85.210.53]) by imf07.hostedemail.com (Postfix) with ESMTP id 5A06340007 for ; Wed, 21 Jun 2023 02:51:50 +0000 (UTC) Authentication-Results: imf07.hostedemail.com; dkim=pass header.d=gmail.com header.s=20221208 header.b=J9rhQVz1; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf07.hostedemail.com: domain of npiggin@gmail.com designates 209.85.210.53 as permitted sender) smtp.mailfrom=npiggin@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1687315910; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=qJNr1vyPvHbMEk8O8XldE8NbrEXH2idSMvV3eZDgIT4=; b=agWFdErsmfPHrPTLdpaDYOcfzzXu1c9CvXKkN/GcqYD44yIqItV56I8QduxRCNZVcNuwUt T/VG3Hh3/wUH/Gp6BfWilgyFUoHca+7DtYpvmxTq+n7AZX2yTBpaswjX0sGxGsr+l9SO5N UqceejEyFFh6IiPl+g1peqwDZnltnk4= ARC-Authentication-Results: i=1; imf07.hostedemail.com; dkim=pass header.d=gmail.com header.s=20221208 header.b=J9rhQVz1; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf07.hostedemail.com: domain of npiggin@gmail.com designates 209.85.210.53 as permitted sender) smtp.mailfrom=npiggin@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1687315910; a=rsa-sha256; cv=none; b=UaWl6Q5v64suypkV5/jwPexpG/PsfJXaNmo5eH1f9mP4OG3Izzf4YIJ5JPicNzDfZg1LVc dmYbLIKbtBAXhzdUoIqHfShmCXgCojCxPB5nC415f9TnEIQrwuKTsFpo0GGJ2E7UxvfH49 9Lc8qhPTz/aANf53xg89u9FR1Ohahn4= Received: by mail-ot1-f53.google.com with SMTP id 46e09a7af769-6b45e465d4fso3578494a34.3 for ; Tue, 20 Jun 2023 19:51:50 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1687315909; x=1689907909; h=in-reply-to:references:subject:cc:to:from:message-id:date :content-transfer-encoding:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=qJNr1vyPvHbMEk8O8XldE8NbrEXH2idSMvV3eZDgIT4=; b=J9rhQVz1R8lKDLiKwNIPca+zvG5Tv3NsIAj1Ji/wwJWZ5NGPo67uNji7TMbGfymVWO GMr37w69IFJ1a/xO2A5DmceFjN0O32RpzdAIssyO44XqTHGckAF25peyXSxvQDN3Hon3 nm7yK6UQvcoGWP40qbNg1er6nj4r5UQsT0ym99rNBvogtZMf2i5gafTMS+lgcgzNQGKq aoUmdiGQ3/vjF0fTi0Y3GwgVLDETdD79PdEDbQpGXiviLmWKyCtsFqxqvYfIKO+j7cmH 1dwMi6f30X6ti1+jJ2sfUBHAIRnjtvhicwwWnFeGYhp7QveaAM4i9Fm9K8L5dJwflH0T aPMw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1687315909; x=1689907909; h=in-reply-to:references:subject:cc:to:from:message-id:date :content-transfer-encoding:mime-version:x-gm-message-state:from:to :cc:subject:date:message-id:reply-to; bh=qJNr1vyPvHbMEk8O8XldE8NbrEXH2idSMvV3eZDgIT4=; b=alIEg35ZMPDNiD6Y+4776/8Yltg9mn3UyH9E0OGSLf36PZnaDCxqIVtWwPUx4zvxTZ 9TrzJkhx9gwu40Xn9g9+78nbSs+uAJXZLmkwRvnfF/NlRr1dunm/emwqAWAv1POywKJb E//zLFWFP6RN9RGtMPfNfL24LO/idXHf0RynVKLdqMHHsXQpPjLFADRavKpp6lmgnaLu sXLL6gaz+kCW85McbiLfolcqxqE/G17SN1OJhDFV3cSi3E9P5jGs/YLS9+6J0GJG858b H+nnWXvYEeLKDz81jGym1+sqoTKoEP2p7O7woyRZA0W5gbpdByA2R8B6uhg+XyvmZcO/ WwMA== X-Gm-Message-State: AC+VfDys2lMFmvyBb0eyBqWrJ+QcbmdqoEemp+zEO2IrIcB7m8MDZZtC pjbnRGP7/U11jXXbAPI3lxA= X-Google-Smtp-Source: ACHHUZ4vmFcBnDcvLrBXtT15BOX4k0uY1EIRYfrVxGT80rydywp/6szlGIEPs7lB1lYg1S65G5/lLQ== X-Received: by 2002:a05:6359:a26:b0:12c:9960:ec1e with SMTP id el38-20020a0563590a2600b0012c9960ec1emr8123207rwb.19.1687315909090; Tue, 20 Jun 2023 19:51:49 -0700 (PDT) Received: from localhost (193-116-203-37.tpgi.com.au. [193.116.203.37]) by smtp.gmail.com with ESMTPSA id q15-20020a170902bd8f00b001a5fccab02dsm2246983pls.177.2023.06.20.19.51.33 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 20 Jun 2023 19:51:48 -0700 (PDT) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Wed, 21 Jun 2023 12:51:31 +1000 Message-Id: From: "Nicholas Piggin" To: "Yu Zhao" Cc: "Andrew Morton" , "Paolo Bonzini" , "Alistair Popple" , "Anup Patel" , "Ben Gardon" , "Borislav Petkov" , "Catalin Marinas" , "Chao Peng" , "Christophe Leroy" , "Dave Hansen" , "Fabiano Rosas" , "Gaosheng Cui" , "Gavin Shan" , "H. Peter Anvin" , "Ingo Molnar" , "James Morse" , "Jason A. Donenfeld" , "Jason Gunthorpe" , "Jonathan Corbet" , "Marc Zyngier" , "Masami Hiramatsu" , "Michael Ellerman" , "Michael Larabel" , "Mike Rapoport" , "Oliver Upton" , "Paul Mackerras" , "Peter Xu" , "Sean Christopherson" , "Steven Rostedt" , "Suzuki K Poulose" , "Thomas Gleixner" , "Thomas Huth" , "Will Deacon" , "Zenghui Yu" , , , , , , , , , , Subject: Re: [PATCH mm-unstable v2 07/10] kvm/powerpc: add kvm_arch_test_clear_young() X-Mailer: aerc 0.14.0 References: <20230526234435.662652-1-yuzhao@google.com> <20230526234435.662652-8-yuzhao@google.com> In-Reply-To: X-Rspam-User: X-Rspamd-Server: rspam12 X-Rspamd-Queue-Id: 5A06340007 X-Stat-Signature: 4oka8z8nw5shftik36gp99t8z9cbast8 X-HE-Tag: 1687315910-327760 X-HE-Meta: U2FsdGVkX19oa5fBRzwgJfSPg5fY0IR3XztZGWt6GXqXLUFE+C2L/N2QrxqekWwqTMvrXhYJyuvvCxql+ktMTumszDBREqvYHSLvUYA4iYV787r9JTc6BJ/kfJjtLDaEG0EofjGgB6bwU66Nd5qd3rhzmLxbPs/S8GFhz/rtT5SJUzzTt7qCtLea6LBPGMHc2tPufAZEiJL0lnVpVWr2tSthW0O3Ua4YsaAKJKrJ3Xl/UdHSVn4STgOBIJ4rqGgUcOPKnNqdPlAzmDa7BIEcz6wpppaLOV0Qg3kGmA/a/LQ0O7GFUQbhi1nsVtjcdGAPwAaTxYOR4MxsiqZhSQtIacfDEvFVlJZguoDNsmyAISuCwCOvtPSqmU9Wq3mtO53C7b5lcXKKmsXm1zdyKq5ZPl5QyexI6tjwWf4IRqzmrLNx4KbMfKdcI8Enwqi3yp21r8DSrHJn7xg4MthxWmi7g3zybZ1uKrQDZ2arLPULc6vgEKK1l+dAFiDYUjBUYoobkumRr9R3FtieTjAa2tuvxcdSyJwqIHyCajPY4vEsgA+6ZLG384dvIVF7yVOaUExS1SwoNVBOER3FVM8ufeDGUoX0Mm8okiUYKVtV0REJgVtSnCEIK6xWsmtXHghSVI6rVrYbub3TjubwDRStpFB+dC2wVaWDiErZXo6vdYJDYtPaLcUoKezMdCHOgPy/1gbPKZthAMbpAqQq7GsajDEBnj57Y3Cn8u0orEfavEBpeKu6nZwf3e4YTPXapLoC+o+hl255ZvbcBkDyF+fT/+Twn0CPH+JndoKBSGxoSAq5IQZiXnsa21/AAbtE6KexDp1F2yx1vKQOgohk1o9J90dzb0VNQncndmVeln6Ggki6Qn78oRcGjbN3ayIgrFPc6b7FLMX86/k3vdQtQyO9XEOEK80v5As2trAFyOXoGVW5UoD71HkfE2imaVbVOaLToPXWFFXc2AhS8n/1bk3tYqs fuAH+MoK nEh/LcqRVoRlRLrHNogZgTzoNI9GSbnmL7nmXchboPBR3pU6JB1qMkvKnG3mVZNs57ItLko9ArzNrBZxrUBNZr15lYX1FGGISMYw4sm2AzUrakfIHFqDMjDGAZIxAHIXdarTxcD5vTG1vvDUroVE1qV572QNvLksjh2p8vCV4WO0fimpzvpnte4bA7/NoG3O/nSmdi3J/HVhhExWrQTKrbaiJtRAjy3oF4mTlRE7+yvoRpmmbdA+Ch4NecSM69v8JUc/Gno39tfD6iJ4+rOM5W6bjoMzf1b+3k3yD/zAGF7RFrdC+Qs/RrHVsa0yGJoZu5hkpqajT82D8sKr/oOqsDci4+aIRMMWSnsL4PK8rjQqvjmg0DGOxsGvpBGehyaUTFII3VZN2dpQbyhA6tlzjCmreiwnsBhffijJMKIkwSy+GIC5LsX1rsOtCRHJysUa3KrDdT3AB6QmO2vDfZwXPaefbg3c1hsrM2+PTw8gI7wIOnPdd8Ah0Ko0BsPYWL6MduvA8DbOXuAAL8DOk9da5GKUyP0cLHr64P1VHwBXKwMKPX8I= X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Wed Jun 21, 2023 at 10:38 AM AEST, Yu Zhao wrote: > On Tue, Jun 20, 2023 at 1:48=E2=80=AFAM Nicholas Piggin wrote: > > > > On Sat May 27, 2023 at 9:44 AM AEST, Yu Zhao wrote: > > > Implement kvm_arch_test_clear_young() to support the fast path in > > > mmu_notifier_ops->test_clear_young(). > > > > > > It focuses on a simple case, i.e., radix MMU sets the accessed bit in > > > KVM PTEs and VMs are not nested, where it can rely on RCU and > > > pte_xchg() to safely clear the accessed bit without taking > > > kvm->mmu_lock. Complex cases fall back to the existing slow path > > > where kvm->mmu_lock is then taken. > > > > > > Signed-off-by: Yu Zhao > > > --- > > > arch/powerpc/include/asm/kvm_host.h | 8 ++++ > > > arch/powerpc/include/asm/kvm_ppc.h | 1 + > > > arch/powerpc/kvm/book3s.c | 6 +++ > > > arch/powerpc/kvm/book3s.h | 1 + > > > arch/powerpc/kvm/book3s_64_mmu_radix.c | 59 ++++++++++++++++++++++++= ++ > > > arch/powerpc/kvm/book3s_hv.c | 5 +++ > > > 6 files changed, 80 insertions(+) > > > > > > diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/inclu= de/asm/kvm_host.h > > > index 14ee0dece853..75c260ea8a9e 100644 > > > --- a/arch/powerpc/include/asm/kvm_host.h > > > +++ b/arch/powerpc/include/asm/kvm_host.h > > > @@ -883,4 +883,12 @@ static inline void kvm_arch_sched_in(struct kvm_= vcpu *vcpu, int cpu) {} > > > static inline void kvm_arch_vcpu_blocking(struct kvm_vcpu *vcpu) {} > > > static inline void kvm_arch_vcpu_unblocking(struct kvm_vcpu *vcpu) {= } > > > > > > +#define kvm_arch_has_test_clear_young kvm_arch_has_test_clear_young > > > +static inline bool kvm_arch_has_test_clear_young(void) > > > +{ > > > + return IS_ENABLED(CONFIG_KVM_BOOK3S_HV_POSSIBLE) && > > > + cpu_has_feature(CPU_FTR_HVMODE) && cpu_has_feature(CPU_F= TR_ARCH_300) && > > > + radix_enabled(); > > > > This could probably be radix_enabled() && !kvmhv_on_pseries(). > > Will do. (I used !kvmhv_on_pseries() in v1 but had second thoughts on > moving kvmhv_on_pseries() into this file.) That should be okay. kvmhv_on_pseries is a property of the host so it seems reasonable to move it here if needed. > > Although unclear why not nested hypervisor... I'd have to think about i= t a bit > > more. Do you have any idea what might go wrong, or just didn't have the > > time to consider it? (Not just powerpc nested but any others). > > Yes, this series excludes nested KVM support on all architures. The > common reason for such a decision on powerpc and x86 (aarch64 doesn't > support nested yet) is that it's quite challenging to make the rmap, a > complex data structure that maps one PFN to multiple GFNs, lockless. > (See kvmhv_insert_nest_rmap().) It might be possible, however, the > potential ROI would be in question. Okay just wondering. rmap (at least the powerpc one) is just a list I think, with a few details. If that is all it is, it might not be so hard to make that lock-free or a fine-grained lock on the rmap chains maybe. But fine to ignore it to start with. > > > +} > > > + > > > #endif /* __POWERPC_KVM_HOST_H__ */ > > > diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/includ= e/asm/kvm_ppc.h > > > index 79a9c0bb8bba..ff1af6a7b44f 100644 > > > --- a/arch/powerpc/include/asm/kvm_ppc.h > > > +++ b/arch/powerpc/include/asm/kvm_ppc.h > > > @@ -287,6 +287,7 @@ struct kvmppc_ops { > > > bool (*unmap_gfn_range)(struct kvm *kvm, struct kvm_gfn_range *= range); > > > bool (*age_gfn)(struct kvm *kvm, struct kvm_gfn_range *range); > > > bool (*test_age_gfn)(struct kvm *kvm, struct kvm_gfn_range *ran= ge); > > > + bool (*test_clear_young)(struct kvm *kvm, struct kvm_gfn_range = *range); > > > bool (*set_spte_gfn)(struct kvm *kvm, struct kvm_gfn_range *ran= ge); > > > void (*free_memslot)(struct kvm_memory_slot *slot); > > > int (*init_vm)(struct kvm *kvm); > > > diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c > > > index 686d8d9eda3e..37bf40b0c4ff 100644 > > > --- a/arch/powerpc/kvm/book3s.c > > > +++ b/arch/powerpc/kvm/book3s.c > > > @@ -899,6 +899,12 @@ bool kvm_test_age_gfn(struct kvm *kvm, struct kv= m_gfn_range *range) > > > return kvm->arch.kvm_ops->test_age_gfn(kvm, range); > > > } > > > > > > +bool kvm_arch_test_clear_young(struct kvm *kvm, struct kvm_gfn_range= *range) > > > +{ > > > + return !kvm->arch.kvm_ops->test_clear_young || > > > + kvm->arch.kvm_ops->test_clear_young(kvm, range); > > > +} > > > + > > > bool kvm_set_spte_gfn(struct kvm *kvm, struct kvm_gfn_range *range) > > > { > > > return kvm->arch.kvm_ops->set_spte_gfn(kvm, range); > > > diff --git a/arch/powerpc/kvm/book3s.h b/arch/powerpc/kvm/book3s.h > > > index 58391b4b32ed..fa2659e21ccc 100644 > > > --- a/arch/powerpc/kvm/book3s.h > > > +++ b/arch/powerpc/kvm/book3s.h > > > @@ -12,6 +12,7 @@ extern void kvmppc_core_flush_memslot_hv(struct kvm= *kvm, > > > extern bool kvm_unmap_gfn_range_hv(struct kvm *kvm, struct kvm_gfn_r= ange *range); > > > extern bool kvm_age_gfn_hv(struct kvm *kvm, struct kvm_gfn_range *ra= nge); > > > extern bool kvm_test_age_gfn_hv(struct kvm *kvm, struct kvm_gfn_rang= e *range); > > > +extern bool kvm_test_clear_young_hv(struct kvm *kvm, struct kvm_gfn_= range *range); > > > extern bool kvm_set_spte_gfn_hv(struct kvm *kvm, struct kvm_gfn_rang= e *range); > > > > > > extern int kvmppc_mmu_init_pr(struct kvm_vcpu *vcpu); > > > diff --git a/arch/powerpc/kvm/book3s_64_mmu_radix.c b/arch/powerpc/kv= m/book3s_64_mmu_radix.c > > > index 3b65b3b11041..0a392e9a100a 100644 > > > --- a/arch/powerpc/kvm/book3s_64_mmu_radix.c > > > +++ b/arch/powerpc/kvm/book3s_64_mmu_radix.c > > > @@ -1088,6 +1088,65 @@ bool kvm_test_age_radix(struct kvm *kvm, struc= t kvm_memory_slot *memslot, > > > return ref; > > > } > > > > > > +bool kvm_test_clear_young_hv(struct kvm *kvm, struct kvm_gfn_range *= range) > > > +{ > > > + bool err; > > > + gfn_t gfn =3D range->start; > > > + > > > + rcu_read_lock(); > > > + > > > + err =3D !kvm_is_radix(kvm); > > > + if (err) > > > + goto unlock; > > > + > > > + /* > > > + * Case 1: This function kvmppc_switch_mmu_to_hpt() > > > + * > > > + * rcu_read_lock() > > > + * Test kvm_is_radix() kvm->arch.radix =3D 0 > > > + * Use kvm->arch.pgtable synchronize_rcu() > > > + * rcu_read_unlock() > > > + * kvmppc_free_radix() > > > + * > > > + * > > > + * Case 2: This function kvmppc_switch_mmu_to_radix() > > > + * > > > + * kvmppc_init_vm_radix() > > > + * smp_wmb() > > > + * Test kvm_is_radix() kvm->arch.radix =3D 1 > > > + * smp_rmb() > > > + * Use kvm->arch.pgtable > > > + */ > > > + smp_rmb(); > > > > Comment could stand to expand slightly on what you are solving, in > > words. > > Will do. > > > If you use synchronize_rcu() on both sides, you wouldn't need the > > barrier, right? > > Case 2 is about memory ordering, which is orthogonal to case 1 (RCU > freeing). So we need the r/w barrier regardless. RCU can take care of memory ordering too though. If you had synchronize_rcu() where smp_wmb() is, then no smp_rmb() neeed here. > > > > + while (gfn < range->end) { > > > + pte_t *ptep; > > > + pte_t old, new; > > > + unsigned int shift; > > > + > > > + ptep =3D find_kvm_secondary_pte_unlocked(kvm, gfn * PAG= E_SIZE, &shift); > > > + if (!ptep) > > > + goto next; > > > + > > > + VM_WARN_ON_ONCE(!page_count(virt_to_page(ptep))); > > > > Not really appropriate at the KVM level. mm enforces this kind of > > thing (with notifiers). > > Will remove this. > > > > + > > > + old =3D READ_ONCE(*ptep); > > > + if (!pte_present(old) || !pte_young(old)) > > > + goto next; > > > + > > > + new =3D pte_mkold(old); > > > + > > > + if (kvm_should_clear_young(range, gfn)) > > > + pte_xchg(ptep, old, new); > > > > *Probably* will work. I can't think of a reason why not at the > > moment anyway :) > > My reasoning: > * It should work if we only change the dedicated A bit, i.e., not > shared for other purposes, because races are safe (the case here). > * It may not work for x86 EPT without the A bit (excluded in this > series) where accesses are trapped by the R/X bits, because races in > changing the R/X bits can be unsafe. (For the benefit of others reading, it works because powerpc's pte_xchg is actually a cmpxchg, for some reason which we really should fix). Although it can fail to clear the bit if the cmpxchg fails. I think pte_xchg is only used on with hash MMU in Linux before this change. I think we may want to keep it that way and use something like kvmppc_radix_update_pte() here to clear out the bit. But don't worry too much about fine details so much before sorting out the core changes I will have a better look after that. Thanks, Nick