Re: [RFC PATCH 6/6] memory: Use CPU register size as default access_size_max
On 5/31/20 9:14 PM, Peter Maydell wrote: > On Sun, 31 May 2020 at 18:54, Philippe Mathieu-Daudé wrote: >> >> Do not restrict 64-bit CPU to 32-bit max access by default. >> >> Signed-off-by: Philippe Mathieu-Daudé >> --- >> 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 >
Re: [RFC PATCH 6/6] memory: Use CPU register size as default access_size_max
On Sun, 31 May 2020 at 18:54, Philippe Mathieu-Daudé wrote: > > Do not restrict 64-bit CPU to 32-bit max access by default. > > Signed-off-by: Philippe Mathieu-Daudé > --- > 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. (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.) thanks -- PMM
[RFC PATCH 6/6] memory: Use CPU register size as default access_size_max
Do not restrict 64-bit CPU to 32-bit max access by default. Signed-off-by: Philippe Mathieu-Daudé --- 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; } access_size = MAX(MIN(size, access_size_max), access_size_min); -- 2.21.3