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

[Blue Swirl]

On Sun, Mar 4, 2012 at 12:33, 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
>>
>> This unassigned memory exception is triggered because CMD646 IDE I/O
>> registers are not accessible:
>
> So here's a theory: the issue is that both device IO
> and bridge IO have the same priority.
> Bridge is initialized with IO at 0 to 4K
> and so we have two devices at the same priority.
> And flipping a type bit affects this,  by chance?
>
> I tried the following and it seems to help.
> But the real fix IMO is to either disable the bridge ranges
> at reset, in hardware, or have the BIOS do this.

I'd suppose bridge ranges should be disabled at reset. BIOS should
probably also program the bridge ranges so that only devices behind
the bridge are visible, or if no devices exist, disable the bridge
completely. It's possible that all of these could be buggy and the
original commit could be correct.

> Avi, could you pls comment on my analysis?
>
> diff --git a/hw/pci.c b/hw/pci.c
> index 6d08cef..286383a 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -986,7 +986,7 @@ static void pci_update_mappings(PCIDevice *d)
>         r->addr = new_addr;
>         if (r->addr != PCI_BAR_UNMAPPED) {
>             memory_region_add_subregion_overlap(r->address_space,
> -                                                r->addr, r->memory, 1);
> +                                                r->addr, r->memory, 2);
>         }
>     }
>  }