Device assignment code clean up and MSI disable support
cover Device assignment code clean up and MSI disable support
I splitted the former patchset into 3 smaller one. Here is the first one.
PATCH 1/3 KVM: Add MSI_ACTION flag for assigned irq
For MSI disable feature later.
Notice I changed ABI here, but due to no userspace patch, I think it's OK.
--- include/linux/kvm.h | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
include/linux/kvm.h +2 -1
@@ -546,6 +546,7 @@ struct kvm_assigned_irq {#define KVM_DEV_ASSIGN_ENABLE_IOMMU (1 << 0) -#define KVM_DEV_IRQ_ASSIGN_ENABLE_MSI (1 << 0) +#define KVM_DEV_IRQ_ASSIGN_MSI_ACTION (1 << 0) +#define KVM_DEV_IRQ_ASSIGN_ENABLE_MSI (1 << 1)#endif
Signature · 1 line
1.5.4.5
Re: [PATCH 1/3] KVM: Add MSI_ACTION flag for assigned irq
Quoted 4 lines · Sheng Yang
For MSI disable feature later.
Notice I changed ABI here, but due to no userspace patch, I think it's OK.
It's not okay, since eventually we will have userspace and it will have to work with older kernels as well.
No released kernel has KVM_DEV_IRQ_ASSIGN_ENABLE_MSI, so it's fine, provided I fold this into the 2.6.29 submission. However, why do this at all? It can only cause confusion.
Signature · 1 line
error compiling committee.c: too many arguments to function
Re: [PATCH 1/3] KVM: Add MSI_ACTION flag for assigned irq
Quoted 6 lines · On Tuesday 30 December 2008 18:19:29 Avi Kivity wrote:
Quoted 4 lines · Sheng Yang
For MSI disable feature later.
Notice I changed ABI here, but due to no userspace patch, I think it's OK.
Quoted 6 lines
It's not okay, since eventually we will have userspace and it will have to work with older kernels as well.
No released kernel has
KVM_DEV_IRQ_ASSIGN_ENABLE_MSI, so it's fine, provided I fold this into the 2.6.29 submission. However, why do this at all? It can only cause confusion.
If we have ENABLE_MSI, and DISABLE, and ENABLE_MSIX, and DISABLE, and MASK_MSIX, and UNMASK, every two action are in pairs but we have to use twice bits to store them. So I'd like to use MSI_ACTION approach...
Signature · 2 lines
regards Yang, Sheng
Re: [PATCH 1/3] KVM: Add MSI_ACTION flag for assigned irq
Quoted 12 lines · Sheng Yang
Quoted 10 lines · On Tuesday 30 December 2008 18:19:29 Avi Kivity wrote:
Quoted 5 lines · Sheng Yang
For MSI disable feature later.
Notice I changed ABI here, but due to no userspace patch, I think it's OK.
It's not okay, since eventually we will have userspace and it will have to work with older kernels as well.
Quoted 7 lines
Quoted 4 lines
No released kernel has
KVM_DEV_IRQ_ASSIGN_ENABLE_MSI, so it's fine, provided I fold this into the 2.6.29 submission. However, why do this at all? It can only cause confusion.If we have
ENABLE_MSI, and DISABLE, andENABLE_MSIX, and DISABLE, andMASK_MSIX, and UNMASK, every two action are in pairs but we have to use twice bits to store them. So I'd like to useMSI_ACTIONapproach...
Well, it you have flags without ENABLE_MSI, doesn't it imply DISABLE_MSI?
The structure contains the state we want to reach, not a command we wish the kernel to perform.
Signature · 1 line
error compiling committee.c: too many arguments to function
Re: [PATCH 1/3] KVM: Add MSI_ACTION flag for assigned irq
Quoted 15 lines · On Tuesday 30 December 2008 18:31:16 Avi Kivity wrote:
Quoted 14 lines · Sheng Yang
Quoted 12 lines · On Tuesday 30 December 2008 18:19:29 Avi Kivity wrote:
Quoted 4 lines · Sheng Yang
For MSI disable feature later.
Notice I changed ABI here, but due to no userspace patch, I think it's OK.
It's not okay, since eventually we will have userspace and it will have to work with older kernels as well.
No released kernel has
KVM_DEV_IRQ_ASSIGN_ENABLE_MSI, so it's fine, provided I fold this into the 2.6.29 submission. However, why do this at all? It can only cause confusion.
Quoted 8 lines
Quoted 3 lines
If we have
ENABLE_MSI, and DISABLE, andENABLE_MSIX, and DISABLE, andMASK_MSIX, and UNMASK, every two action are in pairs but we have to use twice bits to store them. So I'd like to useMSI_ACTIONapproach...Well, it you have flags without
ENABLE_MSI, doesn't it implyDISABLE_MSI?The structure contains the state we want to reach, not a command we wish the kernel to perform.
Yes, that's what I want. But check more than one flags(for MSI-X) to determine where to go is not that clear. So I add a flag here to indicate the operation type which I think is a little more clear.
Signature · 2 lines
regards Yang, Sheng
Re: [PATCH 1/3] KVM: Add MSI_ACTION flag for assigned irq
Quoted 11 lines · Sheng Yang
Quoted 8 lines
Quoted 3 lines
MASK_MSIX, and UNMASK, every two action are in pairs but we have to use twice bits to store them. So I'd like to useMSI_ACTIONapproach...Well, it you have flags without
ENABLE_MSI, doesn't it implyDISABLE_MSI?The structure contains the state we want to reach, not a command we wish the kernel to perform.
Yes, that's what I want. But check more than one flags(for MSI-X) to determine where to go is not that clear. So I add a flag here to indicate the operation type which I think is a little more clear.
Don't understand. Do you mean MSI and MSI-X are mutually exclusive?
If so, we can add a comment.
Signature · 1 line
error compiling committee.c: too many arguments to function
Re: [PATCH 1/3] KVM: Add MSI_ACTION flag for assigned irq
Quoted 10 lines · On Tuesday 30 December 2008 18:41:54 Avi Kivity wrote:
Quoted 9 lines · Sheng Yang
Quoted 8 lines
Quoted 2 lines
MASK_MSIX, and UNMASK, every two action are in pairs but we have to use twice bits to store them. So I'd like to useMSI_ACTIONapproach...Well, it you have flags without
ENABLE_MSI, doesn't it implyDISABLE_MSI?The structure contains the state we want to reach, not a command we wish the kernel to perform.
Quoted 7 lines
Quoted 3 lines
Yes, that's what I want. But check more than one flags(for MSI-X) to determine where to go is not that clear. So I add a flag here to indicate the operation type which I think is a little more clear.
Don't understand. Do you mean MSI and MSI-X are mutually exclusive?
If so, we can add a comment.
Yes, MSI/MSI-X are mutually exclusive.
OK, I will try to keep this, just hope the logic of code won't become too complicate..
Signature · 2 lines
regards Yang, Sheng
PATCH 2/3 KVM: Use kvm_free_assigned_irq() for free irq
In reply to Sheng YangWhich is more convenient...
--- virt/kvm/kvm_main.c | 10 ++-------- 1 files changed, 2 insertions(+), 8 deletions(-)
virt/kvm/kvm_main.c +2 -8
@@ -284,11 +284,7 @@ static int assigned_device_update_intx(struct kvm *kvm,return 0 ;if (irqchip_in_kernel (kvm)) { -if (!msi2intx && - adev->irq_requested_type &KVM_ASSIGNED_DEV_HOST_MSI ) { -free_irq (adev->host_irq, (void *)kvm); -pci_disable_msi (adev->dev); - } +kvm_free_assigned_irq (kvm, adev);if (!capable (CAP_SYS_RAWIO ))return -EPERM ;
@@ -339,9 +335,7 @@ static int assigned_device_update_msi(struct kvm *kvm,if (irqchip_in_kernel (kvm)) {if (!msi2intx) { -if (adev->irq_requested_type & -KVM_ASSIGNED_DEV_HOST_INTX ) -free_irq (adev->host_irq, (void *)adev); +kvm_free_assigned_irq (kvm, adev); r =pci_enable_msi (adev->dev);if (r)
Signature · 1 line
1.5.4.5
Re: [PATCH 2/3] KVM: Use kvm_free_assigned_irq() for free irq
Quoted 12 lines · Sheng Yang
Which is more convenient...
Signed-off-by Sheng Yang <[email protected]>--- virt/kvm/kvm_main.c | 10 ++-------- 1 files changed, 2 insertions(+), 8 deletions(-)virt/kvm/kvm_main.c +0
-0
virt/kvm/kvm_main.c static int assigned_device_update_intx(struct kvm *kvm, · line 288
Quoted 10 lines
virt/kvm/kvm_main.c +1
-5@@ -284,11 +284,7 @@ static int assigned_device_update_intx(struct kvm *kvm,return 0 ;if (irqchip_in_kernel (kvm)) { -if (!msi2intx && - adev->irq_requested_type &KVM_ASSIGNED_DEV_HOST_MSI ) { -free_irq (adev->host_irq, (void *)kvm); -pci_disable_msi (adev->dev); - } +kvm_free_assigned_irq (kvm, adev);
This will call cancel_work_sync(), which may wait upon kvm->lock, which I think we hold here -> deadlock.
I think that the current code has even bigger problems (races), since a scheduled work can arrive after the interrupt has been freed and reallocated.
Signature · 1 line
error compiling committee.c: too many arguments to function
Re: [PATCH 2/3] KVM: Use kvm_free_assigned_irq() for free irq
Quoted 26 lines · On Tuesday 30 December 2008 18:28:21 Avi Kivity wrote:
Quoted 12 lines · Sheng Yang
Which is more convenient...
Signed-off-by Sheng Yang <[email protected]>--- virt/kvm/kvm_main.c | 10 ++-------- 1 files changed, 2 insertions(+), 8 deletions(-)virt/kvm/kvm_main.c +0
-0virt/kvm/kvm_main.c static int assigned_device_update_intx(struct kvm · line 287
Quoted 9 lines
virt/kvm/kvm_main.c +1
-5@@ -284,11 +284,7 @@ static int assigned_device_update_intx(struct kvm *kvm, return 0;if (irqchip_in_kernel (kvm)) { -if (!msi2intx && - adev->irq_requested_type &KVM_ASSIGNED_DEV_HOST_MSI ) { -free_irq (adev->host_irq, (void *)kvm); -pci_disable_msi (adev->dev); - } +kvm_free_assigned_irq (kvm, adev);This will call
cancel_work_sync(), which may wait uponkvm->lock, which I think we hold here -> deadlock.
Quoted 3 lines
I think that the current code has even bigger problems (races), since a scheduled work can arrive after the interrupt has been freed and reallocated.
For the race problem, how about put a cancel_work_sync() for all devices at the beginning of kvm_destroy_vm? Something named kvm_arch_cancel_work_sync...
Signature · 2 lines
regards Yang, Sheng
Re: [PATCH 2/3] KVM: Use kvm_free_assigned_irq() for free irq
Quoted 10 lines · Sheng Yang
Quoted 7 lines
This will call
cancel_work_sync(), which may wait uponkvm->lock, which I think we hold here -> deadlock.I think that the current code has even bigger problems (races), since a scheduled work can arrive after the interrupt has been freed and reallocated.
For the race problem, how about put a
cancel_work_sync()for all devices at the beginning ofkvm_destroy_vm? Something namedkvm_arch_cancel_work_sync...
There is no race at destroy time since the work_struct has a reference on struct kvm. So destruction is only triggered after the last interrupt.
(but what if we get another interrupt immediately afterwards?!)
Signature · 1 line
error compiling committee.c: too many arguments to function
Re: [PATCH 2/3] KVM: Use kvm_free_assigned_irq() for free irq
Quoted 8 lines · On Tuesday 30 December 2008 19:16:27 Avi Kivity wrote:
Quoted 7 lines · Sheng Yang
Quoted 6 lines
This will call
cancel_work_sync(), which may wait uponkvm->lock, which I think we hold here -> deadlock.I think that the current code has even bigger problems (races), since a scheduled work can arrive after the interrupt has been freed and reallocated.
Quoted 8 lines
Quoted 3 lines
For the race problem, how about put a
cancel_work_sync()for all devices at the beginning ofkvm_destroy_vm? Something namedkvm_arch_cancel_work_sync...There is no race at destroy time since the
work_structhas a reference on struct kvm. So destruction is only triggered after the last interrupt.(but what if we get another interrupt immediately afterwards?!)
I don't like that reference, I think we can get it done more elegantly...
Yes, for MSI, we didn't disable interrupt, so got trouble.
So still, how about deal with this at the beginning of kvm_destory_vm? Discard currently reference count. In the function, disable all known interrupt(and result some nested disabled but we would free them later), then cancel_work_sync(). At this time, I think the state is legal(at least workable).
Signature · 2 lines
regards Yang, Sheng
Re: [PATCH 2/3] KVM: Use kvm_free_assigned_irq() for free irq
Quoted 2 lines · Sheng Yang
So still, how about deal with this at the beginning of
kvm_destory_vm? Discard currently reference count. In the function, disable all known interrupt(and result some nested disabled but we would free them later), thencancel_work_sync(). At this time, I think the state is legal(at least workable).
Certainly, if it's the first thing we do, it's legal (and the mutex is valid etc.)
Just be sure to add a nice comment there.
Signature · 1 line
error compiling committee.c: too many arguments to function
PATCH 3/3 KVM: Add support to disable MSI for assigned device
In reply to Sheng YangMSI is always enabled by default for msi2intx=1. But if msi2intx=0, we have to disable MSI if guest require to do so.
The patch also discard unnecessary msi2intx judgment if guest want to update MSI state.
--- virt/kvm/kvm_main.c | 12 ++++++++++-- 1 files changed, 10 insertions(+), 2 deletions(-)
virt/kvm/kvm_main.c +10 -2
@@ -328,6 +328,15 @@ static int assigned_device_update_msi(struct kvm *kvm, adev->irq_requested_type &= ~KVM_ASSIGNED_DEV_GUEST_MSI ; adev->guest_irq = airq->guest_irq; adev->ack_notifier.gsi = airq->guest_irq; + }else { +/* +* Guest require to disable device MSI, we disable MSI and +* re-enable INTx by default again. Notice it's only for +* non-msi2intx. +*/ +kvm_free_assigned_irq (kvm, adev); +assigned_device_update_intx (kvm, adev, airq); +return 0 ; }if (adev->irq_requested_type &KVM_ASSIGNED_DEV_HOST_MSI )
@@ -399,8 +408,7 @@ static int kvm_vm_ioctl_assign_irq(struct kvm *kvm, } } -if ((!msi2intx && - (assigned_irq->flags &KVM_DEV_IRQ_ASSIGN_ENABLE_MSI )) || +if ((assigned_irq->flags &KVM_DEV_IRQ_ASSIGN_MSI_ACTION ) || (msi2intx && match->dev->msi_enabled)) {#ifdef CONFIG_X86 r =assigned_device_update_msi (kvm, match, assigned_irq);
Signature · 1 line
1.5.4.5