[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] Re: [PATCH V5 17/29] pci: make pci configuration transactio
From: |
Michael S. Tsirkin |
Subject: |
[Qemu-devel] Re: [PATCH V5 17/29] pci: make pci configuration transaction more accurate. |
Date: |
Fri, 9 Oct 2009 14:52:25 +0200 |
User-agent: |
Mutt/1.5.19 (2009-01-05) |
On Fri, Oct 09, 2009 at 03:28:50PM +0900, Isaku Yamahata wrote:
> This patch sorts out/enhances pci code to track pci bus topology
> more accurately.
> - Track host bus bridge with pci domain number. Although the
> current qemu implementation supports only pci domian 0 yet.
> - 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.
Overall I think this is a good idea.
> 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.
Another point: why store these separately at all?
Can't we just use the pci config registers directly?
> However it would be addressed later because not all the pci device
> isn't converted to qdev yet.
>
> Signed-off-by: Isaku Yamahata <address@hidden>
Some comments below. Could you please test something like windows
boot and verify that using a more complex structure does not
affect boot times?
> ---
> hw/pci-hotplug.c | 4 +-
> hw/pci.c | 115
> ++++++++++++++++++++++++++++++++++++++++--------------
> hw/pci.h | 8 ++-
> 3 files changed, 92 insertions(+), 35 deletions(-)
>
> diff --git a/hw/pci-hotplug.c b/hw/pci-hotplug.c
> index ccd2cf3..d50919b 100644
> --- a/hw/pci-hotplug.c
> +++ b/hw/pci-hotplug.c
> @@ -87,7 +87,7 @@ void drive_hot_add(Monitor *mon, const QDict *qdict)
> if (pci_read_devaddr(mon, pci_addr, &dom, &pci_bus, &slot)) {
> goto err;
> }
> - 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);
> goto err;
> @@ -222,7 +222,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;
> diff --git a/hw/pci.c b/hw/pci.c
> index ee3ab05..33cc476 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;
> @@ -45,7 +48,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;
> @@ -70,7 +76,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",
> @@ -128,6 +140,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;
> +}
> +
> void pci_bus_new_inplace(PCIBus *bus, DeviceState *parent,
> const char *name, int devfn_min)
> {
> @@ -135,8 +169,13 @@ void pci_bus_new_inplace(PCIBus *bus, DeviceState
> *parent,
>
> qbus_create_inplace(&bus->qbus, &pci_bus_info, parent, name);
> bus->devfn_min = devfn_min;
> - 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);
> }
> @@ -178,7 +217,8 @@ PCIBus *pci_register_bus(DeviceState *parent, const char
> *name,
> return bus;
> }
>
> -static void pci_register_secondary_bus(PCIBus *bus,
> +static void pci_register_secondary_bus(PCIBus *parent,
> + PCIBus *bus,
> PCIDevice *dev,
> pci_map_irq_fn map_irq,
> const char *name)
> @@ -186,8 +226,11 @@ static void pci_register_secondary_bus(PCIBus *bus,
BTW, should not it be possible to remove buses?
I don't see code removing bus from the tree, anywhere.
> qbus_create_inplace(&bus->qbus, &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];
These seem unnecessary: init value is 0 anyway.
But you need to update these at reset, I don't see where you do this.
> + QLIST_INIT(&bus->child);
> + QLIST_INSERT_HEAD(&parent->child, bus, sibling);
> }
>
> int pci_bus_num(PCIBus *s)
> @@ -300,7 +343,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 (pci_find_bus(pci_find_host_bus(dom), bus) == NULL)
Replace == NULL with ! please.
> return -1;
>
> *domp = dom;
> @@ -330,7 +373,7 @@ 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) {
> @@ -338,7 +381,7 @@ 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)
> @@ -622,8 +665,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];
> @@ -643,8 +685,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];
> @@ -750,7 +791,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;
> @@ -805,30 +846,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);
> + }
> }
>
> static const char * const pci_nic_models[] = {
> @@ -917,21 +960,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;
> @@ -973,7 +1028,7 @@ PCIBus *pci_bridge_init(PCIBus *bus, int devfn, uint16_t
> vid, uint16_t did,
> qdev_init_nofail(&dev->qdev);
>
> s = DO_UPCAST(PCIBridge, dev, dev);
> - pci_register_secondary_bus(&s->bus, &s->dev, map_irq, name);
> + pci_register_secondary_bus(bus, &s->bus, &s->dev, map_irq, name);
> return &s->bus;
> }
>
> diff --git a/hw/pci.h b/hw/pci.h
> index 269981a..3a3838d 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -159,6 +159,7 @@ 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 */
> #define PCI_ROM_ADDRESS1 0x38 /* Same as PCI_ROM_ADDRESS, but for
> htype 1 */
>
> /* Size of the standard PCI config header */
> @@ -270,9 +271,10 @@ PCIDevice *pci_nic_init_nofail(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);
> PCIBus *pci_get_bus_devfn(int *devfnp, const char *devaddr);
>
> int pci_read_devaddr(Monitor *mon, const char *addr, int *domp, int *busp,
> --
> 1.6.0.2
[Qemu-devel] [PATCH V5 11/29] pci_host.h: move functions in pci_host.h into .c file., Isaku Yamahata, 2009/10/09
[Qemu-devel] [PATCH V5 27/29] pci/bridge: don't update bar mapping when bar2-5 is changed., Isaku Yamahata, 2009/10/09
[Qemu-devel] [PATCH V5 18/29] pci: factor out the conversion logic from io port address into pci device., Isaku Yamahata, 2009/10/09