From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: xen-devel <xen-devel@lists.xenproject.org>,
qemu-devel@nongnu.org,
Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Subject: Re: [PATCH 1/6] xen/MSI-X: latch MSI-X table writes
Date: Tue, 16 Jun 2015 14:35:41 +0100 [thread overview]
Message-ID: <alpine.DEB.2.02.1506161215090.21829__5935.00683685324$1434461942$gmane$org@kaball.uk.xensource.com> (raw)
In-Reply-To: <5571AB41020000780008153F@mail.emea.novell.com>
On Fri, 5 Jun 2015, Jan Beulich wrote:
> The remaining log message in pci_msix_write() is wrong, as there guest
> behavior may only appear to be wrong: For one, the old logic didn't
> take the mask-all bit into account. And then this shouldn't depend on
> host device state (i.e. the host may have masked the entry without the
> guest having done so). Plus these writes shouldn't be dropped even when
> an entry gets unmasked. Instead, if they can't be made take effect
> right away, they should take effect on the next unmasking or enabling
> operation - the specification explicitly describes such caching
> behavior.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> @@ -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.
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? Here it looks like we
are using the latest values when maskall is set or
PCI_MSIX_ENTRY_CTRL_MASKBIT is set, that is the opposite of what we
want.
> + entry->addr = entry->latch(LOWER_ADDR) |
> + ((uint64_t)entry->latch(UPPER_ADDR) << 32);
> + entry->data = entry->latch(DATA);
> + }
> +
> rc = msi_msix_setup(s, entry->addr, entry->data, &pirq, true, entry_nr,
> entry->pirq == XEN_PT_UNASSIGNED_PIRQ);
> if (rc) {
> @@ -444,39 +432,28 @@ static void pci_msix_write(void *opaque,
> offset = addr % PCI_MSIX_ENTRY_SIZE;
>
> if (offset != PCI_MSIX_ENTRY_VECTOR_CTRL) {
> - const volatile uint32_t *vec_ctrl;
> -
> if (get_entry_value(entry, offset) == val
> && entry->pirq != XEN_PT_UNASSIGNED_PIRQ) {
> return;
> }
>
> + entry->updated = true;
> + } else if (msix->enabled && entry->updated &&
> + !(val & PCI_MSIX_ENTRY_CTRL_MASKBIT)) {
> + const volatile uint32_t *vec_ctrl;
> +
> /*
> * If Xen intercepts the mask bit access, entry->vec_ctrl may not be
> * up-to-date. Read from hardware directly.
> */
> vec_ctrl = s->msix->phys_iomem_base + entry_nr * PCI_MSIX_ENTRY_SIZE
> + PCI_MSIX_ENTRY_VECTOR_CTRL;
> + set_entry_value(entry, offset, *vec_ctrl);
Why are you calling set_entry_value with the hardware vec_ctrl value? It
doesn't look correct to me. In any case, if you wanted to do it,
shouldn't you just set/unset PCI_MSIX_ENTRY_CTRL_MASKBIT instead of the
whole *vec_ctrl?
> - if (msix->enabled && !(*vec_ctrl & PCI_MSIX_ENTRY_CTRL_MASKBIT)) {
> - if (!entry->warned) {
> - entry->warned = true;
> - XEN_PT_ERR(&s->dev, "Can't update msix entry %d since MSI-X is"
> - " already enabled.\n", entry_nr);
> - }
> - return;
> - }
> -
> - entry->updated = true;
> + xen_pt_msix_update_one(s, entry_nr);
Shouldn't we call xen_pt_msix_update_one only if (*vec_ctrl &
PCI_MSIX_ENTRY_CTRL_MASKBIT)? In other words, only when we see a
MASKBIT -> !MASKBIT transition?
> }
>
> set_entry_value(entry, offset, val);
> -
> - if (offset == PCI_MSIX_ENTRY_VECTOR_CTRL) {
> - if (msix->enabled && !(val & PCI_MSIX_ENTRY_CTRL_MASKBIT)) {
> - xen_pt_msix_update_one(s, entry_nr);
> - }
> - }
> }
>
> static uint64_t pci_msix_read(void *opaque, hwaddr addr,
next prev parent reply other threads:[~2015-06-16 13:37 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-06-05 11:55 [Qemu-devel] [PATCH 0/6] xen/pass-through: XSA-120, 128...131 follow-up Jan Beulich
2015-06-05 11:59 ` [Qemu-devel] [PATCH 1/6] xen/MSI-X: latch MSI-X table writes Jan Beulich
2015-06-05 11:59 ` Jan Beulich
2015-06-16 13:35 ` Stefano Stabellini [this message]
2015-06-16 13:35 ` [Qemu-devel] " Stefano Stabellini
2015-06-16 14:04 ` Jan Beulich
2015-06-16 14:04 ` [Qemu-devel] " Jan Beulich
2015-06-16 14:48 ` Stefano Stabellini
2015-06-16 14:48 ` [Qemu-devel] " Stefano Stabellini
2015-06-05 12:01 ` [Qemu-devel] [PATCH 2/6] xen/MSI-X: drive maskall and enable bits through hypercalls Jan Beulich
2015-06-05 12:01 ` Jan Beulich
2015-06-16 14:03 ` Stefano Stabellini
2015-06-16 14:03 ` [Qemu-devel] " Stefano Stabellini
2015-06-16 14:19 ` Jan Beulich
2015-06-16 14:56 ` Stefano Stabellini
2015-06-16 16:03 ` Jan Beulich
2015-06-16 16:03 ` Jan Beulich
2015-06-16 14:56 ` Stefano Stabellini
2015-06-16 14:19 ` Jan Beulich
2015-06-05 12:02 ` [Qemu-devel] [PATCH 3/6] xen/MSI-X: really enforce alignment Jan Beulich
2015-06-05 12:02 ` Jan Beulich
2015-06-16 14:08 ` Stefano Stabellini
2015-06-16 14:08 ` [Qemu-devel] " Stefano Stabellini
2015-06-05 12:03 ` [Qemu-devel] [PATCH 4/6] xen/pass-through: correctly deal with RW1C bits Jan Beulich
2015-06-05 12:03 ` Jan Beulich
2015-06-16 14:19 ` Stefano Stabellini
2015-06-16 14:19 ` [Qemu-devel] " Stefano Stabellini
2015-06-16 14:38 ` Jan Beulich
2015-06-16 14:38 ` Jan Beulich
2015-06-05 12:04 ` [Qemu-devel] [PATCH 5/6] xen/pass-through: log errno values rather than function return ones Jan Beulich
2015-06-05 12:04 ` Jan Beulich
2015-06-16 14:23 ` Stefano Stabellini
2015-06-16 14:23 ` [Qemu-devel] " Stefano Stabellini
2015-06-05 12:04 ` [Qemu-devel] [PATCH 6/6] xen/pass-through: constify some static data Jan Beulich
2015-06-05 12:04 ` Jan Beulich
2015-06-16 14:27 ` Stefano Stabellini
2015-06-16 14:27 ` [Qemu-devel] " Stefano Stabellini
2015-06-16 14:41 ` Jan Beulich
2015-06-16 14:43 ` Stefano Stabellini
2015-06-16 14:43 ` [Qemu-devel] " Stefano Stabellini
2015-06-16 14:41 ` Jan Beulich
-- strict thread matches above, loose matches on Subject: below --
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='alpine.DEB.2.02.1506161215090.21829__5935.00683685324$1434461942$gmane$org@kaball.uk.xensource.com' \
--to=stefano.stabellini@eu.citrix.com \
--cc=JBeulich@suse.com \
--cc=qemu-devel@nongnu.org \
--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.