[PATCH] pl031: Expose RTCICR as proper WC register

[PATCH] pl031: Expose RTCICR as proper WC register

[Alexander Graf]

The current pl031 RTCICR register implementation always clears the IRQ
pending status on a register write, regardless of the value it writes.

To justify that behavior, it references the arm926e documentation
(DDI0287B) and indicates that said document states that any write clears
the internal IRQ state. I could however not find any text in that document
backing the statement. In fact, it explicitly says:

  "Writing 1 to bit 0 of RTCICR clears the RTCINTR flag."

which describes it as much as a write-to-clear register as the PL031 spec
(DDI0224) does:

  "Writing 1 to bit position 0 clears the corresponding interrupt.
   Writing 0 has no effect."

Let's remove the bogus comment and instead follow both specs to what they
say.

Reported-by: Hendrik Borghorst <hborghor@amazon.de>
Signed-off-by: Alexander Graf <graf@amazon.com>
---
 hw/rtc/pl031.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/hw/rtc/pl031.c b/hw/rtc/pl031.c
index 3a982752a2..c57cf83165 100644
--- a/hw/rtc/pl031.c
+++ b/hw/rtc/pl031.c
@@ -149,11 +149,7 @@ static void pl031_write(void * opaque, hwaddr offset,
         pl031_update(s);
         break;
     case RTC_ICR:
-        /* The PL031 documentation (DDI0224B) states that the interrupt is
-           cleared when bit 0 of the written value is set.  However the
-           arm926e documentation (DDI0287B) states that the interrupt is
-           cleared when any value is written.  */
-        s->is = 0;
+        s->is &= ~value;
         pl031_update(s);
         break;
     case RTC_CR:
-- 
2.17.1




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

Re: [PATCH] pl031: Expose RTCICR as proper WC register

[Peter Maydell]

On Mon, 4 Nov 2019 at 11:52, Alexander Graf <graf@amazon.com> wrote:
>
> The current pl031 RTCICR register implementation always clears the IRQ
> pending status on a register write, regardless of the value it writes.
>
> To justify that behavior, it references the arm926e documentation
> (DDI0287B) and indicates that said document states that any write clears
> the internal IRQ state. I could however not find any text in that document
> backing the statement. In fact, it explicitly says:
>
>   "Writing 1 to bit 0 of RTCICR clears the RTCINTR flag."
>
> which describes it as much as a write-to-clear register as the PL031 spec
> (DDI0224) does:
>
>   "Writing 1 to bit position 0 clears the corresponding interrupt.
>    Writing 0 has no effect."

DDI0287B page 11-2 section 11.1 says
"The interrupt is cleared by writing any data value to the
interrupt clear register RTCICR". As you note, this contradicts
what it says later on in section 11.2.2.

(Interestingly, the PL030 does have a "write any value to
clear the interrupt" register, RTCEOI.)

I'm fairly sure this patch is right and the DDI0287B document
has an error, since it isn't internally consistent and doesn't
match the proper PL031 TRM.

Did you find this because you had a guest that assumed the
other behaviour? This bug has been in QEMU for a very long time,
and it seems odd for a guest to deliberately perform an action
(writing 0) which is documented to have no effect on the device...

> Let's remove the bogus comment and instead follow both specs to what they
> say.
>
> Reported-by: Hendrik Borghorst <hborghor@amazon.de>
> Signed-off-by: Alexander Graf <graf@amazon.com>
> ---
>  hw/rtc/pl031.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/hw/rtc/pl031.c b/hw/rtc/pl031.c
> index 3a982752a2..c57cf83165 100644
> --- a/hw/rtc/pl031.c
> +++ b/hw/rtc/pl031.c
> @@ -149,11 +149,7 @@ static void pl031_write(void * opaque, hwaddr offset,
>          pl031_update(s);
>          break;
>      case RTC_ICR:
> -        /* The PL031 documentation (DDI0224B) states that the interrupt is
> -           cleared when bit 0 of the written value is set.  However the
> -           arm926e documentation (DDI0287B) states that the interrupt is
> -           cleared when any value is written.  */
> -        s->is = 0;
> +        s->is &= ~value;
>          pl031_update(s);
>          break;
>      case RTC_CR:
> --
> 2.17.1

thanks
-- PMM

Re: [PATCH] pl031: Expose RTCICR as proper WC register

[Alexander Graf]

Hey Peter,

On 08.11.19 17:58, Peter Maydell wrote:
> On Mon, 4 Nov 2019 at 11:52, Alexander Graf <graf@amazon.com> wrote:
>>
>> The current pl031 RTCICR register implementation always clears the IRQ
>> pending status on a register write, regardless of the value it writes.
>>
>> To justify that behavior, it references the arm926e documentation
>> (DDI0287B) and indicates that said document states that any write clears
>> the internal IRQ state. I could however not find any text in that document
>> backing the statement. In fact, it explicitly says:
>>
>>    "Writing 1 to bit 0 of RTCICR clears the RTCINTR flag."
>>
>> which describes it as much as a write-to-clear register as the PL031 spec
>> (DDI0224) does:
>>
>>    "Writing 1 to bit position 0 clears the corresponding interrupt.
>>     Writing 0 has no effect."
> 
> DDI0287B page 11-2 section 11.1 says
> "The interrupt is cleared by writing any data value to the
> interrupt clear register RTCICR". As you note, this contradicts
> what it says later on in section 11.2.2.
> 
> (Interestingly, the PL030 does have a "write any value to
> clear the interrupt" register, RTCEOI.)
> 
> I'm fairly sure this patch is right and the DDI0287B document
> has an error, since it isn't internally consistent and doesn't
> match the proper PL031 TRM.
> 
> Did you find this because you had a guest that assumed the
> other behaviour? This bug has been in QEMU for a very long time,
> and it seems odd for a guest to deliberately perform an action
> (writing 0) which is documented to have no effect on the device...

We found this bug by trying to find justification for the behavior in 
the spec and apparently my spec reading skills were lacking. I could not 
find the reference you cited above.

So no, I did not see any guest breakage.

I still think that being consistent with the actual PL031 spec is 
preferable though. If any real world guest breaks because of this, we 
can still revert this patch and document the exact breakage in the 
comment instead.


Alex



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

Re: [PATCH] pl031: Expose RTCICR as proper WC register

[Peter Maydell]

On Tue, 12 Nov 2019 at 07:28, Alexander Graf <graf@amazon.com> wrote:
>
> Hey Peter,
>
> On 08.11.19 17:58, Peter Maydell wrote:
> > Did you find this because you had a guest that assumed the
> > other behaviour? This bug has been in QEMU for a very long time,
> > and it seems odd for a guest to deliberately perform an action
> > (writing 0) which is documented to have no effect on the device...
>
> We found this bug by trying to find justification for the behavior in
> the spec and apparently my spec reading skills were lacking. I could not
> find the reference you cited above.
>
> So no, I did not see any guest breakage.
>
> I still think that being consistent with the actual PL031 spec is
> preferable though. If any real world guest breaks because of this, we
> can still revert this patch and document the exact breakage in the
> comment instead.

Yeah, I agree; I'm essentially just gathering material
for the commit message here. (The gold standard would be
to go find some hardware with a real pl031 and prod it
to confirm behaviour, but that's more effort than really
seems justified to me.)

thanks
-- PMM

Re: [PATCH] pl031: Expose RTCICR as proper WC register

[Peter Maydell]

On Tue, 12 Nov 2019 at 11:57, Peter Maydell <peter.maydell@linaro.org> wrote:
> On Tue, 12 Nov 2019 at 07:28, Alexander Graf <graf@amazon.com> wrote:
> > I still think that being consistent with the actual PL031 spec is
> > preferable though. If any real world guest breaks because of this, we
> > can still revert this patch and document the exact breakage in the
> > comment instead.
>
> Yeah, I agree; I'm essentially just gathering material
> for the commit message here. (The gold standard would be
> to go find some hardware with a real pl031 and prod it
> to confirm behaviour, but that's more effort than really
> seems justified to me.)

I propose to put this in for 4.2 with an updated commit message:

===begin===
    pl031: Expose RTCICR as proper WC register

    The current PL031 RTCICR register implementation always clears the
    IRQ pending status on a register write, regardless of the value the
    guest writes.

    To justify that behavior, it references the ARM926EJ-S Development
    Chip Reference Manual (DDI0287B) and indicates that said document
    states that any write clears the internal IRQ state.  It is indeed
    true that in section 11.1 this document says:

      "The interrupt is cleared by writing any data value to the
       interrupt clear register RTCICR".

    However, later in section 11.2.2 it contradicts itself by saying:

      "Writing 1 to bit 0 of RTCICR clears the RTCINTR flag."

    The latter statement matches the PL031 TRM (DDI0224C), which says:

      "Writing 1 to bit position 0 clears the corresponding interrupt.
       Writing 0 has no effect."

    Let's assume that the self-contradictory DDI0287B is in error, and
    follow the reference manual for the device itself, by making the
    register write-one-to-clear.
===endit===

Is that OK?

thanks
-- PMM

Re: [PATCH] pl031: Expose RTCICR as proper WC register

[Alexander Graf]

On 14.11.19 15:42, Peter Maydell wrote:
> On Tue, 12 Nov 2019 at 11:57, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On Tue, 12 Nov 2019 at 07:28, Alexander Graf <graf@amazon.com> wrote:
>>> I still think that being consistent with the actual PL031 spec is
>>> preferable though. If any real world guest breaks because of this, we
>>> can still revert this patch and document the exact breakage in the
>>> comment instead.
>>
>> Yeah, I agree; I'm essentially just gathering material
>> for the commit message here. (The gold standard would be
>> to go find some hardware with a real pl031 and prod it
>> to confirm behaviour, but that's more effort than really
>> seems justified to me.)
> 
> I propose to put this in for 4.2 with an updated commit message:
> 
> ===begin===
>      pl031: Expose RTCICR as proper WC register
> 
>      The current PL031 RTCICR register implementation always clears the
>      IRQ pending status on a register write, regardless of the value the
>      guest writes.
> 
>      To justify that behavior, it references the ARM926EJ-S Development
>      Chip Reference Manual (DDI0287B) and indicates that said document
>      states that any write clears the internal IRQ state.  It is indeed
>      true that in section 11.1 this document says:
> 
>        "The interrupt is cleared by writing any data value to the
>         interrupt clear register RTCICR".
> 
>      However, later in section 11.2.2 it contradicts itself by saying:
> 
>        "Writing 1 to bit 0 of RTCICR clears the RTCINTR flag."
> 
>      The latter statement matches the PL031 TRM (DDI0224C), which says:
> 
>        "Writing 1 to bit position 0 clears the corresponding interrupt.
>         Writing 0 has no effect."
> 
>      Let's assume that the self-contradictory DDI0287B is in error, and
>      follow the reference manual for the device itself, by making the
>      register write-one-to-clear.
> ===endit===
> 
> Is that OK?

It's much better. Will you just fix it up inline for me please? :)


Thanks,

Alex



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

Re: [PATCH] pl031: Expose RTCICR as proper WC register

[Peter Maydell]

On Thu, 14 Nov 2019 at 20:45, Alexander Graf <graf@amazon.com> wrote:
> On 14.11.19 15:42, Peter Maydell wrote:
> > Is that OK?
>
> It's much better. Will you just fix it up inline for me please? :)

Sure :-)

-- PMM