* [PATCH 0/2] allwinner-a10: Fix interrupt controller regression
@ 2023-06-06 10:46 Peter Maydell
2023-06-06 10:46 ` [PATCH 1/2] hw/intc/allwinner-a10-pic: Handle IRQ levels other than 0 or 1 Peter Maydell
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Peter Maydell @ 2023-06-06 10:46 UTC (permalink / raw)
To: qemu-arm, qemu-devel
Cc: Beniamino Galvani, Strahinja Jankovic, Guenter Roeck,
qemu-stable, Michael Tokarev
In commit 2c5fa0778c3b430 we fixed an endianness bug in the Allwinner
A10 PIC model; however in the process we introduced a regression.
This is because the old code was robust against the incoming 'level'
argument being something other than 0 or 1, whereas the new code was
not.
In particular, the allwinner-sdhost code treats its IRQ line as
0-vs-non-0 rather than 0-vs-1, so when the SD controller set its IRQ
line for any reason other than transmit the interrupt controller would
ignore it. The observed effect was a guest timeout when rebooting the
guest kernel.
Patch 1 in this series fixes the regression by restoring the
old behaviour of aw_a10_pic_set_irq() for non-0 levels; it
is stable material.
Patch 2 changes the SD controller to follow our usual convention that
simple IRQ lines only send 0 or 1; this isn't strictly necessary with
patch 1, but it avoids future surprises. It doesn't need to go to
stable.
thanks
-- PMM
Peter Maydell (2):
hw/intc/allwinner-a10-pic: Handle IRQ levels other than 0 or 1
hw/sd/allwinner-sdhost: Don't send non-boolean IRQ line levels
hw/intc/allwinner-a10-pic.c | 2 +-
hw/sd/allwinner-sdhost.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] hw/intc/allwinner-a10-pic: Handle IRQ levels other than 0 or 1
2023-06-06 10:46 [PATCH 0/2] allwinner-a10: Fix interrupt controller regression Peter Maydell
@ 2023-06-06 10:46 ` Peter Maydell
2023-06-06 14:14 ` Guenter Roeck
2023-06-06 10:46 ` [PATCH 2/2] hw/sd/allwinner-sdhost: Don't send non-boolean IRQ line levels Peter Maydell
2023-06-06 12:36 ` [PATCH 0/2] allwinner-a10: Fix interrupt controller regression Philippe Mathieu-Daudé
2 siblings, 1 reply; 9+ messages in thread
From: Peter Maydell @ 2023-06-06 10:46 UTC (permalink / raw)
To: qemu-arm, qemu-devel
Cc: Beniamino Galvani, Strahinja Jankovic, Guenter Roeck,
qemu-stable, Michael Tokarev
In commit 2c5fa0778c3b430 we fixed an endianness bug in the Allwinner
A10 PIC model; however in the process we introduced a regression.
This is because the old code was robust against the incoming 'level'
argument being something other than 0 or 1, whereas the new code was
not.
In particular, the allwinner-sdhost code treats its IRQ line
as 0-vs-non-0 rather than 0-vs-1, so when the SD controller
set its IRQ line for any reason other than transmit the
interrupt controller would ignore it. The observed effect
was a guest timeout when rebooting the guest kernel.
Handle level values other than 0 or 1, to restore the old
behaviour.
Fixes: 2c5fa0778c3b430 ("hw/intc/allwinner-a10-pic: Don't use set_bit()/clear_bit()")
Cc: qemu-stable@nongnu.org
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
hw/intc/allwinner-a10-pic.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/intc/allwinner-a10-pic.c b/hw/intc/allwinner-a10-pic.c
index 4875e68ba6a..d0bf8d545ba 100644
--- a/hw/intc/allwinner-a10-pic.c
+++ b/hw/intc/allwinner-a10-pic.c
@@ -51,7 +51,7 @@ static void aw_a10_pic_set_irq(void *opaque, int irq, int level)
AwA10PICState *s = opaque;
uint32_t *pending_reg = &s->irq_pending[irq / 32];
- *pending_reg = deposit32(*pending_reg, irq % 32, 1, level);
+ *pending_reg = deposit32(*pending_reg, irq % 32, 1, !!level);
aw_a10_pic_update(s);
}
--
2.34.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] hw/sd/allwinner-sdhost: Don't send non-boolean IRQ line levels
2023-06-06 10:46 [PATCH 0/2] allwinner-a10: Fix interrupt controller regression Peter Maydell
2023-06-06 10:46 ` [PATCH 1/2] hw/intc/allwinner-a10-pic: Handle IRQ levels other than 0 or 1 Peter Maydell
@ 2023-06-06 10:46 ` Peter Maydell
2023-06-06 12:39 ` Philippe Mathieu-Daudé
2023-06-06 14:14 ` Guenter Roeck
2023-06-06 12:36 ` [PATCH 0/2] allwinner-a10: Fix interrupt controller regression Philippe Mathieu-Daudé
2 siblings, 2 replies; 9+ messages in thread
From: Peter Maydell @ 2023-06-06 10:46 UTC (permalink / raw)
To: qemu-arm, qemu-devel
Cc: Beniamino Galvani, Strahinja Jankovic, Guenter Roeck,
qemu-stable, Michael Tokarev
QEMU allows qemu_irq lines to transfer arbitrary integers. However
the convention is that for a simple IRQ line the values transferred
are always 0 and 1. The A10 SD controller device instead assumes a
0-vs-non-0 convention, which happens to work with the interrupt
controller it is wired up to.
Coerce the value to boolean to follow our usual convention.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
hw/sd/allwinner-sdhost.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/sd/allwinner-sdhost.c b/hw/sd/allwinner-sdhost.c
index 92a0f42708d..62df8f3d396 100644
--- a/hw/sd/allwinner-sdhost.c
+++ b/hw/sd/allwinner-sdhost.c
@@ -191,7 +191,7 @@ static void allwinner_sdhost_update_irq(AwSdHostState *s)
}
trace_allwinner_sdhost_update_irq(irq);
- qemu_set_irq(s->irq, irq);
+ qemu_set_irq(s->irq, !!irq);
}
static void allwinner_sdhost_update_transfer_cnt(AwSdHostState *s,
--
2.34.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 0/2] allwinner-a10: Fix interrupt controller regression
2023-06-06 10:46 [PATCH 0/2] allwinner-a10: Fix interrupt controller regression Peter Maydell
2023-06-06 10:46 ` [PATCH 1/2] hw/intc/allwinner-a10-pic: Handle IRQ levels other than 0 or 1 Peter Maydell
2023-06-06 10:46 ` [PATCH 2/2] hw/sd/allwinner-sdhost: Don't send non-boolean IRQ line levels Peter Maydell
@ 2023-06-06 12:36 ` Philippe Mathieu-Daudé
2 siblings, 0 replies; 9+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-06-06 12:36 UTC (permalink / raw)
To: Peter Maydell, qemu-arm, qemu-devel
Cc: Beniamino Galvani, Strahinja Jankovic, Guenter Roeck,
qemu-stable, Michael Tokarev
On 6/6/23 12:46, Peter Maydell wrote:
> Peter Maydell (2):
> hw/intc/allwinner-a10-pic: Handle IRQ levels other than 0 or 1
> hw/sd/allwinner-sdhost: Don't send non-boolean IRQ line levels
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] hw/sd/allwinner-sdhost: Don't send non-boolean IRQ line levels
2023-06-06 10:46 ` [PATCH 2/2] hw/sd/allwinner-sdhost: Don't send non-boolean IRQ line levels Peter Maydell
@ 2023-06-06 12:39 ` Philippe Mathieu-Daudé
2023-06-06 12:55 ` Peter Maydell
2023-06-06 14:14 ` Guenter Roeck
1 sibling, 1 reply; 9+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-06-06 12:39 UTC (permalink / raw)
To: Peter Maydell, qemu-arm, qemu-devel
Cc: Beniamino Galvani, Strahinja Jankovic, Guenter Roeck,
qemu-stable, Michael Tokarev
Hi Peter,
On 6/6/23 12:46, Peter Maydell wrote:
> QEMU allows qemu_irq lines to transfer arbitrary integers. However
> the convention is that for a simple IRQ line the values transferred
> are always 0 and 1. The A10 SD controller device instead assumes a
> 0-vs-non-0 convention, which happens to work with the interrupt
> controller it is wired up to.
>
> Coerce the value to boolean to follow our usual convention.
I remember once wanting to convert qemu_set_irq() to take a boolean
argument but someone said using integer was more useful because ...?
I searched a bit but can't find that in my mail archives, maybe this
was on IRC. Any clue? (I find simpler to use a boolean rather than
having a convention of using integers restricted to [0, 1] range).
Regards,
Phil.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] hw/sd/allwinner-sdhost: Don't send non-boolean IRQ line levels
2023-06-06 12:39 ` Philippe Mathieu-Daudé
@ 2023-06-06 12:55 ` Peter Maydell
2023-06-06 13:13 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 9+ messages in thread
From: Peter Maydell @ 2023-06-06 12:55 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: qemu-arm, qemu-devel, Beniamino Galvani, Strahinja Jankovic,
Guenter Roeck, qemu-stable, Michael Tokarev
On Tue, 6 Jun 2023 at 13:39, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Hi Peter,
>
> On 6/6/23 12:46, Peter Maydell wrote:
> > QEMU allows qemu_irq lines to transfer arbitrary integers. However
> > the convention is that for a simple IRQ line the values transferred
> > are always 0 and 1. The A10 SD controller device instead assumes a
> > 0-vs-non-0 convention, which happens to work with the interrupt
> > controller it is wired up to.
> >
> > Coerce the value to boolean to follow our usual convention.
>
> I remember once wanting to convert qemu_set_irq() to take a boolean
> argument but someone said using integer was more useful because ...?
> I searched a bit but can't find that in my mail archives, maybe this
> was on IRC. Any clue? (I find simpler to use a boolean rather than
> having a convention of using integers restricted to [0, 1] range).
We have a lot of use cases where we just want to transfer a boolean
value between two devices. We have a few use cases where we want to
transfer an arbitrary integer across the channel between two devices.
(For instance hw/intc/etraxfs_pic.c:pic_update() sends a vector
number to the CPU via a qemu_irq -- see commit f4f643882d9dc467.)
At the moment we use qemu_irq() for both. In theory we could
construct a parallel set of machinery for wiring up and setting
values for the "want an integer" case and restrict qemu_irq() to
bool only, but the lazy path is to use the same function for both.
(If we had machinery that made it easy to construct arbitrary
strongly-typed signal connections, that might perhaps be ideal.
But it's probably not very easy especially in C.)
thanks
-- PMM
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] hw/sd/allwinner-sdhost: Don't send non-boolean IRQ line levels
2023-06-06 12:55 ` Peter Maydell
@ 2023-06-06 13:13 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 9+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-06-06 13:13 UTC (permalink / raw)
To: Peter Maydell
Cc: qemu-arm, qemu-devel, Beniamino Galvani, Strahinja Jankovic,
Guenter Roeck, qemu-stable, Michael Tokarev
On 6/6/23 14:55, Peter Maydell wrote:
> On Tue, 6 Jun 2023 at 13:39, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> Hi Peter,
>>
>> On 6/6/23 12:46, Peter Maydell wrote:
>>> QEMU allows qemu_irq lines to transfer arbitrary integers. However
>>> the convention is that for a simple IRQ line the values transferred
>>> are always 0 and 1. The A10 SD controller device instead assumes a
>>> 0-vs-non-0 convention, which happens to work with the interrupt
>>> controller it is wired up to.
>>>
>>> Coerce the value to boolean to follow our usual convention.
>>
>> I remember once wanting to convert qemu_set_irq() to take a boolean
>> argument but someone said using integer was more useful because ...?
>> I searched a bit but can't find that in my mail archives, maybe this
>> was on IRC. Any clue? (I find simpler to use a boolean rather than
>> having a convention of using integers restricted to [0, 1] range).
>
> We have a lot of use cases where we just want to transfer a boolean
> value between two devices. We have a few use cases where we want to
> transfer an arbitrary integer across the channel between two devices.
> (For instance hw/intc/etraxfs_pic.c:pic_update() sends a vector
> number to the CPU via a qemu_irq -- see commit f4f643882d9dc467.)
Thanks for this reference!
> At the moment we use qemu_irq() for both. In theory we could
> construct a parallel set of machinery for wiring up and setting
> values for the "want an integer" case and restrict qemu_irq() to
> bool only, but the lazy path is to use the same function for both.
> (If we had machinery that made it easy to construct arbitrary
> strongly-typed signal connections, that might perhaps be ideal.
> But it's probably not very easy especially in C.)
I see, thanks.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] hw/intc/allwinner-a10-pic: Handle IRQ levels other than 0 or 1
2023-06-06 10:46 ` [PATCH 1/2] hw/intc/allwinner-a10-pic: Handle IRQ levels other than 0 or 1 Peter Maydell
@ 2023-06-06 14:14 ` Guenter Roeck
0 siblings, 0 replies; 9+ messages in thread
From: Guenter Roeck @ 2023-06-06 14:14 UTC (permalink / raw)
To: Peter Maydell
Cc: qemu-arm, qemu-devel, Beniamino Galvani, Strahinja Jankovic,
qemu-stable, Michael Tokarev
On Tue, Jun 06, 2023 at 11:46:08AM +0100, Peter Maydell wrote:
> In commit 2c5fa0778c3b430 we fixed an endianness bug in the Allwinner
> A10 PIC model; however in the process we introduced a regression.
> This is because the old code was robust against the incoming 'level'
> argument being something other than 0 or 1, whereas the new code was
> not.
>
> In particular, the allwinner-sdhost code treats its IRQ line
> as 0-vs-non-0 rather than 0-vs-1, so when the SD controller
> set its IRQ line for any reason other than transmit the
> interrupt controller would ignore it. The observed effect
> was a guest timeout when rebooting the guest kernel.
>
> Handle level values other than 0 or 1, to restore the old
> behaviour.
>
> Fixes: 2c5fa0778c3b430 ("hw/intc/allwinner-a10-pic: Don't use set_bit()/clear_bit()")
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Tested-by: Guenter Roeck <linux@roeck-us.net>
Tested on top of 8.0.2, both this patch alone as well as this
patch plus the second patch in the series.
Guenter
> ---
> hw/intc/allwinner-a10-pic.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/intc/allwinner-a10-pic.c b/hw/intc/allwinner-a10-pic.c
> index 4875e68ba6a..d0bf8d545ba 100644
> --- a/hw/intc/allwinner-a10-pic.c
> +++ b/hw/intc/allwinner-a10-pic.c
> @@ -51,7 +51,7 @@ static void aw_a10_pic_set_irq(void *opaque, int irq, int level)
> AwA10PICState *s = opaque;
> uint32_t *pending_reg = &s->irq_pending[irq / 32];
>
> - *pending_reg = deposit32(*pending_reg, irq % 32, 1, level);
> + *pending_reg = deposit32(*pending_reg, irq % 32, 1, !!level);
> aw_a10_pic_update(s);
> }
>
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] hw/sd/allwinner-sdhost: Don't send non-boolean IRQ line levels
2023-06-06 10:46 ` [PATCH 2/2] hw/sd/allwinner-sdhost: Don't send non-boolean IRQ line levels Peter Maydell
2023-06-06 12:39 ` Philippe Mathieu-Daudé
@ 2023-06-06 14:14 ` Guenter Roeck
1 sibling, 0 replies; 9+ messages in thread
From: Guenter Roeck @ 2023-06-06 14:14 UTC (permalink / raw)
To: Peter Maydell
Cc: qemu-arm, qemu-devel, Beniamino Galvani, Strahinja Jankovic,
qemu-stable, Michael Tokarev
On Tue, Jun 06, 2023 at 11:46:09AM +0100, Peter Maydell wrote:
> QEMU allows qemu_irq lines to transfer arbitrary integers. However
> the convention is that for a simple IRQ line the values transferred
> are always 0 and 1. The A10 SD controller device instead assumes a
> 0-vs-non-0 convention, which happens to work with the interrupt
> controller it is wired up to.
>
> Coerce the value to boolean to follow our usual convention.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Tested-by: Guenter Roeck <linux@roeck-us.net>
> ---
> hw/sd/allwinner-sdhost.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/sd/allwinner-sdhost.c b/hw/sd/allwinner-sdhost.c
> index 92a0f42708d..62df8f3d396 100644
> --- a/hw/sd/allwinner-sdhost.c
> +++ b/hw/sd/allwinner-sdhost.c
> @@ -191,7 +191,7 @@ static void allwinner_sdhost_update_irq(AwSdHostState *s)
> }
>
> trace_allwinner_sdhost_update_irq(irq);
> - qemu_set_irq(s->irq, irq);
> + qemu_set_irq(s->irq, !!irq);
> }
>
> static void allwinner_sdhost_update_transfer_cnt(AwSdHostState *s,
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-06-06 14:15 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-06 10:46 [PATCH 0/2] allwinner-a10: Fix interrupt controller regression Peter Maydell
2023-06-06 10:46 ` [PATCH 1/2] hw/intc/allwinner-a10-pic: Handle IRQ levels other than 0 or 1 Peter Maydell
2023-06-06 14:14 ` Guenter Roeck
2023-06-06 10:46 ` [PATCH 2/2] hw/sd/allwinner-sdhost: Don't send non-boolean IRQ line levels Peter Maydell
2023-06-06 12:39 ` Philippe Mathieu-Daudé
2023-06-06 12:55 ` Peter Maydell
2023-06-06 13:13 ` Philippe Mathieu-Daudé
2023-06-06 14:14 ` Guenter Roeck
2023-06-06 12:36 ` [PATCH 0/2] allwinner-a10: Fix interrupt controller regression Philippe Mathieu-Daudé
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.