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 bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 31642EB64D7 for ; Tue, 20 Jun 2023 07:48:32 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:References:Message-Id:To: From:Subject:Cc:Date:Mime-Version:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=tyTscxmy3d5vVmry2pmCRCJXD8+hDPkjbihAdtyZlc4=; b=2ogdXVLDobRftp twI8ddBj+xbp0er4utUjq9iZAgS6Y+wRtiB76lvKFBsHKvFzv2fD9NB+kVzFkBA/YlPELl1+AWvDg zNuzwzj9+gRI+QxWcDwcXRIYAcdpHil7tYq+HLEKHVBJfr12GJqlNR00KeuKGkLd1DRkVo6U9Eq5+ dbPNPfTMavy9SISm9n7kClqv71/HWUMXRGebc8Y25qYiYsL2eVSoLFd4E28qEwhhcjiZRonI7dhnr lG5WdwO9uJbUTslZdMcAoVkQVVn7Henq3eYYwVl7FjzpshOLS/IZPzTsnwqwfIm10BF2/BALpn9uC NQNRLNBnSeMq+r8l++cQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qBW5x-00AS95-1q; Tue, 20 Jun 2023 07:48:09 +0000 Received: from mail-oo1-xc2f.google.com ([2607:f8b0:4864:20::c2f]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qBW5u-00AS7N-0W for linux-arm-kernel@lists.infradead.org; Tue, 20 Jun 2023 07:48:07 +0000 Received: by mail-oo1-xc2f.google.com with SMTP id 006d021491bc7-55e57337756so1225080eaf.0 for ; Tue, 20 Jun 2023 00:48:05 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1687247285; x=1689839285; h=in-reply-to:references:message-id:to:from:subject:cc:date :content-transfer-encoding:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=9/UfqACRIbp+KfcxB9xT42vufkCG9I6wiWPxaYnWI9o=; b=YRkxkxfrSg9A9g1ooeKb1wazJtUjLoPERxhfKAH2GwD7qw69XbLPN1Ba1FtUAvm8FL QdF4uXVuR3ymVv0p1G1ASryHTPSZYRDV2/AzOy1m/umugzTdoMz95o2l4WRUF4Iwghp3 GPXCl2dLw4v4bhe3AzveZyBYxyqQZ3++OqPtuynx+nNPaBvXUWbtmTiP8XxosVRGw3af wiTb2HdEqZxd4zej9B8EQQMu678bU7zyk2fz4wu/zoqSNubkH3xaNhY9KOM9ajlVhA8T ELkPzQLgIAx+Z+fAf5N0tP2dDEdJyROExBnsn+lxAbjQYu38/OlA9Py/ox4kdV59okLI 4Lbg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1687247285; x=1689839285; h=in-reply-to:references:message-id:to:from:subject:cc:date :content-transfer-encoding:mime-version:x-gm-message-state:from:to :cc:subject:date:message-id:reply-to; bh=9/UfqACRIbp+KfcxB9xT42vufkCG9I6wiWPxaYnWI9o=; b=dg5lKYxHKhiLFe1ox7bZj/XYCPVkSBG1ng2fVOul9CftFMCQVTcdJxa7GpkYFSZji+ hYhgwhlbmjVKomQ+tQ4rqwh0BCGLDiDPzioA7FtPCkqqB+1IpOP7Gv9LvjSmZNxbXNYK spF0Ye5R6UwwkeMwiY6Oe0hXgDDxg1/ivcxuC83tda45aQnPdFJVew0bpMZzoODAONuL z903sBU8shxqoP5j/4gr1EszpmLTBABbYj2WooOgKaOiz84168Iboz4og1m951goQ1ug gi8Rk+yHjYhU5o8Jt00BAlnBpK4hyJWHamHCerDUWToYa1bxQlVgRq4X9icv3Vl0XWJd xP7w== X-Gm-Message-State: AC+VfDzXEiitUVgfV701wuS7LPJVhnrMw6AofHinFQYne31tWvfv+RVM 6p3hEyZdtsDUuwaxK4NGLnU= X-Google-Smtp-Source: ACHHUZ46XxwEUTpJsCjGTBnalzzt+j7MWruxVgUmQbM1nUv/3dXWVSuVX//QnCfJBuhR+6fpBvpozw== X-Received: by 2002:a05:6808:1a09:b0:39e:cb88:17f8 with SMTP id bk9-20020a0568081a0900b0039ecb8817f8mr9119278oib.7.1687247284740; Tue, 20 Jun 2023 00:48:04 -0700 (PDT) Received: from localhost ([124.170.190.103]) by smtp.gmail.com with ESMTPSA id nr5-20020a17090b240500b0025dc5749b4csm6789071pjb.21.2023.06.20.00.47.49 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 20 Jun 2023 00:48:04 -0700 (PDT) Mime-Version: 1.0 Date: Tue, 20 Jun 2023 17:47:47 +1000 Cc: "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() From: "Nicholas Piggin" To: "Yu Zhao" , "Andrew Morton" , "Paolo Bonzini" Message-Id: X-Mailer: aerc 0.14.0 References: <20230526234435.662652-1-yuzhao@google.com> <20230526234435.662652-8-yuzhao@google.com> In-Reply-To: <20230526234435.662652-8-yuzhao@google.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230620_004806_202578_3B8C04A8 X-CRM114-Status: GOOD ( 31.37 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org 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/include/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_FTR_ARCH_300) && > + radix_enabled(); This could probably be radix_enabled() && !kvmhv_on_pseries(). Although unclear why not nested hypervisor... I'd have to think about it 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). > +} > + > #endif /* __POWERPC_KVM_HOST_H__ */ > diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/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 *range); > + bool (*test_clear_young)(struct kvm *kvm, struct kvm_gfn_range *range); > bool (*set_spte_gfn)(struct kvm *kvm, struct kvm_gfn_range *range); > 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 kvm_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_range *range); > extern bool kvm_age_gfn_hv(struct kvm *kvm, struct kvm_gfn_range *range); > extern bool kvm_test_age_gfn_hv(struct kvm *kvm, struct kvm_gfn_range *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_range *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/kvm/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, struct 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 = range->start; > + > + rcu_read_lock(); > + > + err = !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 = 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 = 1 > + * smp_rmb() > + * Use kvm->arch.pgtable > + */ > + smp_rmb(); Comment could stand to expand slightly on what you are solving, in words. If you use synchronize_rcu() on both sides, you wouldn't need the barrier, right? > + while (gfn < range->end) { > + pte_t *ptep; > + pte_t old, new; > + unsigned int shift; > + > + ptep = find_kvm_secondary_pte_unlocked(kvm, gfn * PAGE_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). > + > + old = READ_ONCE(*ptep); > + if (!pte_present(old) || !pte_young(old)) > + goto next; > + > + new = 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 :) Thanks, Nick > +next: > + gfn += shift ? BIT(shift - PAGE_SHIFT) : 1; > + } > +unlock: > + rcu_read_unlock(); > + > + return err; > +} > + > /* Returns the number of PAGE_SIZE pages that are dirty */ > static int kvm_radix_test_clear_dirty(struct kvm *kvm, > struct kvm_memory_slot *memslot, int pagenum) > diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c > index 130bafdb1430..20a81ec9fde8 100644 > --- a/arch/powerpc/kvm/book3s_hv.c > +++ b/arch/powerpc/kvm/book3s_hv.c > @@ -5262,6 +5262,8 @@ int kvmppc_switch_mmu_to_hpt(struct kvm *kvm) > spin_lock(&kvm->mmu_lock); > kvm->arch.radix = 0; > spin_unlock(&kvm->mmu_lock); > + /* see the comments in kvm_test_clear_young_hv() */ I'm guilty of such comments at times, but it wouldn't hurt to say /* Finish concurrent kvm_test_clear_young_hv access to page tables */ Then you know where to look for more info and you have a vague idea what it's for. > + synchronize_rcu(); > kvmppc_free_radix(kvm); > > lpcr = LPCR_VPM1; > @@ -5286,6 +5288,8 @@ int kvmppc_switch_mmu_to_radix(struct kvm *kvm) > if (err) > return err; > kvmppc_rmap_reset(kvm); > + /* see the comments in kvm_test_clear_young_hv() */ > + smp_wmb(); > /* Mutual exclusion with kvm_unmap_gfn_range etc. */ > spin_lock(&kvm->mmu_lock); > kvm->arch.radix = 1; > @@ -6185,6 +6189,7 @@ static struct kvmppc_ops kvm_ops_hv = { > .unmap_gfn_range = kvm_unmap_gfn_range_hv, > .age_gfn = kvm_age_gfn_hv, > .test_age_gfn = kvm_test_age_gfn_hv, > + .test_clear_young = kvm_test_clear_young_hv, > .set_spte_gfn = kvm_set_spte_gfn_hv, > .free_memslot = kvmppc_core_free_memslot_hv, > .init_vm = kvmppc_core_init_vm_hv, Thanks for looking at the powerpc conversion! Thanks, Nick _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel