diff mbox

[6/8] ivshmem: generalize ivshmem_setup_interrupts

Message ID 1450697444-30119-7-git-send-email-marcandre.lureau@redhat.com
State New
Headers show

Commit Message

Marc-André Lureau Dec. 21, 2015, 11:30 a.m. UTC 
From: Marc-André Lureau <marcandre.lureau@redhat.com>

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 <marcandre.lureau@redhat.com>
---
 hw/misc/ivshmem.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

Comments

Markus Armbruster Jan. 29, 2016, 3:59 p.m. UTC | #1 
marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> 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 <marcandre.lureau@redhat.com>
> ---
>  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?

> +        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.

>  
>  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 Feb. 1, 2016, 2:50 p.m. UTC | #2 
Hi

On Fri, Jan 29, 2016 at 4:59 PM, Markus Armbruster <armbru@redhat.com> wrote:
> marcandre.lureau@redhat.com writes:
>
>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> 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 <marcandre.lureau@redhat.com>
>> ---
>>  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.
>
Markus Armbruster Feb. 1, 2016, 4:50 p.m. UTC | #3 
Marc-André Lureau <marcandre.lureau@gmail.com> writes:

> Hi
>
> On Fri, Jan 29, 2016 at 4:59 PM, Markus Armbruster <armbru@redhat.com> wrote:
>> marcandre.lureau@redhat.com writes:
>>
>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>
>>> 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 <marcandre.lureau@redhat.com>
>>> ---
>>>  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.

Sounds like I can expect a respin, correct me if I'm wrong.

[...]
diff mbox

Patch

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) {
+        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;
 }
 
 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;
         }