Re: [PATCH] memory: Revert "memory: accept mismatching sizes in memory_region_access_valid"

[Mark Cave-Ayland]

On 30/08/2020 08:43, Nathan Chancellor wrote:

> On Sun, Aug 30, 2020 at 08:24:15AM +0100, Mark Cave-Ayland wrote:
>> On 30/08/2020 07:49, Nathan Chancellor wrote:
>>
>>> Unfortunately, it does not. I applied it on top of latest
>>> git (ac8b279f13865d1a4f1958d3bf34240c1c3af90d) and I can still
>>> reproduce my failure. Is it possible that type of fix is needed
>>> in a RISC-V specific driver?
>>>
>>> Would you like me to comment on the Launchpad bug as well?
>>
>> Hi Nathan,
>>
>> I came up with a quick patch to enable some logging to catch memory accesses 
>> being
>> refused for a similar bug report at
>> https://bugs.launchpad.net/qemu/+bug/1886318/comments/13.
>>
>> Can you apply the same change to your tree and report any messages on stderr 
>> as you
>> do your board reboot test?
>>
>>
>> ATB,
>>
>> Mark.
> 
> Thanks Mark, that helped.
> 
> ...
> [    3.807738] reboot: Power down
> invalid size: riscv.sifive.test addr 0 size: 2
> sbi_trap_error: hart0: trap handler failed (error -2)
> sbi_trap_error: hart0: mcause=0x0000000000000007 mtval=0x0000000000100000
> sbi_trap_error: hart0: mepc=0x000000008000d4cc mstatus=0x0000000000001822
> sbi_trap_error: hart0: ra=0x000000008000999e sp=0x0000000080015c78
> sbi_trap_error: hart0: gp=0xffffffe000e765d0 tp=0xffffffe0081c0000
> sbi_trap_error: hart0: s0=0x0000000080015c88 s1=0x0000000000000040
> sbi_trap_error: hart0: a0=0x0000000000000000 a1=0x0000000080004024
> sbi_trap_error: hart0: a2=0x0000000080004024 a3=0x0000000080004024
> sbi_trap_error: hart0: a4=0x0000000000100000 a5=0x0000000000005555
> sbi_trap_error: hart0: a6=0x0000000000004024 a7=0x0000000080011158
> sbi_trap_error: hart0: s2=0x0000000000000000 s3=0x0000000080016000
> sbi_trap_error: hart0: s4=0x0000000000000000 s5=0x0000000000000000
> sbi_trap_error: hart0: s6=0x0000000000000001 s7=0x0000000000000000
> sbi_trap_error: hart0: s8=0x0000000000000000 s9=0x0000000000000000
> sbi_trap_error: hart0: s10=0x0000000000000000 s11=0x0000000000000008
> sbi_trap_error: hart0: t0=0x0000000000000000 t1=0x0000000000000000
> sbi_trap_error: hart0: t2=0x0000000000000000 t3=0x0000000000000000
> sbi_trap_error: hart0: t4=0x0000000000000000 t5=0x0000000000000000
> sbi_trap_error: hart0: t6=0x0000000000000000
> 
> With this diff, I can successfully shut down the board. No idea if that
> is valid or not though.
> 
> Cheers,
> Nathan
> 
> ============================================================
> 
> diff --git a/hw/riscv/sifive_test.c b/hw/riscv/sifive_test.c
> index 0c78fb2c93..8c70dd69df 100644
> --- a/hw/riscv/sifive_test.c
> +++ b/hw/riscv/sifive_test.c
> @@ -59,7 +59,7 @@ static const MemoryRegionOps sifive_test_ops = {
>      .write = sifive_test_write,
>      .endianness = DEVICE_NATIVE_ENDIAN,
>      .valid = {
> -        .min_access_size = 4,
> +        .min_access_size = 2,
>          .max_access_size = 4
>      }
>  };

Okay - so according to the definition above, before your patch the sifive_test 
device
has a min and max access size of 4, i.e. all writes less than 32-bits are 
invalid.
With your patch you reduce the min access size to 2 which allows 16-bit writes 
and so
allows your shutdown test to succeed.

I'm afraid I'm not familiar enough with RISCV or the sifive_test device 
specification
to know whether the QEMU definition is correct and the access should be 
refused, or
whether the guest is using the wrong size when writing to the sifive_test device
register.


ATB,

Mark.