Re: [PATCH 0/16 v5] Device assignment improvement in userspace

[Marcelo Tosatti <mtosatti@redhat.com>]

On Mon, Mar 16, 2009 at 11:10:47AM +0200, Avi Kivity wrote:
> Sheng Yang wrote:
>> Patch 1 and 2 are new ones, all the others had been sent before.
>>
>> This (huge) patchset, contained:
>>
>> Patch 1..2 are new interface after reworked device assignment kernel part.
>>
>> Patch 3..6 are generic capability support mechanism. These may can be adopted
>> by QEmu upstream as well.
>>
>> Patch 7..10 enable MSI with device assignment on KVM. Also due to reworked
>> device assignment kernel part discard MSI convert to INTx mechanism, patch 10
>> enable it again in userspace.
>>
>> Patch 11..13 enable MSI-X with device assignment on KVM.
>>
>> And Patch 14..16 enable SR-IOV with KVM.
>>
>> Update from latest series:
>>
>> 1. Convert to the new ioctl interface.
>> 2. Merge capability configuration space with PCIDevice one.
>> 3. Support of deassign IRQ(unload driver) with MSI/MSI-X better.
>> 4. Not assume IRQ0 means no INTx any longer, but check interrupt pin field in
>> configuration space for the judgment.
>>
>> Please help to review! Thanks!
>>   
>
> This looks ready to apply.  I'd like Marcelo to look it over, though,  
> before.

Looks good to me, ready to be applied.

There is one pending detail in the ioctl interface. Its a minor issue,
but might become troublesome later (and can be fixed after the patchset
has been applied).

The unassign ioctl takes "struct kvm_assigned_irq" and parses its flags
to decide what to do, in this way:

- If any bit is set in the guest mask (GUEST_INTX, GUEST_MSI,
  GUEST_MSIX), we disable guest-side interrupt.
- Likewise for host, disabling host-side interrupt.

        host_irq_type = irq_requested_type & KVM_DEV_IRQ_HOST_MASK;
        guest_irq_type = irq_requested_type & KVM_DEV_IRQ_GUEST_MASK;

        if (host_irq_type)
                deassign_host_irq(kvm, assigned_dev);
        if (guest_irq_type)
                deassign_guest_irq(kvm, assigned_dev);

This is a little confusing. If we simply want to disable
_whatever is assigned_ in either guest or host side, we want a
UNASSIGN_GUEST/UNASSIGN_HOST pair of flags (this is how the ioctl
behaves, but we pass more flags and don't use them effectively).

Or, if the unassign ioctl continues to receive guest/host flags with
interrupt type detail, it should error out if userspace passed a type
that does not match what is currently assigned.

The current behaviour is simpler for userspace, but then we'd need not
to pass "struct kvm_assigned_irq".

Sheng, what do you say?