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

[PATCH v3] 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 does not necessarily match 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 | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/arch/s390/pci/pci_irq.c b/arch/s390/pci/pci_irq.c
index 743f257cf2cb..75217fb63d7b 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,15 @@ 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);
+			else
+				cpu = 0;
+			cpu_addr = smp_cpu_get_cpu_address(cpu);
+
 			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 v3] s390/pci: fix CPU address in MSI for directed IRQ

[Halil Pasic]

On Thu, 26 Nov 2020 18:00:37 +0100
Alexander Gordeev <agordeev@linux.ibm.com> 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 does not necessarily match 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 | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/s390/pci/pci_irq.c b/arch/s390/pci/pci_irq.c
> index 743f257cf2cb..75217fb63d7b 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,15 @@ 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);
> +			else
> +				cpu = 0;
> +			cpu_addr = smp_cpu_get_cpu_address(cpu);
> +

I thin style wise, I would prefer keeping the ternary operator instead
of rewriting it as an if-then-else, i.e.:
                        cpu_addr = smp_cpu_get_cpu_address(msi->affinity ?      
                                cpumask_first(&msi->affinity->mask) : 0);
but either way:

Reviewed-by: Halil Pasic <pasic@linux.ibm.com> 

>  			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 v3] s390/pci: fix CPU address in MSI for directed IRQ

[Niklas Schnelle]

On 11/27/20 9:56 AM, Halil Pasic wrote:
> On Thu, 26 Nov 2020 18:00:37 +0100
> Alexander Gordeev <agordeev@linux.ibm.com> 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 does not necessarily match 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 | 14 +++++++++++---
>>  1 file changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/s390/pci/pci_irq.c b/arch/s390/pci/pci_irq.c
>> index 743f257cf2cb..75217fb63d7b 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,15 @@ 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);
>> +			else
>> +				cpu = 0;
>> +			cpu_addr = smp_cpu_get_cpu_address(cpu);
>> +
> 
> I thin style wise, I would prefer keeping the ternary operator instead
> of rewriting it as an if-then-else, i.e.:
>                         cpu_addr = smp_cpu_get_cpu_address(msi->affinity ?      
>                                 cpumask_first(&msi->affinity->mask) : 0);
> but either way:
> 
> Reviewed-by: Halil Pasic <pasic@linux.ibm.com> 

Thanks for your review, lets keep the if/else its certainly not less
readable even if it may be less pretty.

Found another thing (not directly in the touched code) but I'm now
wondering about. In zpci_handle_cpu_local_irq()
we do
	struct airq_iv *dibv = zpci_ibv[smp_processor_id()];

does that also need to use some _address() variant? If it does that
then dicatates that the CPU addresses must 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 v3] s390/pci: fix CPU address in MSI for directed IRQ

[Halil Pasic]

On Fri, 27 Nov 2020 11:08:10 +0100
Niklas Schnelle <schnelle@linux.ibm.com> wrote:

> 
> 
> On 11/27/20 9:56 AM, Halil Pasic wrote:
> > On Thu, 26 Nov 2020 18:00:37 +0100
> > Alexander Gordeev <agordeev@linux.ibm.com> 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 does not necessarily match 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 | 14 +++++++++++---
> >>  1 file changed, 11 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/arch/s390/pci/pci_irq.c b/arch/s390/pci/pci_irq.c
> >> index 743f257cf2cb..75217fb63d7b 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,15 @@ 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);
> >> +			else
> >> +				cpu = 0;
> >> +			cpu_addr = smp_cpu_get_cpu_address(cpu);
> >> +
> > 
> > I thin style wise, I would prefer keeping the ternary operator instead
> > of rewriting it as an if-then-else, i.e.:
> >                         cpu_addr = smp_cpu_get_cpu_address(msi->affinity ?      
> >                                 cpumask_first(&msi->affinity->mask) : 0);
> > but either way:
> > 
> > Reviewed-by: Halil Pasic <pasic@linux.ibm.com> 
> 
> Thanks for your review, lets keep the if/else its certainly not less
> readable even if it may be less pretty.
> 
> Found another thing (not directly in the touched code) but I'm now
> wondering about. In zpci_handle_cpu_local_irq()
> we do
> 	struct airq_iv *dibv = zpci_ibv[smp_processor_id()];
> 
> does that also need to use some _address() variant? If it does that
> then dicatates that the CPU addresses must start at 0.
> 

