From: "Jan Beulich" <JBeulich@suse.com>
To: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: xen-devel <xen-devel@lists.xenproject.org>, qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] [Xen-devel] [PATCH 1/6] xen/MSI-X: latch MSI-X table writes
Date: Tue, 24 Nov 2015 07:23:39 -0700 [thread overview]
Message-ID: <565480FB02000078000B875E@prv-mh.provo.novell.com> (raw)
>>> Tue, 16 Jun 2015 15:48:16 +0100 Stefano Stabellini wrote:
>On Tue, 16 Jun 2015, Jan Beulich wrote:
>> >>> On 16.06.15 at 15:35, Stefano Stabellini wrote:
>> > On Fri, 5 Jun 2015, Jan Beulich wrote:
I'm sorry for getting back to this only now.
>> >> @@ -322,6 +323,13 @@ static int xen_pt_msix_update_one(XenPCI
>> >>
>> >> pirq = entry->pirq;
>> >>
>> >> + if (pirq == XEN_PT_UNASSIGNED_PIRQ || s->msix->maskall ||
>> >> + (entry->latch(VECTOR_CTRL) & PCI_MSIX_ENTRY_CTRL_MASKBIT)) {
>> >
>> > I admit I am having difficulties understanding the full purpose of these
>> > checks. Please add a comment on them.
>>
>> The comment would (pointlessly imo) re-state what the code already
>> says:
>>
>> > I guess the intention is only to make changes using the latest values,
>> > the ones in entry->latch, when the right conditions are met, otherwise
>> > keep using the old values. Is that right?
>> >
>> > In that case, don't we want to use the latest values on MASKBIT ->
>> > !MASKBIT transitions? In general when unmasking?
>>
>> This is what we want. And with that, the questions you ask further
>> down should be answered too: The function gets invoked with the
>> pre-change mask flag state in ->latch[], and updates the values
>> used for actually setting up when that one has the entry masked
>> (or mask-all is set). The actual new value gets written to ->latch[]
>> after the call.
>
>I think this logic is counter-intuitive and prone to confuse the reader.
>This change doesn't make sense on its own: when one will read
>xen_pt_msix_update_one, won't be able to understand the function without
>checking the call sites.
>
>Could we turn it around to be more obvious? Here check if
>!(entry->latch(VECTOR_CTRL) & PCI_MSIX_ENTRY_CTRL_MASKBIT) and below only
>call xen_pt_msix_update_one on MASKBIT -> !MASKBIT transactions?
>
>Or something like that?
That would maybe be doable with just pci_msix_write() as a caller,
but not with the one in xen_pt_msix_update() (where we have no
transition of the mask bit from set to clear).
Jan
WARNING: multiple messages have this Message-ID (diff)
From: "Jan Beulich" <JBeulich@suse.com>
To: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: xen-devel <xen-devel@lists.xenproject.org>, qemu-devel@nongnu.org
Subject: Re: [PATCH 1/6] xen/MSI-X: latch MSI-X table writes
Date: Tue, 24 Nov 2015 07:23:39 -0700 [thread overview]
Message-ID: <565480FB02000078000B875E@prv-mh.provo.novell.com> (raw)
>>> Tue, 16 Jun 2015 15:48:16 +0100 Stefano Stabellini wrote:
>On Tue, 16 Jun 2015, Jan Beulich wrote:
>> >>> On 16.06.15 at 15:35, Stefano Stabellini wrote:
>> > On Fri, 5 Jun 2015, Jan Beulich wrote:
I'm sorry for getting back to this only now.
>> >> @@ -322,6 +323,13 @@ static int xen_pt_msix_update_one(XenPCI
>> >>
>> >> pirq = entry->pirq;
>> >>
>> >> + if (pirq == XEN_PT_UNASSIGNED_PIRQ || s->msix->maskall ||
>> >> + (entry->latch(VECTOR_CTRL) & PCI_MSIX_ENTRY_CTRL_MASKBIT)) {
>> >
>> > I admit I am having difficulties understanding the full purpose of these
>> > checks. Please add a comment on them.
>>
>> The comment would (pointlessly imo) re-state what the code already
>> says:
>>
>> > I guess the intention is only to make changes using the latest values,
>> > the ones in entry->latch, when the right conditions are met, otherwise
>> > keep using the old values. Is that right?
>> >
>> > In that case, don't we want to use the latest values on MASKBIT ->
>> > !MASKBIT transitions? In general when unmasking?
>>
>> This is what we want. And with that, the questions you ask further
>> down should be answered too: The function gets invoked with the
>> pre-change mask flag state in ->latch[], and updates the values
>> used for actually setting up when that one has the entry masked
>> (or mask-all is set). The actual new value gets written to ->latch[]
>> after the call.
>
>I think this logic is counter-intuitive and prone to confuse the reader.
>This change doesn't make sense on its own: when one will read
>xen_pt_msix_update_one, won't be able to understand the function without
>checking the call sites.
>
>Could we turn it around to be more obvious? Here check if
>!(entry->latch(VECTOR_CTRL) & PCI_MSIX_ENTRY_CTRL_MASKBIT) and below only
>call xen_pt_msix_update_one on MASKBIT -> !MASKBIT transactions?
>
>Or something like that?
That would maybe be doable with just pci_msix_write() as a caller,
but not with the one in xen_pt_msix_update() (where we have no
transition of the mask bit from set to clear).
Jan
next reply other threads:[~2015-11-24 14:23 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-11-24 14:23 Jan Beulich [this message]
2015-11-24 14:23 ` [PATCH 1/6] xen/MSI-X: latch MSI-X table writes Jan Beulich
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=565480FB02000078000B875E@prv-mh.provo.novell.com \
--to=jbeulich@suse.com \
--cc=qemu-devel@nongnu.org \
--cc=stefano.stabellini@eu.citrix.com \
--cc=xen-devel@lists.xenproject.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.