Patchwork [2/6] pci-assign: Add support for no-route

login
register
mail settings
Submitter Alex Williamson
Date Oct. 2, 2012, 7:21 p.m.
Message ID <20121002192147.31100.41594.stgit@bling.home>
Download mbox | patch
Permalink /patch/188638/
State New
Headers show

Comments

Alex Williamson - Oct. 2, 2012, 7:21 p.m.
In the event that a pci-assign device is added to a chipset that
hasn't yet implemented the INTx routing interface, exit gracefully
instead of killing the VM.  I'm sure we'll implement this for q35,
but there's no reason for such a harsh response.

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

 hw/kvm/pci-assign.c |    6 ++++++
 1 file changed, 6 insertions(+)
Anthony Liguori - Oct. 2, 2012, 7:55 p.m.
Alex Williamson <alex.williamson@redhat.com> writes:

> In the event that a pci-assign device is added to a chipset that
> hasn't yet implemented the INTx routing interface, exit gracefully
> instead of killing the VM.  I'm sure we'll implement this for q35,
> but there's no reason for such a harsh response.
>
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
>
>  hw/kvm/pci-assign.c |    6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/hw/kvm/pci-assign.c b/hw/kvm/pci-assign.c
> index 05b93d9..7ce0f37 100644
> --- a/hw/kvm/pci-assign.c
> +++ b/hw/kvm/pci-assign.c
> @@ -887,6 +887,12 @@ static int assign_intx(AssignedDevice *dev)
>  
>      intx_route = pci_device_route_intx_to_irq(&dev->dev, dev->intpin);
>      assert(intx_route.mode != PCI_INTX_INVERTED);
> +    if (intx_route.mode == PCI_INTX_NOROUTE) {
> +        error_report("pci-assign: chipset provides no INTx routing "
> +                     "information, but device supports INTx interrupt mode.\n");
> +        pci_device_set_intx_routing_notifier(&dev->dev, NULL);
> +        return -ENOTSUP;
> +    }

Please don't use error_report() in new code.  Propagate an Error object
using error_setg() and friends.

Regards,

Anthony Liguori

>  
>      if (dev->intx_route.mode == intx_route.mode &&
>          dev->intx_route.irq == intx_route.irq) {
Alex Williamson - Oct. 2, 2012, 8:12 p.m.
On Tue, 2012-10-02 at 14:55 -0500, Anthony Liguori wrote:
> Alex Williamson <alex.williamson@redhat.com> writes:
> 
> > In the event that a pci-assign device is added to a chipset that
> > hasn't yet implemented the INTx routing interface, exit gracefully
> > instead of killing the VM.  I'm sure we'll implement this for q35,
> > but there's no reason for such a harsh response.
> >
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > ---
> >
> >  hw/kvm/pci-assign.c |    6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/hw/kvm/pci-assign.c b/hw/kvm/pci-assign.c
> > index 05b93d9..7ce0f37 100644
> > --- a/hw/kvm/pci-assign.c
> > +++ b/hw/kvm/pci-assign.c
> > @@ -887,6 +887,12 @@ static int assign_intx(AssignedDevice *dev)
> >  
> >      intx_route = pci_device_route_intx_to_irq(&dev->dev, dev->intpin);
> >      assert(intx_route.mode != PCI_INTX_INVERTED);
> > +    if (intx_route.mode == PCI_INTX_NOROUTE) {
> > +        error_report("pci-assign: chipset provides no INTx routing "
> > +                     "information, but device supports INTx interrupt mode.\n");
> > +        pci_device_set_intx_routing_notifier(&dev->dev, NULL);
> > +        return -ENOTSUP;
> > +    }
> 
> Please don't use error_report() in new code.  Propagate an Error object
> using error_setg() and friends.

That doesn't really seem to be an option since this function is called
both from the driver initfn and the interrupt routing change notifier.
Neither of those give me an Error object... suggestions?  Thanks,

Alex
Paolo Bonzini - Oct. 3, 2012, 9:52 a.m.
Il 02/10/2012 22:12, Alex Williamson ha scritto:
> On Tue, 2012-10-02 at 14:55 -0500, Anthony Liguori wrote:
>> Alex Williamson <alex.williamson@redhat.com> writes:
>>
>>> In the event that a pci-assign device is added to a chipset that
>>> hasn't yet implemented the INTx routing interface, exit gracefully
>>> instead of killing the VM.  I'm sure we'll implement this for q35,
>>> but there's no reason for such a harsh response.
>>>
>>> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
>>> ---
>>>
>>>  hw/kvm/pci-assign.c |    6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/hw/kvm/pci-assign.c b/hw/kvm/pci-assign.c
>>> index 05b93d9..7ce0f37 100644
>>> --- a/hw/kvm/pci-assign.c
>>> +++ b/hw/kvm/pci-assign.c
>>> @@ -887,6 +887,12 @@ static int assign_intx(AssignedDevice *dev)
>>>  
>>>      intx_route = pci_device_route_intx_to_irq(&dev->dev, dev->intpin);
>>>      assert(intx_route.mode != PCI_INTX_INVERTED);
>>> +    if (intx_route.mode == PCI_INTX_NOROUTE) {
>>> +        error_report("pci-assign: chipset provides no INTx routing "
>>> +                     "information, but device supports INTx interrupt mode.\n");
>>> +        pci_device_set_intx_routing_notifier(&dev->dev, NULL);
>>> +        return -ENOTSUP;
>>> +    }
>>
>> Please don't use error_report() in new code.  Propagate an Error object
>> using error_setg() and friends.
> 
> That doesn't really seem to be an option since this function is called
> both from the driver initfn and the interrupt routing change notifier.
> Neither of those give me an Error object... suggestions?  Thanks,

Both assigned_dev_update_irq_routing and assigned_dev_update_msi would
have to print the error themselves (qerror_report_err + error_free).
Looks like a waste of time, frankly.

Paolo
Anthony Liguori - Oct. 3, 2012, 12:32 p.m.
Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 02/10/2012 22:12, Alex Williamson ha scritto:
>> On Tue, 2012-10-02 at 14:55 -0500, Anthony Liguori wrote:
>>> Alex Williamson <alex.williamson@redhat.com> writes:
>>>
>>>> In the event that a pci-assign device is added to a chipset that
>>>> hasn't yet implemented the INTx routing interface, exit gracefully
>>>> instead of killing the VM.  I'm sure we'll implement this for q35,
>>>> but there's no reason for such a harsh response.
>>>>
>>>> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
>>>> ---
>>>>
>>>>  hw/kvm/pci-assign.c |    6 ++++++
>>>>  1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/hw/kvm/pci-assign.c b/hw/kvm/pci-assign.c
>>>> index 05b93d9..7ce0f37 100644
>>>> --- a/hw/kvm/pci-assign.c
>>>> +++ b/hw/kvm/pci-assign.c
>>>> @@ -887,6 +887,12 @@ static int assign_intx(AssignedDevice *dev)
>>>>  
>>>>      intx_route = pci_device_route_intx_to_irq(&dev->dev, dev->intpin);
>>>>      assert(intx_route.mode != PCI_INTX_INVERTED);
>>>> +    if (intx_route.mode == PCI_INTX_NOROUTE) {
>>>> +        error_report("pci-assign: chipset provides no INTx routing "
>>>> +                     "information, but device supports INTx interrupt mode.\n");
>>>> +        pci_device_set_intx_routing_notifier(&dev->dev, NULL);
>>>> +        return -ENOTSUP;
>>>> +    }
>>>
>>> Please don't use error_report() in new code.  Propagate an Error object
>>> using error_setg() and friends.
>> 
>> That doesn't really seem to be an option since this function is called
>> both from the driver initfn and the interrupt routing change notifier.
>> Neither of those give me an Error object... suggestions?  Thanks,
>
> Both assigned_dev_update_irq_routing and assigned_dev_update_msi would
> have to print the error themselves (qerror_report_err + error_free).
> Looks like a waste of time, frankly.

That's a pity but I do agree.

Regards,

Anthony Liguori

>
> Paolo

Patch

diff --git a/hw/kvm/pci-assign.c b/hw/kvm/pci-assign.c
index 05b93d9..7ce0f37 100644
--- a/hw/kvm/pci-assign.c
+++ b/hw/kvm/pci-assign.c
@@ -887,6 +887,12 @@  static int assign_intx(AssignedDevice *dev)
 
     intx_route = pci_device_route_intx_to_irq(&dev->dev, dev->intpin);
     assert(intx_route.mode != PCI_INTX_INVERTED);
+    if (intx_route.mode == PCI_INTX_NOROUTE) {
+        error_report("pci-assign: chipset provides no INTx routing "
+                     "information, but device supports INTx interrupt mode.\n");
+        pci_device_set_intx_routing_notifier(&dev->dev, NULL);
+        return -ENOTSUP;
+    }
 
     if (dev->intx_route.mode == intx_route.mode &&
         dev->intx_route.irq == intx_route.irq) {