target/riscv: PMP violation due to wrong size parameter

riscv_cpu_tlb_fill() uses the `size` parameter to check PMP violation
using pmp_hart_has_privs().
However, the size passed from tlb_fill(), which is called by
get_page_addr_code(), is always a hard-coded value 0.
This causes a false PMP violation if the instruction presents on a
PMP boundary.

In order to fix, simply correct the size to 4 if the access_type is
MMU_INST_FETCH.

Signed-off-by: Dayeol Lee <dayeol@berkeley.edu>
---
 target/riscv/cpu.h        | 1 +
 target/riscv/cpu_helper.c | 9 ++++++++-
 2 files changed, 9 insertions(+), 1 deletion(-)

Patchew URL: https://patchew.org/QEMU/20191007052813.25814-1-dayeol@berkeley.edu/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20191007052813.25814-1-dayeol@berkeley.edu
Subject: [PATCH] target/riscv: PMP violation due to wrong size parameter

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Switched to a new branch 'test'
7ca9470 target/riscv: PMP violation due to wrong size parameter

=== OUTPUT BEGIN ===
ERROR: suspect code indent for conditional statements (4, 6)
#48: FILE: target/riscv/cpu_helper.c:474:
+    if (access_type == MMU_INST_FETCH) {
+      pmp_size = RISCV_INSN_LENGTH;

total: 1 errors, 0 warnings, 30 lines checked

Commit 7ca947048450 (target/riscv: PMP violation due to wrong size parameter) has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20191007052813.25814-1-dayeol@berkeley.edu/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

On 10/6/19 10:28 PM, Dayeol Lee wrote:
> riscv_cpu_tlb_fill() uses the `size` parameter to check PMP violation
> using pmp_hart_has_privs().
> However, the size passed from tlb_fill(), which is called by
> get_page_addr_code(), is always a hard-coded value 0.
> This causes a false PMP violation if the instruction presents on a
> PMP boundary.
> 
> In order to fix, simply correct the size to 4 if the access_type is
> MMU_INST_FETCH.

That's not correct.

In general, size 0 means "unknown size".  In this case, the one tlb lookup is
going to be used by lots of instructions -- everything that fits on the page.

If you want to support PMP on things that are not page boundaries, then you
will also have to call tlb_set_page with size != TARGET_PAGE_SIZE.

Fixing that will cause instructions within that page to be executed one at a
time, which also means they will be tlb_fill'd one at a time, which means that
you'll get the correct size value.

Which will be 2 or 4, depending on whether the configuration supports the
Compressed extension, and not just 4.


r~

Thank you very much for the clarification!

I found tlb_set_page with size != TARGET_PAGE_SIZE makes the translation
way too slow; the Linux doesn't seem to boot.

If that's the only way to reduce PMP granularity to less than
TARGET_PAGE_SIZE,
Can we just set the PMP default granularity to TARGET_PAGE_SIZE as it did
before?

OR

Can we bypass the partial match violation when size is unknown? (check the
starting address only)

I think both of the options does not exactly match with the ISA
specification,
but given that size=0 always causes the problem, I want it to be fixed as
soon as possible.

Any thoughts would be appreciated!

On Mon, Oct 7, 2019, 6:00 AM Richard Henderson <richard.henderson@linaro.org>
wrote:

> On 10/6/19 10:28 PM, Dayeol Lee wrote:
> > riscv_cpu_tlb_fill() uses the `size` parameter to check PMP violation
> > using pmp_hart_has_privs().
> > However, the size passed from tlb_fill(), which is called by
> > get_page_addr_code(), is always a hard-coded value 0.
> > This causes a false PMP violation if the instruction presents on a
> > PMP boundary.
> >
> > In order to fix, simply correct the size to 4 if the access_type is
> > MMU_INST_FETCH.
>
> That's not correct.
>
> In general, size 0 means "unknown size".  In this case, the one tlb lookup
> is
> going to be used by lots of instructions -- everything that fits on the
> page.
>
> If you want to support PMP on things that are not page boundaries, then you
> will also have to call tlb_set_page with size != TARGET_PAGE_SIZE.
>
> Fixing that will cause instructions within that page to be executed one at
> a
> time, which also means they will be tlb_fill'd one at a time, which means
> that
> you'll get the correct size value.
>
> Which will be 2 or 4, depending on whether the configuration supports the
> Compressed extension, and not just 4.
>
>
> r~
>

>

On 10/7/19 10:19 AM, Dayeol Lee wrote:
> Thank you very much for the clarification!
> 
> I found tlb_set_page with size != TARGET_PAGE_SIZE makes the translation way
> too slow; the Linux doesn't seem to boot.

To clarify, PMP specifies a range.  That range has only two end points.
Therefore, a maximum of 2 pages may be affected by a mis-aligned PMP boundary.

It sounds like you're getting size != TARGET_PAGE_SIZE for all pages.


r~

On Mon, Oct 7, 2019 at 11:25 AM Richard Henderson <
richard.henderson@linaro.org> wrote:

> On 10/7/19 10:19 AM, Dayeol Lee wrote:
> > Thank you very much for the clarification!
> >
> > I found tlb_set_page with size != TARGET_PAGE_SIZE makes the translation
> way
> > too slow; the Linux doesn't seem to boot.
>
> To clarify, PMP specifies a range.  That range has only two end points.
> Therefore, a maximum of 2 pages may be affected by a mis-aligned PMP
> boundary.
>
> It sounds like you're getting size != TARGET_PAGE_SIZE for all pages.
>
>
The cause of the problem is not a mis-aligned PMP boundary.
Let's say a PMP range is 0x1000 - 0x2000
if pmp_hart_has_privs() gets addr=0x2000 and size=0,
pmp_hart_has_privs() will ALWAYS return false because the code assumes size
> 0.
It checks if (addr) and (addr + size - 1) are within the PMP range for each
PMP entry.
(addr + size - 1) is supposed to be the last byte address of the memory
access, but it ends up with (addr - 1) if size = 0.
Thus, pmp_hart_has_privs() returns false as (addr - 1) = 0x1fff is within
the range, and addr = 0x2000 is out of the range (partial match violation).

On 10/7/19 11:41 AM, Dayeol Lee wrote:
> if pmp_hart_has_privs() gets addr=0x2000 and size=0,
> pmp_hart_has_privs() will ALWAYS return false because the code assumes size > 0.
> It checks if (addr) and (addr + size - 1) are within the PMP range for each PMP
> entry.

You certainly could do

    if (size == 0) {
        size = -(addr | TARGET_PAGE_MASK);
    }

to assume that all bytes from addr to the end of the page are accessed.  That
would avoid changing too much of the rest of the logic.

That said, this code will continue to not work for mis-aligned boundaries.

r~


> (addr + size - 1) is supposed to be the last byte address of the memory access,
> but it ends up with (addr - 1) if size = 0.
> Thus, pmp_hart_has_privs() returns false as (addr - 1) = 0x1fff is within the
> range, and addr = 0x2000 is out of the range (partial match violation).