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

Avi Kivity
2008-07-17T09:50:09Z

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