| 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: <aBsaBDB17LAV48ZB@lpieralisi>
Date: Wed, 7 May 2025 10:29:56 +0200
From: Lorenzo Pieralisi <lpieralisi@...nel.org>
To: Thomas Gleixner <tglx@...utronix.de>
Cc: Marc Zyngier <maz@...nel.org>, 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 v3 20/25] irqchip/gic-v5: Add GICv5 PPI support
On Tue, May 06, 2025 at 05:00:31PM +0200, Thomas Gleixner wrote:
> On Tue, May 06 2025 at 14:23, Lorenzo Pieralisi wrote:
> > +
> > +static u8 pri_bits = 5;
>
> __ro_after_init ?
Ok.
>
> > +#define GICV5_IRQ_PRI_MASK 0x1f
>
> Please put a new line before the #define and use a TAB between the
> symbol and the value.
Ok, sorry.
> > +#define GICV5_IRQ_PRI_MI \
> > + (GICV5_IRQ_PRI_MASK & GENMASK(4, 5 - pri_bits))
>
> No line break required. You have 100 characters
Right.
> > +#define READ_PPI_REG(irq, reg) \
> > + ({ \
> > + u64 __ppi_val; \
> > + \
> > + if (irq < 64) \
> > + __ppi_val = read_sysreg_s(SYS_ICC_PPI_##reg##R0_EL1); \
> > + else \
> > + __ppi_val = read_sysreg_s(SYS_ICC_PPI_##reg##R1_EL1); \
> > + __ppi_val; \
> > + })
> > +
> > +#define WRITE_PPI_REG(set, irq, bit, reg) \
> > + do { \
> > + if (set) { \
> > + if (irq < 64) \
> > + write_sysreg_s(bit, SYS_ICC_PPI_S##reg##R0_EL1);\
> > + else \
> > + write_sysreg_s(bit, SYS_ICC_PPI_S##reg##R1_EL1);\
> > + } else { \
> > + if (irq < 64) \
> > + write_sysreg_s(bit, SYS_ICC_PPI_C##reg##R0_EL1);\
> > + else \
> > + write_sysreg_s(bit, SYS_ICC_PPI_C##reg##R1_EL1);\
> > + } \
> > + } while (0)
>
> I'm not convinced that these need to be macros.
>
> static __always_inline u64 read_ppi_sysreg_s(unsigned int irq, const unsigned int which)
> {
> switch (which) {
> case PPI_HM:
> return irq < 64 ? read_sysreg_s(SYS_ICC_PPI_HM_R0_EL1) :
> read_sysreg_s(SYS_ICC_PPI_HM_R1_EL1;
> case ....:
>
> default:
> BUILD_BUG_ON(1);
> }
> }
>
> static __always_inline void write_ppi_sysreg_s(unsigned int irq, bool set, const unsigned int which)
> {
> u64 bit = BIT_ULL(irq % 64);
>
> switch (which) {
> case PPI_HM:
> if (irq < 64)
> write_sysreg_s(bit, SYS_ICC_PPI_HM_R0_EL1);
> else
> write_sysreg_s(bit, SYS_ICC_PPI_HM_R1_EL1;
> return;
> case ....:
>
> default:
> BUILD_BUG_ON(1);
> }
> }
>
> Or something like that.
Done.
> > +static int gicv5_ppi_set_type(struct irq_data *d, unsigned int type)
> > +{
> > + /*
> > + * The PPI trigger mode is not configurable at runtime,
> > + * therefore this function simply confirms that the `type`
> > + * parameter matches what is present.
> > + */
> > + u64 hmr = READ_PPI_REG(d->hwirq, HM);
> > +
> > + switch (type) {
> > + case IRQ_TYPE_LEVEL_HIGH:
> > + case IRQ_TYPE_LEVEL_LOW:
> > + if (((hmr >> (d->hwirq % 64)) & 0x1) != GICV5_PPI_HM_LEVEL)
> > + return -EINVAL;
>
> Blink!
>
> How does this test distinguish between LEVEL_LOW and LEVEL_HIGH? It only
> tests for level, no? So the test is interesting at best ...
There is no HIGH/LOW concept for level interrupts in the architecture,
level interrupts are asserted/de-asserted. On top of that, as you
already noticed, for PPIs this can't even be changed so this function
is utterly pointless.
> Secondly this comparison is confusing at best especially given that you
> mask with a hex constant (0x1) first.
>
> if (hmr & BIT_UL(d->hwirq % 64))
> return -EINVAL;
>
> Aside of that why do you have a set_type() function if there is no way
> to set the type?
Yes, that's useless, the kernel is not there to validate firmware, I
will remove it.
> > +
> > +static int gicv5_ppi_irq_get_irqchip_state(struct irq_data *d,
> > + enum irqchip_irq_state which,
> > + bool *val)
> > +{
> > + u64 pendr, activer, hwirq_id_bit = BIT_ULL(d->hwirq % 64);
> > +
> > + switch (which) {
> > + case IRQCHIP_STATE_PENDING:
> > + pendr = READ_PPI_REG(d->hwirq, SPEND);
> > +
> > + *val = !!(pendr & hwirq_id_bit);
> > +
> > + return 0;
>
> *val = !!(read_ppi_reg(d->hwirq, PPI_SPEND) & bit);
> return 0;
>
> would take up less space and be readable.
Ok done.
> > + case IRQCHIP_STATE_ACTIVE:
> > + activer = READ_PPI_REG(d->hwirq, SACTIVE);
> > +
> > + *val = !!(activer & hwirq_id_bit);
> > +
> > + return 0;
> > + default:
> > + pr_debug("Unexpected PPI irqchip state\n");
> > + }
> > +
> > + return -EINVAL;
>
> Move the return into the default case.
Ok.
> > +static int __init gicv5_init_domains(struct fwnode_handle *handle)
> > +{
> > + struct irq_domain *d;
> > +
> > + d = irq_domain_create_linear(handle, PPI_NR, &gicv5_irq_ppi_domain_ops,
> > + NULL);
>
> Please use the full 100 charactes all over the place.
Ok.
> > + if (!d)
> > + return -ENOMEM;
> > +
> > + irq_domain_update_bus_token(d, DOMAIN_BUS_WIRED);
> > + gicv5_global_data.ppi_domain = d;
> > +
> > + gicv5_global_data.fwnode = handle;
>
> The random choices of seperating code with new lines are really
> amazing.
I separated code that initializes the domain from one that initialises
fwnode - it made sense to *me*, I don't know what makes sense to others
unless there are rules (or a script/bot reformatting the code for a
given subsytem) that one can follow I am afraid.
> > +static int __init gicv5_of_init(struct device_node *node, struct device_node *parent)
> > +{
> > + int ret;
> > +
> > + ret = gicv5_init_domains(&node->fwnode);
>
> int ret = ....;
Ok.
> > + if (ret)
>
> Thanks,
>
> tglx
Thanks for reviewing.
Lorenzo
Powered by blists - more mailing lists