QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Dayeol Lee <dayeol@berkeley.edu>
To: Jonathan Behrens <fintelia@gmail.com>
Cc: "open list:RISC-V TCG CPUs" <qemu-riscv@nongnu.org>,
	Sagar Karandikar <sagark@eecs.berkeley.edu>,
	Bastian Koppelmann <kbastian@mail.uni-paderborn.de>,
	Palmer Dabbelt <palmer@sifive.com>,
	Richard Henderson <richard.henderson@linaro.org>,
	qemu-devel@nongnu.org,
	Alistair Francis <Alistair.Francis@wdc.com>,
	diodesign@tuta.io
Subject: Re: [PATCH] target/riscv: PMP violation due to wrong size parameter
Date: Sat, 12 Oct 2019 11:30:25 -0700
Message-ID: <CACjxMEsTuKEsL2OPVDGuUX8mCJCcOcLO1d0c3YFwmY1xxqP+Tg@mail.gmail.com> (raw)
In-Reply-To: <CANnJOVHy8TuitQHBQ=+uh6ZKB=5iEZaE2CsLosti5mHQvRi_KA@mail.gmail.com>

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

No it doesn't mean that.
But the following code will make the size TARGET_PAGE_SIZE - (page offset)
if the address is not aligned.

pmp_size = -(address | TARGET_PAGE_MASK)


On Fri, Oct 11, 2019, 7:37 PM Jonathan Behrens <fintelia@gmail.com> wrote:

