RE: [PATCH v6 4/8] net/rnp: add mbx basic api feature

["11" <caowenbo@mucse.com>]

Hi Ferruh,

Please see the below comment :)

Regards Wenbo

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@amd.com>
> Sent: 2023年9月5日 23:45
> To: Wenbo Cao <caowenbo@mucse.com>
> Cc: dev@dpdk.org; thomas@monjalon.net; andrew.rybchenko@oktetlabs.ru;
> yaojun@mucse.com; Stephen Hemminger <stephen@networkplumber.org>
> Subject: Re: [PATCH v6 4/8] net/rnp: add mbx basic api feature
> 
> On 9/1/2023 3:30 AM, Wenbo Cao wrote:
> > mbx base code is for communicate with the firmware
> >
> 
> I assume mbx is mailbox, can you please use full name in the patch title and
> commit log.
> 
Yes, mbx is mailbox.
I will change the commit log 
> 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?
> 
> > Signed-off-by: Wenbo Cao <caowenbo@mucse.com>
> > Suggested-by: Stephen Hemminger <stephen@networkplumber.org>
> > ---
> >  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 +++++-
> 
> Why there is no meson file in base folder?
> 
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 ++
> 
> 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.
> 
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)
> > +{
> >
> 
> 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.
> 
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.
> 
> 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.
> 
For secondary process support to set, I find refer driver don't have limit for 
'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.
> 
For secondary process support to set, I find refer driver don't have limit for 
'dev->dev_ops' set in SECONDARY mode.
User can set api , if 'dev->dev_ops' set don’t  limit.
> Needing "mac_ops" in the secondary can be bad design desicion, can you please
> double check it?
> 
> <...>
> 
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 <rte_log.h>
> >
> >  #include "base/rnp_hw.h"
> >
> > @@ -14,14 +15,17 @@
> >
> >  struct rnp_eth_port {
> >  	struct rnp_eth_adapter *adapt;
> > +	struct rnp_hw *hw;
> 
> adapter already has the 'hw', is there a specific reason not to access it via 'port-
> >adapt->hw'
> 
> >  	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 == RNP_DEV_ID_N10G && hw->mbx.pf_num) {
> > +		hw->iobar4 = (void *)((uint8_t *)hw->iobar4 + 0x100000);
> > +		hw->msix_base = (void *)((uint8_t *)hw->iobar4 + 0xa4000);
> > +		hw->msix_base = (void *)((uint8_t *)hw->msix_base + 0x200);
> > +	} else {
> > +		hw->msix_base = (void *)((uint8_t *)hw->iobar4 + 0xa4000);
> > +	}
> > +	/* === dma status/config====== */
> > +	hw->link_sync = (void *)((uint8_t *)hw->iobar4 + 0x000c);
> > +	hw->dma_axi_en = (void *)((uint8_t *)hw->iobar4 + 0x0010);
> > +	hw->dma_axi_st = (void *)((uint8_t *)hw->iobar4 + 0x0014);
> > +
> > +	if (hw->mbx.pf_num)
> > +		hw->msix_base = (void *)((uint8_t *)0x200);
> > +	/* === queue registers === */
> > +	hw->dma_base = (void *)((uint8_t *)hw->iobar4 + 0x08000);
> > +	hw->veb_base = (void *)((uint8_t *)hw->iobar4 + 0x0);
> > +	hw->eth_base = (void *)((uint8_t *)hw->iobar4 + 0x10000);
> 
> There are lots of hardcoded values in this function, can you make them macros?
> 
Yes, I can change it to macros.
> > +	/* mac */
> > +	for (i = 0; i < RNP_MAX_HW_PORT_PERR_PF; i++)
> > +		hw->mac_base[i] = (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 <ethdev_driver.h>
> >
> >  #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
> > +	 */
> 
> As commented above, I am not sure why you need this, it should be sufficient to
> have mac_ops in primary proccess.
> 
This is because of supporting 
 .promiscuous_enable
 .promiscuous_disable
 .allmulticast_enable
 .allmulticast_disable
 .fw_version_get
In the secondary process
> > +	share_priv = 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 = share_priv;
> > +
> > +	return 0;
> > +}
> > +
> > +static void
> > +rnp_common_ops_init(struct rnp_eth_adapter *adapter) {
> > +	struct rnp_share_ops *share_priv;
> > +
> > +	share_priv = adapter->share_priv;
> > +	share_priv->mbx_api = &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 = pci_dev->id.device_id;
> >  	hw->vendor_id = pci_dev->id.vendor_id;
> >  	hw->device_id = pci_dev->id.device_id;
> > +	adapter->max_vfs = pci_dev->max_vfs;
> > +	ret = rnp_process_resource_init(dev);
> > +	if (ret) {
> > +		PMD_DRV_LOG(ERR, "share prive resource init failed");
> > +		return ret;
> > +	}
> > +	adapter->share_priv = dev->process_private;
> 
> 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?
> 
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 
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);
> >
> 
> 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.
> 
> 
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 = adapter;
> > +	port->hw = hw;
> 
> At this stage port is NULL, probably above needs to go to other patch that sets
> 'port'.
> 
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 = 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)
> 
> With these there are three different macros used to log in 'rnp_drv_logtype' type:
> 
> RNP_PMD_LOG
> PMD_DRV_LOG
> RNP_PMD_DRV_LOG
> 
> 'RNP_PMD_DRV_LOG' and 'PMD_DRV_LOG' looks exact same, do you need all
> these macros, why not stick to one?
> 
> 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).
> 
> <...>
> 
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 = (mbx_id == MBX_CM3CPU) ?
> > +		CPU_PF_SHM_DATA : PF_VF_SHM_DATA(mbx_id);
> > +	u32 CTRL_REG = (mbx_id == MBX_CM3CPU) ?
> > +		PF2CPU_MBOX_CTRL : PF2VF_MBOX_CTRL(mbx_id);
> > +	int32_t ret_val = 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 = 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 = 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
> 
> Who sets this define, if not used please remove it.
> 
This need to remove 😊.