[PATCH v2 05/19] net/xsc: add ioctl command interface

[PATCH v2 05/19] net/xsc: add ioctl command interface

[WanRenyong]

IOCTL command interface is one of methods used to interact with
firmware by PMD. By using ioctl interface, PMD sends command to
the kernel module, then the kernel module translates the command
and sends it to firmware, at last, the kernel module send back
PDM the result from firmware.

Signed-off-by: WanRenyong <wanry@yunsilicon.com>
---
 drivers/net/xsc/meson.build |  1 +
 drivers/net/xsc/xsc_ctrl.c  | 56 ++++++++++++++++++++++++
 drivers/net/xsc/xsc_ctrl.h  | 86 +++++++++++++++++++++++++++++++++++++
 3 files changed, 143 insertions(+)
 create mode 100644 drivers/net/xsc/xsc_ctrl.c
 create mode 100644 drivers/net/xsc/xsc_ctrl.h

diff --git a/drivers/net/xsc/meson.build b/drivers/net/xsc/meson.build
index 8bf6ee7b47..f38ebdfe0f 100644
--- a/drivers/net/xsc/meson.build
+++ b/drivers/net/xsc/meson.build
@@ -10,6 +10,7 @@ sources = files(
         'xsc_ethdev.c',
         'xsc_dev.c',
         'xsc_utils.c',
+        'xsc_ctrl.c',
 )
 
 libnames = ['ibverbs']
diff --git a/drivers/net/xsc/xsc_ctrl.c b/drivers/net/xsc/xsc_ctrl.c
new file mode 100644
index 0000000000..3e37bd914e
--- /dev/null
+++ b/drivers/net/xsc/xsc_ctrl.c
@@ -0,0 +1,56 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright 2024 Yunsilicon Technology Co., Ltd.
+ */
+
+#include <stddef.h>
+#include <unistd.h>
+#include <string.h>
+#include <stdint.h>
+#include <stdlib.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <unistd.h>
+#include <sys/mman.h>
+
+#include <rte_common.h>
+#include <rte_malloc.h>
+#include <rte_memcpy.h>
+
+#include "xsc_log.h"
+#include "xsc_dev.h"
+#include "xsc_ctrl.h"
+
+int
+xsc_ioctl(struct xsc_dev *dev, int cmd, int opcode,
+	  void *data_in, int in_len, void *data_out, int out_len)
+{
+	struct xsc_ioctl_hdr *hdr;
+	int data_len = RTE_MAX(in_len, out_len);
+	int alloc_len = sizeof(struct xsc_ioctl_hdr) + data_len;
+	int ret = 0;
+
+	hdr = rte_zmalloc(NULL, alloc_len, RTE_CACHE_LINE_SIZE);
+	if (hdr == NULL) {
+		PMD_DRV_LOG(ERR, "Failed to allocate xsc ioctl cmd memory");
+		return -ENOMEM;
+	}
+
+	hdr->check_field = XSC_IOCTL_CHECK_FIELD;
+	hdr->attr.opcode = opcode;
+	hdr->attr.length = data_len;
+	hdr->attr.error = 0;
+
+	if (data_in != NULL && in_len > 0)
+		rte_memcpy(hdr + 1, data_in, in_len);
+
+	ret = ioctl(dev->ctrl_fd, cmd, hdr);
+	if (ret == 0) {
+		if (hdr->attr.error != 0)
+			ret = hdr->attr.error;
+		else if (data_out != NULL && out_len > 0)
+			rte_memcpy(data_out, hdr + 1, out_len);
+	}
+
+	rte_free(hdr);
+	return ret;
+}
diff --git a/drivers/net/xsc/xsc_ctrl.h b/drivers/net/xsc/xsc_ctrl.h
new file mode 100644
index 0000000000..d343e1b1a7
--- /dev/null
+++ b/drivers/net/xsc/xsc_ctrl.h
@@ -0,0 +1,86 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright 2024 Yunsilicon Technology Co., Ltd.
+ */
+
+#ifndef _XSC_CTRL_H_
+#define _XSC_CTRL_H_
+
+#include <sys/ioctl.h>
+
+#define XSC_IOCTL_CHECK_FIELD	0x01234567
+
+#define XSC_IOCTL_MAGIC	0x1b
+#define XSC_IOCTL_CMDQ \
+	_IOWR(XSC_IOCTL_MAGIC, 1, struct xsc_ioctl_hdr)
+#define XSC_IOCTL_DRV_GET \
+	_IOR(XSC_IOCTL_MAGIC, 2, struct xsc_ioctl_hdr)
+#define XSC_IOCTL_CMDQ_RAW \
+	_IOWR(XSC_IOCTL_MAGIC, 5, struct xsc_ioctl_hdr)
+
+enum xsc_ioctl_opcode {
+	XSC_IOCTL_GET_HW_INFO			= 0x100,
+};
+
+enum xsc_ioctl_opmod {
+	XSC_IOCTL_OP_GET_LOCAL,
+};
+
+struct xsc_ioctl_attr {
+	uint16_t opcode; /* ioctl cmd */
+	uint16_t length; /* data length */
+	uint32_t error;  /* ioctl error info */
+	uint8_t data[0]; /* specific table info */
+};
+
+struct xsc_ioctl_hdr {
+	uint32_t check_field;
+	uint32_t domain;
+	uint32_t bus;
+	uint32_t devfn;
+	struct xsc_ioctl_attr attr;
+};
+
+struct xsc_ioctl_data_tl {
+	uint16_t table;
+	uint16_t opmod;
+	uint16_t length;
+	uint16_t rsvd;
+};
+
+struct xsc_ioctl_get_hwinfo {
+	uint32_t domain;
+	uint32_t bus;
+	uint32_t devfn;
+	uint32_t pcie_no;
+	uint32_t func_id;
+	uint32_t pcie_host;
+	uint32_t mac_phy_port;
+	uint32_t funcid_to_logic_port_off;
+	uint16_t lag_id;
+	uint16_t raw_qp_id_base;
+	uint16_t raw_rss_qp_id_base;
+	uint16_t pf0_vf_funcid_base;
+	uint16_t pf0_vf_funcid_top;
+	uint16_t pf1_vf_funcid_base;
+	uint16_t pf1_vf_funcid_top;
+	uint16_t pcie0_pf_funcid_base;
+	uint16_t pcie0_pf_funcid_top;
+	uint16_t pcie1_pf_funcid_base;
+	uint16_t pcie1_pf_funcid_top;
+	uint16_t lag_port_start;
+	uint16_t raw_tpe_qp_num;
+	int send_seg_num;
+	int recv_seg_num;
+	uint8_t on_chip_tbl_vld;
+	uint8_t dma_rw_tbl_vld;
+	uint8_t pct_compress_vld;
+	uint32_t chip_version;
+	uint32_t hca_core_clock;
+	uint8_t mac_bit;
+	uint8_t esw_mode;
+};
+
+int xsc_ioctl(struct xsc_dev *dev, int cmd, int opcode,
+	      void *data_in, int in_len, void *data_out, int out_len);
+
+#endif /* _XSC_CTRL_H_ */
-- 
2.25.1

