[Qemu-devel] [PATCH] sam460ex: Fix PCI interrupt connections

[Qemu-devel] [PATCH] sam460ex: Fix PCI interrupt connections

[Sebastian Bauer]

The four interrupts of the PCI bus are connected to the same UIC pin on the
real Sam460ex. Evidence for this can be found in the UBoot source for the
Sam460ex in the Sam460ex.c file where PCI_INTERRUPT_LINE in written. This
change brings the connection in line with this.

This fixes the problem that can be observed when adding further PCI cards
that get their interrupt rotated to other interrupts than PCI INT A. In
particular, the bug was observed and verified to be fixed (after this
change) with an additional OHCI PCI card.

Signed-off-by: Sebastian Bauer <mail@sebastianbauer.info>
---
 hw/ppc/sam460ex.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
index 0999efcc1e..b2b22f280d 100644
--- a/hw/ppc/sam460ex.c
+++ b/hw/ppc/sam460ex.c
@@ -515,9 +515,9 @@ static void sam460ex_init(MachineState *machine)
 
     /* PCI bus */
     ppc460ex_pcie_init(env);
-    /* FIXME: is this correct? */
+    /* All PCI ints are connected to the same UIC pin (cf. UBoot source) */
     dev = sysbus_create_varargs("ppc440-pcix-host", 0xc0ec00000,
-                                uic[1][0], uic[1][20], uic[1][21], uic[1][22],
+                                uic[1][0], uic[1][0], uic[1][0], uic[1][0],
                                 NULL);
     pci_bus = (PCIBus *)qdev_get_child_bus(dev, "pci.0");
     if (!pci_bus) {
-- 
2.18.0

Re: [Qemu-devel] [PATCH] sam460ex: Fix PCI interrupt connections

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1766 bytes --]

On Mon, Jul 30, 2018 at 06:39:04AM +0200, Sebastian Bauer wrote:
> The four interrupts of the PCI bus are connected to the same UIC pin on the
> real Sam460ex. Evidence for this can be found in the UBoot source for the
> Sam460ex in the Sam460ex.c file where PCI_INTERRUPT_LINE in written. This
> change brings the connection in line with this.
> 
> This fixes the problem that can be observed when adding further PCI cards
> that get their interrupt rotated to other interrupts than PCI INT A. In
> particular, the bug was observed and verified to be fixed (after this
> change) with an additional OHCI PCI card.
> 
> Signed-off-by: Sebastian Bauer <mail@sebastianbauer.info>

Applied to ppc-for-3.0.

> ---
>  hw/ppc/sam460ex.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
> index 0999efcc1e..b2b22f280d 100644
> --- a/hw/ppc/sam460ex.c
> +++ b/hw/ppc/sam460ex.c
> @@ -515,9 +515,9 @@ static void sam460ex_init(MachineState *machine)
>  
>      /* PCI bus */
>      ppc460ex_pcie_init(env);
> -    /* FIXME: is this correct? */
> +    /* All PCI ints are connected to the same UIC pin (cf. UBoot source) */
>      dev = sysbus_create_varargs("ppc440-pcix-host", 0xc0ec00000,
> -                                uic[1][0], uic[1][20], uic[1][21], uic[1][22],
> +                                uic[1][0], uic[1][0], uic[1][0], uic[1][0],
>                                  NULL);
>      pci_bus = (PCIBus *)qdev_get_child_bus(dev, "pci.0");
>      if (!pci_bus) {

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH] sam460ex: Fix PCI interrupt connections

[BALATON Zoltan]

