[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] pci: Display PCI IRQ pin in "info pci"
From: |
Markus Armbruster |
Subject: |
Re: [PATCH] pci: Display PCI IRQ pin in "info pci" |
Date: |
Tue, 26 May 2020 10:27:34 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) |
Peter Xu <address@hidden> writes:
> Hi, Markus,
>
> Thanks for commenting. Please see below.
>
> On Mon, May 25, 2020 at 10:19:09AM +0200, Markus Armbruster wrote:
>> Peter Xu <address@hidden> 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 <address@hidden>
>> > CC: Alex Williamson <address@hidden>
>> > CC: Michael S. Tsirkin <address@hidden>
>> > CC: Marcel Apfelbaum <address@hidden>
>> > CC: Julia Suvorova <address@hidden>
>> > CC: Markus Armbruster <address@hidden>
>> > Signed-off-by: Peter Xu <address@hidden>
>> > ---
>> >
>> > 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 <address@hidden>
I'm *not* passing judgement on whether exposing the pin register via QMP
makes sense. That's for the PCI maintainers to decide.