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

[Blue Swirl]

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.

I get this with a modified OpenBIOS and no secondary bridges:
OpenBIOS for Sparc64
Initializing PCI host bridge...
pci_host_set_reg reg 000001FE 00000000 00000000 02000000

=== CHANGED === package path old=/pci new=/address@hidden,0
0:0.0 - 108e:a000 - host bridge found - /address@hidden,0 -
pci_host_set_reg reg 000001FE 00000000 00000000 02000000
scanning new pci bus 0 under bridge /address@hidden,0

Scanning bus 0 at /address@hidden,0...
0:0.0 - 108e:a000 - host bridge found - /address@hidden,0 - host bridge
already configured
0:1.0 - 1234:1111 - /address@hidden,0/QEMU,VGA -
ob_pci_encode_unit space=0 dev=1 fn=0 buf=1

=== CHANGED === package path old=/address@hidden,0/QEMU,VGA 
new=/address@hidden,0/QEMU,address@hidden
pci_set_reg reg 00000800 00000000 00000000 00000000 00000000 42000810
00000000 00000000 00000000 00800000
ob_pci_decode_unit idx=00000000ffe8be08
ob_pci_decode_unit idx=00000000ffe8be08 addr=0000000000000000
0000000000000000 0000000000000800
0:2.0 - 108e:1000 - /address@hidden,0/ebus -
ob_pci_encode_unit space=0 dev=2 fn=0 buf=2

=== CHANGED === package path old=/address@hidden,0/ebus 
new=/address@hidden,0/address@hidden
pci_set_reg reg 00001000 00000000 00000000 00000000 00000000 02001010
00000000 00000000 00000000 01000000 02001014 00000000 00000000
00000000 00800000
ob_pci_decode_unit idx=00000000ffe29248
ob_pci_decode_unit idx=00000000ffe29248 addr=0000000000000000
0000000000000000 0000000000001000
ob_pci_decode_unit idx=00000000ffe29248
ob_pci_decode_unit idx=00000000ffe29248 addr=0000000000000000
0000000000000000 0000000000001000
ob_pci_decode_unit idx=00000000ffe29248
ob_pci_decode_unit idx=00000000ffe29248 addr=0000000000000000
0000000000000000 0000000000001000
ob_pci_decode_unit idx=00000000ffe29248
ob_pci_decode_unit idx=00000000ffe29248 addr=0000000000000000
0000000000000000 0000000000001000
ob_pci_decode_unit idx=00000000ffe29248
ob_pci_decode_unit idx=00000000ffe29248 addr=0000000000000000
0000000000000000 0000000000001000
ob_pci_decode_unit idx=00000000ffe29248
ob_pci_decode_unit idx=00000000ffe29248 addr=0000000000000000
0000000000000000 0000000000001000
ob_pci_decode_unit idx=00000000ffe8bf38
ob_pci_decode_unit idx=00000000ffe8bf38 addr=0000000000000000
0000000000000000 0000000000001000
0:3.0 - 10ec:8029 - /address@hidden,0/NE2000 -
ob_pci_encode_unit space=0 dev=3 fn=0 buf=3

=== CHANGED === package path old=/address@hidden,0/NE2000 
new=/address@hidden,0/address@hidden
pci_set_reg reg 00001800 00000000 00000000 00000000 00000000 01001810
00000000 00000000 00000000 00000100
0:4.0 - 1095:646 - /address@hidden,0/pci-ata -
ob_pci_encode_unit space=0 dev=4 fn=0 buf=4

=== CHANGED === package path old=/address@hidden,0/pci-ata 
new=/address@hidden,0/address@hidden
pci_set_reg reg 00002000 00000000 00000000 00000000 00000000 01002010
00000000 00000000 00000000 00000008 01002014 00000000 00000000
00000000 00000004 01002018 00000000 00000000 00000000 00000008
0100201C 00000000 00000000 00000000 00000004 01002020 00000000
00000000 00000000 00000010
Unhandled Exception 0x0000000000000032
PC = 0x00000000ffd1a1e4 NPC = 0x00000000ffd1a1e8
Stopping execution
QEMU 1.0.50 monitor - type 'help' for more information

>
>
>> >> 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