Re: [PATCH 3/8] KVM: Handle device assignment to guests

[Avi Kivity <avi@qumranet.com>]

Ben-Ami Yassour wrote:
> On Wed, 2008-07-16 at 18:04 +0300, Avi Kivity wrote:
>   
>> Ben-Ami Yassour wrote:
>>     
>>> +/* Stores information for identifying host PCI devices assigned to the
>>> + * guest: this is used in the host kernel and in the userspace.
>>> + */
>>> +struct kvm_pci_pt_info {
>>> +	unsigned char busnr;
>>> +	unsigned int devfn;
>>> +	__u32 irq;
>>> +};
>>>   
>>>       
>> Badly aligned.  Please use __u32 for all fields, and add a __u32 padding 
>> field at the end so we have the same ABI for i386 and x86_64.
>>
>> Some pci devices support more than one irq.  Should we make irq an 
>> array?  Alternatively, decouple irq assignment from pci information
>> and 
>> let userspace handle everything.  This lets us assign non-pci devices 
>> (like serial ports, etc.).
>>     
> Can we limit the first version to single irq devices, and handle this
> after the merge?
>
>   

Yes.  But we need to get at least the userspace interface right (and 
perhaps only partially implement it).  Breaking the userspace interface 
later is possible, but I'd rather avoid it.

>>>   * ioctls for vcpu fds
>>> diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
>>> index 8ce93c7..0b5bc40 100644
>>> --- a/virt/kvm/ioapic.c
>>> +++ b/virt/kvm/ioapic.c
>>> @@ -288,13 +288,22 @@ void kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
>>>  static void __kvm_ioapic_update_eoi(struct kvm_ioapic *ioapic, int gsi)
>>>  {
>>>  	union ioapic_redir_entry *ent;
>>> +	struct kvm_pci_pt_dev_list *match;
>>> +	unsigned long flags;
>>>  
>>>  	ent = &ioapic->redirtbl[gsi];
>>>  	ASSERT(ent->fields.trig_mode == IOAPIC_LEVEL_TRIG);
>>>  
>>>  	ent->fields.remote_irr = 0;
>>> -	if (!ent->fields.mask && (ioapic->irr & (1 << gsi)))
>>> -		ioapic_service(ioapic, gsi);
>>> +
>>> +	read_lock_irqsave(&kvm_pci_pt_lock, flags);
>>> +	match = kvm_find_pci_pt_dev(&ioapic->kvm->arch.pci_pt_dev_head, NULL,
>>> +				    gsi, KVM_PT_SOURCE_IRQ_ACK);
>>> +	read_unlock_irqrestore(&kvm_pci_pt_lock, flags);
>>> +	if (!match) {
>>> +		if (!ent->fields.mask && (ioapic->irr & (1 << gsi)))
>>> +			ioapic_service(ioapic, gsi);
>>> +	}
>>>   
>>>       
>> What's device assignment code doing in the ioapic?  This should be done 
>> through the notifier (if it needs to return a value, great).
>>
>>     
>
> This purpose of this code is to avoid the call to ioapic_service for
> assigned devices, because it is not required and causes a double
> injection of the interrupt.
>
> Can you please explain why as part of eoi we inject a new interrupt?
>   

If a level triggered interrupt remains active after the eoi, the ioapic 
has to inject it.  This is used to support shared interrupts, or when 
the device has re-raised the line by the time the ack arrives.

I don't see why it should behave differently for assigned devices.

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