Re: [PATCH] hw/aspeed: Correct minimum access size for all models

[Troy Lee]

On Mon, Nov 18, 2024 at 10:18 AM Joel Stanley <joel@jms.id.au> wrote:
>
> Guest code was performing a byte load to the SCU MMIO region, leading to
> the guest code crashing (it should be using proper accessors, but
> that is not Qemu's bug). Hardware and the documentation[1] both agree that
> byte loads are okay, so change all of the aspeed devices to accept a
> minimum access size of 1.
>
> [1] See the 'ARM Address Space Mapping' table in the ASPEED docs. This
> is section 6.1 in the ast2400 and ast2700, and 7.1 in the ast2500 and
> ast2600 datasheets.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2636
> Signed-off-by: Joel Stanley <joel@jms.id.au>

Reviewed-by: Troy Lee <leetroy@gmail.com>

Troy Lee

> ---
>  hw/fsi/aspeed_apb2opb.c  | 2 +-
>  hw/gpio/aspeed_gpio.c    | 4 ++--
>  hw/intc/aspeed_vic.c     | 2 +-
>  hw/misc/aspeed_scu.c     | 4 ++--
>  hw/misc/aspeed_sdmc.c    | 2 +-
>  hw/misc/aspeed_xdma.c    | 2 +-
>  hw/net/ftgmac100.c       | 4 ++--
>  hw/sd/aspeed_sdhci.c     | 2 +-
>  hw/timer/aspeed_timer.c  | 2 +-
>  hw/watchdog/wdt_aspeed.c | 2 +-
>  10 files changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/hw/fsi/aspeed_apb2opb.c b/hw/fsi/aspeed_apb2opb.c
> index 0e2cc143f105..855dccf6094c 100644
> --- a/hw/fsi/aspeed_apb2opb.c
> +++ b/hw/fsi/aspeed_apb2opb.c
> @@ -259,7 +259,7 @@ static const struct MemoryRegionOps aspeed_apb2opb_ops = {
>      .read = fsi_aspeed_apb2opb_read,
>      .write = fsi_aspeed_apb2opb_write,
>      .valid.max_access_size = 4,
> -    .valid.min_access_size = 4,
> +    .valid.min_access_size = 1,
>      .impl.max_access_size = 4,
>      .impl.min_access_size = 4,
>      .endianness = DEVICE_LITTLE_ENDIAN,
> diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
> index a5b3f454e800..c8bb7e590696 100644
> --- a/hw/gpio/aspeed_gpio.c
> +++ b/hw/gpio/aspeed_gpio.c
> @@ -1372,7 +1372,7 @@ static const MemoryRegionOps aspeed_gpio_ops = {
>      .read       = aspeed_gpio_read,
>      .write      = aspeed_gpio_write,
>      .endianness = DEVICE_LITTLE_ENDIAN,
> -    .valid.min_access_size = 4,
> +    .valid.min_access_size = 1,
>      .valid.max_access_size = 4,
>  };
>
> @@ -1380,7 +1380,7 @@ static const MemoryRegionOps aspeed_gpio_2700_ops = {
>      .read       = aspeed_gpio_2700_read,
>      .write      = aspeed_gpio_2700_write,
>      .endianness = DEVICE_LITTLE_ENDIAN,
> -    .valid.min_access_size = 4,
> +    .valid.min_access_size = 1,
>      .valid.max_access_size = 4,
>  };
>
> diff --git a/hw/intc/aspeed_vic.c b/hw/intc/aspeed_vic.c
> index 55fe51a6675f..8ee662064469 100644
> --- a/hw/intc/aspeed_vic.c
> +++ b/hw/intc/aspeed_vic.c
> @@ -286,7 +286,7 @@ static const MemoryRegionOps aspeed_vic_ops = {
>      .read = aspeed_vic_read,
>      .write = aspeed_vic_write,
>      .endianness = DEVICE_LITTLE_ENDIAN,
> -    .valid.min_access_size = 4,
> +    .valid.min_access_size = 1,
>      .valid.max_access_size = 4,
>      .valid.unaligned = false,
>  };
> diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c
> index 2c919349cfc0..b7a62da45907 100644
> --- a/hw/misc/aspeed_scu.c
> +++ b/hw/misc/aspeed_scu.c
> @@ -436,7 +436,7 @@ static const MemoryRegionOps aspeed_ast2500_scu_ops = {
>      .read = aspeed_scu_read,
>      .write = aspeed_ast2500_scu_write,
>      .endianness = DEVICE_LITTLE_ENDIAN,
> -    .valid.min_access_size = 4,
> +    .valid.min_access_size = 1,
>      .valid.max_access_size = 4,
>      .valid.unaligned = false,
>  };
> @@ -777,7 +777,7 @@ static const MemoryRegionOps aspeed_ast2600_scu_ops = {
>      .read = aspeed_ast2600_scu_read,
>      .write = aspeed_ast2600_scu_write,
>      .endianness = DEVICE_LITTLE_ENDIAN,
> -    .valid.min_access_size = 4,
> +    .valid.min_access_size = 1,
>      .valid.max_access_size = 4,
>      .valid.unaligned = false,
>  };
> diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
> index 4bc9faf691d6..ba700b008e5e 100644
> --- a/hw/misc/aspeed_sdmc.c
> +++ b/hw/misc/aspeed_sdmc.c
> @@ -195,7 +195,7 @@ static const MemoryRegionOps aspeed_sdmc_ops = {
>      .read = aspeed_sdmc_read,
>      .write = aspeed_sdmc_write,
>      .endianness = DEVICE_LITTLE_ENDIAN,
> -    .valid.min_access_size = 4,
> +    .valid.min_access_size = 1,
>      .valid.max_access_size = 4,
>  };
>
> diff --git a/hw/misc/aspeed_xdma.c b/hw/misc/aspeed_xdma.c
> index 1dd32f72f453..f222c632c099 100644
> --- a/hw/misc/aspeed_xdma.c
> +++ b/hw/misc/aspeed_xdma.c
> @@ -114,7 +114,7 @@ static const MemoryRegionOps aspeed_xdma_ops = {
>      .read = aspeed_xdma_read,
>      .write = aspeed_xdma_write,
>      .endianness = DEVICE_NATIVE_ENDIAN,
> -    .valid.min_access_size = 4,
> +    .valid.min_access_size = 1,
>      .valid.max_access_size = 4,
>  };
>
> diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
> index 478356ee3e10..c8f6e1138ed0 100644
> --- a/hw/net/ftgmac100.c
> +++ b/hw/net/ftgmac100.c
> @@ -1150,7 +1150,7 @@ static ssize_t ftgmac100_receive(NetClientState *nc, 
> const uint8_t *buf,
>  static const MemoryRegionOps ftgmac100_ops = {
>      .read = ftgmac100_read,
>      .write = ftgmac100_write,
> -    .valid.min_access_size = 4,
> +    .valid.min_access_size = 1,
>      .valid.max_access_size = 4,
>      .endianness = DEVICE_LITTLE_ENDIAN,
>  };
> @@ -1158,7 +1158,7 @@ static const MemoryRegionOps ftgmac100_ops = {
>  static const MemoryRegionOps ftgmac100_high_ops = {
>      .read = ftgmac100_high_read,
>      .write = ftgmac100_high_write,
> -    .valid.min_access_size = 4,
> +    .valid.min_access_size = 1,
>      .valid.max_access_size = 4,
>      .endianness = DEVICE_LITTLE_ENDIAN,
>  };
> diff --git a/hw/sd/aspeed_sdhci.c b/hw/sd/aspeed_sdhci.c
> index 98d5460905df..85e3f05e438f 100644
> --- a/hw/sd/aspeed_sdhci.c
> +++ b/hw/sd/aspeed_sdhci.c
> @@ -123,7 +123,7 @@ static const MemoryRegionOps aspeed_sdhci_ops = {
>      .read = aspeed_sdhci_read,
>      .write = aspeed_sdhci_write,
>      .endianness = DEVICE_NATIVE_ENDIAN,
> -    .valid.min_access_size = 4,
> +    .valid.min_access_size = 1,
>      .valid.max_access_size = 4,
>  };
>
> diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
> index 149f7cc5a6aa..a116488aa2dd 100644
> --- a/hw/timer/aspeed_timer.c
> +++ b/hw/timer/aspeed_timer.c
> @@ -460,7 +460,7 @@ static const MemoryRegionOps aspeed_timer_ops = {
>      .read = aspeed_timer_read,
>      .write = aspeed_timer_write,
>      .endianness = DEVICE_LITTLE_ENDIAN,
> -    .valid.min_access_size = 4,
> +    .valid.min_access_size = 1,
>      .valid.max_access_size = 4,
>      .valid.unaligned = false,
>  };
> diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c
> index 39c3f362a833..d9fd6fc9079f 100644
> --- a/hw/watchdog/wdt_aspeed.c
> +++ b/hw/watchdog/wdt_aspeed.c
> @@ -229,7 +229,7 @@ static const MemoryRegionOps aspeed_wdt_ops = {
>      .read = aspeed_wdt_read,
>      .write = aspeed_wdt_write,
>      .endianness = DEVICE_LITTLE_ENDIAN,
> -    .valid.min_access_size = 4,
> +    .valid.min_access_size = 1,
>      .valid.max_access_size = 4,
>      .valid.unaligned = false,
>  };
> --
> 2.45.2
>