Re: [PATCH v2] s390/pci: fix CPU address in MSI for directed IRQ

[Alexander Gordeev <agordeev@linux.ibm.com>]

On Thu, Nov 26, 2020 at 04:08:57PM +0100, Niklas Schnelle wrote:
> 
> 
> On 11/26/20 1:19 PM, Alexander Gordeev wrote:
> > The directed MSIs are delivered to CPUs whose address is
> > written to the MSI message data. The current code assumes
> > that a CPU logical number (as it is seen by the kernel)
> > is also that CPU address.
> > 
> > The above assumption is not correct, as the CPU address
> > is rather the value returned by STAP instruction. That
> > value is not necessarily matches the kernel logical CPU
> > number.
> 
> I took the liberty of correcting the "is not" grammar error
> above to "does not necessarily match".
> 
> > 
> > Fixes: e979ce7bced2 ("s390/pci: provide support for CPU directed interrupts")
> > Signed-off-by: Alexander Gordeev <agordeev@linux.ibm.com>
> 
> Still works well and checkpatches clean. I 
> 
> > ---
> >  arch/s390/pci/pci_irq.c | 15 ++++++++++++---
> >  1 file changed, 12 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/s390/pci/pci_irq.c b/arch/s390/pci/pci_irq.c
> > index 743f257cf2cb..1309fd302f58 100644
> > --- a/arch/s390/pci/pci_irq.c
> > +++ b/arch/s390/pci/pci_irq.c
> > @@ -103,9 +103,10 @@ static int zpci_set_irq_affinity(struct irq_data *data, const struct cpumask *de
> >  {
> >  	struct msi_desc *entry = irq_get_msi_desc(data->irq);
> >  	struct msi_msg msg = entry->msg;
> > +	int cpu_addr = smp_cpu_get_cpu_address(cpumask_first(dest));
> >  
> >  	msg.address_lo &= 0xff0000ff;
> > -	msg.address_lo |= (cpumask_first(dest) << 8);
> > +	msg.address_lo |= (cpu_addr << 8);
> >  	pci_write_msi_msg(data->irq, &msg);
> >  
> >  	return IRQ_SET_MASK_OK;
> > @@ -238,6 +239,7 @@ int arch_setup_msi_irqs(struct pci_dev *pdev, int nvec, int type)
> >  	unsigned long bit;
> >  	struct msi_desc *msi;
> >  	struct msi_msg msg;
> > +	int cpu_addr;
> >  	int rc, irq;
> >  
> >  	zdev->aisb = -1UL;
> > @@ -287,9 +289,16 @@ int arch_setup_msi_irqs(struct pci_dev *pdev, int nvec, int type)
> >  					 handle_percpu_irq);
> >  		msg.data = hwirq - bit;
> >  		if (irq_delivery == DIRECTED) {
> > +			if (msi->affinity) {
> > +				cpu = cpumask_first(&msi->affinity->mask);
> > +				cpu_addr = smp_cpu_get_cpu_address(cpu);
> > +			} else {
> > +				cpu_addr = 0;
> > +			}
> 
> One question I haven't really figured out from looking at the spec is
> why using cpu_addr = 0; is a good fallback. Shouldn't that be smp_cpu_get_cpu_address(0) or
> do we now know that the CPU addresses always start at 0?

Nice catch! I think the safest way is smp_cpu_get_cpu_address(0)
whatever the spec says. I'll send v3. 

> > +
> >  			msg.address_lo = zdev->msi_addr & 0xff0000ff;
> > -			msg.address_lo |= msi->affinity ?
> > -				(cpumask_first(&msi->affinity->mask) << 8) : 0;
> > +			msg.address_lo |= (cpu_addr << 8);
> > +
> >  			for_each_possible_cpu(cpu) {
> >  				airq_iv_set_data(zpci_ibv[cpu], hwirq, irq);
> >  			}
> >