| Message ID | 20200822142127.1316231-1-f4bug@amsat.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [Bug,1892540,RFC,v2] hw/display/tcx: Allow 64-bit accesses to framebuffer stippler and blitter | expand |
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, 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
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.
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 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?
On 8/30/20 8:18 AM, mst@redhat.com wrote: > 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. Yes, Message-id: <20200822142127.1316231-1-f4bug@amsat.org> https://www.mail-archive.com/qemu-devel@nongnu.org/msg732515.html > > the patch seems to work for me: > > Tested-by: Michael S. Tsirkin <mst@redhat.com> Thanks! > > > 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: > Thanks, can I add "Tested-by: Andreas Gustafsson <gson@gson.org>" > to the patch? Fine by me.
On 01/09/2020 11:04, Andreas Gustafsson wrote: > Philippe Mathieu-Daudé wrote: >> Thanks, can I add "Tested-by: Andreas Gustafsson <gson@gson.org>" >> to the patch? > > Fine by me. I've added the above Tested-by tag (and also that from MST) and applied this to my qemu-sparc branch. 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. >
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)
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(-)