| Message ID | 20191011231406.9808-1-dayeol@berkeley.edu |
|---|---|
| State | New |
| Headers | show |
| Series | target/riscv: PMP violation due to wrong size parameter | expand |
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 > > >
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 >> >> >>
Hi, Could this patch go through? If not please let me know so that I can fix. Thank you! Dayeol On Sat, Oct 12, 2019, 11:30 AM Dayeol Lee <dayeol@berkeley.edu> wrote: > 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 >>> >>> >>>
On Tue, 15 Oct 2019 10:04:32 PDT (-0700), dayeol@berkeley.edu wrote: > Hi, > > Could this patch go through? > If not please let me know so that I can fix. > Thank you! Sorry, I dropped this one. It's in the patch queue now. We should also check for size==0 in pmp_hart_has_privs(), as that won't work. LMK if you want to send a patch for that. > > Dayeol > > > On Sat, Oct 12, 2019, 11:30 AM Dayeol Lee <dayeol@berkeley.edu> wrote: > >> 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 >>>> >>>> >>>>
I'll move the entire check into pmp_hart_has_privs as it makes more sense. Thanks! On Fri, Oct 18, 2019, 3:01 PM Palmer Dabbelt <palmer@sifive.com> wrote: > On Tue, 15 Oct 2019 10:04:32 PDT (-0700), dayeol@berkeley.edu wrote: > > Hi, > > > > Could this patch go through? > > If not please let me know so that I can fix. > > Thank you! > > Sorry, I dropped this one. It's in the patch queue now. We should also > check > for size==0 in pmp_hart_has_privs(), as that won't work. LMK if you want > to > send a patch for that. > > > > > Dayeol > > > > > > On Sat, Oct 12, 2019, 11:30 AM Dayeol Lee <dayeol@berkeley.edu> wrote: > > > >> 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 > >>>> > >>>> > >>>> >
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) {