On Mon, 18 Nov 2024 at 20:40, Peter Maydell <peter.mayd...@linaro.org> wrote: > > On Mon, 18 Nov 2024 at 02:19, Joel Stanley <j...@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 <j...@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
