| Message ID | 20200531175425.10329-7-f4bug@amsat.org |
|---|---|
| State | New |
| Headers | show |
| Series | exec/memory: Rework some address and access size limits | expand |
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. (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
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 >
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);
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(-)