On Sun, Dec 04, 2011 at 02:22:12PM +0100, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kis...@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 <ag...@suse.de>
> CC: Gerd Hoffmann <kra...@redhat.com>
> CC: Isaku Yamahata <yamah...@valinux.co.jp>
> Signed-off-by: Jan Kiszka <jan.kis...@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
