Re: [Qemu-devel] [PATCH v2 6/8] ivshmem: generalize ivshmem_setup_interrupts

[Markus Armbruster]

address@hidden writes:

> From: Marc-André Lureau <address@hidden>
>
> Call ivshmem_setup_interrupts() with or without MSI, always allocate
> msi_vectors that is going to be used in all case in the following patch.
>
> Signed-off-by: Marc-André Lureau <address@hidden>
> ---
>  hw/misc/ivshmem.c | 26 ++++++++++++++++----------
>  1 file changed, 16 insertions(+), 10 deletions(-)
>
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index 5029789..c601a8a 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -769,18 +769,25 @@ static void ivshmem_reset(DeviceState *d)
>      ivshmem_use_msix(s);
>  }
>  
> -static int ivshmem_setup_msi(IVShmemState * s)
> +static int ivshmem_setup_interrupts(IVShmemState *s)
>  {
> -    if (msix_init_exclusive_bar(PCI_DEVICE(s), s->vectors, 1)) {
> -        return -1;
> +    /* allocate QEMU callback data for receiving interrupts */
> +    if (s->vectors > 0) {
> +        s->msi_vectors = g_malloc0(s->vectors * sizeof(MSIVector));
> +        if (!s->msi_vectors) {

Can't happen; s->vectors * sizeof(MSIVector) != 0.

> +            return -1;
> +        }
>      }
>  
> -    IVSHMEM_DPRINTF("msix initialized (%d vectors)\n", s->vectors);
> +    if (ivshmem_has_feature(s, IVSHMEM_MSI)) {
> +        if (msix_init_exclusive_bar(PCI_DEVICE(s), s->vectors, 1)) {
> +            return -1;
> +        }
>  
> -    /* allocate QEMU char devices for receiving interrupts */
> -    s->msi_vectors = g_malloc0(s->vectors * sizeof(MSIVector));
> +        IVSHMEM_DPRINTF("msix initialized (%d vectors)\n", s->vectors);
> +        ivshmem_use_msix(s);
> +    }
>  
> -    ivshmem_use_msix(s);
>      return 0;
>  }
>  
> @@ -947,9 +954,8 @@ static void pci_ivshmem_realize(PCIDevice *dev, Error 
> **errp)
>          IVSHMEM_DPRINTF("using shared memory server (socket = %s)\n",
>                          s->server_chr->filename);
>  
> -        if (ivshmem_has_feature(s, IVSHMEM_MSI) &&
> -            ivshmem_setup_msi(s)) {
> -            error_setg(errp, "msix initialization failed");
> +        if (ivshmem_setup_interrupts(s) < 0) {
> +            error_setg(errp, "failed to initialize interrupts");
>              return;
>          }

With the useless conditional dropped:
Reviewed-by: Markus Armbruster <address@hidden>