[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] Re: [PATCH 22/25] pci/brdige: qdevfy and initialize seconda
From: |
Isaku Yamahata |
Subject: |
[Qemu-devel] Re: [PATCH 22/25] pci/brdige: qdevfy and initialize secondary bus and subordinate bus. |
Date: |
Mon, 5 Oct 2009 18:51:22 +0900 |
User-agent: |
Mutt/1.5.6i |
On Sun, Oct 04, 2009 at 01:04:02PM +0200, Michael S. Tsirkin wrote:
> On Sat, Oct 03, 2009 at 05:16:14AM +0900, Isaku Yamahata wrote:
> > qdevfy pci bridge, and
> > initialize secondary bus and subordinate bus in configuration space.
> >
> > Signed-off-by: Isaku Yamahata <address@hidden>
> > ---
> > hw/apb_pci.c | 6 ++-
> > hw/pci.c | 110
> > +++++++++++++++++++++++++++++++++++++++++++---------------
> > hw/pci.h | 12 ++++++
> > 3 files changed, 98 insertions(+), 30 deletions(-)
> >
> > diff --git a/hw/apb_pci.c b/hw/apb_pci.c
> > index 00165e5..5ccb654 100644
> > --- a/hw/apb_pci.c
> > +++ b/hw/apb_pci.c
> > @@ -238,10 +238,12 @@ PCIBus *pci_apb_init(target_phys_addr_t special_base,
> > 0, 32);
> > pci_create_simple(d->host_state.bus, 0, "pbm");
> > /* APB secondary busses */
> > - *bus2 = pci_bridge_init(d->host_state.bus, 8, PCI_VENDOR_ID_SUN,
> > + *bus2 = pci_bridge_init(d->host_state.bus,
> > + PCI_DEVFN(1, 0), PCI_VENDOR_ID_SUN,
> > PCI_DEVICE_ID_SUN_SIMBA, 1, 1, pci_apb_map_irq,
> > "Advanced PCI Bus secondary bridge 1");
> > - *bus3 = pci_bridge_init(d->host_state.bus, 9, PCI_VENDOR_ID_SUN,
> > + *bus3 = pci_bridge_init(d->host_state.bus,
> > + PCI_DEVFN(1, 1), PCI_VENDOR_ID_SUN,
> > PCI_DEVICE_ID_SUN_SIMBA, 2, 2, pci_apb_map_irq,
> > "Advanced PCI Bus secondary bridge 2");
> >
> > diff --git a/hw/pci.c b/hw/pci.c
> > index b708d71..cc5738a 100644
> > --- a/hw/pci.c
> > +++ b/hw/pci.c
> > @@ -133,12 +133,17 @@ PCIBus *pci_find_host_bus(int domain)
> > return NULL;
> > }
> >
> > +static int pci_bus_get_instance_id(void)
> > +{
> > + static int nbus = 0;
> > + return nbus++;
> > +}
> > +
>
> I have not looked into this deeply, but looks scary.
> How does this interact with e.g. hotplug?
> How are these id's used? What if they overflow?
> If you just want a random number, let user pass it in?
Those fear sounds reasonable. I'll leave them untouched.
> > 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)
> > {
> > PCIBus *bus;
> > - static int nbus = 0;
> >
> > bus = FROM_QBUS(PCIBus, qbus_create(&pci_bus_info, parent, name));
> > bus->set_irq = set_irq;
> > @@ -154,7 +159,7 @@ PCIBus *pci_register_bus(DeviceState *parent, const
> > char *name,
> > QLIST_INIT(&bus->child);
> > pci_host_bus_register(0, bus); /* for now only pci domain 0 is
> > supported */
> >
> > - vmstate_register(nbus++, &vmstate_pcibus, bus);
> > + vmstate_register(pci_bus_get_instance_id(), &vmstate_pcibus, bus);
> > qemu_register_reset(pci_bus_reset, bus);
> > return bus;
> > }
> > @@ -162,7 +167,7 @@ PCIBus *pci_register_bus(DeviceState *parent, const
> > char *name,
> > static PCIBus *pci_register_secondary_bus(PCIBus *parent,
> > PCIDevice *dev,
> > pci_map_irq_fn map_irq,
> > - const char *name)
> > + const char *name, int devfn_min)
> > {
> > PCIBus *bus;
> >
> > @@ -172,8 +177,10 @@ static PCIBus *pci_register_secondary_bus(PCIBus
> > *parent,
> >
> > bus->bus_num = dev->config[PCI_SECONDARY_BUS];
> > bus->sub_bus = dev->config[PCI_SUBORDINATE_BUS];
> > + bus->devfn_min = devfn_min;
> > QLIST_INIT(&bus->child);
> > QLIST_INSERT_HEAD(&parent->child, bus, sibling);
> > + vmstate_register(pci_bus_get_instance_id(), &vmstate_pcibus, bus);
> > return bus;
> > }
> >
> > @@ -1154,7 +1161,7 @@ typedef struct {
> > PCIBus *bus;
> > } PCIBridge;
> >
> > -static void pci_bridge_write_config(PCIDevice *d,
> > +void pci_bridge_write_config(PCIDevice *d,
> > uint32_t address, uint32_t val, int len)
> > {
> > PCIBridge *s = (PCIBridge *)d;
> > @@ -1213,35 +1220,82 @@ PCIDevice *pci_find_device(PCIBus *bus, int
> > bus_num, int slot, int function)
> > return bus->devices[PCI_DEVFN(slot, 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)
> > +int pci_bridge_initfn(PCIDevice *pci_dev)
> > {
> > - PCIBridge *s;
> > - s = (PCIBridge *)pci_register_device(bus, name, sizeof(PCIBridge),
> > - devfn, NULL,
> > pci_bridge_write_config);
> > -
> > - pci_config_set_vendor_id(s->dev.config, vid);
> > - pci_config_set_device_id(s->dev.config, did);
> > -
> > - s->dev.config[0x04] = 0x06; // command = bus master, pci mem
> > - s->dev.config[0x05] = 0x00;
> > - s->dev.config[0x06] = 0xa0; // status = fast back-to-back, 66MHz, no
> > error
> > - s->dev.config[0x07] = 0x00; // status = fast devsel
> > - s->dev.config[0x08] = 0x00; // revision
> > - s->dev.config[0x09] = 0x00; // programming i/f
> > - pci_config_set_class(s->dev.config, PCI_CLASS_BRIDGE_PCI);
> > - s->dev.config[0x0D] = 0x10; // latency_timer
> > - s->dev.config[PCI_HEADER_TYPE] =
> > + uint8_t *pci_conf;
> > +
> > + pci_conf = pci_dev->config;
>
> This seems to just cause churn.
>
> > + pci_conf[0x04] = 0x06; // command = bus master, pci mem
> > + pci_conf[0x05] = 0x00;
> > + pci_conf[0x06] = 0xa0; // status = fast back-to-back, 66MHz, no error
> > + pci_conf[0x07] = 0x00; // status = fast devsel
> > + pci_conf[0x08] = 0x00; // revision
> > + pci_conf[0x09] = 0x00; // programming i/f
> > + pci_config_set_class(pci_conf, PCI_CLASS_BRIDGE_PCI);
> > + pci_conf[0x0D] = 0x10; // latency_timer
> > + pci_conf[PCI_HEADER_TYPE] =
> > PCI_HEADER_TYPE_MULTI_FUNCTION | PCI_HEADER_TYPE_BRIDGE; //
> > header_type
> > - s->dev.config[0x1E] = 0xa0; // secondary status
> > + pci_conf[0x18] = pci_dev->bus->bus_num;// primary bus number
> > + /* secondary bus and subordinate bus will be set by the caller */
> > + pci_conf[0x1E] = 0xa0; // secondary status
>
> If we are touching this code, maybe clean it up?
> - use symbolic constants
> - most registers are probably inited sanely by default already (e.g. 0)
> - get rid of // comments
Will do.
> > +
>
> kill empty line.
>
> > + /* vid/did will be set later */
>
> later when?
>
> > +
> > + return 0;
> > +}
> > +
> > +#define PCI_BRIDGE_DEFAULT "default PCI to PCI bridge"
> > +static PCIDeviceInfo pci_bridge_info_default = {
> > + .qdev.name = PCI_BRIDGE_DEFAULT,
> > + .qdev.size = sizeof(PCIBridge),
> > + .config_write = pci_bridge_write_config,
> > + .init = pci_bridge_initfn,
> > + .pcie = 0,
> > +};
> > +
> > +static void pci_bridge_register_device(void)
> > +{
> > + pci_qdev_register(&pci_bridge_info_default);
> > +}
> > +device_init(pci_bridge_register_device);
> > +
> > +PCIBus *pci_bridge_create_simple(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
> > *bus_name,
> > + const char *name)
> > +{
> > + DeviceState *qdev;
> > + uint8_t* pci_conf;
> > + PCIDevice *d;
> > + PCIBridge *br;
> > +
> > + qdev = qdev_create(&bus->qbus, name);
> > +
> > + qdev_prop_set_uint32(qdev, "addr", devfn);
>
> What is this "addr" property?
>
> > +
> > + qdev_init(qdev);
> > +
> > + d = DO_UPCAST(PCIDevice, qdev, qdev);
> > + pci_conf = d->config;
>
> this variable is not worth having
>
> > + pci_config_set_vendor_id(pci_conf, vid);
> > + pci_config_set_device_id(pci_conf, did);
> >
> > assert(sec_bus <= sub_bus);
> > - s->dev.config[PCI_SECONDARY_BUS] = sec_bus;
> > - s->dev.config[PCI_SUBORDINATE_BUS] = sub_bus;
> > + pci_set_byte(d->config + PCI_SECONDARY_BUS, sec_bus);
> > + pci_set_byte(d->config + PCI_SUBORDINATE_BUS, sub_bus);
>
> We don't particularly have to replace single byte
> accesses by pci_set_byte: it's there just so that
> if code uses others like pci_set_word, it can look
> uniform. This function doesn't though.
>
> > - s->bus = pci_register_secondary_bus(bus, &s->dev, map_irq, name);
> > - return s->bus;
> > + br = DO_UPCAST(PCIBridge, dev, d);
> > + br->bus = pci_register_secondary_bus(bus, d, map_irq, name, 0);
> > + return br->bus;
> > +}
> > +
> > +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)
> > +{
> > + return pci_bridge_create_simple(bus, devfn, vid, did, sec_bus, sub_bus,
> > + map_irq, name, PCI_BRIDGE_DEFAULT);
> > }
> >
> > static int pci_qdev_init(DeviceState *qdev, DeviceInfo *base)
> > diff --git a/hw/pci.h b/hw/pci.h
> > index 645dacd..be241b4 100644
> > --- a/hw/pci.h
> > +++ b/hw/pci.h
> > @@ -175,6 +175,8 @@ static inline int pci_bar_is_64bit(const PCIIORegion *r)
> > /* 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 */
> > +#define PCI_SUBORDINATE_BUS 0x1a /* Highest bus number behind the
> > bridge */
>
> seems to be duplicate
>
> > +
> >
>
> kill empty line
>
> > /* Size of the standard PCI config header */
> > #define PCI_CONFIG_HEADER_SIZE 0x40
> > @@ -304,6 +306,16 @@ 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);
> > +PCIBus *pci_bridge_create_simple(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
> > *bus_name,
> > + const char *name);
> > +void pci_bridge_write_config(PCIDevice *d,
> > + uint32_t address, uint32_t val, int len);
> > +int pci_bridge_initfn(PCIDevice *pci_dev);
> > +void pci_bridge_set_map_irq(PCIBus *bus, pci_map_irq_fn map_irq);
> > +
> >
> > static inline void
> > pci_set_byte(uint8_t *config, uint8_t val)
>
--
yamahata
- [Qemu-devel] Re: [PATCH 18/25] pci: add helper functions for pci config write function., (continued)
- [Qemu-devel] [PATCH 09/25] pci: introduce pcibus_t to represent pci bus address/size instead of uint32_t, Isaku Yamahata, 2009/10/02
- [Qemu-devel] [PATCH 11/25] pci: typedef pcibus_t as uint64_t instead of uint32_t., Isaku Yamahata, 2009/10/02
- [Qemu-devel] [PATCH 07/25] pci: helper functions to access PCIDevice::config, Isaku Yamahata, 2009/10/02
- [Qemu-devel] [PATCH 23/25] pci: add helper function to initialize wmask., Isaku Yamahata, 2009/10/02
- [Qemu-devel] [PATCH 22/25] pci/brdige: qdevfy and initialize secondary bus and subordinate bus., Isaku Yamahata, 2009/10/02
- [Qemu-devel] [PATCH 13/25] pci: make pci configuration transaction more accurate., Isaku Yamahata, 2009/10/02
- [Qemu-devel] [PATCH 24/25] pci: initialize wmask according to pci header type., Isaku Yamahata, 2009/10/02