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

[Blue Swirl]

On Sun, Mar 4, 2012 at 14:23, Michael S. Tsirkin <address@hidden> wrote:
> On Sun, Mar 04, 2012 at 01:38:38PM +0000, Blue Swirl wrote:
>> On Sun, Mar 4, 2012 at 13:28, Michael S. Tsirkin <address@hidden> wrote:
>> > On Sun, Mar 04, 2012 at 12:37:57PM +0000, Blue Swirl wrote:
>> >> On Sun, Mar 4, 2012 at 12:21, Michael S. Tsirkin <address@hidden> 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 <address@hidden> 
>> >> >> 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 <address@hidden>
>> >> >> > Signed-off-by: Michael S. Tsirkin <address@hidden>
>> >> >> >
>> >> >> > 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.
>> >
>> > Invalid read and address after block are also worrying.
>> >
>> > irqs are allocated with
>> >  #define MAX_PILS 16
>> >
>> >    irq = qemu_allocate_irqs(cpu_set_irq, env, MAX_PILS);
>> >
>> > then passed to apb:
>> >
>> >    pci_bus = pci_apb_init(APB_SPECIAL_BASE, APB_MEM_BASE, irq, &pci_bus2,
>> >                           &pci_bus3);
>> >
>> > which does:
>> > PCIBus *pci_apb_init(target_phys_addr_t special_base,
>> >                     target_phys_addr_t mem_base,
>> >                     qemu_irq *pic, PCIBus **bus2, PCIBus **bus3)
>> >
>> > and
>> >
>> >   for (i = 0; i < 32; i++) {
>> >        sysbus_connect_irq(s, i, pic[i]);
>> >    }
>>
>> Awful. But using 32 for MAX_PILS does not help either.
>
>
> Could you please clarify what is the SABRE device?
> Is it, in fact, a bridge device? Or not?

Yes, it's the host bridge, also known as PBM. It's documented in
UltraSPARC IIi User's Manual and there it says that the device is
found in the configuration space.

The secondary bridges are Simbas and should be called APBs.

>
>
> --
> MST