diff mbox

[v3,8/8] vmxnet3: remove unnecessary internal msix state flag

Message ID 1473839464-8670-9-git-send-email-caoj.fnst@cn.fujitsu.com (mailing list archive)
State New, archived
Headers show

Commit Message

Cao jin Sept. 14, 2016, 7:51 a.m. UTC
The corresponding msi flag is already dropped in commit 1070048e.

CC: Dmitry Fleytman <dmitry@daynix.com>
CC: Jason Wang <jasowang@redhat.com>
CC: Michael S. Tsirkin <mst@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Marcel Apfelbaum <marcel@redhat.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/net/vmxnet3.c | 38 ++++++++++++++------------------------
 1 file changed, 14 insertions(+), 24 deletions(-)

Comments

Markus Armbruster Sept. 29, 2016, 2:42 p.m. UTC | #1
Cao jin <caoj.fnst@cn.fujitsu.com> writes:

> The corresponding msi flag is already dropped in commit 1070048e.

Repeating the rationale for the change wouldn't hurt:

    Internal flag msix_used is unnecessary, it has the same effect as
    msix_enabled().

    The corresponding msi flag is already dropped in commit 1070048e.

Can hopefully be touched up on commit.

>
> CC: Dmitry Fleytman <dmitry@daynix.com>
> CC: Jason Wang <jasowang@redhat.com>
> CC: Michael S. Tsirkin <mst@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Marcel Apfelbaum <marcel@redhat.com>
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>  hw/net/vmxnet3.c | 38 ++++++++++++++------------------------
>  1 file changed, 14 insertions(+), 24 deletions(-)
>
> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> index 7d44af1..1decb53 100644
> --- a/hw/net/vmxnet3.c
> +++ b/hw/net/vmxnet3.c
> @@ -281,8 +281,6 @@ typedef struct {
>          Vmxnet3RxqDescr rxq_descr[VMXNET3_DEVICE_MAX_RX_QUEUES];
>          Vmxnet3TxqDescr txq_descr[VMXNET3_DEVICE_MAX_TX_QUEUES];
>  
> -        /* Whether MSI-X support was installed successfully */
> -        bool msix_used;
>          hwaddr drv_shmem;
>          hwaddr temp_shared_guest_driver_memory;
>  
> @@ -359,7 +357,7 @@ static bool _vmxnet3_assert_interrupt_line(VMXNET3State *s, uint32_t int_idx)
>  {
>      PCIDevice *d = PCI_DEVICE(s);
>  
> -    if (s->msix_used && msix_enabled(d)) {
> +    if (msix_enabled(d)) {
>          VMW_IRPRN("Sending MSI-X notification for vector %u", int_idx);
>          msix_notify(d, int_idx);
>          return false;
> @@ -383,7 +381,7 @@ static void _vmxnet3_deassert_interrupt_line(VMXNET3State *s, int lidx)
>       * This function should never be called for MSI(X) interrupts
>       * because deassertion never required for message interrupts
>       */
> -    assert(!s->msix_used || !msix_enabled(d));
> +    assert(!msix_enabled(d));
>      /*
>       * This function should never be called for MSI(X) interrupts
>       * because deassertion never required for message interrupts
> @@ -421,7 +419,7 @@ static void vmxnet3_trigger_interrupt(VMXNET3State *s, int lidx)
>      s->interrupt_states[lidx].is_pending = true;
>      vmxnet3_update_interrupt_line_state(s, lidx);
>  
> -    if (s->msix_used && msix_enabled(d) && s->auto_int_masking) {
> +    if (msix_enabled(d) && s->auto_int_masking) {
>          goto do_automask;
>      }
>  
> @@ -1427,7 +1425,7 @@ static void vmxnet3_update_features(VMXNET3State *s)
>  
>  static bool vmxnet3_verify_intx(VMXNET3State *s, int intx)
>  {
> -    return s->msix_used || msi_enabled(PCI_DEVICE(s))
> +    return msix_enabled(PCI_DEVICE(s)) || msi_enabled(PCI_DEVICE(s))
>          || intx == pci_get_byte(s->parent_obj.config + PCI_INTERRUPT_PIN) - 1;
>  }
>  
> @@ -1444,18 +1442,18 @@ static void vmxnet3_validate_interrupts(VMXNET3State *s)
>      int i;
>  
>      VMW_CFPRN("Verifying event interrupt index (%d)", s->event_int_idx);
> -    vmxnet3_validate_interrupt_idx(s->msix_used, s->event_int_idx);
> +    vmxnet3_validate_interrupt_idx(msix_enabled(PCI_DEVICE(s)), s->event_int_idx);
>  
>      for (i = 0; i < s->txq_num; i++) {
>          int idx = s->txq_descr[i].intr_idx;
>          VMW_CFPRN("Verifying TX queue %d interrupt index (%d)", i, idx);
> -        vmxnet3_validate_interrupt_idx(s->msix_used, idx);
> +        vmxnet3_validate_interrupt_idx(msix_enabled(PCI_DEVICE(s)), idx);
>      }
>  
>      for (i = 0; i < s->rxq_num; i++) {
>          int idx = s->rxq_descr[i].intr_idx;
>          VMW_CFPRN("Verifying RX queue %d interrupt index (%d)", i, idx);
> -        vmxnet3_validate_interrupt_idx(s->msix_used, idx);
> +        vmxnet3_validate_interrupt_idx(msix_enabled(PCI_DEVICE(s)), idx);
>      }
>  }
>  
> @@ -2184,6 +2182,7 @@ vmxnet3_use_msix_vectors(VMXNET3State *s, int num_vectors)
>  static bool
>  vmxnet3_init_msix(VMXNET3State *s)
>  {
> +    bool msix;
>      PCIDevice *d = PCI_DEVICE(s);
>      int res = msix_init(d, VMXNET3_MAX_INTRS,
>                          &s->msix_bar,
> @@ -2198,17 +2197,18 @@ vmxnet3_init_msix(VMXNET3State *s)
>  
>      if (0 > res) {
>          VMW_WRPRN("Failed to initialize MSI-X, board's MSI support is broken");
> -        s->msix_used = false;
> +        msix = false;
>      } else {
>          if (!vmxnet3_use_msix_vectors(s, VMXNET3_MAX_INTRS)) {
>              VMW_WRPRN("Failed to use MSI-X vectors, error %d", res);
>              msix_uninit(d, &s->msix_bar, &s->msix_bar);
> -            s->msix_used = false;
> +            msix = false;
>          } else {
> -            s->msix_used = true;
> +            msix = true;
>          }
>      }
> -    return s->msix_used;
> +
> +    return msix;
>  }
>  
>  static void
> @@ -2216,7 +2216,7 @@ vmxnet3_cleanup_msix(VMXNET3State *s)
>  {
>      PCIDevice *d = PCI_DEVICE(s);
>  
> -    if (s->msix_used) {
> +    if (msix_enabled(d)) {
>          vmxnet3_unuse_msix_vectors(s, VMXNET3_MAX_INTRS);
>          msix_uninit(d, &s->msix_bar, &s->msix_bar);
>      }
> @@ -2551,21 +2551,11 @@ static void vmxnet3_put_rxq_descr(QEMUFile *f, void *pv, size_t size)
>  static int vmxnet3_post_load(void *opaque, int version_id)
>  {
>      VMXNET3State *s = opaque;
> -    PCIDevice *d = PCI_DEVICE(s);
>  
>      net_tx_pkt_init(&s->tx_pkt, PCI_DEVICE(s),
>                      s->max_tx_frags, s->peer_has_vhdr);
>      net_rx_pkt_init(&s->rx_pkt, s->peer_has_vhdr);
>  
> -    if (s->msix_used) {
> -        if  (!vmxnet3_use_msix_vectors(s, VMXNET3_MAX_INTRS)) {
> -            VMW_WRPRN("Failed to re-use MSI-X vectors");
> -            msix_uninit(d, &s->msix_bar, &s->msix_bar);
> -            s->msix_used = false;
> -            return -1;
> -        }
> -    }
> -
>      vmxnet3_validate_queues(s);
>      vmxnet3_validate_interrupts(s);

This hunk isn't obvious.  Can you explain the change?
Cao jin Sept. 30, 2016, 6:58 a.m. UTC | #2
On 09/29/2016 10:42 PM, Markus Armbruster wrote:
> Cao jin <caoj.fnst@cn.fujitsu.com> writes:
>

>>   static int vmxnet3_post_load(void *opaque, int version_id)
>>   {
>>       VMXNET3State *s = opaque;
>> -    PCIDevice *d = PCI_DEVICE(s);
>>
>>       net_tx_pkt_init(&s->tx_pkt, PCI_DEVICE(s),
>>                       s->max_tx_frags, s->peer_has_vhdr);
>>       net_rx_pkt_init(&s->rx_pkt, s->peer_has_vhdr);
>>
>> -    if (s->msix_used) {
>> -        if  (!vmxnet3_use_msix_vectors(s, VMXNET3_MAX_INTRS)) {
>> -            VMW_WRPRN("Failed to re-use MSI-X vectors");
>> -            msix_uninit(d, &s->msix_bar, &s->msix_bar);
>> -            s->msix_used = false;
>> -            return -1;
>> -        }
>> -    }
>> -
>>       vmxnet3_validate_queues(s);
>>       vmxnet3_validate_interrupts(s);
>
> This hunk isn't obvious.  Can you explain the change?
>

flag msix_used is used in VMStateDescription.post_Load().

1st, I think msix's code here is not necessary, because in destination, 
device has been realized before incoming migration, So I don't know why 
re-use MSI-X vectors here. Dmitry, could help to explain?

2nd, this patch is going to remove this flag, so I removed the hunk.
Markus Armbruster Sept. 30, 2016, 1:08 p.m. UTC | #3
Cao jin <caoj.fnst@cn.fujitsu.com> writes:

> On 09/29/2016 10:42 PM, Markus Armbruster wrote:
>> Cao jin <caoj.fnst@cn.fujitsu.com> writes:
>>
>
>>>   static int vmxnet3_post_load(void *opaque, int version_id)
>>>   {
>>>       VMXNET3State *s = opaque;
>>> -    PCIDevice *d = PCI_DEVICE(s);
>>>
>>>       net_tx_pkt_init(&s->tx_pkt, PCI_DEVICE(s),
>>>                       s->max_tx_frags, s->peer_has_vhdr);
>>>       net_rx_pkt_init(&s->rx_pkt, s->peer_has_vhdr);
>>>
>>> -    if (s->msix_used) {
>>> -        if  (!vmxnet3_use_msix_vectors(s, VMXNET3_MAX_INTRS)) {
>>> -            VMW_WRPRN("Failed to re-use MSI-X vectors");
>>> -            msix_uninit(d, &s->msix_bar, &s->msix_bar);
>>> -            s->msix_used = false;
>>> -            return -1;
>>> -        }
>>> -    }
>>> -
>>>       vmxnet3_validate_queues(s);
>>>       vmxnet3_validate_interrupts(s);
>>
>> This hunk isn't obvious.  Can you explain the change?
>>
>
> flag msix_used is used in VMStateDescription.post_Load().
>
> 1st, I think msix's code here is not necessary, because in
> destination, device has been realized before incoming migration, So I
> don't know why re-use MSI-X vectors here. Dmitry, could help to
> explain?
>
> 2nd, this patch is going to remove this flag, so I removed the hunk.

We need to find out whether the call of vmxnet3_use_msix_vectors() is
necessary.  I suspect it's not only not necessary, but actively wrong.

If that's true, removing becomes a bug fix that should be a separate
patch.

If it's only unnecessary, the removal may stay in this patch, but it
needs to be explained.  Separate patch might be easier to explain.  Your
choice.

If it correct and necessary, then this patch needs to be changed not to
drop it.  Instead, replace s->msix_used by msix_enabled(d) like you do
elsewhere.

Dmitry, can you help us find out?
Dmitry Fleytman Oct. 6, 2016, 9:39 a.m. UTC | #4
> On 30 Sep 2016, at 15:08 PM, Markus Armbruster <armbru@redhat.com> wrote:
> 
> Cao jin <caoj.fnst@cn.fujitsu.com> writes:
> 
>> On 09/29/2016 10:42 PM, Markus Armbruster wrote:
>>> Cao jin <caoj.fnst@cn.fujitsu.com> writes:
>>> 
>> 
>>>>  static int vmxnet3_post_load(void *opaque, int version_id)
>>>>  {
>>>>      VMXNET3State *s = opaque;
>>>> -    PCIDevice *d = PCI_DEVICE(s);
>>>> 
>>>>      net_tx_pkt_init(&s->tx_pkt, PCI_DEVICE(s),
>>>>                      s->max_tx_frags, s->peer_has_vhdr);
>>>>      net_rx_pkt_init(&s->rx_pkt, s->peer_has_vhdr);
>>>> 
>>>> -    if (s->msix_used) {
>>>> -        if  (!vmxnet3_use_msix_vectors(s, VMXNET3_MAX_INTRS)) {
>>>> -            VMW_WRPRN("Failed to re-use MSI-X vectors");
>>>> -            msix_uninit(d, &s->msix_bar, &s->msix_bar);
>>>> -            s->msix_used = false;
>>>> -            return -1;
>>>> -        }
>>>> -    }
>>>> -
>>>>      vmxnet3_validate_queues(s);
>>>>      vmxnet3_validate_interrupts(s);
>>> 
>>> This hunk isn't obvious.  Can you explain the change?
>>> 
>> 
>> flag msix_used is used in VMStateDescription.post_Load().
>> 
>> 1st, I think msix's code here is not necessary, because in
>> destination, device has been realized before incoming migration, So I
>> don't know why re-use MSI-X vectors here. Dmitry, could help to
>> explain?
>> 
>> 2nd, this patch is going to remove this flag, so I removed the hunk.
> 
> We need to find out whether the call of vmxnet3_use_msix_vectors() is
> necessary.  I suspect it's not only not necessary, but actively wrong.
> 
> If that's true, removing becomes a bug fix that should be a separate
> patch.
> 
> If it's only unnecessary, the removal may stay in this patch, but it
> needs to be explained.  Separate patch might be easier to explain.  Your
> choice.
> 
> If it correct and necessary, then this patch needs to be changed not to
> drop it.  Instead, replace s->msix_used by msix_enabled(d) like you do
> elsewhere.
> 
> Dmitry, can you help us find out?

Hello,

Yes, from what I see, this call is wrong and leads to
reference leaks on device unload at migration target.
It should be removed.

Best regards,
Dmitry
Dr. David Alan Gilbert Oct. 6, 2016, 3:43 p.m. UTC | #5
* Dmitry Fleytman (dmitry@daynix.com) wrote:
> 
> > On 30 Sep 2016, at 15:08 PM, Markus Armbruster <armbru@redhat.com> wrote:
> > 
> > Cao jin <caoj.fnst@cn.fujitsu.com> writes:
> > 
> >> On 09/29/2016 10:42 PM, Markus Armbruster wrote:
> >>> Cao jin <caoj.fnst@cn.fujitsu.com> writes:
> >>> 
> >> 
> >>>>  static int vmxnet3_post_load(void *opaque, int version_id)
> >>>>  {
> >>>>      VMXNET3State *s = opaque;
> >>>> -    PCIDevice *d = PCI_DEVICE(s);
> >>>> 
> >>>>      net_tx_pkt_init(&s->tx_pkt, PCI_DEVICE(s),
> >>>>                      s->max_tx_frags, s->peer_has_vhdr);
> >>>>      net_rx_pkt_init(&s->rx_pkt, s->peer_has_vhdr);
> >>>> 
> >>>> -    if (s->msix_used) {
> >>>> -        if  (!vmxnet3_use_msix_vectors(s, VMXNET3_MAX_INTRS)) {
> >>>> -            VMW_WRPRN("Failed to re-use MSI-X vectors");
> >>>> -            msix_uninit(d, &s->msix_bar, &s->msix_bar);
> >>>> -            s->msix_used = false;
> >>>> -            return -1;
> >>>> -        }
> >>>> -    }
> >>>> -
> >>>>      vmxnet3_validate_queues(s);
> >>>>      vmxnet3_validate_interrupts(s);
> >>> 
> >>> This hunk isn't obvious.  Can you explain the change?
> >>> 
> >> 
> >> flag msix_used is used in VMStateDescription.post_Load().
> >> 
> >> 1st, I think msix's code here is not necessary, because in
> >> destination, device has been realized before incoming migration, So I
> >> don't know why re-use MSI-X vectors here. Dmitry, could help to
> >> explain?
> >> 
> >> 2nd, this patch is going to remove this flag, so I removed the hunk.
> > 
> > We need to find out whether the call of vmxnet3_use_msix_vectors() is
> > necessary.  I suspect it's not only not necessary, but actively wrong.
> > 
> > If that's true, removing becomes a bug fix that should be a separate
> > patch.
> > 
> > If it's only unnecessary, the removal may stay in this patch, but it
> > needs to be explained.  Separate patch might be easier to explain.  Your
> > choice.
> > 
> > If it correct and necessary, then this patch needs to be changed not to
> > drop it.  Instead, replace s->msix_used by msix_enabled(d) like you do
> > elsewhere.
> > 
> > Dmitry, can you help us find out?
> 
> Hello,
> 
> Yes, from what I see, this call is wrong and leads to
> reference leaks on device unload at migration target.
> It should be removed.

Talking of oddities in vmxnet3's msix load/save,
vmxnet3 has the honour of being the only device that
has both a register_savevm (which registers vmxnet3-msix)
and also has a ->vmsd entry (dc->vmsd = &vmstate_vmxnet3)

What's the history behind that? Is there some ordering requirement
etc about the order the two get loaded/saved?

Dave

> Best regards,
> Dmitry
> 
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Dmitry Fleytman Oct. 6, 2016, 7:33 p.m. UTC | #6
> On 6 Oct 2016, at 17:43, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:
> 
> * Dmitry Fleytman (dmitry@daynix.com) wrote:
>> 
>>> On 30 Sep 2016, at 15:08 PM, Markus Armbruster <armbru@redhat.com> wrote:
>>> 
>>> Cao jin <caoj.fnst@cn.fujitsu.com> writes:
>>> 
>>>>> On 09/29/2016 10:42 PM, Markus Armbruster wrote:
>>>>> Cao jin <caoj.fnst@cn.fujitsu.com> writes:
>>>>> 
>>>> 
>>>>>> static int vmxnet3_post_load(void *opaque, int version_id)
>>>>>> {
>>>>>>     VMXNET3State *s = opaque;
>>>>>> -    PCIDevice *d = PCI_DEVICE(s);
>>>>>> 
>>>>>>     net_tx_pkt_init(&s->tx_pkt, PCI_DEVICE(s),
>>>>>>                     s->max_tx_frags, s->peer_has_vhdr);
>>>>>>     net_rx_pkt_init(&s->rx_pkt, s->peer_has_vhdr);
>>>>>> 
>>>>>> -    if (s->msix_used) {
>>>>>> -        if  (!vmxnet3_use_msix_vectors(s, VMXNET3_MAX_INTRS)) {
>>>>>> -            VMW_WRPRN("Failed to re-use MSI-X vectors");
>>>>>> -            msix_uninit(d, &s->msix_bar, &s->msix_bar);
>>>>>> -            s->msix_used = false;
>>>>>> -            return -1;
>>>>>> -        }
>>>>>> -    }
>>>>>> -
>>>>>>     vmxnet3_validate_queues(s);
>>>>>>     vmxnet3_validate_interrupts(s);
>>>>> 
>>>>> This hunk isn't obvious.  Can you explain the change?
>>>>> 
>>>> 
>>>> flag msix_used is used in VMStateDescription.post_Load().
>>>> 
>>>> 1st, I think msix's code here is not necessary, because in
>>>> destination, device has been realized before incoming migration, So I
>>>> don't know why re-use MSI-X vectors here. Dmitry, could help to
>>>> explain?
>>>> 
>>>> 2nd, this patch is going to remove this flag, so I removed the hunk.
>>> 
>>> We need to find out whether the call of vmxnet3_use_msix_vectors() is
>>> necessary.  I suspect it's not only not necessary, but actively wrong.
>>> 
>>> If that's true, removing becomes a bug fix that should be a separate
>>> patch.
>>> 
>>> If it's only unnecessary, the removal may stay in this patch, but it
>>> needs to be explained.  Separate patch might be easier to explain.  Your
>>> choice.
>>> 
>>> If it correct and necessary, then this patch needs to be changed not to
>>> drop it.  Instead, replace s->msix_used by msix_enabled(d) like you do
>>> elsewhere.
>>> 
>>> Dmitry, can you help us find out?
>> 
>> Hello,
>> 
>> Yes, from what I see, this call is wrong and leads to
>> reference leaks on device unload at migration target.
>> It should be removed.
> 
> Talking of oddities in vmxnet3's msix load/save,
> vmxnet3 has the honour of being the only device that
> has both a register_savevm (which registers vmxnet3-msix)
> and also has a ->vmsd entry (dc->vmsd = &vmstate_vmxnet3)
> 
> What's the history behind that? Is there some ordering requirement
> etc about the order the two get loaded/saved?
> 
> Dave

Hi Dave,

There is no specific history behind that. Vmxnet3 code was written long time ago and this part is just a legacy code that was not cleaned up as QEMU code base evolved.

~Dmitry

> 
>> Best regards,
>> Dmitry
>> 
>> 
>> 
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Cao jin Oct. 11, 2016, 3:35 a.m. UTC | #7
On 10/06/2016 05:39 PM, Dmitry Fleytman wrote:
>

>
> Hello,
>
> Yes, from what I see, this call is wrong and leads to
> reference leaks on device unload at migration target.
> It should be removed.
>
> Best regards,
> Dmitry
>

Hi,Dmitry

     From what I saw, current code will call msix_vector_use twice on 
migration target, which is wrong. I am not quite follow you on 
"reference leak", are you saying the same thing as me? if not, could you 
help to explain more?
Dmitry Fleytman Oct. 11, 2016, 4:18 a.m. UTC | #8
> On 11 Oct 2016, at 6:35, Cao jin <caoj.fnst@cn.fujitsu.com> wrote:
> 
> 
> 
>> On 10/06/2016 05:39 PM, Dmitry Fleytman wrote:
>> 
> 
>> 
>> Hello,
>> 
>> Yes, from what I see, this call is wrong and leads to
>> reference leaks on device unload at migration target.
>> It should be removed.
>> 
>> Best regards,
>> Dmitry
>> 
> 
> Hi,Dmitry
> 
>    From what I saw, current code will call msix_vector_use twice on migration target, which is wrong. I am not quite follow you on "reference leak", are you saying the same thing as me? if not, could you help to explain more?

Hello,

Yes, I meant exactly this :)

msix_vector_use() increments vector reference counter each time it is called,
so device holds two references to used vectors after migration.

The second reference will be leaked because there is no second call to msix_vector_unuse() on unload.

Best Regards,
Dmitry

> 
> -- 
> Yours Sincerely,
> 
> Cao jin
> 
>
diff mbox

Patch

diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 7d44af1..1decb53 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -281,8 +281,6 @@  typedef struct {
         Vmxnet3RxqDescr rxq_descr[VMXNET3_DEVICE_MAX_RX_QUEUES];
         Vmxnet3TxqDescr txq_descr[VMXNET3_DEVICE_MAX_TX_QUEUES];
 
-        /* Whether MSI-X support was installed successfully */
-        bool msix_used;
         hwaddr drv_shmem;
         hwaddr temp_shared_guest_driver_memory;
 
@@ -359,7 +357,7 @@  static bool _vmxnet3_assert_interrupt_line(VMXNET3State *s, uint32_t int_idx)
 {
     PCIDevice *d = PCI_DEVICE(s);
 
-    if (s->msix_used && msix_enabled(d)) {
+    if (msix_enabled(d)) {
         VMW_IRPRN("Sending MSI-X notification for vector %u", int_idx);
         msix_notify(d, int_idx);
         return false;
@@ -383,7 +381,7 @@  static void _vmxnet3_deassert_interrupt_line(VMXNET3State *s, int lidx)
      * This function should never be called for MSI(X) interrupts
      * because deassertion never required for message interrupts
      */
-    assert(!s->msix_used || !msix_enabled(d));
+    assert(!msix_enabled(d));
     /*
      * This function should never be called for MSI(X) interrupts
      * because deassertion never required for message interrupts
@@ -421,7 +419,7 @@  static void vmxnet3_trigger_interrupt(VMXNET3State *s, int lidx)
     s->interrupt_states[lidx].is_pending = true;
     vmxnet3_update_interrupt_line_state(s, lidx);
 
-    if (s->msix_used && msix_enabled(d) && s->auto_int_masking) {
+    if (msix_enabled(d) && s->auto_int_masking) {
         goto do_automask;
     }
 
@@ -1427,7 +1425,7 @@  static void vmxnet3_update_features(VMXNET3State *s)
 
 static bool vmxnet3_verify_intx(VMXNET3State *s, int intx)
 {
-    return s->msix_used || msi_enabled(PCI_DEVICE(s))
+    return msix_enabled(PCI_DEVICE(s)) || msi_enabled(PCI_DEVICE(s))
         || intx == pci_get_byte(s->parent_obj.config + PCI_INTERRUPT_PIN) - 1;
 }
 
@@ -1444,18 +1442,18 @@  static void vmxnet3_validate_interrupts(VMXNET3State *s)
     int i;
 
     VMW_CFPRN("Verifying event interrupt index (%d)", s->event_int_idx);
-    vmxnet3_validate_interrupt_idx(s->msix_used, s->event_int_idx);
+    vmxnet3_validate_interrupt_idx(msix_enabled(PCI_DEVICE(s)), s->event_int_idx);
 
     for (i = 0; i < s->txq_num; i++) {
         int idx = s->txq_descr[i].intr_idx;
         VMW_CFPRN("Verifying TX queue %d interrupt index (%d)", i, idx);
-        vmxnet3_validate_interrupt_idx(s->msix_used, idx);
+        vmxnet3_validate_interrupt_idx(msix_enabled(PCI_DEVICE(s)), idx);
     }
 
     for (i = 0; i < s->rxq_num; i++) {
         int idx = s->rxq_descr[i].intr_idx;
         VMW_CFPRN("Verifying RX queue %d interrupt index (%d)", i, idx);
-        vmxnet3_validate_interrupt_idx(s->msix_used, idx);
+        vmxnet3_validate_interrupt_idx(msix_enabled(PCI_DEVICE(s)), idx);
     }
 }
 
@@ -2184,6 +2182,7 @@  vmxnet3_use_msix_vectors(VMXNET3State *s, int num_vectors)
 static bool
 vmxnet3_init_msix(VMXNET3State *s)
 {
+    bool msix;
     PCIDevice *d = PCI_DEVICE(s);
     int res = msix_init(d, VMXNET3_MAX_INTRS,
                         &s->msix_bar,
@@ -2198,17 +2197,18 @@  vmxnet3_init_msix(VMXNET3State *s)
 
     if (0 > res) {
         VMW_WRPRN("Failed to initialize MSI-X, board's MSI support is broken");
-        s->msix_used = false;
+        msix = false;
     } else {
         if (!vmxnet3_use_msix_vectors(s, VMXNET3_MAX_INTRS)) {
             VMW_WRPRN("Failed to use MSI-X vectors, error %d", res);
             msix_uninit(d, &s->msix_bar, &s->msix_bar);
-            s->msix_used = false;
+            msix = false;
         } else {
-            s->msix_used = true;
+            msix = true;
         }
     }
-    return s->msix_used;
+
+    return msix;
 }
 
 static void
@@ -2216,7 +2216,7 @@  vmxnet3_cleanup_msix(VMXNET3State *s)
 {
     PCIDevice *d = PCI_DEVICE(s);
 
-    if (s->msix_used) {
+    if (msix_enabled(d)) {
         vmxnet3_unuse_msix_vectors(s, VMXNET3_MAX_INTRS);
         msix_uninit(d, &s->msix_bar, &s->msix_bar);
     }
@@ -2551,21 +2551,11 @@  static void vmxnet3_put_rxq_descr(QEMUFile *f, void *pv, size_t size)
 static int vmxnet3_post_load(void *opaque, int version_id)
 {
     VMXNET3State *s = opaque;
-    PCIDevice *d = PCI_DEVICE(s);
 
     net_tx_pkt_init(&s->tx_pkt, PCI_DEVICE(s),
                     s->max_tx_frags, s->peer_has_vhdr);
     net_rx_pkt_init(&s->rx_pkt, s->peer_has_vhdr);
 
-    if (s->msix_used) {
-        if  (!vmxnet3_use_msix_vectors(s, VMXNET3_MAX_INTRS)) {
-            VMW_WRPRN("Failed to re-use MSI-X vectors");
-            msix_uninit(d, &s->msix_bar, &s->msix_bar);
-            s->msix_used = false;
-            return -1;
-        }
-    }
-
     vmxnet3_validate_queues(s);
     vmxnet3_validate_interrupts(s);