From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 55825A00C3; Fri, 15 May 2020 12:07:07 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 45BA01DAF5; Fri, 15 May 2020 12:07:06 +0200 (CEST) Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [205.139.110.61]) by dpdk.org (Postfix) with ESMTP id B8F971DAF3 for ; Fri, 15 May 2020 12:07:04 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1589537224; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=kD3hChpr2JMOdXQtcVstH9iHbZ0c3Ntb+OIQWSc24PI=; b=i5MFJYCUivq3y3y0hNlzIr8ZM+jerRS7g3w85bPF+qWzMe6XY9uxsaVEvdGsDqzn4rm0/m LG5rX2aAgluz/FdmU8ZJhSRlrDpeAAgdg6lz3uEqRyK/Gak4BHbjWa3LqZh4T51/b/Regd XA3x4eZZh/MkLiolo29BAlU44fg/f30= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-20-kJY3RfIROtCt66OwjlHnQg-1; Fri, 15 May 2020 06:07:02 -0400 X-MC-Unique: kJY3RfIROtCt66OwjlHnQg-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id EAA9F83DE2E; Fri, 15 May 2020 10:07:00 +0000 (UTC) Received: from [10.72.13.11] (ovpn-13-11.pek2.redhat.com [10.72.13.11]) by smtp.corp.redhat.com (Postfix) with ESMTP id 21D3A25277; Fri, 15 May 2020 10:06:39 +0000 (UTC) To: "Wang, Xiao W" , Maxime Coquelin , "Ye, Xiaolong" , "shahafs@mellanox.com" , "matan@mellanox.com" , "amorenoz@redhat.com" , "viacheslavo@mellanox.com" , "dev@dpdk.org" Cc: "lulu@redhat.com" , "Xu, Rosen" References: <20200514080218.1435344-1-maxime.coquelin@redhat.com> <20200514080218.1435344-4-maxime.coquelin@redhat.com> <5004de61-e6f7-a38f-0836-60bcd5c4523d@redhat.com> From: Jason Wang Message-ID: <8a36b473-34f8-bca8-ccfd-73b1a7c8b6c8@redhat.com> Date: Fri, 15 May 2020 18:06:38 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: Content-Language: en-US X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH 3/9] vdpa/ifc: add support to vDPA queue enable X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 2020/5/15 下午5:42, Wang, Xiao W wrote: > > Hi, > > Best Regards, > > Xiao > > > -----Original Message----- > > > From: Jason Wang > > > Sent: Friday, May 15, 2020 5:09 PM > > > To: Maxime Coquelin ; Ye, Xiaolong > > > ; shahafs@mellanox.com; matan@mellanox.com; > > > amorenoz@redhat.com; Wang, Xiao W ; > > > viacheslavo@mellanox.com; dev@dpdk.org > > > Cc: lulu@redhat.com > > > Subject: Re: [PATCH 3/9] vdpa/ifc: add support to vDPA queue enable > > > > > > > > > On 2020/5/14 下午4:02, Maxime Coquelin wrote: > > > > This patch adds support to enabling and disabling > > > > vrings on a per-vring granularity. > > > > > > > > Signed-off-by: Maxime Coquelin > > > > > > > > > > A question here, I see in qemu peer_attach() may try to generate > > > VHOST_USER_SET_VRING_ENABLE, but just from the name I think it should > > > behave as queue_enable defined in virtio specification which is > > > explicitly under the control of guest? > > > > > > (Note, in Cindy's vDPA series, we must invent new vhost_ops to differ > > > from this one). > > From my view, common_cfg.enable reg is used for registering a queue to > hypervisor&vhost, but not ENABLE. > Well, what's your definition of "enable" in this context? Spec said: queue_enable The driver uses this to selectively prevent the device from executing requests from this virtqueue. 1 - enabled; 0 - disabled. This means, if queue_enable is not set to 1, device can not execute request for this specific virtqueue. > The control queue message VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET is for > enable/disable queue pairs. > But in qemu this is hooked to VHOST_USER_SET_VRING_ENABLE, see peer_attach(). And this patch hook VHOST_USER_SET_VRING_ENABLE to queue_enable. This means IFCVF uses queue_enable instead of control vq or other register for setting multiqueue stuff? My understanding is that IFCVF has dedicated register to do this. Note setting mq is different from queue_enable, changing the number of queues should let the underlayer NIC to properly configure its steering/switching/filtering logic to make sure traffic were only sent to the queues that is set by driver. So hooking VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET to qeue_enable looks wrong. > Think about when virtio net probes, all queues are selected and > "enabled" by init_vqs(), > I think we're talking about aligning the implementation with spec not just make it work for some specific drivers. Driver may choose to not enable a virtqueue by not setting 1 to queue_enable. Thanks > but MQ is not enabled until virtnet_set_channels() by user config with > "ethtool". > > Based on this, below reg writing is not OK to enable MQ. IFC HW > supports below registers for VF pass-thru case. > > Actually, we have specific reg designed to enable MQ in VDPA case. > > > > +IFCVF_WRITE_REG16(qid, &cfg->queue_select); > > > > +IFCVF_WRITE_REG16(enable, &cfg->queue_enable); > > BRs, > > Xiao > > > > > > Thanks > > > > > > > > > > --- > > > >drivers/vdpa/ifc/base/ifcvf.c |9 +++++++++ > > > >drivers/vdpa/ifc/base/ifcvf.h |4 ++++ > > > >drivers/vdpa/ifc/ifcvf_vdpa.c | 23 ++++++++++++++++++++++- > > > >3 files changed, 35 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/vdpa/ifc/base/ifcvf.c > b/drivers/vdpa/ifc/base/ifcvf.c > > > > index 3c0b2dff66..dd4e7468ae 100644 > > > > --- a/drivers/vdpa/ifc/base/ifcvf.c > > > > +++ b/drivers/vdpa/ifc/base/ifcvf.c > > > > @@ -327,3 +327,12 @@ ifcvf_get_queue_notify_off(struct ifcvf_hw > *hw, int > > > qid) > > > >return (u8 *)hw->notify_addr[qid] - > > > >(u8 *)hw->mem_resource[hw->notify_region].addr; > > > >} > > > > + > > > > +void > > > > +ifcvf_queue_enable(struct ifcvf_hw *hw, u16 qid,u16 enable) > > > > +{ > > > > +struct ifcvf_pci_common_cfg *cfg = hw->common_cfg; > > > > + > > > > +IFCVF_WRITE_REG16(qid, &cfg->queue_select); > > > > +IFCVF_WRITE_REG16(enable, &cfg->queue_enable); > > > > +} > > > > diff --git a/drivers/vdpa/ifc/base/ifcvf.h > b/drivers/vdpa/ifc/base/ifcvf.h > > > > index eb04a94067..bd85010eff 100644 > > > > --- a/drivers/vdpa/ifc/base/ifcvf.h > > > > +++ b/drivers/vdpa/ifc/base/ifcvf.h > > > > @@ -159,4 +159,8 @@ ifcvf_get_notify_region(struct ifcvf_hw *hw); > > > >u64 > > > >ifcvf_get_queue_notify_off(struct ifcvf_hw *hw, int qid); > > > > > > > > +void > > > > +ifcvf_queue_enable(struct ifcvf_hw *hw, u16 qid,u16 enable); > > > > + > > > > + > > > >#endif /* _IFCVF_H_ */ > > > > diff --git a/drivers/vdpa/ifc/ifcvf_vdpa.c > b/drivers/vdpa/ifc/ifcvf_vdpa.c > > > > index ec97178dcb..55ce0cf13d 100644 > > > > --- a/drivers/vdpa/ifc/ifcvf_vdpa.c > > > > +++ b/drivers/vdpa/ifc/ifcvf_vdpa.c > > > > @@ -937,6 +937,27 @@ ifcvf_dev_close(int vid) > > > >return 0; > > > >} > > > > > > > > +static int > > > > +ifcvf_set_vring_state(int vid, int vring, int state) > > > > +{ > > > > +int did; > > > > +struct internal_list *list; > > > > +struct ifcvf_internal *internal; > > > > + > > > > +did = rte_vhost_get_vdpa_device_id(vid); > > > > +list = find_internal_resource_by_did(did); > > > > +if (list == NULL) { > > > > +DRV_LOG(ERR, "Invalid device id: %d", did); > > > > +return -1; > > > > +} > > > > + > > > > +internal = list->internal; > > > > + > > > > +ifcvf_queue_enable(&internal->hw, (uint16_t)vring, (uint16_t) state); > > > > + > > > > +return 0; > > > > +} > > > > + > > > >static int > > > >ifcvf_set_features(int vid) > > > >{ > > > > @@ -1086,7 +1107,7 @@ static struct rte_vdpa_dev_ops ifcvf_ops = { > > > >.get_protocol_features = ifcvf_get_protocol_features, > > > >.dev_conf = ifcvf_dev_config, > > > >.dev_close = ifcvf_dev_close, > > > > -.set_vring_state = NULL, > > > > +.set_vring_state = ifcvf_set_vring_state, > > > >.set_features = ifcvf_set_features, > > > >.migration_done = NULL, > > > >.get_vfio_group_fd = ifcvf_get_vfio_group_fd, >