From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49868) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z4rTj-0007Pv-HO for qemu-devel@nongnu.org; Tue, 16 Jun 2015 10:04:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z4rTf-0002vw-F8 for qemu-devel@nongnu.org; Tue, 16 Jun 2015 10:04:39 -0400 Received: from mail.emea.novell.com ([130.57.118.101]:37143) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z4rTf-0002vG-6F for qemu-devel@nongnu.org; Tue, 16 Jun 2015 10:04:35 -0400 Message-Id: <5580490D02000078000858C8@mail.emea.novell.com> Date: Tue, 16 Jun 2015 15:04:29 +0100 From: "Jan Beulich" References: <5571AA3B020000780008152E@mail.emea.novell.com> <5571AB41020000780008153F@mail.emea.novell.com> In-Reply-To: Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: quoted-printable Content-Disposition: inline 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: Stefano Stabellini Cc: xen-devel , qemu-devel@nongnu.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 >> =20 >> pirq =3D entry->pirq; >> =20 >> + if (pirq =3D=3D XEN_PT_UNASSIGNED_PIRQ || s->msix->maskall || >> + (entry->latch(VECTOR_CTRL) & PCI_MSIX_ENTRY_CTRL_MASKBIT)) { >=20 > 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? >=20 > 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 =3D addr % PCI_MSIX_ENTRY_SIZE; >> =20 >> if (offset !=3D PCI_MSIX_ENTRY_VECTOR_CTRL) { >> - const volatile uint32_t *vec_ctrl; >> - >> if (get_entry_value(entry, offset) =3D=3D val >> && entry->pirq !=3D XEN_PT_UNASSIGNED_PIRQ) { >> return; >> } >> =20 >> + entry->updated =3D 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 =3D s->msix->phys_iomem_base + entry_nr * PCI_MSIX_ENT= RY_SIZE >> + PCI_MSIX_ENTRY_VECTOR_CTRL; >> + set_entry_value(entry, offset, *vec_ctrl); >=20 > 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 =3D true; >> - XEN_PT_ERR(&s->dev, "Can't update msix entry %d since = MSI-X is" >> - " already enabled.\n", entry_nr); >> - } >> - return; >> - } >> - >> - entry->updated =3D true; >> + xen_pt_msix_update_one(s, entry_nr); >=20 > 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