From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from dpdk.org (dpdk.org [92.243.14.124])
	by inbox.dpdk.org (Postfix) with ESMTP id 91F23A00D7;
	Thu, 31 Oct 2019 18:49:35 +0100 (CET)
Received: from [92.243.14.124] (localhost [127.0.0.1])
	by dpdk.org (Postfix) with ESMTP id EA59B1C2E1;
	Thu, 31 Oct 2019 18:49:34 +0100 (CET)
Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28])
 by dpdk.org (Postfix) with ESMTP id 2D7301C2DD
 for <dev@dpdk.org>; Thu, 31 Oct 2019 18:49:33 +0100 (CET)
Received: from mail-wr1-f72.google.com (mail-wr1-f72.google.com
 [209.85.221.72])
 (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits))
 (No client certificate requested)
 by mx1.redhat.com (Postfix) with ESMTPS id 3E5D687634
 for <dev@dpdk.org>; Thu, 31 Oct 2019 17:49:32 +0000 (UTC)
Received: by mail-wr1-f72.google.com with SMTP id 7so3860542wrl.2
 for <dev@dpdk.org>; Thu, 31 Oct 2019 10:49:32 -0700 (PDT)
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
 d=1e100.net; s=20161025;
 h=x-gm-message-state:to:cc:references:from:autocrypt:subject
 :message-id:date:user-agent:mime-version:in-reply-to
 :content-language:content-transfer-encoding;
 bh=/OFiX39/uTBD1CR6mFYMSxw/DbdnnMhhczLztL6gmHg=;
 b=ejpvCtA6H4lbxKHwQZQRsHitPA15rsa9pKw6zZrUJIcXMCBH0lICwLCfWjy9zRCRS2
 FBlBnBTcO+nOdHXGZSMH6QeswJxG2bb9qSqUVpN1F4Ga1TnFFd9FG2OfM6YO7RsyuqsT
 0mOehMx4b7PRpFtvypCpNoKL5ZkJ7p4GNksHPjbKBFs5nIbvabMToUgpyAiBonXYdXgF
 /dVGH/0TzyC6RNhiPeme6IPZ5Wkx1t8V9YK4WvZ7rtGTPZQGm/BP4i5d0OA9/QEMjpP3
 WL6r0eqvvCb3PsAyr3qRx3xgYbfo6e+tO8Hj3VKH/XUBFiE9BaorcbpPEw9XSgsg2ox3
 YB6A==
X-Gm-Message-State: APjAAAXep3QWgHOfhtaQXqoqzGx8wuljk+sKLfDGktV9CNfyuXz653N6
 ol1DvbEydvXHJrwiTh/iNSfnbuAIsRQZvfFEFGWbR1qnjW7woh000CLrCsOdQJm+umUwq85IZXw
 Vr9Q=
X-Received: by 2002:a05:600c:210b:: with SMTP id
 u11mr4517526wml.170.1572544170415; 
 Thu, 31 Oct 2019 10:49:30 -0700 (PDT)
X-Google-Smtp-Source: APXvYqz1djGm+8MO1vRqpBrQShSPEGOMKl5Ve3VCZPUv+5QXhGrqHJKkMHSwAAMlboXlkt0TyWH/YA==
X-Received: by 2002:a05:600c:210b:: with SMTP id
 u11mr4517505wml.170.1572544170192; 
 Thu, 31 Oct 2019 10:49:30 -0700 (PDT)
Received: from amorenoz.users.ipa.redhat.com ([46.222.48.191])
 by smtp.gmail.com with ESMTPSA id s21sm5807326wrb.31.2019.10.31.10.48.34
 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);
 Thu, 31 Oct 2019 10:49:29 -0700 (PDT)
To: "Liu, Yong" <yong.liu@intel.com>, "Bie, Tiwei" <tiwei.bie@intel.com>
Cc: "maxime.coquelin@redhat.com" <maxime.coquelin@redhat.com>,
 "Wang, Zhihong" <zhihong.wang@intel.com>, "dev@dpdk.org" <dev@dpdk.org>
