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 E41BCC369DC for ; Tue, 29 Apr 2025 16:00:13 +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:Content-Type:MIME-Version: References:In-Reply-To:Subject:Cc:To:From:Message-ID: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=V4nfCleZM/rZB6hBq0fVvrE61jggjUIhZu5M7gK6ilo=; b=3s6KyQnfk9fE298CYklFNMReio zAvS+9UEoNQjVjQeSRyqgdyOAO1bu652ps6ozefwLfTqoNRJ21RLp80zvbFSWC7XSwIL/a2Nv+54k vJZb4mfISTLet8132du3XZfaoLYGzUcJ70sYB21PB4I2jqcEuebH9ZZu8+q/+ORmhLFdRUIE/ZOxq ChjUN1dTEnsFfaXJDOgfWBw9bcmTC+h4Wy+PuKA+YLiIHFCMymwfaSU4iEOiIGF7O96jkwrKDGkcS nz9QzWtQE3XUfPVS0GMU9XL9b5NikjjE3pD2v4egZDx0JSQL1LVl6kENcd899wS8T+R/6qV+C2Urh J9/tOOPA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98.2 #2 (Red Hat Linux)) id 1u9nNI-0000000ABzU-3hA0; Tue, 29 Apr 2025 16:00:00 +0000 Received: from sea.source.kernel.org ([172.234.252.31]) by bombadil.infradead.org with esmtps (Exim 4.98.2 #2 (Red Hat Linux)) id 1u9n27-0000000A9TO-0War for linux-arm-kernel@lists.infradead.org; Tue, 29 Apr 2025 15:38:08 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by sea.source.kernel.org (Postfix) with ESMTP id 7EAB24392C; Tue, 29 Apr 2025 15:38:04 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 5FC4CC4CEE3; Tue, 29 Apr 2025 15:38:06 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1745941086; bh=59601Ig09d04Qy4EQZj0MsYCMTZuBRq5iNyIeqGERhk=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=eJLh/JrqbeVQZlZMi5bJIO0i3HWaOK1fMNCoN83s6Yd0id+LvMGcAk52UavDHEXXV PIflSN2oH5u+NbKlLjMtcEchK96FGvjDAW3iPk5vEXf5v1F+kNZIALEsHcswlmoC84 +Ld/BCpSxjtJS9EE3+HLD12pmUCcvJhxDqFRYrJLRLT+LbnGWzJEll9lrxlxdXZVQo YJw70CLQxSHkqPb4AIPWHIqnlpLf6Nn0z+N+VabiLsjYHjY9GCfdccdNS+S3TjIKrp 74s22EkvkDLnIl37BYSgk4xK772UCeNJkq5Qfn8XM+EG1N9F+WHL+ygw8/UpdGr59Q 2VWsANknxjVdg== Received: from sofa.misterjones.org ([185.219.108.64] helo=goblin-girl.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.95) (envelope-from ) id 1u9n23-009xzs-5Y; Tue, 29 Apr 2025 16:38:04 +0100 Date: Tue, 29 Apr 2025 16:38:02 +0100 Message-ID: <86cycvhtb9.wl-maz@kernel.org> From: Marc Zyngier To: Lorenzo Pieralisi Cc: Thomas Gleixner , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Catalin Marinas , Will Deacon , Arnd Bergmann , Sascha Bischoff , Timothy Hayes , "Liam R. Howlett" , Mark Rutland , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org Subject: Re: [PATCH v2 19/22] irqchip/gic-v5: Add GICv5 CPU interface/IRS support In-Reply-To: References: <20250424-gicv5-host-v2-0-545edcaf012b@kernel.org> <20250424-gicv5-host-v2-19-545edcaf012b@kernel.org> <868qnkjngi.wl-maz@kernel.org> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/30.1 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: lpieralisi@kernel.org, tglx@linutronix.de, robh@kernel.org, krzk+dt@kernel.org, conor+dt@kernel.org, catalin.marinas@arm.com, will@kernel.org, arnd@arndb.de, sascha.bischoff@arm.com, timothy.hayes@arm.com, Liam.Howlett@oracle.com, mark.rutland@arm.com, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20250429_083807_210530_D6986081 X-CRM114-Status: GOOD ( 81.04 ) 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, 29 Apr 2025 15:54:07 +0100, Lorenzo Pieralisi wrote: > > On Mon, Apr 28, 2025 at 04:49:17PM +0100, Marc Zyngier wrote: > > On Thu, 24 Apr 2025 11:25:30 +0100, > > Lorenzo Pieralisi wrote: > > > > > > Implement GICv5 CPU interface and IRS support, to manage interrupt > > > state, priority and routing for all GICv5 interrupt types. > > > > > > The GICv5 CPU interface implements support for PE-Private Peripheral > > > Interrupts (PPI), that are handled (enabled/prioritized/delivered) > > > entirely within the CPU interface hardware. > > > > > > To enable PPI interrupts, implement the baseline GICv5 host kernel > > > driver infrastructure required to handle interrupts on a GICv5 system. > > > > > > Add the exception handling code path and definitions for GICv5 > > > instructions. > > > > > > Add GICv5 PPI handling code as a specific IRQ domain to: > > > > > > - Set-up PPI priority > > > - Manage PPI configuration and state > > > - Manage IRQ flow handler > > > - IRQs allocation/free > > > - Hook-up a PPI specific IRQchip to provide the relevant methods > > > > > > PPI IRQ priority is chosen as the minimum allowed priority by the > > > system design (after probing the number of priority bits implemented > > > by the CPU interface). > > > > > > The GICv5 Interrupt Routing Service (IRS) component implements > > > interrupt management and routing in the GICv5 architecture. > > > > > > A GICv5 system comprises one or more IRSes, that together > > > handle the interrupt routing and state for the system. > > > > > > An IRS supports Shared Peripheral Interrupts (SPIs), that are > > > interrupt sources directly connected to the IRS; they do not > > > rely on memory for storage. The number of supported SPIs is > > > fixed for a given implementation and can be probed through IRS > > > IDR registers. > > > > > > SPI interrupt state and routing are managed through GICv5 > > > instructions. > > > > > > Each core (PE in GICv5 terms) in a GICv5 system is identified with > > > an Interrupt AFFinity ID (IAFFID). > > > > > > An IRS manages a set of cores that are connected to it. > > > > > > Firmware provides a topology description that the driver uses > > > to detect to which IRS a CPU (ie an IAFFID) is associated with. > > > > > > Use probeable information and firmware description to initialize > > > the IRSes and implement GICv5 IRS SPIs support through an > > > SPI-specific IRQ domain. > > > > > > The GICv5 IRS driver: > > > > > > - Probes IRSes in the system to detect SPI ranges > > > - Associates an IRS with a set of cores connected to it > > > - Adds an IRQchip structure for SPI handling > > > > > > SPIs priority is set to a value corresponding to the lowest > > > permissible priority in the system (taking into account the > > > implemented priority bits of the IRS and CPU interface). > > > > > > Since all IRQs are set to the same priority value, the value > > > itself does not matter as long as it is a valid one. > > > > > > An IRS supports Logical Peripheral Interrupts (LPIs) and implement > > > Linux IPIs on top of it. > > > > > > LPIs are used for interrupt signals that are translated by a > > > GICv5 ITS (Interrupt Translation Service) but also for software > > > generated IRQs - namely interrupts that are not driven by a HW > > > signal, ie IPIs. > > > > > > LPIs rely on memory storage for interrupt routing and state. > > > > > > Memory storage is handled by the IRS - that is configured > > > at probe time by the driver with the required memory. > > > > > > LPIs state and routing information is kept in the Interrupt > > > State Table (IST). > > > > > > IRSes provide support for 1- or 2-level IST tables configured > > > to support a maximum number of interrupts that depend on the > > > OS configuration and the HW capabilities. > > > > > > On systems that provide 2-level IST support, always allow > > > the maximum number of LPIs; On systems with only 1-level > > > support, limit the number of LPIs to 2^12 to prevent > > > wasting memory (presumably a system that supports a 1-level > > > only IST is not expecting a large number of interrupts). > > > > > > On a 2-level IST system, L2 entries are allocated on > > > demand. > > > > > > The IST table memory is allocated using the kmalloc() interface; > > > the allocation required may be smaller than a page and must be > > > made up of contiguous physical pages if larger than a page. > > > > > > On systems where the IRS is not cache-coherent with the CPUs, > > > cache mainteinance operations are executed to clean and > > > invalidate the allocated memory to the point of coherency > > > making it visible to the IRS components. > > > > > > Add an LPI IRQ domain to: > > > > > > - Manage LPI state and routing > > > - Add LPI IRQchip structure and callbacks > > > - LPI domain allocation/de-allocation > > > > > > On GICv5 systems, IPIs are implemented using LPIs. > > > > > > Implement an IPI-specific IRQ domain created as a child/subdomain > > > of the LPI domain to allocate the required number of LPIs needed > > > to implement the IPIs. > > > > > > Move the arm64 IPI enum declaration to a header file so that the > > > GICv5 driver code can detect how many IPIs are required by arch code. > > > > > > IPIs are backed by LPIs, add LPIs allocation/de-allocation > > > functions. > > > > > > The LPI INTID namespace is managed using an IDA to alloc/free LPI > > > INTIDs. > > > > > > Associate an IPI irqchip with IPI IRQ descriptors to provide > > > core code with the irqchip.ipi_send_single() method required > > > to raise an IPI. > > > > > > Co-developed-by: Sascha Bischoff > > > Signed-off-by: Sascha Bischoff > > > Co-developed-by: Timothy Hayes > > > Signed-off-by: Timothy Hayes > > > Signed-off-by: Lorenzo Pieralisi > > > Cc: Will Deacon > > > Cc: Thomas Gleixner > > > Cc: Catalin Marinas > > > Cc: Marc Zyngier > > > --- > > > MAINTAINERS | 2 + > > > arch/arm64/include/asm/arch_gicv5.h | 91 +++ > > > arch/arm64/include/asm/smp.h | 17 + > > > arch/arm64/kernel/smp.c | 17 - > > > drivers/irqchip/Kconfig | 5 + > > > drivers/irqchip/Makefile | 1 + > > > drivers/irqchip/irq-gic-v5-irs.c | 841 ++++++++++++++++++++++++++++ > > > drivers/irqchip/irq-gic-v5.c | 1058 +++++++++++++++++++++++++++++++++++ > > > drivers/irqchip/irq-gic-v5.h | 184 ++++++ > > > > Nit: the split between include/asm/arch_gicv5.h and > > drivers/irqchip/irq-gic-v5.h is pretty pointless (this is obviously > > inherited from the GICv3 on 32bit setup). Given that GICv5 is strictly > > arm64, this split is no longer necessary. > > That's true but I thought I would leave sys instructions and barriers > in arm64 arch code headers rather than moving them out. I am up for > whatever you folks think it is best. I can see two options: - either you unify the two and place the result in include/linux/irqchip/arm-gic-v5.h - or you move the system stuff and the barriers to arch/arm64/include/asm/sysreg.h together with the rest of the pile, *and* move the other include file as above > > > +#define GSB_ACK __emit_inst(0xd5000000 | sys_insn(1, 0, 12, 0, 1) | 31) > > > +#define GSB_SYS __emit_inst(0xd5000000 | sys_insn(1, 0, 12, 0, 0) | 31) > > > > Can you please express this in terms of __SYS_BARRIER_INSN(), with a > > slight rework of the SB definition? It would limit the propagation of > > the 0xd5 constant and make it clear what 31 stands for. > > I tried but the __SYS_BARRIER_INSN() is a different class (GICv5 > barriers are sys instructions the SB barrier has a slightly different > encoding), so maybe something like this ? > > #define GSB_ACK __msr_s(sys_insn(1, 0, 12, 0, 1), "xzr") Why a different encoding? It looks completely similar to me. What I was expecting to see is something along the lines of (completely untested): diff --git a/arch/arm64/include/asm/barrier.h b/arch/arm64/include/asm/barrier.h index 1ca947d5c9396..7a00781d1d788 100644 --- a/arch/arm64/include/asm/barrier.h +++ b/arch/arm64/include/asm/barrier.h @@ -44,6 +44,8 @@ SB_BARRIER_INSN"nop\n", \ ARM64_HAS_SB)) +#define gsb_ack() asm volatile("GSC_ACK_BARRIER_INSN\n" : : : "memory") + #ifdef CONFIG_ARM64_PSEUDO_NMI #define pmr_sync() \ do { \ diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h index 690b6ebd118f4..a84993c3d117b 100644 --- a/arch/arm64/include/asm/sysreg.h +++ b/arch/arm64/include/asm/sysreg.h @@ -112,10 +112,13 @@ /* Register-based PAN access, for save/restore purposes */ #define SYS_PSTATE_PAN sys_reg(3, 0, 4, 2, 3) -#define __SYS_BARRIER_INSN(CRm, op2, Rt) \ - __emit_inst(0xd5000000 | sys_insn(0, 3, 3, (CRm), (op2)) | ((Rt) & 0x1f)) +#define __SYS_BARRIER_INSN(op0, op1, CRn, CRm, op2, Rt) \ + __emit_inst(0xd5000000 | \ + sys_insn((op0), (op1), (CRn), (CRm), (op2)) | \ + ((Rt) & 0x1f)) -#define SB_BARRIER_INSN __SYS_BARRIER_INSN(0, 7, 31) +#define SB_BARRIER_INSN __SYS_BARRIER_INSN(0, 3, 3, 0, 7, 31) +#define GSB_ACK_BARRIER_INSN __SYS_BARRIER_INSN(1, 0, 12, 0, 1, 31) /* Data cache zero operations */ #define SYS_DC_ISW sys_insn(1, 0, 7, 6, 2) and the same thing for GSB SYS. > > > +#include "irq-gic-v5.h" > > > > Why the ""? The normal include path (linux/irqchip) should work. > > irq-gic-v5.h lives in drivers/irqchip, should I move it into > include/linux/irqchip (or I am missing something, likely) ? > > I did not do it because it is only consumed by files in that directory. I full intend for KVM to consume all of this pretty much directly, and the KVM code will definitely not live in drivers/irqchip. > > > + ret = readl_poll_timeout_atomic(reg, tmp, tmp & mask, 1, 10 * USEC_PER_MSEC); > > > + > > > + if (val) > > > + *val = tmp; > > > > Updating the result value on error is rather odd. Do you rely on this > > anywhere? If not, consider avoiding the write-back on timeout. > > I do rely on it to avoid reading the register once again on return if > the wait succeeded (but we don't know unless for some registers we > check another bit in the returned value). That's a bit annoying. But hey... > > > + /* > > > + * The polling wait (in gicv5_wait_for_op()) on a GIC register > > > + * provides the memory barriers (through MMIO accessors) > > > + * required to synchronize CPU and GIC access to IST memory. > > > + */ > > > > This comment would be better placed with the helper itself, and avoid > > the repeats that I can see in the rest of the code. > > On this, I thought about that. The problem is, the comment would be > buried in a helper function whereas it is in this function that we > are actually handing over memory to the GIC so in a way I thought > it was more appropriate to add it where the explanation is needed > and possibly clearer. It's not clearer. If you want it to be clearer, name the helper in a way that makes it actions explicit (gicv5_irs_ist_synchronise() ?), and let the comment explain how the synchronisation is enforced next to the helper. Using explicit names is far better than duplicating comments, IMHO. > > > + list_for_each_entry(irs_data, &irs_nodes, entry) { > > > + if (!irs_data->spi_range) > > > + continue; > > > + > > > + min = irs_data->spi_min; > > > + max = irs_data->spi_min + irs_data->spi_range - 1; > > > + if (spi_id >= min && spi_id <= max) > > > + return irs_data; > > > + } > > > > Funnily enough, this is exactly the sort of iterative searches the > > maple tree could have avoided, by storing INTID (and range) specific > > data. Never mind. > > I did it with a range store with an Xarray but then removed it because > I thought it was overkill, one of those things I am not sure what's > best. Probably not an issue for now. Time will tell as we get larger machines with multiple IRSes. > > Just a passing comment: consider splitting this patch in two (IRS on > > one side, CPUif on the other). I'm only half-way through, and it's > > quite tiring... :-/ > > Well, on this I am having a hard time as I replied to Thomas as well. > > We do need CPUIF + IRS (LPI so IPIs) to make this a functional > patch. We don't need things to be functional. We need things to not break the build. So as long as things are split in a logical way and, this should be fine. > If I split per component, this might simplify review (and that's what > I did in v1 but Thomas complained that could not find SMP > initialization). Well, you could have one patch clearly labelled as "SMP init", which would both satisfy tglx's requirements *and* my tired brain. > > What I could do to reduce the number of lines is moving SPI support > in a separate patch but I don't think there is a way to split CPUIF > and IRS and have a patch series that is bisectable and works at every > stage (by the way the last patch should be merged with this one because > technically with this patch we have a working GICv5). The trivial way to make it bisectable is to avoid compiling it until it is sufficiently feature-complete. Any sizeable amount of new code relies on this to a degree or another. Also, think of the commit messages: what you have here is a massive wall of text that would be more appropriate for a cover letter, and could be much smaller on a per-patch basis without losing any meaningful content. Thanks, M. -- Without deviation from the norm, progress is not possible.