Re: [PATCH v2 05/19] net/xsc: add ioctl command interface

[Stephen Hemminger]

On Wed, 11 Sep 2024 10:07:26 +0800
"WanRenyong" <wanry@yunsilicon.com> wrote:

> +	hdr = rte_zmalloc(NULL, alloc_len, RTE_CACHE_LINE_SIZE);
> +	if (hdr == NULL) {
> +		PMD_DRV_LOG(ERR, "Failed to allocate xsc ioctl cmd memory");
> +		return -ENOMEM;
> +	}
> +

This is local to function, use malloc() not rte_zmalloc().
It doesn't need to come from hugepages.

Re: [PATCH v2 05/19] net/xsc: add ioctl command interface

[WanRenyong]

On 2024/9/11 11:48, Stephen Hemminger wrote:
> On Wed, 11 Sep 2024 10:07:26 +0800
> "WanRenyong" <wanry@yunsilicon.com> wrote:
>
>> +	hdr = rte_zmalloc(NULL, alloc_len, RTE_CACHE_LINE_SIZE);
>> +	if (hdr == NULL) {
>> +		PMD_DRV_LOG(ERR, "Failed to allocate xsc ioctl cmd memory");
>> +		return -ENOMEM;
>> +	}
>> +
> This is local to function, use malloc() not rte_zmalloc().
> It doesn't need to come from hugepages.

Hello, Stephen,

Thanks for you review,  will fix it in next version.

-- 
Thanks,
WanRenyong

Re: [PATCH v2 05/19] net/xsc: add ioctl command interface

[Stephen Hemminger]

On Wed, 11 Sep 2024 10:07:26 +0800
"WanRenyong" <wanry@yunsilicon.com> wrote:

