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

[Marc-André Lureau]

Hi

On Fri, Jan 29, 2016 at 4:59 PM, Markus Armbruster <address@hidden> wrote:
> 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 | 27 +++++++++++++++++----------
>>  1 file changed, 17 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
>> index dcfc8cc..11780b1 100644
>> --- a/hw/misc/ivshmem.c
>> +++ b/hw/misc/ivshmem.c
>> @@ -768,19 +768,28 @@ static void ivshmem_reset(DeviceState *d)
>>      ivshmem_use_msix(s);
>>  }
>>
>> -static int ivshmem_setup_msi(IVShmemState * s)
>> +static int ivshmem_setup_interrupts(IVShmemState *s, Error **errp)
>>  {
>> -    if (msix_init_exclusive_bar(PCI_DEVICE(s), s->vectors, 1)) {
>> -        return -1;
>> +    /* allocate QEMU callback data for receiving interrupts */
>> +    s->msi_vectors = g_malloc0(s->vectors * sizeof(MSIVector));
>> +    if (!s->msi_vectors) {
>
> Happens exactly when s->vectors is zero.  Is that a legitimate
> configuration?

msix_init_exclusive_bar() already failed with nvectors == 0. However
it is acceptable for !msi to have nvectors == 0. Fixed.

>> +        goto fail;
>>      }
>>
>> -    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)) {
>> +            goto fail;
>> +        }
>>
>> -    /* 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;
>> +
>> +fail:
>> +    error_setg(errp, "failed to initialize interrupts");
>> +    return -1;
>>  }
>
> Recommend not to move the error_setg().  Keeps this function simpler, at
> no cost.

ok

>
>>
>>  static void ivshmem_enable_irqfd(IVShmemState *s)
>> @@ -946,9 +955,7 @@ 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, errp) < 0) {
>>              return;
>>          }
>
> Yup, the only change is we now allocate s->msi_vectors whether we have
> IVSHMEM_MSI or not.
>



-- 
Marc-André Lureau