On Wed, 2012-02-29 at 16:38 +0100, Jan Kiszka wrote:
> On 2012-02-29 16:22, Alex Williamson wrote:
> > On Tue, 2012-02-28 at 14:19 +0100, Jan Kiszka wrote:
> >> PCI 2.3 allows to generically disable IRQ sources at device level. This
> >> enables us to share legacy IRQs of such devices with other host devices
> >> when passing them to a guest.
> >>
> >> The new IRQ sharing feature introduced here is optional, user space has
> >> to request it explicitly.
> >
> > Is this really true? Looks like it's automatic.
>
> It is true: no IRQ sharing without userspace setting
> KVM_DEV_ASSIGN_PCI_2_3 during KVM_ASSIGN_PCI_DEVICE. My qemu-kvm patches
> will allow to control this, but make it default on (reasons for this
> will be provided in that context).
Ah right, we only clear it if not available. Thanks.
> >
> >> Moreover, user space can inform us about its
> >> view of PCI_COMMAND_INTX_DISABLE so that we can avoid unmasking the
> >> interrupt and signaling it if the guest masked it via the virtualized
> >> PCI config space.
> >>
> >> Signed-off-by: Jan Kiszka <jan.kiszka@xxxxxxxxxxx>
> >> ---
> >>
> >> Changes in v4:
> >> - Integrated doc changes as proposed by Alex
> >> - Fixed deassign_host_irq /wrt MSI
> >> - Fixed kvm_vm_ioctl_set_pci_irq_mask /wrt INTx unmasking of non-2.3
> >> devices
> >>
> >> Documentation/virtual/kvm/api.txt | 41 +++++++
> >> arch/x86/kvm/x86.c | 1 +
> >> include/linux/kvm.h | 6 +
> >> include/linux/kvm_host.h | 2 +
> >> virt/kvm/assigned-dev.c | 209 +++++++++++++++++++++++++++++++-----
> >> 5 files changed, 230 insertions(+), 29 deletions(-)
> > [snip]
> >> +static int kvm_vm_ioctl_set_pci_irq_mask(struct kvm *kvm,
> >> + struct kvm_assigned_pci_dev *assigned_dev)
> >> +{
> >> + int r = 0;
> >> + struct kvm_assigned_dev_kernel *match;
> >> +
> >> + mutex_lock(&kvm->lock);
> >> +
> >> + match = kvm_find_assigned_dev(&kvm->arch.assigned_dev_head,
> >> + assigned_dev->assigned_dev_id);
> >> + if (!match) {
> >> + r = -ENODEV;
> >> + goto out;
> >> + }
> >> +
> >> + mutex_lock(&match->intx_mask_lock);
> >> +
> >> + match->flags &= ~KVM_DEV_ASSIGN_MASK_INTX;
> >> + match->flags |= assigned_dev->flags & KVM_DEV_ASSIGN_MASK_INTX;
> >> +
> >> + if (match->irq_requested_type & KVM_DEV_IRQ_GUEST_INTX) {
> >> + if (assigned_dev->flags & KVM_DEV_ASSIGN_MASK_INTX) {
> >> + kvm_set_irq(match->kvm, match->irq_source_id,
> >> + match->guest_irq, 0);
> >> + /*
> >> + * Masking at hardware-level is performed on demand,
> >> + * i.e. when an IRQ actually arrives at the host.
> >> + */
> >> + } else if (!(assigned_dev->flags & KVM_DEV_ASSIGN_PCI_2_3)) {
> >> + /*
> >> + * Unmask the IRQ line if required. Unmasking at
> >> + * device level will be performed by user space.
> >> + */
> >> + spin_lock_irq(&match->intx_lock);
> >> + if (match->host_irq_disabled) {
> >> + enable_irq(match->host_irq);
> >> + match->host_irq_disabled = false;
> >> + }
> >> + spin_unlock_irq(&match->intx_lock);
> >
> > This still looks broken. If we start with a PCI 2.3 device with INTx
> > disabled, how does this ever kick start to get another interrupt?
> > Shouldn't we just call kvm_assigned_dev_ack_irq() here and handle both
> > INTx modes? Thanks,
>
> No, that would be wrong. The IRQ must be delivered to the guest first.
>
> And that will happen with 2.3 once userspace unmasks the device INTx
> and, thus, triggers another host-side IRQ. Same for non-2.3 devices,
> just that this happens on enable_irq.
Ok, got it. I was tempted to write in the revised description that
userspace could skip the device command register update if INTx disable
is the only change, but because of the way this works, that is clearly
not the case. Thanks.
Acked-by: Alex Williamson <alex.williamson@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
