From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Michael S. Tsirkin" <mst@redhat.com>
Subject: Re: [PATCHv3] qemu-kvm: enable msi with irqchip
Date: Sun, 5 Jul 2009 21:26:33 +0300
Message-ID: <20090705182633.GA3073@redhat.com>
References: <20090705173545.GA9882@redhat.com> <4A50EAD4.8010800@redhat.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: kvm@vger.kernel.org
To: Avi Kivity <avi@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx2.redhat.com ([66.187.237.31]:33532 "EHLO mx2.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1751687AbZGES1I (ORCPT <rfc822;kvm@vger.kernel.org>);
	Sun, 5 Jul 2009 14:27:08 -0400
Received: from int-mx2.corp.redhat.com (int-mx2.corp.redhat.com [172.16.27.26])
	by mx2.redhat.com (8.13.8/8.13.8) with ESMTP id n65IRBCs003530
	for <kvm@vger.kernel.org>; Sun, 5 Jul 2009 14:27:11 -0400
Content-Disposition: inline
In-Reply-To: <4A50EAD4.8010800@redhat.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

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.