From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52573) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ab3y3-0002H6-Pb for qemu-devel@nongnu.org; Wed, 02 Mar 2016 05:25:20 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ab3y0-0008OZ-U9 for qemu-devel@nongnu.org; Wed, 02 Mar 2016 05:25:19 -0500 Received: from mx1.redhat.com ([209.132.183.28]:54095) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ab3y0-0008OQ-NK for qemu-devel@nongnu.org; Wed, 02 Mar 2016 05:25:16 -0500 From: Markus Armbruster References: <1456771254-17511-1-git-send-email-armbru@redhat.com> <1456771254-17511-18-git-send-email-armbru@redhat.com> Date: Wed, 02 Mar 2016 11:25:13 +0100 In-Reply-To: (=?utf-8?Q?=22Marc-Andr=C3=A9?= Lureau"'s message of "Tue, 1 Mar 2016 17:57:27 +0100") Message-ID: <87si09npme.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 17/38] ivshmem: Clean up MSI-X conditions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?utf-8?Q?Marc-Andr=C3=A9?= Lureau Cc: Paolo Bonzini , cam , Claudio Fontana , QEMU , David Marchand Marc-Andr=C3=A9 Lureau writes: > On Mon, Feb 29, 2016 at 7:40 PM, Markus Armbruster wr= ote: >> There are three predicates related to MSI-X: >> >> * ivshmem_has_feature(s, IVSHMEM_MSI) is true unless the non-MSI-X >> variant of the device is selected with msi=3Doff. >> >> * msix_present() is true when the device has the PCI capability MSI-X. >> It's initially false, and becomes true during successful realize of >> the MSI-X variant of the device. Thus, it's the same as >> ivshmem_has_feature(s, IVSHMEM_MSI) for realized devices. >> >> * msix_enabled() is true when msix_present() is true and guest software >> has enabled MSI-X. >> >> Code that differs between the non-MSI-X and the MSI-X variant of the >> device needs to be guarded by ivshmem_has_feature(s, IVSHMEM_MSI) or >> by msix_present(), except the latter works only for realized devices. >> >> Code that depends on whether MSI-X is in use needs to be guarded with >> msix_enabled(). >> >> Code review led me to two minor messes: >> >> * ivshmem_vector_notify() calls msix_notify() even when >> !msix_enabled(), unlike most other MSI-X-capable devices. As far as >> I can tell, msix_notify() does nothing when !msix_enabled(). Add >> the guard anyway. >> > > sure, feel free to split in a seperate patch with my Review-by. > >> * Most callers of ivshmem_use_msix() guard it with >> ivshmem_has_feature(s, IVSHMEM_MSI). Not necessary, because >> ivshmem_use_msix() does nothing when !msix_present(). That's >> ivshmem's only use of msix_present(), though. Rename >> ivshmem_use_msix() to ivshmem_vector_use(), replace msix_present() >> by ivshmem_has_feature() there, and drop the redundant guards. > > I prefer that code related to msix remains within msix blocks if > possible, improving readability imho. > > Furthermore, since the function is msix specific, I think it's worth > keeping the "msix" in the name. Since ivshmem_msix_use() wasn't good > enough for you, perhaps we need the full-blown > ivshmem_msix_vectors_use() instead. "Vectors" means actually two related, but distinct things with ivshmem: * the communication channels to transmit interrupts among peers, and * the MSI-X vectors. You can have the former without the latter, with msi=3Doff. I guess there are two views of the function, both reasonable: 1. Prepare usage of "vectors", i.e. either kind. Name the function ivshmem_msix_vectors_use(), and call it unconditionally. The fact that it does only MSI-X stuff is implementation detail. 2. Prepare usage of MSI-X vectors. Name the function ivshmem_msix_vectors_use() or similar, and calls it only when ivshmem_has_feature(s, IVSHMEM_MSI), for consistency with other MSI-X functions. You prefer 2, I prefer 1. But it's not a deal-breaker for me; if you feel strongly, I can do 2.