[Qemu-devel] Re: [PATCH 5/7] pci: make use of qdev reset frame work to pci bus reset.

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Nov 17, 2010 at 01:50:25PM +0900, Isaku Yamahata wrote:
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  hw/pci.c |   38 ++++++++++++++++++++++++++++++++++----
>  hw/pci.h |    3 +++
>  2 files changed, 37 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index 962886e..b6f58de 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -43,12 +43,14 @@
>  
>  static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent);
>  static char *pcibus_get_dev_path(DeviceState *dev);
> +static int pcibus_reset(BusState *qbus);
>  
>  struct BusInfo pci_bus_info = {
>      .name       = "PCI",
>      .size       = sizeof(PCIBus),
>      .print_dev  = pcibus_dev_print,
>      .get_dev_path = pcibus_get_dev_path,
> +    .reset      = pcibus_reset,
>      .props      = (Property[]) {
>          DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
>          DEFINE_PROP_STRING("romfile", PCIDevice, romfile),
> @@ -133,7 +135,7 @@ static void pci_update_irq_status(PCIDevice *dev)
>      }
>  }
>  
> -static void pci_device_reset(PCIDevice *dev)
> +void pci_device_reset_default(PCIDevice *dev)
>  {
>      int r;
>  
> @@ -161,9 +163,29 @@ static void pci_device_reset(PCIDevice *dev)
>      pci_update_mappings(dev);
>  }
>  
> -static void pci_bus_reset(void *opaque)
> +static void pci_device_reset(PCIDevice *dev)
> +{
> +    if (!dev->qdev.info) {
> +        /* not all pci devices haven't been qdev'fied yet

Double negation :)

> +           TODO: remove this when all pci devices are qdev'fied. */
> +        pci_device_reset_default(dev);
> +    } else {
> +        /*
> +         * TODO:
> +         * each device should know what to do on RST#.
> +         * move pci_device_reset_default() into each callback.
> +         */

Is this doing anything besides give devices another way to shoot
themselves in the foot?  Handling this all in one place seems easier,
assuming everyone just calls pci_device_reset_default in the end.  Or do
you expect some devices to avoid calling pci_device_reset_default?

> +        qdev_reset_all(&dev->qdev);
> +        pci_device_reset_default(dev);
> +    }
> +}
> +
> +/*
> + * Trigger pci bus reset under a given bus.
> + * This functions emulates RST#.
> + */
> +void pci_bus_reset(PCIBus *bus)
>  {
> -    PCIBus *bus = opaque;
>      int i;
>  
>      for (i = 0; i < bus->nirq; i++) {
> @@ -176,6 +198,15 @@ static void pci_bus_reset(void *opaque)
>      }
>  }
>  
> +static int pcibus_reset(BusState *qbus)
> +{
> +    pci_bus_reset(DO_UPCAST(PCIBus, qbus, qbus));
> +
> +    /* topology traverse is done by pci_bus_reset().
> +       Tell qbus/qdev walker not to traverse the tree */
> +    return 1;
> +}
> +
>  static void pci_host_bus_register(int domain, PCIBus *bus)
>  {
>      struct PCIHostBus *host;
> @@ -230,7 +261,6 @@ void pci_bus_new_inplace(PCIBus *bus, DeviceState *parent,
>      pci_host_bus_register(0, bus); /* for now only pci domain 0 is supported */
>  
>      vmstate_register(NULL, -1, &vmstate_pcibus, bus);
> -    qemu_register_reset(pci_bus_reset, bus);
>  }
>  
>  PCIBus *pci_bus_new(DeviceState *parent, const char *name, int devfn_min)
> diff --git a/hw/pci.h b/hw/pci.h
> index 7100804..280a2f8 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -225,6 +225,9 @@ 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);
>  
> +void pci_bus_reset(PCIBus *bus);
> +void pci_device_reset_default(PCIDevice *dev);
> +
>  void pci_bus_set_mem_base(PCIBus *bus, target_phys_addr_t base);
>  
>  PCIDevice *pci_nic_init(NICInfo *nd, const char *default_model,
> -- 
> 1.7.1.1