[4/6] msi: Invoke msi/msix_reset from PCI core

From: Jan Kiszka <jan.kiszka@siemens.com>

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 Sun, Dec 04, 2011 at 02:22:12PM +0100, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> 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>

What makes me unhappy with this proposal is that msix_write_config, for
example, becomes in fact an internal interface. So devices should be
calling some functions like msix_init from msix.h, but not others like
msix_write_config.

It used to be simple: devices should call msix_.
Now, how are devices to figure it out?

E.g. the comment near msix_write_config says:
/* Handle MSI-X capability config write. */

This puts it at level 11 on Rusty's misuse scale:
Read the documentation and you will get it wrong.

So I tried writing a wapper, something like pci_capability.h, that would
hide the detail and handle all capabilities seamlessly.  Where I got
stuck was migration though, format is ordered so we can't just move the
fields around.  So I decided to wait until we switch to an unordered
format, then it'll become easy.

Thoughts?

> ---
>  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 a6bfbb9..fc2fb3b 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 399227f..5d5829d 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -31,6 +31,8 @@
>  #include "loader.h"
>  #include "range.h"
>  #include "qmp-commands.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);
>  }
>  
>  /*
> diff --git a/hw/pci_bridge.c b/hw/pci_bridge.c
> index 650d165..6799978 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
> @@ -296,6 +298,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 64c6a94..16a5b08 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -271,7 +271,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 d3c387d..464eefa 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 8283695..0d8d254 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.3.4

On 2011-12-04 15:24, Michael S. Tsirkin wrote:
> On Sun, Dec 04, 2011 at 02:22:12PM +0100, Jan Kiszka wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> 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>
> 
> What makes me unhappy with this proposal is that msix_write_config, for
> example, becomes in fact an internal interface. So devices should be
> calling some functions like msix_init from msix.h, but not others like
> msix_write_config.
> 
> It used to be simple: devices should call msix_.
> Now, how are devices to figure it out?
> 
> E.g. the comment near msix_write_config says:
> /* Handle MSI-X capability config write. */

That should be aligned to msi_write_config's comment.

My goal is to reduce the number of calls devices have to do in order to
use MSI. We have quite a few correct examples by now, so it should not
be too hard to figure out what to do to use standard MSI[X] services.

Maybe a PCI skeleton device model would help further. Or up-to-date
documentation, thought that may be even harder. ;)

> 
> This puts it at level 11 on Rusty's misuse scale:
> Read the documentation and you will get it wrong.
> 
> So I tried writing a wapper, something like pci_capability.h, that would
> hide the detail and handle all capabilities seamlessly.  Where I got
> stuck was migration though, format is ordered so we can't just move the
> fields around.  So I decided to wait until we switch to an unordered
> format, then it'll become easy.
> 
> Thoughts?

MSI-X save/restore is, well, unfortunate. Just like the whole PCI layer
in this regard. But I don't think that should block this particular step
as it frees device models from an unneeded burden.

Jan

On 2011-12-04 15:48, Michael S. Tsirkin wrote:
> On Sun, Dec 04, 2011 at 03:35:38PM +0100, Jan Kiszka wrote:
>> On 2011-12-04 15:24, Michael S. Tsirkin wrote:
>>> On Sun, Dec 04, 2011 at 02:22:12PM +0100, Jan Kiszka wrote:
>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>
>>>> 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>
>>>
>>> What makes me unhappy with this proposal is that msix_write_config, for
>>> example, becomes in fact an internal interface. So devices should be
>>> calling some functions like msix_init from msix.h, but not others like
>>> msix_write_config.
>>>
>>> It used to be simple: devices should call msix_.
>>> Now, how are devices to figure it out?
>>>
>>> E.g. the comment near msix_write_config says:
>>> /* Handle MSI-X capability config write. */
>>
>> That should be aligned to msi_write_config's comment.
>>
>> My goal is to reduce the number of calls devices have to do in order to
>> use MSI. We have quite a few correct examples by now, so it should not
>> be too hard to figure out what to do to use standard MSI[X] services.
>>
>> Maybe a PCI skeleton device model would help further. Or up-to-date
>> documentation, thought that may be even harder. ;)
> 
> Maybe it's time to move code into hw/pci/ ?
> Then we could have private interfaces without
> kludges like pci_internals.h ...
> 

Sounds reasonable.

Jan

On Sun, Dec 04, 2011 at 03:35:38PM +0100, Jan Kiszka wrote:
> On 2011-12-04 15:24, Michael S. Tsirkin wrote:
> > On Sun, Dec 04, 2011 at 02:22:12PM +0100, Jan Kiszka wrote:
> >> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>
> >> 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>
> > 
> > What makes me unhappy with this proposal is that msix_write_config, for
> > example, becomes in fact an internal interface. So devices should be
> > calling some functions like msix_init from msix.h, but not others like
> > msix_write_config.
> > 
> > It used to be simple: devices should call msix_.
> > Now, how are devices to figure it out?
> > 
> > E.g. the comment near msix_write_config says:
> > /* Handle MSI-X capability config write. */
> 
> That should be aligned to msi_write_config's comment.
> 
> My goal is to reduce the number of calls devices have to do in order to
> use MSI. We have quite a few correct examples by now, so it should not
> be too hard to figure out what to do to use standard MSI[X] services.
> 
> Maybe a PCI skeleton device model would help further. Or up-to-date
> documentation, thought that may be even harder. ;)

Maybe it's time to move code into hw/pci/ ?
Then we could have private interfaces without
kludges like pci_internals.h ...