Message ID | 20191007052813.25814-1-dayeol@berkeley.edu |
---|---|
State | New |
Headers | show |
Series | target/riscv: PMP violation due to wrong size parameter | expand |
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).
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h index 0adb307f32..386c80e764 100644 --- a/target/riscv/cpu.h +++ b/target/riscv/cpu.h @@ -88,6 +88,7 @@ enum { #define MMU_USER_IDX 3 #define MAX_RISCV_PMPS (16) +#define RISCV_INSN_LENGTH 4 typedef struct CPURISCVState CPURISCVState; diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c index e32b6126af..877e89dbf2 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,15 @@ 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 (access_type == MMU_INST_FETCH) { + pmp_size = RISCV_INSN_LENGTH; + } 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) {
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(-)