Received: by 2002:a05:6358:11c7:b0:104:8066:f915 with SMTP id i7csp2217172rwl; Thu, 13 Apr 2023 03:26:08 -0700 (PDT) X-Google-Smtp-Source: AKy350b0asJpTCSZ/66C6sRBpSW1gbYcwLFjFkYHDHefLEsotmBJhZnYuJTSIVdyIISVxE373YG6 X-Received: by 2002:a17:90a:b796:b0:23d:1b82:7236 with SMTP id m22-20020a17090ab79600b0023d1b827236mr1550528pjr.16.1681381568305; Thu, 13 Apr 2023 03:26:08 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1681381568; cv=none; d=google.com; s=arc-20160816; b=X86byvweOIMM+Iu5r4hpTI4rcN12T1VfF97CcxEOIXLfCG7U3NSqwjy98AFCyaGJW4 aS/XVnfyIP51Blyx53d7z1RSZTShQsys1qfXNMhPK42fzeiDFkQI8y+EZBjiBcnOa4Nx QI147kQhBCypiPbqjAWr0QeW7yPHpOHwR0flRSCxQshpWNae+GLq1KQ7wI1xqECBQRVM 9WrqnyC27HICETIwtETvbWiQbfmJV1bm5/NWQthY9N4H0Z4e5QMZbPnTxezqVthiumVb SbMLwNfns+g9X4JRXXPTKWXMK/2lrTt2kZyLLBNCxBHeLNZGHRc1zPUs06UksAxwMi3q AI4g== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:in-reply-to:content-disposition:mime-version :references:message-id:subject:cc:to:from:date; bh=3oGeBATb8j1lDF1URFSaFkZ9xDOtINHIbwRTXxa5ktM=; b=0FLIZvksfU44B39pOIqYJ8U1unSy4Zd3IPJnQYi0Qxj6pEn2AWjeu7RWaxPa9oRHFd Jd2ukKoZvCXvFR4uWz0r2KQMppip80Q+wJLbn7J9ixbTx6FZbtToab5a3nNYDodbPoku /IYQ5S2eLNmjq6OuX3r4bOfuV80HFmLlPgV1/4y7k15WXiy4hYIswD+1yEW8xhzWh3Ue rhJ0j6fGO/CEnKMEuO+0gmCYg3lMt8vTdWMIEMX/KE9yZUzuqsXN16eGQM5JShEboWwx 3qaGEomC5UiWVHf7C6AylMXatG9bCniL9z2xRZBAAZdP596BDimbNx8b/FQGLAAxiTnu tzVg== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=arm.com Return-Path: Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id t2-20020a17090ad50200b0024698d04db8si1672526pju.25.2023.04.13.03.25.55; Thu, 13 Apr 2023 03:26:08 -0700 (PDT) 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; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=arm.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230017AbjDMKUM (ORCPT + 99 others); Thu, 13 Apr 2023 06:20:12 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:40090 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229598AbjDMKUK (ORCPT ); Thu, 13 Apr 2023 06:20:10 -0400 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id 42B9A118 for ; Thu, 13 Apr 2023 03:20:09 -0700 (PDT) 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 7A7154B3; Thu, 13 Apr 2023 03:20:53 -0700 (PDT) Received: from bogus (e103737-lin.cambridge.arm.com [10.1.197.49]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 93A3F3F6C4; Thu, 13 Apr 2023 03:20:07 -0700 (PDT) Date: Thu, 13 Apr 2023 11:20:05 +0100 From: Sudeep Holla To: Pierre Gondois Cc: linux-kernel@vger.kernel.org, Radu Rendec , Alexandre Ghiti , Sudeep Holla , Conor Dooley , Will Deacon , Greg Kroah-Hartman , "Rafael J. Wysocki" , Palmer Dabbelt , Gavin Shan Subject: Re: [PATCH v3 4/4] cacheinfo: Add use_arch[|_cache]_info field/function Message-ID: <20230413102005.u3ohh2mlxdsoz7ka@bogus> References: <20230413091436.230134-1-pierre.gondois@arm.com> <20230413091436.230134-5-pierre.gondois@arm.com> <20230413094952.ncdldvv2ysjqwbsv@bogus> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_MED, SPF_HELO_NONE,SPF_NONE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Apr 13, 2023 at 12:17:09PM +0200, Pierre Gondois wrote: > > > On 4/13/23 11:49, Sudeep Holla wrote: > > On Thu, Apr 13, 2023 at 11:14:34AM +0200, Pierre Gondois wrote: > > > The cache information can be extracted from either a Device > > > Tree (DT), the PPTT ACPI table, or arch registers (clidr_el1 > > > for arm64). > > > > > > The clidr_el1 register is used only if DT/ACPI information is not > > > available. It does not states how caches are shared among CPUs. > > > > > > Add a use_arch_cache_info field/function to identify when the > > > DT/ACPI doesn't provide cache information. Use this information > > > to assume L1 caches are privates and L2 and higher are shared among > > > all CPUs. > > > > > > > I have tentatively merged first 3 patches along with Radu's series(waiting > > for build tests still before confirming). I am not yet sure on this. > > > > > Signed-off-by: Pierre Gondois > > > --- > > > drivers/base/cacheinfo.c | 13 ++++++++++++- > > > include/linux/cacheinfo.h | 10 ++++++++++ > > > 2 files changed, 22 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c > > > index 06de9a468958..49dbb4357911 100644 > > > --- a/drivers/base/cacheinfo.c > > > +++ b/drivers/base/cacheinfo.c > > > @@ -40,7 +40,8 @@ static inline bool cache_leaves_are_shared(struct cacheinfo *this_leaf, > > > * For non DT/ACPI systems, assume unique level 1 caches, > > > * system-wide shared caches for all other levels. > > > */ > > > - if (!(IS_ENABLED(CONFIG_OF) || IS_ENABLED(CONFIG_ACPI))) > > > + if (!(IS_ENABLED(CONFIG_OF) || IS_ENABLED(CONFIG_ACPI)) || > > > + this_leaf->use_arch_info) > > > > Can't we just use use_arch_cache_info() here ? > > I think that if we use use_arch_cache_info() here, then arm64 platforms > will always return here and never check fw_token/this_leaf->id values. > Indeed, we also need to know that no cache information is available in > DT/ACPI, cf. [1] > Ah right, I missed to see that. I was sure there is a reason but couldn't figure out myself quickly. > > > > > return (this_leaf->level != 1) && (sib_leaf->level != 1); > > > if ((sib_leaf->attributes & CACHE_ID) && > > > @@ -349,6 +350,7 @@ static int cache_shared_cpu_map_setup(unsigned int cpu) > > > struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu); > > > struct cacheinfo *this_leaf, *sib_leaf; > > > unsigned int index, sib_index; > > > + bool use_arch_info = false; > > > int ret = 0; > > > if (this_cpu_ci->cpu_map_populated) > > > @@ -361,6 +363,12 @@ static int cache_shared_cpu_map_setup(unsigned int cpu) > > > */ > > > if (!last_level_cache_is_valid(cpu)) { > > > ret = cache_setup_properties(cpu); > > > + if (ret && use_arch_cache_info()) { > > > + // Possibility to rely on arch specific information. > > [1] > > > > + use_arch_info = true; > > > + ret = 0; > > > + } > > > + > > > if (ret) > > > return ret; > > > } > > > @@ -370,6 +378,9 @@ static int cache_shared_cpu_map_setup(unsigned int cpu) > > > this_leaf = per_cpu_cacheinfo_idx(cpu, index); > > > + if (use_arch_info) > > > + this_leaf->use_arch_info = true; > > > + > > > cpumask_set_cpu(cpu, &this_leaf->shared_cpu_map); > > > for_each_online_cpu(i) { > > > struct cpu_cacheinfo *sib_cpu_ci = get_cpu_cacheinfo(i); > > > diff --git a/include/linux/cacheinfo.h b/include/linux/cacheinfo.h > > > index 908e19d17f49..fed675b251a2 100644 > > > --- a/include/linux/cacheinfo.h > > > +++ b/include/linux/cacheinfo.h > > > @@ -66,6 +66,7 @@ struct cacheinfo { > > > #define CACHE_ALLOCATE_POLICY_MASK \ > > > (CACHE_READ_ALLOCATE | CACHE_WRITE_ALLOCATE) > > > #define CACHE_ID BIT(4) > > > + bool use_arch_info; > > > > Do you see the need to stash this value as it is either globally true or > > false based on the arch ? > > A static variable could be used instead and set to true if we fail to fetch the > cache information from DT/ACPI, cf. [1]. The only possible transition for this > variable would be from false->true. I'll check if this works like this. > Yes that would be good. > > > > > void *fw_token; > > > bool disable_sysfs; > > > void *priv; > > > @@ -129,4 +130,13 @@ static inline int get_cpu_cacheinfo_id(int cpu, int level) > > > return -1; > > > } > > > +static inline bool use_arch_cache_info(void) > > > +{ > > > +#if defined(CONFIG_ARM64) > > > + return true; > > > +#else > > > + return false; > > > +#endif > > > +} > > > + > > > > Can we just have it as: > > #ifdef CONFIG_ARM64 > > #define use_arch_cache_info() (true) > > #else > > #define use_arch_cache_info() (false) > > #endif > > Yes sure, I'll post a v4 with this along Conor's requested change. > Sure. -- Regards, Sudeep