> How do you know that the access won't straddle a page boundary? Is there a
> guarantee somewhere that size=0 means that the access is naturally aligned?
>
> Jonathan
>
>
> On Fri, Oct 11, 2019 at 7:14 PM Dayeol Lee <dayeol@berkeley.edu> wrote:
>
>> riscv_cpu_tlb_fill() uses the `size` parameter to check PMP violation
>> using pmp_hart_has_privs().
>> However, if the size is unknown (=0), the ending address will be
>> `addr - 1` as it is `addr + size - 1` in `pmp_hart_has_privs()`.
>> This always causes a false PMP violation on the starting address of the
>> range, as `addr - 1` is not in the range.
>>
>> In order to fix, we just assume that all bytes from addr to the end of
>> the page will be accessed if the size is unknown.
>>
>> Signed-off-by: Dayeol Lee <dayeol@berkeley.edu>
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>  target/riscv/cpu_helper.c | 13 ++++++++++++-
>>  1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
>> index e32b6126af..7d9a22b601 100644
>> --- a/target/riscv/cpu_helper.c
>> +++ b/target/riscv/cpu_helper.c
>> @@ -441,6 +441,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address,
>> int size,
>>      CPURISCVState *env = &cpu->env;
>>      hwaddr pa = 0;
>>      int prot;
>> +    int pmp_size = 0;
>>      bool pmp_violation = false;
>>      int ret = TRANSLATE_FAIL;
>>      int mode = mmu_idx;
>> @@ -460,9 +461,19 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address,
>> int size,
>>                    "%s address=%" VADDR_PRIx " ret %d physical "
>> TARGET_FMT_plx
>>                    " prot %d\n", __func__, address, ret, pa, prot);
>>
>> +    /*
>> +     * if size is unknown (0), assume that all bytes
>> +     * from addr to the end of the page will be accessed.
>> +     */
>> +    if (size == 0) {
>> +        pmp_size = -(address | TARGET_PAGE_MASK);
>> +    } else {
>> +        pmp_size = size;
>> +    }
>> +
>>      if (riscv_feature(env, RISCV_FEATURE_PMP) &&
>>          (ret == TRANSLATE_SUCCESS) &&
>> -        !pmp_hart_has_privs(env, pa, size, 1 << access_type, mode)) {
>> +        !pmp_hart_has_privs(env, pa, pmp_size, 1 << access_type, mode)) {
>>          ret = TRANSLATE_PMP_FAIL;
>>      }
>>      if (ret == TRANSLATE_PMP_FAIL) {
>> --
>> 2.20.1
>>
>>
>>

[-- Attachment #2: Type: text/html, Size: 3830 bytes --]

<div dir="auto"><div>No it doesn&#39;t mean that.</div><div dir="auto">But the following code will make the size TARGET_PAGE_SIZE - (page offset) if the address is not aligned.</div><div dir="auto"><br></div><div dir="auto"><span style="font-family:sans-serif">pmp_size = -(address | TARGET_PAGE_MASK)</span><br><br><br><div class="gmail_quote" dir="auto"><div dir="ltr" class="gmail_attr">On Fri, Oct 11, 2019, 7:37 PM Jonathan Behrens &lt;<a href="mailto:fintelia@gmail.com" target="_blank" rel="noreferrer">fintelia@gmail.com</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>How do you know that the access won&#39;t straddle a page boundary? Is there a guarantee somewhere that size=0 means that the access is naturally aligned?</div><div><br></div><div>Jonathan<br></div><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Oct 11, 2019 at 7:14 PM Dayeol Lee &lt;<a href="mailto:dayeol@berkeley.edu" rel="noreferrer noreferrer" target="_blank">dayeol@berkeley.edu</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">riscv_cpu_tlb_fill() uses the `size` parameter to check PMP violation<br>
using pmp_hart_has_privs().<br>
However, if the size is unknown (=0), the ending address will be<br>
`addr - 1` as it is `addr + size - 1` in `pmp_hart_has_privs()`.<br>
This always causes a false PMP violation on the starting address of the<br>
range, as `addr - 1` is not in the range.<br>
<br>
In order to fix, we just assume that all bytes from addr to the end of<br>
the page will be accessed if the size is unknown.<br>
<br>
Signed-off-by: Dayeol Lee &lt;<a href="mailto:dayeol@berkeley.edu" rel="noreferrer noreferrer" target="_blank">dayeol@berkeley.edu</a>&gt;<br>
Reviewed-by: Richard Henderson &lt;<a href="mailto:richard.henderson@linaro.org" rel="noreferrer noreferrer" target="_blank">richard.henderson@linaro.org</a>&gt;<br>
---<br>
 target/riscv/cpu_helper.c | 13 ++++++++++++-<br>
 1 file changed, 12 insertions(+), 1 deletion(-)<br>
<br>
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c<br>
index e32b6126af..7d9a22b601 100644<br>
--- a/target/riscv/cpu_helper.c<br>
+++ b/target/riscv/cpu_helper.c<br>
@@ -441,6 +441,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,<br>
     CPURISCVState *env = &amp;cpu-&gt;env;<br>
     hwaddr pa = 0;<br>
     int prot;<br>
+    int pmp_size = 0;<br>
     bool pmp_violation = false;<br>
     int ret = TRANSLATE_FAIL;<br>
     int mode = mmu_idx;<br>
@@ -460,9 +461,19 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,<br>
                   &quot;%s address=%&quot; VADDR_PRIx &quot; ret %d physical &quot; TARGET_FMT_plx<br>
                   &quot; prot %d\n&quot;, __func__, address, ret, pa, prot);<br>
<br>
+    /*<br>
+     * if size is unknown (0), assume that all bytes<br>
+     * from addr to the end of the page will be accessed.<br>
+     */<br>
+    if (size == 0) {<br>
+        pmp_size = -(address | TARGET_PAGE_MASK);<br>
+    } else {<br>
+        pmp_size = size;<br>
+    }<br>
+<br>
     if (riscv_feature(env, RISCV_FEATURE_PMP) &amp;&amp;<br>
         (ret == TRANSLATE_SUCCESS) &amp;&amp;<br>
-        !pmp_hart_has_privs(env, pa, size, 1 &lt;&lt; access_type, mode)) {<br>
+        !pmp_hart_has_privs(env, pa, pmp_size, 1 &lt;&lt; access_type, mode)) {<br>
         ret = TRANSLATE_PMP_FAIL;<br>
     }<br>
     if (ret == TRANSLATE_PMP_FAIL) {<br>
-- <br>
2.20.1<br>
<br>
<br>
</blockquote></div>
</blockquote></div></div></div>

  reply index

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-11 23:14 Dayeol Lee
2019-10-12  2:37 ` Jonathan Behrens
2019-10-12 18:30   ` Dayeol Lee [this message]
2019-10-15 17:04     ` Dayeol Lee
2019-10-18 19:01       ` Palmer Dabbelt
2019-10-18 19:28         ` Dayeol Lee
  -- strict thread matches above, loose matches on Subject: below --
2019-10-22 21:21 Dayeol Lee
2019-10-23 15:50 ` Palmer Dabbelt
2019-10-07  5:28 Dayeol Lee
2019-10-07  6:20 ` no-reply
2019-10-07 13:00 ` Richard Henderson
2019-10-07 17:19   ` Dayeol Lee
2019-10-07 18:25     ` Richard Henderson
2019-10-07 18:41       ` Dayeol Lee
2019-10-08  3:18         ` Richard Henderson

Reply instructions:

You may reply publically 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=CACjxMEsTuKEsL2OPVDGuUX8mCJCcOcLO1d0c3YFwmY1xxqP+Tg@mail.gmail.com \
    --to=dayeol@berkeley.edu \
    --cc=Alistair.Francis@wdc.com \
    --cc=diodesign@tuta.io \
    --cc=fintelia@gmail.com \
    --cc=kbastian@mail.uni-paderborn.de \
    --cc=palmer@sifive.com \
    --cc=qemu-devel@nongnu.org \
    --cc=qemu-riscv@nongnu.org \
    --cc=richard.henderson@linaro.org \
    --cc=sagark@eecs.berkeley.edu \
    /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

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