From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41127) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z4r3P-00039G-8Y for qemu-devel@nongnu.org; Tue, 16 Jun 2015 09:37:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z4r3K-0006E0-9W for qemu-devel@nongnu.org; Tue, 16 Jun 2015 09:37:27 -0400 Received: from smtp.citrix.com ([66.165.176.89]:15019) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z4r3K-000696-1D for qemu-devel@nongnu.org; Tue, 16 Jun 2015 09:37:22 -0400 Date: Tue, 16 Jun 2015 14:35:41 +0100 From: Stefano Stabellini In-Reply-To: <5571AB41020000780008153F@mail.emea.novell.com> Message-ID: References: <5571AA3B020000780008152E@mail.emea.novell.com> <5571AB41020000780008153F@mail.emea.novell.com> MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" Subject: Re: [Qemu-devel] [PATCH 1/6] xen/MSI-X: latch MSI-X table writes List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jan Beulich Cc: xen-devel , qemu-devel@nongnu.org, Stefano Stabellini 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 > > @@ -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,