| 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: <SJ0PR11MB5865EEEBECA6DA8BA3216B328FA92@SJ0PR11MB5865.namprd11.prod.outlook.com>
Date: Tue, 23 Jul 2024 07:54:39 +0000
From: "Romanowski, Rafal" <rafal.romanowski@...el.com>
To: Simon Horman <horms@...nel.org>, "Zaki, Ahmed" <ahmed.zaki@...el.com>
CC: "Guo, Junfeng" <junfeng.guo@...el.com>, "netdev@...r.kernel.org"
<netdev@...r.kernel.org>, Marcin Szycik <marcin.szycik@...ux.intel.com>,
"Nguyen, Anthony L" <anthony.l.nguyen@...el.com>, "Keller, Jacob E"
<jacob.e.keller@...el.com>, "intel-wired-lan@...ts.osuosl.org"
<intel-wired-lan@...ts.osuosl.org>
Subject: RE: [Intel-wired-lan] [PATCH iwl-next v2 02/13] ice: parse and init
various DDP parser sections
> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@...osl.org> On Behalf Of Simon
> Horman
> Sent: Friday, May 31, 2024 3:14 PM
> To: Zaki, Ahmed <ahmed.zaki@...el.com>
> Cc: Guo, Junfeng <junfeng.guo@...el.com>; netdev@...r.kernel.org; Marcin
> Szycik <marcin.szycik@...ux.intel.com>; Nguyen, Anthony L
> <anthony.l.nguyen@...el.com>; Keller, Jacob E <jacob.e.keller@...el.com>; intel-
> wired-lan@...ts.osuosl.org
> Subject: Re: [Intel-wired-lan] [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)
>
> ...
Tested-by: Rafal Romanowski <rafal.romanowski@...el.com>
Powered by blists - more mailing lists