Message ID | dbe02ee94b991cd5aed954979b57c997b6650ecf.1323004933.git.jan.kiszka@web.de |
---|---|
State | New |
Headers | show |
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 ...
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); }