* [Qemu-devel] [PATCH v2 0/3] kvm: irqchip: skip msi update when msi disabled @ 2017-05-09 6:00 Peter Xu 2017-05-09 6:00 ` [Qemu-devel] [PATCH v2 1/3] kvm: irqchip: trace changes on msi add/remove Peter Xu ` (3 more replies) 0 siblings, 4 replies; 12+ messages in thread From: Peter Xu @ 2017-05-09 6:00 UTC (permalink / raw) To: qemu-devel; +Cc: Michael S . Tsirkin, Paolo Bonzini, peterx v2: - missed one line in last patch to include msix header First two patches are new traces, please collect them if needed. The third patch is a fix to a qemu crash reported here: https://bugzilla.redhat.com/show_bug.cgi?id=1448813 Please see its commit message for more information. Please review. Thanks. Peter Xu (3): kvm: irqchip: trace changes on msi add/remove msix: trace control bit write op kvm: irqchip: skip update msi when disabled hw/pci/msix.c | 11 +++++++++-- hw/pci/trace-events | 3 +++ kvm-all.c | 4 +++- target/i386/kvm.c | 13 +++++++++---- trace-events | 3 ++- 5 files changed, 26 insertions(+), 8 deletions(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [Qemu-devel] [PATCH v2 1/3] kvm: irqchip: trace changes on msi add/remove 2017-05-09 6:00 [Qemu-devel] [PATCH v2 0/3] kvm: irqchip: skip msi update when msi disabled Peter Xu @ 2017-05-09 6:00 ` Peter Xu 2017-05-10 10:45 ` Philippe Mathieu-Daudé 2017-05-09 6:00 ` [Qemu-devel] [PATCH v2 2/3] msix: trace control bit write op Peter Xu ` (2 subsequent siblings) 3 siblings, 1 reply; 12+ messages in thread From: Peter Xu @ 2017-05-09 6:00 UTC (permalink / raw) To: qemu-devel; +Cc: Michael S . Tsirkin, Paolo Bonzini, peterx It'll be nice to know which virq belongs to which device/vector when adding msi routes, so adding two more parameters for the add trace. Meanwhile, releasing virq has no tracing before. Add one for it. Signed-off-by: Peter Xu <peterx@redhat.com> --- kvm-all.c | 4 +++- trace-events | 3 ++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/kvm-all.c b/kvm-all.c index 90b8573..2598b1f 100644 --- a/kvm-all.c +++ b/kvm-all.c @@ -1144,6 +1144,7 @@ void kvm_irqchip_release_virq(KVMState *s, int virq) } clear_gsi(s, virq); kvm_arch_release_virq_post(virq); + trace_kvm_irqchip_release_virq(virq); } static unsigned int kvm_hash_msi(uint32_t data) @@ -1287,7 +1288,8 @@ int kvm_irqchip_add_msi_route(KVMState *s, int vector, PCIDevice *dev) return -EINVAL; } - trace_kvm_irqchip_add_msi_route(virq); + trace_kvm_irqchip_add_msi_route(dev ? dev->name : (char *)"N/A", + vector, virq); kvm_add_routing_entry(s, &kroute); kvm_arch_add_msi_route_post(&kroute, vector, dev); diff --git a/trace-events b/trace-events index e582d63..f01ec05 100644 --- a/trace-events +++ b/trace-events @@ -69,8 +69,9 @@ kvm_device_ioctl(int fd, int type, void *arg) "dev fd %d, type 0x%x, arg %p" kvm_failed_reg_get(uint64_t id, const char *msg) "Warning: Unable to retrieve ONEREG %" PRIu64 " from KVM: %s" kvm_failed_reg_set(uint64_t id, const char *msg) "Warning: Unable to set ONEREG %" PRIu64 " to KVM: %s" kvm_irqchip_commit_routes(void) "" -kvm_irqchip_add_msi_route(int virq) "Adding MSI route virq=%d" +kvm_irqchip_add_msi_route(char *name, int vector, int virq) "dev %s vector %d virq %d" kvm_irqchip_update_msi_route(int virq) "Updating MSI route virq=%d" +kvm_irqchip_release_virq(int virq) "virq %d" # TCG related tracing (mostly disabled by default) # cpu-exec.c -- 2.7.4 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/3] kvm: irqchip: trace changes on msi add/remove 2017-05-09 6:00 ` [Qemu-devel] [PATCH v2 1/3] kvm: irqchip: trace changes on msi add/remove Peter Xu @ 2017-05-10 10:45 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 12+ messages in thread From: Philippe Mathieu-Daudé @ 2017-05-10 10:45 UTC (permalink / raw) To: Peter Xu, qemu-devel; +Cc: Paolo Bonzini, Michael S . Tsirkin On 05/09/2017 03:00 AM, Peter Xu wrote: > It'll be nice to know which virq belongs to which device/vector when > adding msi routes, so adding two more parameters for the add trace. > > Meanwhile, releasing virq has no tracing before. Add one for it. > > Signed-off-by: Peter Xu <peterx@redhat.com> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > kvm-all.c | 4 +++- > trace-events | 3 ++- > 2 files changed, 5 insertions(+), 2 deletions(-) > > diff --git a/kvm-all.c b/kvm-all.c > index 90b8573..2598b1f 100644 > --- a/kvm-all.c > +++ b/kvm-all.c > @@ -1144,6 +1144,7 @@ void kvm_irqchip_release_virq(KVMState *s, int virq) > } > clear_gsi(s, virq); > kvm_arch_release_virq_post(virq); > + trace_kvm_irqchip_release_virq(virq); > } > > static unsigned int kvm_hash_msi(uint32_t data) > @@ -1287,7 +1288,8 @@ int kvm_irqchip_add_msi_route(KVMState *s, int vector, PCIDevice *dev) > return -EINVAL; > } > > - trace_kvm_irqchip_add_msi_route(virq); > + trace_kvm_irqchip_add_msi_route(dev ? dev->name : (char *)"N/A", > + vector, virq); > > kvm_add_routing_entry(s, &kroute); > kvm_arch_add_msi_route_post(&kroute, vector, dev); > diff --git a/trace-events b/trace-events > index e582d63..f01ec05 100644 > --- a/trace-events > +++ b/trace-events > @@ -69,8 +69,9 @@ kvm_device_ioctl(int fd, int type, void *arg) "dev fd %d, type 0x%x, arg %p" > kvm_failed_reg_get(uint64_t id, const char *msg) "Warning: Unable to retrieve ONEREG %" PRIu64 " from KVM: %s" > kvm_failed_reg_set(uint64_t id, const char *msg) "Warning: Unable to set ONEREG %" PRIu64 " to KVM: %s" > kvm_irqchip_commit_routes(void) "" > -kvm_irqchip_add_msi_route(int virq) "Adding MSI route virq=%d" > +kvm_irqchip_add_msi_route(char *name, int vector, int virq) "dev %s vector %d virq %d" > kvm_irqchip_update_msi_route(int virq) "Updating MSI route virq=%d" > +kvm_irqchip_release_virq(int virq) "virq %d" > > # TCG related tracing (mostly disabled by default) > # cpu-exec.c > ^ permalink raw reply [flat|nested] 12+ messages in thread
* [Qemu-devel] [PATCH v2 2/3] msix: trace control bit write op 2017-05-09 6:00 [Qemu-devel] [PATCH v2 0/3] kvm: irqchip: skip msi update when msi disabled Peter Xu 2017-05-09 6:00 ` [Qemu-devel] [PATCH v2 1/3] kvm: irqchip: trace changes on msi add/remove Peter Xu @ 2017-05-09 6:00 ` Peter Xu 2017-05-10 10:44 ` Philippe Mathieu-Daudé ` (2 more replies) 2017-05-09 6:00 ` [Qemu-devel] [PATCH v2 3/3] kvm: irqchip: skip update msi when disabled Peter Xu 2017-05-10 15:32 ` [Qemu-devel] [PATCH v2 0/3] kvm: irqchip: skip msi update when msi disabled Paolo Bonzini 3 siblings, 3 replies; 12+ messages in thread From: Peter Xu @ 2017-05-09 6:00 UTC (permalink / raw) To: qemu-devel; +Cc: Michael S . Tsirkin, Paolo Bonzini, peterx Meanwhile, abstract a function to detect msix masked bit. Signed-off-by: Peter Xu <peterx@redhat.com> --- hw/pci/msix.c | 11 +++++++++-- hw/pci/trace-events | 3 +++ 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/hw/pci/msix.c b/hw/pci/msix.c index bb54e8b..fc5fe51 100644 --- a/hw/pci/msix.c +++ b/hw/pci/msix.c @@ -22,6 +22,7 @@ #include "hw/xen/xen.h" #include "qemu/range.h" #include "qapi/error.h" +#include "trace.h" #define MSIX_CAP_LENGTH 12 @@ -130,10 +131,14 @@ static void msix_handle_mask_update(PCIDevice *dev, int vector, bool was_masked) } } +static bool msix_masked(PCIDevice *dev) +{ + return dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] & MSIX_MASKALL_MASK; +} + static void msix_update_function_masked(PCIDevice *dev) { - dev->msix_function_masked = !msix_enabled(dev) || - (dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] & MSIX_MASKALL_MASK); + dev->msix_function_masked = !msix_enabled(dev) || msix_masked(dev); } /* Handle MSI-X capability config write. */ @@ -148,6 +153,8 @@ void msix_write_config(PCIDevice *dev, uint32_t addr, return; } + trace_msix_write_config(dev->name, msix_enabled(dev), msix_masked(dev)); + was_masked = dev->msix_function_masked; msix_update_function_masked(dev); diff --git a/hw/pci/trace-events b/hw/pci/trace-events index 2b9cf24..83c8f5a 100644 --- a/hw/pci/trace-events +++ b/hw/pci/trace-events @@ -7,3 +7,6 @@ pci_update_mappings_add(void *d, uint32_t bus, uint32_t slot, uint32_t func, int # hw/pci/pci_host.c pci_cfg_read(const char *dev, unsigned devid, unsigned fnid, unsigned offs, unsigned val) "%s %02u:%u @0x%x -> 0x%x" pci_cfg_write(const char *dev, unsigned devid, unsigned fnid, unsigned offs, unsigned val) "%s %02u:%u @0x%x <- 0x%x" + +# hw/pci/msix.c +msix_write_config(char *name, bool enabled, bool masked) "dev %s enabled %d masked %d" -- 2.7.4 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/3] msix: trace control bit write op 2017-05-09 6:00 ` [Qemu-devel] [PATCH v2 2/3] msix: trace control bit write op Peter Xu @ 2017-05-10 10:44 ` Philippe Mathieu-Daudé 2017-05-10 18:30 ` Michael S. Tsirkin 2017-05-10 18:30 ` Michael S. Tsirkin 2 siblings, 0 replies; 12+ messages in thread From: Philippe Mathieu-Daudé @ 2017-05-10 10:44 UTC (permalink / raw) To: Peter Xu, qemu-devel; +Cc: Paolo Bonzini, Michael S . Tsirkin On 05/09/2017 03:00 AM, Peter Xu wrote: > Meanwhile, abstract a function to detect msix masked bit. > > Signed-off-by: Peter Xu <peterx@redhat.com> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > hw/pci/msix.c | 11 +++++++++-- > hw/pci/trace-events | 3 +++ > 2 files changed, 12 insertions(+), 2 deletions(-) > > diff --git a/hw/pci/msix.c b/hw/pci/msix.c > index bb54e8b..fc5fe51 100644 > --- a/hw/pci/msix.c > +++ b/hw/pci/msix.c > @@ -22,6 +22,7 @@ > #include "hw/xen/xen.h" > #include "qemu/range.h" > #include "qapi/error.h" > +#include "trace.h" > > #define MSIX_CAP_LENGTH 12 > > @@ -130,10 +131,14 @@ static void msix_handle_mask_update(PCIDevice *dev, int vector, bool was_masked) > } > } > > +static bool msix_masked(PCIDevice *dev) > +{ > + return dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] & MSIX_MASKALL_MASK; > +} > + > static void msix_update_function_masked(PCIDevice *dev) > { > - dev->msix_function_masked = !msix_enabled(dev) || > - (dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] & MSIX_MASKALL_MASK); > + dev->msix_function_masked = !msix_enabled(dev) || msix_masked(dev); > } > > /* Handle MSI-X capability config write. */ > @@ -148,6 +153,8 @@ void msix_write_config(PCIDevice *dev, uint32_t addr, > return; > } > > + trace_msix_write_config(dev->name, msix_enabled(dev), msix_masked(dev)); > + > was_masked = dev->msix_function_masked; > msix_update_function_masked(dev); > > diff --git a/hw/pci/trace-events b/hw/pci/trace-events > index 2b9cf24..83c8f5a 100644 > --- a/hw/pci/trace-events > +++ b/hw/pci/trace-events > @@ -7,3 +7,6 @@ pci_update_mappings_add(void *d, uint32_t bus, uint32_t slot, uint32_t func, int > # hw/pci/pci_host.c > pci_cfg_read(const char *dev, unsigned devid, unsigned fnid, unsigned offs, unsigned val) "%s %02u:%u @0x%x -> 0x%x" > pci_cfg_write(const char *dev, unsigned devid, unsigned fnid, unsigned offs, unsigned val) "%s %02u:%u @0x%x <- 0x%x" > + > +# hw/pci/msix.c > +msix_write_config(char *name, bool enabled, bool masked) "dev %s enabled %d masked %d" > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/3] msix: trace control bit write op 2017-05-09 6:00 ` [Qemu-devel] [PATCH v2 2/3] msix: trace control bit write op Peter Xu 2017-05-10 10:44 ` Philippe Mathieu-Daudé @ 2017-05-10 18:30 ` Michael S. Tsirkin 2017-05-10 18:30 ` Michael S. Tsirkin 2 siblings, 0 replies; 12+ messages in thread From: Michael S. Tsirkin @ 2017-05-10 18:30 UTC (permalink / raw) To: Peter Xu; +Cc: qemu-devel, Paolo Bonzini On Tue, May 09, 2017 at 02:00:43PM +0800, Peter Xu wrote: > Meanwhile, abstract a function to detect msix masked bit. > > Signed-off-by: Peter Xu <peterx@redhat.com> Acked-by: Michael S. Tsirkin <mst@redhat.com> > --- > hw/pci/msix.c | 11 +++++++++-- > hw/pci/trace-events | 3 +++ > 2 files changed, 12 insertions(+), 2 deletions(-) > > diff --git a/hw/pci/msix.c b/hw/pci/msix.c > index bb54e8b..fc5fe51 100644 > --- a/hw/pci/msix.c > +++ b/hw/pci/msix.c > @@ -22,6 +22,7 @@ > #include "hw/xen/xen.h" > #include "qemu/range.h" > #include "qapi/error.h" > +#include "trace.h" > > #define MSIX_CAP_LENGTH 12 > > @@ -130,10 +131,14 @@ static void msix_handle_mask_update(PCIDevice *dev, int vector, bool was_masked) > } > } > > +static bool msix_masked(PCIDevice *dev) > +{ > + return dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] & MSIX_MASKALL_MASK; > +} > + > static void msix_update_function_masked(PCIDevice *dev) > { > - dev->msix_function_masked = !msix_enabled(dev) || > - (dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] & MSIX_MASKALL_MASK); > + dev->msix_function_masked = !msix_enabled(dev) || msix_masked(dev); > } > > /* Handle MSI-X capability config write. */ > @@ -148,6 +153,8 @@ void msix_write_config(PCIDevice *dev, uint32_t addr, > return; > } > > + trace_msix_write_config(dev->name, msix_enabled(dev), msix_masked(dev)); > + > was_masked = dev->msix_function_masked; > msix_update_function_masked(dev); > > diff --git a/hw/pci/trace-events b/hw/pci/trace-events > index 2b9cf24..83c8f5a 100644 > --- a/hw/pci/trace-events > +++ b/hw/pci/trace-events > @@ -7,3 +7,6 @@ pci_update_mappings_add(void *d, uint32_t bus, uint32_t slot, uint32_t func, int > # hw/pci/pci_host.c > pci_cfg_read(const char *dev, unsigned devid, unsigned fnid, unsigned offs, unsigned val) "%s %02u:%u @0x%x -> 0x%x" > pci_cfg_write(const char *dev, unsigned devid, unsigned fnid, unsigned offs, unsigned val) "%s %02u:%u @0x%x <- 0x%x" > + > +# hw/pci/msix.c > +msix_write_config(char *name, bool enabled, bool masked) "dev %s enabled %d masked %d" > -- > 2.7.4 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/3] msix: trace control bit write op 2017-05-09 6:00 ` [Qemu-devel] [PATCH v2 2/3] msix: trace control bit write op Peter Xu 2017-05-10 10:44 ` Philippe Mathieu-Daudé 2017-05-10 18:30 ` Michael S. Tsirkin @ 2017-05-10 18:30 ` Michael S. Tsirkin 2 siblings, 0 replies; 12+ messages in thread From: Michael S. Tsirkin @ 2017-05-10 18:30 UTC (permalink / raw) To: Peter Xu; +Cc: qemu-devel, Paolo Bonzini On Tue, May 09, 2017 at 02:00:43PM +0800, Peter Xu wrote: > Meanwhile, abstract a function to detect msix masked bit. > > Signed-off-by: Peter Xu <peterx@redhat.com> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > --- > hw/pci/msix.c | 11 +++++++++-- > hw/pci/trace-events | 3 +++ > 2 files changed, 12 insertions(+), 2 deletions(-) > > diff --git a/hw/pci/msix.c b/hw/pci/msix.c > index bb54e8b..fc5fe51 100644 > --- a/hw/pci/msix.c > +++ b/hw/pci/msix.c > @@ -22,6 +22,7 @@ > #include "hw/xen/xen.h" > #include "qemu/range.h" > #include "qapi/error.h" > +#include "trace.h" > > #define MSIX_CAP_LENGTH 12 > > @@ -130,10 +131,14 @@ static void msix_handle_mask_update(PCIDevice *dev, int vector, bool was_masked) > } > } > > +static bool msix_masked(PCIDevice *dev) > +{ > + return dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] & MSIX_MASKALL_MASK; > +} > + > static void msix_update_function_masked(PCIDevice *dev) > { > - dev->msix_function_masked = !msix_enabled(dev) || > - (dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] & MSIX_MASKALL_MASK); > + dev->msix_function_masked = !msix_enabled(dev) || msix_masked(dev); > } > > /* Handle MSI-X capability config write. */ > @@ -148,6 +153,8 @@ void msix_write_config(PCIDevice *dev, uint32_t addr, > return; > } > > + trace_msix_write_config(dev->name, msix_enabled(dev), msix_masked(dev)); > + > was_masked = dev->msix_function_masked; > msix_update_function_masked(dev); > > diff --git a/hw/pci/trace-events b/hw/pci/trace-events > index 2b9cf24..83c8f5a 100644 > --- a/hw/pci/trace-events > +++ b/hw/pci/trace-events > @@ -7,3 +7,6 @@ pci_update_mappings_add(void *d, uint32_t bus, uint32_t slot, uint32_t func, int > # hw/pci/pci_host.c > pci_cfg_read(const char *dev, unsigned devid, unsigned fnid, unsigned offs, unsigned val) "%s %02u:%u @0x%x -> 0x%x" > pci_cfg_write(const char *dev, unsigned devid, unsigned fnid, unsigned offs, unsigned val) "%s %02u:%u @0x%x <- 0x%x" > + > +# hw/pci/msix.c > +msix_write_config(char *name, bool enabled, bool masked) "dev %s enabled %d masked %d" > -- > 2.7.4 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [Qemu-devel] [PATCH v2 3/3] kvm: irqchip: skip update msi when disabled 2017-05-09 6:00 [Qemu-devel] [PATCH v2 0/3] kvm: irqchip: skip msi update when msi disabled Peter Xu 2017-05-09 6:00 ` [Qemu-devel] [PATCH v2 1/3] kvm: irqchip: trace changes on msi add/remove Peter Xu 2017-05-09 6:00 ` [Qemu-devel] [PATCH v2 2/3] msix: trace control bit write op Peter Xu @ 2017-05-09 6:00 ` Peter Xu 2017-05-11 2:56 ` Peter Xu 2017-05-10 15:32 ` [Qemu-devel] [PATCH v2 0/3] kvm: irqchip: skip msi update when msi disabled Paolo Bonzini 3 siblings, 1 reply; 12+ messages in thread From: Peter Xu @ 2017-05-09 6:00 UTC (permalink / raw) To: qemu-devel; +Cc: Michael S . Tsirkin, Paolo Bonzini, peterx It's possible that one device kept its irqfd/virq there even when MSI/MSIX was disabled globally for that device. One example is virtio-net-pci (see commit f1d0f15a6 and virtio_pci_vq_vector_mask()). It is used as a fast path to avoid allocate/release irqfd/virq frequently when guest enables/disables MSIX. However, this fast path brought a problem to msi_route_list, that the device MSIRouteEntry is still dangling there even if MSIX disabled - then we cannot know which message to fetch, even if we can, the messages are meaningless. In this case, we can just simply ignore this entry. It's safe, since when MSIX is enabled again, we'll rebuild them no matter what. Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1448813 Signed-off-by: Peter Xu <peterx@redhat.com> --- target/i386/kvm.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/target/i386/kvm.c b/target/i386/kvm.c index 55865db..554950d 100644 --- a/target/i386/kvm.c +++ b/target/i386/kvm.c @@ -43,6 +43,7 @@ #include "standard-headers/asm-x86/hyperv.h" #include "hw/pci/pci.h" #include "hw/pci/msi.h" +#include "hw/pci/msix.h" #include "migration/migration.h" #include "exec/memattrs.h" #include "trace.h" @@ -3510,12 +3511,16 @@ static void kvm_update_msi_routes_all(void *private, bool global, int cnt = 0; MSIRouteEntry *entry; MSIMessage msg; + PCIDevice *dev; + /* TODO: explicit route update */ QLIST_FOREACH(entry, &msi_route_list, list) { - cnt++; - msg = pci_get_msi_message(entry->dev, entry->vector); - kvm_irqchip_update_msi_route(kvm_state, entry->virq, - msg, entry->dev); + dev = entry->dev; + if (!msix_enabled(dev) && !msi_enabled(dev)) { + continue; + } + msg = pci_get_msi_message(dev, entry->vector); + kvm_irqchip_update_msi_route(kvm_state, entry->virq, msg, dev); } kvm_irqchip_commit_routes(kvm_state); trace_kvm_x86_update_msi_routes(cnt); -- 2.7.4 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/3] kvm: irqchip: skip update msi when disabled 2017-05-09 6:00 ` [Qemu-devel] [PATCH v2 3/3] kvm: irqchip: skip update msi when disabled Peter Xu @ 2017-05-11 2:56 ` Peter Xu 2017-05-11 7:48 ` Paolo Bonzini 0 siblings, 1 reply; 12+ messages in thread From: Peter Xu @ 2017-05-11 2:56 UTC (permalink / raw) To: qemu-devel, Paolo Bonzini; +Cc: Michael S . Tsirkin On Tue, May 09, 2017 at 02:00:44PM +0800, Peter Xu wrote: > It's possible that one device kept its irqfd/virq there even when > MSI/MSIX was disabled globally for that device. One example is > virtio-net-pci (see commit f1d0f15a6 and virtio_pci_vq_vector_mask()). > It is used as a fast path to avoid allocate/release irqfd/virq > frequently when guest enables/disables MSIX. > > However, this fast path brought a problem to msi_route_list, that the > device MSIRouteEntry is still dangling there even if MSIX disabled - > then we cannot know which message to fetch, even if we can, the messages > are meaningless. In this case, we can just simply ignore this entry. > > It's safe, since when MSIX is enabled again, we'll rebuild them no > matter what. > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1448813 > > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > target/i386/kvm.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/target/i386/kvm.c b/target/i386/kvm.c > index 55865db..554950d 100644 > --- a/target/i386/kvm.c > +++ b/target/i386/kvm.c > @@ -43,6 +43,7 @@ > #include "standard-headers/asm-x86/hyperv.h" > #include "hw/pci/pci.h" > #include "hw/pci/msi.h" > +#include "hw/pci/msix.h" > #include "migration/migration.h" > #include "exec/memattrs.h" > #include "trace.h" > @@ -3510,12 +3511,16 @@ static void kvm_update_msi_routes_all(void *private, bool global, > int cnt = 0; > MSIRouteEntry *entry; > MSIMessage msg; > + PCIDevice *dev; > + > /* TODO: explicit route update */ > QLIST_FOREACH(entry, &msi_route_list, list) { > - cnt++; Oops, I think this line should be kept. It does not affect too much, only the trace below. But still, no reason to remove it. Paolo, would it still possible to touch it up in your queue? Sorry for the inconvenience! > - msg = pci_get_msi_message(entry->dev, entry->vector); > - kvm_irqchip_update_msi_route(kvm_state, entry->virq, > - msg, entry->dev); > + dev = entry->dev; > + if (!msix_enabled(dev) && !msi_enabled(dev)) { > + continue; > + } > + msg = pci_get_msi_message(dev, entry->vector); > + kvm_irqchip_update_msi_route(kvm_state, entry->virq, msg, dev); > } > kvm_irqchip_commit_routes(kvm_state); > trace_kvm_x86_update_msi_routes(cnt); > -- > 2.7.4 > -- Peter Xu ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/3] kvm: irqchip: skip update msi when disabled 2017-05-11 2:56 ` Peter Xu @ 2017-05-11 7:48 ` Paolo Bonzini 2017-05-11 7:54 ` Peter Xu 0 siblings, 1 reply; 12+ messages in thread From: Paolo Bonzini @ 2017-05-11 7:48 UTC (permalink / raw) To: Peter Xu, qemu-devel; +Cc: Michael S . Tsirkin On 11/05/2017 04:56, Peter Xu wrote: >> @@ -3510,12 +3511,16 @@ static void kvm_update_msi_routes_all(void *private, bool global, >> int cnt = 0; >> MSIRouteEntry *entry; >> MSIMessage msg; >> + PCIDevice *dev; >> + >> /* TODO: explicit route update */ >> QLIST_FOREACH(entry, &msi_route_list, list) { >> - cnt++; > > Oops, I think this line should be kept. It does not affect too much, > only the trace below. But still, no reason to remove it. > > Paolo, would it still possible to touch it up in your queue? Yup, fixed. Paolo ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/3] kvm: irqchip: skip update msi when disabled 2017-05-11 7:48 ` Paolo Bonzini @ 2017-05-11 7:54 ` Peter Xu 0 siblings, 0 replies; 12+ messages in thread From: Peter Xu @ 2017-05-11 7:54 UTC (permalink / raw) To: Paolo Bonzini; +Cc: qemu-devel, Michael S . Tsirkin On Thu, May 11, 2017 at 09:48:52AM +0200, Paolo Bonzini wrote: > > > On 11/05/2017 04:56, Peter Xu wrote: > >> @@ -3510,12 +3511,16 @@ static void kvm_update_msi_routes_all(void *private, bool global, > >> int cnt = 0; > >> MSIRouteEntry *entry; > >> MSIMessage msg; > >> + PCIDevice *dev; > >> + > >> /* TODO: explicit route update */ > >> QLIST_FOREACH(entry, &msi_route_list, list) { > >> - cnt++; > > > > Oops, I think this line should be kept. It does not affect too much, > > only the trace below. But still, no reason to remove it. > > > > Paolo, would it still possible to touch it up in your queue? > > Yup, fixed. That's great. Thanks! -- Peter Xu ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/3] kvm: irqchip: skip msi update when msi disabled 2017-05-09 6:00 [Qemu-devel] [PATCH v2 0/3] kvm: irqchip: skip msi update when msi disabled Peter Xu ` (2 preceding siblings ...) 2017-05-09 6:00 ` [Qemu-devel] [PATCH v2 3/3] kvm: irqchip: skip update msi when disabled Peter Xu @ 2017-05-10 15:32 ` Paolo Bonzini 3 siblings, 0 replies; 12+ messages in thread From: Paolo Bonzini @ 2017-05-10 15:32 UTC (permalink / raw) To: Peter Xu, qemu-devel; +Cc: Michael S . Tsirkin On 09/05/2017 08:00, Peter Xu wrote: > v2: > - missed one line in last patch to include msix header > > First two patches are new traces, please collect them if needed. > > The third patch is a fix to a qemu crash reported here: > > https://bugzilla.redhat.com/show_bug.cgi?id=1448813 > > Please see its commit message for more information. > > Please review. Thanks. > > Peter Xu (3): > kvm: irqchip: trace changes on msi add/remove > msix: trace control bit write op > kvm: irqchip: skip update msi when disabled > > hw/pci/msix.c | 11 +++++++++-- > hw/pci/trace-events | 3 +++ > kvm-all.c | 4 +++- > target/i386/kvm.c | 13 +++++++++---- > trace-events | 3 ++- > 5 files changed, 26 insertions(+), 8 deletions(-) > Looks good, queued for 2.10. Michael, would you like to ack patch 2? Thanks, Paolo ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2017-05-11 7:54 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-05-09 6:00 [Qemu-devel] [PATCH v2 0/3] kvm: irqchip: skip msi update when msi disabled Peter Xu 2017-05-09 6:00 ` [Qemu-devel] [PATCH v2 1/3] kvm: irqchip: trace changes on msi add/remove Peter Xu 2017-05-10 10:45 ` Philippe Mathieu-Daudé 2017-05-09 6:00 ` [Qemu-devel] [PATCH v2 2/3] msix: trace control bit write op Peter Xu 2017-05-10 10:44 ` Philippe Mathieu-Daudé 2017-05-10 18:30 ` Michael S. Tsirkin 2017-05-10 18:30 ` Michael S. Tsirkin 2017-05-09 6:00 ` [Qemu-devel] [PATCH v2 3/3] kvm: irqchip: skip update msi when disabled Peter Xu 2017-05-11 2:56 ` Peter Xu 2017-05-11 7:48 ` Paolo Bonzini 2017-05-11 7:54 ` Peter Xu 2017-05-10 15:32 ` [Qemu-devel] [PATCH v2 0/3] kvm: irqchip: skip msi update when msi disabled Paolo Bonzini
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).