Patchwork [RFC,v2,5/9] hw/vfio: set interrupts using pci irq wrappers

login
register
mail settings
Submitter Marcel Apfelbaum
Date Oct. 2, 2013, 12:41 p.m.
Message ID <1380717694-13091-6-git-send-email-marcel.a@redhat.com>
Download mbox | patch
Permalink /patch/279739/
State New
Headers show

Comments

Marcel Apfelbaum - Oct. 2, 2013, 12:41 p.m.
pci_set_irq and the other pci irq wrappers use
PCI_INTERRUPT_PIN config register to compute device
INTx pin to assert/deassert.

Save INTx pin into the config register before calling
pci_set_irq

Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
---
 hw/misc/vfio.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)
Alex Williamson - Oct. 2, 2013, 3:58 p.m.
On Wed, 2013-10-02 at 15:41 +0300, Marcel Apfelbaum wrote:
> pci_set_irq and the other pci irq wrappers use
> PCI_INTERRUPT_PIN config register to compute device
> INTx pin to assert/deassert.
> 
> Save INTx pin into the config register before calling
> pci_set_irq
> 
> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> ---
>  hw/misc/vfio.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)

Seems ok, but why not take advantage of the pci_irq_raise/lower()
wrappers?  BTW, with PCI being active low, should those be
assert/deassert to avoid confusion confusion with the actual signal
level?  Thanks,

Alex

