From mboxrd@z Thu Jan  1 00:00:00 1970
From: Maxime Coquelin <maxime.coquelin@redhat.com>
Subject: Re: [PATCH] vhost/crypto: handle virtually
	non-contiguous buffers
Date: Wed, 16 May 2018 11:46:24 +0200
Message-ID: <7a8e99e2-de29-2cd6-0df5-faf64aaff8fa@redhat.com>
References: <20180510154122.85407-1-roy.fan.zhang@intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
To: Fan Zhang <roy.fan.zhang@intel.com>, dev@dpdk.org
Return-path: <dev-bounces@dpdk.org>
Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73])
 by dpdk.org (Postfix) with ESMTP id 487331B3CF
 for <dev@dpdk.org>; Wed, 16 May 2018 11:46:27 +0200 (CEST)
In-Reply-To: <20180510154122.85407-1-roy.fan.zhang@intel.com>
Content-Language: en-US
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>



On 05/10/2018 05:41 PM, Fan Zhang wrote:
> This patch enables the handling of buffers non-contiguous in
> virtual address space in the vhost_crypto. Instead of using
> rte_vhost_va_from_guest_pa(), the host virtual address is
> converted by vhost_iova_to_vva() for wider use cases.
> 
> For copy mode, the copy length is limited to the chunck size,
> next chuncks VAs being fetched afterward.
> 
> Signed-off-by: Fan Zhang <roy.fan.zhang@intel.com>
> ---
>   lib/librte_vhost/vhost_crypto.c | 201 ++++++++++++++++++++++++++--------------
>   1 file changed, 129 insertions(+), 72 deletions(-)
...
> +	req = get_data_ptr(vc_req, &desc, sizeof(*req), VHOST_ACCESS_RO);
> +	if (unlikely(req == NULL))

Adding braces around the switch would be less error prone IMHO.

> +		switch (vcrypto->option) {
> +		case RTE_VHOST_CRYPTO_ZERO_COPY_ENABLE:
> +			err = VIRTIO_CRYPTO_BADMSG;
> +			VC_LOG_ERR("Invalid descriptor");
> +			goto error_exit;
> +		case RTE_VHOST_CRYPTO_ZERO_COPY_DISABLE:
> +			req = &tmp_req;
> +			if (unlikely(copy_data(req, vc_req, &desc, sizeof(*req))
> +					< 0)) {
> +				err = VIRTIO_CRYPTO_BADMSG;
> +				VC_LOG_ERR("Invalid descriptor");
> +				goto error_exit;
> +			}
> +			break;
> +		default:
> +			err = VIRTIO_CRYPTO_ERR;
> +			VC_LOG_ERR("Invalid option");
> +			goto error_exit;
> +		}
>   

Other than than, it looks good to me.

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

I will add the braces when applying.

Thanks,
Maxime