There is a data type demotion bug in target/riscv/pmp.c
When the target_ulong is 8 bytes, pmpcfg_csr_read returns only lower 4
bytes.
---
 target/riscv/pmp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
index c828950..4b6c20e 100644
--- a/target/riscv/pmp.c
+++ b/target/riscv/pmp.c
@@ -337,7 +337,7 @@ target_ulong pmpcfg_csr_read(CPURISCVState *env, uint32_t reg_index)
 
     for (i = 0; i < sizeof(target_ulong); i++) {
         val = pmp_read_cfg(env, (reg_index * sizeof(target_ulong)) + i);
-        cfg_val |= (val << (i * 8));
+        cfg_val |= ((target_ulong)val << ((target_ulong)i * 8));
     }
 
     PMP_DEBUG("hart " TARGET_FMT_ld ": reg%d, val: 0x" TARGET_FMT_lx,
-- 
2.7.4

On Wed, 17 Oct 2018 23:44:26 PDT (-0700), dayeol@berkeley.edu wrote:
> There is a data type demotion bug in target/riscv/pmp.c
> When the target_ulong is 8 bytes, pmpcfg_csr_read returns only lower 4
> bytes.

This is missing at least the first requirement from 
https://wiki.qemu.org/Contribute/SubmitAPatch -- that's as far as I checked :).  
I can't take this one, please submit a v2.

> ---
>  target/riscv/pmp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index c828950..4b6c20e 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -337,7 +337,7 @@ target_ulong pmpcfg_csr_read(CPURISCVState *env, uint32_t reg_index)
>
>      for (i = 0; i < sizeof(target_ulong); i++) {
>          val = pmp_read_cfg(env, (reg_index * sizeof(target_ulong)) + i);
> -        cfg_val |= (val << (i * 8));
> +        cfg_val |= ((target_ulong)val << ((target_ulong)i * 8));
>      }
>
>      PMP_DEBUG("hart " TARGET_FMT_ld ": reg%d, val: 0x" TARGET_FMT_lx,

I believe this is correct: pmp_read_cfg() gives us the 8-bit PMP value, which 
then needs to be mixed together to form a single CSR value.  The default 
promotion rules will result in an integer here ("i*8" is integer, which flows 
through) resulting in a 32-bit signed value on most hosts.  That's obviously 
bogus on RV64I, with the high bits of the CSR being wrong.

Aside from the metadata

Reviewed-by: Palmer Dabbelt <palmer@sifive.com>

Thanks!

Hi Lee,

On 18/10/2018 08:44, Dayeol Lee wrote:
> There is a data type demotion bug in target/riscv/pmp.c
> When the target_ulong is 8 bytes, pmpcfg_csr_read returns only lower 4
> bytes.
> ---
>  target/riscv/pmp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index c828950..4b6c20e 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -337,7 +337,7 @@ target_ulong pmpcfg_csr_read(CPURISCVState *env, uint32_t reg_index)
>  
>      for (i = 0; i < sizeof(target_ulong); i++) {
>          val = pmp_read_cfg(env, (reg_index * sizeof(target_ulong)) + i);
> -        cfg_val |= (val << (i * 8));
> +        cfg_val |= ((target_ulong)val << ((target_ulong)i * 8));

Casting 'val' seems correct, however you don't need to cast the 'i'.

Also you can remove the external parenthesis.

>      }
>  
>      PMP_DEBUG("hart " TARGET_FMT_ld ": reg%d, val: 0x" TARGET_FMT_lx,
> 

Regards,

Phil.

On 18 October 2018 at 18:37, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> Hi Lee,
>
> On 18/10/2018 08:44, Dayeol Lee wrote:
>> There is a data type demotion bug in target/riscv/pmp.c
>> When the target_ulong is 8 bytes, pmpcfg_csr_read returns only lower 4
>> bytes.
>> ---
>>  target/riscv/pmp.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
>> index c828950..4b6c20e 100644
>> --- a/target/riscv/pmp.c
>> +++ b/target/riscv/pmp.c
>> @@ -337,7 +337,7 @@ target_ulong pmpcfg_csr_read(CPURISCVState *env, uint32_t reg_index)
>>
>>      for (i = 0; i < sizeof(target_ulong); i++) {
>>          val = pmp_read_cfg(env, (reg_index * sizeof(target_ulong)) + i);
>> -        cfg_val |= (val << (i * 8));
>> +        cfg_val |= ((target_ulong)val << ((target_ulong)i * 8));
>
> Casting 'val' seems correct, however you don't need to cast the 'i'.

You could just declare 'val' as a target_ulong to start with,
and avoid the need for a cast at all, I think ?

thanks
-- PMM

On 18/10/2018 19:44, Peter Maydell wrote:
> On 18 October 2018 at 18:37, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>> Hi Lee,
>>
>> On 18/10/2018 08:44, Dayeol Lee wrote:
>>> There is a data type demotion bug in target/riscv/pmp.c
>>> When the target_ulong is 8 bytes, pmpcfg_csr_read returns only lower 4
>>> bytes.
>>> ---
>>>  target/riscv/pmp.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
>>> index c828950..4b6c20e 100644
>>> --- a/target/riscv/pmp.c
>>> +++ b/target/riscv/pmp.c
>>> @@ -337,7 +337,7 @@ target_ulong pmpcfg_csr_read(CPURISCVState *env, uint32_t reg_index)
>>>
>>>      for (i = 0; i < sizeof(target_ulong); i++) {
>>>          val = pmp_read_cfg(env, (reg_index * sizeof(target_ulong)) + i);
>>> -        cfg_val |= (val << (i * 8));
>>> +        cfg_val |= ((target_ulong)val << ((target_ulong)i * 8));
>>
>> Casting 'val' seems correct, however you don't need to cast the 'i'.
> 
> You could just declare 'val' as a target_ulong to start with,
> and avoid the need for a cast at all, I think ?

I did not look at the whole code, just the patch context.
Now looking at the code this is obvious ;)
Thanks Peter!