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

[Philippe Mathieu-Daudé]

On Mon, Aug 8, 2022 at 11:15 AM Konrad, Frederic
<Frederic.Konrad@amd.com> wrote:
> > -----Original Message-----
> > From: Peter Maydell <peter.maydell@linaro.org>
> > Sent: 02 August 2022 15:34
> > To: Konrad, Frederic <Frederic.Konrad@amd.com>
> > Cc: qemu-devel@nongnu.org; Fabien Chouteau <chouteau@adacore.com>;
> > Frederic Konrad <konrad.frederic@yahoo.fr>; f4bug@amsat.org
> > Subject: Re: [PATCH for-7.1] hw/misc/grlib_ahb_apb_pnp: Support 8 and 16
> > bit accesses
> >
> > 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).
> > >
> > > 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.
>
> Ok got it, thanks.
>
> Reviewed-by: Frederic Konrad <fkonrad@amd.com>

Thanks Frederic.

Mark, Peter, since I'm preparing a pull request for MIPS, I'll queue
this single SPARC patch.

Per https://gitlab.com/qemu-project/qemu/-/issues/1132#note_1048558340
I'll also add:
Tested-by: Tomasz Martyniak <gitlab.com/tom4r>

Regards,

Phil.