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

[Konrad, Frederic]

> -----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>

> 
> -- PMM