On Mon, 30 Jul 2018, Sebastian Bauer wrote:
> The four interrupts of the PCI bus are connected to the same UIC pin on the
> real Sam460ex. Evidence for this can be found in the UBoot source for the
> Sam460ex in the Sam460ex.c file where PCI_INTERRUPT_LINE in written. This
> change brings the connection in line with this.
>
> This fixes the problem that can be observed when adding further PCI cards
> that get their interrupt rotated to other interrupts than PCI INT A. In
> particular, the bug was observed and verified to be fixed (after this
> change) with an additional OHCI PCI card.
>
> Signed-off-by: Sebastian Bauer <mail@sebastianbauer.info>
> ---
> hw/ppc/sam460ex.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
> index 0999efcc1e..b2b22f280d 100644
> --- a/hw/ppc/sam460ex.c
> +++ b/hw/ppc/sam460ex.c
> @@ -515,9 +515,9 @@ static void sam460ex_init(MachineState *machine)
>
>     /* PCI bus */
>     ppc460ex_pcie_init(env);
> -    /* FIXME: is this correct? */
> +    /* All PCI ints are connected to the same UIC pin (cf. UBoot source) */
>     dev = sysbus_create_varargs("ppc440-pcix-host", 0xc0ec00000,
> -                                uic[1][0], uic[1][20], uic[1][21], uic[1][22],
> +                                uic[1][0], uic[1][0], uic[1][0], uic[1][0],

I don't understand QOM. Does this really work? It will ultimately do

qdev_connect_gpio_out_named(dev, SYSBUS_DEVICE_GPIO_IRQ, 0, uic[1][0]);
qdev_connect_gpio_out_named(dev, SYSBUS_DEVICE_GPIO_IRQ, 1, uic[1][0]);
qdev_connect_gpio_out_named(dev, SYSBUS_DEVICE_GPIO_IRQ, 2, uic[1][0]);
qdev_connect_gpio_out_named(dev, SYSBUS_DEVICE_GPIO_IRQ, 3, uic[1][0]);

which will call for each of these

object_property_set_link(dev, uic[1][0], "some name", &errp);

Would this correctly add all device interrupts to the uic[1][0] irq or 
will this replace it so only the last line will be connected instead of 
the first after this patch? Could someone with more understanding 
about QOM confirm this?

If this does not work this way would mapping all PCI interrupts to first 
line in ppc440_pcix_map_irq at hw/ppc/ppc440_pcix.c:417 and assign only 
that to uic[1][0] be a better fix?

Regards,
BALATON Zoltan

Re: [Qemu-devel] [PATCH] sam460ex: Fix PCI interrupt connections

[Sebastian Bauer]

Am 2018-07-30 13:06, schrieb BALATON Zoltan:
> On Mon, 30 Jul 2018, Sebastian Bauer wrote:
>> The four interrupts of the PCI bus are connected to the same UIC pin 
>> on the
>> real Sam460ex. Evidence for this can be found in the UBoot source for 
>> the
>> Sam460ex in the Sam460ex.c file where PCI_INTERRUPT_LINE in written. 
>> This
>> change brings the connection in line with this.
>> 
>> This fixes the problem that can be observed when adding further PCI 
>> cards
>> that get their interrupt rotated to other interrupts than PCI INT A. 
>> In
>> particular, the bug was observed and verified to be fixed (after this
>> change) with an additional OHCI PCI card.
>> 
>> Signed-off-by: Sebastian Bauer <mail@sebastianbauer.info>
>> ---
>> hw/ppc/sam460ex.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
>> index 0999efcc1e..b2b22f280d 100644
>> --- a/hw/ppc/sam460ex.c
>> +++ b/hw/ppc/sam460ex.c
>> @@ -515,9 +515,9 @@ static void sam460ex_init(MachineState *machine)
>> 
>>     /* PCI bus */
>>     ppc460ex_pcie_init(env);
>> -    /* FIXME: is this correct? */
>> +    /* All PCI ints are connected to the same UIC pin (cf. UBoot 
>> source) */
>>     dev = sysbus_create_varargs("ppc440-pcix-host", 0xc0ec00000,
>> -                                uic[1][0], uic[1][20], uic[1][21], 
>> uic[1][22],
>> +                                uic[1][0], uic[1][0], uic[1][0], 
>> uic[1][0],
> 
> I don't understand QOM. Does this really work? It will ultimately do
> 
> qdev_connect_gpio_out_named(dev, SYSBUS_DEVICE_GPIO_IRQ, 0, uic[1][0]);
> qdev_connect_gpio_out_named(dev, SYSBUS_DEVICE_GPIO_IRQ, 1, uic[1][0]);
> qdev_connect_gpio_out_named(dev, SYSBUS_DEVICE_GPIO_IRQ, 2, uic[1][0]);
> qdev_connect_gpio_out_named(dev, SYSBUS_DEVICE_GPIO_IRQ, 3, uic[1][0]);
> 
> which will call for each of these
> 
> object_property_set_link(dev, uic[1][0], "some name", &errp);

I don't know QOM myself, but my research was that "some name" is 
different on each invocation. So you associate the same value to the 
same object (dev) under a property different name. What was not obvious 
to me was who is the owner of the value. But according to the doc QOM 
has the notation of strong references, but it is not created like that 
in qdev_connect_gpio_out_named(). So I assume that it is weak reference 
and the parent is not the owner.

> Would this correctly add all device interrupts to the uic[1][0] irq or
> will this replace it so only the last line will be connected instead
> of the first after this patch? Could someone with more understanding
> about QOM confirm this?

I don't know how this affects QOM, but as I see it the PCI bus has four 
interrupts and so you need to specify all of them (and specifying wrong 
ones seems to be not ideal either). The code assumes this throughout, 
e.g., see ppc440_pcix_initfn().

As I have written in the commit log, I tested this change. I used two 
cards, the (default) SATA and the OHCI controller and everything was 
working nicely (contrary to the previous state where only the SATA card 
worked because this was put into slot 1). Did you have a chance to test 
it yourself?

> If this does not work this way would mapping all PCI interrupts to
> first line in ppc440_pcix_map_irq at hw/ppc/ppc440_pcix.c:417 and
> assign only that to uic[1][0] be a better fix?

I thought about this, but then it needs to passed that we are dealing 
with a SAM board, which seems to be more complicated than this solution. 
You also would need to adjust that ppc440_pcix_initfn() and I'm not sure 
if it is possible to register a root PCI bus with only one interrupt and 
how the remaining code deals with that. Overall, this solution seem to 
be much simpler. Of course, if it doesn't work the way I proposed it 
must be changed again.

But thinking more about it, what it is indeed not so clear to me is how 
the interrupt states are logical combined. The hardware most likely 
would do an "or" (but it is difficult to verify this without 
schematics), not sure what QEMU does. It may just forward the level 
change which would be not the right thing of course. I'll investigate 
it. The behaviour would still be better than the previous state as 
usually both interrupt handlers are invoked, so no interrupt will be 
missed.

Bye
Sebastian

Re: [Qemu-devel] [PATCH] sam460ex: Fix PCI interrupt connections

[BALATON Zoltan]

On Mon, 30 Jul 2018, Sebastian Bauer wrote:
> Am 2018-07-30 13:06, schrieb BALATON Zoltan:
>> On Mon, 30 Jul 2018, Sebastian Bauer wrote:
>>> The four interrupts of the PCI bus are connected to the same UIC pin on 
>>> the
>>> real Sam460ex. Evidence for this can be found in the UBoot source for the
>>> Sam460ex in the Sam460ex.c file where PCI_INTERRUPT_LINE in written. This
>>> change brings the connection in line with this.
>>> 
>>> This fixes the problem that can be observed when adding further PCI cards
>>> that get their interrupt rotated to other interrupts than PCI INT A. In
>>> particular, the bug was observed and verified to be fixed (after this
>>> change) with an additional OHCI PCI card.
>>> 
>>> Signed-off-by: Sebastian Bauer <mail@sebastianbauer.info>
>>> ---
>>> hw/ppc/sam460ex.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
>>> index 0999efcc1e..b2b22f280d 100644
>>> --- a/hw/ppc/sam460ex.c
>>> +++ b/hw/ppc/sam460ex.c
>>> @@ -515,9 +515,9 @@ static void sam460ex_init(MachineState *machine)
>>>
>>>     /* PCI bus */
>>>     ppc460ex_pcie_init(env);
>>> -    /* FIXME: is this correct? */
>>> +    /* All PCI ints are connected to the same UIC pin (cf. UBoot source) 
>>> */
>>>     dev = sysbus_create_varargs("ppc440-pcix-host", 0xc0ec00000,
>>> -                                uic[1][0], uic[1][20], uic[1][21], 
>>> uic[1][22],
>>> +                                uic[1][0], uic[1][0], uic[1][0], 
>>> uic[1][0],
>> 
>> I don't understand QOM. Does this really work? It will ultimately do
>> 
>> qdev_connect_gpio_out_named(dev, SYSBUS_DEVICE_GPIO_IRQ, 0, uic[1][0]);
>> qdev_connect_gpio_out_named(dev, SYSBUS_DEVICE_GPIO_IRQ, 1, uic[1][0]);
>> qdev_connect_gpio_out_named(dev, SYSBUS_DEVICE_GPIO_IRQ, 2, uic[1][0]);
>> qdev_connect_gpio_out_named(dev, SYSBUS_DEVICE_GPIO_IRQ, 3, uic[1][0]);
>> 
>> which will call for each of these
>> 
>> object_property_set_link(dev, uic[1][0], "some name", &errp);
>
> I don't know QOM myself, but my research was that "some name" is different on 
> each invocation. So you associate the same value to the same object (dev) 
> under a property different name. What was not obvious to me was who is the 
> owner of the value. But according to the doc QOM has the notation of strong 
> references, but it is not created like that in qdev_connect_gpio_out_named(). 
> So I assume that it is weak reference and the parent is not the owner.

Yes, you are right, different name is generated for each irq line. But all 
this does not seem to matter because at the end irqs are raised/lowered 
directly in the irq array through functions passed when calling 
pci_register_root_bus so I'm not sure what these properties are used for 
if they are used at all so their value probably don't matter.

>> Would this correctly add all device interrupts to the uic[1][0] irq or
>> will this replace it so only the last line will be connected instead
>> of the first after this patch? Could someone with more understanding
>> about QOM confirm this?
>
> I don't know how this affects QOM, but as I see it the PCI bus has four 
> interrupts and so you need to specify all of them (and specifying wrong ones 
> seems to be not ideal either). The code assumes this throughout, e.g., see 
> ppc440_pcix_initfn().

I think the number of irq lines could be set, the functions have an nirq 
or num_irq parameters so the 4 lines is only assumed because PCI defines 
that and this is what's modelled but we could use different value here. 
Sam460ex seems to connect all four PCI irq lines to a single irq but I'm 
not sure what's the best way to model this (implementing 1 line in 
ppc440_pcix model or trying to merge these at some higher level). Using 
wrong irq lines is definitely wrong and was only left there because I had 
no clue about this when I've written that so your patch is definitely 
closer to what the board does.

> As I have written in the commit log, I tested this change. I used two cards, 
> the (default) SATA and the OHCI controller and everything was working nicely 
> (contrary to the previous state where only the SATA card worked because this 
> was put into slot 1). Did you have a chance to test it yourself?

I could not test it yet, I was trying to understand the code instead to 
make sure it works than just verifying it fixes one particular problem 
which I believe you've done.

>> If this does not work this way would mapping all PCI interrupts to
>> first line in ppc440_pcix_map_irq at hw/ppc/ppc440_pcix.c:417 and
>> assign only that to uic[1][0] be a better fix?
>
> I thought about this, but then it needs to passed that we are dealing with a 
> SAM board, which seems to be more complicated than this solution. You also 
> would need to adjust that ppc440_pcix_initfn() and I'm not sure if it is 
> possible to register a root PCI bus with only one interrupt and how the 
> remaining code deals with that. Overall, this solution seem to be much 
> simpler. Of course, if it doesn't work the way I proposed it must be changed 
> again.

The ppc440_pcix model is only used on the sam460ex so I think we can 
change it to model that and care about other cases when/if another SoC is 
added that has this part with different irqs just to keep things simple.

> But thinking more about it, what it is indeed not so clear to me is how the 
> interrupt states are logical combined. The hardware most likely would do an 
> "or" (but it is difficult to verify this without schematics), not sure what 
> QEMU does. It may just forward the level change which would be not the right 
> thing of course. I'll investigate it. The behaviour would still be better 
> than the previous state as usually both interrupt handlers are invoked, so no 
> interrupt will be missed.

I was trying to read hw/pci/pci.c to find out what would specifying the 
same irq multiple times do but I'm still not sure I could undestand it 
completely. AFAIU it will call the map_irq and set_irq functions specified 
in pci_register_root_bus. When we have the same irq in multiple lines 
theoretically it may happen that one slot is raising the interrupt and 
another is lowering another line which would change the same line here but 
I'm not sure how this is synchronised and if this could cause any problem.

Probably we can just change the map function in ppc440_pcix.c to always 
return the first line then what's specified for other lines should not 
matter and the above problem is avoided. We could even get rid of those 
additional irqs by changing ppc440_pcix.c to only model a single line but 
I'd need someone with better understanding of this to confirm that I got 
this right.

David, who should we ask to get advice on this?

Regards,
BALATON Zoltan

Re: [Qemu-devel] [PATCH] sam460ex: Fix PCI interrupt connections

[Peter Maydell]

On 30 July 2018 at 12:06, BALATON Zoltan <balaton@eik.bme.hu> wrote:
> On Mon, 30 Jul 2018, Sebastian Bauer wrote:
>>
>> The four interrupts of the PCI bus are connected to the same UIC pin on
>> the
>> real Sam460ex. Evidence for this can be found in the UBoot source for the
>> Sam460ex in the Sam460ex.c file where PCI_INTERRUPT_LINE in written. This
>> change brings the connection in line with this.
>>
>> This fixes the problem that can be observed when adding further PCI cards
>> that get their interrupt rotated to other interrupts than PCI INT A. In
>> particular, the bug was observed and verified to be fixed (after this
>> change) with an additional OHCI PCI card.
>>
>> Signed-off-by: Sebastian Bauer <mail@sebastianbauer.info>
>> ---
>> hw/ppc/sam460ex.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
>> index 0999efcc1e..b2b22f280d 100644
>> --- a/hw/ppc/sam460ex.c
>> +++ b/hw/ppc/sam460ex.c
>> @@ -515,9 +515,9 @@ static void sam460ex_init(MachineState *machine)
>>
>>     /* PCI bus */
>>     ppc460ex_pcie_init(env);
>> -    /* FIXME: is this correct? */
>> +    /* All PCI ints are connected to the same UIC pin (cf. UBoot source)
>> */
>>     dev = sysbus_create_varargs("ppc440-pcix-host", 0xc0ec00000,
>> -                                uic[1][0], uic[1][20], uic[1][21],
>> uic[1][22],
>> +                                uic[1][0], uic[1][0], uic[1][0],
>> uic[1][0],
>
>
> I don't understand QOM. Does this really work? It will ultimately do
>
> qdev_connect_gpio_out_named(dev, SYSBUS_DEVICE_GPIO_IRQ, 0, uic[1][0]);
> qdev_connect_gpio_out_named(dev, SYSBUS_DEVICE_GPIO_IRQ, 1, uic[1][0]);
> qdev_connect_gpio_out_named(dev, SYSBUS_DEVICE_GPIO_IRQ, 2, uic[1][0]);
> qdev_connect_gpio_out_named(dev, SYSBUS_DEVICE_GPIO_IRQ, 3, uic[1][0]);

You are correct; this will not do the intended thing. If you
want to wire up multiple outputs which are logically ORed
together into a single input, you need to instantiate an
OR gate for that (we have the TYPE_OR_IRQ for this).

thanks
-- PMM

Re: [Qemu-devel] [PATCH] sam460ex: Fix PCI interrupt connections

[Peter Maydell]

On 30 July 2018 at 23:37, BALATON Zoltan <balaton@eik.bme.hu> wrote:
> I think the number of irq lines could be set, the functions have an nirq or
> num_irq parameters so the 4 lines is only assumed because PCI defines that
> and this is what's modelled but we could use different value here. Sam460ex
> seems to connect all four PCI irq lines to a single irq but I'm not sure
> what's the best way to model this (implementing 1 line in ppc440_pcix model
> or trying to merge these at some higher level). Using wrong irq lines is
> definitely wrong and was only left there because I had no clue about this
> when I've written that so your patch is definitely closer to what the board
> does.
>
>> As I have written in the commit log, I tested this change. I used two
>> cards, the (default) SATA and the OHCI controller and everything was working
>> nicely (contrary to the previous state where only the SATA card worked
>> because this was put into slot 1). Did you have a chance to test it
>> yourself?
>
>
> I could not test it yet, I was trying to understand the code instead to make
> sure it works than just verifying it fixes one particular problem which I
> believe you've done.

QEMU's implementation of qemu_irq signal lines is that the destination
end provides the qemu_irq, which under the hood is a pointer to a
struct containing a pointer to the function in the destination device
which gets called when the source end says "the line level has changed".
This means that there won't be a compile time or runtime error if you
pass that qemu_irq to multiple sources (ie device outputs) simultaneously.
But the behaviour will be wrong, because the destination will see all
the "level is 0", "level is 1" calls from all the sources intermingled, eg

device A output:   ____|^^^^^^^^^^^^^|______

device B output:   _______|^^^^^|___________

destination sees:  ____|^^^^^^^^|___________

(because the destination gets the "level now 0" call when B's output
goes to zero). To get the desired behaviour:

logical OR:        ____|^^^^^^^^^^^^^|_____

you need an OR gate. (I'm assuming wired-OR because that's the
usual thing when several devices share an interrupt.)

If your testing involves a setup which doesn't happen to assert
several of the interrupt lines simultaneously you won't notice this
problem.

> Probably we can just change the map function in ppc440_pcix.c to always
> return the first line then what's specified for other lines should not
> matter and the above problem is avoided. We could even get rid of those
> additional irqs by changing ppc440_pcix.c to only model a single line but
> I'd need someone with better understanding of this to confirm that I got
> this right.

I think that would also fix the bug. The logically preferable
approach would depend rather on what the actual hardware does:
is there a PCI controller chip with 4 interrupt outputs which
the board wires together to a single interrupt controller line,
or does the PCI controller chip itself have just one output
and do the merging of the different PCI interrupts itself?

thanks
-- PMM

Re: [Qemu-devel] [PATCH] sam460ex: Fix PCI interrupt connections

[BALATON Zoltan]

On Mon, 30 Jul 2018, Peter Maydell wrote:
> On 30 July 2018 at 12:06, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>> On Mon, 30 Jul 2018, Sebastian Bauer wrote:
>>>
>>> The four interrupts of the PCI bus are connected to the same UIC pin on
>>> the
>>> real Sam460ex. Evidence for this can be found in the UBoot source for the
>>> Sam460ex in the Sam460ex.c file where PCI_INTERRUPT_LINE in written. This
>>> change brings the connection in line with this.
>>>
>>> This fixes the problem that can be observed when adding further PCI cards
>>> that get their interrupt rotated to other interrupts than PCI INT A. In
>>> particular, the bug was observed and verified to be fixed (after this
>>> change) with an additional OHCI PCI card.
>>>
>>> Signed-off-by: Sebastian Bauer <mail@sebastianbauer.info>
>>> ---
>>> hw/ppc/sam460ex.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
>>> index 0999efcc1e..b2b22f280d 100644
>>> --- a/hw/ppc/sam460ex.c
>>> +++ b/hw/ppc/sam460ex.c
>>> @@ -515,9 +515,9 @@ static void sam460ex_init(MachineState *machine)
>>>
>>>     /* PCI bus */
>>>     ppc460ex_pcie_init(env);
>>> -    /* FIXME: is this correct? */
>>> +    /* All PCI ints are connected to the same UIC pin (cf. UBoot source)
>>> */
>>>     dev = sysbus_create_varargs("ppc440-pcix-host", 0xc0ec00000,
>>> -                                uic[1][0], uic[1][20], uic[1][21],
>>> uic[1][22],
>>> +                                uic[1][0], uic[1][0], uic[1][0],
>>> uic[1][0],
>>
>>
>> I don't understand QOM. Does this really work? It will ultimately do
>>
>> qdev_connect_gpio_out_named(dev, SYSBUS_DEVICE_GPIO_IRQ, 0, uic[1][0]);
>> qdev_connect_gpio_out_named(dev, SYSBUS_DEVICE_GPIO_IRQ, 1, uic[1][0]);
>> qdev_connect_gpio_out_named(dev, SYSBUS_DEVICE_GPIO_IRQ, 2, uic[1][0]);
>> qdev_connect_gpio_out_named(dev, SYSBUS_DEVICE_GPIO_IRQ, 3, uic[1][0]);
>
> You are correct; this will not do the intended thing. If you
> want to wire up multiple outputs which are logically ORed
> together into a single input, you need to instantiate an
> OR gate for that (we have the TYPE_OR_IRQ for this).

Thanks for confirming it. However after reading hw/pci/pci.c I still don't 
see where these properties are used at all. It looks like it will just 
call the map_irq and set_irq functions specified in pci_register_root_bus 
passing them the irq array given in opaque and these will change irq lines 
directly. So I think we could just change the map function to map 
everything to the first line and then the other lines don't matter at all. 
We could even get rid of them and change ppc440_pcix to have a single line 
as used in the sam460ex (which is the only board using this model 
currently so we can add more complexity when/if anything else needs it to 
be different). Is there anything why this simple solution would not work 
that justifies adding more complexity now?

Regards,
BALATON Zoltan

Re: [Qemu-devel] [PATCH] sam460ex: Fix PCI interrupt connections

[Peter Maydell]

On 31 July 2018 at 00:00, BALATON Zoltan <balaton@eik.bme.hu> wrote:
> On Mon, 30 Jul 2018, Peter Maydell wrote:
>>
>> On 30 July 2018 at 12:06, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>>> I don't understand QOM. Does this really work? It will ultimately do
>>>
>>> qdev_connect_gpio_out_named(dev, SYSBUS_DEVICE_GPIO_IRQ, 0, uic[1][0]);
>>> qdev_connect_gpio_out_named(dev, SYSBUS_DEVICE_GPIO_IRQ, 1, uic[1][0]);
>>> qdev_connect_gpio_out_named(dev, SYSBUS_DEVICE_GPIO_IRQ, 2, uic[1][0]);
>>> qdev_connect_gpio_out_named(dev, SYSBUS_DEVICE_GPIO_IRQ, 3, uic[1][0]);
>>
>>
>> You are correct; this will not do the intended thing. If you
>> want to wire up multiple outputs which are logically ORed
>> together into a single input, you need to instantiate an
>> OR gate for that (we have the TYPE_OR_IRQ for this).
>
>
> Thanks for confirming it. However after reading hw/pci/pci.c I still don't
> see where these properties are used at all. It looks like it will just call
> the map_irq and set_irq functions specified in pci_register_root_bus passing
> them the irq array given in opaque and these will change irq lines directly.

When ppc4xx_pci_set_irq() calls qemu_set_irq() it passes it a
qemu_irq which is one of the s->irq[] which was initialized
via sysbus_init_irq(). The qdev_connect_gpio_out_named() calls
above are what connect those to the interrupt controller at the
other end of the line. Without them qemu_set_irq() does nothing
(because an initialized-but-never-connected qemu_irq is actually
a NULL pointer, and qemu_set_irq() exits doing nothing for NULL).

On the output end of a device, a qemu_irq connection is a QOM property
which is a link (pointer) property (a qemu_irq is a pointer to an
opaque struct). On the input end of a device, initializing an input
GPIO creates those opaque structs and sets the callback function
which will be invoked for a level change. The various functions for
wiring up gpio connections get a pointer to the input device's struct
and pass it to the output device by setting its QOM property.

> So I think we could just change the map function to map everything to the
> first line and then the other lines don't matter at all. We could even get
> rid of them and change ppc440_pcix to have a single line as used in the
> sam460ex (which is the only board using this model currently so we can add
> more complexity when/if anything else needs it to be different). Is there
> anything why this simple solution would not work that justifies adding more
> complexity now?

It would work. But creating an OR gate is half a dozen lines or so of
code, so it's not much more work than changing the PCI controller.
So we should prefer whichever is closest to what the real hardware
does, assuming we can determine that.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] sam460ex: Fix PCI interrupt connections

[BALATON Zoltan]

On Tue, 31 Jul 2018, Peter Maydell wrote:
> On 30 July 2018 at 23:37, BALATON Zoltan <balaton@eik.bme.hu> wrote:
> QEMU's implementation of qemu_irq signal lines is that the destination
> end provides the qemu_irq, which under the hood is a pointer to a
> struct containing a pointer to the function in the destination device
> which gets called when the source end says "the line level has changed".
> This means that there won't be a compile time or runtime error if you
> pass that qemu_irq to multiple sources (ie device outputs) simultaneously.
> But the behaviour will be wrong, because the destination will see all
> the "level is 0", "level is 1" calls from all the sources intermingled, eg
>
> device A output:   ____|^^^^^^^^^^^^^|______
>
> device B output:   _______|^^^^^|___________
>
> destination sees:  ____|^^^^^^^^|___________
>
> (because the destination gets the "level now 0" call when B's output
> goes to zero). To get the desired behaviour:
>
> logical OR:        ____|^^^^^^^^^^^^^|_____
>
> you need an OR gate. (I'm assuming wired-OR because that's the
> usual thing when several devices share an interrupt.)
>
> If your testing involves a setup which doesn't happen to assert
> several of the interrupt lines simultaneously you won't notice this
> problem.

I think the testing with SATA and USB mouse on a PCI card is likely to 
assert several interrupts simultanelously (eg. when moving the mouse while 
loading stuff from disk) but the OS might have some ways to still recover 
from this so maybe we won't notice it anyway. As this is now confirmed 
that using the same irq multiple times is wrong I think we need a better 
fix.

David, can you please drop this patch, we'll come up with a different fix.

>> Probably we can just change the map function in ppc440_pcix.c to always
>> return the first line then what's specified for other lines should not
>> matter and the above problem is avoided. We could even get rid of those
>> additional irqs by changing ppc440_pcix.c to only model a single line but
>> I'd need someone with better understanding of this to confirm that I got
>> this right.
>
> I think that would also fix the bug. The logically preferable
> approach would depend rather on what the actual hardware does:
> is there a PCI controller chip with 4 interrupt outputs which
> the board wires together to a single interrupt controller line,
> or does the PCI controller chip itself have just one output
> and do the merging of the different PCI interrupts itself?

Hmm, don't really know. The PCI controller is part of the SoC but we don't 
have the manual of this SoC. The physical board itself also has a single 
PCI slot so however it's wired internally it's only using one irq for 
that. Not sure what other internal PCI devices are there. A comment in 
U-Boot sources says this (it lists PCIB twice but maybe that's meant to be 
PCID?):

// IRQ2  = PCI_INT (PCIA, PCIB, PCIC, PCIB)

So that suggests probably there are 4 PCI irqs that are wired together but 
probably this is inside the SoC. It could also be read as there's only a 
single PCI_INT that delivers all four PCI interrupts. So if we go from 
that either adding an OR gate to sam460ex.c that ORs together the PCI 
lines and connects it to uic[1][0] would work, or changing ppc440_pcix.c 
to provide a single PCI interrupt? We don't really have definitive info 
other than this comment so whatever Sebastian prefers and you approve is 
fine with me.

Thank you,
BALATON Zoltan

Re: [Qemu-devel] [PATCH] sam460ex: Fix PCI interrupt connections

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4017 bytes --]

On Tue, Jul 31, 2018 at 01:31:46AM +0200, BALATON Zoltan wrote:
> On Tue, 31 Jul 2018, Peter Maydell wrote:
> > On 30 July 2018 at 23:37, BALATON Zoltan <balaton@eik.bme.hu> wrote:
> > QEMU's implementation of qemu_irq signal lines is that the destination
> > end provides the qemu_irq, which under the hood is a pointer to a
> > struct containing a pointer to the function in the destination device
> > which gets called when the source end says "the line level has changed".
> > This means that there won't be a compile time or runtime error if you
> > pass that qemu_irq to multiple sources (ie device outputs) simultaneously.
> > But the behaviour will be wrong, because the destination will see all
> > the "level is 0", "level is 1" calls from all the sources intermingled, eg
> > 
> > device A output:   ____|^^^^^^^^^^^^^|______
> > 
> > device B output:   _______|^^^^^|___________
> > 
> > destination sees:  ____|^^^^^^^^|___________
> > 
> > (because the destination gets the "level now 0" call when B's output
> > goes to zero). To get the desired behaviour:
> > 
> > logical OR:        ____|^^^^^^^^^^^^^|_____
> > 
> > you need an OR gate. (I'm assuming wired-OR because that's the
> > usual thing when several devices share an interrupt.)
> > 
> > If your testing involves a setup which doesn't happen to assert
> > several of the interrupt lines simultaneously you won't notice this
> > problem.
> 
> I think the testing with SATA and USB mouse on a PCI card is likely to
> assert several interrupts simultanelously (eg. when moving the mouse while
> loading stuff from disk) but the OS might have some ways to still recover
> from this so maybe we won't notice it anyway. As this is now confirmed that
> using the same irq multiple times is wrong I think we need a better fix.
> 
> David, can you please drop this patch, we'll come up with a different fix.

Done.  Should have looked at that patch a bit closer.

> > > Probably we can just change the map function in ppc440_pcix.c to always
> > > return the first line then what's specified for other lines should not
> > > matter and the above problem is avoided. We could even get rid of those
> > > additional irqs by changing ppc440_pcix.c to only model a single line but
> > > I'd need someone with better understanding of this to confirm that I got
> > > this right.
> > 
> > I think that would also fix the bug. The logically preferable
> > approach would depend rather on what the actual hardware does:
> > is there a PCI controller chip with 4 interrupt outputs which
> > the board wires together to a single interrupt controller line,
> > or does the PCI controller chip itself have just one output
> > and do the merging of the different PCI interrupts itself?
> 
> Hmm, don't really know. The PCI controller is part of the SoC but we don't
> have the manual of this SoC. The physical board itself also has a single PCI
> slot so however it's wired internally it's only using one irq for that. Not
> sure what other internal PCI devices are there. A comment in U-Boot sources
> says this (it lists PCIB twice but maybe that's meant to be PCID?):
> 
> // IRQ2  = PCI_INT (PCIA, PCIB, PCIC, PCIB)
> 
> So that suggests probably there are 4 PCI irqs that are wired together but
> probably this is inside the SoC. It could also be read as there's only a
> single PCI_INT that delivers all four PCI interrupts. So if we go from that
> either adding an OR gate to sam460ex.c that ORs together the PCI lines and
> connects it to uic[1][0] would work, or changing ppc440_pcix.c to provide a
> single PCI interrupt? We don't really have definitive info other than this
> comment so whatever Sebastian prefers and you approve is fine with me.
> 
> Thank you,
> BALATON Zoltan
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH] sam460ex: Fix PCI interrupt connections

[Sebastian Bauer]

Am 2018-07-31 02:18, schrieb David Gibson:
>> David, can you please drop this patch, we'll come up with a different 
>> fix.
> Done.  Should have looked at that patch a bit closer.

I created a follow up patch, unfortunately, based on the previous patch, 
as I did not spot your mail earlier than now. Note that the previous 
patch was an improvement over the old behaviour and under normal 
conditions the difference was not visible (as the OS serves both 
interrupts in the same run and at least on AmigaOS the interrupt 
handling is deferred but IRQs are acked quickly; it is still not correct 
though). The old implementation was not correct at all, i.e., all 
interrupts were missed. So it was technical an improvement (and the 
easiest to come up without deep understanding QEMU) ;)

