* [PATCH v3] s390/pci: fix CPU address in MSI for directed IRQ @ 2020-11-26 17:00 Alexander Gordeev 2020-11-27 8:56 ` Halil Pasic 2020-12-09 15:08 ` Naresh Kamboju 0 siblings, 2 replies; 8+ messages in thread From: Alexander Gordeev @ 2020-11-26 17:00 UTC (permalink / raw) To: Niklas Schnelle; +Cc: Alexander Gordeev, linux-s390, linux-kernel, Halil Pasic 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v3] s390/pci: fix CPU address in MSI for directed IRQ 2020-11-26 17:00 [PATCH v3] s390/pci: fix CPU address in MSI for directed IRQ Alexander Gordeev @ 2020-11-27 8:56 ` Halil Pasic 2020-11-27 10:08 ` Niklas Schnelle 2020-12-09 15:08 ` Naresh Kamboju 1 sibling, 1 reply; 8+ messages in thread From: Halil Pasic @ 2020-11-27 8:56 UTC (permalink / raw) To: Alexander Gordeev; +Cc: Niklas Schnelle, linux-s390, linux-kernel 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); > } ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] s390/pci: fix CPU address in MSI for directed IRQ 2020-11-27 8:56 ` Halil Pasic @ 2020-11-27 10:08 ` Niklas Schnelle 2020-11-27 15:39 ` Halil Pasic 0 siblings, 1 reply; 8+ messages in thread From: Niklas Schnelle @ 2020-11-27 10:08 UTC (permalink / raw) To: Halil Pasic, Alexander Gordeev; +Cc: linux-s390, linux-kernel 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); >> } > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] s390/pci: fix CPU address in MSI for directed IRQ 2020-11-27 10:08 ` Niklas Schnelle @ 2020-11-27 15:39 ` Halil Pasic 2020-11-30 8:30 ` Niklas Schnelle 0 siblings, 1 reply; 8+ messages in thread From: Halil Pasic @ 2020-11-27 15:39 UTC (permalink / raw) To: Niklas Schnelle; +Cc: Alexander Gordeev, linux-s390, linux-kernel 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); > >> } > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] s390/pci: fix CPU address in MSI for directed IRQ 2020-11-27 15:39 ` Halil Pasic @ 2020-11-30 8:30 ` Niklas Schnelle 2020-11-30 8:55 ` Halil Pasic 0 siblings, 1 reply; 8+ messages in thread From: Niklas Schnelle @ 2020-11-30 8:30 UTC (permalink / raw) To: Halil Pasic; +Cc: Alexander Gordeev, linux-s390, linux-kernel 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); >>>> } >>> > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] s390/pci: fix CPU address in MSI for directed IRQ 2020-11-30 8:30 ` Niklas Schnelle @ 2020-11-30 8:55 ` Halil Pasic 2020-11-30 9:50 ` Niklas Schnelle 0 siblings, 1 reply; 8+ messages in thread From: Halil Pasic @ 2020-11-30 8:55 UTC (permalink / raw) To: Niklas Schnelle; +Cc: Alexander Gordeev, linux-s390, linux-kernel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] s390/pci: fix CPU address in MSI for directed IRQ 2020-11-30 8:55 ` Halil Pasic @ 2020-11-30 9:50 ` Niklas Schnelle 0 siblings, 0 replies; 8+ messages in thread From: Niklas Schnelle @ 2020-11-30 9:50 UTC (permalink / raw) To: Halil Pasic; +Cc: Alexander Gordeev, linux-s390, linux-kernel 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 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] s390/pci: fix CPU address in MSI for directed IRQ 2020-11-26 17:00 [PATCH v3] s390/pci: fix CPU address in MSI for directed IRQ Alexander Gordeev 2020-11-27 8:56 ` Halil Pasic @ 2020-12-09 15:08 ` Naresh Kamboju 1 sibling, 0 replies; 8+ messages in thread From: Naresh Kamboju @ 2020-12-09 15:08 UTC (permalink / raw) To: Alexander Gordeev Cc: Niklas Schnelle, linux-s390, open list, Halil Pasic, linux-stable, lkft-triage, Greg Kroah-Hartman, Sasha Levin 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-12-09 15:10 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-11-26 17:00 [PATCH v3] s390/pci: fix CPU address in MSI for directed IRQ Alexander Gordeev 2020-11-27 8:56 ` Halil Pasic 2020-11-27 10:08 ` Niklas Schnelle 2020-11-27 15:39 ` Halil Pasic 2020-11-30 8:30 ` Niklas Schnelle 2020-11-30 8:55 ` Halil Pasic 2020-11-30 9:50 ` Niklas Schnelle 2020-12-09 15:08 ` Naresh Kamboju
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).