| 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: <DM6PR11MB4610FA6E30A5E4F75DCD5C53F34D2@DM6PR11MB4610.namprd11.prod.outlook.com>
Date: Wed, 23 Oct 2024 12:27:19 +0000
From: "Kwapulinski, Piotr" <piotr.kwapulinski@...el.com>
To: Kalesh Anakkur Purayil <kalesh-anakkur.purayil@...adcom.com>
CC: "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>, "Wegrzyn, Stefan"
<stefan.wegrzyn@...el.com>, "Jagielski, Jedrzej"
<jedrzej.jagielski@...el.com>, "Sokolowski, Jan" <jan.sokolowski@...el.com>
Subject: RE: [PATCH iwl-next v9 2/7] ixgbe: Add support for E610 device
capabilities detection
>-----Original Message-----
>From: Kalesh Anakkur Purayil <kalesh-anakkur.purayil@...adcom.com>
>Sent: Friday, October 18, 2024 5:06 AM
>To: Kwapulinski, Piotr <piotr.kwapulinski@...el.com>
>Cc: intel-wired-lan@...ts.osuosl.org; netdev@...r.kernel.org; Wegrzyn, Stefan <stefan.wegrzyn@...el.com>; Jagielski, Jedrzej <jedrzej.jagielski@...el.com>; Sokolowski, Jan <jan.sokolowski@...el.com>
>Subject: Re: [PATCH iwl-next v9 2/7] ixgbe: Add support for E610 device capabilities detection
>
>On Thu, Oct 3, 2024 at 7:48 PM Piotr Kwapulinski
><piotr.kwapulinski@...el.com> wrote:
>>
>> Add low level support for E610 device capabilities detection. The
>> capabilities are discovered via the Admin Command Interface. Discover the
>> following capabilities:
>> - function caps: vmdq, dcb, rss, rx/tx qs, msix, nvm, orom, reset
>> - device caps: vsi, fdir, 1588
>> - phy caps
>>
>> Co-developed-by: Stefan Wegrzyn <stefan.wegrzyn@...el.com>
>> Signed-off-by: Stefan Wegrzyn <stefan.wegrzyn@...el.com>
>> Co-developed-by: Jedrzej Jagielski <jedrzej.jagielski@...el.com>
>> Signed-off-by: Jedrzej Jagielski <jedrzej.jagielski@...el.com>
>> Reviewed-by: Jan Sokolowski <jan.sokolowski@...el.com>
>> Signed-off-by: Piotr Kwapulinski <piotr.kwapulinski@...el.com>
>
>Hi Piotr,
>
>Some minor cosmetic comments in line.
>
>> ---
>> drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c | 540 ++++++++++++++++++
>> drivers/net/ethernet/intel/ixgbe/ixgbe_e610.h | 12 +
>> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 8 +
>> 3 files changed, 560 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
>> index 28bd7da..3bc88df 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
>> @@ -493,3 +493,543 @@ void ixgbe_release_res(struct ixgbe_hw *hw, enum ixgbe_aci_res_ids res)
>> total_delay++;
>> }
>> }
>> +
>> +/**
>> + * ixgbe_parse_e610_caps - Parse common device/function capabilities
>> + * @hw: pointer to the HW struct
>> + * @caps: pointer to common capabilities structure
>> + * @elem: the capability element to parse
>> + * @prefix: message prefix for tracing capabilities
>> + *
>> + * Given a capability element, extract relevant details into the common
>> + * capability structure.
>> + *
>> + * Return: true if the capability matches one of the common capability ids,
>> + * false otherwise.
>> + */
>> +static bool ixgbe_parse_e610_caps(struct ixgbe_hw *hw,
>> + struct ixgbe_hw_caps *caps,
>> + struct ixgbe_aci_cmd_list_caps_elem *elem,
>> + const char *prefix)
>> +{
>> + u32 logical_id = elem->logical_id;
>> + u32 phys_id = elem->phys_id;
>> + u32 number = elem->number;
>> + u16 cap = elem->cap;
>> +
>> + switch (cap) {
>> + case IXGBE_ACI_CAPS_VALID_FUNCTIONS:
>> + caps->valid_functions = number;
>> + break;
>> + case IXGBE_ACI_CAPS_SRIOV:
>> + caps->sr_iov_1_1 = (number == 1);
>> + break;
>> + case IXGBE_ACI_CAPS_VMDQ:
>> + caps->vmdq = (number == 1);
>> + break;
>> + case IXGBE_ACI_CAPS_DCB:
>> + caps->dcb = (number == 1);
>> + caps->active_tc_bitmap = logical_id;
>> + caps->maxtc = phys_id;
>> + break;
>> + case IXGBE_ACI_CAPS_RSS:
>> + caps->rss_table_size = number;
>> + caps->rss_table_entry_width = logical_id;
>> + break;
>> + case IXGBE_ACI_CAPS_RXQS:
>> + caps->num_rxq = number;
>> + caps->rxq_first_id = phys_id;
>> + break;
>> + case IXGBE_ACI_CAPS_TXQS:
>> + caps->num_txq = number;
>> + caps->txq_first_id = phys_id;
>> + break;
>> + case IXGBE_ACI_CAPS_MSIX:
>> + caps->num_msix_vectors = number;
>> + caps->msix_vector_first_id = phys_id;
>> + break;
>> + case IXGBE_ACI_CAPS_NVM_VER:
>> + break;
>> + case IXGBE_ACI_CAPS_MAX_MTU:
>> + caps->max_mtu = number;
>> + break;
>> + case IXGBE_ACI_CAPS_PCIE_RESET_AVOIDANCE:
>> + caps->pcie_reset_avoidance = (number > 0);
>> + break;
>> + case IXGBE_ACI_CAPS_POST_UPDATE_RESET_RESTRICT:
>> + caps->reset_restrict_support = (number == 1);
>> + break;
>> + case IXGBE_ACI_CAPS_EXT_TOPO_DEV_IMG0:
>> + case IXGBE_ACI_CAPS_EXT_TOPO_DEV_IMG1:
>> + case IXGBE_ACI_CAPS_EXT_TOPO_DEV_IMG2:
>> + case IXGBE_ACI_CAPS_EXT_TOPO_DEV_IMG3:
>> + {
>> + u8 index = cap - IXGBE_ACI_CAPS_EXT_TOPO_DEV_IMG0;
>> +
>> + caps->ext_topo_dev_img_ver_high[index] = number;
>> + caps->ext_topo_dev_img_ver_low[index] = logical_id;
>> + caps->ext_topo_dev_img_part_num[index] =
>> + FIELD_GET(IXGBE_EXT_TOPO_DEV_IMG_PART_NUM_M, phys_id);
>> + caps->ext_topo_dev_img_load_en[index] =
>> + (phys_id & IXGBE_EXT_TOPO_DEV_IMG_LOAD_EN) != 0;
>> + caps->ext_topo_dev_img_prog_en[index] =
>> + (phys_id & IXGBE_EXT_TOPO_DEV_IMG_PROG_EN) != 0;
>> + break;
>> + }
>> + default:
>> + /* Not one of the recognized common capabilities */
>> + return false;
>> + }
>> +
>> + return true;
>> +}
>> +
>> +/**
>> + * ixgbe_parse_valid_functions_cap - Parse IXGBE_ACI_CAPS_VALID_FUNCTIONS caps
>> + * @hw: pointer to the HW struct
>> + * @dev_p: pointer to device capabilities structure
>> + * @cap: capability element to parse
>> + *
>> + * Parse IXGBE_ACI_CAPS_VALID_FUNCTIONS for device capabilities.
>> + */
>> +static void
>> +ixgbe_parse_valid_functions_cap(struct ixgbe_hw *hw,
>> + struct ixgbe_hw_dev_caps *dev_p,
>> + struct ixgbe_aci_cmd_list_caps_elem *cap)
>> +{
>> + u32 number = cap->number;
>[Kalesh] Do you really need a local variable here? Same comment
>applies to functions below as well.
>> +
>> + dev_p->num_funcs = hweight32(number);
>> +}
>> +
>> +/**
>> + * ixgbe_parse_vf_dev_caps - Parse IXGBE_ACI_CAPS_VF device caps
>> + * @hw: pointer to the HW struct
>> + * @dev_p: pointer to device capabilities structure
>> + * @cap: capability element to parse
>> + *
>> + * Parse IXGBE_ACI_CAPS_VF for device capabilities.
>> + */
>> +static void ixgbe_parse_vf_dev_caps(struct ixgbe_hw *hw,
>> + struct ixgbe_hw_dev_caps *dev_p,
>> + struct ixgbe_aci_cmd_list_caps_elem *cap)
>> +{
>> + u32 number = cap->number;
>> +
>> + dev_p->num_vfs_exposed = number;
>> +}
>> +
>> +/**
>> + * ixgbe_parse_vsi_dev_caps - Parse IXGBE_ACI_CAPS_VSI device caps
>> + * @hw: pointer to the HW struct
>> + * @dev_p: pointer to device capabilities structure
>> + * @cap: capability element to parse
>> + *
>> + * Parse IXGBE_ACI_CAPS_VSI for device capabilities.
>> + */
>> +static void ixgbe_parse_vsi_dev_caps(struct ixgbe_hw *hw,
>> + struct ixgbe_hw_dev_caps *dev_p,
>> + struct ixgbe_aci_cmd_list_caps_elem *cap)
>> +{
>> + u32 number = cap->number;
>> +
>> + dev_p->num_vsi_allocd_to_host = number;
>> +}
>> +
>> +/**
>> + * ixgbe_parse_fdir_dev_caps - Parse IXGBE_ACI_CAPS_FD device caps
>> + * @hw: pointer to the HW struct
>> + * @dev_p: pointer to device capabilities structure
>> + * @cap: capability element to parse
>> + *
>> + * Parse IXGBE_ACI_CAPS_FD for device capabilities.
>> + */
>> +static void ixgbe_parse_fdir_dev_caps(struct ixgbe_hw *hw,
>> + struct ixgbe_hw_dev_caps *dev_p,
>> + struct ixgbe_aci_cmd_list_caps_elem *cap)
>> +{
>> + u32 number = cap->number;
>> +
>> + dev_p->num_flow_director_fltr = number;
>> +}
>> +
>> +/**
>> + * ixgbe_parse_dev_caps - Parse device capabilities
>> + * @hw: pointer to the HW struct
>> + * @dev_p: pointer to device capabilities structure
>> + * @buf: buffer containing the device capability records
>> + * @cap_count: the number of capabilities
>> + *
>> + * Helper device to parse device (0x000B) capabilities list. For
>> + * capabilities shared between device and function, this relies on
>> + * ixgbe_parse_e610_caps.
>> + *
>> + * Loop through the list of provided capabilities and extract the relevant
>> + * data into the device capabilities structured.
>> + */
>> +static void ixgbe_parse_dev_caps(struct ixgbe_hw *hw,
>> + struct ixgbe_hw_dev_caps *dev_p,
>> + void *buf, u32 cap_count)
>> +{
>> + struct ixgbe_aci_cmd_list_caps_elem *cap_resp;
>> + u32 i;
>> +
>> + cap_resp = (struct ixgbe_aci_cmd_list_caps_elem *)buf;
>> +
>> + memset(dev_p, 0, sizeof(*dev_p));
>> +
>> + for (i = 0; i < cap_count; i++) {
>> + u16 cap = cap_resp[i].cap;
>> +
>> + ixgbe_parse_e610_caps(hw, &dev_p->common_cap, &cap_resp[i],
>> + "dev caps");
>> +
>> + switch (cap) {
>> + case IXGBE_ACI_CAPS_VALID_FUNCTIONS:
>> + ixgbe_parse_valid_functions_cap(hw, dev_p,
>> + &cap_resp[i]);
>> + break;
>> + case IXGBE_ACI_CAPS_VF:
>> + ixgbe_parse_vf_dev_caps(hw, dev_p, &cap_resp[i]);
>> + break;
>> + case IXGBE_ACI_CAPS_VSI:
>> + ixgbe_parse_vsi_dev_caps(hw, dev_p, &cap_resp[i]);
>> + break;
>> + case IXGBE_ACI_CAPS_FD:
>> + ixgbe_parse_fdir_dev_caps(hw, dev_p, &cap_resp[i]);
>> + break;
>> + default:
>> + /* Don't list common capabilities as unknown */
>> + break;
>> + }
>> + }
>> +}
>> +
>> +/**
>> + * ixgbe_parse_vf_func_caps - Parse IXGBE_ACI_CAPS_VF function caps
>> + * @hw: pointer to the HW struct
>> + * @func_p: pointer to function capabilities structure
>> + * @cap: pointer to the capability element to parse
>> + *
>> + * Extract function capabilities for IXGBE_ACI_CAPS_VF.
>> + */
>> +static void ixgbe_parse_vf_func_caps(struct ixgbe_hw *hw,
>> + struct ixgbe_hw_func_caps *func_p,
>> + struct ixgbe_aci_cmd_list_caps_elem *cap)
>> +{
>> + u32 logical_id = cap->logical_id;
>> + u32 number = cap->number;
>> +
>> + func_p->num_allocd_vfs = number;
>> + func_p->vf_base_id = logical_id;
>> +}
>> +
>> +/**
>> + * ixgbe_get_num_per_func - determine number of resources per PF
>> + * @hw: pointer to the HW structure
>> + * @max: value to be evenly split between each PF
>> + *
>> + * Determine the number of valid functions by going through the bitmap returned
>> + * from parsing capabilities and use this to calculate the number of resources
>> + * per PF based on the max value passed in.
>> + *
>> + * Return: the number of resources per PF or 0, if no PH are available.
>> + */
>> +static u32 ixgbe_get_num_per_func(struct ixgbe_hw *hw, u32 max)
>> +{
>> +#define IXGBE_CAPS_VALID_FUNCS_M GENMASK(7, 0)
>> + u8 funcs = hweight8(hw->dev_caps.common_cap.valid_functions &
>> + IXGBE_CAPS_VALID_FUNCS_M);
>> +
>> + return funcs ? (max / funcs) : 0;
>> +}
>> +
>> +/**
>> + * ixgbe_parse_vsi_func_caps - Parse IXGBE_ACI_CAPS_VSI function caps
>> + * @hw: pointer to the HW struct
>> + * @func_p: pointer to function capabilities structure
>> + * @cap: pointer to the capability element to parse
>> + *
>> + * Extract function capabilities for IXGBE_ACI_CAPS_VSI.
>> + */
>> +static void ixgbe_parse_vsi_func_caps(struct ixgbe_hw *hw,
>> + struct ixgbe_hw_func_caps *func_p,
>> + struct ixgbe_aci_cmd_list_caps_elem *cap)
>> +{
>> + func_p->guar_num_vsi = ixgbe_get_num_per_func(hw, IXGBE_MAX_VSI);
>> +}
>> +
>> +/**
>> + * ixgbe_parse_func_caps - Parse function capabilities
>> + * @hw: pointer to the HW struct
>> + * @func_p: pointer to function capabilities structure
>> + * @buf: buffer containing the function capability records
>> + * @cap_count: the number of capabilities
>> + *
>> + * Helper function to parse function (0x000A) capabilities list. For
>> + * capabilities shared between device and function, this relies on
>> + * ixgbe_parse_e610_caps.
>> + *
>> + * Loop through the list of provided capabilities and extract the relevant
>> + * data into the function capabilities structured.
>> + */
>> +static void ixgbe_parse_func_caps(struct ixgbe_hw *hw,
>> + struct ixgbe_hw_func_caps *func_p,
>> + void *buf, u32 cap_count)
>> +{
>> + struct ixgbe_aci_cmd_list_caps_elem *cap_resp;
>> + u32 i;
>> +
>> + cap_resp = (struct ixgbe_aci_cmd_list_caps_elem *)buf;
>> +
>> + memset(func_p, 0, sizeof(*func_p));
>> +
>> + for (i = 0; i < cap_count; i++) {
>> + u16 cap = cap_resp[i].cap;
>> +
>> + ixgbe_parse_e610_caps(hw, &func_p->common_cap,
>> + &cap_resp[i], "func caps");
>> +
>> + switch (cap) {
>> + case IXGBE_ACI_CAPS_VF:
>> + ixgbe_parse_vf_func_caps(hw, func_p, &cap_resp[i]);
>> + break;
>> + case IXGBE_ACI_CAPS_VSI:
>> + ixgbe_parse_vsi_func_caps(hw, func_p, &cap_resp[i]);
>> + break;
>> + default:
>> + /* Don't list common capabilities as unknown */
>> + break;
>> + }
>> + }
>> +}
>> +
>> +/**
>> + * ixgbe_aci_list_caps - query function/device capabilities
>> + * @hw: pointer to the HW struct
>> + * @buf: a buffer to hold the capabilities
>> + * @buf_size: size of the buffer
>> + * @cap_count: if not NULL, set to the number of capabilities reported
>> + * @opc: capabilities type to discover, device or function
>> + *
>> + * Get the function (0x000A) or device (0x000B) capabilities description from
>> + * firmware and store it in the buffer.
>> + *
>> + * If the cap_count pointer is not NULL, then it is set to the number of
>> + * capabilities firmware will report. Note that if the buffer size is too
>> + * small, it is possible the command will return -ENOMEM. The
>> + * cap_count will still be updated in this case. It is recommended that the
>> + * buffer size be set to IXGBE_ACI_MAX_BUFFER_SIZE (the largest possible
>> + * buffer that firmware could return) to avoid this.
>> + *
>> + * Return: the exit code of the operation.
>> + * Exit code of -ENOMEM means the buffer size is too small.
>> + */
>> +int ixgbe_aci_list_caps(struct ixgbe_hw *hw, void *buf, u16 buf_size,
>> + u32 *cap_count, enum ixgbe_aci_opc opc)
>> +{
>> + struct ixgbe_aci_cmd_list_caps *cmd;
>> + struct ixgbe_aci_desc desc;
>> + int err;
>> +
>> + cmd = &desc.params.get_cap;
>> +
>> + if (opc != ixgbe_aci_opc_list_func_caps &&
>> + opc != ixgbe_aci_opc_list_dev_caps)
>> + return -EINVAL;
>> +
>> + ixgbe_fill_dflt_direct_cmd_desc(&desc, opc);
>> + err = ixgbe_aci_send_cmd(hw, &desc, buf, buf_size);
>> +
>> + if (cap_count)
>[Kalesh] Do you need a check for !err as well here?
Thank you for pointing this out - in fact, it is tried/caught by the callers.
Piotr
[...]
Powered by blists - more mailing lists