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

[Arnd Bergmann]

On Tue, Apr 8, 2025, at 12:50, Lorenzo Pieralisi wrote:
> @@ -22,12 +25,346 @@ static u32 irs_readl(struct gicv5_irs_chip_data 
> *irs_data, const u64 reg_offset)
>  	return readl_relaxed(irs_data->irs_base + reg_offset);
>  }
> 
> +static u64 irs_readq(struct gicv5_irs_chip_data *irs_data, const u64 
> reg_offset)
> +{
> +	return readq_relaxed(irs_data->irs_base + reg_offset);
> +}
> +
>  static void irs_writel(struct gicv5_irs_chip_data *irs_data, const u32 
> val,
>  		       const u64 reg_offset)
>  {
>  	writel_relaxed(val, irs_data->irs_base + reg_offset);
>  }
> 
> +static void irs_writeq(struct gicv5_irs_chip_data *irs_data, const u64 
> val,
> +		       const u64 reg_offset)
> +{
> +	writeq_relaxed(val, irs_data->irs_base + reg_offset);
> +}

I think the use of _relaxed memory accessors needs some code
comments here. The definition of these is that you don't care
about ordering relative to DMA master accesses, yet you seem to
very much have accesses to the 'ist' from the GIC, as well as
DMA accesses from an MSI device, and I would expect both to
require ordering.

> +/* 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);
> +

What is the significance of the 1 second timeout? This is probably
a million times longer than I would expect any hardware interaction
to be specified to take. Are you waiting for another thread here?

> +	l2istsz = BIT(n + 1);
> +	if (l2istsz > KMALLOC_MAX_SIZE) {
> +		u8 lpi_id_cap = ilog2(KMALLOC_MAX_SIZE) - 2 + istsz;
> +
> +		pr_warn("Limiting LPI ID bits from %u to %u\n",
> +			lpi_id_bits, lpi_id_cap);
> +		lpi_id_bits = lpi_id_cap;
> +		l2istsz = KMALLOC_MAX_SIZE;
> +	}

The use of KMALLOC_MAX_SIZE seem arbitrary here. I remember discussing
this in the past and concluding that this is fine for all cases
that may be relevant, but it would be good to explain the reasoning
in a comment.

> +	if (irs_data->flags & IRS_FLAGS_NON_COHERENT)
> +		dcache_clean_inval_poc((unsigned long)ist,
> +				       (unsigned long)ist + l2istsz);
> +	else
> +		dsb(ishst);
...
> +	baser = (virt_to_phys(ist) & GICV5_IRS_IST_BASER_ADDR_MASK) |
> +		FIELD_PREP(GICV5_IRS_IST_BASER_VALID, 0x1);

Here it seems like you are open-coding the DMA mapping interface
details, in particular the mapping of the 'ist' memory area into
the gic's DMA master space, the coherency and the barrier that is
normally part of a (non-relaxed) writeq().  Is there a reason
you can't use the normal interfaces here, using dma_alloc_coherent()
or dma_alloc_noncoherent()?

Do you expect actual implementation to not be cache-coherent?

        Arnd