[v2,02/10] net/gve: add logs and OS specific implementation
Checks
Commit Message
Add GVE PMD logs.
Add some MACRO definitions and memory operations which are specific
for DPDK.
Signed-off-by: Haiyue Wang <haiyue.wang@intel.com>
Signed-off-by: Xiaoyun Li <xiaoyun.li@intel.com>
Signed-off-by: Junfeng Guo <junfeng.guo@intel.com>
---
drivers/net/gve/gve_adminq.h | 2 +
drivers/net/gve/gve_desc.h | 2 +
drivers/net/gve/gve_desc_dqo.h | 2 +
drivers/net/gve/gve_logs.h | 22 +++++
drivers/net/gve/gve_osdep.h | 149 +++++++++++++++++++++++++++++++++
drivers/net/gve/gve_register.h | 2 +
6 files changed, 179 insertions(+)
create mode 100644 drivers/net/gve/gve_logs.h
create mode 100644 drivers/net/gve/gve_osdep.h
Comments
On 8/29/2022 9:41 AM, Junfeng Guo wrote:
>
> Add GVE PMD logs.
> Add some MACRO definitions and memory operations which are specific
> for DPDK.
>
> Signed-off-by: Haiyue Wang <haiyue.wang@intel.com>
> Signed-off-by: Xiaoyun Li <xiaoyun.li@intel.com>
> Signed-off-by: Junfeng Guo <junfeng.guo@intel.com>
<...>
> diff --git a/drivers/net/gve/gve_logs.h b/drivers/net/gve/gve_logs.h
> new file mode 100644
> index 0000000000..a050253f59
> --- /dev/null
> +++ b/drivers/net/gve/gve_logs.h
> @@ -0,0 +1,22 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(C) 2022 Intel Corporation
> + */
> +
> +#ifndef _GVE_LOGS_H_
> +#define _GVE_LOGS_H_
> +
> +extern int gve_logtype_init;
> +extern int gve_logtype_driver;
> +
> +#define PMD_INIT_LOG(level, fmt, args...) \
> + rte_log(RTE_LOG_ ## level, gve_logtype_init, "%s(): " fmt "\n", \
> + __func__, ##args)
> +
> +#define PMD_DRV_LOG_RAW(level, fmt, args...) \
> + rte_log(RTE_LOG_ ## level, gve_logtype_driver, "%s(): " fmt, \
> + __func__, ## args)
> + > +#define PMD_DRV_LOG(level, fmt, args...) \
> + PMD_DRV_LOG_RAW(level, fmt "\n", ## args)
> +
Why 'PMD_DRV_LOG_RAW' is needed, why not directly use 'PMD_DRV_LOG'?
Do you really need two different log types? How do you differentiate
'init' & 'driver' types? As far as I can see there is mixed usage of them.
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@xilinx.com>
> Sent: Friday, September 2, 2022 01:21
> To: Guo, Junfeng <junfeng.guo@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>
> Cc: dev@dpdk.org; Li, Xiaoyun <xiaoyun.li@intel.com>;
> awogbemila@google.com; Richardson, Bruce
> <bruce.richardson@intel.com>; Wang, Haiyue <haiyue.wang@intel.com>
> Subject: Re: [PATCH v2 02/10] net/gve: add logs and OS specific
> implementation
>
> On 8/29/2022 9:41 AM, Junfeng Guo wrote:
>
> >
> > Add GVE PMD logs.
> > Add some MACRO definitions and memory operations which are specific
> > for DPDK.
> >
> > Signed-off-by: Haiyue Wang <haiyue.wang@intel.com>
> > Signed-off-by: Xiaoyun Li <xiaoyun.li@intel.com>
> > Signed-off-by: Junfeng Guo <junfeng.guo@intel.com>
>
> <...>
>
> > diff --git a/drivers/net/gve/gve_logs.h b/drivers/net/gve/gve_logs.h
> > new file mode 100644
> > index 0000000000..a050253f59
> > --- /dev/null
> > +++ b/drivers/net/gve/gve_logs.h
> > @@ -0,0 +1,22 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(C) 2022 Intel Corporation
> > + */
> > +
> > +#ifndef _GVE_LOGS_H_
> > +#define _GVE_LOGS_H_
> > +
> > +extern int gve_logtype_init;
> > +extern int gve_logtype_driver;
> > +
> > +#define PMD_INIT_LOG(level, fmt, args...) \
> > + rte_log(RTE_LOG_ ## level, gve_logtype_init, "%s(): " fmt "\n", \
> > + __func__, ##args)
> > +
> > +#define PMD_DRV_LOG_RAW(level, fmt, args...) \
> > + rte_log(RTE_LOG_ ## level, gve_logtype_driver, "%s(): " fmt, \
> > + __func__, ## args)
> > + > +#define PMD_DRV_LOG(level, fmt, args...) \
> > + PMD_DRV_LOG_RAW(level, fmt "\n", ## args)
> > +
>
> Why 'PMD_DRV_LOG_RAW' is needed, why not directly use
> 'PMD_DRV_LOG'?
It seems that the _RAW macro was first introduced at i40e driver logs file.
Since sometimes the trailing '\n' is added at the end of the log message in
the base code, the PMD_DRV_LOG_RAW macro that will not add one is
used to keep consistent of the new line character.
Well, looks that the macro PMD_DRV_LOG_RAW is somewhat redundant.
I think it's ok to remove PMD_DRV_LOG_RAW and keep all the log messages
end without the trailing '\n'. Thanks!
>
>
> Do you really need two different log types? How do you differentiate
> 'init' & 'driver' types? As far as I can see there is mixed usage of them.
The PMD_INIT_LOG is used at the init stage, while the PMD_DRV_LOG
is used at the driver normal running stage. I agree that there might be
mixed usage of these two macros. I'll try to check all these usages and
update them at correct conditions in the coming versions.
If you insist that only one log type is needed to keep the code clean,
then I could update them as you expected. Thanks!
Regards,
Junfeng
On 9/7/2022 7:58 AM, Guo, Junfeng wrote:
> CAUTION: This message has originated from an External Source. Please use proper judgment and caution when opening attachments, clicking links, or responding to this email.
>
>
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@xilinx.com>
>> Sent: Friday, September 2, 2022 01:21
>> To: Guo, Junfeng <junfeng.guo@intel.com>; Zhang, Qi Z
>> <qi.z.zhang@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>
>> Cc: dev@dpdk.org; Li, Xiaoyun <xiaoyun.li@intel.com>;
>> awogbemila@google.com; Richardson, Bruce
>> <bruce.richardson@intel.com>; Wang, Haiyue <haiyue.wang@intel.com>
>> Subject: Re: [PATCH v2 02/10] net/gve: add logs and OS specific
>> implementation
>>
>> On 8/29/2022 9:41 AM, Junfeng Guo wrote:
>>
>>>
>>> Add GVE PMD logs.
>>> Add some MACRO definitions and memory operations which are specific
>>> for DPDK.
>>>
>>> Signed-off-by: Haiyue Wang <haiyue.wang@intel.com>
>>> Signed-off-by: Xiaoyun Li <xiaoyun.li@intel.com>
>>> Signed-off-by: Junfeng Guo <junfeng.guo@intel.com>
>>
>> <...>
>>
>>> diff --git a/drivers/net/gve/gve_logs.h b/drivers/net/gve/gve_logs.h
>>> new file mode 100644
>>> index 0000000000..a050253f59
>>> --- /dev/null
>>> +++ b/drivers/net/gve/gve_logs.h
>>> @@ -0,0 +1,22 @@
>>> +/* SPDX-License-Identifier: BSD-3-Clause
>>> + * Copyright(C) 2022 Intel Corporation
>>> + */
>>> +
>>> +#ifndef _GVE_LOGS_H_
>>> +#define _GVE_LOGS_H_
>>> +
>>> +extern int gve_logtype_init;
>>> +extern int gve_logtype_driver;
>>> +
>>> +#define PMD_INIT_LOG(level, fmt, args...) \
>>> + rte_log(RTE_LOG_ ## level, gve_logtype_init, "%s(): " fmt "\n", \
>>> + __func__, ##args)
>>> +
>>> +#define PMD_DRV_LOG_RAW(level, fmt, args...) \
>>> + rte_log(RTE_LOG_ ## level, gve_logtype_driver, "%s(): " fmt, \
>>> + __func__, ## args)
>>> + > +#define PMD_DRV_LOG(level, fmt, args...) \
>>> + PMD_DRV_LOG_RAW(level, fmt "\n", ## args)
>>> +
>>
>> Why 'PMD_DRV_LOG_RAW' is needed, why not directly use
>> 'PMD_DRV_LOG'?
>
> It seems that the _RAW macro was first introduced at i40e driver logs file.
> Since sometimes the trailing '\n' is added at the end of the log message in
> the base code, the PMD_DRV_LOG_RAW macro that will not add one is
> used to keep consistent of the new line character.
>
> Well, looks that the macro PMD_DRV_LOG_RAW is somewhat redundant.
> I think it's ok to remove PMD_DRV_LOG_RAW and keep all the log messages
> end without the trailing '\n'. Thanks!
>
Or you can add '\n' to 'PMD_DRV_LOG', to not change all logs. Only
having two macro seems unnecessary.
>>
>>
>> Do you really need two different log types? How do you differentiate
>> 'init' & 'driver' types? As far as I can see there is mixed usage of them.
>
> The PMD_INIT_LOG is used at the init stage, while the PMD_DRV_LOG
> is used at the driver normal running stage. I agree that there might be
> mixed usage of these two macros. I'll try to check all these usages and
> update them at correct conditions in the coming versions.
> If you insist that only one log type is needed to keep the code clean,
> then I could update them as you expected. Thanks!
>
I do not insist, but it looks like you are complicating things, is there
really a benefit to have two different log types?
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@xilinx.com>
> Sent: Wednesday, September 7, 2022 19:17
> To: Guo, Junfeng <junfeng.guo@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>
> Cc: dev@dpdk.org; Li, Xiaoyun <xiaoyun.li@intel.com>;
> awogbemila@google.com; Richardson, Bruce
> <bruce.richardson@intel.com>; Wang, Haiyue <haiyue.wang@intel.com>
> Subject: Re: [PATCH v2 02/10] net/gve: add logs and OS specific
> implementation
>
> On 9/7/2022 7:58 AM, Guo, Junfeng wrote:
> > CAUTION: This message has originated from an External Source. Please
> use proper judgment and caution when opening attachments, clicking
> links, or responding to this email.
> >
> >
> >> -----Original Message-----
> >> From: Ferruh Yigit <ferruh.yigit@xilinx.com>
> >> Sent: Friday, September 2, 2022 01:21
> >> To: Guo, Junfeng <junfeng.guo@intel.com>; Zhang, Qi Z
> >> <qi.z.zhang@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>
> >> Cc: dev@dpdk.org; Li, Xiaoyun <xiaoyun.li@intel.com>;
> >> awogbemila@google.com; Richardson, Bruce
> >> <bruce.richardson@intel.com>; Wang, Haiyue
> <haiyue.wang@intel.com>
> >> Subject: Re: [PATCH v2 02/10] net/gve: add logs and OS specific
> >> implementation
> >>
> >> On 8/29/2022 9:41 AM, Junfeng Guo wrote:
> >>
> >>>
> >>> Add GVE PMD logs.
> >>> Add some MACRO definitions and memory operations which are
> specific
> >>> for DPDK.
> >>>
> >>> Signed-off-by: Haiyue Wang <haiyue.wang@intel.com>
> >>> Signed-off-by: Xiaoyun Li <xiaoyun.li@intel.com>
> >>> Signed-off-by: Junfeng Guo <junfeng.guo@intel.com>
> >>
> >> <...>
> >>
> >>> diff --git a/drivers/net/gve/gve_logs.h b/drivers/net/gve/gve_logs.h
> >>> new file mode 100644
> >>> index 0000000000..a050253f59
> >>> --- /dev/null
> >>> +++ b/drivers/net/gve/gve_logs.h
> >>> @@ -0,0 +1,22 @@
> >>> +/* SPDX-License-Identifier: BSD-3-Clause
> >>> + * Copyright(C) 2022 Intel Corporation
> >>> + */
> >>> +
> >>> +#ifndef _GVE_LOGS_H_
> >>> +#define _GVE_LOGS_H_
> >>> +
> >>> +extern int gve_logtype_init;
> >>> +extern int gve_logtype_driver;
> >>> +
> >>> +#define PMD_INIT_LOG(level, fmt, args...) \
> >>> + rte_log(RTE_LOG_ ## level, gve_logtype_init, "%s(): " fmt "\n", \
> >>> + __func__, ##args)
> >>> +
> >>> +#define PMD_DRV_LOG_RAW(level, fmt, args...) \
> >>> + rte_log(RTE_LOG_ ## level, gve_logtype_driver, "%s(): " fmt, \
> >>> + __func__, ## args)
> >>> + > +#define PMD_DRV_LOG(level, fmt, args...) \
> >>> + PMD_DRV_LOG_RAW(level, fmt "\n", ## args)
> >>> +
> >>
> >> Why 'PMD_DRV_LOG_RAW' is needed, why not directly use
> >> 'PMD_DRV_LOG'?
> >
> > It seems that the _RAW macro was first introduced at i40e driver logs
> file.
> > Since sometimes the trailing '\n' is added at the end of the log message
> in
> > the base code, the PMD_DRV_LOG_RAW macro that will not add one is
> > used to keep consistent of the new line character.
> >
> > Well, looks that the macro PMD_DRV_LOG_RAW is somewhat
> redundant.
> > I think it's ok to remove PMD_DRV_LOG_RAW and keep all the log
> messages
> > end without the trailing '\n'. Thanks!
> >
>
> Or you can add '\n' to 'PMD_DRV_LOG', to not change all logs. Only
> having two macro seems unnecessary.
Yes, already did as this form in the coming version gve pmd code. Thanks!
>
> >>
> >>
> >> Do you really need two different log types? How do you differentiate
> >> 'init' & 'driver' types? As far as I can see there is mixed usage of them.
> >
> > The PMD_INIT_LOG is used at the init stage, while the PMD_DRV_LOG
> > is used at the driver normal running stage. I agree that there might be
> > mixed usage of these two macros. I'll try to check all these usages and
> > update them at correct conditions in the coming versions.
> > If you insist that only one log type is needed to keep the code clean,
> > then I could update them as you expected. Thanks!
> >
>
> I do not insist, but it looks like you are complicating things, is there
> really a benefit to have two different log types?
Well, these two types may be used to show init/driver logs, respectively.
But It seems that there is no such specific need to use two log types in the
GVE PMD. Anyway, I think it is good time to keep the code clean and not
just inherit from previous drivers. We can add new log type in the future
if it's required. Thanks!
@@ -8,6 +8,8 @@
#ifndef _GVE_ADMINQ_H
#define _GVE_ADMINQ_H
+#include "gve_osdep.h"
+
/* Admin queue opcodes */
enum gve_adminq_opcodes {
GVE_ADMINQ_DESCRIBE_DEVICE = 0x1,
@@ -9,6 +9,8 @@
#ifndef _GVE_DESC_H_
#define _GVE_DESC_H_
+#include "gve_osdep.h"
+
/* A note on seg_addrs
*
* Base addresses encoded in seg_addr are not assumed to be physical
@@ -9,6 +9,8 @@
#ifndef _GVE_DESC_DQO_H_
#define _GVE_DESC_DQO_H_
+#include "gve_osdep.h"
+
#define GVE_TX_MAX_HDR_SIZE_DQO 255
#define GVE_TX_MIN_TSO_MSS_DQO 88
new file mode 100644
@@ -0,0 +1,22 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(C) 2022 Intel Corporation
+ */
+
+#ifndef _GVE_LOGS_H_
+#define _GVE_LOGS_H_
+
+extern int gve_logtype_init;
+extern int gve_logtype_driver;
+
+#define PMD_INIT_LOG(level, fmt, args...) \
+ rte_log(RTE_LOG_ ## level, gve_logtype_init, "%s(): " fmt "\n", \
+ __func__, ##args)
+
+#define PMD_DRV_LOG_RAW(level, fmt, args...) \
+ rte_log(RTE_LOG_ ## level, gve_logtype_driver, "%s(): " fmt, \
+ __func__, ## args)
+
+#define PMD_DRV_LOG(level, fmt, args...) \
+ PMD_DRV_LOG_RAW(level, fmt "\n", ## args)
+
+#endif
new file mode 100644
@@ -0,0 +1,149 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(C) 2022 Intel Corporation
+ */
+
+#ifndef _GVE_OSDEP_H_
+#define _GVE_OSDEP_H_
+
+#include <string.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <stdarg.h>
+#include <inttypes.h>
+#include <stdbool.h>
+
+#include <rte_bitops.h>
+#include <rte_byteorder.h>
+#include <rte_common.h>
+#include <rte_ether.h>
+#include <rte_io.h>
+#include <rte_log.h>
+#include <rte_malloc.h>
+#include <rte_memcpy.h>
+#include <rte_memzone.h>
+
+#include "gve_logs.h"
+
+typedef uint8_t u8;
+typedef uint16_t u16;
+typedef uint32_t u32;
+typedef uint64_t u64;
+
+typedef rte_be16_t __sum16;
+
+typedef rte_be16_t __be16;
+typedef rte_be32_t __be32;
+typedef rte_be64_t __be64;
+
+typedef rte_iova_t dma_addr_t;
+
+#define ETH_MIN_MTU RTE_ETHER_MIN_MTU
+#define ETH_ALEN RTE_ETHER_ADDR_LEN
+#define PAGE_SIZE 4096
+
+#define BIT(nr) RTE_BIT32(nr)
+
+#define be16_to_cpu(x) rte_be_to_cpu_16(x)
+#define be32_to_cpu(x) rte_be_to_cpu_32(x)
+#define be64_to_cpu(x) rte_be_to_cpu_64(x)
+
+#define cpu_to_be16(x) rte_cpu_to_be_16(x)
+#define cpu_to_be32(x) rte_cpu_to_be_32(x)
+#define cpu_to_be64(x) rte_cpu_to_be_64(x)
+
+#define READ_ONCE32(x) rte_read32(&(x))
+
+#define ____cacheline_aligned __rte_cache_aligned
+#define __packed __rte_packed
+#define __iomem
+
+#define msleep(ms) rte_delay_ms(ms)
+
+/* These macros are used to generate compilation errors if a struct/union
+ * is not exactly the correct length. It gives a divide by zero error if
+ * the struct/union is not of the correct size, otherwise it creates an
+ * enum that is never used.
+ */
+#define GVE_CHECK_STRUCT_LEN(n, X) enum gve_static_assert_enum_##X \
+ { gve_static_assert_##X = (n) / ((sizeof(struct X) == (n)) ? 1 : 0) }
+#define GVE_CHECK_UNION_LEN(n, X) enum gve_static_asset_enum_##X \
+ { gve_static_assert_##X = (n) / ((sizeof(union X) == (n)) ? 1 : 0) }
+
+static __rte_always_inline u8
+readb(volatile void *addr)
+{
+ return rte_read8(addr);
+}
+
+static __rte_always_inline void
+writeb(u8 value, volatile void *addr)
+{
+ rte_write8(value, addr);
+}
+
+static __rte_always_inline void
+writel(u32 value, volatile void *addr)
+{
+ rte_write32(value, addr);
+}
+
+static __rte_always_inline u32
+ioread32be(const volatile void *addr)
+{
+ return rte_be_to_cpu_32(rte_read32(addr));
+}
+
+static __rte_always_inline void
+iowrite32be(u32 value, volatile void *addr)
+{
+ writel(rte_cpu_to_be_32(value), addr);
+}
+
+/* DMA memory allocation tracking */
+struct gve_dma_mem {
+ void *va;
+ rte_iova_t pa;
+ uint32_t size;
+ const void *zone;
+};
+
+static inline void *
+gve_alloc_dma_mem(struct gve_dma_mem *mem, u64 size)
+{
+ static uint16_t gve_dma_memzone_id;
+ const struct rte_memzone *mz = NULL;
+ char z_name[RTE_MEMZONE_NAMESIZE];
+
+ if (!mem)
+ return NULL;
+
+ snprintf(z_name, sizeof(z_name), "gve_dma_%u",
+ __atomic_fetch_add(&gve_dma_memzone_id, 1, __ATOMIC_RELAXED));
+ mz = rte_memzone_reserve_aligned(z_name, size, SOCKET_ID_ANY,
+ RTE_MEMZONE_IOVA_CONTIG,
+ PAGE_SIZE);
+ if (!mz)
+ return NULL;
+
+ mem->size = size;
+ mem->va = mz->addr;
+ mem->pa = mz->iova;
+ mem->zone = mz;
+ PMD_DRV_LOG(DEBUG, "memzone %s is allocated", mz->name);
+
+ return mem->va;
+}
+
+static inline void
+gve_free_dma_mem(struct gve_dma_mem *mem)
+{
+ PMD_DRV_LOG(DEBUG, "memzone %s to be freed",
+ ((const struct rte_memzone *)mem->zone)->name);
+
+ rte_memzone_free(mem->zone);
+ mem->zone = NULL;
+ mem->va = NULL;
+ mem->pa = 0;
+}
+
+#endif /* _GVE_OSDEP_H_ */
@@ -7,6 +7,8 @@
#ifndef _GVE_REGISTER_H_
#define _GVE_REGISTER_H_
+#include "gve_osdep.h"
+
/* Fixed Configuration Registers */
struct gve_registers {
__be32 device_status;