[PATCH] PCI: Fix pcibios_update_irq misuse of irq number

[PATCH] PCI: Fix pcibios_update_irq misuse of irq number

[Marc Zyngier]

pcibios_update_irq writes an irq number into the config space
of a given PCI device, but ignores the fact that this number
is a virtual interrupt number, which might be a very different
value from what the underlying hardware is using.

The obvious fix is to fetch the HW interrupt number from the
corresponding irq_data structure. This is slightly complicated
by the fact that this interrupt might be services by a stacked
domain.

This has been tested on KVM with kvmtool.

Reported-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Tested-by: Andre Przywara <andre.przywara@arm.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/pci/setup-irq.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/setup-irq.c b/drivers/pci/setup-irq.c
index 4e2d595..828cbc9 100644
--- a/drivers/pci/setup-irq.c
+++ b/drivers/pci/setup-irq.c
@@ -15,11 +15,19 @@
 #include <linux/errno.h>
 #include <linux/ioport.h>
 #include <linux/cache.h>
+#include <linux/irq.h>
 
 void __weak pcibios_update_irq(struct pci_dev *dev, int irq)
 {
-	dev_dbg(&dev->dev, "assigning IRQ %02d\n", irq);
-	pci_write_config_byte(dev, PCI_INTERRUPT_LINE, irq);
+	struct irq_data *d;
+
+	d = irq_get_irq_data(irq);
+#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
+	while (d->parent_data)
+		d = d->parent_data;
+#endif
+	dev_dbg(&dev->dev, "assigning IRQ %02ld\n", d->hwirq);
+	pci_write_config_byte(dev, PCI_INTERRUPT_LINE, d->hwirq);
 }
 
 static void pdev_fixup_irq(struct pci_dev *dev,
-- 
2.1.4

Re: [PATCH] PCI: Fix pcibios_update_irq misuse of irq number

[Jiang Liu]

On 2015/1/28 22:51, Marc Zyngier wrote:
> pcibios_update_irq writes an irq number into the config space
> of a given PCI device, but ignores the fact that this number
> is a virtual interrupt number, which might be a very different
> value from what the underlying hardware is using.
> 
> The obvious fix is to fetch the HW interrupt number from the
> corresponding irq_data structure. This is slightly complicated
> by the fact that this interrupt might be services by a stacked
> domain.
> 
> This has been tested on KVM with kvmtool.
> 
> Reported-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Tested-by: Andre Przywara <andre.przywara@arm.com>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  drivers/pci/setup-irq.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/setup-irq.c b/drivers/pci/setup-irq.c
> index 4e2d595..828cbc9 100644
> --- a/drivers/pci/setup-irq.c
> +++ b/drivers/pci/setup-irq.c
> @@ -15,11 +15,19 @@
>  #include <linux/errno.h>
>  #include <linux/ioport.h>
>  #include <linux/cache.h>
> +#include <linux/irq.h>
>  
>  void __weak pcibios_update_irq(struct pci_dev *dev, int irq)
>  {
> -	dev_dbg(&dev->dev, "assigning IRQ %02d\n", irq);
> -	pci_write_config_byte(dev, PCI_INTERRUPT_LINE, irq);
> +	struct irq_data *d;
> +
> +	d = irq_get_irq_data(irq);
> +#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
> +	while (d->parent_data)
> +		d = d->parent_data;
> +#endif
> +	dev_dbg(&dev->dev, "assigning IRQ %02ld\n", d->hwirq);
> +	pci_write_config_byte(dev, PCI_INTERRUPT_LINE, d->hwirq);
>  }
Hi Mark,
	Instead of modifying the common version, how about
implementing an arch specific version? Arch may have different
way to determine the irq number. Above implementation doesn't
work with x86, for example.
Regards!
Gerry

>  
>  static void pdev_fixup_irq(struct pci_dev *dev,
> 

Re: [PATCH] PCI: Fix pcibios_update_irq misuse of irq number

[Marc Zyngier]

Hi Gerry,

On 28/01/15 15:21, Jiang Liu wrote:
> 
> 
> On 2015/1/28 22:51, Marc Zyngier wrote:
>> pcibios_update_irq writes an irq number into the config space
>> of a given PCI device, but ignores the fact that this number
>> is a virtual interrupt number, which might be a very different
>> value from what the underlying hardware is using.
>>
>> The obvious fix is to fetch the HW interrupt number from the
>> corresponding irq_data structure. This is slightly complicated
>> by the fact that this interrupt might be services by a stacked
>> domain.
>>
>> This has been tested on KVM with kvmtool.
>>
>> Reported-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>> Tested-by: Andre Przywara <andre.przywara@arm.com>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  drivers/pci/setup-irq.c | 12 ++++++++++--
>>  1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pci/setup-irq.c b/drivers/pci/setup-irq.c
>> index 4e2d595..828cbc9 100644
>> --- a/drivers/pci/setup-irq.c
>> +++ b/drivers/pci/setup-irq.c
>> @@ -15,11 +15,19 @@
>>  #include <linux/errno.h>
>>  #include <linux/ioport.h>
>>  #include <linux/cache.h>
>> +#include <linux/irq.h>
>>  
>>  void __weak pcibios_update_irq(struct pci_dev *dev, int irq)
>>  {
>> -	dev_dbg(&dev->dev, "assigning IRQ %02d\n", irq);
>> -	pci_write_config_byte(dev, PCI_INTERRUPT_LINE, irq);
>> +	struct irq_data *d;
>> +
>> +	d = irq_get_irq_data(irq);
>> +#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
>> +	while (d->parent_data)
>> +		d = d->parent_data;
>> +#endif
>> +	dev_dbg(&dev->dev, "assigning IRQ %02ld\n", d->hwirq);
>> +	pci_write_config_byte(dev, PCI_INTERRUPT_LINE, d->hwirq);
>>  }
> Hi Mark,
> 	Instead of modifying the common version, how about
> implementing an arch specific version? Arch may have different
> way to determine the irq number. Above implementation doesn't
> work with x86, for example.

If you look at the Makefile, this file is used on:

obj-$(CONFIG_ALPHA) += setup-irq.o
obj-$(CONFIG_ARM) += setup-irq.o
obj-$(CONFIG_UNICORE32) += setup-irq.o
obj-$(CONFIG_SUPERH) += setup-irq.o
obj-$(CONFIG_MIPS) += setup-irq.o
obj-$(CONFIG_TILE) += setup-irq.o
obj-$(CONFIG_SPARC_LEON) += setup-irq.o
obj-$(CONFIG_M68K) += setup-irq.o

x86 doesn't use that at all.

That aside, do you know any case where this transformation would lead to
an invalid result? Worse case, irq should be equal to hwirq, shouldn't
it? Or are you thinking of something else?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH] PCI: Fix pcibios_update_irq misuse of irq number

[Bjorn Helgaas]

On Wed, Jan 28, 2015 at 9:27 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> Hi Gerry,
>
> On 28/01/15 15:21, Jiang Liu wrote:
>>
>>
>> On 2015/1/28 22:51, Marc Zyngier wrote:
>>> pcibios_update_irq writes an irq number into the config space
>>> of a given PCI device, but ignores the fact that this number
>>> is a virtual interrupt number, which might be a very different
>>> value from what the underlying hardware is using.
>>>
>>> The obvious fix is to fetch the HW interrupt number from the
>>> corresponding irq_data structure. This is slightly complicated
>>> by the fact that this interrupt might be services by a stacked
>>> domain.
>>>
>>> This has been tested on KVM with kvmtool.
>>>
>>> Reported-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>>> Tested-by: Andre Przywara <andre.przywara@arm.com>
>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>> ---
>>>  drivers/pci/setup-irq.c | 12 ++++++++++--
>>>  1 file changed, 10 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/pci/setup-irq.c b/drivers/pci/setup-irq.c
>>> index 4e2d595..828cbc9 100644
>>> --- a/drivers/pci/setup-irq.c
>>> +++ b/drivers/pci/setup-irq.c
>>> @@ -15,11 +15,19 @@
>>>  #include <linux/errno.h>
>>>  #include <linux/ioport.h>
>>>  #include <linux/cache.h>
>>> +#include <linux/irq.h>
>>>
>>>  void __weak pcibios_update_irq(struct pci_dev *dev, int irq)
>>>  {
>>> -    dev_dbg(&dev->dev, "assigning IRQ %02d\n", irq);
>>> -    pci_write_config_byte(dev, PCI_INTERRUPT_LINE, irq);
>>> +    struct irq_data *d;
>>> +
>>> +    d = irq_get_irq_data(irq);
>>> +#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
>>> +    while (d->parent_data)
>>> +            d = d->parent_data;
>>> +#endif
>>> +    dev_dbg(&dev->dev, "assigning IRQ %02ld\n", d->hwirq);
>>> +    pci_write_config_byte(dev, PCI_INTERRUPT_LINE, d->hwirq);
>>>  }
>> Hi Mark,
>>       Instead of modifying the common version, how about
>> implementing an arch specific version? Arch may have different
>> way to determine the irq number. Above implementation doesn't
>> work with x86, for example.
>
> If you look at the Makefile, this file is used on:
>
> obj-$(CONFIG_ALPHA) += setup-irq.o
> obj-$(CONFIG_ARM) += setup-irq.o
> obj-$(CONFIG_UNICORE32) += setup-irq.o
> obj-$(CONFIG_SUPERH) += setup-irq.o
> obj-$(CONFIG_MIPS) += setup-irq.o
> obj-$(CONFIG_TILE) += setup-irq.o
> obj-$(CONFIG_SPARC_LEON) += setup-irq.o
> obj-$(CONFIG_M68K) += setup-irq.o
>
> x86 doesn't use that at all.

Since you're looking at this, Marc, do you see a nice way to get rid
of these arch dependencies in the Makefile and unify this a bit?  We
still have this pci_fixup_irqs() ugliness -- it's not really
arch-specific at all, but it's called from arch code, and it uses
for_each_pci_dev(), which obviously only works for things present at
boot and not for things hot-added later.

Bjorn

Re: [PATCH] PCI: Fix pcibios_update_irq misuse of irq number

[Bjorn Helgaas]

On Wed, Jan 28, 2015 at 8:51 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> pcibios_update_irq writes an irq number into the config space
> of a given PCI device, but ignores the fact that this number
> is a virtual interrupt number, which might be a very different
> value from what the underlying hardware is using.
>
> The obvious fix is to fetch the HW interrupt number from the
> corresponding irq_data structure. This is slightly complicated
> by the fact that this interrupt might be services by a stacked
> domain.
>
> This has been tested on KVM with kvmtool.
>
> Reported-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Tested-by: Andre Przywara <andre.przywara@arm.com>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>

Jiang, are you OK with this patch as-is now, since it isn't used on x86?

Marc, Lorenzo, I assume this actually fixes a bug.  Can we get any
more details about what happens when you hit the bug, and how you
reproduced it (what platform, driver, etc.)?

Bjorn

> ---
>  drivers/pci/setup-irq.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/setup-irq.c b/drivers/pci/setup-irq.c
> index 4e2d595..828cbc9 100644
> --- a/drivers/pci/setup-irq.c
> +++ b/drivers/pci/setup-irq.c
> @@ -15,11 +15,19 @@
>  #include <linux/errno.h>
>  #include <linux/ioport.h>
>  #include <linux/cache.h>
> +#include <linux/irq.h>
>
>  void __weak pcibios_update_irq(struct pci_dev *dev, int irq)
>  {
> -       dev_dbg(&dev->dev, "assigning IRQ %02d\n", irq);
> -       pci_write_config_byte(dev, PCI_INTERRUPT_LINE, irq);
> +       struct irq_data *d;
> +
> +       d = irq_get_irq_data(irq);
> +#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
> +       while (d->parent_data)
> +               d = d->parent_data;
> +#endif
> +       dev_dbg(&dev->dev, "assigning IRQ %02ld\n", d->hwirq);
> +       pci_write_config_byte(dev, PCI_INTERRUPT_LINE, d->hwirq);
>  }
>
>  static void pdev_fixup_irq(struct pci_dev *dev,
> --
> 2.1.4
>

Re: [PATCH] PCI: Fix pcibios_update_irq misuse of irq number

[Jiang Liu]

On 2015/2/2 23:57, Bjorn Helgaas wrote:
> On Wed, Jan 28, 2015 at 8:51 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> pcibios_update_irq writes an irq number into the config space
>> of a given PCI device, but ignores the fact that this number
>> is a virtual interrupt number, which might be a very different
>> value from what the underlying hardware is using.
>>
>> The obvious fix is to fetch the HW interrupt number from the
>> corresponding irq_data structure. This is slightly complicated
>> by the fact that this interrupt might be services by a stacked
>> domain.
>>
>> This has been tested on KVM with kvmtool.
>>
>> Reported-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>> Tested-by: Andre Przywara <andre.przywara@arm.com>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> 
> Jiang, are you OK with this patch as-is now, since it isn't used on x86?
Sure, I'm OK. I missed the point that it's not used on x86 at all
in previous review.

> 
> Marc, Lorenzo, I assume this actually fixes a bug.  Can we get any
> more details about what happens when you hit the bug, and how you
> reproduced it (what platform, driver, etc.)?
> 
> Bjorn
> 
>> ---
>>  drivers/pci/setup-irq.c | 12 ++++++++++--
>>  1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pci/setup-irq.c b/drivers/pci/setup-irq.c
>> index 4e2d595..828cbc9 100644
>> --- a/drivers/pci/setup-irq.c
>> +++ b/drivers/pci/setup-irq.c
>> @@ -15,11 +15,19 @@
>>  #include <linux/errno.h>
>>  #include <linux/ioport.h>
>>  #include <linux/cache.h>
>> +#include <linux/irq.h>
>>
>>  void __weak pcibios_update_irq(struct pci_dev *dev, int irq)
>>  {
>> -       dev_dbg(&dev->dev, "assigning IRQ %02d\n", irq);
>> -       pci_write_config_byte(dev, PCI_INTERRUPT_LINE, irq);
>> +       struct irq_data *d;
>> +
>> +       d = irq_get_irq_data(irq);
>> +#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
>> +       while (d->parent_data)
>> +               d = d->parent_data;
>> +#endif
>> +       dev_dbg(&dev->dev, "assigning IRQ %02ld\n", d->hwirq);
>> +       pci_write_config_byte(dev, PCI_INTERRUPT_LINE, d->hwirq);
>>  }
>>
>>  static void pdev_fixup_irq(struct pci_dev *dev,
>> --
>> 2.1.4
>>

Re: [PATCH] PCI: Fix pcibios_update_irq misuse of irq number

[Marc Zyngier]

On 28/01/15 15:43, Bjorn Helgaas wrote:
> On Wed, Jan 28, 2015 at 9:27 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> Hi Gerry,
>>
>> On 28/01/15 15:21, Jiang Liu wrote:
>>>
>>>
>>> On 2015/1/28 22:51, Marc Zyngier wrote:
>>>> pcibios_update_irq writes an irq number into the config space
>>>> of a given PCI device, but ignores the fact that this number
>>>> is a virtual interrupt number, which might be a very different
>>>> value from what the underlying hardware is using.
>>>>
>>>> The obvious fix is to fetch the HW interrupt number from the
>>>> corresponding irq_data structure. This is slightly complicated
>>>> by the fact that this interrupt might be services by a stacked
>>>> domain.
>>>>
>>>> This has been tested on KVM with kvmtool.
>>>>
>>>> Reported-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>>>> Tested-by: Andre Przywara <andre.przywara@arm.com>
>>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>>> ---
>>>>  drivers/pci/setup-irq.c | 12 ++++++++++--
>>>>  1 file changed, 10 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/setup-irq.c b/drivers/pci/setup-irq.c
>>>> index 4e2d595..828cbc9 100644
>>>> --- a/drivers/pci/setup-irq.c
>>>> +++ b/drivers/pci/setup-irq.c
>>>> @@ -15,11 +15,19 @@
>>>>  #include <linux/errno.h>
>>>>  #include <linux/ioport.h>
>>>>  #include <linux/cache.h>
>>>> +#include <linux/irq.h>
>>>>
>>>>  void __weak pcibios_update_irq(struct pci_dev *dev, int irq)
>>>>  {
>>>> -    dev_dbg(&dev->dev, "assigning IRQ %02d\n", irq);
>>>> -    pci_write_config_byte(dev, PCI_INTERRUPT_LINE, irq);
>>>> +    struct irq_data *d;
>>>> +
>>>> +    d = irq_get_irq_data(irq);
>>>> +#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
>>>> +    while (d->parent_data)
>>>> +            d = d->parent_data;
>>>> +#endif
>>>> +    dev_dbg(&dev->dev, "assigning IRQ %02ld\n", d->hwirq);
>>>> +    pci_write_config_byte(dev, PCI_INTERRUPT_LINE, d->hwirq);
>>>>  }
>>> Hi Mark,
>>>       Instead of modifying the common version, how about
>>> implementing an arch specific version? Arch may have different
>>> way to determine the irq number. Above implementation doesn't
>>> work with x86, for example.
>>
>> If you look at the Makefile, this file is used on:
>>
>> obj-$(CONFIG_ALPHA) += setup-irq.o
>> obj-$(CONFIG_ARM) += setup-irq.o
>> obj-$(CONFIG_UNICORE32) += setup-irq.o
>> obj-$(CONFIG_SUPERH) += setup-irq.o
>> obj-$(CONFIG_MIPS) += setup-irq.o
>> obj-$(CONFIG_TILE) += setup-irq.o
>> obj-$(CONFIG_SPARC_LEON) += setup-irq.o
>> obj-$(CONFIG_M68K) += setup-irq.o
>>
>> x86 doesn't use that at all.
> 
> Since you're looking at this, Marc, do you see a nice way to get rid
> of these arch dependencies in the Makefile and unify this a bit?  We
> still have this pci_fixup_irqs() ugliness -- it's not really
> arch-specific at all, but it's called from arch code, and it uses
> for_each_pci_dev(), which obviously only works for things present at
> boot and not for things hot-added later.

I can have a look at this in the next cycle - I'm a bit strapped for
time just now.

As for for_each_pci_dev(), I'm not completely clear about what it should
be replaced for. Do we have some form of notifier for this?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH] PCI: Fix pcibios_update_irq misuse of irq number

[Bjorn Helgaas]

On Mon, Feb 2, 2015 at 10:15 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 28/01/15 15:43, Bjorn Helgaas wrote:
>> On Wed, Jan 28, 2015 at 9:27 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>> Hi Gerry,
>>>
>>> On 28/01/15 15:21, Jiang Liu wrote:
>>>>
>>>>
>>>> On 2015/1/28 22:51, Marc Zyngier wrote:
>>>>> pcibios_update_irq writes an irq number into the config space
>>>>> of a given PCI device, but ignores the fact that this number
>>>>> is a virtual interrupt number, which might be a very different
>>>>> value from what the underlying hardware is using.
>>>>>
>>>>> The obvious fix is to fetch the HW interrupt number from the
>>>>> corresponding irq_data structure. This is slightly complicated
>>>>> by the fact that this interrupt might be services by a stacked
>>>>> domain.
>>>>>
>>>>> This has been tested on KVM with kvmtool.
>>>>>
>>>>> Reported-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>>>>> Tested-by: Andre Przywara <andre.przywara@arm.com>
>>>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>>>> ---
>>>>>  drivers/pci/setup-irq.c | 12 ++++++++++--
>>>>>  1 file changed, 10 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/pci/setup-irq.c b/drivers/pci/setup-irq.c
>>>>> index 4e2d595..828cbc9 100644
>>>>> --- a/drivers/pci/setup-irq.c
>>>>> +++ b/drivers/pci/setup-irq.c
>>>>> @@ -15,11 +15,19 @@
>>>>>  #include <linux/errno.h>
>>>>>  #include <linux/ioport.h>
>>>>>  #include <linux/cache.h>
>>>>> +#include <linux/irq.h>
>>>>>
>>>>>  void __weak pcibios_update_irq(struct pci_dev *dev, int irq)
>>>>>  {
>>>>> -    dev_dbg(&dev->dev, "assigning IRQ %02d\n", irq);
>>>>> -    pci_write_config_byte(dev, PCI_INTERRUPT_LINE, irq);
>>>>> +    struct irq_data *d;
>>>>> +
>>>>> +    d = irq_get_irq_data(irq);
>>>>> +#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
>>>>> +    while (d->parent_data)
>>>>> +            d = d->parent_data;
>>>>> +#endif
>>>>> +    dev_dbg(&dev->dev, "assigning IRQ %02ld\n", d->hwirq);
>>>>> +    pci_write_config_byte(dev, PCI_INTERRUPT_LINE, d->hwirq);
>>>>>  }
>>>> Hi Mark,
>>>>       Instead of modifying the common version, how about
>>>> implementing an arch specific version? Arch may have different
>>>> way to determine the irq number. Above implementation doesn't
>>>> work with x86, for example.
>>>
>>> If you look at the Makefile, this file is used on:
>>>
>>> obj-$(CONFIG_ALPHA) += setup-irq.o
>>> obj-$(CONFIG_ARM) += setup-irq.o
>>> obj-$(CONFIG_UNICORE32) += setup-irq.o
>>> obj-$(CONFIG_SUPERH) += setup-irq.o
>>> obj-$(CONFIG_MIPS) += setup-irq.o
>>> obj-$(CONFIG_TILE) += setup-irq.o
>>> obj-$(CONFIG_SPARC_LEON) += setup-irq.o
>>> obj-$(CONFIG_M68K) += setup-irq.o
>>>
>>> x86 doesn't use that at all.
>>
>> Since you're looking at this, Marc, do you see a nice way to get rid
>> of these arch dependencies in the Makefile and unify this a bit?  We
>> still have this pci_fixup_irqs() ugliness -- it's not really
>> arch-specific at all, but it's called from arch code, and it uses
>> for_each_pci_dev(), which obviously only works for things present at
>> boot and not for things hot-added later.
>
> I can have a look at this in the next cycle - I'm a bit strapped for
> time just now.
>
> As for for_each_pci_dev(), I'm not completely clear about what it should
> be replaced for. Do we have some form of notifier for this?

I think this should be done somewhere in the enumeration path, e.g.,
maybe in pci_device_add().  Then we shouldn't need a
for_each_pci_dev() loop or a notifier at all.

Bjorn

Re: [PATCH] PCI: Fix pcibios_update_irq misuse of irq number

[Marc Zyngier]

On 02/02/15 15:57, Bjorn Helgaas wrote:
> On Wed, Jan 28, 2015 at 8:51 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> pcibios_update_irq writes an irq number into the config space
>> of a given PCI device, but ignores the fact that this number
>> is a virtual interrupt number, which might be a very different
>> value from what the underlying hardware is using.
>>
>> The obvious fix is to fetch the HW interrupt number from the
>> corresponding irq_data structure. This is slightly complicated
>> by the fact that this interrupt might be services by a stacked
>> domain.
>>
>> This has been tested on KVM with kvmtool.
>>
>> Reported-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>> Tested-by: Andre Przywara <andre.przywara@arm.com>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> 
> Jiang, are you OK with this patch as-is now, since it isn't used on x86?
> 
> Marc, Lorenzo, I assume this actually fixes a bug.  Can we get any
> more details about what happens when you hit the bug, and how you
> reproduced it (what platform, driver, etc.)?

It definitely fixes a bug. This has been found by running a KVM guest
using kvmtool PCI emulation, where the following things happen:

- Guest programs a virtual (bogus) interrupt number in the PCI device
config space (virtio disk in this case)
- kvmtool uses that interrupt number as is, without any other form of
validation
- Either the injection fails (because the interrupt is out of the range
of the virtual interrupt controller) -> virtio PCI device goes dead
- or the injection succeeds because this is a valid interrupt number,
but signals an unrelated peripheral -> virtio PCI device goes dead.

This can be trivially reproduced on any ARM PCI system that requires
legacy interrupts (i.e. no MSI support), and that uses a GIC interrupt
controller. Doing it in a VM is just much more convenient.

Hope this helps,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH] PCI: Fix pcibios_update_irq misuse of irq number

[Russell King - ARM Linux]

On Wed, Jan 28, 2015 at 02:51:23PM +0000, Marc Zyngier wrote:
>  void __weak pcibios_update_irq(struct pci_dev *dev, int irq)
>  {
> -	dev_dbg(&dev->dev, "assigning IRQ %02d\n", irq);
> -	pci_write_config_byte(dev, PCI_INTERRUPT_LINE, irq);
> +	struct irq_data *d;
> +
> +	d = irq_get_irq_data(irq);
> +#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
> +	while (d->parent_data)
> +		d = d->parent_data;
> +#endif
> +	dev_dbg(&dev->dev, "assigning IRQ %02ld\n", d->hwirq);
> +	pci_write_config_byte(dev, PCI_INTERRUPT_LINE, d->hwirq);

I'm really not convinced about this being the correct thing to do.

Let's take an older ARM system, such as a Footbridge based system with a
PCI southbridge.

Such a system has IRQs 0-15 as the PCI southbridge ISA interrupts.  Then
there are four PCI interrupts provided by the on-board Footbridge.

Right now, PCI devices are programmed with the OS specific interrupt
number - eg:

00:06.1 IDE interface: Contaq Microsystems 82c693 (prog-if 80 [Master])
        Flags: medium devsel, IRQ 14
30: 00 00 00 00 00 00 00 00 00 00 00 00 0e 01 00 00

00:06.2 IDE interface: Contaq Microsystems 82c693 (prog-if 00 [])
        Flags: medium devsel, IRQ 15
30: 00 00 00 00 00 00 00 00 00 00 00 00 0f 02 00 00

00:06.3 USB Controller: Contaq Microsystems 82c693 (prog-if 10 [OHCI])
        Flags: medium devsel, IRQ 12
30: 00 00 00 00 00 00 00 00 00 00 00 00 0c 01 00 00

00:07.0 Mass storage controller: Integrated Technology Express, Inc. IT/ITE8212
Dual channel ATA RAID controller (rev 13)
        Flags: bus master, 66MHz, medium devsel, latency 0, IRQ 24
30: 00 00 02 04 80 00 00 00 00 00 00 00 18 01 08 08

00:08.0 Ethernet controller: 3Com Corporation 3c905C-TX/TX-M [Tornado] (rev 74)
        Flags: bus master, medium devsel, latency 32, IRQ 22
30: 00 00 06 04 dc 00 00 00 00 00 00 00 16 01 0a 0a

00:09.0 VGA compatible controller: S3 Inc. 86c775/86c785 [Trio 64V2/DX or /GX] (rev 16) (prog-if 00 [VGA controller])
        Flags: medium devsel, IRQ 21
30: 00 00 00 0c 00 00 00 00 00 00 00 00 15 01 00 00

What your change would mean is that the IRQs currently being programmed
>= 16 would be programmed into with numbers with 16 removed from them.
This means that legacy stuff (eg on the Southbridge which really do signal
via the ISA IRQ controller) end up using the same number range as those
which take PCI specific IRQs.

This surely is not sane.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] PCI: Fix pcibios_update_irq misuse of irq number

[Arnd Bergmann]

On Wednesday 28 January 2015 14:51:23 Marc Zyngier wrote:
>  void __weak pcibios_update_irq(struct pci_dev *dev, int irq)
>  {
> -       dev_dbg(&dev->dev, "assigning IRQ %02d\n", irq);
> -       pci_write_config_byte(dev, PCI_INTERRUPT_LINE, irq);
> +       struct irq_data *d;
> +
> +       d = irq_get_irq_data(irq);
> +#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
> +       while (d->parent_data)
> +               d = d->parent_data;
> +#endif
> +       dev_dbg(&dev->dev, "assigning IRQ %02ld\n", d->hwirq);
> +       pci_write_config_byte(dev, PCI_INTERRUPT_LINE, d->hwirq);
>  }

I'm puzzled by this. Why is it even important what we write into
the config space? Isn't this just an interface between BIOS and
OS for systems that rely on the interrupt numbers to be statically
assigned before boot?

	Arnd

Re: [PATCH] PCI: Fix pcibios_update_irq misuse of irq number

[Marc Zyngier]

Hi Russell,

On 02/02/15 16:33, Russell King - ARM Linux wrote:
> On Wed, Jan 28, 2015 at 02:51:23PM +0000, Marc Zyngier wrote:
>>  void __weak pcibios_update_irq(struct pci_dev *dev, int irq)
>>  {
>> -	dev_dbg(&dev->dev, "assigning IRQ %02d\n", irq);
>> -	pci_write_config_byte(dev, PCI_INTERRUPT_LINE, irq);
>> +	struct irq_data *d;
>> +
>> +	d = irq_get_irq_data(irq);
>> +#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
>> +	while (d->parent_data)
>> +		d = d->parent_data;
>> +#endif
>> +	dev_dbg(&dev->dev, "assigning IRQ %02ld\n", d->hwirq);
>> +	pci_write_config_byte(dev, PCI_INTERRUPT_LINE, d->hwirq);
> 
> I'm really not convinced about this being the correct thing to do.
> 
> Let's take an older ARM system, such as a Footbridge based system with a
> PCI southbridge.
> 
> Such a system has IRQs 0-15 as the PCI southbridge ISA interrupts.  Then
> there are four PCI interrupts provided by the on-board Footbridge.
> 
> Right now, PCI devices are programmed with the OS specific interrupt
> number - eg:
> 
> 00:06.1 IDE interface: Contaq Microsystems 82c693 (prog-if 80 [Master])
>         Flags: medium devsel, IRQ 14
> 30: 00 00 00 00 00 00 00 00 00 00 00 00 0e 01 00 00
> 
> 00:06.2 IDE interface: Contaq Microsystems 82c693 (prog-if 00 [])
>         Flags: medium devsel, IRQ 15
> 30: 00 00 00 00 00 00 00 00 00 00 00 00 0f 02 00 00
> 
> 00:06.3 USB Controller: Contaq Microsystems 82c693 (prog-if 10 [OHCI])
>         Flags: medium devsel, IRQ 12
> 30: 00 00 00 00 00 00 00 00 00 00 00 00 0c 01 00 00
> 
> 00:07.0 Mass storage controller: Integrated Technology Express, Inc. IT/ITE8212
> Dual channel ATA RAID controller (rev 13)
>         Flags: bus master, 66MHz, medium devsel, latency 0, IRQ 24
> 30: 00 00 02 04 80 00 00 00 00 00 00 00 18 01 08 08
> 
> 00:08.0 Ethernet controller: 3Com Corporation 3c905C-TX/TX-M [Tornado] (rev 74)
>         Flags: bus master, medium devsel, latency 32, IRQ 22
> 30: 00 00 06 04 dc 00 00 00 00 00 00 00 16 01 0a 0a
> 
> 00:09.0 VGA compatible controller: S3 Inc. 86c775/86c785 [Trio 64V2/DX or /GX] (rev 16) (prog-if 00 [VGA controller])
>         Flags: medium devsel, IRQ 21
> 30: 00 00 00 0c 00 00 00 00 00 00 00 00 15 01 00 00
> 
> What your change would mean is that the IRQs currently being programmed
>> = 16 would be programmed into with numbers with 16 removed from them.
> This means that legacy stuff (eg on the Southbridge which really do signal
> via the ISA IRQ controller) end up using the same number range as those
> which take PCI specific IRQs.
> 
> This surely is not sane.

I suppose this is ebsa285? I must confess I don't see how to distinguish
the two cases (the GIC case uses a purely virtual number, and the
footbridge case uses something that seems to be physical).

A very easy fix would be to entirely contain this change within #ifdef
CONFIG_IRQ_DOMAIN_HIERARCHY/#endif, but that doesn't fill me with
confidence.

What I don't get is how the hwirq field is set in this case. It probably
isn't very useful (as there is no domain lookup), so I would have hoped
to see irq == hwirq. Obviously, this is not the case. What am I missing?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH] PCI: Fix pcibios_update_irq misuse of irq number

[Russell King - ARM Linux]

On Mon, Feb 02, 2015 at 06:08:17PM +0000, Marc Zyngier wrote:
> Hi Russell,
> 
> On 02/02/15 16:33, Russell King - ARM Linux wrote:
> > What your change would mean is that the IRQs currently being programmed
> >> = 16 would be programmed into with numbers with 16 removed from them.
> > This means that legacy stuff (eg on the Southbridge which really do signal
> > via the ISA IRQ controller) end up using the same number range as those
> > which take PCI specific IRQs.
> > 
> > This surely is not sane.
> 
> I suppose this is ebsa285? I must confess I don't see how to distinguish
> the two cases (the GIC case uses a purely virtual number, and the
> footbridge case uses something that seems to be physical).

Yep.

> A very easy fix would be to entirely contain this change within #ifdef
> CONFIG_IRQ_DOMAIN_HIERARCHY/#endif, but that doesn't fill me with
> confidence.
> 
> What I don't get is how the hwirq field is set in this case. It probably
> isn't very useful (as there is no domain lookup), so I would have hoped
> to see irq == hwirq. Obviously, this is not the case. What am I missing?

Well, it depends how this register is to be interpreted.  The PCI specs
say that it's used to communicate the interrupt line routing information
from POST to device drivers and operating systems.  "The value in this
register tells which input of the system interrupt controller(s) the
device's interrupt pin is connected to."  Note the plural there - which
imples that any per-interrupt controller numbering scheme is quite
certainly wrong.

It also implies that there is a known, shared numberspace between POST and
OS implementations on a platform which is used by PCI devices to describe
how the PCI interrupts are wired up.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] PCI: Fix pcibios_update_irq misuse of irq number

