* [PATCH for-7.1] hw/misc/grlib_ahb_apb_pnp: Support 8 and 16 bit accesses
@ 2022-08-02 13:19 Peter Maydell
2022-08-02 14:20 ` Konrad, Frederic
0 siblings, 1 reply; 6+ messages in thread
From: Peter Maydell @ 2022-08-02 13:19 UTC (permalink / raw)
To: qemu-devel; +Cc: Fabien Chouteau, Frederic Konrad
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).
---
hw/misc/grlib_ahb_apb_pnp.c | 10 ++++++----
hw/misc/trace-events | 4 ++--
2 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/hw/misc/grlib_ahb_apb_pnp.c b/hw/misc/grlib_ahb_apb_pnp.c
index 43e001c3c7b..5b05f158592 100644
--- a/hw/misc/grlib_ahb_apb_pnp.c
+++ b/hw/misc/grlib_ahb_apb_pnp.c
@@ -136,7 +136,8 @@ static uint64_t grlib_ahb_pnp_read(void *opaque, hwaddr offset, unsigned size)
uint32_t val;
val = ahb_pnp->regs[offset >> 2];
- trace_grlib_ahb_pnp_read(offset, val);
+ val = extract32(val, (4 - (offset & 3) - size) * 8, size * 8);
+ trace_grlib_ahb_pnp_read(offset, size, val);
return val;
}
@@ -152,7 +153,7 @@ static const MemoryRegionOps grlib_ahb_pnp_ops = {
.write = grlib_ahb_pnp_write,
.endianness = DEVICE_BIG_ENDIAN,
.impl = {
- .min_access_size = 4,
+ .min_access_size = 1,
.max_access_size = 4,
},
};
@@ -247,7 +248,8 @@ static uint64_t grlib_apb_pnp_read(void *opaque, hwaddr offset, unsigned size)
uint32_t val;
val = apb_pnp->regs[offset >> 2];
- trace_grlib_apb_pnp_read(offset, val);
+ val = extract32(val, (4 - (offset & 3) - size) * 8, size * 8);
+ trace_grlib_apb_pnp_read(offset, size, val);
return val;
}
@@ -263,7 +265,7 @@ static const MemoryRegionOps grlib_apb_pnp_ops = {
.write = grlib_apb_pnp_write,
.endianness = DEVICE_BIG_ENDIAN,
.impl = {
- .min_access_size = 4,
+ .min_access_size = 1,
.max_access_size = 4,
},
};
diff --git a/hw/misc/trace-events b/hw/misc/trace-events
index 4d51a80de1d..c18bc0605e8 100644
--- a/hw/misc/trace-events
+++ b/hw/misc/trace-events
@@ -247,8 +247,8 @@ via1_adb_poll(uint8_t data, const char *vadbint, int status, int index, int size
via1_auxmode(int mode) "setting auxmode to %d"
# grlib_ahb_apb_pnp.c
-grlib_ahb_pnp_read(uint64_t addr, uint32_t value) "AHB PnP read addr:0x%03"PRIx64" data:0x%08x"
-grlib_apb_pnp_read(uint64_t addr, uint32_t value) "APB PnP read addr:0x%03"PRIx64" data:0x%08x"
+grlib_ahb_pnp_read(uint64_t addr, unsigned size, uint32_t value) "AHB PnP read addr:0x%03"PRIx64" size:%u data:0x%08x"
+grlib_apb_pnp_read(uint64_t addr, unsigned size, uint32_t value) "APB PnP read addr:0x%03"PRIx64" size:%u data:0x%08x"
# led.c
led_set_intensity(const char *color, const char *desc, uint8_t intensity_percent) "LED desc:'%s' color:%s intensity: %u%%"
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* RE: [PATCH for-7.1] hw/misc/grlib_ahb_apb_pnp: Support 8 and 16 bit accesses
2022-08-02 13:19 [PATCH for-7.1] hw/misc/grlib_ahb_apb_pnp: Support 8 and 16 bit accesses Peter Maydell
@ 2022-08-02 14:20 ` Konrad, Frederic
2022-08-02 14:33 ` Peter Maydell
0 siblings, 1 reply; 6+ messages in thread
From: Konrad, Frederic @ 2022-08-02 14:20 UTC (permalink / raw)
To: Peter Maydell, qemu-devel; +Cc: Fabien Chouteau, Frederic Konrad, f4bug
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?
Regards,
Fred
> ---
> hw/misc/grlib_ahb_apb_pnp.c | 10 ++++++----
> hw/misc/trace-events | 4 ++--
> 2 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/hw/misc/grlib_ahb_apb_pnp.c b/hw/misc/grlib_ahb_apb_pnp.c
> index 43e001c3c7b..5b05f158592 100644
> --- a/hw/misc/grlib_ahb_apb_pnp.c
> +++ b/hw/misc/grlib_ahb_apb_pnp.c
> @@ -136,7 +136,8 @@ static uint64_t grlib_ahb_pnp_read(void *opaque,
> hwaddr offset, unsigned size)
> uint32_t val;
>
> val = ahb_pnp->regs[offset >> 2];
> - trace_grlib_ahb_pnp_read(offset, val);
> + val = extract32(val, (4 - (offset & 3) - size) * 8, size * 8);
> + trace_grlib_ahb_pnp_read(offset, size, val);
>
> return val;
> }
> @@ -152,7 +153,7 @@ static const MemoryRegionOps grlib_ahb_pnp_ops =
> {
> .write = grlib_ahb_pnp_write,
> .endianness = DEVICE_BIG_ENDIAN,
> .impl = {
> - .min_access_size = 4,
> + .min_access_size = 1,
> .max_access_size = 4,
> },
> };
> @@ -247,7 +248,8 @@ static uint64_t grlib_apb_pnp_read(void *opaque,
> hwaddr offset, unsigned size)
> uint32_t val;
>
> val = apb_pnp->regs[offset >> 2];
> - trace_grlib_apb_pnp_read(offset, val);
> + val = extract32(val, (4 - (offset & 3) - size) * 8, size * 8);
> + trace_grlib_apb_pnp_read(offset, size, val);
>
> return val;
> }
> @@ -263,7 +265,7 @@ static const MemoryRegionOps grlib_apb_pnp_ops =
> {
> .write = grlib_apb_pnp_write,
> .endianness = DEVICE_BIG_ENDIAN,
> .impl = {
> - .min_access_size = 4,
> + .min_access_size = 1,
> .max_access_size = 4,
> },
> };
> diff --git a/hw/misc/trace-events b/hw/misc/trace-events
> index 4d51a80de1d..c18bc0605e8 100644
> --- a/hw/misc/trace-events
> +++ b/hw/misc/trace-events
> @@ -247,8 +247,8 @@ via1_adb_poll(uint8_t data, const char *vadbint, int
> status, int index, int size
> via1_auxmode(int mode) "setting auxmode to %d"
>
> # grlib_ahb_apb_pnp.c
> -grlib_ahb_pnp_read(uint64_t addr, uint32_t value) "AHB PnP read
> addr:0x%03"PRIx64" data:0x%08x"
> -grlib_apb_pnp_read(uint64_t addr, uint32_t value) "APB PnP read
> addr:0x%03"PRIx64" data:0x%08x"
> +grlib_ahb_pnp_read(uint64_t addr, unsigned size, uint32_t value) "AHB PnP
> read addr:0x%03"PRIx64" size:%u data:0x%08x"
> +grlib_apb_pnp_read(uint64_t addr, unsigned size, uint32_t value) "APB PnP
> read addr:0x%03"PRIx64" size:%u data:0x%08x"
>
> # led.c
> led_set_intensity(const char *color, const char *desc, uint8_t
> intensity_percent) "LED desc:'%s' color:%s intensity: %u%%"
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH for-7.1] hw/misc/grlib_ahb_apb_pnp: Support 8 and 16 bit accesses
2022-08-02 14:20 ` Konrad, Frederic
@ 2022-08-02 14:33 ` Peter Maydell
2022-08-03 21:24 ` Philippe Mathieu-Daudé via
2022-08-08 9:15 ` Konrad, Frederic
0 siblings, 2 replies; 6+ messages in thread
From: Peter Maydell @ 2022-08-02 14:33 UTC (permalink / raw)
To: Konrad, Frederic; +Cc: qemu-devel, Fabien Chouteau, Frederic Konrad, f4bug
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.
-- PMM
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH for-7.1] hw/misc/grlib_ahb_apb_pnp: Support 8 and 16 bit accesses
2022-08-02 14:33 ` Peter Maydell
@ 2022-08-03 21:24 ` Philippe Mathieu-Daudé via
2022-08-08 9:15 ` Konrad, Frederic
1 sibling, 0 replies; 6+ messages in thread
From: Philippe Mathieu-Daudé via @ 2022-08-03 21:24 UTC (permalink / raw)
To: Peter Maydell
Cc: Konrad, Frederic, qemu-devel, Fabien Chouteau, Frederic Konrad,
Andrew Jeffery, Joel Stanley, Cédric Le Goater
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH for-7.1] hw/misc/grlib_ahb_apb_pnp: Support 8 and 16 bit accesses
2022-08-02 14:33 ` Peter Maydell
2022-08-03 21:24 ` Philippe Mathieu-Daudé via
@ 2022-08-08 9:15 ` Konrad, Frederic
2022-08-08 21:46 ` Philippe Mathieu-Daudé via
1 sibling, 1 reply; 6+ messages in thread
From: Konrad, Frederic @ 2022-08-08 9:15 UTC (permalink / raw)
To: Peter Maydell; +Cc: qemu-devel, Fabien Chouteau, Frederic Konrad, f4bug
> -----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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH for-7.1] hw/misc/grlib_ahb_apb_pnp: Support 8 and 16 bit accesses
2022-08-08 9:15 ` Konrad, Frederic
@ 2022-08-08 21:46 ` Philippe Mathieu-Daudé via
0 siblings, 0 replies; 6+ messages in thread
From: Philippe Mathieu-Daudé via @ 2022-08-08 21:46 UTC (permalink / raw)
To: Konrad, Frederic, Mark Cave-Ayland
Cc: Peter Maydell, qemu-devel, Fabien Chouteau, Frederic Konrad
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-08-08 21:47 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-02 13:19 [PATCH for-7.1] hw/misc/grlib_ahb_apb_pnp: Support 8 and 16 bit accesses Peter Maydell
2022-08-02 14:20 ` Konrad, Frederic
2022-08-02 14:33 ` Peter Maydell
2022-08-03 21:24 ` Philippe Mathieu-Daudé via
2022-08-08 9:15 ` Konrad, Frederic
2022-08-08 21:46 ` Philippe Mathieu-Daudé via
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.