[0/3][RESEND] Device assignment code clean up and MSI disable support

[0/3][RESEND] Device assignment code clean up and MSI disable support

[Sheng Yang]

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

[Sheng Yang]

For MSI disable feature later.

Notice I changed ABI here, but due to no userspace patch, I think it's OK.

Signed-off-by: Sheng Yang <sheng@linux.intel.com>
---
 include/linux/kvm.h |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/include/linux/kvm.h b/include/linux/kvm.h
index 42f51dc..c24f207 100644
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -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
-- 
1.5.4.5

Re: [PATCH 1/3] KVM: Add MSI_ACTION flag for assigned irq

[Avi Kivity]

Sheng Yang wrote:
> 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.


-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 1/3] KVM: Add MSI_ACTION flag for assigned irq

[Sheng Yang]

On Tuesday 30 December 2008 18:19:29 Avi Kivity wrote:
> Sheng Yang wrote:
> > 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.

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...

-- 
regards
Yang, Sheng

Re: [PATCH 1/3] KVM: Add MSI_ACTION flag for assigned irq

[Avi Kivity]

Sheng Yang wrote:
> On Tuesday 30 December 2008 18:19:29 Avi Kivity wrote:
>   
>> Sheng Yang wrote:
>>     
>>> 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.
>>     
>
> 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...
>   

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.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 1/3] KVM: Add MSI_ACTION flag for assigned irq

[Sheng Yang]

On Tuesday 30 December 2008 18:31:16 Avi Kivity wrote:
> Sheng Yang wrote:
> > On Tuesday 30 December 2008 18:19:29 Avi Kivity wrote:
> >> Sheng Yang wrote:
> >>> 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.
> >
> > 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...
>
> 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.

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.

-- 
regards
Yang, Sheng

Re: [PATCH 1/3] KVM: Add MSI_ACTION flag for assigned irq

[Avi Kivity]

Sheng Yang wrote:
>>> 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...
>>>       
>> 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.
>>     
>
> 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.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 1/3] KVM: Add MSI_ACTION flag for assigned irq

[Sheng Yang]

On Tuesday 30 December 2008 18:41:54 Avi Kivity wrote:
> Sheng Yang wrote:
> >>> 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...
> >>
> >> 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.
> >
> > 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..

-- 
regards
Yang, Sheng

[PATCH 2/3] KVM: Use kvm_free_assigned_irq() for free irq

[Sheng Yang]

Which is more convenient...

Signed-off-by: Sheng Yang <sheng@linux.intel.com>
---
 virt/kvm/kvm_main.c |   10 ++--------
 1 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index ffd261d..cd84b3e 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -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)
-- 
1.5.4.5

Re: [PATCH 2/3] KVM: Use kvm_free_assigned_irq() for free irq

[Avi Kivity]

Sheng Yang wrote:
> Which is more convenient...
>
> Signed-off-by: Sheng Yang <sheng@linux.intel.com>
> ---
>  virt/kvm/kvm_main.c |   10 ++--------
>  1 files changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index ffd261d..cd84b3e 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -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.


-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 2/3] KVM: Use kvm_free_assigned_irq() for free irq

[Sheng Yang]

On Tuesday 30 December 2008 18:28:21 Avi Kivity wrote:
> Sheng Yang wrote:
> > Which is more convenient...
> >
> > Signed-off-by: Sheng Yang <sheng@linux.intel.com>
> > ---
> >  virt/kvm/kvm_main.c |   10 ++--------
> >  1 files changed, 2 insertions(+), 8 deletions(-)
> >
> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > index ffd261d..cd84b3e 100644
> > --- a/virt/kvm/kvm_main.c
> > +++ b/virt/kvm/kvm_main.c
> > @@ -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.

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...

-- 
regards
Yang, Sheng

Re: [PATCH 2/3] KVM: Use kvm_free_assigned_irq() for free irq

[Avi Kivity]

Sheng Yang wrote:
>> 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.
>>     
>
> 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...
>   

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?!)

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 2/3] KVM: Use kvm_free_assigned_irq() for free irq

[Sheng Yang]

On Tuesday 30 December 2008 19:16:27 Avi Kivity wrote:
> Sheng Yang wrote:
> >> 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.
> >
> > 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...
>
> 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?!)

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).

-- 
regards
Yang, Sheng

Re: [PATCH 2/3] KVM: Use kvm_free_assigned_irq() for free irq

[Avi Kivity]

Sheng Yang wrote:
> 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).
>   

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.

-- 
error compiling committee.c: too many arguments to function

[PATCH 3/3] KVM: Add support to disable MSI for assigned device

[Sheng Yang]

MSI 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.

Signed-off-by: Sheng Yang <sheng@linux.intel.com>
---
 virt/kvm/kvm_main.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index cd84b3e..111738b 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -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);
-- 
1.5.4.5