linux-kernel - Re: [PATCH 20/24] irqchip/gic-v5: Add GICv5 LPI/IPI support

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87wmbtu4cz.ffs@tglx>
Date: Wed, 09 Apr 2025 10:27:24 +0200
From: Thomas Gleixner <tglx@...utronix.de>
To: Lorenzo Pieralisi <lpieralisi@...nel.org>, 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>
Cc: Sascha Bischoff <sascha.bischoff@....com>, Timothy Hayes
 <timothy.hayes@....com>, linux-arm-kernel@...ts.infradead.org,
 linux-kernel@...r.kernel.org, devicetree@...r.kernel.org, Lorenzo
 Pieralisi <lpieralisi@...nel.org>
Subject: Re: [PATCH 20/24] irqchip/gic-v5: Add GICv5 LPI/IPI support

On Tue, Apr 08 2025 at 12:50, Lorenzo Pieralisi wrote:
> +/* Wait for completion of an IST change */
> +static int gicv5_irs_ist_wait_for_idle(struct gicv5_irs_chip_data *irs_data)
> +{
> +	int ret;
> +	u32 val;
> +
> +	ret = readl_relaxed_poll_timeout_atomic(
> +			irs_data->irs_base + GICV5_IRS_IST_STATUSR, val,
> +			FIELD_GET(GICV5_IRS_IST_STATUSR_IDLE, val), 1,
> +			USEC_PER_SEC);
> +
> +	if (ret == -ETIMEDOUT)
> +		pr_err_ratelimited("IST_STATUSR.IDLE timeout...\n");
> +
> +	return ret;

I'm sure I've seen that code before and without looking I'm sure the
diff between the two functions is ~2 lines.

> +
> +	mtree_lock(&lpi_mt);
> +	ret = mas_empty_area(&mas, 0, num_lpis - 1, lpis);
> +	if (ret) {
> +		pr_err("Failed to perform a dynamic alloc in the LPI MT!\n");
> +		return ret;
> +	}
> +
> +	lpi_base = mas.index;
> +
> +	/*
> +	 * We don't really care about the entry itself, only about
> +	 * allocation of a maple tree ranges describing in use LPIs.
> +	 * That's why, upon allocation, we try to merge slots adjacent
> +	 * with the empty one we are allocating to minimize the number
> +	 * of slots we take from maple tree nodes for nothing, all
> +	 * we need to keep track of is in use ranges.
> +	 */

I'm really not convinced that you need a maple tree and the code
complexity for this. What's wrong with a simple bitmap other than that
it might need 1MB memory?

> +static int gicv5_irq_lpi_domain_alloc(struct irq_domain *domain,
> +				      unsigned int virq, unsigned int nr_irqs,
> +				      void *arg)
> +{
> +	irq_hw_number_t hwirq;
> +	struct irq_data *irqd;
> +	u32 *base_lpi = arg;
> +	int i, ret;
> +
> +	hwirq = *base_lpi;
> +
> +	for (i = 0; i < nr_irqs; i++) {
> +		irqd = irq_desc_get_irq_data(irq_to_desc(virq + i));

irq_get_irq_data() and irq_domain_get_irq_data() exist for a reason.

> +
> +		irq_domain_set_info(domain, virq + i, hwirq + i,
> +				    &gicv5_lpi_irq_chip, NULL,
> +				    handle_fasteoi_irq, NULL, NULL);
> +		irqd_set_single_target(irqd);
> +static int gicv5_irq_ipi_domain_alloc(struct irq_domain *domain,
> +				      unsigned int virq, unsigned int nr_irqs,
> +				      void *arg)
> +{
> +	int ret, i;
> +	u32 lpi;
> +	struct irq_data *irqd = irq_desc_get_irq_data(irq_to_desc(virq));

Again. Zero reason to fiddle with irq_desc.

Thanks,

        tglx