References: <20191030110723.20195-1-yong.liu@intel.com>
 <20191030145602.1948-1-yong.liu@intel.com> <20191031104220.GA22299@___>
 <86228AFD5BCD8E4EBFD2B90117B5E81E633EE261@SHSMSX103.ccr.corp.intel.com>
From: Adrian Moreno <amorenoz@redhat.com>
Autocrypt: addr=amorenoz@redhat.com; prefer-encrypt=mutual; keydata=
 mQENBF1syNUBCADQ9dk3fDMxOZ/+OQpmbanpodYxEv8IRtDz8PXw8YX7UyGfozOpLjQ8Fftj
 ZxuubYNbt2QVbSgviFilFdNWu2eTnN/JaGtfhmTOLPVoakkPHZF8lbgImMoch7L0fH8wN2IM
 KPxQyPNlX+K9FD5brHsV1lfe1TwAxmhcvLW8yNrVq+9eDIDykxc7tH4exIqXgZroahGxMHKy
 c8Ti2kJka/t6pDfRaY0J+6J7I1nrn6GXXSMNA45EH8+0N/QlcXhP3rfftnoPeVmpjswzvJqY
 FNjf/Q5VPLx7RX0Qx+y8mMB2JcChV5Bl7D7x5EUbItj6+Sy7QfOgCtPegk9HSrBCNYaLABEB
 AAG0I0FkcmlhbiBNb3Jlbm8gPGFtb3Jlbm96QHJlZGhhdC5jb20+iQFUBBMBCAA+FiEEogUD
 gihhmbOPHy26d5C5fbYeFsUFAl1syNUCGwMFCQHhM4AFCwkIBwIGFQoJCAsCBBYCAwECHgEC
 F4AACgkQd5C5fbYeFsX7qwgArGHSkX+ILNcujkVzjTG4OtkpJMPFlkn/1PxSEKD0jLuzx14B
 COzpg/Mqj3Re/QBuOas+ci9bsUA0/2nORtmmEBvzDOJpR5FH1jaGCx8USlY4WM6QqEDNZgTw
 hsy9KhjFzFjMk+oo3HyItXA+Uq9yrRBTjNBGTXxezMRcMuUZ4MIAfY0IRBglL2BufiuL43jD
 BvTENNFLoQ/wFV7qkFWSkv+8IjTsxr7M6XUo1QLd29Hn0dvwssN579HL1+BP46i2REpzeBEG
 L75iVChi+YnIQQNMJ9NYarVabZx4Y1Gn8+7B/1SNArDV+IDgnYgt7E58otoV2Ap310dmtuvE
 VbxGpbkBDQRdbMjVAQgAqyp9oA7WDu7/Y9T4Ommt69iZx8os7shUIfdgPEy5xrcPn6qGwN1/
 HQ4j8nWfBG9uuX1X0RXUZIUEtYTxtED4yaCQMTqDUf9cBAwAA2mYxBfoiNYx8YqxM+sT0/J4
 2qmDd+y+20UR4yzHE8AmIbspTzDFIJDAi+jKSR8F355z0sfW7CIMDC4ZWrPsskjEy1YN/U10
 r6tRRH1kNyrCSbTG0d9MtcQO58h7DLXuzUhErB+BtG52A04t5cweIJTJC+koV5XPeilzlHnm
 RFoj0ncruGa9Odns21BNt3cy9wLfK+aUnWuAB1uc6bJGQPiAwjkilz7g7MBRUuIQ2Zt7HGLc
 SwARAQABiQE8BBgBCAAmFiEEogUDgihhmbOPHy26d5C5fbYeFsUFAl1syNUCGwwFCQHhM4AA
 CgkQd5C5fbYeFsUlSwf8CH+u/IXaE7WeWxwFkMaORfW8cM4q0xrL3M6yRGuQNW+kMjnrvK9U
 J9G+L1/5uTRbDQ/4LdoKqize8LjehA+iF6ba4t9Npikh8fLKWgaJfQ/hPhH4C3O5gWPOLTW6
 ylGxiuER4CdFwQIoAMMslhFA7G+teeOKBq36E+1+zrybI6Xy1UBSlpDK9j4CtTnMQejjuSQb
 Qhle+l8VroaUHq869wjAhRHHhqmtJKggI+OvzgQpDIwfHIDypb1BuKydi2W6cVYEALUYyCLS
 dTBDhzj8zR5tPCsga8J7+TclQzkWOiI2C6ZtiWrMsL/Uym3uXk5nsoc7lSj7yLd/MrBRhYfP JQ==
