qemu-kvm: msix table update thinko fix
PATCH 1/2 qemu-kvm: msix table update thinko fix
In reply to a message not in the archiveWhen updating msi irq routing entries, we should memcpy the new entry over the old one. Current code gets it wrong, and only works because it's uncommon for guests to change MSI tables.
--- qemu-kvm.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
qemu-kvm.c +1 -1
@@ -1552,7 +1552,7 @@ int kvm_update_routing_entry(kvm_context_t kvm,if (e->u.msi.address_lo == entry->u.msi.address_lo && e->u.msi.address_hi == entry->u.msi.address_hi && e->u.msi.data == entry->u.msi.data) { -memcpy (&e->u.msi, &entry->u.msi,sizeof e->u.msi); +memcpy (&e->u.msi, &newentry->u.msi,sizeof e->u.msi);return 0 ; }break ;
Signature · 1 line
1.6.2.5
PATCH 2/2 qemu-kvm: broken MSI routing work-around
In reply to a message not in the archiveKernels up to 2.6.31 have a bug: MSI entries where looked at when irq is acked, address_lo was interpreted as irqchip, and address_hi as pin. If that matches a real interrupt this prevents ack notifier from being processed.
Since these kernels ignore the value for address_hi when delivering MSI, work around this by setting a value that never matches an interrupt pin number.
Pointers to relevant kernel code, for reference: in kernel v2.6.31-rc3:kvm_notify_acked_irq - fails to check irq type,kvm_set_msi - ignored address_hi in message
--- qemu-kvm.c | 32 ++++++++++++++++++++++++++++++-- 1 files changed, 30 insertions(+), 2 deletions(-)
qemu-kvm.c +30 -2
@@ -38,6 +38,32 @@#error libkvm: userspace and kernel version mismatch#endif +#ifdef KVM_CAP_IRQ_ROUTING +/* Broken MSI routing work-around: +* Kernels up to 2.6.31 have a bug: MSI entries where looked at when irq +* is acked, address_lo was interpreted as irqchip, and +* address_hi as pin. If that matches a real interrupt this prevents +* ack notifier from being processed. +* +* Since these kernels ignore the value for address_hi when delivering +* MSI, work around this by setting a value that never matches an +* interrupt pin number. +*/ +#define KVM_BROKEN_MSI_ROUTING 1 +static inline +void kvm_broken_msi_fix (struct kvm_irq_routing_entry *entry) +{ +if (entry->type ==KVM_IRQ_ROUTING_MSI ) { + entry->u.msi.address_hi =0xffffffff ; + } +} +static inline +unsigned kvm_broken_msi_address_hi (struct kvm_irq_routing_entry *entry) +{ +return 0xffffffff ; +} +#endif +int kvm_allowed =1 ;int kvm_irqchip =1 ;int kvm_pit =1 ;
@@ -1433,6 +1459,7 @@ int kvm_add_routing_entry(kvm_context_t kvm, new->type = entry->type; new->flags = entry->flags; new->u = entry->u; +kvm_broken_msi_fix (new);set_gsi (kvm, entry->gsi);
@@ -1489,7 +1516,7 @@ int kvm_del_routing_entry(kvm_context_t kvm,if (e->u.msi.address_lo == entry->u.msi.address_lo && e->u.msi.address_hi == - entry->u.msi.address_hi +kvm_broken_msi_address_hi (entry) && e->u.msi.data == entry->u.msi.data) { p = &kvm->irq_routes-> entries[--kvm->irq_routes->nr];
@@ -1550,9 +1577,10 @@ int kvm_update_routing_entry(kvm_context_t kvm,break ;case KVM_IRQ_ROUTING_MSI :if (e->u.msi.address_lo == entry->u.msi.address_lo && - e->u.msi.address_hi == entry->u.msi.address_hi && + e->u.msi.address_hi ==kvm_broken_msi_address_hi (entry) && e->u.msi.data == entry->u.msi.data) {memcpy (&e->u.msi, &newentry->u.msi,sizeof e->u.msi); +kvm_broken_msi_fix (e);return 0 ; }break ;
Signature · 1 line
1.6.2.5
Quoted 13 lines · Michael S. Tsirkin
over the old one. Current code gets it wrong, and only works because it's uncommon for guests to change MSI tables.
Signed-off-by Michael S. Tsirkin <[email protected]>--- qemu-kvm.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)qemu-kvm.c +0
-0
qemu-kvm.c int kvm_update_routing_entry(kvm_context_t kvm, · line 1558
Quoted 8 lines
qemu-kvm.c +1
-1@@ -1552,7 +1552,7 @@ int kvm_update_routing_entry(kvm_context_t kvm,if (e->u.msi.address_lo == entry->u.msi.address_lo && e->u.msi.address_hi == entry->u.msi.address_hi && e->u.msi.data == entry->u.msi.data) { -memcpy (&e->u.msi, &entry->u.msi,sizeof e->u.msi); +memcpy (&e->u.msi, &newentry->u.msi,sizeof e->u.msi);return 0 ; }break ;
Non-msi case seems to suffer from the same problem?
Quoted 6 lines · Marcelo Tosatti
Quoted 2 lines · Michael S. Tsirkin
over the old one. Current code gets it wrong, and only works because it's uncommon for guests to change MSI tables.
Non-msi case seems to suffer from the same problem?
Correct of course. Sent out v2 which fixes non-msi as well.
Thanks!
Signature · 1 line
MST
Re: [PATCH 2/2] qemu-kvm: broken MSI routing work-around
In reply to Michael S. TsirkinQuoted 5 lines · Michael S. Tsirkin
Kernels up to 2.6.31 have a bug: MSI entries where looked at when irq is acked,
address_lowas interpreted as irqchip, andaddress_hias pin. If that matches a real interrupt this prevents ack notifier from being processed.
Quoted 7 lines
Since these kernels ignore the value for
address_hiwhen delivering MSI, work around this by setting a value that never matches an interrupt pin number.Pointers to relevant kernel code, for reference: in kernel v2.6.31-rc3:
kvm_notify_acked_irq- fails to check irq type,kvm_set_msi- ignoredaddress_hiin message
Ugh, its still broken AFAICS? If so, what is the plan to fix it?
Also, you'd never want to use address_hi for something, in the future?
Quoted 77 lines
Signed-off-by Michael S. Tsirkin <[email protected]>--- qemu-kvm.c | 32 ++++++++++++++++++++++++++++++-- 1 files changed, 30 insertions(+), 2 deletions(-)qemu-kvm.c +30
-2@@ -38,6 +38,32 @@#error libkvm: userspace and kernel version mismatch#endif +#ifdef KVM_CAP_IRQ_ROUTING +/* Broken MSI routing work-around: +* Kernels up to 2.6.31 have a bug: MSI entries where looked at when irq +* is acked, address_lo was interpreted as irqchip, and +* address_hi as pin. If that matches a real interrupt this prevents +* ack notifier from being processed. +* +* Since these kernels ignore the value for address_hi when delivering +* MSI, work around this by setting a value that never matches an +* interrupt pin number. +*/ +#define KVM_BROKEN_MSI_ROUTING 1 +static inline +void kvm_broken_msi_fix (struct kvm_irq_routing_entry *entry) +{ +if (entry->type ==KVM_IRQ_ROUTING_MSI ) { + entry->u.msi.address_hi =0xffffffff ; + } +} +static inline +unsigned kvm_broken_msi_address_hi (struct kvm_irq_routing_entry *entry) +{ +return 0xffffffff ; +} +#endif +int kvm_allowed =1 ;int kvm_irqchip =1 ;int kvm_pit =1 ;@@ -1433,6 +1459,7 @@ int kvm_add_routing_entry(kvm_context_t kvm, new->type = entry->type; new->flags = entry->flags; new->u = entry->u; +kvm_broken_msi_fix (new);set_gsi (kvm, entry->gsi);@@ -1489,7 +1516,7 @@ int kvm_del_routing_entry(kvm_context_t kvm,if (e->u.msi.address_lo == entry->u.msi.address_lo && e->u.msi.address_hi == - entry->u.msi.address_hi +kvm_broken_msi_address_hi (entry) && e->u.msi.data == entry->u.msi.data) { p = &kvm->irq_routes-> entries[--kvm->irq_routes->nr];@@ -1550,9 +1577,10 @@ int kvm_update_routing_entry(kvm_context_t kvm,break ;case KVM_IRQ_ROUTING_MSI :if (e->u.msi.address_lo == entry->u.msi.address_lo && - e->u.msi.address_hi == entry->u.msi.address_hi && + e->u.msi.address_hi ==kvm_broken_msi_address_hi (entry) && e->u.msi.data == entry->u.msi.data) {memcpy (&e->u.msi, &newentry->u.msi,sizeof e->u.msi); +kvm_broken_msi_fix (e);return 0 ; }break ;Signature · 5 lines
1.6.2.5 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] qemu-kvm: broken MSI routing work-around
Quoted 10 lines · Marcelo Tosatti
Quoted 9 lines · Michael S. Tsirkin
Kernels up to 2.6.31 have a bug: MSI entries where looked at when irq is acked,
address_lowas interpreted as irqchip, andaddress_hias pin. If that matches a real interrupt this prevents ack notifier from being processed.Since these kernels ignore the value for
address_hiwhen delivering MSI, work around this by setting a value that never matches an interrupt pin number.
Quoted 6 lines
Quoted 3 lines
Pointers to relevant kernel code, for reference: in kernel v2.6.31-rc3:
kvm_notify_acked_irq- fails to check irq type,kvm_set_msi- ignoredaddress_hiin messageUgh, its still broken AFAICS? If so, what is the plan to fix it?
I'd like Avi's input on that.
One can imagine multiple ways to fix this in kernel. One possible way proposed by Gleb recently is switching to a new send_msi ioctl. If we do it this way, we can just keep the old interface as broken.
Quoted 2 lines
Also, you'd never want to use
address_hifor something, in the future?
AFAIK apic always has 0 in address_hi. Maybe x2apic is different, we can deactivate the workaround in userspace if we ever enable x2apic.
Quoted 77 lines
Quoted 77 lines
Signed-off-by Michael S. Tsirkin <[email protected]>--- qemu-kvm.c | 32 ++++++++++++++++++++++++++++++-- 1 files changed, 30 insertions(+), 2 deletions(-)qemu-kvm.c +30
-2@@ -38,6 +38,32 @@#error libkvm: userspace and kernel version mismatch#endif +#ifdef KVM_CAP_IRQ_ROUTING +/* Broken MSI routing work-around: +* Kernels up to 2.6.31 have a bug: MSI entries where looked at when irq +* is acked, address_lo was interpreted as irqchip, and +* address_hi as pin. If that matches a real interrupt this prevents +* ack notifier from being processed. +* +* Since these kernels ignore the value for address_hi when delivering +* MSI, work around this by setting a value that never matches an +* interrupt pin number. +*/ +#define KVM_BROKEN_MSI_ROUTING 1 +static inline +void kvm_broken_msi_fix (struct kvm_irq_routing_entry *entry) +{ +if (entry->type ==KVM_IRQ_ROUTING_MSI ) { + entry->u.msi.address_hi =0xffffffff ; + } +} +static inline +unsigned kvm_broken_msi_address_hi (struct kvm_irq_routing_entry *entry) +{ +return 0xffffffff ; +} +#endif +int kvm_allowed =1 ;int kvm_irqchip =1 ;int kvm_pit =1 ;@@ -1433,6 +1459,7 @@ int kvm_add_routing_entry(kvm_context_t kvm, new->type = entry->type; new->flags = entry->flags; new->u = entry->u; +kvm_broken_msi_fix (new);set_gsi (kvm, entry->gsi);@@ -1489,7 +1516,7 @@ int kvm_del_routing_entry(kvm_context_t kvm,if (e->u.msi.address_lo == entry->u.msi.address_lo && e->u.msi.address_hi == - entry->u.msi.address_hi +kvm_broken_msi_address_hi (entry) && e->u.msi.data == entry->u.msi.data) { p = &kvm->irq_routes-> entries[--kvm->irq_routes->nr];@@ -1550,9 +1577,10 @@ int kvm_update_routing_entry(kvm_context_t kvm,break ;case KVM_IRQ_ROUTING_MSI :if (e->u.msi.address_lo == entry->u.msi.address_lo && - e->u.msi.address_hi == entry->u.msi.address_hi && + e->u.msi.address_hi ==kvm_broken_msi_address_hi (entry) && e->u.msi.data == entry->u.msi.data) {memcpy (&e->u.msi, &newentry->u.msi,sizeof e->u.msi); +kvm_broken_msi_fix (e);return 0 ; }break ;Signature · 5 lines
1.6.2.5 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] qemu-kvm: broken MSI routing work-around
Quoted 23 lines · Michael S. Tsirkin
Quoted 10 lines · Marcelo Tosatti
Quoted 9 lines · Michael S. Tsirkin
Kernels up to 2.6.31 have a bug: MSI entries where looked at when irq is acked,
address_lowas interpreted as irqchip, andaddress_hias pin. If that matches a real interrupt this prevents ack notifier from being processed.Since these kernels ignore the value for
address_hiwhen delivering MSI, work around this by setting a value that never matches an interrupt pin number.Quoted 6 lines
Quoted 3 lines
Pointers to relevant kernel code, for reference: in kernel v2.6.31-rc3:
kvm_notify_acked_irq- fails to check irq type,kvm_set_msi- ignoredaddress_hiin messageUgh, its still broken AFAICS? If so, what is the plan to fix it?
I'd like Avi's input on that.
One can imagine multiple ways to fix this in kernel. One possible way proposed by Gleb recently is switching to a newsend_msiioctl. If we do it this way, we can just keep the old interface as broken.
Quoted 6 lines
Quoted 2 lines
Also, you'd never want to use
address_hifor something, in the future?AFAIK apic always has 0 in
address_hi. Maybe x2apic is different, we can deactivate the workaround in userspace if we ever enable x2apic.
x2apic doesn't use address_hi either without interrupt remapping.
Quoted 69 lines
Quoted 69 lines
Quoted 69 lines
Signed-off-by Michael S. Tsirkin <[email protected]>--- qemu-kvm.c | 32 ++++++++++++++++++++++++++++++-- 1 files changed, 30 insertions(+), 2 deletions(-)qemu-kvm.c +30
-2@@ -38,6 +38,32 @@#error libkvm: userspace and kernel version mismatch#endif +#ifdef KVM_CAP_IRQ_ROUTING +/* Broken MSI routing work-around: +* Kernels up to 2.6.31 have a bug: MSI entries where looked at when irq +* is acked, address_lo was interpreted as irqchip, and +* address_hi as pin. If that matches a real interrupt this prevents +* ack notifier from being processed. +* +* Since these kernels ignore the value for address_hi when delivering +* MSI, work around this by setting a value that never matches an +* interrupt pin number. +*/ +#define KVM_BROKEN_MSI_ROUTING 1 +static inline +void kvm_broken_msi_fix (struct kvm_irq_routing_entry *entry) +{ +if (entry->type ==KVM_IRQ_ROUTING_MSI ) { + entry->u.msi.address_hi =0xffffffff ; + } +} +static inline +unsigned kvm_broken_msi_address_hi (struct kvm_irq_routing_entry *entry) +{ +return 0xffffffff ; +} +#endif +int kvm_allowed =1 ;int kvm_irqchip =1 ;int kvm_pit =1 ;@@ -1433,6 +1459,7 @@ int kvm_add_routing_entry(kvm_context_t kvm, new->type = entry->type; new->flags = entry->flags; new->u = entry->u; +kvm_broken_msi_fix (new);set_gsi (kvm, entry->gsi);@@ -1489,7 +1516,7 @@ int kvm_del_routing_entry(kvm_context_t kvm,if (e->u.msi.address_lo == entry->u.msi.address_lo && e->u.msi.address_hi == - entry->u.msi.address_hi +kvm_broken_msi_address_hi (entry) && e->u.msi.data == entry->u.msi.data) { p = &kvm->irq_routes-> entries[--kvm->irq_routes->nr];@@ -1550,9 +1577,10 @@ int kvm_update_routing_entry(kvm_context_t kvm,break ;case KVM_IRQ_ROUTING_MSI :if (e->u.msi.address_lo == entry->u.msi.address_lo && - e->u.msi.address_hi == entry->u.msi.address_hi && + e->u.msi.address_hi ==kvm_broken_msi_address_hi (entry) && e->u.msi.data == entry->u.msi.data) {memcpy (&e->u.msi, &newentry->u.msi,sizeof e->u.msi); +kvm_broken_msi_fix (e);return 0 ;
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
qemu-kvm.c +0
-0}break ;Signature · 5 lines
1.6.2.5 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Gleb.