[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 19/40] ivshmem: Clean up MSI-X conditions
From: |
Marc-André Lureau |
Subject: |
Re: [Qemu-devel] [PATCH v3 19/40] ivshmem: Clean up MSI-X conditions |
Date: |
Fri, 18 Mar 2016 16:04:15 +0100 |
On Tue, Mar 15, 2016 at 7:34 PM, Markus Armbruster <address@hidden> 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.
>
> * 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. Guard it
> consistently, and drop the now redundant msix_present() check.
> While there, rename ivshmem_use_msix() to ivshmem_msix_vector_use().
>
> Signed-off-by: Markus Armbruster <address@hidden>
Reviewed-by: Marc-André Lureau <address@hidden>
> ---
> hw/misc/ivshmem.c | 22 ++++++++++------------
> 1 file changed, 10 insertions(+), 12 deletions(-)
>
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index 1debce3..abcb1c1 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -274,7 +274,9 @@ static void ivshmem_vector_notify(void *opaque)
>
> IVSHMEM_DPRINTF("interrupt on vector %p %d\n", pdev, vector);
> if (ivshmem_has_feature(s, IVSHMEM_MSI)) {
> - msix_notify(pdev, vector);
> + if (msix_enabled(pdev)) {
> + msix_notify(pdev, vector);
> + }
> } else {
> ivshmem_IntrStatus_write(s, 1);
> }
> @@ -713,16 +715,11 @@ static void ivshmem_check_version(void *opaque, const
> uint8_t * buf, int size)
> /* Select the MSI-X vectors used by device.
> * ivshmem maps events to vectors statically, so
> * we just enable all vectors on init and after reset. */
> -static void ivshmem_use_msix(IVShmemState * s)
> +static void ivshmem_msix_vector_use(IVShmemState *s)
> {
> PCIDevice *d = PCI_DEVICE(s);
> int i;
>
> - IVSHMEM_DPRINTF("%s, msix present: %d\n", __func__, msix_present(d));
> - if (!msix_present(d)) {
> - return;
> - }
> -
> for (i = 0; i < s->vectors; i++) {
> msix_vector_use(d, i);
> }
> @@ -734,7 +731,9 @@ static void ivshmem_reset(DeviceState *d)
>
> s->intrstatus = 0;
> s->intrmask = 0;
> - ivshmem_use_msix(s);
> + if (ivshmem_has_feature(s, IVSHMEM_MSI)) {
> + ivshmem_msix_vector_use(s);
> + }
> }
>
> static int ivshmem_setup_interrupts(IVShmemState *s)
> @@ -748,7 +747,7 @@ static int ivshmem_setup_interrupts(IVShmemState *s)
> }
>
> IVSHMEM_DPRINTF("msix initialized (%d vectors)\n", s->vectors);
> - ivshmem_use_msix(s);
> + ivshmem_msix_vector_use(s);
> }
>
> return 0;
> @@ -1040,9 +1039,8 @@ static int ivshmem_post_load(void *opaque, int
> version_id)
> IVShmemState *s = opaque;
>
> if (ivshmem_has_feature(s, IVSHMEM_MSI)) {
> - ivshmem_use_msix(s);
> + ivshmem_msix_vector_use(s);
> }
> -
> return 0;
> }
>
> @@ -1070,7 +1068,7 @@ static int ivshmem_load_old(QEMUFile *f, void *opaque,
> int version_id)
>
> if (ivshmem_has_feature(s, IVSHMEM_MSI)) {
> msix_load(pdev, f);
> - ivshmem_use_msix(s);
> + ivshmem_msix_vector_use(s);
> } else {
> s->intrstatus = qemu_get_be32(f);
> s->intrmask = qemu_get_be32(f);
> --
> 2.4.3
>
>
--
Marc-André Lureau
- [Qemu-devel] [PATCH v3 00/40] ivshmem: Fixes, cleanups, device model split, Markus Armbruster, 2016/03/15
- [Qemu-devel] [PATCH v3 04/40] qemu-doc: Fix ivshmem huge page example, Markus Armbruster, 2016/03/15
- [Qemu-devel] [PATCH v3 02/40] ivshmem-server: Fix and clean up command line help, Markus Armbruster, 2016/03/15
- [Qemu-devel] [PATCH v3 14/40] ivshmem: Drop ivshmem_event() stub, Markus Armbruster, 2016/03/15
- [Qemu-devel] [PATCH v3 08/40] ivshmem-test: Clean up wait for devices to become operational, Markus Armbruster, 2016/03/15
- [Qemu-devel] [PATCH v3 07/40] ivshmem-test: Improve test case /ivshmem/single, Markus Armbruster, 2016/03/15
- [Qemu-devel] [PATCH v3 24/40] ivshmem: Plug leaks on unplug, fix peer disconnect, Markus Armbruster, 2016/03/15
- [Qemu-devel] [PATCH v3 19/40] ivshmem: Clean up MSI-X conditions, Markus Armbruster, 2016/03/15
- Re: [Qemu-devel] [PATCH v3 19/40] ivshmem: Clean up MSI-X conditions,
Marc-André Lureau <=
- [Qemu-devel] [PATCH v3 06/40] tests/libqos/pci-pc: Fix qpci_pc_iomap() to map BARs aligned, Markus Armbruster, 2016/03/15
- [Qemu-devel] [PATCH v3 12/40] ivshmem: Compile debug prints unconditionally to prevent bit-rot, Markus Armbruster, 2016/03/15
- [Qemu-devel] [PATCH v3 09/40] ivshmem-test: Improve test cases /ivshmem/server-*, Markus Armbruster, 2016/03/15
- [Qemu-devel] [PATCH v3 03/40] ivshmem-server: Don't overload POSIX shmem and file name, Markus Armbruster, 2016/03/15
- [Qemu-devel] [PATCH v3 13/40] ivshmem: Clean up after commit 9940c32, Markus Armbruster, 2016/03/15
- [Qemu-devel] [PATCH v3 17/40] ivshmem: Failed realize() can leave migration blocker behind, Markus Armbruster, 2016/03/15
- [Qemu-devel] [PATCH v3 20/40] ivshmem: Leave INTx alone when using MSI-X, Markus Armbruster, 2016/03/15
- [Qemu-devel] [PATCH v3 25/40] ivshmem: Receive shared memory synchronously in realize(), Markus Armbruster, 2016/03/15
- [Qemu-devel] [PATCH v3 18/40] ivshmem: Clean up register callbacks, Markus Armbruster, 2016/03/15