Anyway, I don't know if that follow up patch is the right approach. But 
building the logical or gate seems also be very much code, especially as 
in pci.c the case of multiple irqs sources are seems to be already 
handled (if I understand that code correctly). The follow up patch most 
likely does not model the hardware correctly, but the behaviour should 
be the same. The logical or gate would model the hardware more closely.

Let me know if I should rebase to the state before my initial patch (I 
just looked and the previous patch was still not dropped) if you think 
that the change is fine.

There is also the possibility to make the special (num-irqs == 1) the 
common case, as the Sam460ex platform is the only user of this bus so 
far (and probably stays the only one). I'm not sure if it is worth all 
the hassle. Also note that the entire ppc440_pcix.c source file seems to 
be created for the sam board. I have no idea why that mapping function 
based on slots was chosen in the first place so I kept it. I would also 
be fine to remove that, which would simplify things a lot.

Let me know how to proceed.

Bye
Sebastian

Re: [Qemu-devel] [PATCH] sam460ex: Fix PCI interrupt connections

[Sebastian Bauer]

Am 2018-07-31 01:15, schrieb Peter Maydell:
> It would work. But creating an OR gate is half a dozen lines or so of
> code, so it's not much more work than changing the PCI controller.
> So we should prefer whichever is closest to what the real hardware
> does, assuming we can determine that.

