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

[Dmitry Fleytman]

> On 6 Oct 2016, at 17:43, Dr. David Alan Gilbert <address@hidden> wrote:
> 
> * Dmitry Fleytman (address@hidden) wrote:
>> 
>>> On 30 Sep 2016, at 15:08 PM, Markus Armbruster <address@hidden> wrote:
>>> 
>>> 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?
>> 
>> 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 / address@hidden / Manchester, UK