Re: [RFC PATCH 6/6] memory: Use CPU register size as default access_size_max

[Philippe Mathieu-Daudé]

On 5/31/20 9:14 PM, Peter Maydell wrote:
> On Sun, 31 May 2020 at 18:54, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> Do not restrict 64-bit CPU to 32-bit max access by default.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>> RFC because this probably require an audit of all devices
>> used on 64-bit targets.
>> But if we find such problematic devices, they should instead
>> enforce their access_size_max = 4 rather than expecting the
>> default value to be valid...
>> ---
>>  memory.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/memory.c b/memory.c
>> index fd6f3d6aca..1d6bb5cdb0 100644
>> --- a/memory.c
>> +++ b/memory.c
>> @@ -1370,7 +1370,7 @@ bool memory_region_access_valid(MemoryRegion *mr,
>>
>>      access_size_max = mr->ops->valid.max_access_size;
>>      if (!mr->ops->valid.max_access_size) {
>> -        access_size_max = 4;
>> +        access_size_max = TARGET_LONG_SIZE;
>>      }
> 
> This is definitely not the right approach. TARGET_LONG_SIZE
> is a property of the CPU, but memory_region_access_valid()
> is testing properties of the MemoryRegion (ie the device
> being addressed). One can have devices in a system with a
> 64-bit CPU which can only handle being accessed at 32-bit
> width (indeed, it's pretty common). The behaviour of a device
> shouldn't change depending on whether we happened to compile
> it into a system with TARGET_LONG_SIZE=4 or 8.

Agreed.

> 
> (If you want to argue that we should make all our devices
> explicit about the valid.max_access_size rather than relying
> on "default means 4" then I wouldn't necessarily disagree.)

Yes, I'd rather not have access_size_max set automagically, but fixing
this require a long audit, and I suppose nobody cares.
I'll drop this patch. Thanks for the review!

> 
> thanks
> -- PMM
>