On Fri, 11 Jul 2025 10:55:17 +0100,
Lorenzo Pieralisi [off-list ref] wrote:

On Tue, Jul 08, 2025 at 06:34:00PM +0100, Marc Zyngier wrote:

quoted

 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.

[...]

quoted

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