I didn't go to the bottom of this, but my understanding is that it
does not need a _address() variant. What we need is, probably, the
mapping between the 'id' and 'address' being a stable one.

Please notice that cpu_enable_directed_irq() is called on each cpu. That
establishes the mapping/relationship between the id and the address,
as the machine cares for the address, and cpu_enable_directed_irq()
cares for the id:
static void __init cpu_enable_directed_irq(void *unused)                        
{                                                                               
        union zpci_sic_iib iib = {{0}};                                         
                                                                                
        iib.cdiib.dibv_addr = (u64) zpci_ibv[smp_processor_id()]->vector;       
                                                                                
        __zpci_set_irq_ctrl(SIC_IRQ_MODE_SET_CPU, 0, &iib);                     
        zpci_set_irq_ctrl(SIC_IRQ_MODE_D_SINGLE, PCI_ISC);                      
}

Now were the id <-> address mapping to change, we would be in trouble. If
that's possible, I don't know. My guess is that it would require cpu hot
unplug. Niklas, are you familiar with that stuff? Should we ask, Heiko
and Vasily?

Regards,
Halil

> > 
> >>  			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 v3] s390/pci: fix CPU address in MSI for directed IRQ

[Niklas Schnelle]

On 11/27/20 4:39 PM, Halil Pasic wrote:
> On Fri, 27 Nov 2020 11:08:10 +0100
> Niklas Schnelle <schnelle@linux.ibm.com> wrote:
> 
>>
>>
>> On 11/27/20 9:56 AM, Halil Pasic wrote:
>>> On Thu, 26 Nov 2020 18:00:37 +0100
>>> Alexander Gordeev <agordeev@linux.ibm.com> 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 does not necessarily match 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 | 14 +++++++++++---
>>>>  1 file changed, 11 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/arch/s390/pci/pci_irq.c b/arch/s390/pci/pci_irq.c
>>>> index 743f257cf2cb..75217fb63d7b 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,15 @@ 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);
>>>> +			else
>>>> +				cpu = 0;
>>>> +			cpu_addr = smp_cpu_get_cpu_address(cpu);
>>>> +
>>>
>>> I thin style wise, I would prefer keeping the ternary operator instead
>>> of rewriting it as an if-then-else, i.e.:
>>>                         cpu_addr = smp_cpu_get_cpu_address(msi->affinity ?      
>>>                                 cpumask_first(&msi->affinity->mask) : 0);
>>> but either way:
>>>
>>> Reviewed-by: Halil Pasic <pasic@linux.ibm.com> 
>>
>> Thanks for your review, lets keep the if/else its certainly not less
>> readable even if it may be less pretty.
>>
>> Found another thing (not directly in the touched code) but I'm now
>> wondering about. In zpci_handle_cpu_local_irq()
>> we do
>> 	struct airq_iv *dibv = zpci_ibv[smp_processor_id()];
>>
>> does that also need to use some _address() variant? If it does that
>> then dicatates that the CPU addresses must start at 0.
>>
> 
> I didn't go to the bottom of this, but my understanding is that it
> does not need a _address() variant. What we need is, probably, the
> mapping between the 'id' and 'address' being a stable one.
> 
> Please notice that cpu_enable_directed_irq() is called on each cpu. That
> establishes the mapping/relationship between the id and the address,
> as the machine cares for the address, and cpu_enable_directed_irq()
> cares for the id:
> static void __init cpu_enable_directed_irq(void *unused)                        
> {                                                                               
>         union zpci_sic_iib iib = {{0}};                                         
>                                                                                 
>         iib.cdiib.dibv_addr = (u64) zpci_ibv[smp_processor_id()]->vector;       
>                                                                                 
>         __zpci_set_irq_ctrl(SIC_IRQ_MODE_SET_CPU, 0, &iib);                     
>         zpci_set_irq_ctrl(SIC_IRQ_MODE_D_SINGLE, PCI_ISC);                      
> }