[Marc Zyngier]

On 02/02/15 17:02, Arnd Bergmann wrote:
> On Wednesday 28 January 2015 14:51:23 Marc Zyngier wrote:
>>  void __weak pcibios_update_irq(struct pci_dev *dev, int irq)
>>  {
>> -       dev_dbg(&dev->dev, "assigning IRQ %02d\n", irq);
>> -       pci_write_config_byte(dev, PCI_INTERRUPT_LINE, irq);
>> +       struct irq_data *d;
>> +
>> +       d = irq_get_irq_data(irq);
>> +#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
>> +       while (d->parent_data)
>> +               d = d->parent_data;
>> +#endif
>> +       dev_dbg(&dev->dev, "assigning IRQ %02ld\n", d->hwirq);
>> +       pci_write_config_byte(dev, PCI_INTERRUPT_LINE, d->hwirq);
>>  }
> 
> I'm puzzled by this. Why is it even important what we write into
> the config space? Isn't this just an interface between BIOS and
> OS for systems that rely on the interrupt numbers to be statically
> assigned before boot?

That's exactly what I thought until Lorenzo reported kvmtool falling
over because of this write. Obviously, some platforms must actually
require this (possibly for bridges that are not known by the firmware).

Entirely removing that code solves my problem too, but that'd cannot be
the right solution...

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH] PCI: Fix pcibios_update_irq misuse of irq number

