| Message ID | 4DB80059.7080308@siemens.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Apr 27, 2011 at 01:39:05PM +0200, Jan Kiszka wrote: > On 2011-04-27 11:34, Avi Kivity wrote: > > On 04/26/2011 04:19 PM, Jan Kiszka wrote: > >> I've still plans to consolidate MSI-X mask notifiers and KVM hooks, but > >> that can wait until we go upstream. > >> > >> This version still makes classic MSI usable in irqchip mode, now not > >> only for PCI devices (AHCI, HDA) but also for the HPET (with msi=on). > >> Moreover, it contains an additional patch to refresh the MSI IRQ routes > >> after vmload. > >> > > > > Immediately after migration: > > > > Program terminated with signal 11, Segmentation fault. > > #0 le32_to_cpupu (vector=0, kmm=0x0, dev=<value optimized out>) at > > ../bswap.h:178 > > 178 return p1[0] | (p1[1] << 8) | (p1[2] << 16) | (p1[3] << 24); > > Missing separate debuginfos, use: debuginfo-install > > SDL-1.2.14-10.fc14.x86_64 cyrus-sasl-lib-2.1.23-12.fc14.x86_64 > > cyrus-sasl-plain-2.1.23-12.fc14.x86_64 db4-4.8.30-2.fc14.x86_64 > > glibc-2.13-1.x86_64 gnutls-2.8.6-2.fc14.x86_64 > > keyutils-libs-1.2-6.fc12.x86_64 krb5-libs-1.8.2-9.fc14.x86_64 > > libX11-1.3.4-4.fc14.x86_64 libXau-1.0.6-1.fc14.x86_64 > > libaio-0.3.109-2.fc13.x86_64 libcom_err-1.41.12-6.fc14.x86_64 > > libcurl-7.21.0-6.fc14.x86_64 libgcc-4.5.1-4.fc14.x86_64 > > libgcrypt-1.4.5-4.fc13.x86_64 libgpg-error-1.9-1.fc14.x86_64 > > libidn-1.18-1.fc14.x86_64 libpng-1.2.44-1.fc14.x86_64 > > libselinux-2.0.96-6.fc14.1.x86_64 libssh2-1.2.4-1.fc14.x86_64 > > libtasn1-2.7-1.fc14.x86_64 libxcb-1.7-1.fc14.x86_64 > > ncurses-libs-5.7-9.20100703.fc14.x86_64 nspr-4.8.7-1.fc14.x86_64 > > nss-3.12.9-9.fc14.x86_64 nss-softokn-freebl-3.12.9-5.fc14.x86_64 > > nss-util-3.12.9-1.fc14.x86_64 openldap-2.4.23-4.fc14.x86_64 > > openssl-1.0.0d-1.fc14.x86_64 zlib-1.2.5-2.fc14.x86_64 > > (gdb) bt > > #0 le32_to_cpupu (vector=0, kmm=0x0, dev=<value optimized out>) at > > ../bswap.h:178 > > #1 pci_get_long (vector=0, kmm=0x0, dev=<value optimized out>) at > > /build/home/tlv/akivity/qemu-kvm/hw/pci.h:326 > > #2 kvm_msi_message_from_vector (vector=0, kmm=0x0, dev=<value optimized > > out>) at /build/home/tlv/akivity/qemu-kvm/hw/msi.c:120 > > #3 0x000000000057d59c in kvm_msi_update (dev=0x23eabb0) at > > /build/home/tlv/akivity/qemu-kvm/hw/msi.c:152 > > #4 0x000000000041e29b in get_pci_config_device (f=0x2466380, > > pv=0x23eac28, size=256) at /build/home/tlv/akivity/qemu-kvm/hw/pci.c:346 > > #5 0x000000000049c36c in vmstate_load_state (f=0x2466380, > > vmsd=0x5fb880, opaque=0x23eabb0, version_id=2) at savevm.c:1374 > > #6 0x000000000049c323 in vmstate_load_state (f=0x2466380, > > vmsd=0x6f07c0, opaque=0x23eabb0, version_id=3) at savevm.c:1372 > > #7 0x000000000049cf84 in vmstate_load (f=0x2466380) at savevm.c:1450 > > #8 qemu_loadvm_state (f=0x2466380) at savevm.c:1817 > > #9 0x0000000000493e69 in process_incoming_migration (f=<value optimized > > out>) at migration.c:66 > > #10 0x0000000000494b97 in tcp_accept_incoming_migration (opaque=<value > > optimized out>) at migration-tcp.c:163 > > #11 0x00000000004a3fa7 in qemu_iohandler_poll (readfds=0x7fff56dc0430, > > writefds=0x7fff56dc03b0, xfds=<value optimized out>, > > ret=<value optimized out>) at iohandler.c:120 > > #12 0x000000000041944a in main_loop_wait (nonblocking=<value optimized > > out>) at /build/home/tlv/akivity/qemu-kvm/vl.c:1336 > > #13 0x0000000000433a97 in kvm_main_loop () at > > /build/home/tlv/akivity/qemu-kvm/qemu-kvm.c:1588 > > #14 0x000000000041a3a6 in main_loop (argc=<value optimized out>, > > argv=<value optimized out>, envp=<value optimized out>) > > at /build/home/tlv/akivity/qemu-kvm/vl.c:1369 > > #15 main (argc=<value optimized out>, argv=<value optimized out>, > > envp=<value optimized out>) at /build/home/tlv/akivity/qemu-kvm/vl.c:3257 > > > > (gdb) fr > > #3 0x000000000057d59c in kvm_msi_update (dev=0x23eabb0) at > > /build/home/tlv/akivity/qemu-kvm/hw/msi.c:152 > > (gdb) p dev.msi_irq_entries > > $10 = (struct KVMMsiMessage *) 0x0 > > > > dev points to the i440fx chipset device. > > > > Yeah, better use this version of patch 8. > > Jan > > ----8<----- > > Establish a post-load notification for the MSI subsystem so that KVM can > refresh its IRQ routing after vmload. > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > --- > > v3: Fix null-pointer deref after vmload by checking for availability of > msi routing entries. > > hw/msi.c | 13 +++++++++++++ > hw/msi.h | 1 + > hw/pci.c | 2 ++ > 3 files changed, 16 insertions(+), 0 deletions(-) > > diff --git a/hw/msi.c b/hw/msi.c > index 18f683b..725b2b7 100644 > --- a/hw/msi.c > +++ b/hw/msi.c > @@ -453,3 +453,16 @@ unsigned int msi_nr_vectors_allocated(const PCIDevice *dev) > uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev)); > return msi_nr_vectors(flags); > } > + > +void msi_post_load(PCIDevice *dev) > +{ > + uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev)); > + > + if (kvm_enabled() && dev->msi_irq_entries) { > + kvm_msi_free(dev); > + > + if (flags & PCI_MSI_FLAGS_ENABLE) { > + kvm_msi_update(dev); > + } > + } > +} > diff --git a/hw/msi.h b/hw/msi.h > index 5766018..6ff0607 100644 > --- a/hw/msi.h > +++ b/hw/msi.h > @@ -32,6 +32,7 @@ void msi_reset(PCIDevice *dev); > void msi_notify(PCIDevice *dev, unsigned int vector); > void msi_write_config(PCIDevice *dev, uint32_t addr, uint32_t val, int len); > unsigned int msi_nr_vectors_allocated(const PCIDevice *dev); > +void msi_post_load(PCIDevice *dev); > > static inline bool msi_present(const PCIDevice *dev) > { > diff --git a/hw/pci.c b/hw/pci.c > index 82e0300..07ec4f9 100644 > --- a/hw/pci.c > +++ b/hw/pci.c > @@ -34,6 +34,7 @@ > #include "device-assignment.h" > #include "qemu-objects.h" > #include "range.h" > +#include "msi.h" > > //#define DEBUG_PCI > #ifdef DEBUG_PCI > @@ -342,6 +343,7 @@ static int get_pci_config_device(QEMUFile *f, void *pv, size_t size) > memcpy(s->config, config, size); > > pci_update_mappings(s); > + msi_post_load(s); Pls don't do this: I'm trying to keep just the core in pci.c and all capabilities in separate files. msix has msix_load, msi will just need one too, and let all devices call that. > > qemu_free(config); > return 0; > -- > 1.7.1 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2011-04-27 16:16, Michael S. Tsirkin wrote: > On Wed, Apr 27, 2011 at 01:39:05PM +0200, Jan Kiszka wrote: >> On 2011-04-27 11:34, Avi Kivity wrote: >>> On 04/26/2011 04:19 PM, Jan Kiszka wrote: >>>> I've still plans to consolidate MSI-X mask notifiers and KVM hooks, but >>>> that can wait until we go upstream. >>>> >>>> This version still makes classic MSI usable in irqchip mode, now not >>>> only for PCI devices (AHCI, HDA) but also for the HPET (with msi=on). >>>> Moreover, it contains an additional patch to refresh the MSI IRQ routes >>>> after vmload. >>>> >>> >>> Immediately after migration: >>> >>> Program terminated with signal 11, Segmentation fault. >>> #0 le32_to_cpupu (vector=0, kmm=0x0, dev=<value optimized out>) at >>> ../bswap.h:178 >>> 178 return p1[0] | (p1[1] << 8) | (p1[2] << 16) | (p1[3] << 24); >>> Missing separate debuginfos, use: debuginfo-install >>> SDL-1.2.14-10.fc14.x86_64 cyrus-sasl-lib-2.1.23-12.fc14.x86_64 >>> cyrus-sasl-plain-2.1.23-12.fc14.x86_64 db4-4.8.30-2.fc14.x86_64 >>> glibc-2.13-1.x86_64 gnutls-2.8.6-2.fc14.x86_64 >>> keyutils-libs-1.2-6.fc12.x86_64 krb5-libs-1.8.2-9.fc14.x86_64 >>> libX11-1.3.4-4.fc14.x86_64 libXau-1.0.6-1.fc14.x86_64 >>> libaio-0.3.109-2.fc13.x86_64 libcom_err-1.41.12-6.fc14.x86_64 >>> libcurl-7.21.0-6.fc14.x86_64 libgcc-4.5.1-4.fc14.x86_64 >>> libgcrypt-1.4.5-4.fc13.x86_64 libgpg-error-1.9-1.fc14.x86_64 >>> libidn-1.18-1.fc14.x86_64 libpng-1.2.44-1.fc14.x86_64 >>> libselinux-2.0.96-6.fc14.1.x86_64 libssh2-1.2.4-1.fc14.x86_64 >>> libtasn1-2.7-1.fc14.x86_64 libxcb-1.7-1.fc14.x86_64 >>> ncurses-libs-5.7-9.20100703.fc14.x86_64 nspr-4.8.7-1.fc14.x86_64 >>> nss-3.12.9-9.fc14.x86_64 nss-softokn-freebl-3.12.9-5.fc14.x86_64 >>> nss-util-3.12.9-1.fc14.x86_64 openldap-2.4.23-4.fc14.x86_64 >>> openssl-1.0.0d-1.fc14.x86_64 zlib-1.2.5-2.fc14.x86_64 >>> (gdb) bt >>> #0 le32_to_cpupu (vector=0, kmm=0x0, dev=<value optimized out>) at >>> ../bswap.h:178 >>> #1 pci_get_long (vector=0, kmm=0x0, dev=<value optimized out>) at >>> /build/home/tlv/akivity/qemu-kvm/hw/pci.h:326 >>> #2 kvm_msi_message_from_vector (vector=0, kmm=0x0, dev=<value optimized >>> out>) at /build/home/tlv/akivity/qemu-kvm/hw/msi.c:120 >>> #3 0x000000000057d59c in kvm_msi_update (dev=0x23eabb0) at >>> /build/home/tlv/akivity/qemu-kvm/hw/msi.c:152 >>> #4 0x000000000041e29b in get_pci_config_device (f=0x2466380, >>> pv=0x23eac28, size=256) at /build/home/tlv/akivity/qemu-kvm/hw/pci.c:346 >>> #5 0x000000000049c36c in vmstate_load_state (f=0x2466380, >>> vmsd=0x5fb880, opaque=0x23eabb0, version_id=2) at savevm.c:1374 >>> #6 0x000000000049c323 in vmstate_load_state (f=0x2466380, >>> vmsd=0x6f07c0, opaque=0x23eabb0, version_id=3) at savevm.c:1372 >>> #7 0x000000000049cf84 in vmstate_load (f=0x2466380) at savevm.c:1450 >>> #8 qemu_loadvm_state (f=0x2466380) at savevm.c:1817 >>> #9 0x0000000000493e69 in process_incoming_migration (f=<value optimized >>> out>) at migration.c:66 >>> #10 0x0000000000494b97 in tcp_accept_incoming_migration (opaque=<value >>> optimized out>) at migration-tcp.c:163 >>> #11 0x00000000004a3fa7 in qemu_iohandler_poll (readfds=0x7fff56dc0430, >>> writefds=0x7fff56dc03b0, xfds=<value optimized out>, >>> ret=<value optimized out>) at iohandler.c:120 >>> #12 0x000000000041944a in main_loop_wait (nonblocking=<value optimized >>> out>) at /build/home/tlv/akivity/qemu-kvm/vl.c:1336 >>> #13 0x0000000000433a97 in kvm_main_loop () at >>> /build/home/tlv/akivity/qemu-kvm/qemu-kvm.c:1588 >>> #14 0x000000000041a3a6 in main_loop (argc=<value optimized out>, >>> argv=<value optimized out>, envp=<value optimized out>) >>> at /build/home/tlv/akivity/qemu-kvm/vl.c:1369 >>> #15 main (argc=<value optimized out>, argv=<value optimized out>, >>> envp=<value optimized out>) at /build/home/tlv/akivity/qemu-kvm/vl.c:3257 >>> >>> (gdb) fr >>> #3 0x000000000057d59c in kvm_msi_update (dev=0x23eabb0) at >>> /build/home/tlv/akivity/qemu-kvm/hw/msi.c:152 >>> (gdb) p dev.msi_irq_entries >>> $10 = (struct KVMMsiMessage *) 0x0 >>> >>> dev points to the i440fx chipset device. >>> >> >> Yeah, better use this version of patch 8. >> >> Jan >> >> ----8<----- >> >> Establish a post-load notification for the MSI subsystem so that KVM can >> refresh its IRQ routing after vmload. >> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> >> --- >> >> v3: Fix null-pointer deref after vmload by checking for availability of >> msi routing entries. >> >> hw/msi.c | 13 +++++++++++++ >> hw/msi.h | 1 + >> hw/pci.c | 2 ++ >> 3 files changed, 16 insertions(+), 0 deletions(-) >> >> diff --git a/hw/msi.c b/hw/msi.c >> index 18f683b..725b2b7 100644 >> --- a/hw/msi.c >> +++ b/hw/msi.c >> @@ -453,3 +453,16 @@ unsigned int msi_nr_vectors_allocated(const PCIDevice *dev) >> uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev)); >> return msi_nr_vectors(flags); >> } >> + >> +void msi_post_load(PCIDevice *dev) >> +{ >> + uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev)); >> + >> + if (kvm_enabled() && dev->msi_irq_entries) { >> + kvm_msi_free(dev); >> + >> + if (flags & PCI_MSI_FLAGS_ENABLE) { >> + kvm_msi_update(dev); >> + } >> + } >> +} >> diff --git a/hw/msi.h b/hw/msi.h >> index 5766018..6ff0607 100644 >> --- a/hw/msi.h >> +++ b/hw/msi.h >> @@ -32,6 +32,7 @@ void msi_reset(PCIDevice *dev); >> void msi_notify(PCIDevice *dev, unsigned int vector); >> void msi_write_config(PCIDevice *dev, uint32_t addr, uint32_t val, int len); >> unsigned int msi_nr_vectors_allocated(const PCIDevice *dev); >> +void msi_post_load(PCIDevice *dev); >> >> static inline bool msi_present(const PCIDevice *dev) >> { >> diff --git a/hw/pci.c b/hw/pci.c >> index 82e0300..07ec4f9 100644 >> --- a/hw/pci.c >> +++ b/hw/pci.c >> @@ -34,6 +34,7 @@ >> #include "device-assignment.h" >> #include "qemu-objects.h" >> #include "range.h" >> +#include "msi.h" >> >> //#define DEBUG_PCI >> #ifdef DEBUG_PCI >> @@ -342,6 +343,7 @@ static int get_pci_config_device(QEMUFile *f, void *pv, size_t size) >> memcpy(s->config, config, size); >> >> pci_update_mappings(s); >> + msi_post_load(s); > > Pls don't do this: I'm trying to keep just the core in > pci.c and all capabilities in separate files. > msix has msix_load, msi will just need one too, > and let all devices call that. > Preferred alternatives are...? Registering a vmstate for msi? Jan
On Wed, Apr 27, 2011 at 04:28:56PM +0200, Jan Kiszka wrote: > On 2011-04-27 16:16, Michael S. Tsirkin wrote: > > On Wed, Apr 27, 2011 at 01:39:05PM +0200, Jan Kiszka wrote: > >> On 2011-04-27 11:34, Avi Kivity wrote: > >>> On 04/26/2011 04:19 PM, Jan Kiszka wrote: > >>>> I've still plans to consolidate MSI-X mask notifiers and KVM hooks, but > >>>> that can wait until we go upstream. > >>>> > >>>> This version still makes classic MSI usable in irqchip mode, now not > >>>> only for PCI devices (AHCI, HDA) but also for the HPET (with msi=on). > >>>> Moreover, it contains an additional patch to refresh the MSI IRQ routes > >>>> after vmload. > >>>> > >>> > >>> Immediately after migration: > >>> > >>> Program terminated with signal 11, Segmentation fault. > >>> #0 le32_to_cpupu (vector=0, kmm=0x0, dev=<value optimized out>) at > >>> ../bswap.h:178 > >>> 178 return p1[0] | (p1[1] << 8) | (p1[2] << 16) | (p1[3] << 24); > >>> Missing separate debuginfos, use: debuginfo-install > >>> SDL-1.2.14-10.fc14.x86_64 cyrus-sasl-lib-2.1.23-12.fc14.x86_64 > >>> cyrus-sasl-plain-2.1.23-12.fc14.x86_64 db4-4.8.30-2.fc14.x86_64 > >>> glibc-2.13-1.x86_64 gnutls-2.8.6-2.fc14.x86_64 > >>> keyutils-libs-1.2-6.fc12.x86_64 krb5-libs-1.8.2-9.fc14.x86_64 > >>> libX11-1.3.4-4.fc14.x86_64 libXau-1.0.6-1.fc14.x86_64 > >>> libaio-0.3.109-2.fc13.x86_64 libcom_err-1.41.12-6.fc14.x86_64 > >>> libcurl-7.21.0-6.fc14.x86_64 libgcc-4.5.1-4.fc14.x86_64 > >>> libgcrypt-1.4.5-4.fc13.x86_64 libgpg-error-1.9-1.fc14.x86_64 > >>> libidn-1.18-1.fc14.x86_64 libpng-1.2.44-1.fc14.x86_64 > >>> libselinux-2.0.96-6.fc14.1.x86_64 libssh2-1.2.4-1.fc14.x86_64 > >>> libtasn1-2.7-1.fc14.x86_64 libxcb-1.7-1.fc14.x86_64 > >>> ncurses-libs-5.7-9.20100703.fc14.x86_64 nspr-4.8.7-1.fc14.x86_64 > >>> nss-3.12.9-9.fc14.x86_64 nss-softokn-freebl-3.12.9-5.fc14.x86_64 > >>> nss-util-3.12.9-1.fc14.x86_64 openldap-2.4.23-4.fc14.x86_64 > >>> openssl-1.0.0d-1.fc14.x86_64 zlib-1.2.5-2.fc14.x86_64 > >>> (gdb) bt > >>> #0 le32_to_cpupu (vector=0, kmm=0x0, dev=<value optimized out>) at > >>> ../bswap.h:178 > >>> #1 pci_get_long (vector=0, kmm=0x0, dev=<value optimized out>) at > >>> /build/home/tlv/akivity/qemu-kvm/hw/pci.h:326 > >>> #2 kvm_msi_message_from_vector (vector=0, kmm=0x0, dev=<value optimized > >>> out>) at /build/home/tlv/akivity/qemu-kvm/hw/msi.c:120 > >>> #3 0x000000000057d59c in kvm_msi_update (dev=0x23eabb0) at > >>> /build/home/tlv/akivity/qemu-kvm/hw/msi.c:152 > >>> #4 0x000000000041e29b in get_pci_config_device (f=0x2466380, > >>> pv=0x23eac28, size=256) at /build/home/tlv/akivity/qemu-kvm/hw/pci.c:346 > >>> #5 0x000000000049c36c in vmstate_load_state (f=0x2466380, > >>> vmsd=0x5fb880, opaque=0x23eabb0, version_id=2) at savevm.c:1374 > >>> #6 0x000000000049c323 in vmstate_load_state (f=0x2466380, > >>> vmsd=0x6f07c0, opaque=0x23eabb0, version_id=3) at savevm.c:1372 > >>> #7 0x000000000049cf84 in vmstate_load (f=0x2466380) at savevm.c:1450 > >>> #8 qemu_loadvm_state (f=0x2466380) at savevm.c:1817 > >>> #9 0x0000000000493e69 in process_incoming_migration (f=<value optimized > >>> out>) at migration.c:66 > >>> #10 0x0000000000494b97 in tcp_accept_incoming_migration (opaque=<value > >>> optimized out>) at migration-tcp.c:163 > >>> #11 0x00000000004a3fa7 in qemu_iohandler_poll (readfds=0x7fff56dc0430, > >>> writefds=0x7fff56dc03b0, xfds=<value optimized out>, > >>> ret=<value optimized out>) at iohandler.c:120 > >>> #12 0x000000000041944a in main_loop_wait (nonblocking=<value optimized > >>> out>) at /build/home/tlv/akivity/qemu-kvm/vl.c:1336 > >>> #13 0x0000000000433a97 in kvm_main_loop () at > >>> /build/home/tlv/akivity/qemu-kvm/qemu-kvm.c:1588 > >>> #14 0x000000000041a3a6 in main_loop (argc=<value optimized out>, > >>> argv=<value optimized out>, envp=<value optimized out>) > >>> at /build/home/tlv/akivity/qemu-kvm/vl.c:1369 > >>> #15 main (argc=<value optimized out>, argv=<value optimized out>, > >>> envp=<value optimized out>) at /build/home/tlv/akivity/qemu-kvm/vl.c:3257 > >>> > >>> (gdb) fr > >>> #3 0x000000000057d59c in kvm_msi_update (dev=0x23eabb0) at > >>> /build/home/tlv/akivity/qemu-kvm/hw/msi.c:152 > >>> (gdb) p dev.msi_irq_entries > >>> $10 = (struct KVMMsiMessage *) 0x0 > >>> > >>> dev points to the i440fx chipset device. > >>> > >> > >> Yeah, better use this version of patch 8. > >> > >> Jan > >> > >> ----8<----- > >> > >> Establish a post-load notification for the MSI subsystem so that KVM can > >> refresh its IRQ routing after vmload. > >> > >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > >> --- > >> > >> v3: Fix null-pointer deref after vmload by checking for availability of > >> msi routing entries. > >> > >> hw/msi.c | 13 +++++++++++++ > >> hw/msi.h | 1 + > >> hw/pci.c | 2 ++ > >> 3 files changed, 16 insertions(+), 0 deletions(-) > >> > >> diff --git a/hw/msi.c b/hw/msi.c > >> index 18f683b..725b2b7 100644 > >> --- a/hw/msi.c > >> +++ b/hw/msi.c > >> @@ -453,3 +453,16 @@ unsigned int msi_nr_vectors_allocated(const PCIDevice *dev) > >> uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev)); > >> return msi_nr_vectors(flags); > >> } > >> + > >> +void msi_post_load(PCIDevice *dev) > >> +{ > >> + uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev)); > >> + > >> + if (kvm_enabled() && dev->msi_irq_entries) { > >> + kvm_msi_free(dev); > >> + > >> + if (flags & PCI_MSI_FLAGS_ENABLE) { > >> + kvm_msi_update(dev); > >> + } > >> + } > >> +} > >> diff --git a/hw/msi.h b/hw/msi.h > >> index 5766018..6ff0607 100644 > >> --- a/hw/msi.h > >> +++ b/hw/msi.h > >> @@ -32,6 +32,7 @@ void msi_reset(PCIDevice *dev); > >> void msi_notify(PCIDevice *dev, unsigned int vector); > >> void msi_write_config(PCIDevice *dev, uint32_t addr, uint32_t val, int len); > >> unsigned int msi_nr_vectors_allocated(const PCIDevice *dev); > >> +void msi_post_load(PCIDevice *dev); > >> > >> static inline bool msi_present(const PCIDevice *dev) > >> { > >> diff --git a/hw/pci.c b/hw/pci.c > >> index 82e0300..07ec4f9 100644 > >> --- a/hw/pci.c > >> +++ b/hw/pci.c > >> @@ -34,6 +34,7 @@ > >> #include "device-assignment.h" > >> #include "qemu-objects.h" > >> #include "range.h" > >> +#include "msi.h" > >> > >> //#define DEBUG_PCI > >> #ifdef DEBUG_PCI > >> @@ -342,6 +343,7 @@ static int get_pci_config_device(QEMUFile *f, void *pv, size_t size) > >> memcpy(s->config, config, size); > >> > >> pci_update_mappings(s); > >> + msi_post_load(s); > > > > Pls don't do this: I'm trying to keep just the core in > > pci.c and all capabilities in separate files. > > msix has msix_load, msi will just need one too, > > and let all devices call that. > > > > Preferred alternatives are...? Registering a vmstate for msi? > > Jan Add msi_load and call that from devices that need it. Like msix_load does now. > -- > Siemens AG, Corporate Technology, CT T DE IT 1 > Corporate Competence Center Embedded Linux -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2011-04-27 16:30, Michael S. Tsirkin wrote: >>>> --- a/hw/pci.c >>>> +++ b/hw/pci.c >>>> @@ -34,6 +34,7 @@ >>>> #include "device-assignment.h" >>>> #include "qemu-objects.h" >>>> #include "range.h" >>>> +#include "msi.h" >>>> >>>> //#define DEBUG_PCI >>>> #ifdef DEBUG_PCI >>>> @@ -342,6 +343,7 @@ static int get_pci_config_device(QEMUFile *f, void *pv, size_t size) >>>> memcpy(s->config, config, size); >>>> >>>> pci_update_mappings(s); >>>> + msi_post_load(s); >>> >>> Pls don't do this: I'm trying to keep just the core in >>> pci.c and all capabilities in separate files. >>> msix has msix_load, msi will just need one too, >>> and let all devices call that. >>> >> >> Preferred alternatives are...? Registering a vmstate for msi? >> >> Jan > > Add msi_load and call that from devices that need it. > Like msix_load does now. > msix_load/save are refactoring candidates IMHO. MSI-X has a real need for storing additional state information, so it should register its own subsection. I don't want to offload this burden to the devices also for MSI. From the devices' POV, why shouldn't msi_init suffice? Jan
On Wed, Apr 27, 2011 at 04:39:53PM +0200, Jan Kiszka wrote: > On 2011-04-27 16:30, Michael S. Tsirkin wrote: > >>>> --- a/hw/pci.c > >>>> +++ b/hw/pci.c > >>>> @@ -34,6 +34,7 @@ > >>>> #include "device-assignment.h" > >>>> #include "qemu-objects.h" > >>>> #include "range.h" > >>>> +#include "msi.h" > >>>> > >>>> //#define DEBUG_PCI > >>>> #ifdef DEBUG_PCI > >>>> @@ -342,6 +343,7 @@ static int get_pci_config_device(QEMUFile *f, void *pv, size_t size) > >>>> memcpy(s->config, config, size); > >>>> > >>>> pci_update_mappings(s); > >>>> + msi_post_load(s); > >>> > >>> Pls don't do this: I'm trying to keep just the core in > >>> pci.c and all capabilities in separate files. > >>> msix has msix_load, msi will just need one too, > >>> and let all devices call that. > >>> > >> > >> Preferred alternatives are...? Registering a vmstate for msi? > >> > >> Jan > > > > Add msi_load and call that from devices that need it. > > Like msix_load does now. > > > > msix_load/save are refactoring candidates IMHO. MSI-X has a real need > for storing additional state information, so it should register its own > subsection. That's an implementation detail though, isn't it. > I don't want to offload this burden to the devices also for > MSI. > From the devices' POV, why shouldn't msi_init suffice? > > Jan One can also claim this about config writes: pci_bridge_write_config(d, address, val, len); pcie_cap_flr_write_config(d, address, val, len); pcie_cap_slot_write_config(d, address, val, len); msi_write_config(d, address, val, len); pcie_aer_write_config(d, address, val, len); which arguably just duplicates the initialization sequence. What I'm trying to do though is to keep it modular and keep module inter-dependencies to a minimum, so that pci is the core and msix depends on it but not the other way around. What I think we should do is to add a pci subdirectory, move all of the stuff there, move pci.c to pci/core.c and add a high level module that depends on them all and deals with all the capabilities. > -- > Siemens AG, Corporate Technology, CT T DE IT 1 > Corporate Competence Center Embedded Linux -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2011-04-27 17:09, Michael S. Tsirkin wrote: > On Wed, Apr 27, 2011 at 04:39:53PM +0200, Jan Kiszka wrote: >> On 2011-04-27 16:30, Michael S. Tsirkin wrote: >>>>>> --- a/hw/pci.c >>>>>> +++ b/hw/pci.c >>>>>> @@ -34,6 +34,7 @@ >>>>>> #include "device-assignment.h" >>>>>> #include "qemu-objects.h" >>>>>> #include "range.h" >>>>>> +#include "msi.h" >>>>>> >>>>>> //#define DEBUG_PCI >>>>>> #ifdef DEBUG_PCI >>>>>> @@ -342,6 +343,7 @@ static int get_pci_config_device(QEMUFile *f, void *pv, size_t size) >>>>>> memcpy(s->config, config, size); >>>>>> >>>>>> pci_update_mappings(s); >>>>>> + msi_post_load(s); >>>>> >>>>> Pls don't do this: I'm trying to keep just the core in >>>>> pci.c and all capabilities in separate files. >>>>> msix has msix_load, msi will just need one too, >>>>> and let all devices call that. >>>>> >>>> >>>> Preferred alternatives are...? Registering a vmstate for msi? >>>> >>>> Jan >>> >>> Add msi_load and call that from devices that need it. >>> Like msix_load does now. >>> >> >> msix_load/save are refactoring candidates IMHO. MSI-X has a real need >> for storing additional state information, so it should register its own >> subsection. > > That's an implementation detail though, isn't it. > >> I don't want to offload this burden to the devices also for >> MSI. >> From the devices' POV, why shouldn't msi_init suffice? >> >> Jan > > One can also claim this about config writes: > pci_bridge_write_config(d, address, val, len); > pcie_cap_flr_write_config(d, address, val, len); > pcie_cap_slot_write_config(d, address, val, len); > msi_write_config(d, address, val, len); > pcie_aer_write_config(d, address, val, len); > which arguably just duplicates the initialization sequence. > > What I'm trying to do though is to keep it modular and > keep module inter-dependencies to a minimum, > so that pci is the core and msix depends on it > but not the other way around. I still don't see the bigger benefit in saving a single bidirectional dependency at core level vs. saving additional callbacks at each and every MSI user. The latter is also a source for bugs. Jan
On Wed, Apr 27, 2011 at 05:21:43PM +0200, Jan Kiszka wrote: > On 2011-04-27 17:09, Michael S. Tsirkin wrote: > > On Wed, Apr 27, 2011 at 04:39:53PM +0200, Jan Kiszka wrote: > >> On 2011-04-27 16:30, Michael S. Tsirkin wrote: > >>>>>> --- a/hw/pci.c > >>>>>> +++ b/hw/pci.c > >>>>>> @@ -34,6 +34,7 @@ > >>>>>> #include "device-assignment.h" > >>>>>> #include "qemu-objects.h" > >>>>>> #include "range.h" > >>>>>> +#include "msi.h" > >>>>>> > >>>>>> //#define DEBUG_PCI > >>>>>> #ifdef DEBUG_PCI > >>>>>> @@ -342,6 +343,7 @@ static int get_pci_config_device(QEMUFile *f, void *pv, size_t size) > >>>>>> memcpy(s->config, config, size); > >>>>>> > >>>>>> pci_update_mappings(s); > >>>>>> + msi_post_load(s); > >>>>> > >>>>> Pls don't do this: I'm trying to keep just the core in > >>>>> pci.c and all capabilities in separate files. > >>>>> msix has msix_load, msi will just need one too, > >>>>> and let all devices call that. > >>>>> > >>>> > >>>> Preferred alternatives are...? Registering a vmstate for msi? > >>>> > >>>> Jan > >>> > >>> Add msi_load and call that from devices that need it. > >>> Like msix_load does now. > >>> > >> > >> msix_load/save are refactoring candidates IMHO. MSI-X has a real need > >> for storing additional state information, so it should register its own > >> subsection. > > > > That's an implementation detail though, isn't it. > > > >> I don't want to offload this burden to the devices also for > >> MSI. > >> From the devices' POV, why shouldn't msi_init suffice? > >> > >> Jan > > > > One can also claim this about config writes: > > pci_bridge_write_config(d, address, val, len); > > pcie_cap_flr_write_config(d, address, val, len); > > pcie_cap_slot_write_config(d, address, val, len); > > msi_write_config(d, address, val, len); > > pcie_aer_write_config(d, address, val, len); > > which arguably just duplicates the initialization sequence. > > > > What I'm trying to do though is to keep it modular and > > keep module inter-dependencies to a minimum, > > so that pci is the core and msix depends on it > > but not the other way around. > > I still don't see the bigger benefit in saving a single bidirectional > dependency at core level vs. saving additional callbacks at each and > every MSI user. The latter is also a source for bugs. > > Jan Yes but let us be consistent with how e.g. config writes are handled. As I said > > What I think we should do is to add a pci subdirectory, move all > > of the stuff there, move pci.c to pci/core.c and > > add a high level module that depends on them all > > and deals with all the capabilities. Which will solve both issues without need for tradeoffs. > -- > Siemens AG, Corporate Technology, CT T DE IT 1 > Corporate Competence Center Embedded Linux -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2011-04-27 18:02, Michael S. Tsirkin wrote: > On Wed, Apr 27, 2011 at 05:21:43PM +0200, Jan Kiszka wrote: >> On 2011-04-27 17:09, Michael S. Tsirkin wrote: >>> On Wed, Apr 27, 2011 at 04:39:53PM +0200, Jan Kiszka wrote: >>>> On 2011-04-27 16:30, Michael S. Tsirkin wrote: >>>>>>>> --- a/hw/pci.c >>>>>>>> +++ b/hw/pci.c >>>>>>>> @@ -34,6 +34,7 @@ >>>>>>>> #include "device-assignment.h" >>>>>>>> #include "qemu-objects.h" >>>>>>>> #include "range.h" >>>>>>>> +#include "msi.h" >>>>>>>> >>>>>>>> //#define DEBUG_PCI >>>>>>>> #ifdef DEBUG_PCI >>>>>>>> @@ -342,6 +343,7 @@ static int get_pci_config_device(QEMUFile *f, void *pv, size_t size) >>>>>>>> memcpy(s->config, config, size); >>>>>>>> >>>>>>>> pci_update_mappings(s); >>>>>>>> + msi_post_load(s); >>>>>>> >>>>>>> Pls don't do this: I'm trying to keep just the core in >>>>>>> pci.c and all capabilities in separate files. >>>>>>> msix has msix_load, msi will just need one too, >>>>>>> and let all devices call that. >>>>>>> >>>>>> >>>>>> Preferred alternatives are...? Registering a vmstate for msi? >>>>>> >>>>>> Jan >>>>> >>>>> Add msi_load and call that from devices that need it. >>>>> Like msix_load does now. >>>>> >>>> >>>> msix_load/save are refactoring candidates IMHO. MSI-X has a real need >>>> for storing additional state information, so it should register its own >>>> subsection. >>> >>> That's an implementation detail though, isn't it. >>> >>>> I don't want to offload this burden to the devices also for >>>> MSI. >>>> From the devices' POV, why shouldn't msi_init suffice? >>>> >>>> Jan >>> >>> One can also claim this about config writes: >>> pci_bridge_write_config(d, address, val, len); >>> pcie_cap_flr_write_config(d, address, val, len); >>> pcie_cap_slot_write_config(d, address, val, len); >>> msi_write_config(d, address, val, len); >>> pcie_aer_write_config(d, address, val, len); >>> which arguably just duplicates the initialization sequence. >>> >>> What I'm trying to do though is to keep it modular and >>> keep module inter-dependencies to a minimum, >>> so that pci is the core and msix depends on it >>> but not the other way around. >> >> I still don't see the bigger benefit in saving a single bidirectional >> dependency at core level vs. saving additional callbacks at each and >> every MSI user. The latter is also a source for bugs. >> >> Jan > > Yes but let us be consistent with how e.g. config writes are > handled. As I said > >>> What I think we should do is to add a pci subdirectory, move all >>> of the stuff there, move pci.c to pci/core.c and >>> add a high level module that depends on them all >>> and deals with all the capabilities. > > Which will solve both issues without need for tradeoffs. OK, but this patch is not about doing a pci refactoring. And it should not touch any device without a need. So what is your suggestion again? Jan
On Wed, Apr 27, 2011 at 06:20:52PM +0200, Jan Kiszka wrote: > On 2011-04-27 18:02, Michael S. Tsirkin wrote: > > On Wed, Apr 27, 2011 at 05:21:43PM +0200, Jan Kiszka wrote: > >> On 2011-04-27 17:09, Michael S. Tsirkin wrote: > >>> On Wed, Apr 27, 2011 at 04:39:53PM +0200, Jan Kiszka wrote: > >>>> On 2011-04-27 16:30, Michael S. Tsirkin wrote: > >>>>>>>> --- a/hw/pci.c > >>>>>>>> +++ b/hw/pci.c > >>>>>>>> @@ -34,6 +34,7 @@ > >>>>>>>> #include "device-assignment.h" > >>>>>>>> #include "qemu-objects.h" > >>>>>>>> #include "range.h" > >>>>>>>> +#include "msi.h" > >>>>>>>> > >>>>>>>> //#define DEBUG_PCI > >>>>>>>> #ifdef DEBUG_PCI > >>>>>>>> @@ -342,6 +343,7 @@ static int get_pci_config_device(QEMUFile *f, void *pv, size_t size) > >>>>>>>> memcpy(s->config, config, size); > >>>>>>>> > >>>>>>>> pci_update_mappings(s); > >>>>>>>> + msi_post_load(s); > >>>>>>> > >>>>>>> Pls don't do this: I'm trying to keep just the core in > >>>>>>> pci.c and all capabilities in separate files. > >>>>>>> msix has msix_load, msi will just need one too, > >>>>>>> and let all devices call that. > >>>>>>> > >>>>>> > >>>>>> Preferred alternatives are...? Registering a vmstate for msi? > >>>>>> > >>>>>> Jan > >>>>> > >>>>> Add msi_load and call that from devices that need it. > >>>>> Like msix_load does now. > >>>>> > >>>> > >>>> msix_load/save are refactoring candidates IMHO. MSI-X has a real need > >>>> for storing additional state information, so it should register its own > >>>> subsection. > >>> > >>> That's an implementation detail though, isn't it. > >>> > >>>> I don't want to offload this burden to the devices also for > >>>> MSI. > >>>> From the devices' POV, why shouldn't msi_init suffice? > >>>> > >>>> Jan > >>> > >>> One can also claim this about config writes: > >>> pci_bridge_write_config(d, address, val, len); > >>> pcie_cap_flr_write_config(d, address, val, len); > >>> pcie_cap_slot_write_config(d, address, val, len); > >>> msi_write_config(d, address, val, len); > >>> pcie_aer_write_config(d, address, val, len); > >>> which arguably just duplicates the initialization sequence. > >>> > >>> What I'm trying to do though is to keep it modular and > >>> keep module inter-dependencies to a minimum, > >>> so that pci is the core and msix depends on it > >>> but not the other way around. > >> > >> I still don't see the bigger benefit in saving a single bidirectional > >> dependency at core level vs. saving additional callbacks at each and > >> every MSI user. The latter is also a source for bugs. > >> > >> Jan > > > > Yes but let us be consistent with how e.g. config writes are > > handled. As I said > > > >>> What I think we should do is to add a pci subdirectory, move all > >>> of the stuff there, move pci.c to pci/core.c and > >>> add a high level module that depends on them all > >>> and deals with all the capabilities. > > > > Which will solve both issues without need for tradeoffs. > > OK, but this patch is not about doing a pci refactoring. And it should > not touch any device without a need. So what is your suggestion again? > > Jan Either add msi_load call in 4 msi users or keep your patch as is. I'll fix it up when I do the refactoring. > -- > Siemens AG, Corporate Technology, CT T DE IT 1 > Corporate Competence Center Embedded Linux -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/hw/msi.c b/hw/msi.c index 18f683b..725b2b7 100644 --- a/hw/msi.c +++ b/hw/msi.c @@ -453,3 +453,16 @@ unsigned int msi_nr_vectors_allocated(const PCIDevice *dev) uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev)); return msi_nr_vectors(flags); } + +void msi_post_load(PCIDevice *dev) +{ + uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev)); + + if (kvm_enabled() && dev->msi_irq_entries) { + kvm_msi_free(dev); + + if (flags & PCI_MSI_FLAGS_ENABLE) { + kvm_msi_update(dev); + } + } +} diff --git a/hw/msi.h b/hw/msi.h index 5766018..6ff0607 100644 --- a/hw/msi.h +++ b/hw/msi.h @@ -32,6 +32,7 @@ void msi_reset(PCIDevice *dev); void msi_notify(PCIDevice *dev, unsigned int vector); void msi_write_config(PCIDevice *dev, uint32_t addr, uint32_t val, int len); unsigned int msi_nr_vectors_allocated(const PCIDevice *dev); +void msi_post_load(PCIDevice *dev); static inline bool msi_present(const PCIDevice *dev) { diff --git a/hw/pci.c b/hw/pci.c index 82e0300..07ec4f9 100644 --- a/hw/pci.c +++ b/hw/pci.c @@ -34,6 +34,7 @@ #include "device-assignment.h" #include "qemu-objects.h" #include "range.h" +#include "msi.h" //#define DEBUG_PCI #ifdef DEBUG_PCI @@ -342,6 +343,7 @@ static int get_pci_config_device(QEMUFile *f, void *pv, size_t size) memcpy(s->config, config, size); pci_update_mappings(s); + msi_post_load(s); qemu_free(config); return 0;