Thanks for the very clear and understandable explanation, I think
you're exactly right. I didn't look very closely and missed that
cpu_enable_directed_irq() uses the smp_processor_id() thereby
establishing the mapping.

> 
> Now were the id <-> address mapping to change, we would be in trouble. If
> that's possible, I don't know. My guess is that it would require cpu hot
> unplug. Niklas, are you familiar with that stuff? Should we ask, Heiko
> and Vasily?
> 
> Regards,
> Halil

I'm not really familiar, with it but I think this is closely related
to what I asked Bernd Nerz. I fear that if CPUs go away we might already
be in trouble at the firmware/hardware/platform level because the CPU Address is
"programmed into the device" so to speak. Thus a directed interrupt from
a device may race with anything reordering/removing CPUs even if
CPU addresses of dead CPUs are not reused and the mapping is stable.

Furthermore our floating fallback path will try to send a SIGP
to the target CPU which clearly doesn't work when that is permanently
gone. Either way I think these issues are out of scope for this fix
so I will go ahead and merge this.

> 
>>>
>>>>  			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 v3] s390/pci: fix CPU address in MSI for directed IRQ

[Halil Pasic]

On Mon, 30 Nov 2020 09:30:33 +0100
Niklas Schnelle <schnelle@linux.ibm.com> wrote:

> I'm not really familiar, with it but I think this is closely related
> to what I asked Bernd Nerz. I fear that if CPUs go away we might already
> be in trouble at the firmware/hardware/platform level because the CPU Address is
> "programmed into the device" so to speak. Thus a directed interrupt from
> a device may race with anything reordering/removing CPUs even if
> CPU addresses of dead CPUs are not reused and the mapping is stable.

From your answer, I read that CPU hot-unplug is supported for LPAR. 
> 
> Furthermore our floating fallback path will try to send a SIGP
> to the target CPU which clearly doesn't work when that is permanently
> gone. Either way I think these issues are out of scope for this fix
> so I will go ahead and merge this.

I agree, it makes on sense to delay this fix.

But if CPU hot-unplug is supported, I believe we should react when
a CPU is unplugged, that is a target of directed interrupts. My guess
is, that in this scenario transient hiccups are unavoidable, and thus
should be accepted, but we should make sure that we recover.

Regards,
Halil

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

[Niklas Schnelle]

On 11/30/20 9:55 AM, Halil Pasic wrote:
> On Mon, 30 Nov 2020 09:30:33 +0100
> Niklas Schnelle <schnelle@linux.ibm.com> wrote:
> 
>> I'm not really familiar, with it but I think this is closely related
>> to what I asked Bernd Nerz. I fear that if CPUs go away we might already
>> be in trouble at the firmware/hardware/platform level because the CPU Address is
>> "programmed into the device" so to speak. Thus a directed interrupt from
>> a device may race with anything reordering/removing CPUs even if
>> CPU addresses of dead CPUs are not reused and the mapping is stable.
> 
> From your answer, I read that CPU hot-unplug is supported for LPAR. 

I'm not sure about hot hot-unplug and firmware
telling us about removed CPUs but at the very least there is:

echo 0 > /sys/devices/system/cpu/cpu6/online

>>
>> Furthermore our floating fallback path will try to send a SIGP
>> to the target CPU which clearly doesn't work when that is permanently
>> gone. Either way I think these issues are out of scope for this fix
>> so I will go ahead and merge this.
> 
> I agree, it makes on sense to delay this fix.
> 
> But if CPU hot-unplug is supported, I believe we should react when
> a CPU is unplugged, that is a target of directed interrupts. My guess
> is, that in this scenario transient hiccups are unavoidable, and thus
> should be accepted, but we should make sure that we recover.

I agree, I just tested the above command on a firmware test system and
deactivated 4 of 8 CPUs.
This is in /proc/interrupts after that:

...
  3:       9392          0          0          0   PCI-MSI  mlx5_async@pci:0001:00:00.0
  4:     282741          0          0          0   PCI-MSI  mlx5_comp0@pci:0001:00:00.0
  5:          0          2          0          0   PCI-MSI  mlx5_comp1@pci:0001:00:00.0
  6:          0          0        104          0   PCI-MSI  mlx5_comp2@pci:0001:00:00.0
  7:          0          0          0          2   PCI-MSI  mlx5_comp3@pci:0001:00:00.0
  8:          0          0          0          0   PCI-MSI  mlx5_comp4@pci:0001:00:00.0
  9:          0          0          0          0   PCI-MSI  mlx5_comp5@pci:0001:00:00.0
 10:          0          0          0          0   PCI-MSI  mlx5_comp6@pci:0001:00:00.0
 11:          0          0          0          0   PCI-MSI  mlx5_comp7@pci:0001:00:00.0
...

So it looks like we are left with registered interrupts
for CPUs which are offline. However I'm not sure how to
trigger a problem with that. I think the drivers would
usually only do a directed interrupt to a CPU that
is currently running the process that triggered the
I/O (I tested this assumption with "taskset -c 2 ping ...").
Now with the CPU offline there cannot be such a
process. So I think for the most part the queue would
just remain unused. Still, if we do get a directed
interrupt for it's my understanding that currently
we will lose that.

I think this could be fixed with
something I tried in a prototype code a while back that
is in zpci_handle_fallback_irq() I handled the IRQ locally.
Back then it looked like Directed IRQs would make it to z15 GA 1.5 and
this was done to help Bernd to debug a Millicode issue (Jup 905371).
I also had a version of that code meant as a possible performance
improvement that would check if the target CPU is
available and only then send the SIGP and otherwise handle it
locally.

> 
> Regards,
> Halil
> 

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

[Naresh Kamboju]

On Thu, 26 Nov 2020 at 22:32, Alexander Gordeev <agordeev@linux.ibm.com> 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 does not necessarily match 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 | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/arch/s390/pci/pci_irq.c b/arch/s390/pci/pci_irq.c
> index 743f257cf2cb..75217fb63d7b 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));

While building S390 the following kernel warning / error noticed
on stable -rc 5.4 branch with gcc-8, gcc-9 and gcc-10 and defconfig

make --silent --keep-going --jobs=8
O=/home/tuxbuild/.cache/tuxmake/builds/6/tmp ARCH=s390
CROSS_COMPILE=s390x-linux-gnu- 'CC=sccache s390x-linux-gnu-gcc'
'HOSTCC=sccache gcc' vmlinux
arch/s390/pci/pci_irq.c: In function 'zpci_set_irq_affinity':
arch/s390/pci/pci_irq.c:106:17: error: implicit declaration of
function 'smp_cpu_get_cpu_address'
[-Werror=implicit-function-declaration]
  106 |  int cpu_addr = smp_cpu_get_cpu_address(cpumask_first(dest));
      |                 ^~~~~~~~~~~~~~~~~~~~~~~


Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>

steps to reproduce:
--------------------------
# TuxMake is a command line tool and Python library that provides
# portable and repeatable Linux kernel builds across a variety of
# architectures, toolchains, kernel configurations, and make targets.
#
# TuxMake supports the concept of runtimes.
# See https://docs.tuxmake.org/runtimes/, for that to work it requires
# that you install podman or docker on your system.
#
# To install tuxmake on your system globally:
# sudo pip3 install -U tuxmake
#
# See https://docs.tuxmake.org/ for complete documentation.

tuxmake --runtime docker --target-arch s390 --toolchain gcc-9
--kconfig defconfig


metadata:
    git_repo: https://gitlab.com/Linaro/lkft/mirrors/stable/linux-stable-rc
    target_arch: s390
    toolchain: gcc-9
    git_describe: v5.4.82-36-gc45075765dae
    kernel_version: 5.4.83-rc1


full build log link,
https://gitlab.com/Linaro/lkft/mirrors/stable/linux-stable-rc/-/jobs/899272224

-- 
Linaro LKFT
https://lkft.linaro.org