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

[Marcelo Tosatti <mtosatti@redhat.com>]

Hi Sheng,

On Tue, Dec 23, 2008 at 04:00:25PM +0800, 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);
>  
>  		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)

Regarding kvm_free_assigned_irq and
assigned_device_update_msi/update_intx:

        if (cancel_work_sync(&assigned_dev->interrupt_work))
                /* We had pending work. That means we will have to take
                 * care of kvm_put_kvm.
                 */ 
                kvm_put_kvm(kvm);
         
        free_irq(assigned_dev->host_irq, (void *)assigned_dev);

What prevents the host IRQ from being triggered between kvm_put_kvm and
free_irq?

Also, if the kvm_put_kvm(kvm) from
kvm_assigned_dev_interrupt_work_handler happens to be the last one,
can't this happen:

- kvm_assigned_dev_interrupt_work_handler
- kvm_put_kvm
- kvm_destroy_vm
- kvm_arch_destroy_vm
- kvm_free_all_assigned_devices
- kvm_free_assigned_device
- kvm_free_assigned_irq
- cancel_work_sync(&assigned_dev->interrupt_work)

deadlock.
    
The msi2intx parameter is somewhat odd. Wouldnt it be more versatile
if controlled from userspace (and per guest) ?