QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] pl031: Expose RTCICR as proper WC register
@ 2019-11-04 11:52 Alexander Graf
  2019-11-08 15:58 ` Peter Maydell
  0 siblings, 1 reply; 7+ messages in thread
From: Alexander Graf @ 2019-11-04 11:52 UTC (permalink / raw)
  To: Peter Maydell; +Cc: qemu-arm, qemu-devel, Hendrik Borghorst

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





^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] pl031: Expose RTCICR as proper WC register
  2019-11-04 11:52 [PATCH] pl031: Expose RTCICR as proper WC register Alexander Graf
@ 2019-11-08 15:58 ` Peter Maydell
  2019-11-12  7:28   ` Alexander Graf
  0 siblings, 1 reply; 7+ messages in thread
From: Peter Maydell @ 2019-11-08 15:58 UTC (permalink / raw)
  To: Alexander Graf; +Cc: Hendrik Borghorst, qemu-arm, QEMU Developers

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] pl031: Expose RTCICR as proper WC register
  2019-11-08 15:58 ` Peter Maydell
@ 2019-11-12  7:28   ` Alexander Graf
  2019-11-12 11:57     ` Peter Maydell
  0 siblings, 1 reply; 7+ messages in thread
From: Alexander Graf @ 2019-11-12  7:28 UTC (permalink / raw)
  To: Peter Maydell; +Cc: qemu-arm, QEMU Developers, Hendrik Borghorst

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



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] pl031: Expose RTCICR as proper WC register
  2019-11-12  7:28   ` Alexander Graf
@ 2019-11-12 11:57     ` Peter Maydell
  2019-11-14 13:42       ` Peter Maydell
  0 siblings, 1 reply; 7+ messages in thread
From: Peter Maydell @ 2019-11-12 11:57 UTC (permalink / raw)
  To: Alexander Graf; +Cc: Hendrik Borghorst, qemu-arm, QEMU Developers

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] pl031: Expose RTCICR as proper WC register
  2019-11-12 11:57     ` Peter Maydell
@ 2019-11-14 13:42       ` Peter Maydell
  2019-11-14 20:45         ` Alexander Graf
  0 siblings, 1 reply; 7+ messages in thread
From: Peter Maydell @ 2019-11-14 13:42 UTC (permalink / raw)
  To: Alexander Graf; +Cc: Hendrik Borghorst, qemu-arm, QEMU Developers

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] pl031: Expose RTCICR as proper WC register
  2019-11-14 13:42       ` Peter Maydell
@ 2019-11-14 20:45         ` Alexander Graf
  2019-11-14 21:01           ` Peter Maydell
  0 siblings, 1 reply; 7+ messages in thread
From: Alexander Graf @ 2019-11-14 20:45 UTC (permalink / raw)
  To: Peter Maydell; +Cc: qemu-arm, QEMU Developers, Hendrik Borghorst



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



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] pl031: Expose RTCICR as proper WC register
  2019-11-14 20:45         ` Alexander Graf
@ 2019-11-14 21:01           ` Peter Maydell
  0 siblings, 0 replies; 7+ messages in thread
From: Peter Maydell @ 2019-11-14 21:01 UTC (permalink / raw)
  To: Alexander Graf; +Cc: Hendrik Borghorst, qemu-arm, QEMU Developers

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-04 11:52 [PATCH] pl031: Expose RTCICR as proper WC register Alexander Graf
2019-11-08 15:58 ` Peter Maydell
2019-11-12  7:28   ` Alexander Graf
2019-11-12 11:57     ` Peter Maydell
2019-11-14 13:42       ` Peter Maydell
2019-11-14 20:45         ` Alexander Graf
2019-11-14 21:01           ` Peter Maydell

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git