Re: [PATCHv3] qemu-kvm: enable msi with irqchip

From: "Michael S. Tsirkin" <mst@redhat.com>
To: Avi Kivity <avi@redhat.com>
Cc: kvm@vger.kernel.org
Subject: Re: [PATCHv3] qemu-kvm: enable msi with irqchip
Date: Sun, 5 Jul 2009 21:26:33 +0300	[thread overview]
Message-ID: <20090705182633.GA3073@redhat.com> (raw)
In-Reply-To: <4A50EAD4.8010800@redhat.com>

On Sun, Jul 05, 2009 at 09:03:00PM +0300, Avi Kivity wrote:
> On 07/05/2009 08:35 PM, Michael S. Tsirkin wrote:
>> Support msi-x with irqchip in kernel: allocate entries
>> when they are used, and update when they are unmasked.
>>
>> @@ -340,6 +447,10 @@ void msix_notify(PCIDevice *dev, unsigned vector)
>>           msix_set_pending(dev, vector);
>>           return;
>>       }
>> +    if (kvm_enabled()&&  qemu_kvm_irqchip_in_kernel()) {
>> +        kvm_set_irq(dev->msix_irq_entries[vector].gsi, 1, NULL);
>>    
>
> Toggle back to zero after setting to one, for consistency.

You don't really want that.  It's an extra system call, on a data path
operation.
KVM knows it is MSI vector, and so can be relied upon to do the right
thing. Only value 1 should be legal for MSI, I'll send a patch to
fail on 0.

>> --- a/qemu-kvm.c
>> +++ b/qemu-kvm.c
>> @@ -1448,6 +1448,60 @@ int kvm_del_routing_entry(kvm_context_t kvm,
>>   #endif
>>   }
>>
>> +int kvm_update_routing_entry(kvm_context_t kvm,
>> +	                  struct kvm_irq_routing_entry* entry,
>> +                          struct kvm_irq_routing_entry* newentry)
>> +{
>> +#ifdef KVM_CAP_IRQ_ROUTING
>> +	struct kvm_irq_routing_entry *e;
>> +	int i, gsi, found = 0;
>> +
>> +        if (entry->gsi != newentry->gsi ||
>> +            entry->type != newentry->type) {
>> +            return -EINVAL;
>> +        }
>> +	gsi = entry->gsi;
>> +
>> +	for (i = 0; i<  kvm->irq_routes->nr; ++i) {
>> +		e =&kvm->irq_routes->entries[i];
>> +		if (e->type != entry->type || e->gsi != gsi) {
>> +                    continue;
>> +                }
>> +		switch (e->type)
>> +		{
>> +		case KVM_IRQ_ROUTING_IRQCHIP: {
>> +			if (e->u.irqchip.irqchip ==
>> +			    entry->u.irqchip.irqchip
>> +			&&  e->u.irqchip.pin ==
>> +			    entry->u.irqchip.pin) {
>> +			    found = 1;
>> +                        }
>>    
>
> this
> is
> not
> readable
>
>> +			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.data == entry->u.msi.data) {
>> +			    found = 1;
>> +                        }
>> +			break;
>> +		}
>> +		default:
>> +			break;
>> +		}
>> +		if (found) {
>> +			memcpy(e, entry, sizeof *e);
>> +			return 0;
>> +		}
>> +	}
>> +	return -ESRCH;
>> +#else
>> +	return -ENOSYS;
>> +#endif
>> +}
>>    
>
> Please detab the whole thing.
>
> You use perror() on functions that return -errno; please fix.
>
> -- 
> Do not meddle in the internals of kernels, for they are subtle and quick to panic.