Marc-André Lureau <marcandre.lur...@gmail.com> writes: > On Mon, Feb 29, 2016 at 7:40 PM, Markus Armbruster <arm...@redhat.com> wrote: >> 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=off. >> >> * 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=off. 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.