From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCH 1/6] xen/MSI-X: latch MSI-X table writes Date: Tue, 16 Jun 2015 15:04:29 +0100 Message-ID: <5580490D02000078000858C8__43592.7504244775$1434463588$gmane$org@mail.emea.novell.com> References: <5571AA3B020000780008152E@mail.emea.novell.com> <5571AB41020000780008153F@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 1Z4rTf-0001qJ-Di for xen-devel@lists.xenproject.org; Tue, 16 Jun 2015 14:04:35 +0000 In-Reply-To: Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Stefano Stabellini Cc: xen-devel , qemu-devel@nongnu.org List-Id: xen-devel@lists.xenproject.org >>> 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. >> @@ -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? >> - 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). Jan