From patchwork Sun Mar 4 19:51:02 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Blue Swirl X-Patchwork-Id: 144520 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 7D155B6FA2 for ; Mon, 5 Mar 2012 06:51:45 +1100 (EST) Received: from localhost ([::1]:56544 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S4HT5-0005X0-1f for incoming@patchwork.ozlabs.org; Sun, 04 Mar 2012 14:51:43 -0500 Received: from eggs.gnu.org ([208.118.235.92]:56929) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S4HSt-0005Wk-2p for qemu-devel@nongnu.org; Sun, 04 Mar 2012 14:51:33 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1S4HSp-0007KM-Pe for qemu-devel@nongnu.org; Sun, 04 Mar 2012 14:51:30 -0500 Received: from mail-iy0-f173.google.com ([209.85.210.173]:52434) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S4HSp-0007KH-EC for qemu-devel@nongnu.org; Sun, 04 Mar 2012 14:51:27 -0500 Received: by iafj26 with SMTP id j26so5041136iaf.4 for ; Sun, 04 Mar 2012 11:51:25 -0800 (PST) Received-SPF: pass (google.com: domain of blauwirbel@gmail.com designates 10.42.153.10 as permitted sender) client-ip=10.42.153.10; Authentication-Results: mr.google.com; spf=pass (google.com: domain of blauwirbel@gmail.com designates 10.42.153.10 as permitted sender) smtp.mail=blauwirbel@gmail.com; dkim=pass header.i=blauwirbel@gmail.com Received: from mr.google.com ([10.42.153.10]) by 10.42.153.10 with SMTP id k10mr4441903icw.24.1330890685532 (num_hops = 1); Sun, 04 Mar 2012 11:51:25 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-type; bh=5dJ37VIfSmeXy1N0gB3tT4vWQzk0Tq1N7/I8k7kQQyU=; b=xN5p2nIyvk7xvCwkOm5Pa3wyWc6/iVErv9Lw+DJ9Hsrnwt3+yYT6ovQurFdpUBJbHJ gzsFSUomlEZdpyz0KfMZDEJFGYwT1BUXF6pTv/6omxDlJK7tp9CZz3m3P95Q9VIfjtG/ oiuwYNicir4cKzMgnEblGxmYb59A+/R5i3FTNoWZR4TWrnjNHZanO1ybwRqvJJk8CqXJ uTs1Erk76YPHaDpEbbbW+QmqC/b1764PzTwZebGwQGAs6E5mIcyqO3YBlbTC2twX1FgL hNE+ivzvq9hPmcW2QxoJs1pEoxVa+i2YbTlmn0egxrpT2fVbxA/WBaXLMQdXbNPoR3ZK rkfg== Received: by 10.42.153.10 with SMTP id k10mr3662873icw.24.1330890683147; Sun, 04 Mar 2012 11:51:23 -0800 (PST) MIME-Version: 1.0 Received: by 10.50.22.40 with HTTP; Sun, 4 Mar 2012 11:51:02 -0800 (PST) In-Reply-To: <20120304173428.GA16058@redhat.com> References: <20120304094614.GA8158@redhat.com> <20120304122100.GA11207@redhat.com> <20120304132805.GC12047@redhat.com> <20120304142333.GA12900@redhat.com> <20120304152201.GC12776@redhat.com> <20120304173428.GA16058@redhat.com> From: Blue Swirl Date: Sun, 4 Mar 2012 19:51:02 +0000 Message-ID: To: "Michael S. Tsirkin" X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 209.85.210.173 Cc: Mark Cave-Ayland , qemu-devel@nongnu.org, Anthony Liguori Subject: Re: [Qemu-devel] [PATCH] pci: fix bridge IO/BASE X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org On Sun, Mar 4, 2012 at 17:35, Michael S. Tsirkin wrote: > On Sun, Mar 04, 2012 at 05:07:34PM +0000, Blue Swirl wrote: >> On Sun, Mar 4, 2012 at 15:22, Michael S. Tsirkin wrote: >> > On Sun, Mar 04, 2012 at 02:35:28PM +0000, Blue Swirl wrote: >> >> On Sun, Mar 4, 2012 at 14:23, Michael S. Tsirkin wrote: >> >> > On Sun, Mar 04, 2012 at 01:38:38PM +0000, Blue Swirl wrote: >> >> >> On Sun, Mar 4, 2012 at 13:28, Michael S. Tsirkin 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 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 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 >> >> >> >> >> > Signed-off-by: Michael S. Tsirkin >> >> >> >> >> > >> >> >> >> >> > 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. >> >> > >> >> > >> >> > Could you please clarify what is the SABRE device? >> >> > Is it, in fact, a bridge device? Or not? >> >> >> >> Yes, it's the host bridge, also known as PBM. It's documented in >> >> UltraSPARC IIi User's Manual >> > >> > Btw would be nice to host the manuals at qemu.org >> > our code points at sun.com URLs :( >> >> I have most if not all manuals, downloaded from sun.com, but I'm not >> sure if they can be redistributed. > > Okay ... > Let's change the link to point to some other place which has them? > >> > I am looking at 19.3.1 PCI Configuration Space >> > and it appears to show that this is a regular device >> > with a couple of custom registers at pffsets 0x40 >> > and 0x41. >> > >> > Why do we want to pretend it is a bridge? >> >> It's the host bridge and the device class is PCI_CLASS_BRIDGE_HOST. > > Yes. But the *header* type is 0 (NORMAL) > while the code in pci_init_mask_bridge > which is the only user of the is_bridge register > initializes a type 1 (BRIDGE) header. > > So it just happens to do a vaguely correct thing. Well, that is still according to device spec. >> > >> >> and there it says that the device is >> >> found in the configuration space. >> >> >> >> The secondary bridges are Simbas and should be called APBs. >> > >> > As far as I can see from the code, it has header type >> > NORMAL but sets is_bridge. >> > This was done by this commit: >> > 776e1bbb6cf4fe66a93c1a5dd814bbb650deca00 >> >> IIRC otherwise some registers are not writable. > > Yes but which ones? I looked at the manual and > it does not list any registers. Playing with code, > it looks like we just need to make *some* > BAR writeable. I tried with > pci_set_long(d->wmask + PCI_BASE_ADDRESS_0, 0xfffffff0); > to > pci_set_long(d->wmask + PCI_BASE_ADDRESS_5, 0xfffffff0); > > and any one of these makes bios get at least to > the prompt. I now know the root cause of the problem. OpenBIOS programs the BARs somewhat correctly just by accident. The initial io_base and mem_base for BARs are not correct, but because the host bridge BARs (and also 6 of which 4 are not even BARs!) are programmed first, the bases happened to settle to values that happen to work. The commit revealed the problem since the settling didn't happen. The mask changes just let the host bridge setup continue to do the magic. By just changing OpenBIOS (see attached patch), I can get the devices to work (assuming that VGA is a separate problem). There's no need to change QEMU. >> > >> >> > >> >> > >> >> > -- >> >> > MST From 3f957e2dc8477f00f6d3a9491d81399ee750c725 Mon Sep 17 00:00:00 2001 Message-Id: <3f957e2dc8477f00f6d3a9491d81399ee750c725.1330890410.git.blauwirbel@gmail.com> From: Blue Swirl Date: Sun, 4 Mar 2012 19:46:38 +0000 Subject: [PATCH] pci: fix BAR setup 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. Avoid to set up BARs for host bridge. Fix bridge check, this lead to setting up 6 BARs instead of more correct 2. If a bridge doesn't have any devices behind it, disable it entirely. Fix Sparc64 PCI memory base. Signed-off-by: Blue Swirl --- arch/sparc64/openbios.c | 2 +- drivers/pci.c | 67 ++++++++++++++++++++++++++++++++++------------ drivers/pci.h | 7 +++++ 3 files changed, 57 insertions(+), 19 deletions(-) diff --git a/arch/sparc64/openbios.c b/arch/sparc64/openbios.c index ac709fe..a1544a8 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 = 0x10000000, .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..6ed0c03 100644 --- a/drivers/pci.c +++ b/drivers/pci.c @@ -966,11 +966,18 @@ 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\n", + config->path, reloc, *p_omask, *io_base, *mem_base); pci_config_write32(addr, config_addr, reloc | *p_omask); config->assigned[reg] = reloc | *p_omask; } @@ -1021,26 +1028,30 @@ ob_pci_configure(pci_addr addr, pci_config_t *config, int num_regs, int rom_bar, pci_config_write16(addr, PCI_COMMAND, cmd); } -static void ob_configure_pci_device(const char* parent_path, - int *bus_num, unsigned long *mem_base, unsigned long *io_base, - int bus, int devnum, int fn, int *p_is_multi); +static int ob_configure_pci_device(const char* parent_path, + int *bus_num, unsigned long *mem_base, + unsigned long *io_base, int bus, int devnum, + int fn, int *p_is_multi); -static void ob_scan_pci_bus(int *bus_num, unsigned long *mem_base, - unsigned long *io_base, const char *path, - int bus) +static int ob_scan_pci_bus(int *bus_num, unsigned long *mem_base, + unsigned long *io_base, const char *path, + int bus) { - int devnum, fn, is_multi; + int devnum, fn, is_multi, ndevices = 0; PCI_DPRINTF("\nScanning bus %d at %s...\n", bus, path); for (devnum = 0; devnum < 32; devnum++) { is_multi = 0; for (fn = 0; fn==0 || (is_multi && fn<8); fn++) { - ob_configure_pci_device(path, bus_num, mem_base, io_base, - bus, devnum, fn, &is_multi); + ndevices += ob_configure_pci_device(path, bus_num, + mem_base, io_base, + bus, devnum, fn, + &is_multi); } } + return ndevices; } static void ob_configure_pci_bridge(pci_addr addr, @@ -1048,6 +1059,9 @@ static void ob_configure_pci_bridge(pci_addr addr, unsigned long *io_base, int primary_bus, pci_config_t *config) { + int ndevices; + uint8_t command; + config->primary_bus = primary_bus; pci_config_write8(addr, PCI_PRIMARY_BUS, config->primary_bus); @@ -1062,16 +1076,30 @@ static void ob_configure_pci_bridge(pci_addr addr, /* make pci bridge parent device, prepare for recursion */ - ob_scan_pci_bus(bus_num, mem_base, io_base, - config->path, config->secondary_bus); + ndevices = ob_scan_pci_bus(bus_num, mem_base, io_base, + config->path, config->secondary_bus); + if (!ndevices) { + /* no devices, disable bridging */ + PCI_DPRINTF("disabling bridge %s\n", config->path); + command = pci_config_read8(addr, PCI_COMMAND); + command &= ~(PCI_COMMAND_IO | PCI_COMMAND_MEMORY | + PCI_COMMAND_VGA_PALETTE | PCI_COMMAND_MASTER); + pci_config_write8(addr, PCI_COMMAND, command); + pci_config_write8(addr, PCI_IO_BASE, 0); + pci_config_write8(addr, PCI_IO_LIMIT, 0); + pci_config_write8(addr, PCI_MEMORY_BASE, 0); + pci_config_write8(addr, PCI_MEMORY_LIMIT, 0); + return; + } /* bus scan updates *bus_num to last revealed pci bus number */ config->subordinate_bus = *bus_num; pci_config_write8(addr, PCI_SUBORDINATE_BUS, config->subordinate_bus); - PCI_DPRINTF("bridge %s PCI bus primary=%d secondary=%d subordinate=%d\n", - config->path, config->primary_bus, config->secondary_bus, - config->subordinate_bus); + PCI_DPRINTF("bridge %s PCI bus primary=%d secondary=%d subordinate=%d" + " ndev=%d\n", + config->path, config->primary_bus, config->secondary_bus, + config->subordinate_bus, ndevices); pci_set_bus_range(config); } @@ -1117,7 +1145,7 @@ static int ob_pci_read_identification(int bus, int devnum, int fn, return 1; } -static void ob_configure_pci_device(const char* parent_path, +static int ob_configure_pci_device(const char* parent_path, int *bus_num, unsigned long *mem_base, unsigned long *io_base, int bus, int devnum, int fn, int *p_is_multi) { @@ -1133,7 +1161,7 @@ static void ob_configure_pci_device(const char* parent_path, int is_host_bridge = 0; if (!ob_pci_read_identification(bus, devnum, fn, &vid, &did, &class, &subclass)) { - return; + return 0; } addr = PCI_ADDR(bus, devnum, fn); @@ -1195,16 +1223,18 @@ static void ob_configure_pci_device(const char* parent_path, if (get_property(phandle, "vendor-id", NULL)) { PCI_DPRINTF("host bridge already configured\n"); - return; + return 0; } } activate_dev(phandle); - if (htype & PCI_HEADER_TYPE_BRIDGE) { + if (htype & PCI_HEADER_TYPE_BRIDGE || (class == PCI_BASE_CLASS_BRIDGE)) { + PCI_DPRINTF("Bridge 2 bars, htype %x\n", htype); num_bars = 2; rom_bar = PCI_ROM_ADDRESS1; } else { + PCI_DPRINTF("Device 6 bars, htype %x\n", htype); num_bars = 6; rom_bar = PCI_ROM_ADDRESS; } @@ -1240,6 +1270,7 @@ static void ob_configure_pci_device(const char* parent_path, ob_configure_pci_bridge(addr, bus_num, mem_base, io_base, bus, &config); } + return 1; } int ob_pci_init(void) diff --git a/drivers/pci.h b/drivers/pci.h index 0f6ae1f..4314507 100644 --- a/drivers/pci.h +++ b/drivers/pci.h @@ -7,6 +7,8 @@ #define PCI_COMMAND 0x04 #define PCI_COMMAND_IO 0x01 #define PCI_COMMAND_MEMORY 0x02 +#define PCI_COMMAND_MASTER 0x4 /* Enable bus mastering */ +#define PCI_COMMAND_VGA_PALETTE 0x20 /* Enable palette snooping */ #define PCI_STATUS 0x06 /* 16 bits */ #define PCI_STATUS_CAP_LIST 0x10 /* Support Capability List */ @@ -44,6 +46,11 @@ #define PCI_BASE_ADDR_4 0x20 #define PCI_BASE_ADDR_5 0x24 +#define PCI_IO_BASE 0x1c /* I/O range behind the bridge */ +#define PCI_IO_LIMIT 0x1d +#define PCI_MEMORY_BASE 0x20 /* Memory range behind */ +#define PCI_MEMORY_LIMIT 0x22 + #define PCI_SUBSYSTEM_VENDOR_ID 0x2c #define PCI_SUBSYSTEM_ID 0x2e -- 1.7.2.5