From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefano Stabellini Subject: Re: [PATCH 1/6] xen/MSI-X: latch MSI-X table writes Date: Tue, 16 Jun 2015 15:48:16 +0100 Message-ID: References: <5571AA3B020000780008152E@mail.emea.novell.com> <5571AB41020000780008153F@mail.emea.novell.com> <5580490D02000078000858C8@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1Z4sei-0005WJ-4s for xen-devel@lists.xenproject.org; Tue, 16 Jun 2015 15:20:04 +0000 In-Reply-To: <5580490D02000078000858C8@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: xen-devel , qemu-devel@nongnu.org, Stefano Stabellini List-Id: xen-devel@lists.xenproject.org On Tue, 16 Jun 2015, Jan Beulich wrote: > >>> On 16.06.15 at 15:35, wrote: > > On Fri, 5 Jun 2015, Jan Beulich wrote: > >> @@ -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? > >> @@ -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? > > The comment above the code explains it: What we have stored locally > may not reflect reality, as we may not have seen all writes (and this > indeed isn't just a "may"). And if out cached value isn't valid anymore, > why would we not want to update all of it, rather than just the mask > bit? OK, however the previous code wasn't actually updating the entirety of vector_ctrl. It was just using the updated value to check for PCI_MSIX_ENTRY_CTRL_MASKBIT. This is something else. The new behavior might be correct, but at least the commit message needs to explain it. > >> - 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? > > The combination of the !(val & PCI_MSIX_ENTRY_CTRL_MASKBIT) > check in the if() surrounding this call and the > (entry->latch(VECTOR_CTRL) & PCI_MSIX_ENTRY_CTRL_MASKBIT) > check inside the function guarantee just that (i.e. the function > invocation is benign in the other case, as entry->addr/entry->data > would remain unchanged). OK, maybe the code works as is, but it took me a long time to make sense of it because it relies on the combinations of three checks in three different places. I would prefer to change it into something more obvious.