| 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: <aGUycEuLadcG+IfV@lpieralisi>
Date: Wed, 2 Jul 2025 15:21:52 +0200
From: Lorenzo Pieralisi <lpieralisi@...nel.org>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>
Cc: Thomas Gleixner <tglx@...utronix.de>,
Catalin Marinas <catalin.marinas@....com>,
Marc Zyngier <maz@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, Will Deacon <will@...nel.org>,
Arnd Bergmann <arnd@...db.de>,
Sascha Bischoff <sascha.bischoff@....com>,
Timothy Hayes <timothy.hayes@....com>,
Bjorn Helgaas <bhelgaas@...gle.com>,
"Liam R. Howlett" <Liam.Howlett@...cle.com>,
Peter Maydell <peter.maydell@...aro.org>,
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, linux-pci@...r.kernel.org
Subject: Re: [PATCH v6 20/31] irqchip/gic-v5: Add GICv5 PPI support
On Wed, Jul 02, 2025 at 02:00:22PM +0100, Jonathan Cameron wrote:
> On Wed, 2 Jul 2025 14:46:10 +0200
> Lorenzo Pieralisi <lpieralisi@...nel.org> wrote:
>
> > On Wed, Jul 02, 2025 at 12:40:19PM +0100, Jonathan Cameron wrote:
> > > On Thu, 26 Jun 2025 12:26:11 +0200
> > > Lorenzo Pieralisi <lpieralisi@...nel.org> wrote:
> > >
> > > > The GICv5 CPU interface implements support for PE-Private Peripheral
> > > > Interrupts (PPI), that are handled (enabled/prioritized/delivered)
> > > > entirely within the CPU interface hardware.
> > >
> > > I can't remember where I got to last time so if I repeat stuff that
> > > you already responded to, feel free to just ignore me this time ;)
> > >
> > > All superficial stuff. Feel free to completely ignore if you like.
> >
> > We are at v6.16-rc4, series has been on the lists for 3 months, it has
> > been reviewed and we would like to get it into v6.17 if possible and
> > deemed reasonable, I am asking you folks please, what should I do ?
> >
> > I can send a v7 with the changes requested below (no bug fixes there)
> > - it is fine by me - but I need to know please asap if we have a
> > plan to get this upstream this cycle.
>
> I'm absolutely fine with leaving these be. The mask stuff I would like
> to clean up as it applies quite widely in the series but that
> can be a follow up as no bugs (so far!).
I am certain that at a given state in the development I used the
FIELD_PREP() on the hwirq_id and then was asked to remove it because
it does not serve any purpose - this, for the records.
Thanks,
Lorenzo
> As Marc said, these are in a good state.
>
> Jonathan
>
> >
> > Thanks,
> > Lorenzo
> >
> > > > diff --git a/drivers/irqchip/irq-gic-v5.c b/drivers/irqchip/irq-gic-v5.c
> > > > new file mode 100644
> > > > index 000000000000..a08daa562d21
> > > > --- /dev/null
> > > > +++ b/drivers/irqchip/irq-gic-v5.c
> > > > @@ -0,0 +1,461 @@
> > > > +// 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))
> > > > +
> > > > +#define PPI_NR 128
> > > > +
> > > > +static bool gicv5_cpuif_has_gcie(void)
> > > > +{
> > > > + return this_cpu_has_cap(ARM64_HAS_GICV5_CPUIF);
> > > > +}
> > > > +
> > > > +struct gicv5_chip_data {
> > > > + struct fwnode_handle *fwnode;
> > > > + struct irq_domain *ppi_domain;
> > > > +};
> > > > +
> > > > +static struct gicv5_chip_data gicv5_global_data __read_mostly;
> > >
> > > > +static void gicv5_hwirq_eoi(u32 hwirq_id, u8 hwirq_type)
> > > > +{
> > > > + u64 cddi = hwirq_id | FIELD_PREP(GICV5_GIC_CDDI_TYPE_MASK, hwirq_type);
> > >
> > > Slight preference for not needing to care where hwirq_id goes in CDDI or how big
> > > it is (other than when I checked the header defines).
> > >
> > > u64 cddi = FIELD_PREP(GICV5_GIC_CDDI_ID_MASK, hwirq_id) |
> > > FIELD_PREP(GICV5_GIC_CDDI_TYPE_MASK, hwirq_type);
> > >
> > >
> > > > +
> > > > + gic_insn(cddi, CDDI);
> > > > +
> > > > + gic_insn(0, CDEOI);
> > > > +}
> > >
> > > > +static int gicv5_ppi_irq_get_irqchip_state(struct irq_data *d,
> > > > + enum irqchip_irq_state which,
> > > > + bool *state)
> > > > +{
> > > > + u64 hwirq_id_bit = BIT_ULL(d->hwirq % 64);
> > > > +
> > > > + switch (which) {
> > > > + case IRQCHIP_STATE_PENDING:
> > > > + *state = !!(read_ppi_sysreg_s(d->hwirq, PPI_PENDING) & hwirq_id_bit);
> > >
> > > Technically don't need the !! but if you really like it I don't mind that much.
> > >
> > > > + return 0;
> > > > + case IRQCHIP_STATE_ACTIVE:
> > > > + *state = !!(read_ppi_sysreg_s(d->hwirq, PPI_ACTIVE) & hwirq_id_bit);
> > > > + return 0;
> > > > + default:
> > > > + pr_debug("Unexpected PPI irqchip state\n");
> > > > + return -EINVAL;
> > > > + }
> > > > +}
> > >
> > >
> > > > +static int gicv5_irq_ppi_domain_translate(struct irq_domain *d,
> > > > + struct irq_fwspec *fwspec,
> > > > + irq_hw_number_t *hwirq,
> > > > + unsigned int *type)
> > > > +{
> > > > + if (!is_of_node(fwspec->fwnode))
> > > > + return -EINVAL;
> > > > +
> > > > + if (fwspec->param_count < 3)
> > >
> > > I don't care that much, but could relax this seeing as fwspec->param[2]
> > > isn't used anyway? Maybe a tiny comment on why it matters?
> > >
> > > > + return -EINVAL;
> > > > +
> > > > + if (fwspec->param[0] != GICV5_HWIRQ_TYPE_PPI)
> > > > + return -EINVAL;
> > > > +
> > > > + *hwirq = fwspec->param[1];
> > > > +
> > > > + /*
> > > > + * Handling mode is hardcoded for PPIs, set the type using
> > > > + * HW reported value.
> > > > + */
> > > > + *type = gicv5_ppi_irq_is_level(*hwirq) ? IRQ_TYPE_LEVEL_LOW : IRQ_TYPE_EDGE_RISING;
> > > > +
> > > > + return 0;
> > >
> > >
> > > > +static int __init gicv5_of_init(struct device_node *node, struct device_node *parent)
> > > > +{
> > > > + int ret = gicv5_init_domains(of_fwnode_handle(node));
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + gicv5_set_cpuif_pribits();
> > > > +
> > > > + ret = gicv5_starting_cpu(smp_processor_id());
> > > > + if (ret)
> > > > + goto out_dom;
> > > > +
> > > > + ret = set_handle_irq(gicv5_handle_irq);
> > > > + if (ret)
> > > > + goto out_int;
> > > > +
> > > > + return 0;
> > > > +
> > > > +out_int:
> > > > + gicv5_cpu_disable_interrupts();
> > > > +out_dom:
> > > > + gicv5_free_domains();
> > >
> > > Naming is always tricky but I'd not really expect gicv5_free_domains() as the
> > > pair of gicv5_init_domains() (which is doing creation rather than just initializing).
> > >
> > > Ah well, names are never prefect and I don't really mind.
> > >
> > > > +
> > > > + return ret;
> > > > +}
> > > > +IRQCHIP_DECLARE(gic_v5, "arm,gic-v5", gicv5_of_init);
> > >
> >
>
Powered by blists - more mailing lists