| Message ID | 20240702154042.3018932-1-zheyuma97@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] hw/misc/bcm2835_thermal: Fix access size handling in bcm2835_thermal_ops | expand |
On 2/7/24 17:40, Zheyu Ma wrote: > The current implementation of bcm2835_thermal_ops sets > impl.max_access_size and valid.min_access_size to 4, but leaves > impl.min_access_size and valid.max_access_size unset, defaulting to 1. > This causes issues when the memory system is presented with an access > of size 2 at an offset of 3, leading to an attempt to synthesize it as > a pair of byte accesses at offsets 3 and 4, which trips an assert. > > Additionally, the lack of valid.max_access_size setting causes another > issue: the memory system tries to synthesize a read using a 4-byte > access at offset 3 even though the device doesn't allow unaligned > accesses. > > This patch addresses these issues by explicitly setting both > impl.min_access_size and valid.max_access_size to 4, ensuring proper > handling of access sizes. > > Error log: > ERROR:hw/misc/bcm2835_thermal.c:55:bcm2835_thermal_read: code should not be reached > Bail out! ERROR:hw/misc/bcm2835_thermal.c:55:bcm2835_thermal_read: code should not be reached > Aborted > > Reproducer: > cat << EOF | qemu-system-aarch64 -display \ > none -machine accel=qtest, -m 512M -machine raspi3b -m 1G -qtest stdio > readw 0x3f212003 > EOF > Suggested-by: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Zheyu Ma <zheyuma97@gmail.com> > --- > Changes in v2: > - Added .valid.min_access_size and .valid.max_access_size settings > to ensure proper handling of valid access sizes. > --- > hw/misc/bcm2835_thermal.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/hw/misc/bcm2835_thermal.c b/hw/misc/bcm2835_thermal.c > index ee7816b8a5..0c49c088a7 100644 > --- a/hw/misc/bcm2835_thermal.c > +++ b/hw/misc/bcm2835_thermal.c > @@ -80,8 +80,10 @@ static void bcm2835_thermal_write(void *opaque, hwaddr addr, > static const MemoryRegionOps bcm2835_thermal_ops = { > .read = bcm2835_thermal_read, > .write = bcm2835_thermal_write, > + .impl.min_access_size = 4, Personally I consider accepting access of .valid.min_access_size > .impl.min_access_size as a bug in the memory core layer, whether being unaligned or not. > .impl.max_access_size = 4, > .valid.min_access_size = 4, > + .valid.max_access_size = 4, > .endianness = DEVICE_NATIVE_ENDIAN, > }; >
On Tue, 2 Jul 2024 at 16:50, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > Personally I consider accepting access of > > .valid.min_access_size > .impl.min_access_size > > as a bug in the memory core layer, whether being > unaligned or not. Mmm. You could make an argument for .valid: min_access_size = 4, unaligned = true .impl.min_access_size = 1, unaligned = false meaning "accesses must be 32 bits, and if the guest makes an unaligned 32 bit access then you can synthesize it with 4 byte reads/writes", I guess. But whether we (a) get that right (b) use it anywhere (c) use it anywhere intentionally is not something I'd be very confident in asserting :-) I definitely think that our "synthesize accesses where valid and impl mismatch" code could use a going-over, so we either get it right or assert if the MemoryRegionOps ask for something we don't handle. Maybe someday... thanks -- PMM
On Tue, 2 Jul 2024 at 16:41, Zheyu Ma <zheyuma97@gmail.com> wrote: > > The current implementation of bcm2835_thermal_ops sets > impl.max_access_size and valid.min_access_size to 4, but leaves > impl.min_access_size and valid.max_access_size unset, defaulting to 1. > This causes issues when the memory system is presented with an access > of size 2 at an offset of 3, leading to an attempt to synthesize it as > a pair of byte accesses at offsets 3 and 4, which trips an assert. > > Additionally, the lack of valid.max_access_size setting causes another > issue: the memory system tries to synthesize a read using a 4-byte > access at offset 3 even though the device doesn't allow unaligned > accesses. > > This patch addresses these issues by explicitly setting both > impl.min_access_size and valid.max_access_size to 4, ensuring proper > handling of access sizes. > > Error log: > ERROR:hw/misc/bcm2835_thermal.c:55:bcm2835_thermal_read: code should not be reached > Bail out! ERROR:hw/misc/bcm2835_thermal.c:55:bcm2835_thermal_read: code should not be reached > Aborted > > Reproducer: > cat << EOF | qemu-system-aarch64 -display \ > none -machine accel=qtest, -m 512M -machine raspi3b -m 1G -qtest stdio > readw 0x3f212003 > EOF > > Signed-off-by: Zheyu Ma <zheyuma97@gmail.com> > --- Applied to target-arm.next, thanks. -- PMM
diff --git a/hw/misc/bcm2835_thermal.c b/hw/misc/bcm2835_thermal.c index ee7816b8a5..0c49c088a7 100644 --- a/hw/misc/bcm2835_thermal.c +++ b/hw/misc/bcm2835_thermal.c @@ -80,8 +80,10 @@ static void bcm2835_thermal_write(void *opaque, hwaddr addr, static const MemoryRegionOps bcm2835_thermal_ops = { .read = bcm2835_thermal_read, .write = bcm2835_thermal_write, + .impl.min_access_size = 4, .impl.max_access_size = 4, .valid.min_access_size = 4, + .valid.max_access_size = 4, .endianness = DEVICE_NATIVE_ENDIAN, };
The current implementation of bcm2835_thermal_ops sets impl.max_access_size and valid.min_access_size to 4, but leaves impl.min_access_size and valid.max_access_size unset, defaulting to 1. This causes issues when the memory system is presented with an access of size 2 at an offset of 3, leading to an attempt to synthesize it as a pair of byte accesses at offsets 3 and 4, which trips an assert. Additionally, the lack of valid.max_access_size setting causes another issue: the memory system tries to synthesize a read using a 4-byte access at offset 3 even though the device doesn't allow unaligned accesses. This patch addresses these issues by explicitly setting both impl.min_access_size and valid.max_access_size to 4, ensuring proper handling of access sizes. Error log: ERROR:hw/misc/bcm2835_thermal.c:55:bcm2835_thermal_read: code should not be reached Bail out! ERROR:hw/misc/bcm2835_thermal.c:55:bcm2835_thermal_read: code should not be reached Aborted Reproducer: cat << EOF | qemu-system-aarch64 -display \ none -machine accel=qtest, -m 512M -machine raspi3b -m 1G -qtest stdio readw 0x3f212003 EOF Signed-off-by: Zheyu Ma <zheyuma97@gmail.com> --- Changes in v2: - Added .valid.min_access_size and .valid.max_access_size settings to ensure proper handling of valid access sizes. --- hw/misc/bcm2835_thermal.c | 2 ++ 1 file changed, 2 insertions(+)