[Arnd Bergmann]

On Tuesday 03 February 2015 10:38:25 Marc Zyngier wrote:
> 
> That's exactly what I thought until Lorenzo reported kvmtool falling
> over because of this write. Obviously, some platforms must actually
> require this (possibly for bridges that are not known by the firmware).

This sounds much like a bug in kvmtool.
 
> Entirely removing that code solves my problem too, but that'd cannot be
> the right solution...

The comment in pdev_fixup_irq() says 

        /* Always tell the device, so the driver knows what is
           the real IRQ to use; the device does not use it. */

which I read to mean that there are drivers that incorrectly use
'pci_read_config_byte(dev, PCI_INTERRUPT_LINE)' as the number
they pass into request_irq, rather than using dev->irq.
However, this means that your patch is actually wrong, because
what the driver cares about is the virtual irq number (which
request_irq expects), not the number relative to some interrupt
controller.

There is another comment in include/linux/pci.h that states
       /*
        * Instead of touching interrupt line and base address registers
        * directly, use the values stored here. They might be different!
        */
       unsigned int    irq;

so apparently this has been a cause for problems in the past,
and drivers that rely on the number are already broken.

I also checked ancient kernel versions from the 2.1 days when the
code was first added. And indeed at the time drivers used to read
the word, but now none of them use the number for anything real,
they were all fixed during linux-2.2 at the latest.

	Arnd

