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(+)

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,
 };
 
-- 
2.34.1

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

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