1. Patchew
  2. QEMU
  3. View series

[PATCH] target/riscv: PMP violation due to wrong size parameter

Dayeol Lee posted 1 patch 4 years, 6 months ago
Test FreeBSD passed
Test checkpatch passed
Test docker-clang@ubuntu passed
Test docker-mingw@fedora passed
Test docker-quick@centos7 passed
Test asan passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20191011231406.9808-1-dayeol@berkeley.edu
Maintainers: Sagar Karandikar <sagark@eecs.berkeley.edu>, Alistair Francis <Alistair.Francis@wdc.com>, Palmer Dabbelt <palmer@sifive.com>, Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
target/riscv/cpu_helper.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
[PATCH] target/riscv: PMP violation due to wrong size parameter
Posted by Dayeol Lee 4 years, 6 months ago 
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
Re: [PATCH] target/riscv: PMP violation due to wrong size parameter
Posted by Jonathan Behrens 4 years, 6 months ago 
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
>
>
>
Re: [PATCH] target/riscv: PMP violation due to wrong size parameter
Posted by Dayeol Lee 4 years, 6 months ago 
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
>>
>>
>>
Re: [PATCH] target/riscv: PMP violation due to wrong size parameter
Posted by Dayeol Lee 4 years, 6 months ago 
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
>>>
>>>
>>>
Re: [PATCH] target/riscv: PMP violation due to wrong size parameter
Posted by Palmer Dabbelt 4 years, 6 months ago 
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
>>>>
>>>>
>>>>
Re: [PATCH] target/riscv: PMP violation due to wrong size parameter
Posted by Dayeol Lee 4 years, 6 months ago 
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
> >>>>
> >>>>
> >>>>
>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.