[Qemu-devel] Re: [PATCH 21/23] pci/brdige: qdevfy and initialize secondary bus and subordinate bus.

[Michael S. Tsirkin]

On Mon, Oct 05, 2009 at 07:07:01PM +0900, Isaku Yamahata wrote:
> qdevfy pci bridge, and initialize secondary bus and subordinate bus
> in configuration space.

Why do you initialize these? Should not guest do this?

> And some clean up to use symbolic constant and to remove // comments.

You also apparently did some unrelated changes. Pls document.

> Signed-off-by: Isaku Yamahata <address@hidden>
> ---
>  hw/pci.c |   85 
> +++++++++++++++++++++++++++++++++++++++++++-------------------
>  hw/pci.h |    9 ++++++
>  2 files changed, 68 insertions(+), 26 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index af864c6..fb93b99 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -174,6 +174,7 @@ static PCIBus *pci_register_secondary_bus(PCIBus *parent,
>      bus->sub_bus = dev->config[PCI_SUBORDINATE_BUS];
>      QLIST_INIT(&bus->child);
>      QLIST_INSERT_HEAD(&parent->child, bus, sibling);
> +    vmstate_register(-1, &vmstate_pcibus, bus);
>      return bus;
>  }
>  
> @@ -1140,7 +1141,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;
> @@ -1193,35 +1194,67 @@ 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)
> +{
> +    uint8_t *pci_conf = pci_dev->config;

We don't need this variable IMO.

> +
> +    /* secondary bus, subordinate bus and vid/did will be set
> +       by pci_bridge_create_simple(), the caller of qdev_init() */
> +    pci_conf[PCI_COMMAND] = PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER;
> +    pci_conf[PCI_STATUS] = PCI_STATUS_66MHZ | PCI_STATUS_FAST_BACK;

command and status are words, so it is better to use
pci_set_word here, even if specific values
happen to be in the 1'st byte.

> +    pci_config_set_class(pci_conf, PCI_CLASS_BRIDGE_PCI);
> +    pci_conf[PCI_LATENCY_TIMER] = 0x10;

Why 0x10?

> +    pci_conf[PCI_HEADER_TYPE] =
> +        PCI_HEADER_TYPE_MULTI_FUNCTION | PCI_HEADER_TYPE_BRIDGE;

Any idea why it's multifunction?
Many (most?) bridges have a single function ...

> +    pci_conf[PCI_PRIMARY_BUS] = pci_dev->bus->bus_num;

I think this is 0 at reset, and assigned by BIOS.

> +    pci_conf[PCI_SEC_STATUS] = PCI_STATUS_66MHZ | PCI_STATUS_FAST_BACK;

pci_set_word.

> +
> +    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)
>  {
> -    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] =
> -        PCI_HEADER_TYPE_MULTI_FUNCTION | PCI_HEADER_TYPE_BRIDGE; // 
> header_type
> -    s->dev.config[0x1E] = 0xa0; // secondary status
> +    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)
> +{
> +    PCIDevice *d;
> +    PCIBridge *br;
> +
> +    d = pci_create_simple(bus, devfn, name);
>  
> +    pci_config_set_vendor_id(d->config, vid);
> +    pci_config_set_device_id(d->config, did);
>      assert(sec_bus <= sub_bus);
> -    s->dev.config[PCI_SECONDARY_BUS] = sec_bus;
> -    s->dev.config[PCI_SUBORDINATE_BUS] = sub_bus;
> +    d->config[PCI_SECONDARY_BUS] = sec_bus;
> +    d->config[PCI_SUBORDINATE_BUS] = sub_bus;
>  
> -    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);
> +    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 1d45437..5cf0b59 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -294,6 +294,15 @@ 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)
> -- 
> 1.6.0.2