> +	if (data_in != NULL && in_len > 0)
> +		rte_memcpy(hdr + 1, data_in, in_len);
> +
> +	ret = ioctl(dev->ctrl_fd, cmd, hdr);
> +	if (ret == 0) {
> +		if (hdr->attr.error != 0)
> +			ret = hdr->attr.error;
> +		else if (data_out != NULL && out_len > 0)
> +			rte_memcpy(data_out, hdr + 1, out_len);

Don't need to use rte_memcpy() here. Better to use regular memcpy() which
has more checking

Re: [PATCH v2 05/19] net/xsc: add ioctl command interface

[WanRenyong]

On 2024/9/11 11:49, Stephen Hemminger wrote:
> On Wed, 11 Sep 2024 10:07:26 +0800
> "WanRenyong" <wanry@yunsilicon.com> wrote:
>
>> +	if (data_in != NULL && in_len > 0)
>> +		rte_memcpy(hdr + 1, data_in, in_len);
>> +
>> +	ret = ioctl(dev->ctrl_fd, cmd, hdr);
>> +	if (ret == 0) {
>> +		if (hdr->attr.error != 0)
>> +			ret = hdr->attr.error;
>> +		else if (data_out != NULL && out_len > 0)
>> +			rte_memcpy(data_out, hdr + 1, out_len);
> Don't need to use rte_memcpy() here. Better to use regular memcpy() which
> has more checking

Hello, Stephen,

Thanks for your review,  will fix it in next version.

-- 
Thanks,
WanRenyong

Re: [PATCH v2 05/19] net/xsc: add ioctl command interface

[Stephen Hemminger]

On Wed, 11 Sep 2024 10:07:26 +0800
"WanRenyong" <wanry@yunsilicon.com> wrote:

> +#define XSC_IOCTL_CHECK_FIELD	0x01234567
> +
> +#define XSC_IOCTL_MAGIC	0x1b
> +#define XSC_IOCTL_CMDQ \
> +	_IOWR(XSC_IOCTL_MAGIC, 1, struct xsc_ioctl_hdr)
> +#define XSC_IOCTL_DRV_GET \
> +	_IOR(XSC_IOCTL_MAGIC, 2, struct xsc_ioctl_hdr)
> +#define XSC_IOCTL_CMDQ_RAW \
> +	_IOWR(XSC_IOCTL_MAGIC, 5, struct xsc_ioctl_hdr)
> +
> +enum xsc_ioctl_opcode {
> +	XSC_IOCTL_GET_HW_INFO			= 0x100,
> +};
> +
> +enum xsc_ioctl_opmod {
> +	XSC_IOCTL_OP_GET_LOCAL,
> +};
> +
> +struct xsc_ioctl_attr {
> +	uint16_t opcode; /* ioctl cmd */
> +	uint16_t length; /* data length */
> +	uint32_t error;  /* ioctl error info */
> +	uint8_t data[0]; /* specific table info */

Do not use zero length array (ZLA). Instead use variable length array (VLA)

> +};
> +

Does this device driver depend on some upstr

Re: [PATCH v2 05/19] net/xsc: add ioctl command interface

[WanRenyong]

On 2024/9/11 11:50, Stephen Hemminger wrote:
> On Wed, 11 Sep 2024 10:07:26 +0800
> "WanRenyong" <wanry@yunsilicon.com> wrote:
>
>> +#define XSC_IOCTL_CHECK_FIELD	0x01234567
>> +
>> +#define XSC_IOCTL_MAGIC	0x1b
>> +#define XSC_IOCTL_CMDQ \
>> +	_IOWR(XSC_IOCTL_MAGIC, 1, struct xsc_ioctl_hdr)
>> +#define XSC_IOCTL_DRV_GET \
>> +	_IOR(XSC_IOCTL_MAGIC, 2, struct xsc_ioctl_hdr)
>> +#define XSC_IOCTL_CMDQ_RAW \
>> +	_IOWR(XSC_IOCTL_MAGIC, 5, struct xsc_ioctl_hdr)
>> +
>> +enum xsc_ioctl_opcode {
>> +	XSC_IOCTL_GET_HW_INFO			= 0x100,
>> +};
>> +
>> +enum xsc_ioctl_opmod {
>> +	XSC_IOCTL_OP_GET_LOCAL,
>> +};
>> +
>> +struct xsc_ioctl_attr {
>> +	uint16_t opcode; /* ioctl cmd */
>> +	uint16_t length; /* data length */
>> +	uint32_t error;  /* ioctl error info */
>> +	uint8_t data[0]; /* specific table info */
> Do not use zero length array (ZLA). Instead use variable length array (VLA)

will fix it in next version.

>
>> +};
>> +
> Does this device driver depend on some upstr
Yes, it depends on linux kernel driver of the device.

Hello, Stephen,

Thanks for your review,  please see above.

-- 
Thanks,
WanRenyong

Re: [PATCH v2 05/19] net/xsc: add ioctl command interface

[Stephen Hemminger]

On Thu, 12 Sep 2024 12:14:08 +0800
"WanRenyong" <wanry@yunsilicon.com> wrote:

> >  
> >> +};
> >> +  
> > Does this device driver depend on some upstr  
> Yes, it depends on linux kernel driver of the device.
> 
> Hello, Stephen,
> 
> Thanks for your review,  please see above.

What is the driver? I don't see in the current kernel.org tree.


My concern is that if the driver is not upstream, it is probably not
going to pass the review of kernel developers. This means security and
API changes would be required.

Ioctl's are considered the worst API to the kernel and unlikely
to be accepted.

Re: [PATCH v2 05/19] net/xsc: add ioctl command interface

[WanRenyong]

On 2024/9/12 13:50, Stephen Hemminger wrote:
> On Thu, 12 Sep 2024 12:14:08 +0800
> "WanRenyong" <wanry@yunsilicon.com> wrote:
>
>>>   
>>>> +};
>>>> +
>>> Does this device driver depend on some upstr
>> Yes, it depends on linux kernel driver of the device.
>>
>> Hello, Stephen,
>>
>> Thanks for your review,  please see above.
> What is the driver? I don't see in the current kernel.org tree.
>
>
> My concern is that if the driver is not upstream, it is probably not
> going to pass the review of kernel developers. This means security and
> API changes would be required.
>
> Ioctl's are considered the worst API to the kernel and unlikely
> to be accepted.

