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

[Lorenzo Pieralisi <lpieralisi@kernel.org>]

On Wed, Apr 09, 2025 at 10:27:24AM +0200, Thomas Gleixner wrote:
> 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.

Yep, you have a point, will do my best to consolidate them.

> > +
> > +	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?

Yes, I wrote it in the cover letter (again, easy to miss), I have
to admit I am not convinced either. We need 24 bits max, so a 2MB
chunk of memory worst case but other than that I can't really argue
to be honest.

I also thought about re-using the GICv3 ITS LPI allocator (which
does the same thing - agree, it is an on-purpose one, so maybe
it is better to reuse a core code construct).

Marc obviously has more background knowledge on this - I am
open to reworking this LPI allocator and remove the MT.

> > +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.

Point taken.

> > +
> > +		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.

I will update it.

Thanks Thomas,
Lorenzo