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

[Joel Stanley <joel@jms.id.au>]

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