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 60886CDD0C6 for ; Tue, 22 Oct 2024 18:32:39 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To:Content-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=mlFaly6j0/+a98jYwOafYxG3F29MQ3cpEBziMjxrChE=; b=k4idIsxQEuw+d8b6NXo2KgqUMk wVEntVWKpz/+SPMKPHAKskWXQps3FUJx0W5z/NWm7+vnqy7/8Tm2H5qBAr3MB4eNnQJv0kvmJfUcX 5zo1y3Gk41o9wO/DaTtYhr5JcWUpMU1VawLzngA6ZvB4U+efW8zpixtVdAcDjCABGU0SA+r1pq8/c mIFG07XqoY8zg5FVCy2uj385uklyMALP7zUvlRdNjXozU3fHSdme5NVOJJRFo7OTgUlfghvIJMC8P iwyDzeUPy37q5n1/YQ2mtr/1DmjjZZ+SubBrAU7Y0Hr2xhc35eABX0ZnH02GaR4WLOZc4Naze+vTJ 4yUAu2+g==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98 #2 (Red Hat Linux)) id 1t3JgA-0000000Bocp-0AJj; Tue, 22 Oct 2024 18:32:26 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.98 #2 (Red Hat Linux)) id 1t3Jed-0000000BoVc-3Y1m for linux-arm-kernel@lists.infradead.org; Tue, 22 Oct 2024 18:30:53 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 92AF9497; Tue, 22 Oct 2024 11:31:18 -0700 (PDT) Received: from arm.com (usa-sjc-mx-foss1.foss.arm.com [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 15C523F71E; Tue, 22 Oct 2024 11:30:46 -0700 (PDT) Date: Tue, 22 Oct 2024 19:30:44 +0100 From: Catalin Marinas To: Yicong Yang Cc: will@kernel.org, maz@kernel.org, mark.rutland@arm.com, broonie@kernel.org, linux-arm-kernel@lists.infradead.org, oliver.upton@linux.dev, ryan.roberts@arm.com, linuxarm@huawei.com, jonathan.cameron@huawei.com, shameerali.kolothum.thodi@huawei.com, prime.zeng@hisilicon.com, xuwei5@huawei.com, wangkefeng.wang@huawei.com, yangyicong@hisilicon.com Subject: Re: [PATCH v3 3/5] arm64: Add support for FEAT_HAFT Message-ID: References: <20241022092734.59984-1-yangyicong@huawei.com> <20241022092734.59984-4-yangyicong@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20241022092734.59984-4-yangyicong@huawei.com> X-TUID: ojFA934owGDK X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20241022_113051_993523_EAE8F3B6 X-CRM114-Status: GOOD ( 30.72 ) 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: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, Oct 22, 2024 at 05:27:32PM +0800, Yicong Yang wrote: > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig > index 3e29b44d2d7b..029d7ad89de5 100644 > --- a/arch/arm64/Kconfig > +++ b/arch/arm64/Kconfig > @@ -2176,6 +2176,21 @@ config ARCH_PKEY_BITS > int > default 3 > > +config ARM64_HAFT > + bool "Support for Hardware managed Access Flag for Table Descriptor" Super nit: s/Descriptor/Descriptors/ > + depends on ARM64_HW_AFDBM > + default y > + help > + The ARMv8.9/ARMv9.5 introduces the feature Hardware managed Access > + Flag for Table descriptors. When enabled an architectural executed > + memory access will update the Access Flag in each Table descriptor > + which is accessed during the translation table walk and for which > + the Access Flag is 0. The Access Flag of the Table descriptor use > + the same bit of PTE_AF. > + > + The feature will only be enabled if all the CPUs in the system > + support this feature. If unsure, say Y. > + > endmenu # "ARMv8.9 architectural features" > > config ARM64_SVE > diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h > index 3d261cc123c1..fba2347c0aa6 100644 > --- a/arch/arm64/include/asm/cpufeature.h > +++ b/arch/arm64/include/asm/cpufeature.h > @@ -879,6 +879,30 @@ static inline bool cpu_has_hw_af(void) > ID_AA64MMFR1_EL1_HAFDBS_SHIFT); > } > > +/* > + * Contrary to the page/block access flag, the table access flag > + * cannot be emulated in software (no access fault will occur). > + * So users should use this feature only if it's supported system > + * wide. > + */ > +static inline bool system_support_haft(void) > +{ > + unsigned int hafdbs; > + u64 mmfr1; > + > + if (!IS_ENABLED(CONFIG_ARM64_HAFT)) > + return false; > + > + /* > + * Check the sanitised registers to see this feature is supported > + * on all the CPUs. > + */ > + mmfr1 = read_sanitised_ftr_reg(SYS_ID_AA64MMFR1_EL1); > + hafdbs = cpuid_feature_extract_unsigned_field(mmfr1, > + ID_AA64MMFR1_EL1_HAFDBS_SHIFT); > + return hafdbs >= ID_AA64MMFR1_EL1_HAFDBS_HAFT; > +} Can we not have just an entry in the arm64_features array with the type ARM64_CPUCAP_SYSTEM_FEATURE and avoid the explicit checks here? > diff --git a/arch/arm64/include/asm/pgalloc.h b/arch/arm64/include/asm/pgalloc.h > index 8ff5f2a2579e..bc1051d65125 100644 > --- a/arch/arm64/include/asm/pgalloc.h > +++ b/arch/arm64/include/asm/pgalloc.h > @@ -30,7 +30,7 @@ static inline void pud_populate(struct mm_struct *mm, pud_t *pudp, pmd_t *pmdp) > { > pudval_t pudval = PUD_TYPE_TABLE; > > - pudval |= (mm == &init_mm) ? PUD_TABLE_UXN : PUD_TABLE_PXN; > + pudval |= (mm == &init_mm) ? PUD_TABLE_AF | PUD_TABLE_UXN : PUD_TABLE_PXN; > __pud_populate(pudp, __pa(pmdp), pudval); > } Why not set the table AF for the task entries? I haven't checked the core code but normally when we map a pte it's mapped as young. While for table AF we wouldn't get a fault, I would have thought the core code follows the same logic. > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c > index 718728a85430..6eeaaa80f6fe 100644 > --- a/arch/arm64/kernel/cpufeature.c > +++ b/arch/arm64/kernel/cpufeature.c > @@ -2046,6 +2046,18 @@ static bool has_hw_dbm(const struct arm64_cpu_capabilities *cap, > > #endif > > +#if CONFIG_ARM64_HAFT > + > +static struct cpumask haft_cpus; > + > +static void cpu_enable_haft(struct arm64_cpu_capabilities const *cap) > +{ > + if (has_cpuid_feature(cap, SCOPE_LOCAL_CPU)) > + cpumask_set_cpu(smp_processor_id(), &haft_cpus); > +} > + > +#endif /* CONFIG_ARM64_HAFT */ > + > #ifdef CONFIG_ARM64_AMU_EXTN > > /* > @@ -2590,6 +2602,17 @@ static const struct arm64_cpu_capabilities arm64_features[] = { > .cpus = &dbm_cpus, > ARM64_CPUID_FIELDS(ID_AA64MMFR1_EL1, HAFDBS, DBM) > }, > +#endif > +#ifdef CONFIG_ARM64_HAFT > + { > + .desc = "Hardware managed Access Flag for Table Descriptor", > + .type = ARM64_CPUCAP_WEAK_LOCAL_CPU_FEATURE, I'd actually use ARM64_CPUCAP_SYSTEM_FEATURE here. We use something similar for HW DBM but there we get a fault and set the pte dirty. You combined it with a system_support_haft() that checks the sanitised regs but I'd rather have a static branch check via cpus_have_cap(). Even with your approach we can have a race with a late CPU hot-plugged that doesn't have the feature in the middle of some core code walking the page tables. With a system feature type, late CPUs not having the feature won't be brought online (if feature enabled) but in general I don't have much sympathy for SoC vendors combining CPUs with incompatible features ;). > + .capability = ARM64_HAFT, > + .matches = has_cpuid_feature, > + .cpu_enable = cpu_enable_haft, > + .cpus = &haft_cpus, > + ARM64_CPUID_FIELDS(ID_AA64MMFR1_EL1, HAFDBS, HAFT) > + }, [...] > diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S > index ccbae4525891..4a58b9b36eb6 100644 > --- a/arch/arm64/mm/proc.S > +++ b/arch/arm64/mm/proc.S > @@ -495,9 +495,15 @@ alternative_else_nop_endif > * via capabilities. > */ > mrs x9, ID_AA64MMFR1_EL1 > - and x9, x9, ID_AA64MMFR1_EL1_HAFDBS_MASK > + ubfx x9, x9, ID_AA64MMFR1_EL1_HAFDBS_SHIFT, #4 > cbz x9, 1f > orr tcr, tcr, #TCR_HA // hardware Access flag update > + > +#ifdef CONFIG_ARM64_HAFT > + cmp x9, ID_AA64MMFR1_EL1_HAFDBS_HAFT > + b.lt 1f > + orr tcr2, tcr2, TCR2_EL1x_HAFT > +#endif /* CONFIG_ARM64_HAFT */ I think we can skip the ID check here and always set the HAFT bit. We do something similar with MTE (not for TCR_HA though, don't remember why). -- Catalin