Re: [PATCH v2 20/22] irqchip/gic-v5: Add GICv5 ITS support

[Marc Zyngier]

On Wed, 30 Apr 2025 14:21:00 +0100,
Lorenzo Pieralisi <lpieralisi@xxxxxxxxxx> wrote:
> 
> On Wed, Apr 30, 2025 at 10:12:58AM +0100, Marc Zyngier wrote:
> > On Thu, 24 Apr 2025 11:25:31 +0100,
> > Lorenzo Pieralisi <lpieralisi@xxxxxxxxxx> wrote:

[...]

> > > 
> > > +void gicv5_irs_syncr(void)
> > > +{
> > > +	struct gicv5_irs_chip_data *irs_data;
> > > +	u32 syncr;
> > > +
> > > +	irs_data = list_first_entry_or_null(&irs_nodes,
> > > +					    struct gicv5_irs_chip_data, entry);
> > > +	if (WARN_ON(!irs_data))
> > > +		return;
> > > +
> > > +	syncr = FIELD_PREP(GICV5_IRS_SYNCR_SYNC, 1);
> > > +	irs_writel_relaxed(irs_data, syncr, GICV5_IRS_SYNCR);
> > > +
> > > +	gicv5_irs_wait_for_op(irs_data->irs_base, GICV5_IRS_SYNC_STATUSR,
> > > +			      GICV5_IRS_SYNC_STATUSR_IDLE);
> > > +}
> > > +
> > 
> > Only the ITS code is using this function. Why isn't it in the ITS code
> > as a static helper?
> 
> I'd need to make irs_nodes global.

You could simply have a helper returning the first IRS node. Not a big
deal anyway.

[...]

> > > +	/*
> > > +	 * Need to determine how many entries there are per L2 - this is based
> > > +	 * on the number of bits in the table.
> > > +	 */
> > > +	events_per_l2_table = BIT(l2_bits);
> > > +	complete_tables = num_events / events_per_l2_table;
> > > +	final_span = order_base_2(num_events % events_per_l2_table);
> > > +
> > > +	for (i = 0; i < num_ents; i++) {
> > > +		size_t l2sz;
> > > +
> > > +		span = i == complete_tables ? final_span : l2_bits;
> > > +
> > > +		itt_l2 = kcalloc(BIT(span), sizeof(*itt_l2), GFP_KERNEL);
> > > +		if (!itt_l2) {
> > > +			ret = -ENOMEM;
> > > +			goto out_free;
> > > +		}
> > 
> > You are allocating a bunch of 64bit pointers. So the alignment is
> > BIT(span + 3) or ARCH_KMALLOC_MINALIGN, whichever is the largest.
> 
> Right, at least 8 bytes.
> 
> > > +
> > > +		its_dev->itt_cfg.l2.l2ptrs[i] = itt_l2;
> > > +
> > > +		l2sz = BIT(span) * sizeof(*itt_l2);
> > > +
> > > +		if (its->flags & ITS_FLAGS_NON_COHERENT)
> > > +			dcache_clean_inval_poc((unsigned long)itt_l2,
> > > +					       (unsigned long)itt_l2 + l2sz);
> > > +
> > > +		val = (virt_to_phys(itt_l2) & GICV5_ITTL1E_L2_ADDR_MASK) |
> > > +		       FIELD_PREP(GICV5_ITTL1E_SPAN, span)		 |
> > > +		       FIELD_PREP(GICV5_ITTL1E_VALID, 0x1);
> > 
> > GICV5_ITTL1E_L2_ADDR_MASK starts at bit 12.
> 
> No, it starts at bit 3.

Ah, you're absolutely right. I looked at the IST version...

[...]

> > > +{
> > > +	struct gicv5_its_dev *its_dev;
> > > +	int ret;
> > > +
> > > +	its_dev = gicv5_its_find_device(its, dev_id);
> > > +	if (!IS_ERR(its_dev)) {
> > > +		pr_debug("A device with this DeviceID (0x%x) has already been registered.\n",
> > > +			 dev_id);
> > > +
> > > +		if (nvec > its_dev->num_events) {
> > > +			pr_debug("Requesting more ITT entries than allocated\n");
> > > +			return ERR_PTR(-ENXIO);
> > > +		}
> > > +
> > > +		its_dev->shared = true;
> > > +
> > > +		return its_dev;
> > 
> > I really think we shouldn't even consider the silliness of
> > non-transparent bridges this time around. That's a terrible system
> > design, and it leads to all sorts of lifetime madness -- the GICv3
> > driver is a testament to it. Modern systems with GICv5 should not have
> > to deal with this nonsense.
> 
> I am not sure we can remove this path for the IWB - even if we model it
> as an MBIgen.

Why? The IWB is (or rather should be) seen as a device. The fact that
it is itself an interrupt controller is am independent issue.

> With Sascha and Tim we tested this code path, I am not sure it would
> work if a driver with a wired IRQ connected to an IWB free an IRQ and
> the ITS device representing the IWB is not shared.

I don't think freeing the IRQ from the end-point perspective should
have any effect on the IWB. At probe time, the IWB should grab all the
LPIs it needs, publish them as part of the wired domain attached to
its fwnode, and be done with it

[...]

> > > +static int gicv5_its_irq_domain_activate(struct irq_domain *domain,
> > > +					 struct irq_data *d, bool reserve)
> > > +{
> > > +	struct gicv5_its_dev *its_dev = irq_data_get_irq_chip_data(d);
> > > +	u16 event_id;
> > > +	u32 lpi;
> > > +
> > > +	event_id = FIELD_GET(GICV5_ITS_HWIRQ_EVENT_ID, d->hwirq);
> > > +	lpi = d->parent_data->hwirq;
> > > +
> > > +	return gicv5_its_alloc_event(its_dev, event_id, lpi);
> > 
> > Huh. This looks wrong. Allocating the event really should happen at
> > alloc time, not at activate time, because the endpoint driver doesn't
> > really expect this to fail for any reason other than a gross bug.
> > 
> > activate should allow the translation to take place, but not rely on
> > allocating events. Compare with GICv3, which only issues the MAPTI
> > command at activate time.
> 
> I am not "allocating an event" (well, then you would say "learn how to
> name your functions" and you are right), I am writing the ITT table for
> an eventid that was preallocated before, so basically, apart from
> paranoia checks, this is the MAPTI equivalent.

Feels like *a lot* of paranoia checks, most of which should not be
possible by construction. You can also get rid of num_mapped_events,
which is clearly some debug stuff.

And yes, this function can do with a bit of renaming.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.