diff mbox series

hw/aspeed: Correct minimum access size for all models

Message ID 20241118021820.4928-1-joel@jms.id.au (mailing list archive)
State New
Headers show
Series hw/aspeed: Correct minimum access size for all models | expand

Commit Message

Joel Stanley Nov. 18, 2024, 2:18 a.m. UTC
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>
---
 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(-)

Comments

Troy Lee Nov. 18, 2024, 2:35 a.m. UTC | #1
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
>
Peter Maydell Nov. 18, 2024, 10:10 a.m. UTC | #2
On Mon, 18 Nov 2024 at 02:19, 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>
> ---
>  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,

Have you reviewed all the device read/write function
implementations for these devices to check whether
(a) changing the .valid value does the right thing, or
(b) whether there are cases where we should instead
be updating the implementation and setting the .impl
min access size ?

thanks
-- PMM
Joel Stanley Nov. 19, 2024, 2:53 a.m. UTC | #3
On Mon, 18 Nov 2024 at 20:40, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Mon, 18 Nov 2024 at 02:19, 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>
> > ---
> >  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,
>
> Have you reviewed all the device read/write function
> implementations for these devices to check whether
> (a) changing the .valid value does the right thing, or

I read the implementation of the read/write memory ops and I believe
it does the right thing. We want devices to accept reads that are of
any size, instead of returning an error.

> (b) whether there are cases where we should instead
> be updating the implementation and setting the .impl
> min access size ?

Reading the documentation for impl vs valid, we definitely want to set
valid to 1. There should be no machine check when performing byte
reads.

I don't think we want to change .impl.min from the default of 1.

I'm not sure if I've missed something that you're trying to point out.
Are there gotchas about setting valid.min=1 that I should know about?

Cheers,

Joel
Peter Maydell Nov. 19, 2024, 10:29 a.m. UTC | #4
On Tue, 19 Nov 2024 at 02:53, Joel Stanley <joel@jms.id.au> wrote:
>
> On Mon, 18 Nov 2024 at 20:40, Peter Maydell <peter.maydell@linaro.org> wrote:
> > Have you reviewed all the device read/write function
> > implementations for these devices to check whether
> > (a) changing the .valid value does the right thing, or
>
> I read the implementation of the read/write memory ops and I believe
> it does the right thing. We want devices to accept reads that are of
> any size, instead of returning an error.
>
> > (b) whether there are cases where we should instead
> > be updating the implementation and setting the .impl
> > min access size ?
>
> Reading the documentation for impl vs valid, we definitely want to set
> valid to 1. There should be no machine check when performing byte
> reads.
>
> I don't think we want to change .impl.min from the default of 1.
>
> I'm not sure if I've missed something that you're trying to point out.
> Are there gotchas about setting valid.min=1 that I should know about?

The "gotcha" is that the memory system's implementation of the
size 1 and 2 reads when .impl.min is 4 and .valid.min is 1
(as for instance with aspeed_apb2opb_ops after this patch)
is "read 4 bytes, return the relevant portion"
and the implementation of size 1 and 2 writes is "pad the
small value with zeroes at either end appropriately so it is
4 bytes and write that". That is often the right thing for
the required behaviour of the device registers, but it is
also quite common that it is the wrong behaviour. For instance
for some devices the write of a byte is supposed to only modify
that byte, and leave the other bytes of a 4-byte register alone.
Or if the device has bit-clears-on-read behaviour for a register
then the default handling will incorrectly do that for bits
that the guest didn't actually read.

Conversely if the device leaves the .impl.min at its default 1
and moves .valid.min from 4 to 1 (as with eg ftgmac100_ops)
the device will now be called for byte reads and writes at any
address in its range. If a write to, say, byte 3 of a 32-bit
register is supposed to update bits [31:24], that won't happen
unless the write function is changed (usually if there's a switch
on offset the write to something that's not at a multiple-of-4
will end up in the default "log an error" code path).

What this adds up to is that it's a bit misleading to have
a single patch which changes the minimum access size for lots
of devices at once, because for each device you need to look
at QEMU's implementation of the read and write functions
together with the spec of the device, and confirm that the
right way to implement "byte writes are supported" for this
particular device is to change .valid.min, and that you don't
also need to make changes to the read or write function code
at the same time or adjust .impl.min. Putting them all in one
patch with no discussion in the commit message of the device
behaviour and implementation was just a bit of a yellow flag
to me that maybe this complexity wasn't considered.

If we get this wrong for one of these devices, it's also likely
to be rather easier to bisect the problem if bisection
can track it down to "we made this change to aspeed_timer"
rather than "we made this change to a dozen devices all at once".

-- PMM
diff mbox series

Patch

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