[v2,5/9] msi: Invoke msi/msix_reset from PCI core

There is no point in pushing this burden to the devices, they may rather
forget to call them (like intel-hda and ahci ATM). Instead, reset
functions are now called from pci_device_reset and pci_bridge_reset.
They do nothing if the MSI/MSI-X is not in use.

CC: Alexander Graf <agraf@suse.de>
CC: Gerd Hoffmann <kraxel@redhat.com>
CC: Isaku Yamahata <yamahata@valinux.co.jp>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/ioh3420.c            |    2 +-
 hw/pci.c                |    5 +++++
 hw/pci_bridge.c         |    4 ++++
 hw/virtio-pci.c         |    1 -
 hw/xio3130_downstream.c |    2 +-
 hw/xio3130_upstream.c   |    1 -
 6 files changed, 11 insertions(+), 4 deletions(-)

On Wed, Jun 08, 2011 at 06:21:48PM +0200, Jan Kiszka wrote:
> There is no point in pushing this burden to the devices, they may rather
> forget to call them (like intel-hda and ahci ATM). Instead, reset
> functions are now called from pci_device_reset and pci_bridge_reset.
> They do nothing if the MSI/MSI-X is not in use.
> 
> CC: Alexander Graf <agraf@suse.de>
> CC: Gerd Hoffmann <kraxel@redhat.com>
> CC: Isaku Yamahata <yamahata@valinux.co.jp>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  hw/ioh3420.c            |    2 +-
>  hw/pci.c                |    5 +++++
>  hw/pci_bridge.c         |    4 ++++
>  hw/virtio-pci.c         |    1 -
>  hw/xio3130_downstream.c |    2 +-
>  hw/xio3130_upstream.c   |    1 -
>  6 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ioh3420.c b/hw/ioh3420.c
> index 95adf09..73bc55f 100644
> --- a/hw/ioh3420.c
> +++ b/hw/ioh3420.c
> @@ -81,7 +81,7 @@ static void ioh3420_write_config(PCIDevice *d,
>  static void ioh3420_reset(DeviceState *qdev)
>  {
>      PCIDevice *d = DO_UPCAST(PCIDevice, qdev, qdev);
> -    msi_reset(d);
> +
>      ioh3420_aer_vector_update(d);
>      pcie_cap_root_reset(d);
>      pcie_cap_deverr_reset(d);
> diff --git a/hw/pci.c b/hw/pci.c
> index 1d297d6..967f812 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -31,6 +31,8 @@
>  #include "loader.h"
>  #include "qemu-objects.h"
>  #include "range.h"
> +#include "msi.h"
> +#include "msix.h"
>  
>  //#define DEBUG_PCI
>  #ifdef DEBUG_PCI
> @@ -191,6 +193,9 @@ void pci_device_reset(PCIDevice *dev)
>          }
>      }
>      pci_update_mappings(dev);
> +
> +    msi_reset(dev);
> +    msix_reset(dev);
>  }
>  
>  /*

I am reluctant to add this dependency in pci.c:
this goes against the efforts to modularise the
pci subsystem. As I see it, pci.c should really only handle the common
header.  Instead I would like to add pci_caps.c and have
that call out to all the capabilities (not just reset, same
applies to other caps really).

I started doing that, if no one beats me to it,
I'll try to dust off that patch and post it next week.

> diff --git a/hw/pci_bridge.c b/hw/pci_bridge.c
> index 464d897..3951252 100644
> --- a/hw/pci_bridge.c
> +++ b/hw/pci_bridge.c
> @@ -32,6 +32,8 @@
>  #include "pci_bridge.h"
>  #include "pci_internals.h"
>  #include "range.h"
> +#include "msi.h"
> +#include "msix.h"
>  
>  /* PCI bridge subsystem vendor ID helper functions */
>  #define PCI_SSVID_SIZEOF        8
> @@ -224,6 +226,8 @@ void pci_bridge_reset(DeviceState *qdev)
>  {
>      PCIDevice *dev = DO_UPCAST(PCIDevice, qdev, qdev);
>      pci_bridge_reset_reg(dev);
> +    msi_reset(dev);
> +    msix_reset(dev);
>  }
>  
>  /* default qdev initialization function for PCI-to-PCI bridge */
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index c19629d..075d657 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -310,7 +310,6 @@ static void virtio_pci_reset(DeviceState *d)
>      VirtIOPCIProxy *proxy = container_of(d, VirtIOPCIProxy, pci_dev.qdev);
>      virtio_pci_stop_ioeventfd(proxy);
>      virtio_reset(proxy->vdev);
> -    msix_reset(&proxy->pci_dev);
>      proxy->flags &= ~VIRTIO_PCI_FLAG_BUS_MASTER_BUG;
>  }
>  
> diff --git a/hw/xio3130_downstream.c b/hw/xio3130_downstream.c
> index 5aa6a6b..a587c3e 100644
> --- a/hw/xio3130_downstream.c
> +++ b/hw/xio3130_downstream.c
> @@ -48,7 +48,7 @@ static void xio3130_downstream_write_config(PCIDevice *d, uint32_t address,
>  static void xio3130_downstream_reset(DeviceState *qdev)
>  {
>      PCIDevice *d = DO_UPCAST(PCIDevice, qdev, qdev);
> -    msi_reset(d);
> +
>      pcie_cap_deverr_reset(d);
>      pcie_cap_slot_reset(d);
>      pcie_cap_ari_reset(d);
> diff --git a/hw/xio3130_upstream.c b/hw/xio3130_upstream.c
> index a7640f5..9d75449 100644
> --- a/hw/xio3130_upstream.c
> +++ b/hw/xio3130_upstream.c
> @@ -47,7 +47,6 @@ static void xio3130_upstream_write_config(PCIDevice *d, uint32_t address,
>  static void xio3130_upstream_reset(DeviceState *qdev)
>  {
>      PCIDevice *d = DO_UPCAST(PCIDevice, qdev, qdev);
> -    msi_reset(d);
>      pci_bridge_reset(qdev);
>      pcie_cap_deverr_reset(d);
>  }
> -- 
> 1.7.1

On 2011-06-08 21:59, Michael S. Tsirkin wrote:
> On Wed, Jun 08, 2011 at 06:21:48PM +0200, Jan Kiszka wrote:
>> There is no point in pushing this burden to the devices, they may rather
>> forget to call them (like intel-hda and ahci ATM). Instead, reset
>> functions are now called from pci_device_reset and pci_bridge_reset.
>> They do nothing if the MSI/MSI-X is not in use.
>>
>> CC: Alexander Graf <agraf@suse.de>
>> CC: Gerd Hoffmann <kraxel@redhat.com>
>> CC: Isaku Yamahata <yamahata@valinux.co.jp>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>  hw/ioh3420.c            |    2 +-
>>  hw/pci.c                |    5 +++++
>>  hw/pci_bridge.c         |    4 ++++
>>  hw/virtio-pci.c         |    1 -
>>  hw/xio3130_downstream.c |    2 +-
>>  hw/xio3130_upstream.c   |    1 -
>>  6 files changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/ioh3420.c b/hw/ioh3420.c
>> index 95adf09..73bc55f 100644
>> --- a/hw/ioh3420.c
>> +++ b/hw/ioh3420.c
>> @@ -81,7 +81,7 @@ static void ioh3420_write_config(PCIDevice *d,
>>  static void ioh3420_reset(DeviceState *qdev)
>>  {
>>      PCIDevice *d = DO_UPCAST(PCIDevice, qdev, qdev);
>> -    msi_reset(d);
>> +
>>      ioh3420_aer_vector_update(d);
>>      pcie_cap_root_reset(d);
>>      pcie_cap_deverr_reset(d);
>> diff --git a/hw/pci.c b/hw/pci.c
>> index 1d297d6..967f812 100644
>> --- a/hw/pci.c
>> +++ b/hw/pci.c
>> @@ -31,6 +31,8 @@
>>  #include "loader.h"
>>  #include "qemu-objects.h"
>>  #include "range.h"
>> +#include "msi.h"
>> +#include "msix.h"
>>  
>>  //#define DEBUG_PCI
>>  #ifdef DEBUG_PCI
>> @@ -191,6 +193,9 @@ void pci_device_reset(PCIDevice *dev)
>>          }
>>      }
>>      pci_update_mappings(dev);
>> +
>> +    msi_reset(dev);
>> +    msix_reset(dev);
>>  }
>>  
>>  /*
> 
> I am reluctant to add this dependency in pci.c:
> this goes against the efforts to modularise the
> pci subsystem. As I see it, pci.c should really only handle the common
> header.  Instead I would like to add pci_caps.c and have
> that call out to all the capabilities (not just reset, same
> applies to other caps really).

Yeah, true. The result should just remain that devices no longer need to
worry about this.

> 
> I started doing that, if no one beats me to it,
> I'll try to dust off that patch and post it next week.

Would be welcome.

Jan