The real way is not known. The only source of information is UBoot which 
sets the interrupt line to 32 for all PCI devices. At least AmigaOS 
doesn't overwrite this setting. But UBoot could be wrong of course, as 
the hardware provides only one PCI slot (I think the sm502 is also 
connected to that bus in a direct fashion, but it doesn't have an PCI 
IRQ). Technically, we could also modify UBoot to chose other lines, at 
least AmigaOS would continue to work (if this maps the QEMU wiring) but 
this UBoot would be incompatible to the original hardware.

Bye
Sebastian

Re: [Qemu-devel] [PATCH] sam460ex: Fix PCI interrupt connections

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2692 bytes --]

On Tue, Jul 31, 2018 at 06:57:31AM +0200, Sebastian Bauer wrote:
> Am 2018-07-31 02:18, schrieb David Gibson:
> > > David, can you please drop this patch, we'll come up with a
> > > different fix.
> > Done.  Should have looked at that patch a bit closer.
> 
> I created a follow up patch, unfortunately, based on the previous patch, as
> I did not spot your mail earlier than now. Note that the previous patch was
> an improvement over the old behaviour and under normal conditions the
> difference was not visible (as the OS serves both interrupts in the same run
> and at least on AmigaOS the interrupt handling is deferred but IRQs are
> acked quickly; it is still not correct though). The old implementation was
> not correct at all, i.e., all interrupts were missed. So it was technical an
> improvement (and the easiest to come up without deep understanding QEMU) ;)
> 
> Anyway, I don't know if that follow up patch is the right approach. But
> building the logical or gate seems also be very much code, especially as in
> pci.c the case of multiple irqs sources are seems to be already handled (if
> I understand that code correctly). The follow up patch most likely does not
> model the hardware correctly, but the behaviour should be the same. The
> logical or gate would model the hardware more closely.

