[Qemu-devel] [PATCH] pci: Return PCI_INTX_DISABLED when no bus INTx routing support

[Qemu-devel] [PATCH] pci: Return PCI_INTX_DISABLED when no bus INTx routing support

[Alex Williamson]

Rather than assert, simply return PCI_INTX_DISABLED when we don't
have a pci_route_irq_fn.  PIIX already returns DISABLED for an
invalid pin, so users already deal with this state.  Users of this
interface should only be acting on an ENABLED or INVERTED return
value (though we really have no support for INVERTED).

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

A compromise to the gridlock; defuse the assert, but don't add
a new state to the API.  Thanks,

Alex

 hw/pci.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/pci.c b/hw/pci.c
index 83d262a..9525220 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -1094,7 +1094,11 @@ PCIINTxRoute pci_device_route_intx_to_irq(PCIDevice *dev, int pin)
          pin = bus->map_irq(dev, pin);
          dev = bus->parent_dev;
     } while (dev);
-    assert(bus->route_intx_to_irq);
+
+    if (!bus->route_intx_to_irq) {
+        return (PCIINTxRoute) { PCI_INTX_DISABLED, -1 };
+    }
+
     return bus->route_intx_to_irq(bus->irq_opaque, pin);
 }
 

Re: [Qemu-devel] [PATCH] pci: Return PCI_INTX_DISABLED when no bus INTx routing support

[Jan Kiszka]

On 2012-10-17 20:25, Alex Williamson wrote:
> Rather than assert, simply return PCI_INTX_DISABLED when we don't
> have a pci_route_irq_fn.  PIIX already returns DISABLED for an
> invalid pin, so users already deal with this state.  Users of this
> interface should only be acting on an ENABLED or INVERTED return
> value (though we really have no support for INVERTED).
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
> 
> A compromise to the gridlock; defuse the assert, but don't add
> a new state to the API.  Thanks,

But how do you tell "unsupported" apart from "disabled" in VFIO? If the
chipset truly disabled the line, you must not provide it to the guest in
some other way.

Jan

> 
> Alex
> 
>  hw/pci.c |    6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index 83d262a..9525220 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -1094,7 +1094,11 @@ PCIINTxRoute pci_device_route_intx_to_irq(PCIDevice *dev, int pin)
>           pin = bus->map_irq(dev, pin);
>           dev = bus->parent_dev;
>      } while (dev);
> -    assert(bus->route_intx_to_irq);
> +
> +    if (!bus->route_intx_to_irq) {
> +        return (PCIINTxRoute) { PCI_INTX_DISABLED, -1 };
> +    }
> +
>      return bus->route_intx_to_irq(bus->irq_opaque, pin);
>  }
>  
> 

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH] pci: Return PCI_INTX_DISABLED when no bus INTx routing support

[Alex Williamson]

On Wed, 2012-10-17 at 20:40 +0200, Jan Kiszka wrote:
> On 2012-10-17 20:25, Alex Williamson wrote:
> > Rather than assert, simply return PCI_INTX_DISABLED when we don't
> > have a pci_route_irq_fn.  PIIX already returns DISABLED for an
> > invalid pin, so users already deal with this state.  Users of this
> > interface should only be acting on an ENABLED or INVERTED return
> > value (though we really have no support for INVERTED).
> > 
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > ---
> > 
> > A compromise to the gridlock; defuse the assert, but don't add
> > a new state to the API.  Thanks,
> 
> But how do you tell "unsupported" apart from "disabled" in VFIO? If the
> chipset truly disabled the line, you must not provide it to the guest in

The downside to not being able to distinguish is that vfio can't tell
the user anything useful about whether it should work, but needs to be
implemented in the chipset or if it's just currently disabled.  Maybe an
error_report below would fix that, but yes, we do lose granularity we
had with NOROUTE.

Functionally, there's no direct kvm interrupt injection when this
returns DISABLED, so vfio routes the interrupt to qemu where we do
qemu_set_irq, and it can get dropped if it's really disabled.
pci-assign will just not program the interrupt to kvm and intx won't
work :-\  Thanks,

Alex


> >  hw/pci.c |    6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/pci.c b/hw/pci.c
> > index 83d262a..9525220 100644
> > --- a/hw/pci.c
> > +++ b/hw/pci.c
> > @@ -1094,7 +1094,11 @@ PCIINTxRoute pci_device_route_intx_to_irq(PCIDevice *dev, int pin)
> >           pin = bus->map_irq(dev, pin);
> >           dev = bus->parent_dev;
> >      } while (dev);
> > -    assert(bus->route_intx_to_irq);
> > +
> > +    if (!bus->route_intx_to_irq) {
> > +        return (PCIINTxRoute) { PCI_INTX_DISABLED, -1 };
> > +    }
> > +
> >      return bus->route_intx_to_irq(bus->irq_opaque, pin);
> >  }
> >  
> > 
> 

