From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Mumgd-0006uW-R4 for qemu-devel@nongnu.org; Mon, 05 Oct 2009 08:29:07 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MumgZ-0006qb-0n for qemu-devel@nongnu.org; Mon, 05 Oct 2009 08:29:07 -0400 Received: from [199.232.76.173] (port=43522 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MumgY-0006qO-PE for qemu-devel@nongnu.org; Mon, 05 Oct 2009 08:29:02 -0400 Received: from mx1.redhat.com ([209.132.183.28]:34855) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MumgY-0001Jz-43 for qemu-devel@nongnu.org; Mon, 05 Oct 2009 08:29:02 -0400 Date: Mon, 5 Oct 2009 14:26:58 +0200 From: "Michael S. Tsirkin" Message-ID: <20091005122658.GC30799@redhat.com> References: <1254737223-16129-1-git-send-email-yamahata@valinux.co.jp> <1254737223-16129-14-git-send-email-yamahata@valinux.co.jp> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1254737223-16129-14-git-send-email-yamahata@valinux.co.jp> Subject: [Qemu-devel] Re: [PATCH 13/23] pci: make pci configuration transaction more accurate. List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Isaku Yamahata Cc: qemu-devel@nongnu.org On Mon, Oct 05, 2009 at 07:06:53PM +0900, Isaku Yamahata wrote: > This patch sorts out/enhances pci code to track pci bus topology > accurately. > - Track host bus bridge with pci domain number. Although the > current qemu support only pci domian 0 yet. try running ispell on comments :) > - Track pci bridge parent-child relationship. > When looking down from pci host bus for pci sub bus, be aware of > secondary bus/subordinate bus. > Thus pci configuration transaction is more accurately emulated. > > This patch adds new member to PCIBus to track pci bus topology. > Since qdev already tracks down bus relationship, those new member > wouldn't be necessary. In fact, I think qdev tracks physical topology, sub_bus is guest-assignable, it can get any value within limits. > However it would be addressed later because not all the pci device > isn't converted to qdev yet. > > Signed-off-by: Isaku Yamahata > --- > hw/apb_pci.c | 4 +- > hw/pci-hotplug.c | 6 +- > hw/pci.c | 121 ++++++++++++++++++++++++++++++++++++++++-------------- > hw/pci.h | 11 ++++- > 4 files changed, 103 insertions(+), 39 deletions(-) > > diff --git a/hw/apb_pci.c b/hw/apb_pci.c > index 9d9f5fc..381b924 100644 > --- a/hw/apb_pci.c > +++ b/hw/apb_pci.c > @@ -254,11 +254,11 @@ PCIBus *pci_apb_init(target_phys_addr_t special_base, > /* APB secondary busses */ > *bus2 = pci_bridge_init(d->host_state.bus, PCI_DEVFN(1, 0), > PCI_VENDOR_ID_SUN, PCI_DEVICE_ID_SUN_SIMBA, > - pci_apb_map_irq, > + 1, 1, pci_apb_map_irq, > "Advanced PCI Bus secondary bridge 1"); > *bus3 = pci_bridge_init(d->host_state.bus, PCI_DEVFN(1, 1), > PCI_VENDOR_ID_SUN, PCI_DEVICE_ID_SUN_SIMBA, > - pci_apb_map_irq, > + 2, 2, pci_apb_map_irq, > "Advanced PCI Bus secondary bridge 2"); > > return d->host_state.bus; How come we need to set these up? I thought we should init to 0 and BIOS will set them up for us? > diff --git a/hw/pci-hotplug.c b/hw/pci-hotplug.c > index f3dc421..890ecc3 100644 > --- a/hw/pci-hotplug.c > +++ b/hw/pci-hotplug.c > @@ -65,7 +65,7 @@ void drive_hot_add(Monitor *mon, const QDict *qdict) > return; > } > > - dev = pci_find_device(pci_bus, slot, 0); > + dev = pci_find_device(pci_find_host_bus(0), pci_bus, slot, 0); > if (!dev) { > monitor_printf(mon, "no pci device with address %s\n", pci_addr); > return; Actually, what we do here is already pretty broken: monitor commands should get topology info, bus number is not topology. > @@ -201,7 +201,7 @@ void pci_device_hot_remove(Monitor *mon, const char *pci_addr) > return; > } > > - d = pci_find_device(bus, slot, 0); > + d = pci_find_device(pci_find_host_bus(0), bus, slot, 0); > if (!d) { > monitor_printf(mon, "slot %d empty\n", slot); > return; > @@ -228,7 +228,7 @@ static int pci_match_fn(void *dev_private, void *arg) > */ > void pci_device_hot_remove_success(int pcibus, int slot) > { > - PCIDevice *d = pci_find_device(pcibus, slot, 0); > + PCIDevice *d = pci_find_device(pci_find_host_bus(0), pcibus, slot, 0); > int class_code; > > if (!d) { > diff --git a/hw/pci.c b/hw/pci.c > index 09a6816..6ddd256 100644 > --- a/hw/pci.c > +++ b/hw/pci.c > @@ -34,9 +34,12 @@ > # define PCI_DPRINTF(format, ...) do { } while (0) > #endif > > +#define PCI_BUS_NUM_MAX 255 > struct PCIBus { > BusState qbus; > int bus_num; > + int sub_bus; > + > int devfn_min; > pci_set_irq_fn set_irq; > pci_map_irq_fn map_irq; > @@ -44,7 +47,10 @@ struct PCIBus { > void *irq_opaque; > PCIDevice *devices[256]; > PCIDevice *parent_dev; > - PCIBus *next; > + > + QLIST_HEAD(, PCIBus) child; /* this will be replaced by qdev later */ > + QLIST_ENTRY(PCIBus) sibling;/* this will be replaced by qdev later */ > + > /* The bus IRQ state is the logical OR of the connected devices. > Keep a count of the number of devices with raised IRQs. */ > int nirq; > @@ -69,7 +75,13 @@ static void pci_set_irq(void *opaque, int irq_num, int level); > target_phys_addr_t pci_mem_base; > static uint16_t pci_default_sub_vendor_id = PCI_SUBVENDOR_ID_REDHAT_QUMRANET; > static uint16_t pci_default_sub_device_id = PCI_SUBDEVICE_ID_QEMU; > -static PCIBus *first_bus; > + > +struct PCIHostBus { > + int domain; > + struct PCIBus *bus; > + QLIST_ENTRY(PCIHostBus) next; > +}; > +static QLIST_HEAD(, PCIHostBus) host_buses; > > static const VMStateDescription vmstate_pcibus = { > .name = "PCIBUS", > @@ -98,6 +110,28 @@ static void pci_bus_reset(void *opaque) > } > } > > +static void pci_host_bus_register(int domain, PCIBus *bus) > +{ > + struct PCIHostBus *host; > + host = qemu_mallocz(sizeof(*host)); > + host->domain = domain; > + host->bus = bus; > + QLIST_INSERT_HEAD(&host_buses, host, next); > +} > + > +PCIBus *pci_find_host_bus(int domain) > +{ > + struct PCIHostBus *host; > + > + QLIST_FOREACH(host, &host_buses, next) { > + if (host->domain == domain) { > + return host->bus; > + } > + } > + > + return NULL; > +} > + > PCIBus *pci_register_bus(DeviceState *parent, const char *name, > pci_set_irq_fn set_irq, pci_map_irq_fn map_irq, > void *irq_opaque, int devfn_min, int nirq) > @@ -112,14 +146,20 @@ PCIBus *pci_register_bus(DeviceState *parent, const char *name, > bus->devfn_min = devfn_min; > bus->nirq = nirq; > bus->irq_count = qemu_mallocz(nirq * sizeof(bus->irq_count[0])); > - bus->next = first_bus; > - first_bus = bus; > + > + /* host bridge */ > + bus->bus_num = 0; > + bus->sub_bus = PCI_BUS_NUM_MAX; > + QLIST_INIT(&bus->child); > + pci_host_bus_register(0, bus); /* for now only pci domain 0 is supported */ > + > vmstate_register(nbus++, &vmstate_pcibus, bus); > qemu_register_reset(pci_bus_reset, bus); > return bus; > } > > -static PCIBus *pci_register_secondary_bus(PCIDevice *dev, > +static PCIBus *pci_register_secondary_bus(PCIBus *parent, > + PCIDevice *dev, > pci_map_irq_fn map_irq, > const char *name) > { > @@ -128,8 +168,11 @@ static PCIBus *pci_register_secondary_bus(PCIDevice *dev, > bus = FROM_QBUS(PCIBus, qbus_create(&pci_bus_info, &dev->qdev, name)); > bus->map_irq = map_irq; > bus->parent_dev = dev; > - bus->next = dev->bus->next; > - dev->bus->next = bus; > + > + bus->bus_num = dev->config[PCI_SECONDARY_BUS]; > + bus->sub_bus = dev->config[PCI_SUBORDINATE_BUS]; > + QLIST_INIT(&bus->child); > + QLIST_INSERT_HEAD(&parent->child, bus, sibling); > return bus; > } > > @@ -242,7 +285,7 @@ static int pci_parse_devaddr(const char *addr, int *domp, int *busp, unsigned *s > return -1; > > /* Note: QEMU doesn't implement domains other than 0 */ > - if (dom != 0 || pci_find_bus(bus) == NULL) > + if (dom != 0 || pci_find_bus(pci_find_host_bus(0), bus) == NULL) > return -1; > > *domp = dom; > @@ -272,7 +315,7 @@ static PCIBus *pci_get_bus_devfn(int *devfnp, const char *devaddr) > > if (!devaddr) { > *devfnp = -1; > - return pci_find_bus(0); > + return pci_find_bus(pci_find_host_bus(0), 0); > } > > if (pci_parse_devaddr(devaddr, &dom, &bus, &slot) < 0) { > @@ -280,7 +323,7 @@ static PCIBus *pci_get_bus_devfn(int *devfnp, const char *devaddr) > } > > *devfnp = slot << 3; > - return pci_find_bus(bus); > + return pci_find_bus(pci_find_host_bus(0), bus); > } > > static void pci_init_cmask(PCIDevice *dev) > @@ -597,8 +640,7 @@ void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len) > addr, val, len); > #endif > bus_num = (addr >> 16) & 0xff; > - while (s && s->bus_num != bus_num) > - s = s->next; > + s = pci_find_bus(s, bus_num); > if (!s) > return; > pci_dev = s->devices[(addr >> 8) & 0xff]; > @@ -619,8 +661,7 @@ uint32_t pci_data_read(void *opaque, uint32_t addr, int len) > uint32_t val; > > bus_num = (addr >> 16) & 0xff; > - while (s && s->bus_num != bus_num) > - s= s->next; > + s = pci_find_bus(s, bus_num); > if (!s) > goto fail; > pci_dev = s->devices[(addr >> 8) & 0xff]; > @@ -727,7 +768,7 @@ static const pci_class_desc pci_class_descriptions[] = > { 0, NULL} > }; > > -static void pci_info_device(PCIDevice *d) > +static void pci_info_device(PCIBus *bus, PCIDevice *d) > { > Monitor *mon = cur_mon; > int i, class; > @@ -781,30 +822,32 @@ static void pci_info_device(PCIDevice *d) > } > monitor_printf(mon, " id \"%s\"\n", d->qdev.id ? d->qdev.id : ""); > if (class == 0x0604 && d->config[0x19] != 0) { > - pci_for_each_device(d->config[0x19], pci_info_device); > + pci_for_each_device(bus, d->config[0x19], pci_info_device); > } > } > > -void pci_for_each_device(int bus_num, void (*fn)(PCIDevice *d)) > +void pci_for_each_device(PCIBus *bus, int bus_num, > + void (*fn)(PCIBus *b, PCIDevice *d)) > { > - PCIBus *bus = first_bus; > PCIDevice *d; > int devfn; > > - while (bus && bus->bus_num != bus_num) > - bus = bus->next; > + bus = pci_find_bus(bus, bus_num); > if (bus) { > for(devfn = 0; devfn < 256; devfn++) { > d = bus->devices[devfn]; > if (d) > - fn(d); > + fn(bus, d); > } > } > } > > void pci_info(Monitor *mon) > { > - pci_for_each_device(0, pci_info_device); > + struct PCIHostBus *host; > + QLIST_FOREACH(host, &host_buses, next) { > + pci_for_each_device(host->bus, 0, pci_info_device); > + } > } > > PCIDevice *pci_create(const char *name, const char *devaddr) > @@ -888,22 +931,33 @@ static void pci_bridge_write_config(PCIDevice *d, > > pci_default_write_config(d, address, val, len); > s->bus->bus_num = d->config[PCI_SECONDARY_BUS]; > + s->bus->sub_bus = d->config[PCI_SUBORDINATE_BUS]; > } > > -PCIBus *pci_find_bus(int bus_num) > +PCIBus *pci_find_bus(PCIBus *bus, int bus_num) > { > - PCIBus *bus = first_bus; > + PCIBus *sec; > > - while (bus && bus->bus_num != bus_num) > - bus = bus->next; > + if (!bus) > + return NULL; > > - return bus; > + if (bus->bus_num == bus_num) { > + return bus; > + } > + > + /* try child bus */ > + QLIST_FOREACH(sec, &bus->child, sibling) { > + if (sec->bus_num <= bus_num && bus_num <= sec->sub_bus) { > + return pci_find_bus(sec, bus_num); > + } > + } > + > + return NULL; > } > > -PCIDevice *pci_find_device(int bus_num, int slot, int function) > +PCIDevice *pci_find_device(PCIBus *bus, int bus_num, int slot, int function) > { > - PCIBus *bus = pci_find_bus(bus_num); > - > + bus = pci_find_bus(bus, bus_num); > if (!bus) > return NULL; > > @@ -911,6 +965,7 @@ PCIDevice *pci_find_device(int bus_num, int slot, int function) > } > > PCIBus *pci_bridge_init(PCIBus *bus, int devfn, uint16_t vid, uint16_t did, > + uint8_t sec_bus, uint8_t sub_bus, > pci_map_irq_fn map_irq, const char *name) > { > PCIBridge *s; > @@ -932,7 +987,11 @@ PCIBus *pci_bridge_init(PCIBus *bus, int devfn, uint16_t vid, uint16_t did, > PCI_HEADER_TYPE_MULTI_FUNCTION | PCI_HEADER_TYPE_BRIDGE; // header_type > s->dev.config[0x1E] = 0xa0; // secondary status > > - s->bus = pci_register_secondary_bus(&s->dev, map_irq, name); > + assert(sec_bus <= sub_bus); > + s->dev.config[PCI_SECONDARY_BUS] = sec_bus; > + s->dev.config[PCI_SUBORDINATE_BUS] = sub_bus; I think that this is wrong, config space is initialized by software. And it is also wrong to bundle this into an unrelated patch. > + > + s->bus = pci_register_secondary_bus(bus, &s->dev, map_irq, name); > return s->bus; > } > > diff --git a/hw/pci.h b/hw/pci.h > index b65ce03..00f2b78 100644 > --- a/hw/pci.h > +++ b/hw/pci.h > @@ -160,6 +160,9 @@ typedef struct PCIIORegion { > > #define PCI_COMMAND_RESERVED_MASK_HI (PCI_COMMAND_RESERVED >> 8) > > +/* Header type 1 (PCI-to-PCI bridges) */ > +#define PCI_SUBORDINATE_BUS 0x1a /* Highest bus number behind the bridge */ > + > /* Size of the standard PCI config header */ > #define PCI_CONFIG_HEADER_SIZE 0x40 > /* Size of the standard PCI config space */ > @@ -268,15 +271,17 @@ PCIDevice *pci_nic_init(NICInfo *nd, const char *default_model, > void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len); > uint32_t pci_data_read(void *opaque, uint32_t addr, int len); > int pci_bus_num(PCIBus *s); > -void pci_for_each_device(int bus_num, void (*fn)(PCIDevice *d)); > -PCIBus *pci_find_bus(int bus_num); > -PCIDevice *pci_find_device(int bus_num, int slot, int function); > +void pci_for_each_device(PCIBus *bus, int bus_num, void (*fn)(PCIBus *bus, PCIDevice *d)); > +PCIBus *pci_find_host_bus(int domain); > +PCIBus *pci_find_bus(PCIBus *bus, int bus_num); > +PCIDevice *pci_find_device(PCIBus *bus, int bus_num, int slot, int function); > > int pci_read_devaddr(Monitor *mon, const char *addr, int *domp, int *busp, > unsigned *slotp); > > void pci_info(Monitor *mon); > PCIBus *pci_bridge_init(PCIBus *bus, int devfn, uint16_t vid, uint16_t did, > + uint8_t sec_bus, uint8_t sub_bus, > pci_map_irq_fn map_irq, const char *name); > > static inline void > -- > 1.6.0.2