Yeah, I think routing all the irqs to pin A should be ok for now.
AIUI it's not user or guest visible so we can fix it up later if we
get more information on the hardware.  I believe there are a number of
other embedded boards that have a similar hack too, since they only
have a single PCI LSI line on the board-level PIC.

> Let me know if I should rebase to the state before my initial patch (I just
> looked and the previous patch was still not dropped) if you think that the
> change is fine.

I'd prefer a rebase onto the current state of my ppc-for-3.0 tree,
please.

> There is also the possibility to make the special (num-irqs == 1) the common
> case, as the Sam460ex platform is the only user of this bus so far (and
> probably stays the only one). I'm not sure if it is worth all the hassle.
> Also note that the entire ppc440_pcix.c source file seems to be created for
> the sam board. I have no idea why that mapping function based on slots was
> chosen in the first place so I kept it. I would also be fine to remove that,
> which would simplify things a lot.
> 
> Let me know how to proceed.
> 
> Bye
> Sebastian
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH] sam460ex: Fix PCI interrupt connections

[BALATON Zoltan]

On Tue, 31 Jul 2018, Sebastian Bauer wrote:
> There is also the possibility to make the special (num-irqs == 1) the common 
> case, as the Sam460ex platform is the only user of this bus so far (and 
> probably stays the only one). I'm not sure if it is worth all the hassle.

