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

[Michael S. Tsirkin]

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]);
    }



> >> This unassigned memory exception is triggered because CMD646 IDE I/O
> >> registers are not accessible:
> >>
> >> (qemu) info pci
> >>   Bus  0, device   0, function 0:
> >>     Host bridge: PCI device 108e:a000
> >>       id ""
> >>   Bus  0, device   1, function 0:
> >>     PCI bridge: PCI device 108e:5000
> >>       BUS 0.
> >>       secondary bus 1.
> >>       subordinate bus 1.
> >>       IO range [0x0000, 0x0fff]
> >>       memory range [0x00000000, 0x000fffff]
> >>       prefetchable memory range [0x00000000, 0x000fffff]
> >>       id ""
> >>   Bus  0, device   1, function 1:
> >>     PCI bridge: PCI device 108e:5000
> >>       BUS 0.
> >>       secondary bus 2.
> >>       subordinate bus 2.
> >>       IO range [0x0000, 0x0fff]
> >>       memory range [0x00000000, 0x000fffff]
> >>       prefetchable memory range [0x00000000, 0x000fffff]
> >>       id ""
> >>   Bus  0, device   2, function 0:
> >>     VGA controller: PCI device 1234:1111
> >>       BAR0: 32 bit prefetchable memory at 0x00800000 [0x00ffffff].
> >>       BAR6: 32 bit memory at 0x01000000 [0x0100ffff].
> >>       id ""
> >>   Bus  0, device   3, function 0:
> >>     Bridge: PCI device 108e:1000
> >>       BAR0: 32 bit memory at 0x02000000 [0x02ffffff].
> >>       BAR1: 32 bit memory at 0x03000000 [0x037fffff].
> >>       id ""
> >>   Bus  0, device   4, function 0:
> >>     Ethernet controller: PCI device 10ec:8029
> >>       IRQ 0.
> >>       BAR0: I/O at 0xffffffffffffffff [0x00fe].
> >>       BAR6: 32 bit memory at 0x03800000 [0x0380ffff].
> >>       id ""
> >>   Bus  0, device   5, function 0:
> >>     IDE controller: PCI device 1095:0646
> >>       IRQ 1.
> >>       BAR0: I/O at 0xffffffffffffffff [0x0006].
> >>       BAR1: I/O at 0xffffffffffffffff [0x0002].
> >>       BAR2: I/O at 0xffffffffffffffff [0x0006].
> >>       BAR3: I/O at 0xffffffffffffffff [0x0002].
> >>       BAR4: I/O at 0xffffffffffffffff [0x000e].
> >>       id ""
> >> (qemu) info mtree
> >> memory
> >> 0000000000000000-7ffffffffffffffe (prio 0, RW): system
> >>   0000000000000000-0000000007ffffff (prio 0, RW): sun4u.ram
> >>   000001fe00000000-000001fe0000ffff (prio 0, RW): apb-config
> >>   000001fe01000000-000001fe01ffffff (prio 0, RW): apb-pci-config
> >>   000001fe02000000-000001fe0200ffff (prio 0, RW): apb-pci-ioport
> >>   000001ff00000000-000001ffffffffff (prio 0, RW): pci-mmio
> >>     000001ff00000000-000001ff000fffff (prio 1, RW): alias
> >> pci_bridge_mem @pci_bridge_pci 0000000000000000-00000000000fffff
> >>     000001ff00000000-000001ff000fffff (prio 1, RW): alias
> >> pci_bridge_pref_mem @pci_bridge_pci 0000000000000000-00000000000fffff
> >>     000001ff00000000-000001ff000fffff (prio 1, RW): alias
> >> pci_bridge_mem @pci_bridge_pci 0000000000000000-00000000000fffff
> >>     000001ff00000000-000001ff000fffff (prio 1, RW): alias
> >> pci_bridge_pref_mem @pci_bridge_pci 0000000000000000-00000000000fffff
> >>     000001ff000a0000-000001ff000affff (prio 2, RW): alias vga.chain4
> >> @vga.vram 0000000000000000-000000000000ffff
> >>     000001ff000a0000-000001ff000bffff (prio 1, RW): vga-lowmem
> >>     000001ff00800000-000001ff00ffffff (prio 1, RW): vga.vram
> >>     000001ff01000000-000001ff0100ffff (prio 1, RW): vga.rom
> >>     000001ff02000000-000001ff02ffffff (prio 1, RW): isa-mmio
> >>     000001ff03000000-000001ff037fffff (prio 1, RW): isa-mmio
> >>     000001ff03800000-000001ff0380ffff (prio 1, RW): ne2000.rom
> >>   000001fff0000000-000001fff03fffff (prio 0, R-): sun4u.prom
> >> pci_bridge_pci
> >> 0000000000000000-7ffffffffffffffe (prio 0, RW): pci_bridge_pci
> >> pci_bridge_pci
> >> 0000000000000000-7ffffffffffffffe (prio 0, RW): pci_bridge_pci
> >> vga.vram
> >> 0000000000800000-0000000000ffffff (prio 1, RW): vga.vram
> >> I/O
> >> 0000000000000000-000000000000ffff (prio 0, RW): io
> >>   0000000000000000-0000000000000fff (prio 1, RW): alias pci_bridge_io
> >> @pci_bridge_io 0000000000000000-0000000000000fff
> >>   0000000000000000-0000000000000fff (prio 1, RW): alias pci_bridge_io
> >> @pci_bridge_io 0000000000000000-0000000000000fff
> >>   0000000000000060-0000000000000060 (prio 0, RW): i8042-data
> >>   0000000000000064-0000000000000064 (prio 0, RW): i8042-cmd
> >>   0000000000000074-0000000000000077 (prio 0, RW): m48t59
> >>   00000000000001ce-00000000000001ce (prio 0, RW): alias vbe @vbe
> >> 00000000000001ce-00000000000001ce
> >>   00000000000001d0-00000000000001d0 (prio 0, RW): alias vbe @vbe
> >> 00000000000001d0-00000000000001d0
> >>   0000000000000378-000000000000037f (prio 0, RW): alias parallel
> >> @parallel 0000000000000378-000000000000037f
> >>   00000000000003b4-00000000000003b5 (prio 0, RW): alias vga @vga
> >> 00000000000003b4-00000000000003b5
> >>   00000000000003ba-00000000000003ba (prio 0, RW): alias vga @vga
> >> 00000000000003ba-00000000000003ba
> >>   00000000000003c0-00000000000003cf (prio 0, RW): alias vga @vga
> >> 00000000000003c0-00000000000003cf
> >>   00000000000003d4-00000000000003d5 (prio 0, RW): alias vga @vga
> >> 00000000000003d4-00000000000003d5
> >>   00000000000003da-00000000000003da (prio 0, RW): alias vga @vga
> >> 00000000000003da-00000000000003da
> >>   00000000000003f1-00000000000003f5 (prio 0, RW): alias fdc @fdc
> >> 00000000000003f1-00000000000003f5
> >>   00000000000003f7-00000000000003f7 (prio 0, RW): alias fdc @fdc
> >> 00000000000003f7-00000000000003f7
> >>   00000000000003f8-00000000000003ff (prio 0, RW): serial
> >>   0000000000000510-0000000000000511 (prio 0, RW): fwcfg
> >
> > And with type 32 range it looks like this:
> >
> > 0000000000000000-7ffffffffffffffe (prio 0, RW): system
> >  0000000000000000-0000000007ffffff (prio 0, RW): sun4u.ram
> >  000001fe00000000-000001fe0000ffff (prio 0, RW): apb-config
> >  000001fe01000000-000001fe01ffffff (prio 0, RW): apb-pci-config
> >  000001fe02000000-000001fe0200ffff (prio 0, RW): apb-pci-ioport
> >  000001ff00000000-000001ffffffffff (prio 0, RW): pci-mmio
> >    000001ff00000000-000001ff000fffff (prio 1, RW): alias pci_bridge_mem
> > @pci_bridge_pci 0000000000000000-00000000000fffff
> >    000001ff00000000-000001ff000fffff (prio 1, RW): alias
> > pci_bridge_pref_mem @pci_bridge_pci 0000000000000000-00000000000fffff
> >    000001ff00000000-000001ff000fffff (prio 1, RW): alias pci_bridge_mem
> > @pci_bridge_pci 0000000000000000-00000000000fffff
> >    000001ff00000000-000001ff000fffff (prio 1, RW): alias
> > pci_bridge_pref_mem @pci_bridge_pci 0000000000000000-00000000000fffff
> >    000001ff000a0000-000001ff000affff (prio 2, RW): alias vga.chain4
> > @vga.vram 0000000000000000-000000000000ffff
> >    000001ff000a0000-000001ff000bffff (prio 1, RW): vga-lowmem
> >    000001ff00800000-000001ff00ffffff (prio 1, RW): vga.vram
> >    000001ff01000000-000001ff0100ffff (prio 1, RW): vga.rom
> >    000001ff02000000-000001ff02ffffff (prio 1, RW): isa-mmio
> >    000001ff03000000-000001ff037fffff (prio 1, RW): isa-mmio
> >  000001fff0000000-000001fff03fffff (prio 0, R-): sun4u.prom
> > pci_bridge_pci
> > 0000000000000000-7ffffffffffffffe (prio 0, RW): pci_bridge_pci
> > pci_bridge_pci
> > 0000000000000000-7ffffffffffffffe (prio 0, RW): pci_bridge_pci
> > vga.vram
> > 0000000000800000-0000000000ffffff (prio 1, RW): vga.vram
> > I/O
> > 0000000000000000-000000000000ffff (prio 0, RW): io
> >  0000000000000000-0000000000000fff (prio 1, RW): alias pci_bridge_io
> > @pci_bridge_io 0000000000000000-0000000000000fff
> >  0000000000000000-0000000000000fff (prio 1, RW): alias pci_bridge_io
> > @pci_bridge_io 0000000000000000-0000000000000fff
> >  0000000000000060-0000000000000060 (prio 0, RW): i8042-data
> >  0000000000000064-0000000000000064 (prio 0, RW): i8042-cmd
> >  0000000000000074-0000000000000077 (prio 0, RW): m48t59
> >  00000000000001ce-00000000000001ce (prio 0, RW): vbe
> >  00000000000001d0-00000000000001d0 (prio 0, RW): vbe
> >  0000000000000378-000000000000037f (prio 0, RW): parallel
> >  00000000000003b4-00000000000003b5 (prio 0, RW): vga
> >  00000000000003ba-00000000000003ba (prio 0, RW): vga
> >  00000000000003c0-00000000000003cf (prio 0, RW): vga
> >  00000000000003d4-00000000000003d5 (prio 0, RW): vga
> >  00000000000003da-00000000000003da (prio 0, RW): vga
> >  00000000000003f1-00000000000003f5 (prio 0, RW): fdc
> >  00000000000003f7-00000000000003f7 (prio 0, RW): fdc
> >  00000000000003f8-00000000000003ff (prio 0, RW): serial
> >  0000000000000400-00000000000004ff (prio 1, RW): ne2000
> >  0000000000000500-0000000000000507 (prio 1, RW): cmd646-data
> >  0000000000000510-0000000000000511 (prio 0, RW): fwcfg
> >  0000000000000580-0000000000000583 (prio 1, RW): cmd646-cmd
> >  0000000000000600-0000000000000607 (prio 1, RW): cmd646-data
> >  0000000000000680-0000000000000683 (prio 1, RW): cmd646-cmd
> >  0000000000000700-000000000000070f (prio 1, RW): cmd646-bmdma
> >    0000000000000700-0000000000000703 (prio 0, RW): cmd646-bmdma-bus
> >    0000000000000704-0000000000000707 (prio 0, RW): cmd646-bmdma-ioport
> >    0000000000000708-000000000000070b (prio 0, RW): cmd646-bmdma-bus
> >    000000000000070c-000000000000070f (prio 0, RW): cmd646-bmdma-ioport
> 
> This looks better.
> 
> >
> > Sill trying to understand what all this means.
> >
> >
> >> >  hw/pci.c |    4 ++--
> >> >  1 files changed, 2 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/hw/pci.c b/hw/pci.c
> >> > index fee27fc..6d08cef 100644
> >> > --- a/hw/pci.c
> >> > +++ b/hw/pci.c
> >> > @@ -633,8 +633,8 @@ static void pci_init_mask_bridge(PCIDevice *d)
> >> >     memset(d->wmask + PCI_PREF_BASE_UPPER32, 0xff, 8);
> >> >
> >> >     /* Supported memory and i/o types */
> >> > -    d->config[PCI_IO_BASE] |= PCI_IO_RANGE_TYPE_32;
> >> > -    d->config[PCI_IO_LIMIT] |= PCI_IO_RANGE_TYPE_32;
> >> > +    d->config[PCI_IO_BASE] |= PCI_IO_RANGE_TYPE_16;
> >> > +    d->config[PCI_IO_LIMIT] |= PCI_IO_RANGE_TYPE_16;
> >> >     pci_word_test_and_set_mask(d->config + PCI_PREF_MEMORY_BASE,
> >> >                                PCI_PREF_RANGE_TYPE_64);
> >> >     pci_word_test_and_set_mask(d->config + PCI_PREF_MEMORY_LIMIT,
> >> > --
> >> > 1.7.9.111.gf3fb0