Re: [PATCH for-7.1] hw/misc/grlib_ahb_apb_pnp: Support 8 and 16 bit accesses

[Philippe Mathieu-Daudé]

On Tue, Aug 2, 2022 at 4:33 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 2 Aug 2022 at 15:20, Konrad, Frederic <Frederic.Konrad@amd.com> wrote:
> >
> > Hi Peter,
> >
> > CC'ing Philippe.
> >
> > > -----Original Message-----
> > > From: Qemu-devel <qemu-devel-
> > > bounces+fkonrad=amd.com@nongnu.org> On Behalf Of Peter Maydell
> > > Sent: 02 August 2022 14:19
> > > To: qemu-devel@nongnu.org
> > > Cc: Fabien Chouteau <chouteau@adacore.com>; Frederic Konrad
> > > <konrad.frederic@yahoo.fr>
> > > Subject: [PATCH for-7.1] hw/misc/grlib_ahb_apb_pnp: Support 8 and 16 bit
> > > accesses
> > >
> > > In real hardware, the APB and AHB PNP data tables can be accessed
> > > with byte and halfword reads as well as word reads.  Our
> > > implementation currently only handles word reads.  Add support for
> > > the 8 and 16 bit accesses.  Note that we only need to handle aligned
> > > accesses -- unaligned accesses should continue to trap, as happens on
> > > hardware.
> > >
> > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1132
> > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > > ---
> > > It would be nice if we could just set the .valid.min_access_size in
> > > the MemoryRegionOps to 1 and have the memory system core synthesize
> > > the 1 and 2 byte accesses from a 4 byte read, but currently that
> > > doesn't work (see various past mailing list threads).

Hmm sorry I missed the past threads, the one I remember is about
unaligned accesses
(https://lore.kernel.org/qemu-devel/20170630030058.28943-1-andrew@aj.id.au/).

> > That looks good to me but I thought this was fixed by 1a5a5570 and 0fbe394a
> > because RTEMS do bytes accesses?
> >
> > Did that break at some point?
>
> I definitely tried letting the .impl vs .valid settings handle this,
> but the access_with_adjusted_size() code doesn't do the right thing.
> (In particular, the test case ELF in the bug report works with
> this patch, and doesn't work without it...)
>
> I'm pretty sure the problem with access_with_adjusted_size() is a
> long-standing bug -- I found a couple of mailing list threads about
> it. We really ought to fix that properly, but that's definitely not
> for-7.1 material.

I agree this is sufficient for 7.1, so:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

But I'd rather keep simple implementations (.impl) and use .valid fields,
adjusting with access_with_adjusted_size().

(At least we now have an ELF reproducer.)

One problem I remember is when PCI is involved.
Here I could only get MMIO working, but not PCI:
https://lore.kernel.org/qemu-devel/20200817161853.593247-1-f4bug@amsat.org/

Commit 98f52cdbb5 ("memory: Fix access_with_adjusted_size(small size)
on big-endian memory regions") might be incomplete...

Regards,

Phil.