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

[Markus Armbruster <armbru@redhat.com>]

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.