[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

[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,
> 

[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...

[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

[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

[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...

[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

[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

[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
>

[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
>>

[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 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...

[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.

[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

[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...

[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.

[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

[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

[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]

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...

[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

[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...

[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

[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

[PATCH] kvmtool: don't use PCI config space IRQ line field

[Andre Przywara]

In PCI config space there is an interrupt line field (offset 0x3f),
which is used to initially communicate the IRQ line number from
firmware to the OS. _Hardware_ should never use this information,
as the OS is free to write any information in there.
But kvmtool uses this number when it triggers IRQs in the guest,
which fails starting with Linux 3.19-rc1, where the PCI layer starts
writing the virtual IRQ number in there.

Fix that by storing the IRQ number in a separate field in
struct virtio_pci, which is independent from the PCI config space
and cannot be influenced by the guest.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
Hi,

this fixes the hangs we have seen with kvmtool and PCI in 3.19-rc1+.
I scanned kvmtool's code for further usage of that config space
field, but couldn't find anything relevant except pci-shmem.c, which
is completely broken atm, so I didn't bother to fix this.

Cheers,
Andre.

 tools/kvm/include/kvm/virtio-pci.h |    8 ++++++++
 tools/kvm/virtio/pci.c             |    9 ++++++---
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/tools/kvm/include/kvm/virtio-pci.h b/tools/kvm/include/kvm/virtio-pci.h
index c795ce7..b70cadd 100644
--- a/tools/kvm/include/kvm/virtio-pci.h
+++ b/tools/kvm/include/kvm/virtio-pci.h
@@ -30,6 +30,14 @@ struct virtio_pci {
 	u8			isr;
 	u32			features;
 
+	/*
+	 * We cannot rely on the INTERRUPT_LINE byte in the config space once
+	 * we have run guest code, as the OS is allowed to use that field
+	 * as a scratch pad to communicate between driver and PCI layer.
+	 * So store our legacy interrupt line number in here for internal use.
+	 */
+	u8			legacy_irq_line;
+
 	/* MSI-X */
 	u16			config_vector;
 	u32			config_gsi;
diff --git a/tools/kvm/virtio/pci.c b/tools/kvm/virtio/pci.c
index 7556239..e17e5a9 100644
--- a/tools/kvm/virtio/pci.c
+++ b/tools/kvm/virtio/pci.c
@@ -141,7 +141,7 @@ static bool virtio_pci__io_in(struct ioport *ioport, struct kvm_cpu *vcpu, u16 p
 		break;
 	case VIRTIO_PCI_ISR:
 		ioport__write8(data, vpci->isr);
-		kvm__irq_line(kvm, vpci->pci_hdr.irq_line, VIRTIO_IRQ_LOW);
+		kvm__irq_line(kvm, vpci->legacy_irq_line, VIRTIO_IRQ_LOW);
 		vpci->isr = VIRTIO_IRQ_LOW;
 		break;
 	default:
@@ -299,7 +299,7 @@ int virtio_pci__signal_vq(struct kvm *kvm, struct virtio_device *vdev, u32 vq)
 			kvm__irq_trigger(kvm, vpci->gsis[vq]);
 	} else {
 		vpci->isr = VIRTIO_IRQ_HIGH;
-		kvm__irq_trigger(kvm, vpci->pci_hdr.irq_line);
+		kvm__irq_trigger(kvm, vpci->legacy_irq_line);
 	}
 	return 0;
 }
@@ -323,7 +323,7 @@ int virtio_pci__signal_config(struct kvm *kvm, struct virtio_device *vdev)
 			kvm__irq_trigger(kvm, vpci->config_gsi);
 	} else {
 		vpci->isr = VIRTIO_PCI_ISR_CONFIG;
-		kvm__irq_trigger(kvm, vpci->pci_hdr.irq_line);
+		kvm__irq_trigger(kvm, vpci->legacy_irq_line);
 	}
 
 	return 0;
@@ -422,6 +422,9 @@ int virtio_pci__init(struct kvm *kvm, void *dev, struct virtio_device *vdev,
 	if (r < 0)
 		goto free_msix_mmio;
 
+	/* save the IRQ that device__register() has allocated */
+	vpci->legacy_irq_line = vpci->pci_hdr.irq_line;
+
 	return 0;
 
 free_msix_mmio:
-- 
1.7.9.5

[PATCH] kvmtool: don't use PCI config space IRQ line field

[Andre Przywara]

In PCI config space there is an interrupt line field (offset 0x3f),
which is used to initially communicate the IRQ line number from
firmware to the OS. _Hardware_ should never use this information,
as the OS is free to write any information in there.
But kvmtool uses this number when it triggers IRQs in the guest,
which fails starting with Linux 3.19-rc1, where the PCI layer starts
writing the virtual IRQ number in there.

Fix that by storing the IRQ number in a separate field in
struct virtio_pci, which is independent from the PCI config space
and cannot be influenced by the guest.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
Hi,

this fixes the hangs we have seen with kvmtool and PCI in 3.19-rc1+.
I scanned kvmtool's code for further usage of that config space
field, but couldn't find anything relevant except pci-shmem.c, which
is completely broken atm, so I didn't bother to fix this.

Cheers,
Andre.

 tools/kvm/include/kvm/virtio-pci.h |    8 ++++++++
 tools/kvm/virtio/pci.c             |    9 ++++++---
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/tools/kvm/include/kvm/virtio-pci.h b/tools/kvm/include/kvm/virtio-pci.h
index c795ce7..b70cadd 100644
--- a/tools/kvm/include/kvm/virtio-pci.h
+++ b/tools/kvm/include/kvm/virtio-pci.h
@@ -30,6 +30,14 @@ struct virtio_pci {
 	u8			isr;
 	u32			features;
 
+	/*
+	 * We cannot rely on the INTERRUPT_LINE byte in the config space once
+	 * we have run guest code, as the OS is allowed to use that field
+	 * as a scratch pad to communicate between driver and PCI layer.
+	 * So store our legacy interrupt line number in here for internal use.
+	 */
+	u8			legacy_irq_line;
+
 	/* MSI-X */
 	u16			config_vector;
 	u32			config_gsi;
diff --git a/tools/kvm/virtio/pci.c b/tools/kvm/virtio/pci.c
index 7556239..e17e5a9 100644
--- a/tools/kvm/virtio/pci.c
+++ b/tools/kvm/virtio/pci.c
@@ -141,7 +141,7 @@ static bool virtio_pci__io_in(struct ioport *ioport, struct kvm_cpu *vcpu, u16 p
 		break;
 	case VIRTIO_PCI_ISR:
 		ioport__write8(data, vpci->isr);
-		kvm__irq_line(kvm, vpci->pci_hdr.irq_line, VIRTIO_IRQ_LOW);
+		kvm__irq_line(kvm, vpci->legacy_irq_line, VIRTIO_IRQ_LOW);
 		vpci->isr = VIRTIO_IRQ_LOW;
 		break;
 	default:
@@ -299,7 +299,7 @@ int virtio_pci__signal_vq(struct kvm *kvm, struct virtio_device *vdev, u32 vq)
 			kvm__irq_trigger(kvm, vpci->gsis[vq]);
 	} else {
 		vpci->isr = VIRTIO_IRQ_HIGH;
-		kvm__irq_trigger(kvm, vpci->pci_hdr.irq_line);
+		kvm__irq_trigger(kvm, vpci->legacy_irq_line);
 	}
 	return 0;
 }
@@ -323,7 +323,7 @@ int virtio_pci__signal_config(struct kvm *kvm, struct virtio_device *vdev)
 			kvm__irq_trigger(kvm, vpci->config_gsi);
 	} else {
 		vpci->isr = VIRTIO_PCI_ISR_CONFIG;
-		kvm__irq_trigger(kvm, vpci->pci_hdr.irq_line);
+		kvm__irq_trigger(kvm, vpci->legacy_irq_line);
 	}
 
 	return 0;
@@ -422,6 +422,9 @@ int virtio_pci__init(struct kvm *kvm, void *dev, struct virtio_device *vdev,
 	if (r < 0)
 		goto free_msix_mmio;
 
+	/* save the IRQ that device__register() has allocated */
+	vpci->legacy_irq_line = vpci->pci_hdr.irq_line;
+
 	return 0;
 
 free_msix_mmio:
-- 
1.7.9.5

[PATCH] kvmtool: don't use PCI config space IRQ line field

[Will Deacon]

On Wed, Feb 04, 2015 at 03:39:50PM +0000, Andre Przywara wrote:
> In PCI config space there is an interrupt line field (offset 0x3f),
> which is used to initially communicate the IRQ line number from
> firmware to the OS. _Hardware_ should never use this information,
> as the OS is free to write any information in there.

Is this true even with probe-only? I appreciate that this isn't a BAR,
but it still feels odd for Linux to write this in that case.

Will

Re: [PATCH] kvmtool: don't use PCI config space IRQ line field

[Will Deacon]

On Wed, Feb 04, 2015 at 03:39:50PM +0000, Andre Przywara wrote:
> In PCI config space there is an interrupt line field (offset 0x3f),
> which is used to initially communicate the IRQ line number from
> firmware to the OS. _Hardware_ should never use this information,
> as the OS is free to write any information in there.

Is this true even with probe-only? I appreciate that this isn't a BAR,
but it still feels odd for Linux to write this in that case.

Will

[PATCH] kvmtool: don't use PCI config space IRQ line field

[Peter Maydell]

On 6 February 2015 at 18:55, Will Deacon <will.deacon@arm.com> wrote:
> On Wed, Feb 04, 2015 at 03:39:50PM +0000, Andre Przywara wrote:
>> In PCI config space there is an interrupt line field (offset 0x3f),
>> which is used to initially communicate the IRQ line number from
>> firmware to the OS. _Hardware_ should never use this information,
>> as the OS is free to write any information in there.
>
> Is this true even with probe-only? I appreciate that this isn't a BAR,
> but it still feels odd for Linux to write this in that case.

The hardware (model) shouldn't be doing anything with the value
in this register anyway, so I think this change to kvmtool is
correct regardless of Linux's behaviour.

-- PMM

Re: [PATCH] kvmtool: don't use PCI config space IRQ line field

[Peter Maydell]

On 6 February 2015 at 18:55, Will Deacon <will.deacon@arm.com> wrote:
> On Wed, Feb 04, 2015 at 03:39:50PM +0000, Andre Przywara wrote:
>> In PCI config space there is an interrupt line field (offset 0x3f),
>> which is used to initially communicate the IRQ line number from
>> firmware to the OS. _Hardware_ should never use this information,
>> as the OS is free to write any information in there.
>
> Is this true even with probe-only? I appreciate that this isn't a BAR,
> but it still feels odd for Linux to write this in that case.

The hardware (model) shouldn't be doing anything with the value
in this register anyway, so I think this change to kvmtool is
correct regardless of Linux's behaviour.

-- PMM

[PATCH] kvmtool: don't use PCI config space IRQ line field

[Will Deacon]

On Fri, Feb 06, 2015 at 07:02:25PM +0000, Peter Maydell wrote:
> On 6 February 2015 at 18:55, Will Deacon <will.deacon@arm.com> wrote:
> > On Wed, Feb 04, 2015 at 03:39:50PM +0000, Andre Przywara wrote:
> >> In PCI config space there is an interrupt line field (offset 0x3f),
> >> which is used to initially communicate the IRQ line number from
> >> firmware to the OS. _Hardware_ should never use this information,
> >> as the OS is free to write any information in there.
> >
> > Is this true even with probe-only? I appreciate that this isn't a BAR,
> > but it still feels odd for Linux to write this in that case.
> 
> The hardware (model) shouldn't be doing anything with the value
> in this register anyway, so I think this change to kvmtool is
> correct regardless of Linux's behaviour.

Well, kvmtool is also pretending to be firmware in this case, which is why
it passes things like probe-only and PSCI nodes.

Will

Re: [PATCH] kvmtool: don't use PCI config space IRQ line field

[Will Deacon]

On Fri, Feb 06, 2015 at 07:02:25PM +0000, Peter Maydell wrote:
> On 6 February 2015 at 18:55, Will Deacon <will.deacon@arm.com> wrote:
> > On Wed, Feb 04, 2015 at 03:39:50PM +0000, Andre Przywara wrote:
> >> In PCI config space there is an interrupt line field (offset 0x3f),
> >> which is used to initially communicate the IRQ line number from
> >> firmware to the OS. _Hardware_ should never use this information,
> >> as the OS is free to write any information in there.
> >
> > Is this true even with probe-only? I appreciate that this isn't a BAR,
> > but it still feels odd for Linux to write this in that case.
> 
> The hardware (model) shouldn't be doing anything with the value
> in this register anyway, so I think this change to kvmtool is
> correct regardless of Linux's behaviour.

Well, kvmtool is also pretending to be firmware in this case, which is why
it passes things like probe-only and PSCI nodes.

Will

[PATCH] kvmtool: don't use PCI config space IRQ line field

[arnd at arndb.de]

> Will Deacon <will.deacon@arm.com> hat am 6. Februar 2015 um 20:07 geschrieben:
> On Fri, Feb 06, 2015 at 07:02:25PM +0000, Peter Maydell wrote:
> > On 6 February 2015 at 18:55, Will Deacon <will.deacon@arm.com> wrote:
> > > On Wed, Feb 04, 2015 at 03:39:50PM +0000, Andre Przywara wrote:
> > >> In PCI config space there is an interrupt line field (offset 0x3f),
> > >> which is used to initially communicate the IRQ line number from
> > >> firmware to the OS. _Hardware_ should never use this information,
> > >> as the OS is free to write any information in there.
> > >
> > > Is this true even with probe-only? I appreciate that this isn't a BAR,
> > > but it still feels odd for Linux to write this in that case.
> >
> > The hardware (model) shouldn't be doing anything with the value
> > in this register anyway, so I think this change to kvmtool is
> > correct regardless of Linux's behaviour.
>
> Well, kvmtool is also pretending to be firmware in this case, which is why
> it passes things like probe-only and PSCI nodes.

And this means that we should expect kvmtool to initialize these fields to
a meaningful value, but not care if the OS writes something else to them.

An interesting question is what value kvmtool should write in there. IIRC,
SBSA says that the each interrupt line on the PCI should be an SPI of the
primary GIC, so I suppose we could write the SPI number, although that would
be different of the traditional Linux interrupt number user for that SPI,
which has an offset added to it.

Of course for any hardware that is not SBSA compliant in this regard and
connects the interrupt line to a secondary irqchip (e.g. gpio), there is
no good 8-bit number we can write in here. Also, Linux does not care,
because it gets the number from DT rather than the PCI config space.

     Arnd

Re: [PATCH] kvmtool: don't use PCI config space IRQ line field

[arnd]

> Will Deacon <will.deacon@arm.com> hat am 6. Februar 2015 um 20:07 geschrieben:
> On Fri, Feb 06, 2015 at 07:02:25PM +0000, Peter Maydell wrote:
> > On 6 February 2015 at 18:55, Will Deacon <will.deacon@arm.com> wrote:
> > > On Wed, Feb 04, 2015 at 03:39:50PM +0000, Andre Przywara wrote:
> > >> In PCI config space there is an interrupt line field (offset 0x3f),
> > >> which is used to initially communicate the IRQ line number from
> > >> firmware to the OS. _Hardware_ should never use this information,
> > >> as the OS is free to write any information in there.
> > >
> > > Is this true even with probe-only? I appreciate that this isn't a BAR,
> > > but it still feels odd for Linux to write this in that case.
> >
> > The hardware (model) shouldn't be doing anything with the value
> > in this register anyway, so I think this change to kvmtool is
> > correct regardless of Linux's behaviour.
>
> Well, kvmtool is also pretending to be firmware in this case, which is why
> it passes things like probe-only and PSCI nodes.

And this means that we should expect kvmtool to initialize these fields to
a meaningful value, but not care if the OS writes something else to them.

An interesting question is what value kvmtool should write in there. IIRC,
SBSA says that the each interrupt line on the PCI should be an SPI of the
primary GIC, so I suppose we could write the SPI number, although that would
be different of the traditional Linux interrupt number user for that SPI,
which has an offset added to it.

Of course for any hardware that is not SBSA compliant in this regard and
connects the interrupt line to a secondary irqchip (e.g. gpio), there is
no good 8-bit number we can write in here. Also, Linux does not care,
because it gets the number from DT rather than the PCI config space.

     Arnd

[PATCH] kvmtool: don't use PCI config space IRQ line field

[Andre Przywara]

In PCI config space there is an interrupt line field (offset 0x3f),
which is used to initially communicate the IRQ line number from
firmware to the OS. _Hardware_ should never use this information,
as the OS is free to write any information in there.
But kvmtool uses this number when it triggers IRQs in the guest,
which fails starting with Linux 3.19-rc1, where the PCI layer starts
writing the virtual IRQ number in there.

Fix that by storing the IRQ number in a separate field in
struct virtio_pci, which is independent from the PCI config space
and cannot be influenced by the guest.
This fixes ARM/ARM64 guests using PCI with newer kernels.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 include/kvm/virtio-pci.h | 8 ++++++++
 virtio/pci.c             | 9 ++++++---
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/include/kvm/virtio-pci.h b/include/kvm/virtio-pci.h
index c795ce7..b70cadd 100644
--- a/include/kvm/virtio-pci.h
+++ b/include/kvm/virtio-pci.h
@@ -30,6 +30,14 @@ struct virtio_pci {
 	u8			isr;
 	u32			features;
 
+	/*
+	 * We cannot rely on the INTERRUPT_LINE byte in the config space once
+	 * we have run guest code, as the OS is allowed to use that field
+	 * as a scratch pad to communicate between driver and PCI layer.
+	 * So store our legacy interrupt line number in here for internal use.
+	 */
+	u8			legacy_irq_line;
+
 	/* MSI-X */
 	u16			config_vector;
 	u32			config_gsi;
diff --git a/virtio/pci.c b/virtio/pci.c
index 7556239..e17e5a9 100644
--- a/virtio/pci.c
+++ b/virtio/pci.c
@@ -141,7 +141,7 @@ static bool virtio_pci__io_in(struct ioport *ioport, struct kvm_cpu *vcpu, u16 p
 		break;
 	case VIRTIO_PCI_ISR:
 		ioport__write8(data, vpci->isr);
-		kvm__irq_line(kvm, vpci->pci_hdr.irq_line, VIRTIO_IRQ_LOW);
+		kvm__irq_line(kvm, vpci->legacy_irq_line, VIRTIO_IRQ_LOW);
 		vpci->isr = VIRTIO_IRQ_LOW;
 		break;
 	default:
@@ -299,7 +299,7 @@ int virtio_pci__signal_vq(struct kvm *kvm, struct virtio_device *vdev, u32 vq)
 			kvm__irq_trigger(kvm, vpci->gsis[vq]);
 	} else {
 		vpci->isr = VIRTIO_IRQ_HIGH;
-		kvm__irq_trigger(kvm, vpci->pci_hdr.irq_line);
+		kvm__irq_trigger(kvm, vpci->legacy_irq_line);
 	}
 	return 0;
 }
@@ -323,7 +323,7 @@ int virtio_pci__signal_config(struct kvm *kvm, struct virtio_device *vdev)
 			kvm__irq_trigger(kvm, vpci->config_gsi);
 	} else {
 		vpci->isr = VIRTIO_PCI_ISR_CONFIG;
-		kvm__irq_trigger(kvm, vpci->pci_hdr.irq_line);
+		kvm__irq_trigger(kvm, vpci->legacy_irq_line);
 	}
 
 	return 0;
@@ -422,6 +422,9 @@ int virtio_pci__init(struct kvm *kvm, void *dev, struct virtio_device *vdev,
 	if (r < 0)
 		goto free_msix_mmio;
 
+	/* save the IRQ that device__register() has allocated */
+	vpci->legacy_irq_line = vpci->pci_hdr.irq_line;
+
 	return 0;
 
 free_msix_mmio:
-- 
2.3.5

[PATCH] kvmtool: don't use PCI config space IRQ line field

[Andre Przywara]

In PCI config space there is an interrupt line field (offset 0x3f),
which is used to initially communicate the IRQ line number from
firmware to the OS. _Hardware_ should never use this information,
as the OS is free to write any information in there.
But kvmtool uses this number when it triggers IRQs in the guest,
which fails starting with Linux 3.19-rc1, where the PCI layer starts
writing the virtual IRQ number in there.

Fix that by storing the IRQ number in a separate field in
struct virtio_pci, which is independent from the PCI config space
and cannot be influenced by the guest.
This fixes ARM/ARM64 guests using PCI with newer kernels.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 include/kvm/virtio-pci.h | 8 ++++++++
 virtio/pci.c             | 9 ++++++---
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/include/kvm/virtio-pci.h b/include/kvm/virtio-pci.h
index c795ce7..b70cadd 100644
--- a/include/kvm/virtio-pci.h
+++ b/include/kvm/virtio-pci.h
@@ -30,6 +30,14 @@ struct virtio_pci {
 	u8			isr;
 	u32			features;
 
+	/*
+	 * We cannot rely on the INTERRUPT_LINE byte in the config space once
+	 * we have run guest code, as the OS is allowed to use that field
+	 * as a scratch pad to communicate between driver and PCI layer.
+	 * So store our legacy interrupt line number in here for internal use.
+	 */
+	u8			legacy_irq_line;
+
 	/* MSI-X */
 	u16			config_vector;
 	u32			config_gsi;
diff --git a/virtio/pci.c b/virtio/pci.c
index 7556239..e17e5a9 100644
--- a/virtio/pci.c
+++ b/virtio/pci.c
@@ -141,7 +141,7 @@ static bool virtio_pci__io_in(struct ioport *ioport, struct kvm_cpu *vcpu, u16 p
 		break;
 	case VIRTIO_PCI_ISR:
 		ioport__write8(data, vpci->isr);
-		kvm__irq_line(kvm, vpci->pci_hdr.irq_line, VIRTIO_IRQ_LOW);
+		kvm__irq_line(kvm, vpci->legacy_irq_line, VIRTIO_IRQ_LOW);
 		vpci->isr = VIRTIO_IRQ_LOW;
 		break;
 	default:
@@ -299,7 +299,7 @@ int virtio_pci__signal_vq(struct kvm *kvm, struct virtio_device *vdev, u32 vq)
 			kvm__irq_trigger(kvm, vpci->gsis[vq]);
 	} else {
 		vpci->isr = VIRTIO_IRQ_HIGH;
-		kvm__irq_trigger(kvm, vpci->pci_hdr.irq_line);
+		kvm__irq_trigger(kvm, vpci->legacy_irq_line);
 	}
 	return 0;
 }
@@ -323,7 +323,7 @@ int virtio_pci__signal_config(struct kvm *kvm, struct virtio_device *vdev)
 			kvm__irq_trigger(kvm, vpci->config_gsi);
 	} else {
 		vpci->isr = VIRTIO_PCI_ISR_CONFIG;
-		kvm__irq_trigger(kvm, vpci->pci_hdr.irq_line);
+		kvm__irq_trigger(kvm, vpci->legacy_irq_line);
 	}
 
 	return 0;
@@ -422,6 +422,9 @@ int virtio_pci__init(struct kvm *kvm, void *dev, struct virtio_device *vdev,
 	if (r < 0)
 		goto free_msix_mmio;
 
+	/* save the IRQ that device__register() has allocated */
+	vpci->legacy_irq_line = vpci->pci_hdr.irq_line;
+
 	return 0;
 
 free_msix_mmio:
-- 
2.3.5

Re: [PATCH] kvmtool: don't use PCI config space IRQ line field

[Will Deacon]

On Thu, Jun 04, 2015 at 04:20:45PM +0100, Andre Przywara wrote:
> In PCI config space there is an interrupt line field (offset 0x3f),
> which is used to initially communicate the IRQ line number from
> firmware to the OS. _Hardware_ should never use this information,
> as the OS is free to write any information in there.
> But kvmtool uses this number when it triggers IRQs in the guest,
> which fails starting with Linux 3.19-rc1, where the PCI layer starts
> writing the virtual IRQ number in there.
> 
> Fix that by storing the IRQ number in a separate field in
> struct virtio_pci, which is independent from the PCI config space
> and cannot be influenced by the guest.
> This fixes ARM/ARM64 guests using PCI with newer kernels.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  include/kvm/virtio-pci.h | 8 ++++++++
>  virtio/pci.c             | 9 ++++++---
>  2 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/include/kvm/virtio-pci.h b/include/kvm/virtio-pci.h
> index c795ce7..b70cadd 100644
> --- a/include/kvm/virtio-pci.h
> +++ b/include/kvm/virtio-pci.h
> @@ -30,6 +30,14 @@ struct virtio_pci {
>  	u8			isr;
>  	u32			features;
>  
> +	/*
> +	 * We cannot rely on the INTERRUPT_LINE byte in the config space once
> +	 * we have run guest code, as the OS is allowed to use that field
> +	 * as a scratch pad to communicate between driver and PCI layer.
> +	 * So store our legacy interrupt line number in here for internal use.
> +	 */
> +	u8			legacy_irq_line;
> +
>  	/* MSI-X */
>  	u16			config_vector;
>  	u32			config_gsi;
> diff --git a/virtio/pci.c b/virtio/pci.c
> index 7556239..e17e5a9 100644
> --- a/virtio/pci.c
> +++ b/virtio/pci.c
> @@ -141,7 +141,7 @@ static bool virtio_pci__io_in(struct ioport *ioport, struct kvm_cpu *vcpu, u16 p
>  		break;
>  	case VIRTIO_PCI_ISR:
>  		ioport__write8(data, vpci->isr);
> -		kvm__irq_line(kvm, vpci->pci_hdr.irq_line, VIRTIO_IRQ_LOW);
> +		kvm__irq_line(kvm, vpci->legacy_irq_line, VIRTIO_IRQ_LOW);
>  		vpci->isr = VIRTIO_IRQ_LOW;
>  		break;
>  	default:
> @@ -299,7 +299,7 @@ int virtio_pci__signal_vq(struct kvm *kvm, struct virtio_device *vdev, u32 vq)
>  			kvm__irq_trigger(kvm, vpci->gsis[vq]);
>  	} else {
>  		vpci->isr = VIRTIO_IRQ_HIGH;
> -		kvm__irq_trigger(kvm, vpci->pci_hdr.irq_line);
> +		kvm__irq_trigger(kvm, vpci->legacy_irq_line);
>  	}
>  	return 0;
>  }
> @@ -323,7 +323,7 @@ int virtio_pci__signal_config(struct kvm *kvm, struct virtio_device *vdev)
>  			kvm__irq_trigger(kvm, vpci->config_gsi);
>  	} else {
>  		vpci->isr = VIRTIO_PCI_ISR_CONFIG;
> -		kvm__irq_trigger(kvm, vpci->pci_hdr.irq_line);
> +		kvm__irq_trigger(kvm, vpci->legacy_irq_line);
>  	}
>  
>  	return 0;
> @@ -422,6 +422,9 @@ int virtio_pci__init(struct kvm *kvm, void *dev, struct virtio_device *vdev,
>  	if (r < 0)
>  		goto free_msix_mmio;
>  
> +	/* save the IRQ that device__register() has allocated */
> +	vpci->legacy_irq_line = vpci->pci_hdr.irq_line;

I'd rather we used the container_of trick that we do for virtio-mmio
devices when assigning the irq in device__register. Then we can avoid
this line completely.

Will

[PATCH] kvmtool: don't use PCI config space IRQ line field

[Will Deacon]

On Thu, Jun 04, 2015 at 04:20:45PM +0100, Andre Przywara wrote:
> In PCI config space there is an interrupt line field (offset 0x3f),
> which is used to initially communicate the IRQ line number from
> firmware to the OS. _Hardware_ should never use this information,
> as the OS is free to write any information in there.
> But kvmtool uses this number when it triggers IRQs in the guest,
> which fails starting with Linux 3.19-rc1, where the PCI layer starts
> writing the virtual IRQ number in there.
> 
> Fix that by storing the IRQ number in a separate field in
> struct virtio_pci, which is independent from the PCI config space
> and cannot be influenced by the guest.
> This fixes ARM/ARM64 guests using PCI with newer kernels.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  include/kvm/virtio-pci.h | 8 ++++++++
>  virtio/pci.c             | 9 ++++++---
>  2 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/include/kvm/virtio-pci.h b/include/kvm/virtio-pci.h
> index c795ce7..b70cadd 100644
> --- a/include/kvm/virtio-pci.h
> +++ b/include/kvm/virtio-pci.h
> @@ -30,6 +30,14 @@ struct virtio_pci {
>  	u8			isr;
>  	u32			features;
>  
> +	/*
> +	 * We cannot rely on the INTERRUPT_LINE byte in the config space once
> +	 * we have run guest code, as the OS is allowed to use that field
> +	 * as a scratch pad to communicate between driver and PCI layer.
> +	 * So store our legacy interrupt line number in here for internal use.
> +	 */
> +	u8			legacy_irq_line;
> +
>  	/* MSI-X */
>  	u16			config_vector;
>  	u32			config_gsi;
> diff --git a/virtio/pci.c b/virtio/pci.c
> index 7556239..e17e5a9 100644
> --- a/virtio/pci.c
> +++ b/virtio/pci.c
> @@ -141,7 +141,7 @@ static bool virtio_pci__io_in(struct ioport *ioport, struct kvm_cpu *vcpu, u16 p
>  		break;
>  	case VIRTIO_PCI_ISR:
>  		ioport__write8(data, vpci->isr);
> -		kvm__irq_line(kvm, vpci->pci_hdr.irq_line, VIRTIO_IRQ_LOW);
> +		kvm__irq_line(kvm, vpci->legacy_irq_line, VIRTIO_IRQ_LOW);
>  		vpci->isr = VIRTIO_IRQ_LOW;
>  		break;
>  	default:
> @@ -299,7 +299,7 @@ int virtio_pci__signal_vq(struct kvm *kvm, struct virtio_device *vdev, u32 vq)
>  			kvm__irq_trigger(kvm, vpci->gsis[vq]);
>  	} else {
>  		vpci->isr = VIRTIO_IRQ_HIGH;
> -		kvm__irq_trigger(kvm, vpci->pci_hdr.irq_line);
> +		kvm__irq_trigger(kvm, vpci->legacy_irq_line);
>  	}
>  	return 0;
>  }
> @@ -323,7 +323,7 @@ int virtio_pci__signal_config(struct kvm *kvm, struct virtio_device *vdev)
>  			kvm__irq_trigger(kvm, vpci->config_gsi);
>  	} else {
>  		vpci->isr = VIRTIO_PCI_ISR_CONFIG;
> -		kvm__irq_trigger(kvm, vpci->pci_hdr.irq_line);
> +		kvm__irq_trigger(kvm, vpci->legacy_irq_line);
>  	}
>  
>  	return 0;
> @@ -422,6 +422,9 @@ int virtio_pci__init(struct kvm *kvm, void *dev, struct virtio_device *vdev,
>  	if (r < 0)
>  		goto free_msix_mmio;
>  
> +	/* save the IRQ that device__register() has allocated */
> +	vpci->legacy_irq_line = vpci->pci_hdr.irq_line;

I'd rather we used the container_of trick that we do for virtio-mmio
devices when assigning the irq in device__register. Then we can avoid
this line completely.

Will

Re: [PATCH] kvmtool: don't use PCI config space IRQ line field

[Andre Przywara]

On 06/05/2015 05:41 PM, Will Deacon wrote:
> On Thu, Jun 04, 2015 at 04:20:45PM +0100, Andre Przywara wrote:

Hi Will,

sorry, almost forgot about this email...

>> In PCI config space there is an interrupt line field (offset 0x3f),
>> which is used to initially communicate the IRQ line number from
>> firmware to the OS. _Hardware_ should never use this information,
>> as the OS is free to write any information in there.
>> But kvmtool uses this number when it triggers IRQs in the guest,
>> which fails starting with Linux 3.19-rc1, where the PCI layer starts
>> writing the virtual IRQ number in there.
>>
>> Fix that by storing the IRQ number in a separate field in
>> struct virtio_pci, which is independent from the PCI config space
>> and cannot be influenced by the guest.
>> This fixes ARM/ARM64 guests using PCI with newer kernels.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>  include/kvm/virtio-pci.h | 8 ++++++++
>>  virtio/pci.c             | 9 ++++++---
>>  2 files changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/kvm/virtio-pci.h b/include/kvm/virtio-pci.h
>> index c795ce7..b70cadd 100644
>> --- a/include/kvm/virtio-pci.h
>> +++ b/include/kvm/virtio-pci.h
>> @@ -30,6 +30,14 @@ struct virtio_pci {
>>  	u8			isr;
>>  	u32			features;
>>  
>> +	/*
>> +	 * We cannot rely on the INTERRUPT_LINE byte in the config space once
>> +	 * we have run guest code, as the OS is allowed to use that field
>> +	 * as a scratch pad to communicate between driver and PCI layer.
>> +	 * So store our legacy interrupt line number in here for internal use.
>> +	 */
>> +	u8			legacy_irq_line;
>> +
>>  	/* MSI-X */
>>  	u16			config_vector;
>>  	u32			config_gsi;
>> diff --git a/virtio/pci.c b/virtio/pci.c
>> index 7556239..e17e5a9 100644
>> --- a/virtio/pci.c
>> +++ b/virtio/pci.c
>> @@ -141,7 +141,7 @@ static bool virtio_pci__io_in(struct ioport *ioport, struct kvm_cpu *vcpu, u16 p
>>  		break;
>>  	case VIRTIO_PCI_ISR:
>>  		ioport__write8(data, vpci->isr);
>> -		kvm__irq_line(kvm, vpci->pci_hdr.irq_line, VIRTIO_IRQ_LOW);
>> +		kvm__irq_line(kvm, vpci->legacy_irq_line, VIRTIO_IRQ_LOW);
>>  		vpci->isr = VIRTIO_IRQ_LOW;
>>  		break;
>>  	default:
>> @@ -299,7 +299,7 @@ int virtio_pci__signal_vq(struct kvm *kvm, struct virtio_device *vdev, u32 vq)
>>  			kvm__irq_trigger(kvm, vpci->gsis[vq]);
>>  	} else {
>>  		vpci->isr = VIRTIO_IRQ_HIGH;
>> -		kvm__irq_trigger(kvm, vpci->pci_hdr.irq_line);
>> +		kvm__irq_trigger(kvm, vpci->legacy_irq_line);
>>  	}
>>  	return 0;
>>  }
>> @@ -323,7 +323,7 @@ int virtio_pci__signal_config(struct kvm *kvm, struct virtio_device *vdev)
>>  			kvm__irq_trigger(kvm, vpci->config_gsi);
>>  	} else {
>>  		vpci->isr = VIRTIO_PCI_ISR_CONFIG;
>> -		kvm__irq_trigger(kvm, vpci->pci_hdr.irq_line);
>> +		kvm__irq_trigger(kvm, vpci->legacy_irq_line);
>>  	}
>>  
>>  	return 0;
>> @@ -422,6 +422,9 @@ int virtio_pci__init(struct kvm *kvm, void *dev, struct virtio_device *vdev,
>>  	if (r < 0)
>>  		goto free_msix_mmio;
>>  
>> +	/* save the IRQ that device__register() has allocated */
>> +	vpci->legacy_irq_line = vpci->pci_hdr.irq_line;
> 
> I'd rather we used the container_of trick that we do for virtio-mmio
> devices when assigning the irq in device__register. Then we can avoid
> this line completely.

Not completely sure I get what you mean, I take it you want to assign
legacy_irq_line in pci__assign_irq() directly (where the IRQ number is
allocated).
But this function is PCI generic code and is used by the VESA
framebuffer and the shmem device on x86 as well. For those devices
dev_hdr is not part of a struct virtio_pci, so we can't do container_of
to assign the legacy_irq_line here directly.
Admittedly this fix should apply to the other two users as well, but
VESA does not use interrupts and pci-shmem is completely broken anyway,
so I didn't bother to fix it in this regard.
Would it be justified to provide an IRQ number field in struct
device_header to address all users?

Or what am I missing here?

Cheers,
Andre

[PATCH] kvmtool: don't use PCI config space IRQ line field

[Andre Przywara]

On 06/05/2015 05:41 PM, Will Deacon wrote:
> On Thu, Jun 04, 2015 at 04:20:45PM +0100, Andre Przywara wrote:

Hi Will,

sorry, almost forgot about this email...

>> In PCI config space there is an interrupt line field (offset 0x3f),
>> which is used to initially communicate the IRQ line number from
>> firmware to the OS. _Hardware_ should never use this information,
>> as the OS is free to write any information in there.
>> But kvmtool uses this number when it triggers IRQs in the guest,
>> which fails starting with Linux 3.19-rc1, where the PCI layer starts
>> writing the virtual IRQ number in there.
>>
>> Fix that by storing the IRQ number in a separate field in
>> struct virtio_pci, which is independent from the PCI config space
>> and cannot be influenced by the guest.
>> This fixes ARM/ARM64 guests using PCI with newer kernels.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>  include/kvm/virtio-pci.h | 8 ++++++++
>>  virtio/pci.c             | 9 ++++++---
>>  2 files changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/kvm/virtio-pci.h b/include/kvm/virtio-pci.h
>> index c795ce7..b70cadd 100644
>> --- a/include/kvm/virtio-pci.h
>> +++ b/include/kvm/virtio-pci.h
>> @@ -30,6 +30,14 @@ struct virtio_pci {
>>  	u8			isr;
>>  	u32			features;
>>  
>> +	/*
>> +	 * We cannot rely on the INTERRUPT_LINE byte in the config space once
>> +	 * we have run guest code, as the OS is allowed to use that field
>> +	 * as a scratch pad to communicate between driver and PCI layer.
>> +	 * So store our legacy interrupt line number in here for internal use.
>> +	 */
>> +	u8			legacy_irq_line;
>> +
>>  	/* MSI-X */
>>  	u16			config_vector;
>>  	u32			config_gsi;
>> diff --git a/virtio/pci.c b/virtio/pci.c
>> index 7556239..e17e5a9 100644
>> --- a/virtio/pci.c
>> +++ b/virtio/pci.c
>> @@ -141,7 +141,7 @@ static bool virtio_pci__io_in(struct ioport *ioport, struct kvm_cpu *vcpu, u16 p
>>  		break;
>>  	case VIRTIO_PCI_ISR:
>>  		ioport__write8(data, vpci->isr);
>> -		kvm__irq_line(kvm, vpci->pci_hdr.irq_line, VIRTIO_IRQ_LOW);
>> +		kvm__irq_line(kvm, vpci->legacy_irq_line, VIRTIO_IRQ_LOW);
>>  		vpci->isr = VIRTIO_IRQ_LOW;
>>  		break;
>>  	default:
>> @@ -299,7 +299,7 @@ int virtio_pci__signal_vq(struct kvm *kvm, struct virtio_device *vdev, u32 vq)
>>  			kvm__irq_trigger(kvm, vpci->gsis[vq]);
>>  	} else {
>>  		vpci->isr = VIRTIO_IRQ_HIGH;
>> -		kvm__irq_trigger(kvm, vpci->pci_hdr.irq_line);
>> +		kvm__irq_trigger(kvm, vpci->legacy_irq_line);
>>  	}
>>  	return 0;
>>  }
>> @@ -323,7 +323,7 @@ int virtio_pci__signal_config(struct kvm *kvm, struct virtio_device *vdev)
>>  			kvm__irq_trigger(kvm, vpci->config_gsi);
>>  	} else {
>>  		vpci->isr = VIRTIO_PCI_ISR_CONFIG;
>> -		kvm__irq_trigger(kvm, vpci->pci_hdr.irq_line);
>> +		kvm__irq_trigger(kvm, vpci->legacy_irq_line);
>>  	}
>>  
>>  	return 0;
>> @@ -422,6 +422,9 @@ int virtio_pci__init(struct kvm *kvm, void *dev, struct virtio_device *vdev,
>>  	if (r < 0)
>>  		goto free_msix_mmio;
>>  
>> +	/* save the IRQ that device__register() has allocated */
>> +	vpci->legacy_irq_line = vpci->pci_hdr.irq_line;
> 
> I'd rather we used the container_of trick that we do for virtio-mmio
> devices when assigning the irq in device__register. Then we can avoid
> this line completely.

Not completely sure I get what you mean, I take it you want to assign
legacy_irq_line in pci__assign_irq() directly (where the IRQ number is
allocated).
But this function is PCI generic code and is used by the VESA
framebuffer and the shmem device on x86 as well. For those devices
dev_hdr is not part of a struct virtio_pci, so we can't do container_of
to assign the legacy_irq_line here directly.
Admittedly this fix should apply to the other two users as well, but
VESA does not use interrupts and pci-shmem is completely broken anyway,
so I didn't bother to fix it in this regard.
Would it be justified to provide an IRQ number field in struct
device_header to address all users?

Or what am I missing here?

Cheers,
Andre

Re: [PATCH] kvmtool: don't use PCI config space IRQ line field

[Will Deacon]

On Mon, Jun 15, 2015 at 11:45:38AM +0100, Andre Przywara wrote:
> On 06/05/2015 05:41 PM, Will Deacon wrote:
> > On Thu, Jun 04, 2015 at 04:20:45PM +0100, Andre Przywara wrote:
> >> In PCI config space there is an interrupt line field (offset 0x3f),
> >> which is used to initially communicate the IRQ line number from
> >> firmware to the OS. _Hardware_ should never use this information,
> >> as the OS is free to write any information in there.
> >> But kvmtool uses this number when it triggers IRQs in the guest,
> >> which fails starting with Linux 3.19-rc1, where the PCI layer starts
> >> writing the virtual IRQ number in there.
> >>
> >> Fix that by storing the IRQ number in a separate field in
> >> struct virtio_pci, which is independent from the PCI config space
> >> and cannot be influenced by the guest.
> >> This fixes ARM/ARM64 guests using PCI with newer kernels.
> >>
> >> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> >> ---
> >>  include/kvm/virtio-pci.h | 8 ++++++++
> >>  virtio/pci.c             | 9 ++++++---
> >>  2 files changed, 14 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/include/kvm/virtio-pci.h b/include/kvm/virtio-pci.h
> >> index c795ce7..b70cadd 100644
> >> --- a/include/kvm/virtio-pci.h
> >> +++ b/include/kvm/virtio-pci.h
> >> @@ -30,6 +30,14 @@ struct virtio_pci {
> >>  	u8			isr;
> >>  	u32			features;
> >>  
> >> +	/*
> >> +	 * We cannot rely on the INTERRUPT_LINE byte in the config space once
> >> +	 * we have run guest code, as the OS is allowed to use that field
> >> +	 * as a scratch pad to communicate between driver and PCI layer.
> >> +	 * So store our legacy interrupt line number in here for internal use.
> >> +	 */
> >> +	u8			legacy_irq_line;
> >> +
> >>  	/* MSI-X */
> >>  	u16			config_vector;
> >>  	u32			config_gsi;
> >> diff --git a/virtio/pci.c b/virtio/pci.c
> >> index 7556239..e17e5a9 100644
> >> --- a/virtio/pci.c
> >> +++ b/virtio/pci.c
> >> @@ -141,7 +141,7 @@ static bool virtio_pci__io_in(struct ioport *ioport, struct kvm_cpu *vcpu, u16 p
> >>  		break;
> >>  	case VIRTIO_PCI_ISR:
> >>  		ioport__write8(data, vpci->isr);
> >> -		kvm__irq_line(kvm, vpci->pci_hdr.irq_line, VIRTIO_IRQ_LOW);
> >> +		kvm__irq_line(kvm, vpci->legacy_irq_line, VIRTIO_IRQ_LOW);
> >>  		vpci->isr = VIRTIO_IRQ_LOW;
> >>  		break;
> >>  	default:
> >> @@ -299,7 +299,7 @@ int virtio_pci__signal_vq(struct kvm *kvm, struct virtio_device *vdev, u32 vq)
> >>  			kvm__irq_trigger(kvm, vpci->gsis[vq]);
> >>  	} else {
> >>  		vpci->isr = VIRTIO_IRQ_HIGH;
> >> -		kvm__irq_trigger(kvm, vpci->pci_hdr.irq_line);
> >> +		kvm__irq_trigger(kvm, vpci->legacy_irq_line);
> >>  	}
> >>  	return 0;
> >>  }
> >> @@ -323,7 +323,7 @@ int virtio_pci__signal_config(struct kvm *kvm, struct virtio_device *vdev)
> >>  			kvm__irq_trigger(kvm, vpci->config_gsi);
> >>  	} else {
> >>  		vpci->isr = VIRTIO_PCI_ISR_CONFIG;
> >> -		kvm__irq_trigger(kvm, vpci->pci_hdr.irq_line);
> >> +		kvm__irq_trigger(kvm, vpci->legacy_irq_line);
> >>  	}
> >>  
> >>  	return 0;
> >> @@ -422,6 +422,9 @@ int virtio_pci__init(struct kvm *kvm, void *dev, struct virtio_device *vdev,
> >>  	if (r < 0)
> >>  		goto free_msix_mmio;
> >>  
> >> +	/* save the IRQ that device__register() has allocated */
> >> +	vpci->legacy_irq_line = vpci->pci_hdr.irq_line;
> > 
> > I'd rather we used the container_of trick that we do for virtio-mmio
> > devices when assigning the irq in device__register. Then we can avoid
> > this line completely.
> 
> Not completely sure I get what you mean, I take it you want to assign
> legacy_irq_line in pci__assign_irq() directly (where the IRQ number is
> allocated).
> But this function is PCI generic code and is used by the VESA
> framebuffer and the shmem device on x86 as well. For those devices
> dev_hdr is not part of a struct virtio_pci, so we can't do container_of
> to assign the legacy_irq_line here directly.
> Admittedly this fix should apply to the other two users as well, but
> VESA does not use interrupts and pci-shmem is completely broken anyway,
> so I didn't bother to fix it in this regard.
> Would it be justified to provide an IRQ number field in struct
> device_header to address all users?
> 
> Or what am I missing here?

If VESA and shmem are broken, they should either be fixed or removed.

If you fix them, then we could have separate virtual buses for virtio-pci
and emulated-pci (or whatever you want to call it). We could also have
a separate bus for passthrough-devices too.

However, that's quite a lot of work for a bug-fix, so I guess the easiest
thing is to extend your current hack to cover VESA and shmem too.

Will

[PATCH] kvmtool: don't use PCI config space IRQ line field

[Will Deacon]

On Mon, Jun 15, 2015 at 11:45:38AM +0100, Andre Przywara wrote:
> On 06/05/2015 05:41 PM, Will Deacon wrote:
> > On Thu, Jun 04, 2015 at 04:20:45PM +0100, Andre Przywara wrote:
> >> In PCI config space there is an interrupt line field (offset 0x3f),
> >> which is used to initially communicate the IRQ line number from
> >> firmware to the OS. _Hardware_ should never use this information,
> >> as the OS is free to write any information in there.
> >> But kvmtool uses this number when it triggers IRQs in the guest,
> >> which fails starting with Linux 3.19-rc1, where the PCI layer starts
> >> writing the virtual IRQ number in there.
> >>
> >> Fix that by storing the IRQ number in a separate field in
> >> struct virtio_pci, which is independent from the PCI config space
> >> and cannot be influenced by the guest.
> >> This fixes ARM/ARM64 guests using PCI with newer kernels.
> >>
> >> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> >> ---
> >>  include/kvm/virtio-pci.h | 8 ++++++++
> >>  virtio/pci.c             | 9 ++++++---
> >>  2 files changed, 14 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/include/kvm/virtio-pci.h b/include/kvm/virtio-pci.h
> >> index c795ce7..b70cadd 100644
> >> --- a/include/kvm/virtio-pci.h
> >> +++ b/include/kvm/virtio-pci.h
> >> @@ -30,6 +30,14 @@ struct virtio_pci {
> >>  	u8			isr;
> >>  	u32			features;
> >>  
> >> +	/*
> >> +	 * We cannot rely on the INTERRUPT_LINE byte in the config space once
> >> +	 * we have run guest code, as the OS is allowed to use that field
> >> +	 * as a scratch pad to communicate between driver and PCI layer.
> >> +	 * So store our legacy interrupt line number in here for internal use.
> >> +	 */
> >> +	u8			legacy_irq_line;
> >> +
> >>  	/* MSI-X */
> >>  	u16			config_vector;
> >>  	u32			config_gsi;
> >> diff --git a/virtio/pci.c b/virtio/pci.c
> >> index 7556239..e17e5a9 100644
> >> --- a/virtio/pci.c
> >> +++ b/virtio/pci.c
> >> @@ -141,7 +141,7 @@ static bool virtio_pci__io_in(struct ioport *ioport, struct kvm_cpu *vcpu, u16 p
> >>  		break;
> >>  	case VIRTIO_PCI_ISR:
> >>  		ioport__write8(data, vpci->isr);
> >> -		kvm__irq_line(kvm, vpci->pci_hdr.irq_line, VIRTIO_IRQ_LOW);
> >> +		kvm__irq_line(kvm, vpci->legacy_irq_line, VIRTIO_IRQ_LOW);
> >>  		vpci->isr = VIRTIO_IRQ_LOW;
> >>  		break;
> >>  	default:
> >> @@ -299,7 +299,7 @@ int virtio_pci__signal_vq(struct kvm *kvm, struct virtio_device *vdev, u32 vq)
> >>  			kvm__irq_trigger(kvm, vpci->gsis[vq]);
> >>  	} else {
> >>  		vpci->isr = VIRTIO_IRQ_HIGH;
> >> -		kvm__irq_trigger(kvm, vpci->pci_hdr.irq_line);
> >> +		kvm__irq_trigger(kvm, vpci->legacy_irq_line);
> >>  	}
> >>  	return 0;
> >>  }
> >> @@ -323,7 +323,7 @@ int virtio_pci__signal_config(struct kvm *kvm, struct virtio_device *vdev)
> >>  			kvm__irq_trigger(kvm, vpci->config_gsi);
> >>  	} else {
> >>  		vpci->isr = VIRTIO_PCI_ISR_CONFIG;
> >> -		kvm__irq_trigger(kvm, vpci->pci_hdr.irq_line);
> >> +		kvm__irq_trigger(kvm, vpci->legacy_irq_line);
> >>  	}
> >>  
> >>  	return 0;
> >> @@ -422,6 +422,9 @@ int virtio_pci__init(struct kvm *kvm, void *dev, struct virtio_device *vdev,
> >>  	if (r < 0)
> >>  		goto free_msix_mmio;
> >>  
> >> +	/* save the IRQ that device__register() has allocated */
> >> +	vpci->legacy_irq_line = vpci->pci_hdr.irq_line;
> > 
> > I'd rather we used the container_of trick that we do for virtio-mmio
> > devices when assigning the irq in device__register. Then we can avoid
> > this line completely.
> 
> Not completely sure I get what you mean, I take it you want to assign
> legacy_irq_line in pci__assign_irq() directly (where the IRQ number is
> allocated).
> But this function is PCI generic code and is used by the VESA
> framebuffer and the shmem device on x86 as well. For those devices
> dev_hdr is not part of a struct virtio_pci, so we can't do container_of
> to assign the legacy_irq_line here directly.
> Admittedly this fix should apply to the other two users as well, but
> VESA does not use interrupts and pci-shmem is completely broken anyway,
> so I didn't bother to fix it in this regard.
> Would it be justified to provide an IRQ number field in struct
> device_header to address all users?
> 
> Or what am I missing here?

If VESA and shmem are broken, they should either be fixed or removed.

If you fix them, then we could have separate virtual buses for virtio-pci
and emulated-pci (or whatever you want to call it). We could also have
a separate bus for passthrough-devices too.

However, that's quite a lot of work for a bug-fix, so I guess the easiest
thing is to extend your current hack to cover VESA and shmem too.

Will

Re: [PATCH] kvmtool: don't use PCI config space IRQ line field

[Andre Przywara]

Hi Will,

On 06/16/2015 06:06 PM, Will Deacon wrote:
> On Mon, Jun 15, 2015 at 11:45:38AM +0100, Andre Przywara wrote:
>> On 06/05/2015 05:41 PM, Will Deacon wrote:
>>> On Thu, Jun 04, 2015 at 04:20:45PM +0100, Andre Przywara wrote:
>>>> In PCI config space there is an interrupt line field (offset 0x3f),
>>>> which is used to initially communicate the IRQ line number from
>>>> firmware to the OS. _Hardware_ should never use this information,
>>>> as the OS is free to write any information in there.
>>>> But kvmtool uses this number when it triggers IRQs in the guest,
>>>> which fails starting with Linux 3.19-rc1, where the PCI layer starts
>>>> writing the virtual IRQ number in there.
>>>>
>>>> Fix that by storing the IRQ number in a separate field in
>>>> struct virtio_pci, which is independent from the PCI config space
>>>> and cannot be influenced by the guest.
>>>> This fixes ARM/ARM64 guests using PCI with newer kernels.
>>>>
>>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>>> ---
>>>>  include/kvm/virtio-pci.h | 8 ++++++++
>>>>  virtio/pci.c             | 9 ++++++---
>>>>  2 files changed, 14 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/include/kvm/virtio-pci.h b/include/kvm/virtio-pci.h
>>>> index c795ce7..b70cadd 100644
>>>> --- a/include/kvm/virtio-pci.h
>>>> +++ b/include/kvm/virtio-pci.h
>>>> @@ -30,6 +30,14 @@ struct virtio_pci {
>>>>  	u8			isr;
>>>>  	u32			features;
>>>>  
>>>> +	/*
>>>> +	 * We cannot rely on the INTERRUPT_LINE byte in the config space once
>>>> +	 * we have run guest code, as the OS is allowed to use that field
>>>> +	 * as a scratch pad to communicate between driver and PCI layer.
>>>> +	 * So store our legacy interrupt line number in here for internal use.
>>>> +	 */
>>>> +	u8			legacy_irq_line;
>>>> +
>>>>  	/* MSI-X */
>>>>  	u16			config_vector;
>>>>  	u32			config_gsi;
>>>> diff --git a/virtio/pci.c b/virtio/pci.c
>>>> index 7556239..e17e5a9 100644
>>>> --- a/virtio/pci.c
>>>> +++ b/virtio/pci.c
>>>> @@ -141,7 +141,7 @@ static bool virtio_pci__io_in(struct ioport *ioport, struct kvm_cpu *vcpu, u16 p
>>>>  		break;
>>>>  	case VIRTIO_PCI_ISR:
>>>>  		ioport__write8(data, vpci->isr);
>>>> -		kvm__irq_line(kvm, vpci->pci_hdr.irq_line, VIRTIO_IRQ_LOW);
>>>> +		kvm__irq_line(kvm, vpci->legacy_irq_line, VIRTIO_IRQ_LOW);
>>>>  		vpci->isr = VIRTIO_IRQ_LOW;
>>>>  		break;
>>>>  	default:
>>>> @@ -299,7 +299,7 @@ int virtio_pci__signal_vq(struct kvm *kvm, struct virtio_device *vdev, u32 vq)
>>>>  			kvm__irq_trigger(kvm, vpci->gsis[vq]);
>>>>  	} else {
>>>>  		vpci->isr = VIRTIO_IRQ_HIGH;
>>>> -		kvm__irq_trigger(kvm, vpci->pci_hdr.irq_line);
>>>> +		kvm__irq_trigger(kvm, vpci->legacy_irq_line);
>>>>  	}
>>>>  	return 0;
>>>>  }
>>>> @@ -323,7 +323,7 @@ int virtio_pci__signal_config(struct kvm *kvm, struct virtio_device *vdev)
>>>>  			kvm__irq_trigger(kvm, vpci->config_gsi);
>>>>  	} else {
>>>>  		vpci->isr = VIRTIO_PCI_ISR_CONFIG;
>>>> -		kvm__irq_trigger(kvm, vpci->pci_hdr.irq_line);
>>>> +		kvm__irq_trigger(kvm, vpci->legacy_irq_line);
>>>>  	}
>>>>  
>>>>  	return 0;
>>>> @@ -422,6 +422,9 @@ int virtio_pci__init(struct kvm *kvm, void *dev, struct virtio_device *vdev,
>>>>  	if (r < 0)
>>>>  		goto free_msix_mmio;
>>>>  
>>>> +	/* save the IRQ that device__register() has allocated */
>>>> +	vpci->legacy_irq_line = vpci->pci_hdr.irq_line;
>>>
>>> I'd rather we used the container_of trick that we do for virtio-mmio
>>> devices when assigning the irq in device__register. Then we can avoid
>>> this line completely.
>>
>> Not completely sure I get what you mean, I take it you want to assign
>> legacy_irq_line in pci__assign_irq() directly (where the IRQ number is
>> allocated).
>> But this function is PCI generic code and is used by the VESA
>> framebuffer and the shmem device on x86 as well. For those devices
>> dev_hdr is not part of a struct virtio_pci, so we can't do container_of
>> to assign the legacy_irq_line here directly.
>> Admittedly this fix should apply to the other two users as well, but
>> VESA does not use interrupts and pci-shmem is completely broken anyway,
>> so I didn't bother to fix it in this regard.
>> Would it be justified to provide an IRQ number field in struct
>> device_header to address all users?
>>
>> Or what am I missing here?
> 
> If VESA and shmem are broken, they should either be fixed or removed.

I am tempted to remove shmem, since it's broken:
a) there is no upstream driver, only some out-of-tree uio driver module
in some Github repo
b) the PCI device BARs do not match what QEMU implements and what the
uio driver expects (IO BAR vs. MMIO BAR)
c) there is (at least one) bug in kvmtool (easily fixed, though)
I haven't completely given up yet fixing it, but that's for another
series ;-)

However ...

> 
> If you fix them, then we could have separate virtual buses for virtio-pci
> and emulated-pci (or whatever you want to call it). We could also have
> a separate bus for passthrough-devices too.
> 
> However, that's quite a lot of work for a bug-fix, so I guess the easiest
> thing is to extend your current hack to cover VESA and shmem too.

So looking more closely, shmem does not support legacy interrupts, only
MSIs. VESA does not support any interrupts at all.
So there is nothing to do here, as the only legacy PCI interrupt user is
virtio, which this patch fixes.
Do you insist on that container_of trick you mentioned earlier? If yes,
please elaborate on that, as I don't see how this should work and how it
could possibly be better (pci__assign_irq() is still generic).

Cheers,
Andre.

[PATCH] kvmtool: don't use PCI config space IRQ line field

[Andre Przywara]

Hi Will,

On 06/16/2015 06:06 PM, Will Deacon wrote:
> On Mon, Jun 15, 2015 at 11:45:38AM +0100, Andre Przywara wrote:
>> On 06/05/2015 05:41 PM, Will Deacon wrote:
>>> On Thu, Jun 04, 2015 at 04:20:45PM +0100, Andre Przywara wrote:
>>>> In PCI config space there is an interrupt line field (offset 0x3f),
>>>> which is used to initially communicate the IRQ line number from
>>>> firmware to the OS. _Hardware_ should never use this information,
>>>> as the OS is free to write any information in there.
>>>> But kvmtool uses this number when it triggers IRQs in the guest,
>>>> which fails starting with Linux 3.19-rc1, where the PCI layer starts
>>>> writing the virtual IRQ number in there.
>>>>
>>>> Fix that by storing the IRQ number in a separate field in
>>>> struct virtio_pci, which is independent from the PCI config space
>>>> and cannot be influenced by the guest.
>>>> This fixes ARM/ARM64 guests using PCI with newer kernels.
>>>>
>>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>>> ---
>>>>  include/kvm/virtio-pci.h | 8 ++++++++
>>>>  virtio/pci.c             | 9 ++++++---
>>>>  2 files changed, 14 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/include/kvm/virtio-pci.h b/include/kvm/virtio-pci.h
>>>> index c795ce7..b70cadd 100644
>>>> --- a/include/kvm/virtio-pci.h
>>>> +++ b/include/kvm/virtio-pci.h
>>>> @@ -30,6 +30,14 @@ struct virtio_pci {
>>>>  	u8			isr;
>>>>  	u32			features;
>>>>  
>>>> +	/*
>>>> +	 * We cannot rely on the INTERRUPT_LINE byte in the config space once
>>>> +	 * we have run guest code, as the OS is allowed to use that field
>>>> +	 * as a scratch pad to communicate between driver and PCI layer.
>>>> +	 * So store our legacy interrupt line number in here for internal use.
>>>> +	 */
>>>> +	u8			legacy_irq_line;
>>>> +
>>>>  	/* MSI-X */
>>>>  	u16			config_vector;
>>>>  	u32			config_gsi;
>>>> diff --git a/virtio/pci.c b/virtio/pci.c
>>>> index 7556239..e17e5a9 100644
>>>> --- a/virtio/pci.c
>>>> +++ b/virtio/pci.c
>>>> @@ -141,7 +141,7 @@ static bool virtio_pci__io_in(struct ioport *ioport, struct kvm_cpu *vcpu, u16 p
>>>>  		break;
>>>>  	case VIRTIO_PCI_ISR:
>>>>  		ioport__write8(data, vpci->isr);
>>>> -		kvm__irq_line(kvm, vpci->pci_hdr.irq_line, VIRTIO_IRQ_LOW);
>>>> +		kvm__irq_line(kvm, vpci->legacy_irq_line, VIRTIO_IRQ_LOW);
>>>>  		vpci->isr = VIRTIO_IRQ_LOW;
>>>>  		break;
>>>>  	default:
>>>> @@ -299,7 +299,7 @@ int virtio_pci__signal_vq(struct kvm *kvm, struct virtio_device *vdev, u32 vq)
>>>>  			kvm__irq_trigger(kvm, vpci->gsis[vq]);
>>>>  	} else {
>>>>  		vpci->isr = VIRTIO_IRQ_HIGH;
>>>> -		kvm__irq_trigger(kvm, vpci->pci_hdr.irq_line);
>>>> +		kvm__irq_trigger(kvm, vpci->legacy_irq_line);
>>>>  	}
>>>>  	return 0;
>>>>  }
>>>> @@ -323,7 +323,7 @@ int virtio_pci__signal_config(struct kvm *kvm, struct virtio_device *vdev)
>>>>  			kvm__irq_trigger(kvm, vpci->config_gsi);
>>>>  	} else {
>>>>  		vpci->isr = VIRTIO_PCI_ISR_CONFIG;
>>>> -		kvm__irq_trigger(kvm, vpci->pci_hdr.irq_line);
>>>> +		kvm__irq_trigger(kvm, vpci->legacy_irq_line);
>>>>  	}
>>>>  
>>>>  	return 0;
>>>> @@ -422,6 +422,9 @@ int virtio_pci__init(struct kvm *kvm, void *dev, struct virtio_device *vdev,
>>>>  	if (r < 0)
>>>>  		goto free_msix_mmio;
>>>>  
>>>> +	/* save the IRQ that device__register() has allocated */
>>>> +	vpci->legacy_irq_line = vpci->pci_hdr.irq_line;
>>>
>>> I'd rather we used the container_of trick that we do for virtio-mmio
>>> devices when assigning the irq in device__register. Then we can avoid
>>> this line completely.
>>
>> Not completely sure I get what you mean, I take it you want to assign
>> legacy_irq_line in pci__assign_irq() directly (where the IRQ number is
>> allocated).
>> But this function is PCI generic code and is used by the VESA
>> framebuffer and the shmem device on x86 as well. For those devices
>> dev_hdr is not part of a struct virtio_pci, so we can't do container_of
>> to assign the legacy_irq_line here directly.
>> Admittedly this fix should apply to the other two users as well, but
>> VESA does not use interrupts and pci-shmem is completely broken anyway,
>> so I didn't bother to fix it in this regard.
>> Would it be justified to provide an IRQ number field in struct
>> device_header to address all users?
>>
>> Or what am I missing here?
> 
> If VESA and shmem are broken, they should either be fixed or removed.

I am tempted to remove shmem, since it's broken:
a) there is no upstream driver, only some out-of-tree uio driver module
in some Github repo
b) the PCI device BARs do not match what QEMU implements and what the
uio driver expects (IO BAR vs. MMIO BAR)
c) there is (at least one) bug in kvmtool (easily fixed, though)
I haven't completely given up yet fixing it, but that's for another
series ;-)

However ...

> 
> If you fix them, then we could have separate virtual buses for virtio-pci
> and emulated-pci (or whatever you want to call it). We could also have
> a separate bus for passthrough-devices too.
> 
> However, that's quite a lot of work for a bug-fix, so I guess the easiest
> thing is to extend your current hack to cover VESA and shmem too.

So looking more closely, shmem does not support legacy interrupts, only
MSIs. VESA does not support any interrupts at all.
So there is nothing to do here, as the only legacy PCI interrupt user is
virtio, which this patch fixes.
Do you insist on that container_of trick you mentioned earlier? If yes,
please elaborate on that, as I don't see how this should work and how it
could possibly be better (pci__assign_irq() is still generic).

Cheers,
Andre.

Re: [PATCH] kvmtool: don't use PCI config space IRQ line field

[Will Deacon]

Hi Andre,

On Thu, Jun 18, 2015 at 06:19:53PM +0100, Andre Przywara wrote:
> I am tempted to remove shmem, since it's broken:
> a) there is no upstream driver, only some out-of-tree uio driver module
> in some Github repo

Right, but that's the same for qemu and we've already made the jump of
merging the driver, so I don't think that's a good argument for throwing
it out of the tree.

> b) the PCI device BARs do not match what QEMU implements and what the
> uio driver expects (IO BAR vs. MMIO BAR)

In what way? A quick look suggests that kvmtool is at least aligned with
said github repo.

> c) there is (at least one) bug in kvmtool (easily fixed, though)

Care to elaborate?

Will

[PATCH] kvmtool: don't use PCI config space IRQ line field

[Will Deacon]

Hi Andre,

On Thu, Jun 18, 2015 at 06:19:53PM +0100, Andre Przywara wrote:
> I am tempted to remove shmem, since it's broken:
> a) there is no upstream driver, only some out-of-tree uio driver module
> in some Github repo

Right, but that's the same for qemu and we've already made the jump of
merging the driver, so I don't think that's a good argument for throwing
it out of the tree.

> b) the PCI device BARs do not match what QEMU implements and what the
> uio driver expects (IO BAR vs. MMIO BAR)

In what way? A quick look suggests that kvmtool is at least aligned with
said github repo.

> c) there is (at least one) bug in kvmtool (easily fixed, though)

Care to elaborate?

Will

Re: [PATCH] kvmtool: don't use PCI config space IRQ line field

[Andre Przywara]

Hi Will,

On 29/06/15 11:10, Will Deacon wrote:
> Hi Andre,
> 
> On Thu, Jun 18, 2015 at 06:19:53PM +0100, Andre Przywara wrote:
>> I am tempted to remove shmem, since it's broken:
>> a) there is no upstream driver, only some out-of-tree uio driver module
>> in some Github repo
> 
> Right, but that's the same for qemu and we've already made the jump of
> merging the driver, so I don't think that's a good argument for throwing
> it out of the tree.

If this driver has some future in the Linux tree, I agree it's worth to
keep it in, though I didn't see any effort to merge it lately.

>> b) the PCI device BARs do not match what QEMU implements and what the
>> uio driver expects (IO BAR vs. MMIO BAR)
> 
> In what way? A quick look suggests that kvmtool is at least aligned with
> said github repo.

The first BAR holds the control registers, QEMU and the UIO driver
require an MMIO region, kvmtool uses PIO :-(

>> c) there is (at least one) bug in kvmtool (easily fixed, though)

The size of the control register region in BAR0 is set to the size of
the shared memory region, where it should be some constant size (at
least 16 Bytes, QEMU uses 256, the spec says 1K, pick one ;-)
As PIO on x86 is at most 64K, this BAR gets ignored by the kernel with
any shmem size above that (it defaults to 4M).
As said the fix is easy, but ...

Those two bugs alone make we wonder if that ever worked on kvmtool,
obviously not with that UIO driver (which seems to work on QEMU).
I have fixes for both issues, but I haven't had a chance of testing this
in real action (just the driver loaded and lspci looking sensible). I
may send the patches later, but this doesn't have high priority for me
(unless someone bugs me ;-)

Cheers,
Andre.

[PATCH] kvmtool: don't use PCI config space IRQ line field

[Andre Przywara]

Hi Will,

On 29/06/15 11:10, Will Deacon wrote:
> Hi Andre,
> 
> On Thu, Jun 18, 2015 at 06:19:53PM +0100, Andre Przywara wrote:
>> I am tempted to remove shmem, since it's broken:
>> a) there is no upstream driver, only some out-of-tree uio driver module
>> in some Github repo
> 
> Right, but that's the same for qemu and we've already made the jump of
> merging the driver, so I don't think that's a good argument for throwing
> it out of the tree.

If this driver has some future in the Linux tree, I agree it's worth to
keep it in, though I didn't see any effort to merge it lately.

>> b) the PCI device BARs do not match what QEMU implements and what the
>> uio driver expects (IO BAR vs. MMIO BAR)
> 
> In what way? A quick look suggests that kvmtool is at least aligned with
> said github repo.

The first BAR holds the control registers, QEMU and the UIO driver
require an MMIO region, kvmtool uses PIO :-(

>> c) there is (at least one) bug in kvmtool (easily fixed, though)

The size of the control register region in BAR0 is set to the size of
the shared memory region, where it should be some constant size (at
least 16 Bytes, QEMU uses 256, the spec says 1K, pick one ;-)
As PIO on x86 is at most 64K, this BAR gets ignored by the kernel with
any shmem size above that (it defaults to 4M).
As said the fix is easy, but ...

Those two bugs alone make we wonder if that ever worked on kvmtool,
obviously not with that UIO driver (which seems to work on QEMU).
I have fixes for both issues, but I haven't had a chance of testing this
in real action (just the driver loaded and lspci looking sensible). I
may send the patches later, but this doesn't have high priority for me
(unless someone bugs me ;-)

Cheers,
Andre.