On Tue, Aug 2, 2022 at 4:33 PM Peter Maydell <peter.mayd...@linaro.org> wrote: > > On Tue, 2 Aug 2022 at 15:20, Konrad, Frederic <frederic.kon...@amd.com> wrote: > > > > Hi Peter, > > > > CC'ing Philippe. > > > > > -----Original Message----- > > > From: Qemu-devel <qemu-devel- > > > bounces+fkonrad=amd....@nongnu.org> On Behalf Of Peter Maydell > > > Sent: 02 August 2022 14:19 > > > To: qemu-devel@nongnu.org > > > Cc: Fabien Chouteau <chout...@adacore.com>; Frederic Konrad > > > <konrad.frede...@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.mayd...@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-and...@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é <f4...@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-f4...@amsat.org/ Commit 98f52cdbb5 ("memory: Fix access_with_adjusted_size(small size) on big-endian memory regions") might be incomplete... Regards, Phil.