I'd vote for this as I've said before. This seems to be the simplest 
solution so just change ppc440_pcix to have one irq instead of 4.

> Also note that the entire ppc440_pcix.c source file seems to be created for 
> the sam board. I have no idea why that mapping function based on slots was 
> chosen in the first place so I kept it. I would also be fine to remove that, 
> which would simplify things a lot.

It may come from other similar devices I've based the implementation on 
(ppc4xx_pci and ppce500) so no particular reason as I had no info on the 
real hardware and since I haven't tried multiple PCI cards it was just 
left there. You can remove it, I don't have anything that says it should 
be there, definitely not needed for sam460ex.

> Let me know how to proceed.

Will you send an updated patch or should I take and modify your latest one 
to help with this?

Regards,
BALATON Zoltan

Re: [Qemu-devel] [PATCH] sam460ex: Fix PCI interrupt connections

[Sebastian Bauer]

Hi,

Am 2018-07-31 11:50, schrieb BALATON Zoltan:
> On Tue, 31 Jul 2018, Sebastian Bauer wrote:
>> There is also the possibility to make the special (num-irqs == 1) the 
>> common case, as the Sam460ex platform is the only user of this bus so 
>> far (and probably stays the only one). I'm not sure if it is worth all 
>> the hassle.
> 
> I'd vote for this as I've said before. This seems to be the simplest
> solution so just change ppc440_pcix to have one irq instead of 4.

