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

[Ferruh Yigit <ferruh.yigit@amd.com>]

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.

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?

>  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.

>  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.

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.

Seconday process is only for datapath, which can access to the queues
and do packet processing.

Needing "mac_ops" in the secondary can be bad design desicion, can you
please double check it?

<...>

> 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?

> +	/* 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.

> +	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?

> +	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.


> +	port->adapt = adapter;
> +	port->hw = hw;

At this stage port is NULL, probably above needs to go to other patch
that sets 'port'.

> +	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?

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).

<...>

> +/**
> + *  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.