Message-ID: <ce8a74ec-b475-d828-edf7-4e80b6c37e5c@redhat.com>
Date: Thu, 31 Oct 2019 18:47:49 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.1.1
MIME-Version: 1.0
In-Reply-To: <86228AFD5BCD8E4EBFD2B90117B5E81E633EE261@SHSMSX103.ccr.corp.intel.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Subject: Re: [dpdk-dev] [PATCH v3] vhost: fix vhost user virtqueue not
	accessible
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

Hi Marvin,

On 10/31/19 3:54 PM, Liu, Yong wrote:
> 
> 
>> -----Original Message-----
>> From: Bie, Tiwei
>> Sent: Thursday, October 31, 2019 6:42 PM
>> To: Liu, Yong <yong.liu@intel.com>
>> Cc: maxime.coquelin@redhat.com; Wang, Zhihong <zhihong.wang@intel.com>;
>> amorenoz@redhat.com; dev@dpdk.org
>> Subject: Re: [PATCH v3] vhost: fix vhost user virtqueue not accessible
>>
>> On Wed, Oct 30, 2019 at 10:56:02PM +0800, Marvin Liu wrote:
>>> Log feature is disabled in vhost user, so that log address was invalid
>>> when checking. Check whether log address is valid can workaround it.
>>> Also log address should be translated in packed ring virtqueue.
>>>
>>> Fixes: 04cfc7fdbfca ("vhost: translate incoming log address to gpa")
>>>
>>> Signed-off-by: Marvin Liu <yong.liu@intel.com>
>>> ---
>>>  lib/librte_vhost/vhost_user.c | 30 +++++++++++++-----------------
>>>  1 file changed, 13 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/lib/librte_vhost/vhost_user.c
>> b/lib/librte_vhost/vhost_user.c
>>> index 61ef699ac..7754d2467 100644
>>> --- a/lib/librte_vhost/vhost_user.c
>>> +++ b/lib/librte_vhost/vhost_user.c
>>> @@ -641,11 +641,23 @@ translate_ring_addresses(struct virtio_net *dev,
>> int vq_index)
>>>  	struct vhost_vring_addr *addr = &vq->ring_addrs;
>>>  	uint64_t len, expected_len;
>>>
>>> +	dev = numa_realloc(dev, vq_index);
>>
>> We need to update `vq->desc` first before doing numa_realloc.
>> https://github.com/DPDK/dpdk/blob/19397c7bf2545e6adab41b657a1f1da3c7344e7b/
>> lib/librte_vhost/vhost_user.c#L445
>>
>>> +	vq = dev->virtqueue[vq_index];
>>> +	if (addr->flags & (1 << VHOST_VRING_F_LOG)) {
>>
I fear the possible consequences of this change.
Before 04cfc7fdbfca the approach was "best-effort". The log address would be
assigned without further checks:

	vq->log_guest_addr = addr->log_guest_addr;

Then, the behavior changed and an error was generated if the log address was
invalid, which I guess is the problem you have hit:

	vq->log_guest_addr =
		translate_log_addr(dev, vq, addr->log_guest_addr);
	if (vq->log_guest_addr == 0) {
		RTE_LOG(DEBUG, VHOST_CONFIG,
			"(%d) failed to map log_guest_addr .\n",
			dev->vid);
		return dev;
	}

In the tests I ran I always saw valid log addresses being sent at ring
initialization phase, but if, as you claim, it's possible that invalid addresses
are given at initialization phase, maybe we should go back to "best-effort"
(i.e: remove the return statement)

But it's unlikely that qemu has enabled logging at ring initialization so this
would effectively disable the translation at the initialization phase. I cannot
forecast the consequences of this change without deeper analysis.

>> `vq` can be reallocated by numa_realloc.
>> We need to update the `addr` pointer before using it.
>>
> 
> Hi Tiwei,
> Numa_realloc function will copy data from original vq structure to new vq when reallocating.
> The content of vhost_ring_addr will be the same in new and old vqs, it may not be necessary to update pointer.
That's true but 'addr' still holds a pointer to the old structure, assigned at
line 641.

Also, note Tiwei's comment regarding updating 'vq->desc'. The idea behind
numa_realloc is to reallocate the vhost_virtqueue structure to the same numa
node as the descriptor ring. This function is updating the descriptor rings, so
I think the idea is to update the ring addresses and then reallocate the
virtqueue structure if needed.

Thanks,
Adrian

> Regards,
> Marvin
> 
>> Thanks,
>> Tiwei
>>
>>
>>> +		vq->log_guest_addr =
>>> +			translate_log_addr(dev, vq, addr->log_guest_addr);
>>> +		if (vq->log_guest_addr == 0) {
>>> +			RTE_LOG(DEBUG, VHOST_CONFIG,
>>> +					"(%d) failed to map log_guest_addr.\n",
>>> +					dev->vid);
>>> +			return dev;
>>> +		}
>>> +	}
>>> +
>>>  	if (vq_is_packed(dev)) {
>>>  		len = sizeof(struct vring_packed_desc) * vq->size;
>>>  		vq->desc_packed = (struct vring_packed_desc *)(uintptr_t)
>>>  			ring_addr_to_vva(dev, vq, addr->desc_user_addr, &len);
>>> -		vq->log_guest_addr = 0;
>>>  		if (vq->desc_packed == NULL ||
>>>  				len != sizeof(struct vring_packed_desc) *
>>>  				vq->size) {
>>> @@ -655,10 +667,6 @@ translate_ring_addresses(struct virtio_net *dev, int
>> vq_index)
>>>  			return dev;
>>>  		}
>>>
>>> -		dev = numa_realloc(dev, vq_index);
>>> -		vq = dev->virtqueue[vq_index];
>>> -		addr = &vq->ring_addrs;
>>> -
>>>  		len = sizeof(struct vring_packed_desc_event);
>>>  		vq->driver_event = (struct vring_packed_desc_event *)
>>>  					(uintptr_t)ring_addr_to_vva(dev,
>>> @@ -701,10 +709,6 @@ translate_ring_addresses(struct virtio_net *dev, int
>> vq_index)
>>>  		return dev;
>>>  	}
>>>
>>> -	dev = numa_realloc(dev, vq_index);
>>> -	vq = dev->virtqueue[vq_index];
>>> -	addr = &vq->ring_addrs;
>>> -
>>>  	len = sizeof(struct vring_avail) + sizeof(uint16_t) * vq->size;
>>>  	if (dev->features & (1ULL << VIRTIO_RING_F_EVENT_IDX))
>>>  		len += sizeof(uint16_t);
>>> @@ -741,14 +745,6 @@ translate_ring_addresses(struct virtio_net *dev, int
>> vq_index)
>>>  		vq->last_avail_idx = vq->used->idx;
>>>  	}
>>>
>>> -	vq->log_guest_addr =
>>> -		translate_log_addr(dev, vq, addr->log_guest_addr);
>>> -	if (vq->log_guest_addr == 0) {
>>> -		RTE_LOG(DEBUG, VHOST_CONFIG,
>>> -			"(%d) failed to map log_guest_addr .\n",
>>> -			dev->vid);
>>> -		return dev;
>>> -	}
>>>  	vq->access_ok = 1;
>>>
>>>  	VHOST_LOG_DEBUG(VHOST_CONFIG, "(%d) mapped address desc: %p\n",
>>> --
>>> 2.17.1
>>>