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

[Dayeol Lee]

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 <address@hidden> wrote:

On Tue, 15 Oct 2019 10:04:32 PDT (-0700), address@hidden 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 <address@hidden> 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 <address@hidden> 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 <address@hidden> 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 <address@hidden>
>>>> Reviewed-by: Richard Henderson <address@hidden>
>>>> ---
>>>>  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
>>>>
>>>>
>>>>