[5/7] pci: make use of qdev reset frame work to pci bus reset.

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(-)

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

On Thu, Nov 18, 2010 at 09:02:35AM +0200, Michael S. Tsirkin wrote:
> > +        /*
> > +         * 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?

I think only single function per a device should know all about reset
behavior and if a device overrides reset behavior, it should take care
of itself fully.
But it seems you don't think so. I can drop the following patch(6/7)
and eliminate this TODO comment.

On Thu, Nov 18, 2010 at 05:22:50PM +0900, Isaku Yamahata wrote:
> On Thu, Nov 18, 2010 at 09:02:35AM +0200, Michael S. Tsirkin wrote:
> > > +        /*
> > > +         * 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?
> 
> I think only single function per a device should know all about reset
> behavior and if a device overrides reset behavior, it should take care
> of itself fully.

Yes. However devices don't seem to override pci reset behavior
- instead they want a callback to reset the devicestate fields.

> But it seems you don't think so.  I can drop the following patch(6/7)
> and eliminate this TODO comment.

Yes, if everyone just calls default reset, let's invoke it from common
core. If we see some devices not call common reset, that's when we
better move it.