Re: [OpenBIOS] [PATCH] pci: fix BAR setup

[Blue Swirl]

On Thu, Apr 19, 2012 at 20:29, Artyom Tarasenko  wrote:
> On Thu, Apr 19, 2012 at 10:04 PM, Blue Swirl  wrote:
> > On Thu, Apr 19, 2012 at 19:06, Artyom Tarasenko  wrote:
> > > On Sun, Mar 11, 2012 at 11:59 AM, Blue Swirl  wrote:
> > > > A change in QEMU on how PCI bridges are setup revealed
> > > > a bug in OpenBIOS PCI setup. On Sparc64, the BARs just
> > > > happened to get somewhat correct values by accident before
> > > > the commit but not after the change.
> > > >
> > > > Don't use arch->io_base for PCI I/O port BARs.
> > > >
> > > > Fix Sparc64 PCI memory base.
> > >
> > > Could it be that the ranges property needs to be adjusted to look as
> > > it did before the patch?
> > >
> > > This patch breaks HelenOS-0.4.3/sparc boot. But if I modify the ranges
> > > property to the old value, HelenOS boots.
> > >
> > > 0 > cd /pci@1fe,0  ok
> > > 0 > .properties
> > > [...]
> > > ranges                    -- 54 : 00 00 00 00 00 00 00 00 00 00 00 00
> > > 00 00 01 fe 01 00 00 00 00 00 00 00 02 00 00 00 01 00 00 00 00 00 00
> > > 00 00 00 00 00 00 00 01 fe 02 00 00 00 00 00 00 00 00 01 00 00 02 00
> > > 00 00 00 00 00 00 00 10 00 00 00 00 01 ff 00 00 00 00 00 00 00 00 10
> > > 00 00 00
> > >
> > > Not sure about the best formatting. If I reformat it like this:
> > >
> > > 00000000 00000000 00000000 000001fe 01000000 00000000 02000000
> > > 01000000 00000000 00000000 000001fe 02000000 00000000 00010000
> > > 02000000 00000000 00100000 000001ff 00000000 00000000 10000000
> > >                               ^^^^ this one (the third) used to be 0
> >
> > On an Ultra-5 this should be
> > 00000000.00000000.00000000.000001fe.01000000.00000000.01000000.
> > 01000000.00000000.00000000.000001fe.02000000.00000000.01000000.
> > 02000000.00000000.00000000.000001ff.00000000.00000001.00000000.
> > 03000000.00000000.00000000.000001ff.00000000.00000001.00000000
> >
> > http://git.kernel.org/?p=linux/kernel/git/davem/prtconfs.git;a=blob;f=ultra5...
> >
> > I guess the problem is that the properties are added by finding the
> > host bridge in the PCI bus (which btw isn't so nice), so we use the
> > pci_mem_base. Using pci_mem_base = 0 would probably fix the BAR, but
> > then VGA would get in the way. Probably something more complex is
> > needed.
>
> Hmm. Complicated. What is actually the meaning of this property? Don't
> we need the values ourselves?
> If these are just BARs, can't we pass the configuration from qemu?

The device drivers in OpenBIOS only want physical addresses, they
should be getting correct values, otherwise they wouldn't work. But an
OS must use 'ranges' to calculate them.

Assistance from QEMU should not be needed, just bridge handling (maybe
also PCI probing, again) should be fixed in OpenBIOS.
> > > > Signed-off-by: Blue Swirl
> > > >
> > > > I removed the bridge setup code, it's not really valid for Sparc64 APB
> > > > bridges anyway and currently no devices exist behind the bridges (but
> > > > that's where they really should be).
> > > >
> > > > Tested Sparc64 and PPC.
> > > >
> > > > arch/sparc64/openbios.c |    2 +-
> > > >  drivers/pci.c           |   11 +++++++++--
> > > >  2 files changed, 10 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/arch/sparc64/openbios.c b/arch/sparc64/openbios.c
> > > > index ac709fe..ab32a8f 100644
> > > > --- a/arch/sparc64/openbios.c
> > > > +++ b/arch/sparc64/openbios.c
> > > > @@ -64,7 +64,7 @@ static const struct hwdef hwdefs[] = {
> > > >             .cfg_base = APB_SPECIAL_BASE,
> > > >             .cfg_len = 0x2000000,
> > > >             .host_mem_base = APB_MEM_BASE,
> > > > .pci_mem_base = 0,
> > > > .pci_mem_base = 0x100000, /* avoid VGA at 0xa0000 */
> > > >
> > > > .mem_len = 0x10000000,
> > > >             .io_base = APB_SPECIAL_BASE + 0x2000000ULL, // PCI Bus I/O space
> > > >             .io_len = 0x10000,
> > > > diff --git a/drivers/pci.c b/drivers/pci.c
> > > > index f8c6414..e270a73 100644
> > > > --- a/drivers/pci.c
> > > > +++ b/drivers/pci.c
> > > > @@ -931,6 +931,7 @@ static void ob_pci_configure_bar(pci_addr addr,
> > > > pci_config_t *config,
> > > >
> > > > if ((*p_omask & 0x0000000f) == 0x4) {
> > > >                 /* 64 bits memory mapping */
> > > > PCI_DPRINTF("Skipping 64 bit BARs for %s\n", config->path);
> > > >
> > > > return;
> > > >         }
> > > >
> > > > @@ -966,11 +967,17 @@ static void ob_pci_configure_bar(pci_addr addr,
> > > > pci_config_t *config,
> > > >                 size = min_align;
> > > >         reloc = (reloc + size -1) & ~(size - 1);
> > > >         if (*io_base == base) {
> > > > PCI_DPRINTF("changing io_base from 0x%lx to 0x%x\n",
> > > > *io_base, reloc + size);
> > > >
> > > > *io_base = reloc + size;
> > > > reloc -= arch->io_base;
> > > >
> > > > } else {
> > > > PCI_DPRINTF("changing mem_base from 0x%lx to 0x%x\n",
> > > > *mem_base, reloc + size);
> > > >
> > > > *mem_base = reloc + size;
> > > >         }
> > > > PCI_DPRINTF("Configuring BARs for %s: reloc 0x%x omask 0x%x "
> > > > "io_base 0x%lx mem_base 0x%lx size 0x%x\n",
> > > > config->path, reloc, *p_omask, *io_base, *mem_base, size);
> > > >
> > > > pci_config_write32(addr, config_addr, reloc | *p_omask);
> > > >         config->assigned[reg] = reloc | *p_omask;
> > > >  }
> > > > @@ -1260,7 +1267,7 @@ int ob_pci_init(void)
> > > >     mem_base = arch->pci_mem_base;
> > > >     /* I/O ports under 0x400 are used by devices mapped at fixed
> > > >        location. */
> > > > io_base = arch->io_base + 0x400;
> > > > io_base = 0x400;
> > > >
> > > > bus = 0;
> > >
> > > solaris/sparc under qemu blog: http://tyom.blogspot.com/search/label/qemu
>
> solaris/sparc under qemu blog: http://tyom.blogspot.com/search/label/qemu