| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20240531131416.GE123401@kernel.org>
Date: Fri, 31 May 2024 14:14:16 +0100
From: Simon Horman <horms@...nel.org>
To: Ahmed Zaki <ahmed.zaki@...el.com>
Cc: intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org,
jacob.e.keller@...el.com, anthony.l.nguyen@...el.com,
Junfeng Guo <junfeng.guo@...el.com>,
Marcin Szycik <marcin.szycik@...ux.intel.com>
Subject: Re: [PATCH iwl-next v2 02/13] ice: parse and init various DDP parser
sections
On Mon, May 27, 2024 at 12:57:59PM -0600, Ahmed Zaki wrote:
> From: Junfeng Guo <junfeng.guo@...el.com>
>
> Parse the following DDP sections:
> - ICE_SID_RXPARSER_IMEM into an array of struct ice_imem_item
> - ICE_SID_RXPARSER_METADATA_INIT into an array of struct ice_metainit_item
> - ICE_SID_RXPARSER_CAM or ICE_SID_RXPARSER_PG_SPILL into an array of
> struct ice_pg_cam_item
> - ICE_SID_RXPARSER_NOMATCH_CAM or ICE_SID_RXPARSER_NOMATCH_SPILL into an
> array of struct ice_pg_nm_cam_item
> - ICE_SID_RXPARSER_CAM into an array of ice_bst_tcam_item
> - ICE_SID_LBL_RXPARSER_TMEM into an array of ice_lbl_item
> - ICE_SID_RXPARSER_MARKER_PTYPE into an array of ice_ptype_mk_tcam_item
> - ICE_SID_RXPARSER_MARKER_GRP into an array of ice_mk_grp_item
> - ICE_SID_RXPARSER_PROTO_GRP into an array of ice_proto_grp_item
> - ICE_SID_RXPARSER_FLAG_REDIR into an array of ice_flg_rd_item
> - ICE_SID_XLT_KEY_BUILDER_SW, ICE_SID_XLT_KEY_BUILDER_ACL,
> ICE_SID_XLT_KEY_BUILDER_FD and ICE_SID_XLT_KEY_BUILDER_RSS into
> struct ice_xlt_kb
>
> Reviewed-by: Marcin Szycik <marcin.szycik@...ux.intel.com>
> Signed-off-by: Qi Zhang <qi.z.zhang@...el.com>
> Signed-off-by: Junfeng Guo <junfeng.guo@...el.com>
> Co-developed-by: Ahmed Zaki <ahmed.zaki@...el.com>
> Signed-off-by: Ahmed Zaki <ahmed.zaki@...el.com>
...
> diff --git a/drivers/net/ethernet/intel/ice/ice_parser.c b/drivers/net/ethernet/intel/ice/ice_parser.c
> index b7865b6a0a9b..aaec10afea32 100644
> --- a/drivers/net/ethernet/intel/ice/ice_parser.c
> +++ b/drivers/net/ethernet/intel/ice/ice_parser.c
> @@ -3,6 +3,1257 @@
>
> #include "ice_common.h"
>
> +struct ice_pkg_sect_hdr {
> + __le16 count;
> + __le16 offset;
> +};
> +
> +/**
> + * ice_parser_sect_item_get - parse a item from a section
> + * @sect_type: section type
> + * @section: section object
> + * @index: index of the item to get
> + * @offset: dummy as prototype of ice_pkg_enum_entry's last parameter
Please consider including a "Return: " clause in new Kernel docs.
This is flagged by ./scripts/kernel-doc -Wall -none.
The -Wall flag was recently added to the Netdev CI.
Likewise elsewhere in this patchset.
> + */
> +static void *ice_parser_sect_item_get(u32 sect_type, void *section,
> + u32 index, u32 __maybe_unused *offset)
> +{
> + size_t data_off = ICE_SEC_DATA_OFFSET;
> + struct ice_pkg_sect_hdr *hdr;
> + size_t size;
> +
> + if (!section)
> + return NULL;
> +
> + switch (sect_type) {
> + case ICE_SID_RXPARSER_IMEM:
> + size = ICE_SID_RXPARSER_IMEM_ENTRY_SIZE;
> + break;
> + case ICE_SID_RXPARSER_METADATA_INIT:
> + size = ICE_SID_RXPARSER_METADATA_INIT_ENTRY_SIZE;
> + break;
> + case ICE_SID_RXPARSER_CAM:
> + size = ICE_SID_RXPARSER_CAM_ENTRY_SIZE;
> + break;
> + case ICE_SID_RXPARSER_PG_SPILL:
> + size = ICE_SID_RXPARSER_PG_SPILL_ENTRY_SIZE;
> + break;
> + case ICE_SID_RXPARSER_NOMATCH_CAM:
> + size = ICE_SID_RXPARSER_NOMATCH_CAM_ENTRY_SIZE;
> + break;
> + case ICE_SID_RXPARSER_NOMATCH_SPILL:
> + size = ICE_SID_RXPARSER_NOMATCH_SPILL_ENTRY_SIZE;
> + break;
> + case ICE_SID_RXPARSER_BOOST_TCAM:
> + size = ICE_SID_RXPARSER_BOOST_TCAM_ENTRY_SIZE;
> + break;
> + case ICE_SID_LBL_RXPARSER_TMEM:
> + data_off = ICE_SEC_LBL_DATA_OFFSET;
> + size = ICE_SID_LBL_ENTRY_SIZE;
> + break;
> + case ICE_SID_RXPARSER_MARKER_PTYPE:
> + size = ICE_SID_RXPARSER_MARKER_TYPE_ENTRY_SIZE;
> + break;
> + case ICE_SID_RXPARSER_MARKER_GRP:
> + size = ICE_SID_RXPARSER_MARKER_GRP_ENTRY_SIZE;
> + break;
> + case ICE_SID_RXPARSER_PROTO_GRP:
> + size = ICE_SID_RXPARSER_PROTO_GRP_ENTRY_SIZE;
> + break;
> + case ICE_SID_RXPARSER_FLAG_REDIR:
> + size = ICE_SID_RXPARSER_FLAG_REDIR_ENTRY_SIZE;
> + break;
> + default:
> + return NULL;
> + }
> +
> + hdr = section;
> + if (index >= le16_to_cpu(hdr->count))
> + return NULL;
> +
> + return (u8 *)section + data_off + (index * size);
nit: I don't think that the cast or parentheses are necessary here.
Likewise elsewhere in this patchset.
It's usually not necessary to cast to or from a void * to some other
type of pointer. And in Networking code it's preferred not to do so.
Similarly, although sometimes it is best for the sake of clarity,
it is preferred not to add parentheses where they are not needed.
...
> +/**
> + * ice_imem_alu_init - parse 96 bits of ALU entry
> + * @alu: pointer to the ALU entry structure
> + * @data: ALU entry data to be parsed
> + * @off: offset of the ALU entry data
> + */
> +static void ice_imem_alu_init(struct ice_alu *alu, u8 *data, u8 off)
> +{
> + u64 d64;
> + u8 idd;
> +
> + d64 = *((u64 *)data) >> off;
> +
> + alu->opc = FIELD_GET(ICE_IM_ALU_OPC, d64);
> + alu->src_start = FIELD_GET(ICE_IM_ALU_SS, d64);
> + alu->src_len = FIELD_GET(ICE_IM_ALU_SL, d64);
> + alu->shift_xlate_sel = FIELD_GET(ICE_IM_ALU_SXS, d64);
> + alu->shift_xlate_key = FIELD_GET(ICE_IM_ALU_SXK, d64);
> + alu->src_reg_id = FIELD_GET(ICE_IM_ALU_SRID, d64);
> + alu->dst_reg_id = FIELD_GET(ICE_IM_ALU_DRID, d64);
> + alu->inc0 = FIELD_GET(ICE_IM_ALU_INC0, d64);
> + alu->inc1 = FIELD_GET(ICE_IM_ALU_INC1, d64);
> + alu->proto_offset_opc = FIELD_GET(ICE_IM_ALU_POO, d64);
> + alu->proto_offset = FIELD_GET(ICE_IM_ALU_PO, d64);
> +
> + idd = (ICE_IM_ALU_BA_S + off) / BITS_PER_BYTE;
> + off = (ICE_IM_ALU_BA_S + off) % BITS_PER_BYTE;
> + d64 = *((u64 *)(&data[idd])) >> off;
> +
> + alu->branch_addr = FIELD_GET(ICE_IM_ALU_BA, d64);
> + alu->imm = FIELD_GET(ICE_IM_ALU_IMM, d64);
> + alu->dedicate_flags_ena = FIELD_GET(ICE_IM_ALU_DFE, d64);
> + alu->dst_start = FIELD_GET(ICE_IM_ALU_DS, d64);
> + alu->dst_len = FIELD_GET(ICE_IM_ALU_DL, d64);
> + alu->flags_extr_imm = FIELD_GET(ICE_IM_ALU_FEI, d64);
> + alu->flags_start_imm = FIELD_GET(ICE_IM_ALU_FSI, d64);
> +}
> +
> +#define ICE_IMEM_BM_S 0
> +#define ICE_IMEM_BKB_S 4
> +#define ICE_IMEM_BKB_IDD (ICE_IMEM_BKB_S / BITS_PER_BYTE)
> +#define ICE_IMEM_BKB_OFF (ICE_IMEM_BKB_S % BITS_PER_BYTE)
> +#define ICE_IMEM_PGP GENMASK(15, 14)
> +#define ICE_IMEM_NPKB_S 16
> +#define ICE_IMEM_NPKB_IDD (ICE_IMEM_NPKB_S / BITS_PER_BYTE)
> +#define ICE_IMEM_NPKB_OFF (ICE_IMEM_NPKB_S % BITS_PER_BYTE)
> +#define ICE_IMEM_PGKB_S 34
> +#define ICE_IMEM_PGKB_IDD (ICE_IMEM_PGKB_S / BITS_PER_BYTE)
> +#define ICE_IMEM_PGKB_OFF (ICE_IMEM_PGKB_S % BITS_PER_BYTE)
> +#define ICE_IMEM_ALU0_S 69
> +#define ICE_IMEM_ALU0_IDD (ICE_IMEM_ALU0_S / BITS_PER_BYTE)
> +#define ICE_IMEM_ALU0_OFF (ICE_IMEM_ALU0_S % BITS_PER_BYTE)
> +#define ICE_IMEM_ALU1_S 165
> +#define ICE_IMEM_ALU1_IDD (ICE_IMEM_ALU1_S / BITS_PER_BYTE)
> +#define ICE_IMEM_ALU1_OFF (ICE_IMEM_ALU1_S % BITS_PER_BYTE)
> +#define ICE_IMEM_ALU2_S 357
> +#define ICE_IMEM_ALU2_IDD (ICE_IMEM_ALU2_S / BITS_PER_BYTE)
> +#define ICE_IMEM_ALU2_OFF (ICE_IMEM_ALU2_S % BITS_PER_BYTE)
> +
> +/**
> + * ice_imem_parse_item - parse 384 bits of IMEM entry
> + * @hw: pointer to the hardware structure
> + * @idx: index of IMEM entry
> + * @item: item of IMEM entry
> + * @data: IMEM entry data to be parsed
> + * @size: size of IMEM entry
> + */
> +static void ice_imem_parse_item(struct ice_hw *hw, u16 idx, void *item,
> + void *data, int __maybe_unused size)
> +{
> + struct ice_imem_item *ii = item;
> + u8 *buf = (u8 *)data;
I think that in this function data can be used directly in place of buf.
> +
> + ii->idx = idx;
> +
> + ice_imem_bm_init(&ii->b_m, *(u8 *)buf);
> + ice_imem_bkb_init(&ii->b_kb,
> + *((u16 *)(&buf[ICE_IMEM_BKB_IDD])) >>
> + ICE_IMEM_BKB_OFF);
nit: I suspect that FIELD_GET can be used here.
And elsewhere where >> is used in this function and
ice_imem_alu_init().
> +
> + ii->pg_prio = FIELD_GET(ICE_IMEM_PGP, *(u16 *)buf);
> +
> + ice_imem_npkb_init(&ii->np_kb,
> + *((u32 *)(&buf[ICE_IMEM_NPKB_IDD])) >>
> + ICE_IMEM_NPKB_OFF);
> + ice_imem_pgkb_init(&ii->pg_kb,
> + *((u64 *)(&buf[ICE_IMEM_PGKB_IDD])) >>
> + ICE_IMEM_PGKB_OFF);
> +
> + ice_imem_alu_init(&ii->alu0,
> + &buf[ICE_IMEM_ALU0_IDD],
> + ICE_IMEM_ALU0_OFF);
> + ice_imem_alu_init(&ii->alu1,
> + &buf[ICE_IMEM_ALU1_IDD],
> + ICE_IMEM_ALU1_OFF);
> + ice_imem_alu_init(&ii->alu2,
> + &buf[ICE_IMEM_ALU2_IDD],
> + ICE_IMEM_ALU2_OFF);
> +}
...
> +#define ICE_MI_GBDM_IDD (ICE_MI_GBDM_S / BITS_PER_BYTE)
> +#define ICE_MI_GBDM_OFF (ICE_MI_GBDM_S % BITS_PER_BYTE)
> +#define ICE_MI_GBDM GENMASK_ULL(65 - ICE_MI_GBDM_S, \
> + 61 - ICE_MI_GBDM_S)
nit: Some macros might make this a bit easier on the eyes (or not?).
(Completely untested!)
#define ICE_GENMASK_OFF_ULL(high, low, offset) \
GENMASK_ULL(high - offset, low - offset)
#define ICE_MI_GBDM_GENMASK_ULL(high, low) \
ICE_GENMASK_OFF_ULL(high, low, ICE_MI_GBDM_S)
#define ICE_MI_GBDS ICE_MI_GBDM_GENMASK_ULL(69, 66)
...
#define ICE_MI_FLAG ICE_GENMASK_OFF_ULL(186, 123, ICE_MI_FLAG_S)
> +#define ICE_MI_GBDS GENMASK_ULL(69 - ICE_MI_GBDM_S, \
> + 66 - ICE_MI_GBDM_S)
> +#define ICE_MI_GBDL GENMASK_ULL(74 - ICE_MI_GBDM_S, \
> + 70 - ICE_MI_GBDM_S)
> +#define ICE_MI_GBI GENMASK_ULL(80 - ICE_MI_GBDM_S, \
> + 77 - ICE_MI_GBDM_S)
> +#define ICE_MI_GCC BIT_ULL(81 - ICE_MI_GBDM_S)
> +#define ICE_MI_GCDM GENMASK_ULL(86 - ICE_MI_GBDM_S, \
> + 82 - ICE_MI_GBDM_S)
> +#define ICE_MI_GCDS GENMASK_ULL(90 - ICE_MI_GBDM_S, \
> + 87 - ICE_MI_GBDM_S)
> +#define ICE_MI_GCDL GENMASK_ULL(95 - ICE_MI_GBDM_S, \
> + 91 - ICE_MI_GBDM_S)
> +#define ICE_MI_GCI GENMASK_ULL(101 - ICE_MI_GBDM_S, \
> + 98 - ICE_MI_GBDM_S)
> +#define ICE_MI_GDC BIT_ULL(102 - ICE_MI_GBDM_S)
> +#define ICE_MI_GDDM GENMASK_ULL(107 - ICE_MI_GBDM_S, \
> + 103 - ICE_MI_GBDM_S)
> +#define ICE_MI_GDDS GENMASK_ULL(111 - ICE_MI_GBDM_S, \
> + 108 - ICE_MI_GBDM_S)
> +#define ICE_MI_GDDL GENMASK_ULL(116 - ICE_MI_GBDM_S, \
> + 112 - ICE_MI_GBDM_S)
> +#define ICE_MI_GDI GENMASK_ULL(122 - ICE_MI_GBDM_S, \
> + 119 - ICE_MI_GBDM_S)
> +#define ICE_MI_FLAG_S 123 /* offset for the 3rd 64-bits field */
> +#define ICE_MI_FLAG_IDD (ICE_MI_FLAG_S / BITS_PER_BYTE)
> +#define ICE_MI_FLAG_OFF (ICE_MI_FLAG_S % BITS_PER_BYTE)
> +#define ICE_MI_FLAG GENMASK_ULL(186 - ICE_MI_FLAG_S, \
> + 123 - ICE_MI_FLAG_S)
...
Powered by blists - more mailing lists