Re: [PATCH] PCI: Fix pcibios_update_irq misuse of irq number

[Marc Zyngier]

On 03/02/15 11:31, Arnd Bergmann wrote:
> On Tuesday 03 February 2015 10:38:25 Marc Zyngier wrote:
>>
>> That's exactly what I thought until Lorenzo reported kvmtool falling
>> over because of this write. Obviously, some platforms must actually
>> require this (possibly for bridges that are not known by the firmware).
> 
> This sounds much like a bug in kvmtool.

Lorenzo and I just came to a similar conclusion, given that the HW
should never use that information.

>> Entirely removing that code solves my problem too, but that'd cannot be
>> the right solution...
> 
> The comment in pdev_fixup_irq() says 
> 
>         /* Always tell the device, so the driver knows what is
>            the real IRQ to use; the device does not use it. */
> 
> which I read to mean that there are drivers that incorrectly use
> 'pci_read_config_byte(dev, PCI_INTERRUPT_LINE)' as the number
> they pass into request_irq, rather than using dev->irq.
> However, this means that your patch is actually wrong, because
> what the driver cares about is the virtual irq number (which
> request_irq expects), not the number relative to some interrupt
> controller.

Yes, I now realise that. That makes a lot more sense actually, because I
was getting very confused about how the HW should interpret that number.

Side question: In the probe-only case, should we still allow this write
to happen?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH] PCI: Fix pcibios_update_irq misuse of irq number

[Arnd Bergmann]

On Tuesday 03 February 2015 11:37:30 Marc Zyngier wrote:
> Side question: In the probe-only case, should we still allow this write
> to happen?

No, my understanding is that PCI_PROBE_ONLY precisely means that we
do not modify the config space and instead trust what is there to
be sensible.

	Arnd