qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Peter Maydell <peter.maydell@linaro.org>
To: Alexander Graf <graf@amazon.com>
Cc: Hendrik Borghorst <hborghor@amazon.de>,
	qemu-arm <qemu-arm@nongnu.org>,
	QEMU Developers <qemu-devel@nongnu.org>
Subject: Re: [PATCH] pl031: Expose RTCICR as proper WC register
Date: Fri, 8 Nov 2019 15:58:39 +0000	[thread overview]
Message-ID: <CAFEAcA8zkR_MZ-28Nc=x4j05U9MsFYfzQtkHxTf65ZwacK020w@mail.gmail.com> (raw)
In-Reply-To: <20191104115228.30745-1-graf@amazon.com>

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


  reply	other threads:[~2019-11-08 16:00 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-04 11:52 [PATCH] pl031: Expose RTCICR as proper WC register Alexander Graf
2019-11-08 15:58 ` Peter Maydell [this message]
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

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAFEAcA8zkR_MZ-28Nc=x4j05U9MsFYfzQtkHxTf65ZwacK020w@mail.gmail.com' \
    --to=peter.maydell@linaro.org \
    --cc=graf@amazon.com \
    --cc=hborghor@amazon.de \
    --cc=qemu-arm@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).