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

[Michael S. Tsirkin]

On Sun, Mar 04, 2012 at 02:33:02PM +0200, Michael S. Tsirkin 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.
> 
> 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);
>          }
>      }
>  }

Grr. Scap the above, the patch does nothing, I didn't test it right.