Hi Ferruh,
Please see the below comment :)
Regards Wenbo
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yi...@amd.com>
> Sent: 2023年9月5日 23:45
> To: Wenbo Cao <caowe...@mucse.com>
> Cc: dev@dpdk.org; tho...@monjalon.net; andrew.rybche...@oktetlabs.ru;
> yao...@mucse.com; Stephen Hemminger <step...@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 <caowe...@mucse.com>
> > Suggested-by: Stephen Hemminger <step...@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 😊.
