Re: [Qemu-devel] [PATCH v3 8/8] vmxnet3: remove unnecessary internal msix state flag

[Markus Armbruster]

Cao jin <address@hidden> writes:

> On 09/29/2016 10:42 PM, Markus Armbruster wrote:
>> Cao jin <address@hidden> 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?