Re: [PATCH v6 5/9] net/zxdh: add msg chan enable implementation

[Thomas Monjalon <thomas@monjalon.net>]

16/10/2024 10:18, Junlong Wang:
> Add msg chan enable implementation to support
> send msg to get infos.

Would be interesting to explain which module is receiving the message.


> +union VPORT {

Why uppercase? for differenciate with vport below?
In general we tend to use only lowercase.

> +	uint16_t vport;
> +	struct {
> +		uint16_t vfid:8;
> +		uint16_t pfid:3;
> +		uint16_t vf_flag:1;
> +		uint16_t epid:3;
> +		uint16_t direct_flag:1;
> +	};
> +};


I am curious about the spinlock function below.
What is it doing exactly?

> +static int32_t zxdh_spinlock_lock(uint32_t virt_lock_id, uint64_t virt_addr,
> +					uint64_t label_addr, uint16_t primary_id)
> +{
> +	uint32_t lock_rd_cnt = 0;
> +
> +	do {
> +		/* read to lock */
> +		uint8_t spl_val = spinklock_read(virt_addr, virt_lock_id);

typo, it should be spinlock_read

> +
> +		if (spl_val == 0) {
> +			label_write((uint64_t)label_addr, virt_lock_id, primary_id);
> +			break;
> +		}
> +		rte_delay_us_block(SPINLOCK_POLLING_SPAN_US);
> +		lock_rd_cnt++;
> +	} while (lock_rd_cnt < MAX_HARD_SPINLOCK_ASK_TIMES);
> +	if (lock_rd_cnt >= MAX_HARD_SPINLOCK_ASK_TIMES)
> +		return -1;
> +
> +	return 0;
> +}

[...]
> +pthread_spinlock_t chan_lock;
> +static int zxdh_bar_chan_lock(uint8_t src, uint8_t dst, uint16_t src_pcieid, uint64_t virt_addr)
> +{
> +	int ret = 0;
> +	uint8_t src_index = zxdh_bar_msg_src_index_trans(src);
> +	uint8_t dst_index = zxdh_bar_msg_dst_index_trans(dst);
> +
> +	if (src_index == BAR_MSG_SRC_ERR || dst_index == BAR_MSG_DST_ERR) {
> +		PMD_MSG_LOG(ERR, "lock ERR: chan doesn't exist.");
> +		return BAR_MSG_ERR_TYPE;
> +	}
> +	uint16_t idx = lock_type_tbl[src_index][dst_index];
> +
> +	if (idx == LOCK_TYPE_SOFT)
> +		pthread_spin_lock(&chan_lock);

In general we avoid the pthread.h functions.
Do you know we have rte_spinlock_lock()?

> +	else
> +		ret = zxdh_bar_hard_lock(src_pcieid, dst, virt_addr);
> +
> +	if (ret != 0)
> +		PMD_MSG_LOG(ERR, "dev: 0x%x failed to lock.", src_pcieid);
> +
> +	return ret;
> +}

