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

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

[Alexander Gordeev]

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.

Fixes: e979ce7bced2 ("s390/pci: provide support for CPU directed interrupts")
Signed-off-by: Alexander Gordeev <agordeev@linux.ibm.com>
---
 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;
+			}
+
 			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);
 			}
-- 
2.26.0

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

[Niklas Schnelle]

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?

> +
>  			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);
>  			}
> 

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

[Alexander Gordeev]

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);
> >  			}
> >