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

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

[Dayeol Lee]

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/pmp.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
index 958c7502a0..7a9fd415ba 100644
--- a/target/riscv/pmp.c
+++ b/target/riscv/pmp.c
@@ -232,6 +232,7 @@ bool pmp_hart_has_privs(CPURISCVState *env, target_ulong addr,
 {
     int i = 0;
     int ret = -1;
+    int pmp_size = 0;
     target_ulong s = 0;
     target_ulong e = 0;
     pmp_priv_t allowed_privs = 0;
@@ -241,11 +242,21 @@ bool pmp_hart_has_privs(CPURISCVState *env, target_ulong addr,
         return true;
     }
 
+    /*
+     * 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 = -(addr | TARGET_PAGE_MASK);
+    } else {
+        pmp_size = size;
+    }
+
     /* 1.10 draft priv spec states there is an implicit order
          from low to high */
     for (i = 0; i < MAX_RISCV_PMPS; i++) {
         s = pmp_is_in_range(env, i, addr);
-        e = pmp_is_in_range(env, i, addr + size - 1);
+        e = pmp_is_in_range(env, i, addr + pmp_size - 1);
 
         /* partially inside */
         if ((s + e) == 1) {
-- 
2.23.0

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

[Palmer Dabbelt]

On Tue, 22 Oct 2019 14:21:29 PDT (-0700), 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/pmp.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index 958c7502a0..7a9fd415ba 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -232,6 +232,7 @@ bool pmp_hart_has_privs(CPURISCVState *env, target_ulong addr,
>  {
>      int i = 0;
>      int ret = -1;
> +    int pmp_size = 0;
>      target_ulong s = 0;
>      target_ulong e = 0;
>      pmp_priv_t allowed_privs = 0;
> @@ -241,11 +242,21 @@ bool pmp_hart_has_privs(CPURISCVState *env, target_ulong addr,
>          return true;
>      }
>
> +    /*
> +     * 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 = -(addr | TARGET_PAGE_MASK);
> +    } else {
> +        pmp_size = size;
> +    }
> +
>      /* 1.10 draft priv spec states there is an implicit order
>           from low to high */
>      for (i = 0; i < MAX_RISCV_PMPS; i++) {
>          s = pmp_is_in_range(env, i, addr);
> -        e = pmp_is_in_range(env, i, addr + size - 1);
> +        e = pmp_is_in_range(env, i, addr + pmp_size - 1);
>
>          /* partially inside */
>          if ((s + e) == 1) {

Thanks.  I've queued this up for my next pull request.

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

[Dayeol Lee]

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

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
> >>>>
> >>>>
> >>>>
>

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

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

[Palmer Dabbelt]

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

[Dayeol Lee]

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

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
>>>
>>>
>>>

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

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

[Dayeol Lee]

[-- 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 --]

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

[Jonathan Behrens]

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

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: 3042 bytes --]

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

[Dayeol Lee]

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

[Richard Henderson]

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).

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

[Dayeol Lee]

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

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).

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

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

[Richard Henderson]

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~

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

[Dayeol Lee]

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

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~
>

>

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

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

[Richard Henderson]

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~

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

[no-reply]

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

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

[Dayeol Lee]

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(-)

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) {
-- 
2.20.1