| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aDgTdorrkNFg7Jkf@lpieralisi>
Date: Thu, 29 May 2025 09:57:42 +0200
From: Lorenzo Pieralisi <lpieralisi@...nel.org>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>
Cc: Marc Zyngier <maz@...nel.org>, Thomas Gleixner <tglx@...utronix.de>,
Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
Catalin Marinas <catalin.marinas@....com>,
Will Deacon <will@...nel.org>, Arnd Bergmann <arnd@...db.de>,
Sascha Bischoff <sascha.bischoff@....com>,
Timothy Hayes <timothy.hayes@....com>,
"Liam R. Howlett" <Liam.Howlett@...cle.com>,
Mark Rutland <mark.rutland@....com>,
Jiri Slaby <jirislaby@...nel.org>,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
devicetree@...r.kernel.org
Subject: Re: [PATCH v4 20/26] irqchip/gic-v5: Add GICv5 PPI support
On Wed, May 28, 2025 at 03:15:24PM +0100, Jonathan Cameron wrote:
[...]
> > +/* Shift and mask definitions for GIC CDDI */
>
> Technically just masks (which are shifted) but none the less I wouldn't
> expect the comment to say Shift and mask.
Fixed.
>
> > +#define GICV5_GIC_CDDI_TYPE_MASK GENMASK_ULL(31, 29)
> > +#define GICV5_GIC_CDDI_ID_MASK GENMASK_ULL(23, 0)
> > +
> > +/* Shift and mask definitions for GICR CDIA */
>
> Likewise.
>
> > +#define GICV5_GIC_CDIA_VALID_MASK BIT_ULL(32)
>
> Maybe
> GICV5_GICR_CDIA_VALID(r) etc given the instruction define name.
Yes I can do that.
> > +#define GICV5_GIC_CDIA_VALID(r) FIELD_GET(GICV5_GIC_CDIA_VALID_MASK, r)
>
> Personally I rarely see benefit in wrapping FIELD_GET() in another macro
> The bare code is only a little shorter and the FIELD_GET() inline keeps things nice
> and clear. It's your code though so keep this if you really want to!
For these things to be honest I am reluctant to change them, it is
subjective - we may end up arguing forever.
> > +#define GICV5_GIC_CDIA_TYPE_MASK GENMASK_ULL(31, 29)
> > +#define GICV5_GIC_CDIA_ID_MASK GENMASK_ULL(23, 0)
> > +
> > +#define gicr_insn(insn) read_sysreg_s(GICV5_OP_GICR_##insn)
> > +#define gic_insn(v, insn) write_sysreg_s(v, GICV5_OP_GIC_##insn)
> >
> > #define ARM64_FEATURE_FIELD_BITS 4
> >
> > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> > index 08bb3b031f23093311cf2f0918ad43e575b581d1..0f268f35b78531775aa233bfc362bfe119a68275 100644
> > --- a/drivers/irqchip/Kconfig
> > +++ b/drivers/irqchip/Kconfig
> > @@ -54,6 +54,11 @@ config ARM_GIC_V3_ITS_FSL_MC
> > depends on FSL_MC_BUS
> > default ARM_GIC_V3_ITS
> >
> > +config ARM_GIC_V5
> > + bool
> > + select IRQ_DOMAIN_HIERARCHY
> > + select GENERIC_IRQ_EFFECTIVE_AFF_MASK
> > +
> > config ARM_NVIC
> > bool
> > select IRQ_DOMAIN_HIERARCHY
> > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> > index 365bcea9a61ff89e2cb41034125b3fc8cd494d81..3f8225bba5f0f9ce5dbb629b6d4782eacf85da44 100644
> > --- a/drivers/irqchip/Makefile
> > +++ b/drivers/irqchip/Makefile
> > @@ -35,6 +35,7 @@ obj-$(CONFIG_ARM_GIC_V3) += irq-gic-v3.o irq-gic-v3-mbi.o irq-gic-common.o
> > obj-$(CONFIG_ARM_GIC_V3_ITS) += irq-gic-v3-its.o irq-gic-v4.o irq-gic-v3-its-msi-parent.o
> > obj-$(CONFIG_ARM_GIC_V3_ITS_FSL_MC) += irq-gic-v3-its-fsl-mc-msi.o
> > obj-$(CONFIG_PARTITION_PERCPU) += irq-partition-percpu.o
> > +obj-$(CONFIG_ARM_GIC_V5) += irq-gic-v5.o
> > obj-$(CONFIG_HISILICON_IRQ_MBIGEN) += irq-mbigen.o
> > obj-$(CONFIG_ARM_NVIC) += irq-nvic.o
> > obj-$(CONFIG_ARM_VIC) += irq-vic.o
> > diff --git a/drivers/irqchip/irq-gic-v5.c b/drivers/irqchip/irq-gic-v5.c
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..a50982e5d98816d88e4fca37cc0ac31684fb6c76
> > --- /dev/null
> > +++ b/drivers/irqchip/irq-gic-v5.c
> > @@ -0,0 +1,460 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (C) 2024-2025 ARM Limited, All Rights Reserved.
> > + */
> > +
> > +#define pr_fmt(fmt) "GICv5: " fmt
> > +
> > +#include <linux/irqdomain.h>
> > +#include <linux/wordpart.h>
> > +
> > +#include <linux/irqchip.h>
> > +#include <linux/irqchip/arm-gic-v5.h>
> > +
> > +#include <asm/cpufeature.h>
> > +#include <asm/exception.h>
> > +
> > +static u8 pri_bits __ro_after_init = 5;
> > +
> > +#define GICV5_IRQ_PRI_MASK 0x1f
> > +#define GICV5_IRQ_PRI_MI (GICV5_IRQ_PRI_MASK & GENMASK(4, 5 - pri_bits))
>
>
> > +struct gicv5_chip_data {
> > + struct fwnode_handle *fwnode;
> > + struct irq_domain *ppi_domain;
> > +};
> > +
> > +static struct gicv5_chip_data gicv5_global_data __read_mostly;
>
>
> > +enum {
> > + PPI_PENDING,
> > + PPI_ACTIVE,
> > + PPI_HM
> > +};
> > +
> > +static __always_inline u64 read_ppi_sysreg_s(unsigned int irq,
> > + const unsigned int which)
>
> Name the enum and use that here rather than an unsigned int?
> Might as well give the compiler a hand.
> Maybe I'm missing a later use of this that means we can't do that.
>
> This is almost enough combinations to justify a look up table but
> I guess the compiler might not figure out how to optimize that.
Yes, while writing it I initially used a named enum, then switched
to this, does not change anything, function is always inline and either
the compiler manages to remove switch cases or the build fails :)
so at the end of the day a named enum does not change much, I
will change it to that though.
>
> > +{
> > + switch (which) {
> > + case PPI_PENDING:
> > + return irq < 64 ? read_sysreg_s(SYS_ICC_PPI_SPENDR0_EL1) :
> > + read_sysreg_s(SYS_ICC_PPI_SPENDR1_EL1);
> > + case PPI_ACTIVE:
> > + return irq < 64 ? read_sysreg_s(SYS_ICC_PPI_SACTIVER0_EL1) :
> > + read_sysreg_s(SYS_ICC_PPI_SACTIVER1_EL1);
> > + case PPI_HM:
> > + return irq < 64 ? read_sysreg_s(SYS_ICC_PPI_HMR0_EL1) :
> > + read_sysreg_s(SYS_ICC_PPI_HMR1_EL1);
> > + default:
> > + BUILD_BUG_ON(1);
> > + }
> > +}
> > +
> > +static __always_inline void write_ppi_sysreg_s(unsigned int irq, bool set,
> > + const unsigned int which)
>
> Likewise - nicer with enum perhaps.
Done.
>
> > +{
> > + u64 bit = BIT_ULL(irq % 64);
> > +
> > + switch (which) {
> > + case PPI_PENDING:
> > + if (set) {
> > + if (irq < 64)
> > + write_sysreg_s(bit, SYS_ICC_PPI_SPENDR0_EL1);
> > + else
> > + write_sysreg_s(bit, SYS_ICC_PPI_SPENDR1_EL1);
> > + } else {
> > + if (irq < 64)
> > + write_sysreg_s(bit, SYS_ICC_PPI_CPENDR0_EL1);
> > + else
> > + write_sysreg_s(bit, SYS_ICC_PPI_CPENDR1_EL1);
> > + }
> > + return;
> > + case PPI_ACTIVE:
> > + if (set) {
> > + if (irq < 64)
> > + write_sysreg_s(bit, SYS_ICC_PPI_SACTIVER0_EL1);
> > + else
> > + write_sysreg_s(bit, SYS_ICC_PPI_SACTIVER1_EL1);
> > + } else {
> > + if (irq < 64)
> > + write_sysreg_s(bit, SYS_ICC_PPI_CACTIVER0_EL1);
> > + else
> > + write_sysreg_s(bit, SYS_ICC_PPI_CACTIVER1_EL1);
> > + }
> > + return;
> > + default:
> > + BUILD_BUG_ON(1);
> > + }
> > +}
> > +
> > +static int gicv5_ppi_irq_get_irqchip_state(struct irq_data *d,
> > + enum irqchip_irq_state which,
> > + bool *val)
> > +{
> > + u64 hwirq_id_bit = BIT_ULL(d->hwirq % 64);
> > +
> > + switch (which) {
> > + case IRQCHIP_STATE_PENDING:
> > + *val = !!(read_ppi_sysreg_s(d->hwirq, PPI_PENDING) & hwirq_id_bit);
>
> The !! isn't needed AFAICS but maybe adds a small amount of documentation value if
> people don't notice that *val is a bool. I'd call it state as per the
> definition as that's kind of more obviously boolean than 'val'.
Ok for naming it 'state'.
> > + return 0;
> > + case IRQCHIP_STATE_ACTIVE:
> > + *val = !!(read_ppi_sysreg_s(d->hwirq, PPI_ACTIVE) & hwirq_id_bit);
> > + return 0;
> > + default:
> > + pr_debug("Unexpected PPI irqchip state\n");
> > + return -EINVAL;
> > + }
> > +}
>
>
> > +
> > +static void __exception_irq_entry gicv5_handle_irq(struct pt_regs *regs)
> > +{
> > + bool valid;
> > + u32 hwirq;
> > + u64 ia;
> > +
> > + ia = gicr_insn(CDIA);
> > + valid = GICV5_GIC_CDIA_VALID(ia);
> > +
> > + if (!valid)
> > + return;
> > +
> > + /*
> > + * Ensure that the CDIA instruction effects (ie IRQ activation) are
> > + * completed before handling the interrupt.
> > + */
> > + gsb_ack();
> > +
> > + /*
> > + * Ensure instruction ordering between an acknowledgment and subsequent
> > + * instructions in the IRQ handler using an ISB.
> > + */
> > + isb();
> > +
> > + hwirq = FIELD_GET(GICV5_HWIRQ_INTID, ia);
>
> As below - the GICV5_HWIRQ defines other than this one are going from
> hwirq to something the GIC cares about - this one is extracting the
> software managed hwirq from the CDIA register.
I don't get what you mean. We are extracting the HW INTID from the value
read with CDIA.
>
> > +
> > + handle_irq_per_domain(hwirq);
> > +}
> > +
> > +static void gicv5_cpu_disable_interrupts(void)
> > +{
> > + u64 cr0;
> > +
> > + cr0 = FIELD_PREP(ICC_CR0_EL1_EN, 0);
> > + write_sysreg_s(cr0, SYS_ICC_CR0_EL1);
>
> This might get more complex later, but if not why not squash
> to one line? Given the register name is right there, there
> isn't a lot of documentation benefit in having cr0 as
> the variable name.
See above. No rule on the matter - it is subjective so I am
reluctant to change it.
> > +}
> > +
> > +static void gicv5_cpu_enable_interrupts(void)
> > +{
> > + u64 cr0, pcr;
> > +
> > + write_sysreg_s(0, SYS_ICC_PPI_ENABLER0_EL1);
> > + write_sysreg_s(0, SYS_ICC_PPI_ENABLER1_EL1);
> > +
> > + gicv5_ppi_priority_init();
> > +
> > + pcr = FIELD_PREP(ICC_PCR_EL1_PRIORITY, GICV5_IRQ_PRI_MI);
> > + write_sysreg_s(pcr, SYS_ICC_PCR_EL1);
> > +
> > + cr0 = FIELD_PREP(ICC_CR0_EL1_EN, 1);
> > + write_sysreg_s(cr0, SYS_ICC_CR0_EL1);
>
> Similar to above, I'd squash into single line.
Ditto.
> > +}
> > +
> > +static int gicv5_starting_cpu(unsigned int cpu)
> > +{
> > + if (WARN(!gicv5_cpuif_has_gcie(),
> > + "GICv5 system components present but CPU does not have FEAT_GCIE"))
>
> Alignment off to my eyes. Either a tab or align with !
Fixed it.
> > + return -ENODEV;
> > +
> > + gicv5_cpu_enable_interrupts();
> > +
> > + return 0;
> > +}
> > +
>
> > diff --git a/include/linux/irqchip/arm-gic-v5.h b/include/linux/irqchip/arm-gic-v5.h
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..4ff0ba64d9840c3844671f7850bb3d81ba2eb1b6
> > --- /dev/null
> > +++ b/include/linux/irqchip/arm-gic-v5.h
> > @@ -0,0 +1,16 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Copyright (C) 2025 ARM Limited, All Rights Reserved.
> > + */
> > +#ifndef __LINUX_IRQCHIP_ARM_GIC_V5_H
> > +#define __LINUX_IRQCHIP_ARM_GIC_V5_H
> > +
> > +#include <asm/sysreg.h>
> > +
> > +#define GICV5_HWIRQ_ID GENMASK(23, 0)
> > +#define GICV5_HWIRQ_TYPE GENMASK(31, 29)
> > +#define GICV5_HWIRQ_INTID GENMASK_ULL(31, 0)
>
> Maybe some hint as to what these are in from their naming?
>
> First two are from hwirq as defined in the irq domain stuff.
> Not the 3rd one if I follow this right.
The three of them are there to handle the HW GICv5 INTID and its related
fields (Rule R_TJPHS), maybe I should add a comment highlighting this ?
Thanks,
Lorenzo
> > +
> > +#define GICV5_HWIRQ_TYPE_PPI UL(0x1)
> > +
> > +#endif
> >
>
Powered by blists - more mailing lists