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 0D733A0559; Mon, 16 Mar 2020 06:58:36 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id CC79D1BE51; Mon, 16 Mar 2020 06:58:34 +0100 (CET) Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by dpdk.org (Postfix) with ESMTP id 01B111AFF for ; Mon, 16 Mar 2020 06:58:33 +0100 (CET) IronPort-SDR: SQRPg2mwQtuDfLwOST5gLE2XWzfBrMNA1zY8ZkBO9vIHo1t7TmGqO1i6mHZd19IE+MjPFY0DNI F6RFYmiCq+aw== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by orsmga106.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 15 Mar 2020 22:58:32 -0700 IronPort-SDR: M/BY/f/aXFMotSAM33gMFI3AzVoi/kdhoF9vZ+0/neNLhkdQth6+pHQ05vs+L1UhW6HxDJvDT3 v34z5reMFDkw== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.70,559,1574150400"; d="scan'208";a="267455284" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by fmsmga004.fm.intel.com with ESMTP; 15 Mar 2020 22:58:32 -0700 Received: from shsmsx605.ccr.corp.intel.com (10.109.6.215) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.439.0; Sun, 15 Mar 2020 22:58:32 -0700 Received: from shsmsx603.ccr.corp.intel.com (10.109.6.143) by SHSMSX605.ccr.corp.intel.com (10.109.6.215) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5; Mon, 16 Mar 2020 13:58:29 +0800 Received: from shsmsx603.ccr.corp.intel.com ([10.109.6.143]) by SHSMSX603.ccr.corp.intel.com ([10.109.6.143]) with mapi id 15.01.1713.004; Mon, 16 Mar 2020 13:58:29 +0800 From: "Wang, Haiyue" To: "Ye, Xiaolong" CC: "dev@dpdk.org" , "Zhang, Qi Z" , "Yang, Qiming" , "Xing, Beilei" , "Zhao1, Wei" Thread-Topic: [PATCH v2 7/7] net/ice: get the VF hardware index in DCF Thread-Index: AQHV9qlYCwBkFD0dkkm3Cf37vb7BUahFlwAAgAUZwcA= Date: Mon, 16 Mar 2020 05:58:29 +0000 Message-ID: <90cc683a586a433fa0411e8b531693e4@intel.com> References: <20200309141437.11800-1-haiyue.wang@intel.com> <20200310065029.40966-1-haiyue.wang@intel.com> <20200310065029.40966-8-haiyue.wang@intel.com> <20200313070128.GF44839@intel.com> In-Reply-To: <20200313070128.GF44839@intel.com> Accept-Language: zh-CN, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-reaction: no-action dlp-version: 11.2.0.6 x-originating-ip: [10.239.127.36] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v2 7/7] net/ice: get the VF hardware index in DCF 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" > -----Original Message----- > From: Ye, Xiaolong > Sent: Friday, March 13, 2020 15:01 > To: Wang, Haiyue > Cc: dev@dpdk.org; Zhang, Qi Z ; Yang, Qiming ; Xing, > Beilei ; Zhao1, Wei > Subject: Re: [PATCH v2 7/7] net/ice: get the VF hardware index in DCF >=20 > On 03/10, Haiyue Wang wrote: > >The DCF (Device Config Function) needs the hardware index of the VFs to > >control the flow setting. And also if the VF resets, the index may be > >changed, so it should handle this in VF reset event. > > > >Signed-off-by: Haiyue Wang > >--- > > drivers/net/ice/ice_dcf.c | 81 +++++++++++++++++++++++++++++++ > > drivers/net/ice/ice_dcf.h | 4 ++ > > drivers/net/ice/ice_dcf_parent.c | 83 +++++++++++++++++++++++++++++++- > > 3 files changed, 167 insertions(+), 1 deletion(-) > > > >diff --git a/drivers/net/ice/ice_dcf.c b/drivers/net/ice/ice_dcf.c > >index 480c449f5..1d3c8fa95 100644 > >--- a/drivers/net/ice/ice_dcf.c > >+++ b/drivers/net/ice/ice_dcf.c > >@@ -269,6 +269,63 @@ ice_dcf_get_vf_resource(struct ice_dcf_hw *hw) > > return 0; > > } > > > >+static int > >+ice_dcf_get_vf_vsi_map(struct ice_dcf_hw *hw) > >+{ > >+ struct virtchnl_dcf_vsi_map *vsi_map; > >+ uint16_t len; > >+ int err; > >+ > >+ err =3D ice_dcf_send_cmd_req_no_irq(hw, VIRTCHNL_OP_DCF_GET_VSI_MAP, > >+ NULL, 0); > >+ if (err) { > >+ PMD_DRV_LOG(ERR, "Fail to send msg OP_DCF_GET_VSI_MAP"); >=20 > Better to make the err log format consistent, either "Failed to xxx" or "= Fail to xxxx", > I prefer to "Failed to xxx". >=20 Fixed in v3 with "Failed to xxx". >=20 > >+ return err; > >+ } > >+ > >+ err =3D ice_dcf_recv_cmd_rsp_no_irq(hw, VIRTCHNL_OP_DCF_GET_VSI_MAP, > >+ hw->arq_buf, ICE_DCF_AQ_BUF_SZ, > >+ &len); > >+ if (err) { > >+ PMD_DRV_LOG(ERR, "Fail to get response of OP_DCF_GET_VSI_MAP"); > >+ return err; > >+ } > >+ > >+ vsi_map =3D (struct virtchnl_dcf_vsi_map *)hw->arq_buf; > >+ if (len < sizeof(*vsi_map) || !vsi_map->num_vfs || > >+ len < sizeof(*vsi_map) + > >+ (vsi_map->num_vfs - 1) * sizeof(vsi_map->vf_vsi[0])) { >=20 > Better to use a tmp variable to make the code more readable. > And is the first len < sizeof(*vsi_map) redundant? Yes, and enhanced this with valid_msg_len for checking the length in v3. >=20 > >+ PMD_DRV_LOG(ERR, "invalid vf vsi map response with length %u", > >+ len); > >+ return -EINVAL; > >+ } > >+ > >+ if (hw->num_vfs !=3D 0 && hw->num_vfs !=3D vsi_map->num_vfs) { > >+ PMD_DRV_LOG(ERR, "The number VSI map (%u) doesn't match the number of= VFs (%u)", > >+ vsi_map->num_vfs, hw->num_vfs); > >+ return -EINVAL; > >+ } > >+ > >+ len =3D vsi_map->num_vfs * sizeof(vsi_map->vf_vsi[0]); > >+ if (!hw->vf_vsi_map) { > >+ hw->num_vfs =3D vsi_map->num_vfs; > >+ hw->vf_vsi_map =3D rte_zmalloc("vf_vsi_ctx", len, 0); > >+ } > >+ > >+ if (!hw->vf_vsi_map) { > >+ PMD_DRV_LOG(ERR, "Fail to alloc memory for VSI context"); > >+ return -ENOMEM; > >+ } >=20 > I think above two blocks can be combined with one if (!hw->vf_vsi_map). >=20 Done in v3. > >+ > >+ if (!memcmp(hw->vf_vsi_map, vsi_map->vf_vsi, len)) { > >+ PMD_DRV_LOG(DEBUG, "VF VSI map doesn't change"); > >+ return 1; > >+ } > >+ > >+ rte_memcpy(hw->vf_vsi_map, vsi_map->vf_vsi, len); > >+ return 0; > >+} > >+ > > static int > > ice_dcf_mode_disable(struct ice_dcf_hw *hw) > > { > >@@ -467,6 +524,23 @@ ice_dcf_send_aq_cmd(void *dcf_hw, struct ice_aq_des= c *desc, > > return err; > > } > > > >+int > >+ice_dcf_handle_vsi_update_event(struct ice_dcf_hw *hw) > >+{ > >+ int err =3D 0; > >+ > >+ rte_spinlock_lock(&hw->vc_cmd_send_lock); > >+ ice_dcf_disable_irq0(hw); > >+ > >+ if (ice_dcf_get_vf_resource(hw) || ice_dcf_get_vf_vsi_map(hw)) > >+ err =3D -1; > >+ > >+ ice_dcf_enable_irq0(hw); > >+ rte_spinlock_unlock(&hw->vc_cmd_send_lock); > >+ > >+ return err; > >+} > >+ > > int > > ice_dcf_init_hw(struct rte_eth_dev *eth_dev, struct ice_dcf_hw *hw) > > { > >@@ -534,6 +608,12 @@ ice_dcf_init_hw(struct rte_eth_dev *eth_dev, struct= ice_dcf_hw *hw) > > goto err_alloc; > > } > > > >+ if (ice_dcf_get_vf_vsi_map(hw) < 0) { > >+ PMD_INIT_LOG(ERR, "Failed to get VF VSI map"); > >+ ice_dcf_mode_disable(hw); > >+ goto err_alloc; > >+ } > >+ > > rte_intr_callback_register(&pci_dev->intr_handle, > > ice_dcf_dev_interrupt_handler, hw); > > rte_intr_enable(&pci_dev->intr_handle); > >@@ -566,5 +646,6 @@ ice_dcf_uninit_hw(struct rte_eth_dev *eth_dev, struc= t ice_dcf_hw *hw) > > iavf_shutdown_adminq(&hw->avf); > > > > rte_free(hw->arq_buf); > >+ rte_free(hw->vf_vsi_map); > > rte_free(hw->vf_res); > > } > >diff --git a/drivers/net/ice/ice_dcf.h b/drivers/net/ice/ice_dcf.h > >index ecd6303a0..12bef4a2a 100644 > >--- a/drivers/net/ice/ice_dcf.h > >+++ b/drivers/net/ice/ice_dcf.h > >@@ -41,6 +41,9 @@ struct ice_dcf_hw { > > > > uint8_t *arq_buf; > > > >+ uint16_t num_vfs; > >+ uint16_t *vf_vsi_map; > >+ > > struct virtchnl_version_info virtchnl_version; > > struct virtchnl_vf_resource *vf_res; /* VF resource */ > > struct virtchnl_vsi_resource *vsi_res; /* LAN VSI */ > >@@ -51,6 +54,7 @@ int ice_dcf_execute_virtchnl_cmd(struct ice_dcf_hw *hw= , > > struct dcf_virtchnl_cmd *cmd); > > int ice_dcf_send_aq_cmd(void *dcf_hw, struct ice_aq_desc *desc, > > void *buf, uint16_t buf_size); > >+int ice_dcf_handle_vsi_update_event(struct ice_dcf_hw *hw); > > int ice_dcf_init_hw(struct rte_eth_dev *eth_dev, struct ice_dcf_hw *hw)= ; > > void ice_dcf_uninit_hw(struct rte_eth_dev *eth_dev, struct ice_dcf_hw *= hw); > > > >diff --git a/drivers/net/ice/ice_dcf_parent.c b/drivers/net/ice/ice_dcf_= parent.c > >index 4c3bb68b1..2735221e9 100644 > >--- a/drivers/net/ice/ice_dcf_parent.c > >+++ b/drivers/net/ice/ice_dcf_parent.c > >@@ -5,10 +5,76 @@ > > #include > > #include > > > >+#include > >+ > > #include "ice_dcf_ethdev.h" > > > >+#define ICE_DCF_VSI_UPDATE_SERVICE_INTERVAL 100000 /* us */ > >+ > >+static __rte_always_inline void > >+ice_dcf_update_vsi_ctx(struct ice_hw *hw, uint16_t vsi_handle, > >+ uint16_t vsi_map) > >+{ > >+ struct ice_vsi_ctx *vsi_ctx; > >+ > >+ if (unlikely(vsi_handle >=3D ICE_MAX_VSI)) { > >+ PMD_DRV_LOG(ERR, "Invalid vsi handle %u", vsi_handle); > >+ return; > >+ } > >+ > >+ vsi_ctx =3D hw->vsi_ctx[vsi_handle]; > >+ > >+ if (vsi_map & VIRTCHNL_DCF_VF_VSI_VALID) { > >+ if (!vsi_ctx) > >+ vsi_ctx =3D ice_malloc(hw, sizeof(*vsi_ctx)); > >+ > >+ if (!vsi_ctx) { > >+ PMD_DRV_LOG(ERR, "No memory for vsi context %u", > >+ vsi_handle); > >+ return; > >+ } >=20 > Above two blocks can be combined. Done in v3. >=20 > >+ > >+ vsi_ctx->vsi_num =3D (vsi_map & VIRTCHNL_DCF_VF_VSI_ID_M) >> > >+ VIRTCHNL_DCF_VF_VSI_ID_S; > >+ hw->vsi_ctx[vsi_handle] =3D vsi_ctx; > >+ > >+ PMD_DRV_LOG(DEBUG, "VF%u is assigned with vsi number %u", > >+ vsi_handle, vsi_ctx->vsi_num); > >+ } else { > >+ hw->vsi_ctx[vsi_handle] =3D NULL; > >+ > >+ ice_free(hw, vsi_ctx); > >+ > >+ PMD_DRV_LOG(NOTICE, "VF%u is disabled", vsi_handle); > >+ } > >+} > >+ > >+static void > >+ice_dcf_update_vf_vsi_map(struct ice_hw *hw, uint16_t num_vfs, > >+ uint16_t *vf_vsi_map) > >+{ > >+ uint16_t vf_id; > >+ > >+ for (vf_id =3D 0; vf_id < num_vfs; vf_id++) > >+ ice_dcf_update_vsi_ctx(hw, vf_id, vf_vsi_map[vf_id]); > >+} > >+ > >+static void > >+ice_dcf_vsi_update_service_handler(void *param) > >+{ > >+ struct ice_dcf_hw *hw =3D param; > >+ > >+ if (!ice_dcf_handle_vsi_update_event(hw)) { > >+ struct ice_dcf_adapter *dcf_ad =3D > >+ container_of(hw, struct ice_dcf_adapter, real_hw); > >+ > >+ ice_dcf_update_vf_vsi_map(&dcf_ad->parent.hw, > >+ hw->num_vfs, hw->vf_vsi_map); > >+ } > >+} > >+ > > void > >-ice_dcf_handle_pf_event_msg(__rte_unused struct ice_dcf_hw *dcf_hw, > >+ice_dcf_handle_pf_event_msg(struct ice_dcf_hw *dcf_hw, > > uint8_t *msg, uint16_t msglen) > > { > > struct virtchnl_pf_event *pf_msg =3D (struct virtchnl_pf_event *)msg; > >@@ -21,6 +87,8 @@ ice_dcf_handle_pf_event_msg(__rte_unused struct ice_dc= f_hw *dcf_hw, > > switch (pf_msg->event) { > > case VIRTCHNL_EVENT_RESET_IMPENDING: > > PMD_DRV_LOG(DEBUG, "VIRTCHNL_EVENT_RESET_IMPENDING event"); > >+ rte_eal_alarm_set(ICE_DCF_VSI_UPDATE_SERVICE_INTERVAL * 2, > >+ ice_dcf_vsi_update_service_handler, dcf_hw); >=20 > Why * 2 for the VIRTCHNL_EVENT_RESET_IMPENDING event? >=20 Original thought is VF itself reset needs more work (by referring to iavf P= MD), after check, no need *2. Fixed in v3. > > break; > > case VIRTCHNL_EVENT_LINK_CHANGE: > > PMD_DRV_LOG(DEBUG, "VIRTCHNL_EVENT_LINK_CHANGE event"); > >@@ -28,6 +96,13 @@ ice_dcf_handle_pf_event_msg(__rte_unused struct ice_d= cf_hw *dcf_hw, > > case VIRTCHNL_EVENT_PF_DRIVER_CLOSE: > > PMD_DRV_LOG(DEBUG, "VIRTCHNL_EVENT_PF_DRIVER_CLOSE event"); > > break; > >+ case VIRTCHNL_EVENT_DCF_VSI_MAP_UPDATE: > >+ PMD_DRV_LOG(DEBUG, "VIRTCHNL_EVENT_DCF_VSI_MAP_UPDATE event : VF%u wi= th VSI num %u", > >+ pf_msg->event_data.vf_vsi_map.vf_id, > >+ pf_msg->event_data.vf_vsi_map.vsi_id); > >+ rte_eal_alarm_set(ICE_DCF_VSI_UPDATE_SERVICE_INTERVAL, > >+ ice_dcf_vsi_update_service_handler, dcf_hw); > >+ break; > > default: > > PMD_DRV_LOG(ERR, "Unknown event received %u", pf_msg->event); > > break; > >@@ -235,6 +310,9 @@ ice_dcf_init_parent_adapter(struct rte_eth_dev *eth_= dev) > > } > > parent_adapter->active_pkg_type =3D ice_load_pkg_type(parent_hw); > > > >+ ice_dcf_update_vf_vsi_map(parent_hw, > >+ hw->num_vfs, hw->vf_vsi_map); > >+ >=20 > No need to split into 2 lines. Done in v3. >=20 > > mac =3D (const struct rte_ether_addr *)hw->avf.mac.addr; > > if (rte_is_valid_assigned_ether_addr(mac)) > > rte_ether_addr_copy(mac, &parent_adapter->pf.dev_addr); > >@@ -259,5 +337,8 @@ ice_dcf_uninit_parent_adapter(struct rte_eth_dev *et= h_dev) > > > > eth_dev->data->mac_addrs =3D NULL; > > > >+ rte_eal_alarm_cancel(ice_dcf_vsi_update_service_handler, > >+ &adapter->real_hw); > >+ > > ice_dcf_uninit_parent_hw(parent_hw); > > } > >-- > >2.25.1 > >