RE: [PATCH] genirq/msi: Activate Multi-MSI early when MSI_FLAG_ACTIVATE_EARLY is set

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Marc Zyngier [mailto:maz@xxxxxxxxxx]
> Sent: 25 January 2021 14:49
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@xxxxxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx; Thomas Gleixner <tglx@xxxxxxxxxxxxx>; Bjorn
> Helgaas <bhelgaas@xxxxxxxxxx>; stable@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH] genirq/msi: Activate Multi-MSI early when
> MSI_FLAG_ACTIVATE_EARLY is set
> 
> On 2021-01-25 14:39, Shameerali Kolothum Thodi wrote:
> >> -----Original Message-----
> >> From: Marc Zyngier [mailto:maz@xxxxxxxxxx]
> >> Sent: 23 January 2021 12:28
> >> To: linux-kernel@xxxxxxxxxxxxxxx
> >> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>; Bjorn Helgaas
> >> <bhelgaas@xxxxxxxxxx>; Shameerali Kolothum Thodi
> >> <shameerali.kolothum.thodi@xxxxxxxxxx>; stable@xxxxxxxxxxxxxxx
> >> Subject: [PATCH] genirq/msi: Activate Multi-MSI early when
> >> MSI_FLAG_ACTIVATE_EARLY is set
> >>
> >> When MSI_FLAG_ACTIVATE_EARLY is set (which is the case for PCI),
> >> we perform the activation of the interrupt (which in the case of
> >> PCI results in the endpoint being programmed) as soon as the
> >> interrupt is allocated.
> >>
> >> But it appears that this is only done for the first vector,
> >> introducing an inconsistent behaviour for PCI Multi-MSI.
> >>
> >> Fix it by iterating over the number of vectors allocated to
> >> each MSI descriptor. This is easily achieved by introducing
> >> a new "for_each_msi_vector" iterator, together with a tiny
> >> bit of refactoring.
> >>
> >> Fixes: f3b0946d629c ("genirq/msi: Make sure PCI MSIs are activated
> >> early")
> >> Reported-by: Shameer Kolothum
> <shameerali.kolothum.thodi@xxxxxxxxxx>
> >> Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx>
> >> Cc: stable@xxxxxxxxxxxxxxx
> >> ---
> >>  include/linux/msi.h |  6 ++++++
> >>  kernel/irq/msi.c    | 44 ++++++++++++++++++++------------------------
> >>  2 files changed, 26 insertions(+), 24 deletions(-)
> >>
> >> diff --git a/include/linux/msi.h b/include/linux/msi.h
> >> index 360a0a7e7341..aef35fd1cf11 100644
> >> --- a/include/linux/msi.h
> >> +++ b/include/linux/msi.h
> >> @@ -178,6 +178,12 @@ struct msi_desc {
> >>  	list_for_each_entry((desc), dev_to_msi_list((dev)), list)
> >>  #define for_each_msi_entry_safe(desc, tmp, dev)	\
> >>  	list_for_each_entry_safe((desc), (tmp), dev_to_msi_list((dev)),
> >> list)
> >> +#define for_each_msi_vector(desc, __irq, dev)				\
> >> +	for_each_msi_entry((desc), (dev))				\
> >> +		if ((desc)->irq)					\
> >> +			for (__irq = (desc)->irq;			\
> >> +			     __irq < ((desc)->irq + (desc)->nvec_used);	\
> >> +			     __irq++)
> >>
> >>  #ifdef CONFIG_IRQ_MSI_IOMMU
> >>  static inline const void *msi_desc_get_iommu_cookie(struct msi_desc
> >> *desc)
> >> diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
> >> index 2c0c4d6d0f83..d924676c8781 100644
> >> --- a/kernel/irq/msi.c
> >> +++ b/kernel/irq/msi.c
> >> @@ -436,22 +436,22 @@ int __msi_domain_alloc_irqs(struct irq_domain
> >> *domain, struct device *dev,
> >>
> >>  	can_reserve = msi_check_reservation_mode(domain, info, dev);
> >>
> >> -	for_each_msi_entry(desc, dev) {
> >> -		virq = desc->irq;
> >> -		if (desc->nvec_used == 1)
> >> -			dev_dbg(dev, "irq %d for MSI\n", virq);
> >> -		else
> >> +	/*
> >> +	 * This flag is set by the PCI layer as we need to activate
> >> +	 * the MSI entries before the PCI layer enables MSI in the
> >> +	 * card. Otherwise the card latches a random msi message.
> >> +	 */
> >> +	if (!(info->flags & MSI_FLAG_ACTIVATE_EARLY))
> >> +		goto skip_activate;
> >
> > This will change the dbg print behavior. From the commit f3b0946d629c,
> > it looks like the below dev_dbg() code was there for
> > !MSI_FLAG_ACTIVATE_EARLY
> > case as well. Not sure how much this matters though.
> 
> I'm not sure this matters either. We may have relied on these statements
> some 6/7 years ago, as the whole hierarchy stuff was brand new, but we
> now have a much better debug infrastructure thanks to Thomas. I'd be
> totally in favour of dropping it.
> 
Ok.

Tested on D06 with gicv4 enabled and Guest MSI dev works fine.

FWIW,
   Tested-by: Shameer Kolothum <shameerali.kolothum.thodi@xxxxxxxxxx>

Thanks,
Shameer