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

[Peter Xu <peterx@redhat.com>]

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