> 
> diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
> index a1c08fb..3d7297c 100644
> --- a/hw/misc/vfio.c
> +++ b/hw/misc/vfio.c
> @@ -297,7 +297,7 @@ static void vfio_intx_interrupt(void *opaque)
>              'A' + vdev->intx.pin);
>  
>      vdev->intx.pending = true;
> -    qemu_set_irq(vdev->pdev.irq[vdev->intx.pin], 1);
> +    pci_set_irq(&vdev->pdev, 1);
>      vfio_mmap_set_enabled(vdev, false);
>      if (vdev->intx.mmap_timeout) {
>          timer_mod(vdev->intx.mmap_timer,
> @@ -315,7 +315,7 @@ static void vfio_eoi(VFIODevice *vdev)
>              vdev->host.bus, vdev->host.slot, vdev->host.function);
>  
>      vdev->intx.pending = false;
> -    qemu_set_irq(vdev->pdev.irq[vdev->intx.pin], 0);
> +    pci_set_irq(&vdev->pdev, 0);
>      vfio_unmask_intx(vdev);
>  }
>  
> @@ -341,7 +341,7 @@ static void vfio_enable_intx_kvm(VFIODevice *vdev)
>      qemu_set_fd_handler(irqfd.fd, NULL, NULL, vdev);
>      vfio_mask_intx(vdev);
>      vdev->intx.pending = false;
> -    qemu_set_irq(vdev->pdev.irq[vdev->intx.pin], 0);
> +    pci_set_irq(&vdev->pdev, 0);
>  
>      /* Get an eventfd for resample/unmask */
>      if (event_notifier_init(&vdev->intx.unmask, 0)) {
> @@ -417,7 +417,7 @@ static void vfio_disable_intx_kvm(VFIODevice *vdev)
>       */
>      vfio_mask_intx(vdev);
>      vdev->intx.pending = false;
> -    qemu_set_irq(vdev->pdev.irq[vdev->intx.pin], 0);
> +    pci_set_irq(&vdev->pdev, 0);
>  
>      /* Tell KVM to stop listening for an INTx irqfd */
>      if (kvm_vm_ioctl(kvm_state, KVM_IRQFD, &irqfd)) {
> @@ -488,6 +488,7 @@ static int vfio_enable_intx(VFIODevice *vdev)
>      vfio_disable_interrupts(vdev);
>  
>      vdev->intx.pin = pin - 1; /* Pin A (1) -> irq[0] */
> +    pci_config_set_interrupt_pin(vdev->pdev.config, pin);
>  
>  #ifdef CONFIG_KVM
>      /*
> @@ -547,7 +548,7 @@ static void vfio_disable_intx(VFIODevice *vdev)
>      vfio_disable_intx_kvm(vdev);
>      vfio_disable_irqindex(vdev, VFIO_PCI_INTX_IRQ_INDEX);
>      vdev->intx.pending = false;
> -    qemu_set_irq(vdev->pdev.irq[vdev->intx.pin], 0);
> +    pci_set_irq(&vdev->pdev, 0);
>      vfio_mmap_set_enabled(vdev, true);
>  
>      fd = event_notifier_get_fd(&vdev->intx.interrupt);
Marcel Apfelbaum - Oct. 2, 2013, 10:16 p.m.
On Wed, 2013-10-02 at 09:58 -0600, Alex Williamson wrote:
> On Wed, 2013-10-02 at 15:41 +0300, Marcel Apfelbaum wrote:
> > pci_set_irq and the other pci irq wrappers use
> > PCI_INTERRUPT_PIN config register to compute device
> > INTx pin to assert/deassert.
> > 
> > Save INTx pin into the config register before calling
> > pci_set_irq
> > 
> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > ---
> >  hw/misc/vfio.c | 11 ++++++-----
> >  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> Seems ok, but why not take advantage of the pci_irq_raise/lower()
> wrappers?  BTW, with PCI being active low, should those be
> assert/deassert to avoid confusion confusion with the actual signal
> level?  Thanks,

Thanks for the review!

I can use pci_irq_raise/lower(), but I wanted to preserve
the current form, re-factoring:
    qemu_set_irq -> pci_set_irq,
    qemu_irq_lower -> pci_irq_lower
    ...
If you think is worth it, I'll change it. (in all the places)

About assert/deassert instead of lower/raise, I am afraid
it will confuse users having two different set of naming
for interrupts usage.
Is easier to understand that pci_irq_lower behaves the same
as qemu_pci_lower, then pci_irq_deassert.

What do you think?

Thanks,
Marcel

> 
> Alex
> 
> > 
> > diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
> > index a1c08fb..3d7297c 100644
> > --- a/hw/misc/vfio.c
> > +++ b/hw/misc/vfio.c
> > @@ -297,7 +297,7 @@ static void vfio_intx_interrupt(void *opaque)
> >              'A' + vdev->intx.pin);
> >  
> >      vdev->intx.pending = true;
> > -    qemu_set_irq(vdev->pdev.irq[vdev->intx.pin], 1);
> > +    pci_set_irq(&vdev->pdev, 1);
> >      vfio_mmap_set_enabled(vdev, false);
> >      if (vdev->intx.mmap_timeout) {
> >          timer_mod(vdev->intx.mmap_timer,
> > @@ -315,7 +315,7 @@ static void vfio_eoi(VFIODevice *vdev)
> >              vdev->host.bus, vdev->host.slot, vdev->host.function);
> >  
> >      vdev->intx.pending = false;
> > -    qemu_set_irq(vdev->pdev.irq[vdev->intx.pin], 0);
> > +    pci_set_irq(&vdev->pdev, 0);
> >      vfio_unmask_intx(vdev);
> >  }
> >  
> > @@ -341,7 +341,7 @@ static void vfio_enable_intx_kvm(VFIODevice *vdev)
> >      qemu_set_fd_handler(irqfd.fd, NULL, NULL, vdev);
> >      vfio_mask_intx(vdev);
> >      vdev->intx.pending = false;
> > -    qemu_set_irq(vdev->pdev.irq[vdev->intx.pin], 0);
> > +    pci_set_irq(&vdev->pdev, 0);
> >  
> >      /* Get an eventfd for resample/unmask */
> >      if (event_notifier_init(&vdev->intx.unmask, 0)) {
> > @@ -417,7 +417,7 @@ static void vfio_disable_intx_kvm(VFIODevice *vdev)
> >       */
> >      vfio_mask_intx(vdev);
> >      vdev->intx.pending = false;
> > -    qemu_set_irq(vdev->pdev.irq[vdev->intx.pin], 0);
> > +    pci_set_irq(&vdev->pdev, 0);
> >  
> >      /* Tell KVM to stop listening for an INTx irqfd */
> >      if (kvm_vm_ioctl(kvm_state, KVM_IRQFD, &irqfd)) {
> > @@ -488,6 +488,7 @@ static int vfio_enable_intx(VFIODevice *vdev)
> >      vfio_disable_interrupts(vdev);
> >  
> >      vdev->intx.pin = pin - 1; /* Pin A (1) -> irq[0] */
> > +    pci_config_set_interrupt_pin(vdev->pdev.config, pin);
> >  
> >  #ifdef CONFIG_KVM
> >      /*
> > @@ -547,7 +548,7 @@ static void vfio_disable_intx(VFIODevice *vdev)
> >      vfio_disable_intx_kvm(vdev);
> >      vfio_disable_irqindex(vdev, VFIO_PCI_INTX_IRQ_INDEX);
> >      vdev->intx.pending = false;
> > -    qemu_set_irq(vdev->pdev.irq[vdev->intx.pin], 0);
> > +    pci_set_irq(&vdev->pdev, 0);
> >      vfio_mmap_set_enabled(vdev, true);
> >  
> >      fd = event_notifier_get_fd(&vdev->intx.interrupt);
> 
> 
>

Patch

diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
index a1c08fb..3d7297c 100644
--- a/hw/misc/vfio.c
+++ b/hw/misc/vfio.c
@@ -297,7 +297,7 @@  static void vfio_intx_interrupt(void *opaque)
             'A' + vdev->intx.pin);
 
     vdev->intx.pending = true;
-    qemu_set_irq(vdev->pdev.irq[vdev->intx.pin], 1);
+    pci_set_irq(&vdev->pdev, 1);
     vfio_mmap_set_enabled(vdev, false);
     if (vdev->intx.mmap_timeout) {
         timer_mod(vdev->intx.mmap_timer,
@@ -315,7 +315,7 @@  static void vfio_eoi(VFIODevice *vdev)
             vdev->host.bus, vdev->host.slot, vdev->host.function);
 
     vdev->intx.pending = false;
-    qemu_set_irq(vdev->pdev.irq[vdev->intx.pin], 0);
+    pci_set_irq(&vdev->pdev, 0);
     vfio_unmask_intx(vdev);
 }
 
@@ -341,7 +341,7 @@  static void vfio_enable_intx_kvm(VFIODevice *vdev)
     qemu_set_fd_handler(irqfd.fd, NULL, NULL, vdev);
     vfio_mask_intx(vdev);
     vdev->intx.pending = false;
-    qemu_set_irq(vdev->pdev.irq[vdev->intx.pin], 0);
+    pci_set_irq(&vdev->pdev, 0);
 
     /* Get an eventfd for resample/unmask */
     if (event_notifier_init(&vdev->intx.unmask, 0)) {
@@ -417,7 +417,7 @@  static void vfio_disable_intx_kvm(VFIODevice *vdev)
      */
     vfio_mask_intx(vdev);
     vdev->intx.pending = false;
-    qemu_set_irq(vdev->pdev.irq[vdev->intx.pin], 0);
+    pci_set_irq(&vdev->pdev, 0);
 
     /* Tell KVM to stop listening for an INTx irqfd */
     if (kvm_vm_ioctl(kvm_state, KVM_IRQFD, &irqfd)) {
@@ -488,6 +488,7 @@  static int vfio_enable_intx(VFIODevice *vdev)
     vfio_disable_interrupts(vdev);
 
     vdev->intx.pin = pin - 1; /* Pin A (1) -> irq[0] */
+    pci_config_set_interrupt_pin(vdev->pdev.config, pin);
 
 #ifdef CONFIG_KVM
     /*
@@ -547,7 +548,7 @@  static void vfio_disable_intx(VFIODevice *vdev)
     vfio_disable_intx_kvm(vdev);
     vfio_disable_irqindex(vdev, VFIO_PCI_INTX_IRQ_INDEX);
     vdev->intx.pending = false;
-    qemu_set_irq(vdev->pdev.irq[vdev->intx.pin], 0);
+    pci_set_irq(&vdev->pdev, 0);
     vfio_mmap_set_enabled(vdev, true);
 
     fd = event_notifier_get_fd(&vdev->intx.interrupt);