From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:55883)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <armbru@redhat.com>) id 1bpxYj-0004Kl-0G
	for qemu-devel@nongnu.org; Fri, 30 Sep 2016 09:09:04 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <armbru@redhat.com>) id 1bpxYd-000613-Uy
	for qemu-devel@nongnu.org; Fri, 30 Sep 2016 09:08:59 -0400
Received: from mx1.redhat.com ([209.132.183.28]:33068)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <armbru@redhat.com>) id 1bpxYd-00060U-OP
	for qemu-devel@nongnu.org; Fri, 30 Sep 2016 09:08:55 -0400
From: Markus Armbruster <armbru@redhat.com>
References: <1473839464-8670-1-git-send-email-caoj.fnst@cn.fujitsu.com>
	<1473839464-8670-9-git-send-email-caoj.fnst@cn.fujitsu.com>
	<87k2du7oo7.fsf@dusky.pond.sub.org> <57EE0CFB.4070002@cn.fujitsu.com>
Date: Fri, 30 Sep 2016 15:08:52 +0200
In-Reply-To: <57EE0CFB.4070002@cn.fujitsu.com> (Cao jin's message of "Fri, 30
	Sep 2016 14:58:03 +0800")
Message-ID: <878tu9ttzv.fsf@dusky.pond.sub.org>
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [Qemu-devel] [PATCH v3 8/8] vmxnet3: remove unnecessary
 internal msix state flag
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel/>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Cao jin <caoj.fnst@cn.fujitsu.com>
Cc: Dmitry Fleytman <dmitry@daynix.com>, Marcel Apfelbaum <marcel@redhat.com>, Jason Wang <jasowang@redhat.com>, qemu-devel@nongnu.org, "Michael S. Tsirkin" <mst@redhat.com>

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?