From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44203) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a1EVX-0004TA-KF for qemu-devel@nongnu.org; Tue, 24 Nov 2015 09:23:52 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a1EVT-0002Ti-MF for qemu-devel@nongnu.org; Tue, 24 Nov 2015 09:23:47 -0500 Received: from prv-mh.provo.novell.com ([137.65.248.74]:34892) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a1EVT-0002T6-FS for qemu-devel@nongnu.org; Tue, 24 Nov 2015 09:23:43 -0500 Message-Id: <565480FB02000078000B875E@prv-mh.provo.novell.com> Date: Tue, 24 Nov 2015 07:23:39 -0700 From: "Jan Beulich" Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: quoted-printable Content-Disposition: inline Subject: Re: [Qemu-devel] [Xen-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 >>> Tue, 16 Jun 2015 15:48:16 +0100 Stefano Stabellini wrote: >On Tue, 16 Jun 2015, Jan Beulich wrote: >> >>> On 16.06.15 at 15:35, Stefano Stabellini wrote: >> > On Fri, 5 Jun 2015, Jan Beulich wrote: I'm sorry for getting back to this only now. >> >> @@ -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. >>=20 >> The comment would (pointlessly imo) re-state what the code already >> says: >>=20 >> > 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? >>=20 >> 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? That would maybe be doable with just pci_msix_write() as a caller, but not with the one in xen_pt_msix_update() (where we have no transition of the mask bit from set to clear). Jan 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, 24 Nov 2015 07:23:39 -0700 Message-ID: <565480FB02000078000B875E@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1a1EVY-00008f-Uu for xen-devel@lists.xenproject.org; Tue, 24 Nov 2015 14:23:49 +0000 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 >>> Tue, 16 Jun 2015 15:48:16 +0100 Stefano Stabellini wrote: >On Tue, 16 Jun 2015, Jan Beulich wrote: >> >>> On 16.06.15 at 15:35, Stefano Stabellini wrote: >> > On Fri, 5 Jun 2015, Jan Beulich wrote: I'm sorry for getting back to this only now. >> >> @@ -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? That would maybe be doable with just pci_msix_write() as a caller, but not with the one in xen_pt_msix_update() (where we have no transition of the mask bit from set to clear). Jan