On Fri, 11 Jul 2025 10:55:17 +0100,
Lorenzo Pieralisi <lpieralisi@xxxxxxxxxx> 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.
