Re: [PATCH v2 09/13] PCI: xgene-msi: Sanitise MSI allocation and affinity setting

[Marc Zyngier <maz@kernel.org>]

On Fri, 11 Jul 2025 10:55:17 +0100,
Lorenzo Pieralisi <lpieralisi@kernel.org> wrote:
> 
> On Tue, Jul 08, 2025 at 06:34:00PM +0100, Marc Zyngier wrote:
> >  static void xgene_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
> >  {
> >  	struct xgene_msi *msi = irq_data_get_irq_chip_data(data);
> > -	u32 reg_set = hwirq_to_reg_set(data->hwirq);
> > -	u32 group = hwirq_to_group(data->hwirq);
> > -	u64 target_addr = msi->msi_addr + (((8 * group) + reg_set) << 16);
> > +	u64 target_addr;
> > +	u32 frame, msir;
> > +	int cpu;
> >  
> > -	msg->address_hi = upper_32_bits(target_addr);
> > -	msg->address_lo = lower_32_bits(target_addr);
> > -	msg->data = hwirq_to_msi_data(data->hwirq);
> > -}
> > +	cpu	= cpumask_first(irq_data_get_effective_affinity_mask(data));
> > +	msir	= FIELD_GET(GENMASK(6, 4), data->hwirq);
> 
> We could use MSInRx_HWIRQ_MASK, I can update it.

Yes, I appear to have missed that one. It'd be good if you could fix
it up.

> More importantly, what code would set data->hwirq[6:4] (and
> data->hwirq[7:7] below) ?

That's obviously driven by the hwirq allocation, which is guaranteed
to happen in the 0:255 range.

[...]

> > @@ -173,23 +167,20 @@ static int xgene_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
> >  				  unsigned int nr_irqs, void *args)
> >  {
> >  	struct xgene_msi *msi = domain->host_data;
> > -	int msi_irq;
> > +	irq_hw_number_t hwirq;
> >  
> >  	mutex_lock(&msi->bitmap_lock);
> >  
> > -	msi_irq = bitmap_find_next_zero_area(msi->bitmap, NR_MSI_VEC, 0,
> > -					     num_possible_cpus(), 0);
> > -	if (msi_irq < NR_MSI_VEC)
> > -		bitmap_set(msi->bitmap, msi_irq, num_possible_cpus());
> > -	else
> > -		msi_irq = -ENOSPC;
> > +	hwirq = find_first_zero_bit(msi->bitmap, NR_MSI_VEC);
> > +	if (hwirq < NR_MSI_VEC)
> > +		set_bit(hwirq, msi->bitmap);
> >  
> >  	mutex_unlock(&msi->bitmap_lock);
> >  
> > -	if (msi_irq < 0)
> > -		return msi_irq;
> > +	if (hwirq >= NR_MSI_VEC)
> > +		return -ENOSPC;
> >  
> > -	irq_domain_set_info(domain, virq, msi_irq,
> > +	irq_domain_set_info(domain, virq, hwirq,
> >  			    &xgene_msi_bottom_irq_chip, domain->host_data,
> >  			    handle_simple_irq, NULL, NULL);
> 
> This is something I don't get. We alloc an MSI, set a bit in the bitmap
> and the hwirq to that value, when we handle the IRQ below in
> 
> xgene_msi_isr()
> 
> hwirq = compute_hwirq(msi_grp, msir_idx, intr_idx);
> ret = generic_handle_domain_irq(xgene_msi->inner_domain, hwirq);
> 
> imagining that we changed the affinity for the IRQ so that the computed
> HWIRQ does not have zeros in bits[7:4], how would the domain HWIRQ
> matching work ?

No. The whole point of this series is that hwirq is now *constant*, no
matter what the affinity says.

> Actually, how would an IRQ fire causing the hwirq[7:4] bits to be != 0
> in the first place ?

hwirq[7:4] is a function of what IRQ fired (giving you a register
frame), and what register was accessed. That 's what allows you to
*rebuild* hwirq as the HW doesn't give it you on a plate (only the
bottom 4 bits are more or less given as a bitmap).

> Forgive me if I am missing something obvious, the *current* MSI handling
> is very hard to grok, it is certain I misunderstood it entirely.

Ignore the current code, it does things that are completely forbidden
by law in any civilised country (although I find it difficult to name
a single civilised country these days).

Thanks,

	M.

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