Hello, Stephen,

Thank you for reply.Our kernel driver is being prepared to open source.

Our PMD is going to coexist with kernel driver to support flow 
bifurcation feature.

Ioctl API is used for interaction between PMD and kernel driver. As you 
said, ioctl is

the worst API, should I consider using read and write instead?

If not, could you please give me some advice?


-- 
Thanks,
WanRenyong

Re: [PATCH v2 05/19] net/xsc: add ioctl command interface

[fengchengwen]

On 2024/9/12 16:19, WanRenyong wrote:
> On 2024/9/12 13:50, Stephen Hemminger wrote:
>> On Thu, 12 Sep 2024 12:14:08 +0800
>> "WanRenyong" <wanry@yunsilicon.com> wrote:
>>
>>>>   
>>>>> +};
>>>>> +
>>>> Does this device driver depend on some upstr
>>> Yes, it depends on linux kernel driver of the device.
>>>
>>> Hello, Stephen,
>>>
>>> Thanks for your review,  please see above.
>> What is the driver? I don't see in the current kernel.org tree.
>>
>>
>> My concern is that if the driver is not upstream, it is probably not
>> going to pass the review of kernel developers. This means security and
>> API changes would be required.
>>
>> Ioctl's are considered the worst API to the kernel and unlikely
>> to be accepted.
> 
> Hello, Stephen,
> 
> Thank you for reply.Our kernel driver is being prepared to open source.
> 
> Our PMD is going to coexist with kernel driver to support flow 
> bifurcation feature.

It seemed uacce bus could handle this, so that you could use several queues on DPDK, and
steer flows to these queues by bifurcation feature.

> 
> Ioctl API is used for interaction between PMD and kernel driver. As you 
> said, ioctl is
> 
> the worst API, should I consider using read and write instead?

It seemed the ioctl couldn't handle VF located in VM, and interact with PF driver which run in host kernel.

> 
> If not, could you please give me some advice?

If your NIC support firmware, could consider use firmware as mid-man between DPDK and kernel.

> 
> 

Re: [PATCH v2 05/19] net/xsc: add ioctl command interface

[WanRenyong]

