Re: [Qemu-devel] [PATCH] pci: fix bridge IO/BASE

["Michael S. Tsirkin" <mst@redhat.com>]

On Sun, Mar 04, 2012 at 12:37:57PM +0000, Blue Swirl wrote:
> On Sun, Mar 4, 2012 at 12:21, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Sun, Mar 04, 2012 at 10:27:24AM +0000, Blue Swirl wrote:
> >> On Sun, Mar 4, 2012 at 09:46, Michael S. Tsirkin <mst@redhat.com> wrote:
> >> > commit 5caef97a16010f818ea8b950e2ee24ba876643ad introduced
> >> > a regression: we do not make IO base/limit upper 16
> >> > bit registers writeable, so we should report a 16 bit
> >> > IO range type, not a 32 bit one.
> >> > Note that PCI_PREF_RANGE_TYPE_32 is 0x0, but PCI_IO_RANGE_TYPE_32 is 0x1.
> >> >
> >> > In particular, this broke sparc64.
> >> >
> >> > Note: this just reverts to behaviour prior to the patch.
> >> > Making PCI_IO_BASE_UPPER16 and PCI_IO_LIMIT_UPPER16
> >> > registers writeable should, and seems to, work just as well, but
> >> > as no system seems to actually be interested in 32 bit IO,
> >> > let's not make unnecessary changes.
> >> >
> >> > Reported-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> >> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >> >
> >> > Mark, can you confirm that this fixes the bug for you?
> >>
> >> No, running
> >> qemu-system-sparc64 -serial stdio
> >> still shows black screen and the following on console:
> >> OpenBIOS for Sparc64
> >> Unhandled Exception 0x0000000000000032
> >> PC = 0x00000000ffd19e18 NPC = 0x00000000ffd19e1c
> >> Stopping execution
> >
> > The weird thing is the range type does not seem to be accessed
> > at all. So I guessed there's some memory corruption here.
> > Running valgrind shows this:
> >
> > --11114-- WARNING: unhandled syscall: 340
> > --11114-- You may be able to write your own handler.
> > --11114-- Read the file README_MISSING_SYSCALL_OR_IOCTL.
> > --11114-- Nevertheless we consider this a bug.  Please report
> > --11114-- it at http://valgrind.org/support/bug_reports.html.
> > ==11114== Invalid read of size 4
> > ==11114==    at 0x2A68C0: pci_apb_init (apb_pci.c:350)
> > ==11114==    by 0x2F7A84: sun4uv_init (sun4u.c:779)
> > ==11114==    by 0x13D716: main (vl.c:3397)
> > ==11114==  Address 0x156c7d30 is 0 bytes after a block of size 64
> > alloc'd
> > ==11114==    at 0x557DD69: malloc (vg_replace_malloc.c:236)
> > ==11114==    by 0x225F56: malloc_and_trace (vl.c:2156)
> > ==11114==    by 0x584AFEC: ??? (in /lib/libglib-2.0.so.0.2800.8)
> > ==11114==    by 0x584B528: g_malloc0 (in /lib/libglib-2.0.so.0.2800.8)
> > ==11114==    by 0x19C50C: qemu_allocate_irqs (irq.c:47)
> > ==11114==    by 0x2F7A4C: sun4uv_init (sun4u.c:778)
> > ==11114==    by 0x13D716: main (vl.c:3397)
> > ==11114==
> > apb: here
> > ==11114== Warning: client switching stacks?  SP change: 0xfec42cbc -->
> > 0x16894008
> > ==11114==          to suppress, use: --max-stackframe=398791500 or
> > greater
> > ==11114== Warning: client switching stacks?  SP change: 0x16893fa0 -->
> > 0xfec42cc0
> > ==11114==          to suppress, use: --max-stackframe=398791392 or
> > greater
> > ==11114== Warning: client switching stacks?  SP change: 0xfec42fe0 -->
> > 0x16893fd0
> > ==11114==          to suppress, use: --max-stackframe=398790640 or
> > greater
> > ==11114==          further instances of this message will not be shown.
> > QEMU 1.0.50 monitor - type 'help' for more information
> > (qemu) ==11114== Thread 2:
> > ==11114== Conditional jump or move depends on uninitialised value(s)
> > ==11114==    at 0x2A8351: compute_all_sub (cc_helper.c:37)
> > ==11114==    by 0x2A8782: helper_compute_psr (cc_helper.c:470)
> > ==11114==    by 0x9AD9A19: ???
> > ==11114==
> > ==11114== Conditional jump or move depends on uninitialised value(s)
> > ==11114==    at 0x2A827C: compute_all_sub_xcc (cc_helper.c:60)
> > ==11114==    by 0x2A8795: helper_compute_psr (cc_helper.c:473)
> > ==11114==    by 0x9AD9A19: ???
> > ==11114==
> > ==11114== Conditional jump or move depends on uninitialised value(s)
> > ==11114==    at 0x2A8296: compute_all_sub_xcc (cc_helper.c:295)
> > ==11114==    by 0x2A8795: helper_compute_psr (cc_helper.c:473)
> > ==11114==    by 0x9AD9A19: ???
> > ==11114==
> >
> > Is the above a problem?
> 
> It looks like Sparc does not reset registers at CPU reset. Nice catch.

The following is likely also needed, maybe the define should be shared
with apb.

diff --git a/hw/sun4u.c b/hw/sun4u.c
index 423108f..19a135a 100644
--- a/hw/sun4u.c
+++ b/hw/sun4u.c
@@ -81,7 +81,7 @@
 #define FW_CFG_SPARC64_HEIGHT (FW_CFG_ARCH_LOCAL + 0x01)
 #define FW_CFG_SPARC64_DEPTH (FW_CFG_ARCH_LOCAL + 0x02)
 
-#define MAX_PILS 16
+#define MAX_PILS 32
 
 #define TICK_MAX             0x7fffffffffffffffULL