hw/display/tcx.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-)
The S24/TCX datasheet is listed as "Unable to locate" on [1].
However the NetBSD revision 1.32 of the driver introduced
64-bit accesses to the stippler and blitter [2]. It is safe
to assume these memory regions are 64-bit accessible.
QEMU implementation is 32-bit, so fill the 'impl' fields.
[1] http://web.archive.org/web/20111209011516/http://wikis.sun.com/display/FOSSdocs/Home
[2] http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/dev/sbus/tcx.c.diff?r1=1.31&r2=1.32
Reported-by: Andreas Gustafsson <gson@gson.org>
Buglink: https://bugs.launchpad.net/bugs/1892540
Fixes: 55d7bfe2293 ("tcx: Implement hardware acceleration")
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
Since v1:
- added missing uncommitted staged changes... (tcx_blit_ops)
---
hw/display/tcx.c | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/hw/display/tcx.c b/hw/display/tcx.c
index 1fb45b1aab8..96c6898b149 100644
--- a/hw/display/tcx.c
+++ b/hw/display/tcx.c
@@ -548,20 +548,28 @@ static const MemoryRegionOps tcx_stip_ops = {
.read = tcx_stip_readl,
.write = tcx_stip_writel,
.endianness = DEVICE_NATIVE_ENDIAN,
- .valid = {
+ .impl = {
.min_access_size = 4,
.max_access_size = 4,
},
+ .valid = {
+ .min_access_size = 4,
+ .max_access_size = 8,
+ },
};
static const MemoryRegionOps tcx_rstip_ops = {
.read = tcx_stip_readl,
.write = tcx_rstip_writel,
.endianness = DEVICE_NATIVE_ENDIAN,
- .valid = {
+ .impl = {
.min_access_size = 4,
.max_access_size = 4,
},
+ .valid = {
+ .min_access_size = 4,
+ .max_access_size = 8,
+ },
};
static uint64_t tcx_blit_readl(void *opaque, hwaddr addr,
@@ -650,10 +658,14 @@ static const MemoryRegionOps tcx_rblit_ops = {
.read = tcx_blit_readl,
.write = tcx_rblit_writel,
.endianness = DEVICE_NATIVE_ENDIAN,
- .valid = {
+ .impl = {
.min_access_size = 4,
.max_access_size = 4,
},
+ .valid = {
+ .min_access_size = 4,
+ .max_access_size = 8,
+ },
};
static void tcx_invalidate_cursor_position(TCXState *s)
--
2.26.2
On 8/22/20 7:21 AM, Philippe Mathieu-Daudé wrote:
> The S24/TCX datasheet is listed as "Unable to locate" on [1].
>
> However the NetBSD revision 1.32 of the driver introduced
> 64-bit accesses to the stippler and blitter [2]. It is safe
> to assume these memory regions are 64-bit accessible.
> QEMU implementation is 32-bit, so fill the 'impl' fields.
>
> [1] http://web.archive.org/web/20111209011516/http://wikis.sun.com/display/FOSSdocs/Home
> [2] http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/dev/sbus/tcx.c.diff?r1=1.31&r2=1.32
>
> Reported-by: Andreas Gustafsson <gson@gson.org>
> Buglink: https://bugs.launchpad.net/bugs/1892540
> Fixes: 55d7bfe2293 ("tcx: Implement hardware acceleration")
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> Since v1:
> - added missing uncommitted staged changes... (tcx_blit_ops)
> ---
> hw/display/tcx.c | 18 +++++++++++++++---
> 1 file changed, 15 insertions(+), 3 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
Hello, since I wrote the NetBSD code in question, here are my 2 cent: On Sat, 29 Aug 2020 08:41:43 -0700 Richard Henderson <richard.henderson@linaro.org> wrote: > On 8/22/20 7:21 AM, Philippe Mathieu-Daudé wrote: > > The S24/TCX datasheet is listed as "Unable to locate" on [1]. I don't have it either, but someone did a lot of reverse engineering and gave me his notes. The hardware isn't that complicated, but quite weird. > > However the NetBSD revision 1.32 of the driver introduced > > 64-bit accesses to the stippler and blitter [2]. It is safe > > to assume these memory regions are 64-bit accessible. > > QEMU implementation is 32-bit, so fill the 'impl' fields. IIRC the real hardware *requires* 64bit accesses for stipple and blitter operations to work. For stipples you write a 64bit word into STIP space, the address defines where in the framebuffer you want to draw, the data contain a 32bit bitmask, foreground colour and a ROP. BLIT space works similarly, the 64bit word contains an offset were to read pixels from, and how many you want to copy. have fun Michael
Le sam. 29 août 2020 18:14, Michael <macallan1888@gmail.com> a écrit : > Hello, > > since I wrote the NetBSD code in question, here are my 2 cent: > > On Sat, 29 Aug 2020 08:41:43 -0700 > Richard Henderson <richard.henderson@linaro.org> wrote: > > > On 8/22/20 7:21 AM, Philippe Mathieu-Daudé wrote: > > > The S24/TCX datasheet is listed as "Unable to locate" on [1]. > > I don't have it either, but someone did a lot of reverse engineering > and gave me his notes. The hardware isn't that complicated, but quite > weird. > > > > However the NetBSD revision 1.32 of the driver introduced > > > 64-bit accesses to the stippler and blitter [2]. It is safe > > > to assume these memory regions are 64-bit accessible. > > > QEMU implementation is 32-bit, so fill the 'impl' fields. > > IIRC the real hardware *requires* 64bit accesses for stipple and > blitter operations to work. For stipples you write a 64bit word into > STIP space, the address defines where in the framebuffer you want to > draw, the data contain a 32bit bitmask, foreground colour and a ROP. > BLIT space works similarly, the 64bit word contains an offset were to > read pixels from, and how many you want to copy. > Thanks Michael for this information! If you don't mind I'll amend it to the commit description so there is a reference for posterity. I'm waiting for *Andreas Gustafsson to test it then will repost.* > have fun > Michael >
Hello, On Sat, 29 Aug 2020 18:45:06 +0200 Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > > > However the NetBSD revision 1.32 of the driver introduced > > > > 64-bit accesses to the stippler and blitter [2]. It is safe > > > > to assume these memory regions are 64-bit accessible. > > > > QEMU implementation is 32-bit, so fill the 'impl' fields. > > > > IIRC the real hardware *requires* 64bit accesses for stipple and > > blitter operations to work. For stipples you write a 64bit word into > > STIP space, the address defines where in the framebuffer you want to > > draw, the data contain a 32bit bitmask, foreground colour and a ROP. > > BLIT space works similarly, the 64bit word contains an offset were to > > read pixels from, and how many you want to copy. > > > > Thanks Michael for this information! > If you don't mind I'll amend it to the commit description so there is a > reference for posterity. One more thing since there seems to be some confusion - 64bit accesses on the framebuffer are fine as well. TCX/S24 is *not* an SBus device, even though its node says it is. S24 is a card that plugs into a special slot on the SS5 mainboard, which is shared with an SBus slot and looks a lot like a horizontal UPA slot. Both S24 and TCX are accessed through the Micro/TurboSPARC's AFX bus which is 64bit wide and intended for graphics. Early FFB docs even mentioned connecting to both AFX and UPA, no idea if that was ever realized in hardware though. have fun Michael
On 29/08/2020 17:45, Philippe Mathieu-Daudé wrote:
> Le sam. 29 août 2020 18:14, Michael <macallan1888@gmail.com
> <mailto:macallan1888@gmail.com>> a écrit :
>
> Hello,
>
> since I wrote the NetBSD code in question, here are my 2 cent:
>
> On Sat, 29 Aug 2020 08:41:43 -0700
> Richard Henderson <richard.henderson@linaro.org
> <mailto:richard.henderson@linaro.org>> wrote:
>
> > On 8/22/20 7:21 AM, Philippe Mathieu-Daudé wrote:
> > > The S24/TCX datasheet is listed as "Unable to locate" on [1].
>
> I don't have it either, but someone did a lot of reverse engineering
> and gave me his notes. The hardware isn't that complicated, but quite
> weird.
>
> > > However the NetBSD revision 1.32 of the driver introduced
> > > 64-bit accesses to the stippler and blitter [2]. It is safe
> > > to assume these memory regions are 64-bit accessible.
> > > QEMU implementation is 32-bit, so fill the 'impl' fields.
>
> IIRC the real hardware *requires* 64bit accesses for stipple and
> blitter operations to work. For stipples you write a 64bit word into
> STIP space, the address defines where in the framebuffer you want to
> draw, the data contain a 32bit bitmask, foreground colour and a ROP.
> BLIT space works similarly, the 64bit word contains an offset were to
> read pixels from, and how many you want to copy.
>
>
> Thanks Michael for this information!
> If you don't mind I'll amend it to the commit description so there is a reference for
> posterity.
>
> I'm waiting for /Andreas Gustafsson to test it then will repost.
Hi Philippe,
Thanks for coming up with this patch! Looks fine to me, just wondering if it should
have a "Fixes: 5d971f9e67 ("memory: Revert "memory: accept mismatching sizes in
memory_region_access_valid"") tag rather than the original commit since that's how
other bugs exposed by that commit have been tagged?
ATB,
Mark.
On 8/30/20 9:32 AM, Mark Cave-Ayland wrote:
> On 29/08/2020 17:45, Philippe Mathieu-Daudé wrote:
>
>> Le sam. 29 août 2020 18:14, Michael <macallan1888@gmail.com
>> <mailto:macallan1888@gmail.com>> a écrit :
>>
>> Hello,
>>
>> since I wrote the NetBSD code in question, here are my 2 cent:
>>
>> On Sat, 29 Aug 2020 08:41:43 -0700
>> Richard Henderson <richard.henderson@linaro.org
>> <mailto:richard.henderson@linaro.org>> wrote:
>>
>> > On 8/22/20 7:21 AM, Philippe Mathieu-Daudé wrote:
>> > > The S24/TCX datasheet is listed as "Unable to locate" on [1].
>>
>> I don't have it either, but someone did a lot of reverse engineering
>> and gave me his notes. The hardware isn't that complicated, but quite
>> weird.
>>
>> > > However the NetBSD revision 1.32 of the driver introduced
>> > > 64-bit accesses to the stippler and blitter [2]. It is safe
>> > > to assume these memory regions are 64-bit accessible.
>> > > QEMU implementation is 32-bit, so fill the 'impl' fields.
>>
>> IIRC the real hardware *requires* 64bit accesses for stipple and
>> blitter operations to work. For stipples you write a 64bit word into
>> STIP space, the address defines where in the framebuffer you want to
>> draw, the data contain a 32bit bitmask, foreground colour and a ROP.
>> BLIT space works similarly, the 64bit word contains an offset were to
>> read pixels from, and how many you want to copy.
>>
>>
>> Thanks Michael for this information!
>> If you don't mind I'll amend it to the commit description so there is a reference for
>> posterity.
>>
>> I'm waiting for /Andreas Gustafsson to test it then will repost.
>
> Hi Philippe,
>
> Thanks for coming up with this patch! Looks fine to me, just wondering if it should
> have a "Fixes: 5d971f9e67 ("memory: Revert "memory: accept mismatching sizes in
> memory_region_access_valid"") tag rather than the original commit since that's how
> other bugs exposed by that commit have been tagged?
I don't think so, the bug was present (hidden) *before* 5d971f9e67 and
we were incorrectly modelling it. I just posted a v3 including Michael
valuable memories :)
>
>
> ATB,
>
> Mark.
>
On Sat, Aug 22, 2020 at 02:21:27PM -0000, Philippe Mathieu-Daudé wrote:
> The S24/TCX datasheet is listed as "Unable to locate" on [1].
>
> However the NetBSD revision 1.32 of the driver introduced
> 64-bit accesses to the stippler and blitter [2]. It is safe
> to assume these memory regions are 64-bit accessible.
> QEMU implementation is 32-bit, so fill the 'impl' fields.
>
> [1] http://web.archive.org/web/20111209011516/http://wikis.sun.com/display/FOSSdocs/Home
> [2] http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/dev/sbus/tcx.c.diff?r1=1.31&r2=1.32
>
> Reported-by: Andreas Gustafsson <gson@gson.org>
> Buglink: https://bugs.launchpad.net/bugs/1892540
> Fixes: 55d7bfe2293 ("tcx: Implement hardware acceleration")
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Philippe, did you submit the patch on the mailing list
normally too? I don't seem to see it there.
the patch seems to work for me:
Tested-by: Michael S. Tsirkin <mst@redhat.com>
CC Nathan who reported a similar failure.
Nathan, does the patch below fix the issue for you?
> ---
> Since v1:
> - added missing uncommitted staged changes... (tcx_blit_ops)
> ---
hw/display/tcx.c | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/hw/display/tcx.c b/hw/display/tcx.c
index 1fb45b1aab8..96c6898b149 100644
--- a/hw/display/tcx.c
+++ b/hw/display/tcx.c
@@ -548,20 +548,28 @@ static const MemoryRegionOps tcx_stip_ops = {
.read = tcx_stip_readl,
.write = tcx_stip_writel,
.endianness = DEVICE_NATIVE_ENDIAN,
- .valid = {
+ .impl = {
.min_access_size = 4,
.max_access_size = 4,
},
+ .valid = {
+ .min_access_size = 4,
+ .max_access_size = 8,
+ },
};
static const MemoryRegionOps tcx_rstip_ops = {
.read = tcx_stip_readl,
.write = tcx_rstip_writel,
.endianness = DEVICE_NATIVE_ENDIAN,
- .valid = {
+ .impl = {
.min_access_size = 4,
.max_access_size = 4,
},
+ .valid = {
+ .min_access_size = 4,
+ .max_access_size = 8,
+ },
};
static uint64_t tcx_blit_readl(void *opaque, hwaddr addr,
@@ -650,10 +658,14 @@ static const MemoryRegionOps tcx_rblit_ops = {
.read = tcx_blit_readl,
.write = tcx_rblit_writel,
.endianness = DEVICE_NATIVE_ENDIAN,
- .valid = {
+ .impl = {
.min_access_size = 4,
.max_access_size = 4,
},
+ .valid = {
+ .min_access_size = 4,
+ .max_access_size = 8,
+ },
};
static void tcx_invalidate_cursor_position(TCXState *s)
-----------------------------------------------------------
I think you shouldn't specify .min_access_size in impl, since
that also allows 1 and 2 byte accesses from guest.
> --
> 2.26.2
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1892540
>
> Title:
> qemu can no longer boot NetBSD/sparc
>
> Status in QEMU:
> New
>
> Bug description:
> Booting NetBSD/sparc in qemu no longer works. It broke between qemu
> version 5.0.0 and 5.1.0, and a bisection identified the following as
> the offending commit:
>
> [5d971f9e672507210e77d020d89e0e89165c8fc9] memory: Revert "memory:
> accept mismatching sizes in memory_region_access_valid"
>
> It's still broken as of 7fd51e68c34fcefdb4d6fd646ed3346f780f89f4.
>
> To reproduce, run
>
> wget http://ftp.netbsd.org/pub/NetBSD/NetBSD-9.0/images/NetBSD-9.0-sparc.iso
> qemu-system-sparc -nographic -cdrom NetBSD-9.0-sparc.iso -boot d
>
> The expected behavior is that the guest boots to the prompt
>
> Installation medium to load the additional utilities from:
>
> The observed behavior is a panic:
>
> [ 1.0000050] system[0]: trap 0x29: pc=0xf0046b14 sfsr=0xb6 sfva=0x54000000
> [ 1.0000050] cpu0: data fault: pc=0xf0046b14 addr=0x54000000 sfsr=0xb6<PERR=0x0,LVL=0x0,AT=0x5,FT=0x5,FAV,OW>
> [ 1.0000050] panic: kernel fault
> [ 1.0000050] halted
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/qemu/+bug/1892540/+subscriptions
Philippe Mathieu-Daudé wrote: > diff --git a/hw/display/tcx.c b/hw/display/tcx.c > index 1fb45b1aab8..96c6898b149 100644 With this patch, the kernel boots successfully for me. -- Andreas Gustafsson, gson@gson.org
On 8/30/20 8:59 AM, Andreas Gustafsson wrote: > Philippe Mathieu-Daudé wrote: >> diff --git a/hw/display/tcx.c b/hw/display/tcx.c >> index 1fb45b1aab8..96c6898b149 100644 > > With this patch, the kernel boots successfully for me. Thanks, can I add "Tested-by: Andreas Gustafsson <gson@gson.org>" to the patch?
© 2016 - 2024 Red Hat, Inc.