From: Markus Armbruster <armbru@redhat.com>
To: Peter Xu <peterx@redhat.com>
Cc: Alex Williamson <alex.williamson@redhat.com>,
"Michael S . Tsirkin" <mst@redhat.com>,
Julia Suvorova <jusual@redhat.com>,
qemu-devel@nongnu.org,
"Dr . David Alan Gilbert" <dgilbert@redhat.com>
Subject: Re: [PATCH] pci: Display PCI IRQ pin in "info pci"
Date: Tue, 26 May 2020 10:27:34 +0200 [thread overview]
Message-ID: <87blmb6qyx.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <20200525141443.GB1058657@xz-x1> (Peter Xu's message of "Mon, 25 May 2020 10:14:43 -0400")
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.
prev parent reply other threads:[~2020-05-26 8:29 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-17 19:59 [PATCH] pci: Display PCI IRQ pin in "info pci" Peter Xu
2020-03-17 20:02 ` Eric Blake
2020-03-17 20:11 ` Peter Xu
2020-03-18 17:06 ` Philippe Mathieu-Daudé
2020-06-09 15:49 ` Michael S. Tsirkin
2020-06-09 16:13 ` Peter Xu
2020-06-09 16:18 ` Philippe Mathieu-Daudé
2020-06-09 17:53 ` Peter Xu
2020-06-09 18:09 ` Michael S. Tsirkin
2020-03-17 20:21 ` Dr. David Alan Gilbert
2020-03-17 20:36 ` Peter Xu
2020-03-18 9:23 ` Dr. David Alan Gilbert
2020-05-23 20:34 ` Peter Xu
2020-05-25 8:19 ` Markus Armbruster
2020-05-25 14:14 ` Peter Xu
2020-05-26 8:27 ` Markus Armbruster [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87blmb6qyx.fsf@dusky.pond.sub.org \
--to=armbru@redhat.com \
--cc=alex.williamson@redhat.com \
--cc=dgilbert@redhat.com \
--cc=jusual@redhat.com \
--cc=mst@redhat.com \
--cc=peterx@redhat.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).