Re: [PATCH 1/3] kvm: fix irq 0 assignment

[Marcelo Tosatti <mtosatti@redhat.com>]

On Wed, Mar 04, 2009 at 03:54:27PM +0800, Sheng Yang wrote:
> Shouldn't update assigned irq if host irq is 0, which means uninitialized
> or don't support INTx.
> 
> Signed-off-by: Sheng Yang <sheng@linux.intel.com>
> ---
>  qemu/hw/device-assignment.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/qemu/hw/device-assignment.c b/qemu/hw/device-assignment.c
> index 32fba2a..7f9c789 100644
> --- a/qemu/hw/device-assignment.c
> +++ b/qemu/hw/device-assignment.c
> @@ -574,6 +574,10 @@ static int assign_irq(AssignedDevInfo *adev)
>      AssignedDevice *dev = adev->assigned_dev;
>      int irq, r = 0;
>  
> +    /* irq 0 means uninitialized */
> +    if (dev->real_device.irq == 0)
> +        return 0;
> +
>      irq = pci_map_irq(&dev->dev, dev->intpin);
>      irq = piix_get_irq(irq);

Sheng,

Please do not special case irq 0. The fact that only x86/IA64 are
supported at the moment does not mean other architectures can't 
use it.

Also, the kernel code to handle dev/irq assignment is quite messy. It
should be only a mechanism, with userspace controlling the policy.

For example:

        if (match->irq_requested_type & KVM_ASSIGNED_DEV_MSIX)
                current_flags |= KVM_DEV_IRQ_ASSIGN_ENABLE_MSIX;
        else if ((match->irq_requested_type & KVM_ASSIGNED_DEV_HOST_MSI) &&
                 (match->irq_requested_type & KVM_ASSIGNED_DEV_GUEST_MSI))
                current_flags |= KVM_DEV_IRQ_ASSIGN_ENABLE_MSI;

And then in userspace you have

+    if (*ctrl_word & PCI_MSIX_ENABLE) {
+        assigned_irq_data.flags = KVM_DEV_IRQ_ASSIGN_ENABLE_MSIX;
+        if (assigned_dev_update_msix_mmio(pci_dev) < 0) {
+            perror("assigned_dev_update_msix_mmio");
+        }
+    }

We really need to fix this before merging anything else in this area.

So ideally the kernel only provides ioctl commands that do one thing 
at a time:

- assign host device
- unassign guest device

- assign host irq (INTx or MSI)
- assign dev irq (INTx or MSI)

- unassign host irq
- unassign dev irq

So you don't have to make policy decisions like

        } else {
                /*
                 * Guest require to disable device MSI, we disable MSI
                 * and
                 * re-enable INTx by default again. Notice it's only for
                 * non-msi2intx.
                 */
                assigned_device_update_intx(kvm, adev, airq);
                return 0;
        }

Because you do in userspace. However, I do not think it is necessary to
create new ioctl commands and deprecate the old ones (it seems possible
to do this using the flags passed in "struct kvm_assigned_irq")

However, the current userspace code probably relies on implicit
behaviour by the kernel part. IMHO it is fair to break older 
userspace. Better change it sooner than later.

For example, don't do this

        if ((changed_flags & KVM_DEV_IRQ_ASSIGN_MSI_ACTION) ||
            (msi2intx && match->dev->msi_enabled)) {

But just enable MSI in either host or guest (one at a time). Return an
error if you can't. And don't make such assumptions:

        } else if (assigned_irq->host_irq == 0 && match->dev->irq == 0)
                /* Host device IRQ 0 means don't support INTx */

To use the current ioctl commands, maybe enforce the "one thing at a
time" nature and fail otherwise.

Can you please investigate about this possibility? Because talk is
cheap and I'm not sure whether you can always separate host/guest IRQ
assignment, but probably as long as done carefully.

Am I missing any reason why policy can't live outside the kernel?