Re: [Qemu-devel] [PATCH] pci: Return PCI_INTX_DISABLED when no bus INTx routing support

[Michael S. Tsirkin]

On Wed, Oct 17, 2012 at 12:54:36PM -0600, Alex Williamson wrote:
> On Wed, 2012-10-17 at 20:40 +0200, Jan Kiszka wrote:
> > On 2012-10-17 20:25, Alex Williamson wrote:
> > > Rather than assert, simply return PCI_INTX_DISABLED when we don't
> > > have a pci_route_irq_fn.  PIIX already returns DISABLED for an
> > > invalid pin, so users already deal with this state.  Users of this
> > > interface should only be acting on an ENABLED or INVERTED return
> > > value (though we really have no support for INVERTED).
> > > 
> > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > > ---
> > > 
> > > A compromise to the gridlock; defuse the assert, but don't add
> > > a new state to the API.  Thanks,
> > 
> > But how do you tell "unsupported" apart from "disabled" in VFIO? If the
> > chipset truly disabled the line, you must not provide it to the guest in
> 
> The downside to not being able to distinguish is that vfio can't tell
> the user anything useful about whether it should work, but needs to be
> implemented in the chipset or if it's just currently disabled.  Maybe an
> error_report below would fix that, but yes, we do lose granularity we
> had with NOROUTE.

Stick error_report in pci_device_route_intx_to_irq you mean? OK why not.

> Functionally, there's no direct kvm interrupt injection when this
> returns DISABLED, so vfio routes the interrupt to qemu where we do
> qemu_set_irq, and it can get dropped if it's really disabled.
> pci-assign will just not program the interrupt to kvm and intx won't
> work :-\  Thanks,
> 
> Alex
> 

All this handling is just dead code anyway, it's there just in case anyway.


> > >  hw/pci.c |    6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/pci.c b/hw/pci.c
> > > index 83d262a..9525220 100644
> > > --- a/hw/pci.c
> > > +++ b/hw/pci.c
> > > @@ -1094,7 +1094,11 @@ PCIINTxRoute pci_device_route_intx_to_irq(PCIDevice *dev, int pin)
> > >           pin = bus->map_irq(dev, pin);
> > >           dev = bus->parent_dev;
> > >      } while (dev);
> > > -    assert(bus->route_intx_to_irq);
> > > +
> > > +    if (!bus->route_intx_to_irq) {
> > > +        return (PCIINTxRoute) { PCI_INTX_DISABLED, -1 };
> > > +    }
> > > +
> > >      return bus->route_intx_to_irq(bus->irq_opaque, pin);
> > >  }
> > >  
> > > 
> > 
> 
> 

Re: [Qemu-devel] [PATCH] pci: Return PCI_INTX_DISABLED when no bus INTx routing support

[Michael S. Tsirkin]

On Wed, Oct 17, 2012 at 08:40:05PM +0200, Jan Kiszka wrote:
> On 2012-10-17 20:25, Alex Williamson wrote:
> > Rather than assert, simply return PCI_INTX_DISABLED when we don't
> > have a pci_route_irq_fn.  PIIX already returns DISABLED for an
> > invalid pin, so users already deal with this state.  Users of this
> > interface should only be acting on an ENABLED or INVERTED return
> > value (though we really have no support for INVERTED).
> > 
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > ---
> > 
> > A compromise to the gridlock; defuse the assert, but don't add
> > a new state to the API.  Thanks,
> 
> But how do you tell "unsupported" apart from "disabled" in VFIO? If the
> chipset truly disabled the line, you must not provide it to the guest in
> some other way.
> 
> Jan

vfio does not care. It merely disables irqfd and delivers irqs to qemu.

> > 
> > Alex
> > 
> >  hw/pci.c |    6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/pci.c b/hw/pci.c
> > index 83d262a..9525220 100644
> > --- a/hw/pci.c
> > +++ b/hw/pci.c
> > @@ -1094,7 +1094,11 @@ PCIINTxRoute pci_device_route_intx_to_irq(PCIDevice *dev, int pin)
> >           pin = bus->map_irq(dev, pin);
> >           dev = bus->parent_dev;
> >      } while (dev);
> > -    assert(bus->route_intx_to_irq);
> > +
> > +    if (!bus->route_intx_to_irq) {
> > +        return (PCIINTxRoute) { PCI_INTX_DISABLED, -1 };
> > +    }
> > +
> >      return bus->route_intx_to_irq(bus->irq_opaque, pin);
> >  }
> >  
> > 
> 
> -- 
> Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
> Corporate Competence Center Embedded Linux