I now think so too, as, when devising my initial patch, I was not aware 
of the fact that this bus emulation was only created for the SAM.

>> Let me know how to proceed.
> Will you send an updated patch or should I take and modify your latest
> one to help with this?

I don't care so much about the how, but I would appreciate it, if the 
bug can be fixed, so that further PCI cards can be used without too much 
hassle (e.g., network cards). David seemed to be OK with my 2nd patch 
after I rebase it if I understood it correctly. My plan is to do this. 
But I'll also submit another alternative (shorter) patch that tailors 
the bus to the SAM only.

Bye
Sebastian

Re: [Qemu-devel] [Qemu-ppc] [PATCH] sam460ex: Fix PCI interrupt connections

[BALATON Zoltan]

On Tue, 31 Jul 2018, Sebastian Bauer wrote:
> Am 2018-07-31 11:50, schrieb BALATON Zoltan:
>> On Tue, 31 Jul 2018, Sebastian Bauer wrote:
>>> There is also the possibility to make the special (num-irqs == 1) the 
>>> common case, as the Sam460ex platform is the only user of this bus so far 
>>> (and probably stays the only one). I'm not sure if it is worth all the 
>>> hassle.
>> 
>> I'd vote for this as I've said before. This seems to be the simplest
>> solution so just change ppc440_pcix to have one irq instead of 4.
>
> I now think so too, as, when devising my initial patch, I was not aware of 
> the fact that this bus emulation was only created for the SAM.
>
>>> Let me know how to proceed.
>> Will you send an updated patch or should I take and modify your latest
>> one to help with this?
>
> I don't care so much about the how, but I would appreciate it, if the bug can 
> be fixed, so that further PCI cards can be used without too much hassle 
> (e.g., network cards). David seemed to be OK with my 2nd patch after I rebase 
> it if I understood it correctly. My plan is to do this. But I'll also submit 
> another alternative (shorter) patch that tailors the bus to the SAM only.

I've sent the shorter version as I think would be the easiest fix, please 
check if this is acceptable which may save you some work on updating your 
more complex patch which I think might not be needed at this point until 
something is found to need more interrupts in the ppc440_pcix model.

I've checked that this improves sound with ES1370, haven't checked with 
pci-ohci so you could verify that.

Regards,
BALATON Zoltan