From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id B027542529; Wed, 6 Sep 2023 12:32:31 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 9E1524027E; Wed, 6 Sep 2023 12:32:31 +0200 (CEST) Received: from smtpbguseast1.qq.com (smtpbguseast1.qq.com [54.204.34.129]) by mails.dpdk.org (Postfix) with ESMTP id 03BDE4027C for ; Wed, 6 Sep 2023 12:32:29 +0200 (CEST) X-QQ-mid: bizesmtp83t1693996327t7zib074 Received: from LAPTOP96V0OHHN ( [183.81.182.182]) by bizesmtp.qq.com (ESMTP) with id ; Wed, 06 Sep 2023 18:32:05 +0800 (CST) X-QQ-SSF: 00400000000000D0F000000A0000000 X-QQ-FEAT: j6SxvrLnJLurf5d4+WblRf6nPgyGr5VWz1H1KRNbOYmsZlbfHn1pZH/UyqvlV xUczuvpV27E0kuKV65N+c3YS+vhpmy4/Ezsn5PoOs0CoUNOiR70dUmtSeAJtTlYwKRkRt3I SdiedIslgHljq7pFBd7sezsnyLDHuHfrNQg3eJ5S27UbP7+/kq8XLu6skvKs1egZI5njCSO xkD+e0hLMKQIXB0ZvEpac1cuM95KesNRKcO2RFFzthDQPgSUaMsL9h0HGw+8JQ8CQrtNKba MWOFP/8tmCXCUWL8/zKyDqeQpct/HWi6X5jV40xxp+GUEBzBvv/d6JK3YQzGTR38SGuvKdj kuaAxKn7BIsJEa3A1UIU3V+TvVEO+6X09+aUhi8f8qU1bcOI/J708pPx1+/hgKfnwx0bGD8 mAvKRE2G9bOyhjz5GhepUA== X-QQ-GoodBg: 2 X-BIZMAIL-ID: 3910804691032054740 From: "11" To: "'Ferruh Yigit'" Cc: , , , , "'Stephen Hemminger'" References: <20230901023050.40893-1-caowenbo@mucse.com> <20230901023050.40893-5-caowenbo@mucse.com> <4ab252f9-56d1-b9c1-1dd2-50ad1d97f4b7@amd.com> In-Reply-To: <4ab252f9-56d1-b9c1-1dd2-50ad1d97f4b7@amd.com> Subject: RE: [PATCH v6 4/8] net/rnp: add mbx basic api feature Date: Wed, 6 Sep 2023 18:32:07 +0800 Message-ID: <643DA378097977FE+013601d9e0ad$6387d2b0$2a977810$@mucse.com> MIME-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Mailer: Microsoft Outlook 16.0 Content-Language: zh-cn Thread-Index: AQI30P9XUj3aWZJ7n+uYhLh06w3v0AF8AshpAhTCgnOvNR/EwA== X-QQ-SENDSIZE: 520 Feedback-ID: bizesmtp:mucse.com:qybglogicsvrgz:qybglogicsvrgz5a-0 X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Hi Ferruh, Please see the below comment :) Regards Wenbo > -----Original Message----- > From: Ferruh Yigit > Sent: 2023=E5=B9=B49=E6=9C=885=E6=97=A5 23:45 > To: Wenbo Cao > Cc: dev@dpdk.org; thomas@monjalon.net; andrew.rybchenko@oktetlabs.ru; > yaojun@mucse.com; Stephen Hemminger > Subject: Re: [PATCH v6 4/8] net/rnp: add mbx basic api feature >=20 > On 9/1/2023 3:30 AM, Wenbo Cao wrote: > > mbx base code is for communicate with the firmware > > >=20 > I assume mbx is mailbox, can you please use full name in the patch = title and > commit log. >=20 Yes, mbx is mailbox. I will change the commit log=20 > How the parties get notified about new messages, is the interrupt = support > enabled or polling, can you please elabore the support in the commit = log? >=20 > > Signed-off-by: Wenbo Cao > > Suggested-by: Stephen Hemminger > > --- > > drivers/net/rnp/base/rnp_api.c | 23 ++ > > drivers/net/rnp/base/rnp_api.h | 7 + > > drivers/net/rnp/base/rnp_cfg.h | 7 + > > drivers/net/rnp/base/rnp_dma_regs.h | 73 ++++ > > drivers/net/rnp/base/rnp_eth_regs.h | 124 +++++++ > > drivers/net/rnp/base/rnp_hw.h | 112 +++++- >=20 > Why there is no meson file in base folder? >=20 For base folder also need meson.build ? I have add the base/.c in the rnp/meson.build. Do I need to split it to the base/meson.build ? > > drivers/net/rnp/meson.build | 1 + > > drivers/net/rnp/rnp.h | 35 ++ > > drivers/net/rnp/rnp_ethdev.c | 70 +++- > > drivers/net/rnp/rnp_logs.h | 9 + > > drivers/net/rnp/rnp_mbx.c | 522 = ++++++++++++++++++++++++++++ > > drivers/net/rnp/rnp_mbx.h | 139 ++++++++ > > drivers/net/rnp/rnp_mbx_fw.c | 271 +++++++++++++++ > > drivers/net/rnp/rnp_mbx_fw.h | 22 ++ >=20 > Should 'rnp_mbx*' files also go under base folder? I can see they use = some 'u8' > like typedef, making me think they are shared between other platforms, = if so base > folder suits better. >=20 Yes, the mbx.c or rnp_mbx_fw.c is share code, need change to the base = folder. > > 14 files changed, 1412 insertions(+), 3 deletions(-) create mode > > 100644 drivers/net/rnp/base/rnp_api.c create mode 100644 > > drivers/net/rnp/base/rnp_api.h create mode 100644 > > drivers/net/rnp/base/rnp_cfg.h create mode 100644 > > drivers/net/rnp/base/rnp_dma_regs.h > > create mode 100644 drivers/net/rnp/base/rnp_eth_regs.h > > create mode 100644 drivers/net/rnp/rnp_mbx.c create mode 100644 > > drivers/net/rnp/rnp_mbx.h create mode 100644 > > drivers/net/rnp/rnp_mbx_fw.c create mode 100644 > > drivers/net/rnp/rnp_mbx_fw.h > > > > diff --git a/drivers/net/rnp/base/rnp_api.c > > b/drivers/net/rnp/base/rnp_api.c new file mode 100644 index > > 0000000000..550da6217d > > --- /dev/null > > +++ b/drivers/net/rnp/base/rnp_api.c > > @@ -0,0 +1,23 @@ > > +#include "rnp.h" > > +#include "rnp_api.h" > > + > > +int > > +rnp_init_hw(struct rte_eth_dev *dev) > > +{ > > >=20 > Base code is mostly for works as HAL and you don't need to pass = "struct > rte_eth_dev" in this level, but mostly adapte (struct rnp_eth_adapter) = or hw > (struct rnp_hw) should be sufficient. >=20 Because of one-pcie-bdf have multiple-port, the hw only manage by the = adapter struct But the adapter struct and hw struct don't have port info(0,1,2,3). For this problem I can change 'rte_eth_dev' to struct rnp_eth_port' > I assume you are passing "struct rte_eth_dev" because "struct = rnp_mac_api" is > stored in 'process_private', this part I am not clear. >=20 > Why need 'mac_ops' pointers for secondary process? > Primary process should be doing all hw initialization, otherwise both = primary and > secondar(y|ies) trying to configure the MAC can cause unexpected = results. >=20 For secondary process support to set, I find refer driver don't have = limit for=20 'dev->dev_ops' set in SECONDARY mode. .promiscuous_enable .promiscuous_disable .allmulticast_enable .allmulticast_disable .fw_version_get > Seconday process is only for datapath, which can access to the queues = and do > packet processing. >=20 For secondary process support to set, I find refer driver don't have = limit for=20 'dev->dev_ops' set in SECONDARY mode. User can set api , if 'dev->dev_ops' set don=E2=80=99t limit. > Needing "mac_ops" in the secondary can be bad design desicion, can you = please > double check it? >=20 > <...> >=20 I refer to some exist driver, it is not special limit for SECONDARY = process for 'dev->dev_ops' set. So when I test some code fond that some api will be cause crash such as = 'get_module_eeprom' And I find this is because of function point api, the secondary process = use primary function point api address. The address for secondary is can't reach. So I try to move the mac_ops, = mbx_ops to the process_prive to resolved this problem. Primary and secondary will have it own function = point api address. > > diff --git a/drivers/net/rnp/rnp.h b/drivers/net/rnp/rnp.h index > > cab1b8e85d..6e12885877 100644 > > --- a/drivers/net/rnp/rnp.h > > +++ b/drivers/net/rnp/rnp.h > > @@ -3,6 +3,7 @@ > > */ > > #ifndef __RNP_H__ > > #define __RNP_H__ > > +#include > > > > #include "base/rnp_hw.h" > > > > @@ -14,14 +15,17 @@ > > > > struct rnp_eth_port { > > struct rnp_eth_adapter *adapt; > > + struct rnp_hw *hw; >=20 > adapter already has the 'hw', is there a specific reason not to access = it via 'port- > >adapt->hw' >=20 > > struct rte_eth_dev *eth_dev; > > } __rte_cache_aligned; > > > > struct rnp_share_ops { > > + const struct rnp_mbx_api *mbx_api; > > } __rte_cache_aligned; > > > > struct rnp_eth_adapter { > > struct rnp_hw hw; > > + uint16_t max_vfs; > > struct rte_pci_device *pdev; > > struct rte_eth_dev *eth_dev; /* primary eth_dev */ > > struct rnp_eth_port *ports[RNP_MAX_PORT_OF_PF]; @@ -34,5 +38,36 > @@ > > struct rnp_eth_adapter { > > (((struct rnp_eth_port *)((eth_dev)->data->dev_private))) > > #define RNP_DEV_TO_ADAPTER(eth_dev) \ > > ((struct rnp_eth_adapter *)(RNP_DEV_TO_PORT(eth_dev)->adapt)) > > +#define RNP_DEV_TO_HW(eth_dev) \ > > + (&((struct rnp_eth_adapter > > +*)(RNP_DEV_TO_PORT((eth_dev))->adapt))->hw) > > +#define RNP_DEV_PP_PRIV_TO_MBX_OPS(dev) \ > > + (((struct rnp_share_ops *)(dev)->process_private)->mbx_api) > > +#define RNP_DEV_TO_MBX_OPS(dev) RNP_DEV_PP_PRIV_TO_MBX_OPS(dev) > > > > +static inline void rnp_reg_offset_init(struct rnp_hw *hw) { > > + uint16_t i; > > + > > + if (hw->device_id =3D=3D RNP_DEV_ID_N10G && hw->mbx.pf_num) { > > + hw->iobar4 =3D (void *)((uint8_t *)hw->iobar4 + 0x100000); > > + hw->msix_base =3D (void *)((uint8_t *)hw->iobar4 + 0xa4000); > > + hw->msix_base =3D (void *)((uint8_t *)hw->msix_base + 0x200); > > + } else { > > + hw->msix_base =3D (void *)((uint8_t *)hw->iobar4 + 0xa4000); > > + } > > + /* =3D=3D=3D dma status/config=3D=3D=3D=3D=3D=3D */ > > + hw->link_sync =3D (void *)((uint8_t *)hw->iobar4 + 0x000c); > > + hw->dma_axi_en =3D (void *)((uint8_t *)hw->iobar4 + 0x0010); > > + hw->dma_axi_st =3D (void *)((uint8_t *)hw->iobar4 + 0x0014); > > + > > + if (hw->mbx.pf_num) > > + hw->msix_base =3D (void *)((uint8_t *)0x200); > > + /* =3D=3D=3D queue registers =3D=3D=3D */ > > + hw->dma_base =3D (void *)((uint8_t *)hw->iobar4 + 0x08000); > > + hw->veb_base =3D (void *)((uint8_t *)hw->iobar4 + 0x0); > > + hw->eth_base =3D (void *)((uint8_t *)hw->iobar4 + 0x10000); >=20 > There are lots of hardcoded values in this function, can you make them = macros? >=20 Yes, I can change it to macros. > > + /* mac */ > > + for (i =3D 0; i < RNP_MAX_HW_PORT_PERR_PF; i++) > > + hw->mac_base[i] =3D (void *)((uint8_t *)hw->iobar4 + 0x60000 + > > +0x10000 * i); } > > #endif /* __RNP_H__ */ > > diff --git a/drivers/net/rnp/rnp_ethdev.c > > b/drivers/net/rnp/rnp_ethdev.c index ae737643a7..a2dc27548a 100644 > > --- a/drivers/net/rnp/rnp_ethdev.c > > +++ b/drivers/net/rnp/rnp_ethdev.c > > @@ -8,6 +8,7 @@ > > #include > > > > #include "rnp.h" > > +#include "rnp_mbx.h" > > #include "rnp_logs.h" > > > > static int > > @@ -89,6 +90,58 @@ rnp_alloc_eth_port(struct rte_pci_device = *primary_pci, > char *name) > > return NULL; > > } > > > > +static void rnp_get_nic_attr(struct rnp_eth_adapter *adapter) { > > + RTE_SET_USED(adapter); > > +} > > + > > +static int > > +rnp_process_resource_init(struct rte_eth_dev *eth_dev) { > > + struct rnp_share_ops *share_priv; > > + > > + /* allocate process_private memory this must can't > > + * belone to the dpdk mem resource manager > > + * such as from rte_malloc or rte_dma_zone.. > > + */ > > + /* use the process_prive point to resolve secondary process > > + * use point-func. This point is per process will be safe to = cover. > > + * This will cause secondary process core-dump because of IPC > > + * Secondary will call primary process point func virt-address > > + * secondary process don't alloc user/pmd to alloc or free > > + * the memory of dpdk-mem resource it will cause hugepage > > + * mem exception > > + * be careful for secondary Process to use the share-mem of > > + * point correlation > > + */ >=20 > As commented above, I am not sure why you need this, it should be = sufficient to > have mac_ops in primary proccess. >=20 This is because of supporting=20 .promiscuous_enable .promiscuous_disable .allmulticast_enable .allmulticast_disable .fw_version_get In the secondary process > > + share_priv =3D calloc(1, sizeof(*share_priv)); > > + if (!share_priv) { > > + PMD_DRV_LOG(ERR, "calloc share_priv failed"); > > + return -ENOMEM; > > + } > > + memset(share_priv, 0, sizeof(*share_priv)); > > + eth_dev->process_private =3D share_priv; > > + > > + return 0; > > +} > > + > > +static void > > +rnp_common_ops_init(struct rnp_eth_adapter *adapter) { > > + struct rnp_share_ops *share_priv; > > + > > + share_priv =3D adapter->share_priv; > > + share_priv->mbx_api =3D &rnp_mbx_pf_ops; } > > + > > +static int > > +rnp_special_ops_init(struct rte_eth_dev *eth_dev) { > > + RTE_SET_USED(eth_dev); > > + > > + return 0; > > +} > > + > > static int > > rnp_eth_dev_init(struct rte_eth_dev *dev) { @@ -124,6 +177,20 @@ > > rnp_eth_dev_init(struct rte_eth_dev *dev) > > hw->device_id =3D pci_dev->id.device_id; > > hw->vendor_id =3D pci_dev->id.vendor_id; > > hw->device_id =3D pci_dev->id.device_id; > > + adapter->max_vfs =3D pci_dev->max_vfs; > > + ret =3D rnp_process_resource_init(dev); > > + if (ret) { > > + PMD_DRV_LOG(ERR, "share prive resource init failed"); > > + return ret; > > + } > > + adapter->share_priv =3D dev->process_private; >=20 > Isn't 'adapter' shared betwen primary and secondary ('port' is = dev_private and > 'port->adapt' is 'adapter'), so updating "adapter->share_priv" defeats = the > purpose of process_private, no? >=20 Yes just process private. dev->process_private; this dev is alloc by = 'rte_eth_dev_pci_generic_probe' So we must record the first port dev->process_private, when the secondary port of the same PF, alloc by rnp_alloc_eth_port=20 we can use lase record info 'adapter->share_priv' turn to 'dev->process_private' (alloc by rnp_alloc_eth_port). > > + rnp_common_ops_init(adapter); > > + rnp_get_nic_attr(adapter); > > + /* We need Use Device Id To Change The Resource Mode */ > > + rnp_special_ops_init(dev); > > >=20 > You can add this when it is needed, so in same patch we can see where = it is > called and how it is implemented, right now it is not possible that = what 'special' > init does. >=20 >=20 Yes, it is used to for multiple-port mode and one pcie-bdf select api. For now, I can remove the code for one pcie-bdf-one-port mode. > > + port->adapt =3D adapter; > > + port->hw =3D hw; >=20 > At this stage port is NULL, probably above needs to go to other patch = that sets > 'port'. >=20 Port(dev_private) is get from the first port (bdf) alloc by = rte_eth_dev_pci_generic_probe > > + rnp_init_mbx_ops_pf(hw); > > for (p_id =3D 0; p_id < adapter->num_ports; p_id++) { > > /* port 0 resource has been allocated When Probe */ > > if (!p_id) { > > @@ -158,11 +225,10 @@ rnp_eth_dev_init(struct rte_eth_dev *dev) > > continue; > > if (port->eth_dev) { > > rnp_dev_close(port->eth_dev); > > - rte_eth_dev_release_port(port->eth_dev); > > if (port->eth_dev->process_private) > > free(port->eth_dev->process_private); > > + rte_eth_dev_release_port(port->eth_dev); > > } > > - rte_free(port); > > } > > rte_free(adapter); > > > > diff --git a/drivers/net/rnp/rnp_logs.h b/drivers/net/rnp/rnp_logs.h > > index 1b3ee33745..f1648aabb5 100644 > > --- a/drivers/net/rnp/rnp_logs.h > > +++ b/drivers/net/rnp/rnp_logs.h > > @@ -13,6 +13,15 @@ extern int rnp_drv_logtype; #define > > RNP_PMD_DRV_LOG(level, fmt, args...) \ > > rte_log(RTE_LOG_##level, rnp_drv_logtype, \ > > "%s() " fmt, __func__, ##args) > > +#define PMD_DRV_LOG_RAW(level, fmt, args...) \ > > + rte_log(RTE_LOG_ ## level, rnp_drv_logtype, "%s(): " fmt, \ > > + __func__, ## args) > > +#define PMD_DRV_LOG(level, fmt, args...) \ > > + PMD_DRV_LOG_RAW(level, fmt "\n", ## args) > > + > > +#define RNP_PMD_LOG(level, fmt, args...) \ > > + rte_log(RTE_LOG_##level, rnp_drv_logtype, \ > > + "rnp_net: (%d) " fmt, __LINE__, ##args) >=20 > With these there are three different macros used to log in = 'rnp_drv_logtype' type: >=20 > RNP_PMD_LOG > PMD_DRV_LOG > RNP_PMD_DRV_LOG >=20 > 'RNP_PMD_DRV_LOG' and 'PMD_DRV_LOG' looks exact same, do you need all > these macros, why not stick to one? >=20 > PMD_DRV_LOG This need to remove it. > If more than one required, can you please comment what is used for = what, and > perhaps rename macros in a way that it is clear which one to use where = (as I > assume one is specific to base files). >=20 > <...> >=20 Thanks a lot, I will add comment for the macros before it. > > +/** > > + * rnp_write_mbx_pf - Places a message in the mailbox > > + * @hw: pointer to the HW structure > > + * @msg: The message buffer > > + * @size: Length of buffer > > + * @mbx_id: the VF index > > + * > > + * returns SUCCESS if it successfully copied message into the = buffer > > +**/ static int32_t rnp_write_mbx_pf(struct rnp_hw *hw, u32 *msg, > > + u16 size, enum MBX_ID mbx_id) > > +{ > > + u32 DATA_REG =3D (mbx_id =3D=3D MBX_CM3CPU) ? > > + CPU_PF_SHM_DATA : PF_VF_SHM_DATA(mbx_id); > > + u32 CTRL_REG =3D (mbx_id =3D=3D MBX_CM3CPU) ? > > + PF2CPU_MBOX_CTRL : PF2VF_MBOX_CTRL(mbx_id); > > + int32_t ret_val =3D 0; > > + u32 stat __rte_unused; > > + u16 i; > > + > > + if (size > RNP_VFMAILBOX_SIZE) { > > + RNP_PMD_LOG(ERR, "%s: size:%d should <%d\n", __func__, > > + size, RNP_VFMAILBOX_SIZE); > > + return -EINVAL; > > + } > > + > > + /* lock the mailbox to prevent pf/vf/cpu race condition */ > > + ret_val =3D rnp_obtain_mbx_lock_pf(hw, mbx_id); > > + if (ret_val) { > > + RNP_PMD_LOG(WARNING, "PF[%d] Can't Get Mbx-Lock Try > Again\n", > > + hw->function); > > + return ret_val; > > + } > > + > > + /* copy the caller specified message to the mailbox memory buffer = */ > > + for (i =3D 0; i < size; i++) { > > +#ifdef MBX_WR_DEBUG > > + mbx_pwr32(hw, DATA_REG + i * 4, msg[i]); #else > > + mbx_wr32(hw, DATA_REG + i * 4, msg[i]); #endif >=20 > Who sets this define, if not used please remove it. >=20 This need to remove =F0=9F=98=8A.