On 2024/9/12 17:18, fengchengwen wrote:
> On 2024/9/12 16:19, WanRenyong wrote:
>> On 2024/9/12 13:50, Stephen Hemminger wrote:
>>> On Thu, 12 Sep 2024 12:14:08 +0800
>>> "WanRenyong" <wanry@yunsilicon.com> wrote:
>>>
>>>>>    
>>>>>> +};
>>>>>> +
>>>>> Does this device driver depend on some upstr
>>>> Yes, it depends on linux kernel driver of the device.
>>>>
>>>> Hello, Stephen,
>>>>
>>>> Thanks for your review,  please see above.
>>> What is the driver? I don't see in the current kernel.org tree.
>>>
>>>
>>> My concern is that if the driver is not upstream, it is probably not
>>> going to pass the review of kernel developers. This means security and
>>> API changes would be required.
>>>
>>> Ioctl's are considered the worst API to the kernel and unlikely
>>> to be accepted.
>> Hello, Stephen,
>>
>> Thank you for reply.Our kernel driver is being prepared to open source.
>>
>> Our PMD is going to coexist with kernel driver to support flow
>> bifurcation feature.
> It seemed uacce bus could handle this, so that you could use several queues on DPDK, and
> steer flows to these queues by bifurcation feature.
>
>> Ioctl API is used for interaction between PMD and kernel driver. As you
>> said, ioctl is
>>
>> the worst API, should I consider using read and write instead?
> It seemed the ioctl couldn't handle VF located in VM, and interact 
> with PF driver which run in host kernel.
>> If not, could you please give me some advice?
> If your NIC support firmware, could consider use firmware as mid-man between DPDK and kernel.
>
>>
Hello, fengchenwen,

Thanks for your reply.
Sorry for not describing it clearly. We know how to implement the 
bifurcation functionality. The main issue is that ioctl is used 
forcommunication with the kernel driver by PMD, but ioctl is not 
considered a good API, we need to find a better approach. Implemented 
via firmware might be a good idea, but it's complex, we can think about 
it in the further.Recently shoud I consider using read and write 
instead?  Is there any advise?

-- 
Thanks,
WanRenyong

Re: [PATCH v2 05/19] net/xsc: add ioctl command interface

[Stephen Hemminger]

On Fri, 13 Sep 2024 10:55:08 +0800
"WanRenyong" <wanry@yunsilicon.com> wrote:

> >  
> >> Ioctl API is used for interaction between PMD and kernel driver. As you
> >> said, ioctl is
> >>
> >> the worst API, should I consider using read and write instead?  
> > It seemed the ioctl couldn't handle VF located in VM, and interact 
> > with PF driver which run in host kernel.  
> >> If not, could you please give me some advice?  
> > If your NIC support firmware, could consider use firmware as mid-man between DPDK and kernel.
> >  
> >>  
> Hello, fengchenwen,
> 
> Thanks for your reply.
> Sorry for not describing it clearly. We know how to implement the 
> bifurcation functionality. The main issue is that ioctl is used 
> forcommunication with the kernel driver by PMD, but ioctl is not 
> considered a good API, we need to find a better approach. Implemented 
> via firmware might be a good idea, but it's complex, we can think about 
> it in the further.Recently shoud I consider using read and write 
> instead?  Is there any advise?


There are already several pre-existing ways to do bifurcation in drivers.
1. RDMA which is used by mlx5 and mana drivers.
2. using SR-IOV and queue steering. (Intel did this but not sure if it still exists)
3. BPF

It seems unlikely a new approach using ioctl() that only works on your device
would be accepted by the netdev developers.

Re: [PATCH v2 05/19] net/xsc: add ioctl command interface

[fengchengwen]

On 2024/9/14 10:54, Stephen Hemminger wrote:
> On Fri, 13 Sep 2024 10:55:08 +0800
> "WanRenyong" <wanry@yunsilicon.com> wrote:
> 
>>>  
>>>> Ioctl API is used for interaction between PMD and kernel driver. As you
>>>> said, ioctl is
>>>>
>>>> the worst API, should I consider using read and write instead?  
>>> It seemed the ioctl couldn't handle VF located in VM, and interact 
>>> with PF driver which run in host kernel.  
>>>> If not, could you please give me some advice?  
>>> If your NIC support firmware, could consider use firmware as mid-man between DPDK and kernel.
>>>  
>>>>  
>> Hello, fengchenwen,
>>
>> Thanks for your reply.
>> Sorry for not describing it clearly. We know how to implement the 
>> bifurcation functionality. The main issue is that ioctl is used 
>> forcommunication with the kernel driver by PMD, but ioctl is not 
>> considered a good API, we need to find a better approach. Implemented 
>> via firmware might be a good idea, but it's complex, we can think about 
>> it in the further.Recently shoud I consider using read and write 
>> instead?  Is there any advise?
> 
> 
> There are already several pre-existing ways to do bifurcation in drivers.
> 1. RDMA which is used by mlx5 and mana drivers.
> 2. using SR-IOV and queue steering. (Intel did this but not sure if it still exists)
> 3. BPF
> 
> It seems unlikely a new approach using ioctl() that only works on your device
> would be accepted by the netdev developers.

I just "grep -rn ioctl | wc -l" and found there are 518.
Some of them are net driver which open char device.

Ioctl could be an option as long as it meets current and future needs.