[...]
> --- a/drivers/net/zxdh/zxdh_msg.h
> +++ b/drivers/net/zxdh/zxdh_msg.h
> @@ -13,6 +13,19 @@ extern "C" {
> 
>  #include <ethdev_driver.h>
> 
> +#define ZXDH_MSIX_INTR_MSG_VEC_BASE  1
> +

You should continue using the prefix ZXDH_
These definitions below have a high risk of namespace conflict in future.
Using a namespace prefix is a good practice.

> +#define BAR_MSG_POLLING_SPAN     100
> +#define BAR_MSG_POLL_CNT_PER_MS  (1 * 1000 / BAR_MSG_POLLING_SPAN)
> +#define BAR_MSG_POLL_CNT_PER_S   (1 * 1000 * 1000 / BAR_MSG_POLLING_SPAN)
> +#define BAR_MSG_TIMEOUT_TH       (10 * 1000 * 1000 / BAR_MSG_POLLING_SPAN)
> +
> +#define BAR_CHAN_MSG_SYNC     0
> +
> +#define BAR_MSG_ADDR_CHAN_INTERVAL  (2 * 1024) /* channel size */
> +#define BAR_MSG_PLAYLOAD_OFFSET     (sizeof(struct bar_msg_header))
> +#define BAR_MSG_PAYLOAD_MAX_LEN     (BAR_MSG_ADDR_CHAN_INTERVAL -
> sizeof(struct bar_msg_header)) +
> 
>  enum DRIVER_TYPE {
>  
>      MSG_CHAN_END_MPF = 0,
>      MSG_CHAN_END_PF,
> 
> @@ -20,6 +33,13 @@ enum DRIVER_TYPE {
> 
>      MSG_CHAN_END_RISC,
>  
>  };
> 
> +enum MSG_VEC {
> +    MSIX_FROM_PFVF = ZXDH_MSIX_INTR_MSG_VEC_BASE,
> +    MSIX_FROM_MPF,
> +    MSIX_FROM_RISCV,
> +    MSG_VEC_NUM,
> +};
> +
> 
>  enum BAR_MSG_RTN {
>  
>      BAR_MSG_OK = 0,
>      BAR_MSG_ERR_MSGID,
> 
> @@ -54,10 +74,117 @@ enum BAR_MSG_RTN {
> 
>      BAR_MSG_ERR_SOCKET, /* netlink sockte err */
>  
>  };
> 
> +enum bar_module_id {
> +    BAR_MODULE_DBG = 0, /* 0:  debug */
> +    BAR_MODULE_TBL,     /* 1:  resource table */
> +    BAR_MODULE_MISX,    /* 2:  config msix */
> +    BAR_MODULE_SDA,     /* 3: */
> +    BAR_MODULE_RDMA,    /* 4: */
> +    BAR_MODULE_DEMO,    /* 5:  channel test */
> +    BAR_MODULE_SMMU,    /* 6: */
> +    BAR_MODULE_MAC,     /* 7:  mac rx/tx stats */
> +    BAR_MODULE_VDPA,    /* 8:  vdpa live migration */
> +    BAR_MODULE_VQM,     /* 9:  vqm live migration */
> +    BAR_MODULE_NP,      /* 10: vf msg callback np */
> +    BAR_MODULE_VPORT,   /* 11: get vport */
> +    BAR_MODULE_BDF,     /* 12: get bdf */
> +    BAR_MODULE_RISC_READY, /* 13: */
> +    BAR_MODULE_REVERSE,    /* 14: byte stream reverse */
> +    BAR_MDOULE_NVME,       /* 15: */
> +    BAR_MDOULE_NPSDK,      /* 16: */
> +    BAR_MODULE_NP_TODO,    /* 17: */
> +    MODULE_BAR_MSG_TO_PF,  /* 18: */
> +    MODULE_BAR_MSG_TO_VF,  /* 19: */
> +
> +    MODULE_FLASH = 32,
> +    BAR_MODULE_OFFSET_GET = 33,
> +    BAR_EVENT_OVS_WITH_VCB = 36,
> +
> +    BAR_MSG_MODULE_NUM = 100,
> +};
> +
> +struct msix_para {
> +    uint16_t pcie_id;
> +    uint16_t vector_risc;
> +    uint16_t vector_pfvf;
> +    uint16_t vector_mpf;
> +    uint64_t virt_addr;
> +    uint16_t driver_type; /* refer to DRIVER_TYPE */
> +};
> +
> +struct msix_msg {
> +    uint16_t pcie_id;
> +    uint16_t vector_risc;
> +    uint16_t vector_pfvf;
> +    uint16_t vector_mpf;
> +};
> +
> +struct zxdh_pci_bar_msg {
> +    uint64_t virt_addr; /* bar addr */
> +    void    *payload_addr;
> +    uint16_t payload_len;
> +    uint16_t emec;
> +    uint16_t src; /* refer to BAR_DRIVER_TYPE */
> +    uint16_t dst; /* refer to BAR_DRIVER_TYPE */
> +    uint16_t module_id;
> +    uint16_t src_pcieid;
> +    uint16_t dst_pcieid;
> +    uint16_t usr;
> +};
> +
> +struct bar_msix_reps {
> +    uint16_t pcie_id;
> +    uint16_t check;
> +    uint16_t vport;
> +    uint16_t rsv;
> +} __rte_packed;
> +
> +struct bar_offset_reps {
> +    uint16_t check;
> +    uint16_t rsv;
> +    uint32_t offset;
> +    uint32_t length;
> +} __rte_packed;
> +
> +struct bar_recv_msg {
> +    uint8_t reps_ok;
> +    uint16_t reps_len;
> +    uint8_t rsv;
> +    /* */
> +    union {
> +        struct bar_msix_reps msix_reps;
> +        struct bar_offset_reps offset_reps;
> +    } __rte_packed;
> +} __rte_packed;
> +
> +struct zxdh_msg_recviver_mem {
> +    void *recv_buffer; /* first 4B is head, followed by payload */
> +    uint64_t buffer_len;
> +};
> +
> +struct bar_msg_header {
> +    uint8_t valid : 1; /* used by __bar_chan_msg_valid_set/get */
> +    uint8_t sync  : 1;
> +    uint8_t emec  : 1; /* emergency? */
> +    uint8_t ack   : 1; /* ack msg? */
> +    uint8_t poll  : 1;
> +    uint8_t usr   : 1;
> +    uint8_t rsv;
> +    uint16_t module_id;
> +    uint16_t len;
> +    uint16_t msg_id;
> +    uint16_t src_pcieid;
> +    uint16_t dst_pcieid; /* used in PF-->VF */