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 5D4ACFF8864 for ; Fri, 1 May 2026 04:45: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: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=1r+qPnVc+K8RwPqdxQHiPCpf5K9Efp8MptO/Z3D2vX4=; b=NFFNvzXyKa1ZEU mtZziAjiAcYrK4WSTR94zq5S7XZvogOazq1xM4fpXfKpuIQ29iwtqeJEf8255GOu+T1WcrfAmH69D TFHYG9CspZFeDZ34Fgq+jaa9SNLeVNMRinAIXiSZJCe1dQnHrXgnzoFT1TfwdQpGt4CSIfR8n53F0 Xe11C/qzRBzB4Lz+nF63MfiIs4y4yBlAGg/VPrN0kGC5VzXJAm8v7oaLyDf3hTc0HZocMgq2sDP9z qVPbUgYQNc4b1qLSUV2/wI/XTtvibOJAhJJYs2OHEszqo2bQUHcDTK/0xavopXuLBFqYtRIZQVGTh 0KxIDkKUcOBStwe6SoUw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98.2 #2 (Red Hat Linux)) id 1wIfkm-00000006Lep-0bgn; Fri, 01 May 2026 04:45:28 +0000 Received: from tor.source.kernel.org ([2600:3c04:e001:324:0:1991:8:25]) by bombadil.infradead.org with esmtps (Exim 4.98.2 #2 (Red Hat Linux)) id 1wIfkk-00000006LeJ-2Nc0 for linux-riscv@lists.infradead.org; Fri, 01 May 2026 04:45:26 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by tor.source.kernel.org (Postfix) with ESMTP id 1780960142; Fri, 1 May 2026 04:45:25 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 2F0D3C2BCB7; Fri, 1 May 2026 04:45:23 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1777610724; bh=0urt9bqbGp3y4rg3e1ksFahHj9pqpb5+cS/9lGfMSaU=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=iWME0HrS3+ZEPckX6tGyQtPb9xHhdBDN3q+ubDJmJc0Bih/PCJrsqizQtlxPBtWCF sHZaYjKEbEkY/RS/x7ZytmU2iTneqz2bS6vpVUlbGw5p155eV1RtlQP6PRg5QnpnQo Q4IphobPOyknjLvhiXir6NvcwdNDtoTwqbBT9h1KUqGo3g4NIvAI8LKqnTAn8By6Z/ XDp+VZ843+nTEfHigSg2wAqkxBO3pQTRZrMDcXiFVkWv4IexvL8zahxJympsqKuZtK n7Km8CIWL+eKkFSRXc/8+cigobYlkT+NclXrtpidDd+1MoYCzxqqq3aEkC3NSyMTpJ MVfmA/DXwSM9A== Date: Thu, 30 Apr 2026 21:45:21 -0700 From: Drew Fustini To: Reinette Chatre Subject: Re: [PATCH RFC v3 04/11] RISC-V: QoS: add CBQRI hardware interface Message-ID: References: <20260414-ssqosid-cbqri-rqsc-v7-0-v3-0-b3b2e7e9847a@kernel.org> <20260414-ssqosid-cbqri-rqsc-v7-0-v3-4-b3b2e7e9847a@kernel.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-BeenThere: linux-riscv@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: x86@kernel.org, Atish Patra , Adrien Ricciardi , Atish Kumar Patra , Conor Dooley , Nicolas Pitre , linux-kernel@vger.kernel.org, Gong Shuai , Liu Zhiwei , guo.wenjia23@zte.com.cn, Krzysztof Kozlowski , linux-riscv@lists.infradead.org, Rob Herring , Alexandre Ghiti , "Rafael J. Wysocki" , acpica-devel@lists.linux.dev, Robert Moore , liu.qingtao2@zte.com.cn, linux-acpi@vger.kernel.org, Ben Horgan , James Morse , Radim =?utf-8?B?S3LEjW3DocWZ?= , Dave Martin , Len Brown , Gong Shuai , Fenghua Yu , Chen Pei , Albert Ou , Kornel =?utf-8?Q?Dul=C4=99ba?= , Babu Moger , Weiwei Li , yunhui cui , Paul Walmsley , Ved Shanbhogue , Vasudevan Srinivasan , Tony Luck , Peter Newman , Conor Dooley , Samuel Holland , Palmer Dabbelt , Paul Walmsley , devicetree@vger.kernel.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-riscv" Errors-To: linux-riscv-bounces+linux-riscv=archiver.kernel.org@lists.infradead.org On Thu, Apr 30, 2026 at 04:15:05PM -0700, Reinette Chatre wrote: > Hi Drew, > > On 4/14/26 6:53 PM, Drew Fustini wrote: > > diff --git a/arch/riscv/kernel/qos/internal.h b/arch/riscv/kernel/qos/internal.h > > new file mode 100644 > > index 000000000000..edbcbd9471b1 > > --- /dev/null > > +++ b/arch/riscv/kernel/qos/internal.h > > ... > > > + > > +struct cbqri_config { > > + u64 cbm; /* capacity block mask */ > > + u64 rbwb; /* reserved bandwidth blocks */ > > +}; > > Is this struct necessary? From what I can tell it is used to pass a parameter to > cbqri_apply_cache_config() and cbqri_apply_bw_config() where each just picks the > one member of interest. Could parameter of interest just be provided directly to > cbqri_apply_cache_config() and cbqri_apply_bw_config() without bouncing it > through this struct first? Agreed, the struct is unnecessary indirection. The two callers each write one field and read it back immediately. I will drop it and pass cbm / rbwb directly. > > + > > +#endif /* _ASM_RISCV_QOS_INTERNAL_H */ > > diff --git a/arch/riscv/kernel/qos/qos_resctrl.c b/arch/riscv/kernel/qos/qos_resctrl.c > > new file mode 100644 > > index 000000000000..6d294f2f2504 > > --- /dev/null > > +++ b/arch/riscv/kernel/qos/qos_resctrl.c > > @@ -0,0 +1,432 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > + > > +#define pr_fmt(fmt) "qos: resctrl: " fmt > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include "internal.h" > > + > > +static struct cbqri_resctrl_res cbqri_resctrl_resources[RDT_NUM_RESOURCES]; > > + > > +static bool exposed_alloc_capable; > > +/* CDP (code data prioritization) on x86 is AT (access type) on RISC-V */ > > +static bool exposed_cdp_l2_capable; > > +static bool exposed_cdp_l3_capable; > > +static bool is_cdp_l2_enabled; > > +static bool is_cdp_l3_enabled; > > These CDP values could also be handled as part of struct cbqri_resctrl_res, similar > to how CDP is managed with struct rdt_hw_resource. There does seem to be a hidden > assumption in this implementation that if *any* cache controller at particular level > supports CDP (which will cause global exposed_cdp_l{2,3}_capable to be true) then all cache > controllers at that level are assumed to support CDP even though this property is > enumerated separately for every cache controller making it possible for this > support to not be uniform. > > ... Good point. This is fixed in v4 which I was about to send. The controller-level cap is now stored on each controller as cc.supports_alloc_at_code and cc.supports_alloc_at_data, and cbqri_resctrl_pick_caches() rejects a heterogeneous-CDP set at the same cache level. Therefore, resctrl never sees a global-capable bit that some controllers cannot honour. I will take a look at folding this into struct cbqri_resctrl_res to make it simpler. > > +/* Perform capacity allocation control operation on capacity controller */ > > +static int cbqri_cc_alloc_op(struct cbqri_controller *ctrl, int operation, int rcid, > > + enum resctrl_conf_type type) > > +{ > > + int reg_offset = CBQRI_CC_ALLOC_CTL_OFF; > > + int status; > > + u64 reg; > > + > > + reg = ioread64(ctrl->base + reg_offset); > > + reg &= ~CBQRI_CONTROL_REGISTERS_OP_MASK; > > + reg |= FIELD_PREP(CBQRI_CONTROL_REGISTERS_OP_MASK, operation); > > + reg &= ~CBQRI_CONTROL_REGISTERS_RCID_MASK; > > + reg |= FIELD_PREP(CBQRI_CONTROL_REGISTERS_RCID_MASK, rcid); > > + > > + /* CBQRI capacity AT is only supported on L2 and L3 caches for now */ > > + if (ctrl->type == CBQRI_CONTROLLER_TYPE_CAPACITY && > > + ((ctrl->cache.cache_level == 2 && is_cdp_l2_enabled) || > > + (ctrl->cache.cache_level == 3 && is_cdp_l3_enabled))) { > > + reg &= ~CBQRI_CONTROL_REGISTERS_AT_MASK; > > + switch (type) { > > + case CDP_CODE: > > + reg |= FIELD_PREP(CBQRI_CONTROL_REGISTERS_AT_MASK, > > + CBQRI_CONTROL_REGISTERS_AT_CODE); > > + break; > > + case CDP_DATA: > > + default: > > + reg |= FIELD_PREP(CBQRI_CONTROL_REGISTERS_AT_MASK, > > + CBQRI_CONTROL_REGISTERS_AT_DATA); > > + break; > > + } > > + } > > There does not seem to be any special enabling when resctrl enables CDP via > resctrl_arch_set_cdp_enabled() so I assume CDP is always enabled? Does that mean > that when CDP is disabled from resctrl perspective (thus, both is_cdp_l2_enabled > and is_cdp_l3_enabled are false, but exposed_cdp_l2_capable and exposed_cdp_l3_capable > are true) then the default behavior is that allocation applies to both code and data > and it is not necessary to set both CBQRI_CONTROL_REGISTERS_AT_CODE and > CBQRI_CONTROL_REGISTERS_AT_DATA? Good question. I think this is a bug. resctrl_arch_set_cdp_enabled() is implemented in v4 and toggles the is_cdp_l{2,3}_enabled flags. When CDP is disabled, the AT field is left at 0 in cbqri_cc_alloc_op(). But BQRI encodes AT=0 as data, so it only programs the data config and silently leaves code config at whatever the hardware default is. The fix would be to issue two CONFIG_LIMIT ops, AT=data and AT=code, for the same cbm when supports_alloc_at_code is true and CDP is disabled. Thanks for catching this. [..] > > + /* > > + * max_rmid is used by resctrl_arch_system_num_rmid_idx() > > + * Find the smallest mcid_count amongst all controllers. > > + */ > > + max_rmid = min(max_rmid, ctrl->mcid_count); > > This computation appears to include all controllers, whether they support monitoring or not. > What will ctrl->mcid_count be on a controller that does not support monitoring? Good catch. I will gate the minimum on ctrl->mon_capable. Thanks, Drew _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv