[PATCH] pci: Display PCI IRQ pin in "info pci"

[PATCH] pci: Display PCI IRQ pin in "info pci"

[Peter Xu]

Sometimes it would be good to be able to read the pin number along
with the IRQ number allocated.  Since we'll dump the IRQ number, no
reason to not dump the pin information.  For example, the vfio-pci
device will overwrite the pin with the hardware pin number.  It would
be nice to know the pin number of one assigned device from QMP/HMP.

CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
CC: Alex Williamson <alex.williamson@redhat.com>
CC: Michael S. Tsirkin <mst@redhat.com>
CC: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
CC: Julia Suvorova <jusual@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---

This helped me to debug an IRQ sharing issue, so may good to have it
in master too.
---
 hw/pci/pci.c       | 1 +
 monitor/hmp-cmds.c | 3 ++-
 qapi/misc.json     | 6 ++++--
 3 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index e1ed6677e1..7ebf532ac9 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1769,6 +1769,7 @@ static PciDeviceInfo *qmp_query_pci_device(PCIDevice *dev, PCIBus *bus,
     info->regions = qmp_query_pci_regions(dev);
     info->qdev_id = g_strdup(dev->qdev.id ? dev->qdev.id : "");
 
+    info->irq_pin = dev->config[PCI_INTERRUPT_PIN];
     if (dev->config[PCI_INTERRUPT_PIN] != 0) {
         info->has_irq = true;
         info->irq = dev->config[PCI_INTERRUPT_LINE];
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 58724031ea..04c86bbb03 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -680,7 +680,8 @@ static void hmp_info_pci_device(Monitor *mon, const PciDeviceInfo *dev)
     }
 
     if (dev->has_irq) {
-        monitor_printf(mon, "      IRQ %" PRId64 ".\n", dev->irq);
+        monitor_printf(mon, "      IRQ %" PRId64 ", pin %c\n",
+                       dev->irq, (char)('A' + dev->irq_pin - 1));
     }
 
     if (dev->has_pci_bridge) {
diff --git a/qapi/misc.json b/qapi/misc.json
index c18fe681fb..f8d33ddb4e 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -403,6 +403,8 @@
 #
 # @irq: if an IRQ is assigned to the device, the IRQ number
 #
+# @irq_pin: the IRQ pin, zero means no IRQ (since 5.1)
+#
 # @qdev_id: the device name of the PCI device
 #
 # @pci_bridge: if the device is a PCI bridge, the bridge information
@@ -417,8 +419,8 @@
 { 'struct': 'PciDeviceInfo',
   'data': {'bus': 'int', 'slot': 'int', 'function': 'int',
            'class_info': 'PciDeviceClass', 'id': 'PciDeviceId',
-           '*irq': 'int', 'qdev_id': 'str', '*pci_bridge': 'PciBridgeInfo',
-           'regions': ['PciMemoryRegion']} }
+           '*irq': 'int', 'irq_pin': 'int', 'qdev_id': 'str',
+           '*pci_bridge': 'PciBridgeInfo', 'regions': ['PciMemoryRegion'] }}
 
 ##
 # @PciInfo:
-- 
2.24.1

Re: [PATCH] pci: Display PCI IRQ pin in "info pci"

[Eric Blake]

On 3/17/20 2:59 PM, Peter Xu wrote:
> Sometimes it would be good to be able to read the pin number along
> with the IRQ number allocated.  Since we'll dump the IRQ number, no
> reason to not dump the pin information.  For example, the vfio-pci
> device will overwrite the pin with the hardware pin number.  It would
> be nice to know the pin number of one assigned device from QMP/HMP.
> 
> CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
> CC: Alex Williamson <alex.williamson@redhat.com>
> CC: Michael S. Tsirkin <mst@redhat.com>
> CC: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> CC: Julia Suvorova <jusual@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> 
> This helped me to debug an IRQ sharing issue, so may good to have it
> in master too.

We're right at soft freeze.


> +++ b/qapi/misc.json
> @@ -403,6 +403,8 @@
>   #
>   # @irq: if an IRQ is assigned to the device, the IRQ number
>   #
> +# @irq_pin: the IRQ pin, zero means no IRQ (since 5.1)
> +#

But if we really do want it in 5.0, this needs a tweak.  Does the delay 
hurt?

Naming convention: We prefer 'irq-pin' unless there is a consistency 
issue....

>   # @qdev_id: the device name of the PCI device
>   #
>   # @pci_bridge: if the device is a PCI bridge, the bridge information
> @@ -417,8 +419,8 @@
>   { 'struct': 'PciDeviceInfo',
>     'data': {'bus': 'int', 'slot': 'int', 'function': 'int',
>              'class_info': 'PciDeviceClass', 'id': 'PciDeviceId',
> -           '*irq': 'int', 'qdev_id': 'str', '*pci_bridge': 'PciBridgeInfo',
> -           'regions': ['PciMemoryRegion']} }
> +           '*irq': 'int', 'irq_pin': 'int', 'qdev_id': 'str',
> +           '*pci_bridge': 'PciBridgeInfo', 'regions': ['PciMemoryRegion'] }}

and the pre-existing pci_bridge is indeed the consistency issue.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH] pci: Display PCI IRQ pin in "info pci"

[Peter Xu]

On Tue, Mar 17, 2020 at 03:02:57PM -0500, Eric Blake wrote:
> On 3/17/20 2:59 PM, Peter Xu wrote:
> > Sometimes it would be good to be able to read the pin number along
> > with the IRQ number allocated.  Since we'll dump the IRQ number, no
> > reason to not dump the pin information.  For example, the vfio-pci
> > device will overwrite the pin with the hardware pin number.  It would
> > be nice to know the pin number of one assigned device from QMP/HMP.
> > 
> > CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > CC: Alex Williamson <alex.williamson@redhat.com>
> > CC: Michael S. Tsirkin <mst@redhat.com>
> > CC: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> > CC: Julia Suvorova <jusual@redhat.com>
> > CC: Markus Armbruster <armbru@redhat.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> > 
> > This helped me to debug an IRQ sharing issue, so may good to have it
> > in master too.
> 
> We're right at soft freeze.
> 
> 
> > +++ b/qapi/misc.json
> > @@ -403,6 +403,8 @@
> >   #
> >   # @irq: if an IRQ is assigned to the device, the IRQ number
> >   #
> > +# @irq_pin: the IRQ pin, zero means no IRQ (since 5.1)
> > +#
> 
> But if we really do want it in 5.0, this needs a tweak.  Does the delay
> hurt?

No it won't; it's something good to have only.  I'll let the
maintainers to decide...

> 
> Naming convention: We prefer 'irq-pin' unless there is a consistency
> issue....
> 
> >   # @qdev_id: the device name of the PCI device
> >   #
> >   # @pci_bridge: if the device is a PCI bridge, the bridge information
> > @@ -417,8 +419,8 @@
> >   { 'struct': 'PciDeviceInfo',
> >     'data': {'bus': 'int', 'slot': 'int', 'function': 'int',
> >              'class_info': 'PciDeviceClass', 'id': 'PciDeviceId',
> > -           '*irq': 'int', 'qdev_id': 'str', '*pci_bridge': 'PciBridgeInfo',
> > -           'regions': ['PciMemoryRegion']} }
> > +           '*irq': 'int', 'irq_pin': 'int', 'qdev_id': 'str',
> > +           '*pci_bridge': 'PciBridgeInfo', 'regions': ['PciMemoryRegion'] }}
> 
> and the pre-existing pci_bridge is indeed the consistency issue.

Yeh, actually every key in this struct. :)

Thanks,

-- 
Peter Xu

Re: [PATCH] pci: Display PCI IRQ pin in "info pci"

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> Sometimes it would be good to be able to read the pin number along
> with the IRQ number allocated.  Since we'll dump the IRQ number, no
> reason to not dump the pin information.  For example, the vfio-pci
> device will overwrite the pin with the hardware pin number.  It would
> be nice to know the pin number of one assigned device from QMP/HMP.
> 
> CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
> CC: Alex Williamson <alex.williamson@redhat.com>
> CC: Michael S. Tsirkin <mst@redhat.com>
> CC: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> CC: Julia Suvorova <jusual@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> 
> This helped me to debug an IRQ sharing issue, so may good to have it
> in master too.
> ---
>  hw/pci/pci.c       | 1 +
>  monitor/hmp-cmds.c | 3 ++-
>  qapi/misc.json     | 6 ++++--
>  3 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index e1ed6677e1..7ebf532ac9 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -1769,6 +1769,7 @@ static PciDeviceInfo *qmp_query_pci_device(PCIDevice *dev, PCIBus *bus,
>      info->regions = qmp_query_pci_regions(dev);
>      info->qdev_id = g_strdup(dev->qdev.id ? dev->qdev.id : "");
>  
> +    info->irq_pin = dev->config[PCI_INTERRUPT_PIN];
>      if (dev->config[PCI_INTERRUPT_PIN] != 0) {
>          info->has_irq = true;
>          info->irq = dev->config[PCI_INTERRUPT_LINE];
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index 58724031ea..04c86bbb03 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -680,7 +680,8 @@ static void hmp_info_pci_device(Monitor *mon, const PciDeviceInfo *dev)
>      }
>  
>      if (dev->has_irq) {
> -        monitor_printf(mon, "      IRQ %" PRId64 ".\n", dev->irq);
> +        monitor_printf(mon, "      IRQ %" PRId64 ", pin %c\n",
> +                       dev->irq, (char)('A' + dev->irq_pin - 1));

Can we trust dev->irq_pin not to be something silly and generate a
non-printable?

Dave

>      }
>  
>      if (dev->has_pci_bridge) {
> diff --git a/qapi/misc.json b/qapi/misc.json
> index c18fe681fb..f8d33ddb4e 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -403,6 +403,8 @@
>  #
>  # @irq: if an IRQ is assigned to the device, the IRQ number
>  #
> +# @irq_pin: the IRQ pin, zero means no IRQ (since 5.1)
> +#
>  # @qdev_id: the device name of the PCI device
>  #
>  # @pci_bridge: if the device is a PCI bridge, the bridge information
> @@ -417,8 +419,8 @@
>  { 'struct': 'PciDeviceInfo',
>    'data': {'bus': 'int', 'slot': 'int', 'function': 'int',
>             'class_info': 'PciDeviceClass', 'id': 'PciDeviceId',
> -           '*irq': 'int', 'qdev_id': 'str', '*pci_bridge': 'PciBridgeInfo',
> -           'regions': ['PciMemoryRegion']} }
> +           '*irq': 'int', 'irq_pin': 'int', 'qdev_id': 'str',
> +           '*pci_bridge': 'PciBridgeInfo', 'regions': ['PciMemoryRegion'] }}
>  
>  ##
>  # @PciInfo:
> -- 
> 2.24.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH] pci: Display PCI IRQ pin in "info pci"

[Peter Xu]

On Tue, Mar 17, 2020 at 08:21:17PM +0000, Dr. David Alan Gilbert wrote:
> * Peter Xu (peterx@redhat.com) wrote:
> > Sometimes it would be good to be able to read the pin number along
> > with the IRQ number allocated.  Since we'll dump the IRQ number, no
> > reason to not dump the pin information.  For example, the vfio-pci
> > device will overwrite the pin with the hardware pin number.  It would
> > be nice to know the pin number of one assigned device from QMP/HMP.
> > 
> > CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > CC: Alex Williamson <alex.williamson@redhat.com>
> > CC: Michael S. Tsirkin <mst@redhat.com>
> > CC: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> > CC: Julia Suvorova <jusual@redhat.com>
> > CC: Markus Armbruster <armbru@redhat.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> > 
> > This helped me to debug an IRQ sharing issue, so may good to have it
> > in master too.
> > ---
> >  hw/pci/pci.c       | 1 +
> >  monitor/hmp-cmds.c | 3 ++-
> >  qapi/misc.json     | 6 ++++--
> >  3 files changed, 7 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index e1ed6677e1..7ebf532ac9 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -1769,6 +1769,7 @@ static PciDeviceInfo *qmp_query_pci_device(PCIDevice *dev, PCIBus *bus,
> >      info->regions = qmp_query_pci_regions(dev);
> >      info->qdev_id = g_strdup(dev->qdev.id ? dev->qdev.id : "");
> >  
> > +    info->irq_pin = dev->config[PCI_INTERRUPT_PIN];
> >      if (dev->config[PCI_INTERRUPT_PIN] != 0) {
> >          info->has_irq = true;
> >          info->irq = dev->config[PCI_INTERRUPT_LINE];
> > diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> > index 58724031ea..04c86bbb03 100644
> > --- a/monitor/hmp-cmds.c
> > +++ b/monitor/hmp-cmds.c
> > @@ -680,7 +680,8 @@ static void hmp_info_pci_device(Monitor *mon, const PciDeviceInfo *dev)
> >      }
> >  
> >      if (dev->has_irq) {
> > -        monitor_printf(mon, "      IRQ %" PRId64 ".\n", dev->irq);
> > +        monitor_printf(mon, "      IRQ %" PRId64 ", pin %c\n",
> > +                       dev->irq, (char)('A' + dev->irq_pin - 1));
> 
> Can we trust dev->irq_pin not to be something silly and generate a
> non-printable?

It should be strictly a value between 1-4 corresponds to irq pin A-D
(note the has_irq check masked out the zero value, or it could be
0-4), so I think it should always make sense. Otherwise I see it a
qemu bug somewhere...

Thanks,

-- 
Peter Xu

Re: [PATCH] pci: Display PCI IRQ pin in "info pci"

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> On Tue, Mar 17, 2020 at 08:21:17PM +0000, Dr. David Alan Gilbert wrote:
> > * Peter Xu (peterx@redhat.com) wrote:
> > > Sometimes it would be good to be able to read the pin number along
> > > with the IRQ number allocated.  Since we'll dump the IRQ number, no
> > > reason to not dump the pin information.  For example, the vfio-pci
> > > device will overwrite the pin with the hardware pin number.  It would
> > > be nice to know the pin number of one assigned device from QMP/HMP.
> > > 
> > > CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > CC: Alex Williamson <alex.williamson@redhat.com>
> > > CC: Michael S. Tsirkin <mst@redhat.com>
> > > CC: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> > > CC: Julia Suvorova <jusual@redhat.com>
> > > CC: Markus Armbruster <armbru@redhat.com>
> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > ---
> > > 
> > > This helped me to debug an IRQ sharing issue, so may good to have it
> > > in master too.
> > > ---
> > >  hw/pci/pci.c       | 1 +
> > >  monitor/hmp-cmds.c | 3 ++-
> > >  qapi/misc.json     | 6 ++++--
> > >  3 files changed, 7 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > index e1ed6677e1..7ebf532ac9 100644
> > > --- a/hw/pci/pci.c
> > > +++ b/hw/pci/pci.c
> > > @@ -1769,6 +1769,7 @@ static PciDeviceInfo *qmp_query_pci_device(PCIDevice *dev, PCIBus *bus,
> > >      info->regions = qmp_query_pci_regions(dev);
> > >      info->qdev_id = g_strdup(dev->qdev.id ? dev->qdev.id : "");
> > >  
> > > +    info->irq_pin = dev->config[PCI_INTERRUPT_PIN];
> > >      if (dev->config[PCI_INTERRUPT_PIN] != 0) {
> > >          info->has_irq = true;
> > >          info->irq = dev->config[PCI_INTERRUPT_LINE];
> > > diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> > > index 58724031ea..04c86bbb03 100644
> > > --- a/monitor/hmp-cmds.c
> > > +++ b/monitor/hmp-cmds.c
> > > @@ -680,7 +680,8 @@ static void hmp_info_pci_device(Monitor *mon, const PciDeviceInfo *dev)
> > >      }
> > >  
> > >      if (dev->has_irq) {
> > > -        monitor_printf(mon, "      IRQ %" PRId64 ".\n", dev->irq);
> > > +        monitor_printf(mon, "      IRQ %" PRId64 ", pin %c\n",
> > > +                       dev->irq, (char)('A' + dev->irq_pin - 1));
> > 
> > Can we trust dev->irq_pin not to be something silly and generate a
> > non-printable?
> 
> It should be strictly a value between 1-4 corresponds to irq pin A-D
> (note the has_irq check masked out the zero value, or it could be
> 0-4), so I think it should always make sense. Otherwise I see it a
> qemu bug somewhere...

OK, then 

Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

for HMP.

Dave


> Thanks,
> 
> -- 
> Peter Xu
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH] pci: Display PCI IRQ pin in "info pci"

[Philippe Mathieu-Daudé]

On 3/17/20 9:11 PM, Peter Xu wrote:
> On Tue, Mar 17, 2020 at 03:02:57PM -0500, Eric Blake wrote:
>> On 3/17/20 2:59 PM, Peter Xu wrote:
>>> Sometimes it would be good to be able to read the pin number along
>>> with the IRQ number allocated.  Since we'll dump the IRQ number, no
>>> reason to not dump the pin information.  For example, the vfio-pci
>>> device will overwrite the pin with the hardware pin number.  It would
>>> be nice to know the pin number of one assigned device from QMP/HMP.
>>>
>>> CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>> CC: Alex Williamson <alex.williamson@redhat.com>
>>> CC: Michael S. Tsirkin <mst@redhat.com>
>>> CC: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
>>> CC: Julia Suvorova <jusual@redhat.com>
>>> CC: Markus Armbruster <armbru@redhat.com>
>>> Signed-off-by: Peter Xu <peterx@redhat.com>
>>> ---
>>>
>>> This helped me to debug an IRQ sharing issue, so may good to have it
>>> in master too.
>>
>> We're right at soft freeze.
>>
>>
>>> +++ b/qapi/misc.json
>>> @@ -403,6 +403,8 @@
>>>    #
>>>    # @irq: if an IRQ is assigned to the device, the IRQ number
>>>    #
>>> +# @irq_pin: the IRQ pin, zero means no IRQ (since 5.1)
>>> +#
>>
>> But if we really do want it in 5.0, this needs a tweak.  Does the delay
>> hurt?
> 
> No it won't; it's something good to have only.  I'll let the
> maintainers to decide...
> 
>>
>> Naming convention: We prefer  unless there is a consistency
>> issue....
>>
>>>    # @qdev_id: the device name of the PCI device
>>>    #
>>>    # @pci_bridge: if the device is a PCI bridge, the bridge information
>>> @@ -417,8 +419,8 @@
>>>    { 'struct': 'PciDeviceInfo',
>>>      'data': {'bus': 'int', 'slot': 'int', 'function': 'int',
>>>               'class_info': 'PciDeviceClass', 'id': 'PciDeviceId',
>>> -           '*irq': 'int', 'qdev_id': 'str', '*pci_bridge': 'PciBridgeInfo',
>>> -           'regions': ['PciMemoryRegion']} }
>>> +           '*irq': 'int', 'irq_pin': 'int', 'qdev_id': 'str',
>>> +           '*pci_bridge': 'PciBridgeInfo', 'regions': ['PciMemoryRegion'] }}
>>
>> and the pre-existing pci_bridge is indeed the consistency issue.
> 
> Yeh, actually every key in this struct. :)

Using 'irq-pin':
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> 
> Thanks,
> 

Re: [PATCH] pci: Display PCI IRQ pin in "info pci"

[Peter Xu]

On Tue, Mar 17, 2020 at 03:59:08PM -0400, Peter Xu wrote:
> Sometimes it would be good to be able to read the pin number along
> with the IRQ number allocated.  Since we'll dump the IRQ number, no
> reason to not dump the pin information.  For example, the vfio-pci
> device will overwrite the pin with the hardware pin number.  It would
> be nice to know the pin number of one assigned device from QMP/HMP.
> 
> CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
> CC: Alex Williamson <alex.williamson@redhat.com>
> CC: Michael S. Tsirkin <mst@redhat.com>
> CC: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> CC: Julia Suvorova <jusual@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Ping?

-- 
Peter Xu

Re: [PATCH] pci: Display PCI IRQ pin in "info pci"

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> Sometimes it would be good to be able to read the pin number along
> with the IRQ number allocated.  Since we'll dump the IRQ number, no
> reason to not dump the pin information.  For example, the vfio-pci
> device will overwrite the pin with the hardware pin number.  It would
> be nice to know the pin number of one assigned device from QMP/HMP.
>
> CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
> CC: Alex Williamson <alex.williamson@redhat.com>
> CC: Michael S. Tsirkin <mst@redhat.com>
> CC: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> CC: Julia Suvorova <jusual@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>
> This helped me to debug an IRQ sharing issue, so may good to have it
> in master too.
> ---
[...]
> diff --git a/qapi/misc.json b/qapi/misc.json
> index c18fe681fb..f8d33ddb4e 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -403,6 +403,8 @@
>  #
>  # @irq: if an IRQ is assigned to the device, the IRQ number
>  #
> +# @irq_pin: the IRQ pin, zero means no IRQ (since 5.1)
> +#

For the IRQ number, we make the member optional, and use "member absent"
for "no IRQ assigned".

For the IRQ pin, we make the member mandatory, and use zero value for
"no IRQ assigned".

Any particular reason for the difference?

>  # @qdev_id: the device name of the PCI device
>  #
>  # @pci_bridge: if the device is a PCI bridge, the bridge information
> @@ -417,8 +419,8 @@
>  { 'struct': 'PciDeviceInfo',
>    'data': {'bus': 'int', 'slot': 'int', 'function': 'int',
>             'class_info': 'PciDeviceClass', 'id': 'PciDeviceId',
> -           '*irq': 'int', 'qdev_id': 'str', '*pci_bridge': 'PciBridgeInfo',
> -           'regions': ['PciMemoryRegion']} }
> +           '*irq': 'int', 'irq_pin': 'int', 'qdev_id': 'str',
> +           '*pci_bridge': 'PciBridgeInfo', 'regions': ['PciMemoryRegion'] }}
>  
>  ##
>  # @PciInfo:

Re: [PATCH] pci: Display PCI IRQ pin in "info pci"

[Peter Xu]

Hi, Markus,

Thanks for commenting.  Please see below.

On Mon, May 25, 2020 at 10:19:09AM +0200, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > Sometimes it would be good to be able to read the pin number along
> > with the IRQ number allocated.  Since we'll dump the IRQ number, no
> > reason to not dump the pin information.  For example, the vfio-pci
> > device will overwrite the pin with the hardware pin number.  It would
> > be nice to know the pin number of one assigned device from QMP/HMP.
> >
> > CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > CC: Alex Williamson <alex.williamson@redhat.com>
> > CC: Michael S. Tsirkin <mst@redhat.com>
> > CC: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> > CC: Julia Suvorova <jusual@redhat.com>
> > CC: Markus Armbruster <armbru@redhat.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >
> > This helped me to debug an IRQ sharing issue, so may good to have it
> > in master too.
> > ---
> [...]
> > diff --git a/qapi/misc.json b/qapi/misc.json
> > index c18fe681fb..f8d33ddb4e 100644
> > --- a/qapi/misc.json
> > +++ b/qapi/misc.json
> > @@ -403,6 +403,8 @@
> >  #
> >  # @irq: if an IRQ is assigned to the device, the IRQ number
> >  #
> > +# @irq_pin: the IRQ pin, zero means no IRQ (since 5.1)
> > +#
> 
> For the IRQ number, we make the member optional, and use "member absent"
> for "no IRQ assigned".
> 
> For the IRQ pin, we make the member mandatory, and use zero value for
> "no IRQ assigned".
> 
> Any particular reason for the difference?

I have two reasons.

Spec-wise, "irq" (PCI_INTERRUPT_LINE) is optional which should depend on
"irq_pin" (PCI_INTERRUPT_PIN), while "irq_pin" itself is not optional according
to PCI local bus spec 3.0:

        Interrupt Pin

        The Interrupt Pin register tells which interrupt pin the device (or
        device function) uses. A value of 1 corresponds to INTA# . A value of 2
        corresponds to INTB# . A value of 3 corresponds to INTC# . A value of 4
        corresponds to INTD# . Devices (or device functions) that do not use an
        interrupt pin must put a 0 in this register. The values 05h through FFh
        are reserved. This register is read-only. Refer to Section 2.2.6 for
        further description of the usage of the INTx# pins.

So it should be a value between 0-4, inclusive.  It applies even if the device
does not support INTx.

Code-wise, we can also make "irq_pin" optional just like "irq" (which will
automatically create the has_irq_pin variable).  However, then it means we'll
have two booleans for the same purpose for intx in PciDeviceInfo, which seems
to be an unnecessary duplication.

So I chose the simple to make it mandatory.

Thanks,

-- 
Peter Xu

Re: [PATCH] pci: Display PCI IRQ pin in "info pci"

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> Hi, Markus,
>
> Thanks for commenting.  Please see below.
>
> On Mon, May 25, 2020 at 10:19:09AM +0200, Markus Armbruster wrote:
>> Peter Xu <peterx@redhat.com> writes:
>> 
>> > Sometimes it would be good to be able to read the pin number along
>> > with the IRQ number allocated.  Since we'll dump the IRQ number, no
>> > reason to not dump the pin information.  For example, the vfio-pci
>> > device will overwrite the pin with the hardware pin number.  It would
>> > be nice to know the pin number of one assigned device from QMP/HMP.
>> >
>> > CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> > CC: Alex Williamson <alex.williamson@redhat.com>
>> > CC: Michael S. Tsirkin <mst@redhat.com>
>> > CC: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
>> > CC: Julia Suvorova <jusual@redhat.com>
>> > CC: Markus Armbruster <armbru@redhat.com>
>> > Signed-off-by: Peter Xu <peterx@redhat.com>
>> > ---
>> >
>> > This helped me to debug an IRQ sharing issue, so may good to have it
>> > in master too.
>> > ---
>> [...]
>> > diff --git a/qapi/misc.json b/qapi/misc.json
>> > index c18fe681fb..f8d33ddb4e 100644
>> > --- a/qapi/misc.json
>> > +++ b/qapi/misc.json
>> > @@ -403,6 +403,8 @@
>> >  #
>> >  # @irq: if an IRQ is assigned to the device, the IRQ number
>> >  #
>> > +# @irq_pin: the IRQ pin, zero means no IRQ (since 5.1)
>> > +#
>> 
>> For the IRQ number, we make the member optional, and use "member absent"
>> for "no IRQ assigned".
>> 
>> For the IRQ pin, we make the member mandatory, and use zero value for
>> "no IRQ assigned".
>> 
>> Any particular reason for the difference?
>
> I have two reasons.
>
> Spec-wise, "irq" (PCI_INTERRUPT_LINE) is optional which should depend on
> "irq_pin" (PCI_INTERRUPT_PIN), while "irq_pin" itself is not optional according
> to PCI local bus spec 3.0:
>
>         Interrupt Pin
>
>         The Interrupt Pin register tells which interrupt pin the device (or
>         device function) uses. A value of 1 corresponds to INTA# . A value of 2
>         corresponds to INTB# . A value of 3 corresponds to INTC# . A value of 4
>         corresponds to INTD# . Devices (or device functions) that do not use an
>         interrupt pin must put a 0 in this register. The values 05h through FFh
>         are reserved. This register is read-only. Refer to Section 2.2.6 for
>         further description of the usage of the INTx# pins.
>
> So it should be a value between 0-4, inclusive.  It applies even if the device
> does not support INTx.
>
> Code-wise, we can also make "irq_pin" optional just like "irq" (which will
> automatically create the has_irq_pin variable).  However, then it means we'll
> have two booleans for the same purpose for intx in PciDeviceInfo, which seems
> to be an unnecessary duplication.

I agree encoding "uses an interrupt pin" in multiple seemingly
independent ways is somewhat ugly.  But making @irq_pin mandatory merely
hides it better: if (has_irq_pin) becomes if (irq_pin).  Same redundancy
with if (has_irq).

> So I chose the simple to make it mandatory.

No big deal either way, thus
Acked-by: Markus Armbruster <armbru@redhat.com>

I'm *not* passing judgement on whether exposing the pin register via QMP
makes sense.  That's for the PCI maintainers to decide.

Re: [PATCH] pci: Display PCI IRQ pin in "info pci"

[Michael S. Tsirkin]

On Wed, Mar 18, 2020 at 06:06:02PM +0100, Philippe Mathieu-Daudé wrote:
> On 3/17/20 9:11 PM, Peter Xu wrote:
> > On Tue, Mar 17, 2020 at 03:02:57PM -0500, Eric Blake wrote:
> > > On 3/17/20 2:59 PM, Peter Xu wrote:
> > > > Sometimes it would be good to be able to read the pin number along
> > > > with the IRQ number allocated.  Since we'll dump the IRQ number, no
> > > > reason to not dump the pin information.  For example, the vfio-pci
> > > > device will overwrite the pin with the hardware pin number.  It would
> > > > be nice to know the pin number of one assigned device from QMP/HMP.
> > > > 
> > > > CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > > CC: Alex Williamson <alex.williamson@redhat.com>
> > > > CC: Michael S. Tsirkin <mst@redhat.com>
> > > > CC: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> > > > CC: Julia Suvorova <jusual@redhat.com>
> > > > CC: Markus Armbruster <armbru@redhat.com>
> > > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > > ---
> > > > 
> > > > This helped me to debug an IRQ sharing issue, so may good to have it
> > > > in master too.
> > > 
> > > We're right at soft freeze.
> > > 
> > > 
> > > > +++ b/qapi/misc.json
> > > > @@ -403,6 +403,8 @@
> > > >    #
> > > >    # @irq: if an IRQ is assigned to the device, the IRQ number
> > > >    #
> > > > +# @irq_pin: the IRQ pin, zero means no IRQ (since 5.1)
> > > > +#
> > > 
> > > But if we really do want it in 5.0, this needs a tweak.  Does the delay
> > > hurt?
> > 
> > No it won't; it's something good to have only.  I'll let the
> > maintainers to decide...
> > 
> > > 
> > > Naming convention: We prefer  unless there is a consistency
> > > issue....
> > > 
> > > >    # @qdev_id: the device name of the PCI device
> > > >    #
> > > >    # @pci_bridge: if the device is a PCI bridge, the bridge information
> > > > @@ -417,8 +419,8 @@
> > > >    { 'struct': 'PciDeviceInfo',
> > > >      'data': {'bus': 'int', 'slot': 'int', 'function': 'int',
> > > >               'class_info': 'PciDeviceClass', 'id': 'PciDeviceId',
> > > > -           '*irq': 'int', 'qdev_id': 'str', '*pci_bridge': 'PciBridgeInfo',
> > > > -           'regions': ['PciMemoryRegion']} }
> > > > +           '*irq': 'int', 'irq_pin': 'int', 'qdev_id': 'str',
> > > > +           '*pci_bridge': 'PciBridgeInfo', 'regions': ['PciMemoryRegion'] }}
> > > 
> > > and the pre-existing pci_bridge is indeed the consistency issue.
> > 
> > Yeh, actually every key in this struct. :)
> 
> Using 'irq-pin':
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>


Peter can you fix and repost pls?

> > 
> > Thanks,
> > 

Re: [PATCH] pci: Display PCI IRQ pin in "info pci"

[Peter Xu]

On Tue, Jun 09, 2020 at 11:49:49AM -0400, Michael S. Tsirkin wrote:
> > > > > @@ -417,8 +419,8 @@
> > > > >    { 'struct': 'PciDeviceInfo',
> > > > >      'data': {'bus': 'int', 'slot': 'int', 'function': 'int',
> > > > >               'class_info': 'PciDeviceClass', 'id': 'PciDeviceId',
> > > > > -           '*irq': 'int', 'qdev_id': 'str', '*pci_bridge': 'PciBridgeInfo',
> > > > > -           'regions': ['PciMemoryRegion']} }
> > > > > +           '*irq': 'int', 'irq_pin': 'int', 'qdev_id': 'str',
> > > > > +           '*pci_bridge': 'PciBridgeInfo', 'regions': ['PciMemoryRegion'] }}
> > > > 
> > > > and the pre-existing pci_bridge is indeed the consistency issue.
> > > 
> > > Yeh, actually every key in this struct. :)
> > 
> > Using 'irq-pin':
> > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> 
> Peter can you fix and repost pls?

I still think irq_pin is better because as I mentioned previously all the
existing keys are using underscores...  Phil?

Thanks,

-- 
Peter Xu

Re: [PATCH] pci: Display PCI IRQ pin in "info pci"

[Philippe Mathieu-Daudé]

On 6/9/20 6:13 PM, Peter Xu wrote:
> On Tue, Jun 09, 2020 at 11:49:49AM -0400, Michael S. Tsirkin wrote:
>>>>>> @@ -417,8 +419,8 @@
>>>>>>    { 'struct': 'PciDeviceInfo',
>>>>>>      'data': {'bus': 'int', 'slot': 'int', 'function': 'int',
>>>>>>               'class_info': 'PciDeviceClass', 'id': 'PciDeviceId',
>>>>>> -           '*irq': 'int', 'qdev_id': 'str', '*pci_bridge': 'PciBridgeInfo',
>>>>>> -           'regions': ['PciMemoryRegion']} }
>>>>>> +           '*irq': 'int', 'irq_pin': 'int', 'qdev_id': 'str',
>>>>>> +           '*pci_bridge': 'PciBridgeInfo', 'regions': ['PciMemoryRegion'] }}
>>>>>
>>>>> and the pre-existing pci_bridge is indeed the consistency issue.
>>>>
>>>> Yeh, actually every key in this struct. :)
>>>
>>> Using 'irq-pin':
>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>
>>
>> Peter can you fix and repost pls?
> 
> I still think irq_pin is better because as I mentioned previously all the
> existing keys are using underscores...  Phil?

No problem!

Re: [PATCH] pci: Display PCI IRQ pin in "info pci"

[Peter Xu]

On Tue, Jun 09, 2020 at 06:18:15PM +0200, Philippe Mathieu-Daudé wrote:
> No problem!

Thanks!

-- 
Peter Xu

Re: [PATCH] pci: Display PCI IRQ pin in "info pci"

[Michael S. Tsirkin]

On Tue, Jun 09, 2020 at 01:53:55PM -0400, Peter Xu wrote:
> On Tue, Jun 09, 2020 at 06:18:15PM +0200, Philippe Mathieu-Daudé wrote:
> > No problem!
> 
> Thanks!

OK so any chance of an ack?

> -- 
> Peter Xu