[Intel-wired-lan] [PATCH net v1] ice: Fix inventory failed error during flash update

[Intel-wired-lan] [PATCH net v1] ice: Fix inventory failed error during flash update

[Mateusz Palczewski]

From: Sylwester Dziedziuch <sylwesterx.dziedziuch@intel.com>

After updating flash image on E810 card with NVM update tool
there was an error: The inventory process failed.

This was reported at bugzilla thread #2114483 and caused by the tool
trying to read devlink parameters fw.mgmt.minsrev and fw.undi.minsrev
but those parameters were not registered by the driver.

The ice NVM flash has a security revision field for the main NVM bank
and the Option ROM bank. In addition to the revision within the module,
the device also has a minimum security revision TLV area. This minimum
security revision field indicates the minimum value that will be
accepted for the associated security revision when loading the NVM bank.

These parameters are permanent (i.e. stored in flash), and are used to
indicate the minimum security revision of the associated NVM bank. If
the image in the bank has a lower security revision, then the flash
loader will not continue loading that flash bank.

Fix this by adding two new devlink parameters fw.mgmt.minsrev
and fw.undi.minsrev and function to read they respective values.

This idea was proposed before with both write and read funcionality
but was rejected by community. This patch focuses on read only.

Fixes: 1adf7ead8204 ("ice: enable initial devlink support")
Signed-off-by: Sylwester Dziedziuch <sylwesterx.dziedziuch@intel.com>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Signed-off-by: Mateusz Palczewski <mateusz.palczewski@intel.com>
Link: https://lore.kernel.org/netdev/20210129004332.3004826-5-anthony.l.nguyen@intel.com/
---
 Documentation/networking/devlink/ice.rst      | 33 +++++++
 .../net/ethernet/intel/ice/ice_adminq_cmd.h   | 17 ++++
 drivers/net/ethernet/intel/ice/ice_devlink.c  | 90 +++++++++++++++++++
 drivers/net/ethernet/intel/ice/ice_nvm.c      | 53 +++++++++++
 drivers/net/ethernet/intel/ice/ice_nvm.h      |  1 +
 drivers/net/ethernet/intel/ice/ice_type.h     |  8 ++
 6 files changed, 202 insertions(+)

diff --git a/Documentation/networking/devlink/ice.rst b/Documentation/networking/devlink/ice.rst
index 8c082b139bbd..aabd33a7f7da 100644
--- a/Documentation/networking/devlink/ice.rst
+++ b/Documentation/networking/devlink/ice.rst
@@ -90,6 +90,39 @@ The ``ice`` driver reports the following versions
       - 0xee16ced7
       - The first 4 bytes of the hash of the netlist module contents.
 
+Parameters
+==========
+
+The minimum security revision fields of the ice device control whether the
+associated flash section can be loaded. If the security revision field of
+the section -- ``fw.mgmt.srev`` for the main firmware section and
+``fw.undi.srev`` for the Option ROM -- is lower than the associated minimum
+security revision, then the device will not load that section of firmware.
+
+The ``ice`` driver implements driver-specific parameters for reading the
+minimum security revision fields associated those two sections of the device
+flash. Note that the device will not allow lowering a minimum security
+revision, nor will it allow increasing the security revision higher than the
+associated security revision of the active flash image.
+
+.. list-table:: Minimum security revision parameters
+      :widths: 5 5 5 85
+
+   * - Name
+     - Type
+     - Mode
+     - Description
+   * - ``fw.undi.minsrev``
+     - u32
+     - permanent
+     - The device's minimum security revision for the ``fw.undi`` section of
+       the flash.
+   * - ``fw.mgmt.minsrev``
+     - u32
+     - permanent
+     - The device's minimum security revision for the ``fw.mgmt`` section of
+       the flash.
+
 Flash Update
 ============
 
diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
index 9939238573a4..4d46f91adbdc 100644
--- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
+++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
@@ -1509,6 +1509,23 @@ struct ice_aqc_nvm_checksum {
 	u8 rsvd2[12];
 };
 
+#define ICE_AQC_NVM_MINSREV_MOD_ID		0x130
+
+/* Used for reading and writing MinSRev using 0x0701 and 0x0703. Note that the
+ * type field is excluded from the section when reading and writing from
+ * a module using the module_typeid field with these AQ commands.
+ */
+struct ice_aqc_nvm_minsrev {
+	__le16 length;
+	__le16 validity;
+#define ICE_AQC_NVM_MINSREV_NVM_VALID          BIT(0)
+#define ICE_AQC_NVM_MINSREV_OROM_VALID         BIT(1)
+	__le16 nvm_minsrev_l;
+	__le16 nvm_minsrev_h;
+	__le16 orom_minsrev_l;
+	__le16 orom_minsrev_h;
+};
+
 /* Used for NVM Set Package Data command - 0x070A */
 struct ice_aqc_nvm_pkg_data {
 	u8 reserved[3];
diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
index 3337314a7b35..95f1653306d5 100644
--- a/drivers/net/ethernet/intel/ice/ice_devlink.c
+++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
@@ -372,6 +372,83 @@ static int ice_devlink_info_get(struct devlink *devlink,
 	return err;
 }
 
+enum ice_devlink_param_id {
+	ICE_DEVLINK_PARAM_ID_BASE = DEVLINK_PARAM_GENERIC_ID_MAX,
+	ICE_DEVLINK_PARAM_ID_FW_MGMT_MINSREV,
+	ICE_DEVLINK_PARAM_ID_FW_UNDI_MINSREV,
+};
+
+/**
+ * ice_devlink_minsrev_get - Get the current minimum security revision
+ * @devlink: pointer to the devlink instance
+ * @id: the parameter ID to get
+ * @ctx: context to return the parameter value
+ *
+ * Returns: zero on success, or an error code on failure.
+ */
+static int
+ice_devlink_minsrev_get(struct devlink *devlink, u32 id,
+			struct devlink_param_gset_ctx *ctx)
+{
+	struct ice_pf *pf = devlink_priv(devlink);
+	struct device *dev = ice_pf_to_dev(pf);
+	struct ice_minsrev_info minsrevs = {};
+
+	if (id != ICE_DEVLINK_PARAM_ID_FW_MGMT_MINSREV &&
+	    id != ICE_DEVLINK_PARAM_ID_FW_UNDI_MINSREV)
+		return -EINVAL;
+
+	if (ice_get_nvm_minsrevs(&pf->hw, &minsrevs)) {
+		dev_warn(dev, "Failed to read minimum security revision data from flash\n");
+		return -EIO;
+	}
+
+	/* We report zero if the device has not yet had a valid minimum
+	 * security revision programmed for the associated module. This makes
+	 * sense because it is not possible to have a security revision of
+	 * less than zero. Thus, all images will be able to load if the
+	 * minimum security revision is zero, the same as the case where the
+	 * minimum value is indicated as invalid.
+	 */
+	switch (id) {
+	case ICE_DEVLINK_PARAM_ID_FW_MGMT_MINSREV:
+		if (minsrevs.nvm_valid)
+			ctx->val.vu32 = minsrevs.nvm;
+		else
+			ctx->val.vu32 = 0;
+		break;
+	case ICE_DEVLINK_PARAM_ID_FW_UNDI_MINSREV:
+		if (minsrevs.orom_valid)
+			ctx->val.vu32 = minsrevs.orom;
+		else
+			ctx->val.vu32 = 0;
+		break;
+	}
+
+	return 0;
+}
+
+/**
+ * ice_devlink_minsrev_set - Set the minimum security revision
+ * @devlink: pointer to the devlink instance
+ * @id: the parameter ID to set
+ * @ctx: context to return the parameter value
+ *
+ * Currently manually changing minimum security revision is not supported.
+ *
+ * Returns: EINVAL.
+ */
+static int
+ice_devlink_minsrev_set(struct devlink *devlink, u32 id,
+			struct devlink_param_gset_ctx *ctx)
+{
+	struct ice_pf *pf = devlink_priv(devlink);
+
+	dev_err(ice_pf_to_dev(pf), "Setting minimum security revision is not available\n");
+
+	return -EINVAL;
+}
+
 /**
  * ice_devlink_reload_empr_start - Start EMP reset to activate new firmware
  * @devlink: pointer to the devlink instance to reload
@@ -589,6 +666,19 @@ static const struct devlink_param ice_devlink_params[] = {
 			      ice_devlink_enable_iw_get,
 			      ice_devlink_enable_iw_set,
 			      ice_devlink_enable_iw_validate),
+	DEVLINK_PARAM_DRIVER(ICE_DEVLINK_PARAM_ID_FW_MGMT_MINSREV,
+			     "fw.mgmt.minsrev",
+			     DEVLINK_PARAM_TYPE_U32,
+			     BIT(DEVLINK_PARAM_CMODE_PERMANENT),
+			     ice_devlink_minsrev_get,
+			     ice_devlink_minsrev_set, NULL),
+	DEVLINK_PARAM_DRIVER(ICE_DEVLINK_PARAM_ID_FW_UNDI_MINSREV,
+			     "fw.undi.minsrev",
+			     DEVLINK_PARAM_TYPE_U32,
+			     BIT(DEVLINK_PARAM_CMODE_PERMANENT),
+			     ice_devlink_minsrev_get,
+			     ice_devlink_minsrev_set, NULL),
+
 
 };
 
diff --git a/drivers/net/ethernet/intel/ice/ice_nvm.c b/drivers/net/ethernet/intel/ice/ice_nvm.c
index 13cdb5ea594d..1c3fa733387d 100644
--- a/drivers/net/ethernet/intel/ice/ice_nvm.c
+++ b/drivers/net/ethernet/intel/ice/ice_nvm.c
@@ -1139,6 +1139,59 @@ int ice_nvm_write_activate(struct ice_hw *hw, u8 cmd_flags, u8 *response_flags)
 	return err;
 }
 
+/**
+ * ice_get_nvm_minsrevs - Get the Minimum Security Revision values from flash
+ * @hw: pointer to the HW struct
+ * @minsrevs: structure to store NVM and OROM minsrev values
+ *
+ * Read the Minimum Security Revision TLV and extract the revision values from
+ * the flash image into a readable structure for processing.
+ */
+int ice_get_nvm_minsrevs(struct ice_hw *hw, struct ice_minsrev_info *minsrevs)
+{
+	struct ice_aqc_nvm_minsrev data;
+	int status;
+	u16 valid;
+
+	status = ice_acquire_nvm(hw, ICE_RES_READ);
+	if (status)
+		return status;
+
+	status = ice_aq_read_nvm(hw, ICE_AQC_NVM_MINSREV_MOD_ID, 0,
+				 sizeof(data), &data, true, false, NULL);
+
+	ice_release_nvm(hw);
+
+	if (status)
+		return status;
+
+	valid = le16_to_cpu(data.validity);
+
+	/* Extract NVM minimum security revision */
+	if (valid & ICE_AQC_NVM_MINSREV_NVM_VALID) {
+		u16 minsrev_l, minsrev_h;
+
+		minsrev_l = le16_to_cpu(data.nvm_minsrev_l);
+		minsrev_h = le16_to_cpu(data.nvm_minsrev_h);
+
+		minsrevs->nvm = minsrev_h << 16 | minsrev_l;
+		minsrevs->nvm_valid = true;
+	}
+
+	/* Extract the OROM minimum security revision */
+	if (valid & ICE_AQC_NVM_MINSREV_OROM_VALID) {
+		u16 minsrev_l, minsrev_h;
+
+		minsrev_l = le16_to_cpu(data.orom_minsrev_l);
+		minsrev_h = le16_to_cpu(data.orom_minsrev_h);
+
+		minsrevs->orom = minsrev_h << 16 | minsrev_l;
+		minsrevs->orom_valid = true;
+	}
+
+	return 0;
+}
+
 /**
  * ice_aq_nvm_update_empr
  * @hw: pointer to the HW struct
diff --git a/drivers/net/ethernet/intel/ice/ice_nvm.h b/drivers/net/ethernet/intel/ice/ice_nvm.h
index 856d1ad4398b..b44ecb8b9341 100644
--- a/drivers/net/ethernet/intel/ice/ice_nvm.h
+++ b/drivers/net/ethernet/intel/ice/ice_nvm.h
@@ -20,6 +20,7 @@ ice_read_flat_nvm(struct ice_hw *hw, u32 offset, u32 *length, u8 *data,
 int
 ice_get_pfa_module_tlv(struct ice_hw *hw, u16 *module_tlv, u16 *module_tlv_len,
 		       u16 module_type);
+int ice_get_nvm_minsrevs(struct ice_hw *hw, struct ice_minsrev_info *minsrevs);
 int ice_get_inactive_orom_ver(struct ice_hw *hw, struct ice_orom_info *orom);
 int ice_get_inactive_nvm_ver(struct ice_hw *hw, struct ice_nvm_info *nvm);
 int
diff --git a/drivers/net/ethernet/intel/ice/ice_type.h b/drivers/net/ethernet/intel/ice/ice_type.h
index 861b64322959..c14fa57b1cb7 100644
--- a/drivers/net/ethernet/intel/ice/ice_type.h
+++ b/drivers/net/ethernet/intel/ice/ice_type.h
@@ -428,6 +428,14 @@ struct ice_nvm_info {
 	u8 minor;
 };
 
+/* Minimum Security Revision information */
+struct ice_minsrev_info {
+	u32 nvm;
+	u32 orom;
+	u8 nvm_valid : 1;
+	u8 orom_valid : 1;
+};
+
 /* netlist version information */
 struct ice_netlist_info {
 	u32 major;			/* major high/low */
-- 
2.27.0

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net v1] ice: Fix inventory failed error during flash update

[Tony Nguyen]

On 8/11/2022 4:45 AM, Mateusz Palczewski wrote:
> From: Sylwester Dziedziuch <sylwesterx.dziedziuch@intel.com>
> 
> After updating flash image on E810 card with NVM update tool
> there was an error: The inventory process failed.
> 
> This was reported at bugzilla thread #2114483 and caused by the tool
> trying to read devlink parameters fw.mgmt.minsrev and fw.undi.minsrev
> but those parameters were not registered by the driver.
> 
> The ice NVM flash has a security revision field for the main NVM bank
> and the Option ROM bank. In addition to the revision within the module,
> the device also has a minimum security revision TLV area. This minimum
> security revision field indicates the minimum value that will be
> accepted for the associated security revision when loading the NVM bank.
> 
> These parameters are permanent (i.e. stored in flash), and are used to
> indicate the minimum security revision of the associated NVM bank. If
> the image in the bank has a lower security revision, then the flash
> loader will not continue loading that flash bank.
> 
> Fix this by adding two new devlink parameters fw.mgmt.minsrev
> and fw.undi.minsrev and function to read they respective values.
> 
> This idea was proposed before with both write and read funcionality
> but was rejected by community. This patch focuses on read only.

Adding Jake for his input since he sent the initial series.

> Fixes: 1adf7ead8204 ("ice: enable initial devlink support")
> Signed-off-by: Sylwester Dziedziuch <sylwesterx.dziedziuch@intel.com>
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> Signed-off-by: Mateusz Palczewski <mateusz.palczewski@intel.com>
> Link: https://lore.kernel.org/netdev/20210129004332.3004826-5-anthony.l.nguyen@intel.com/
> ---
>   Documentation/networking/devlink/ice.rst      | 33 +++++++
>   .../net/ethernet/intel/ice/ice_adminq_cmd.h   | 17 ++++
>   drivers/net/ethernet/intel/ice/ice_devlink.c  | 90 +++++++++++++++++++
>   drivers/net/ethernet/intel/ice/ice_nvm.c      | 53 +++++++++++
>   drivers/net/ethernet/intel/ice/ice_nvm.h      |  1 +
>   drivers/net/ethernet/intel/ice/ice_type.h     |  8 ++
>   6 files changed, 202 insertions(+)
> 
> diff --git a/Documentation/networking/devlink/ice.rst b/Documentation/networking/devlink/ice.rst
> index 8c082b139bbd..aabd33a7f7da 100644
> --- a/Documentation/networking/devlink/ice.rst
> +++ b/Documentation/networking/devlink/ice.rst
> @@ -90,6 +90,39 @@ The ``ice`` driver reports the following versions
>         - 0xee16ced7
>         - The first 4 bytes of the hash of the netlist module contents.
>   
> +Parameters
> +==========
> +
> +The minimum security revision fields of the ice device control whether the
> +associated flash section can be loaded. If the security revision field of
> +the section -- ``fw.mgmt.srev`` for the main firmware section and
> +``fw.undi.srev`` for the Option ROM -- is lower than the associated minimum
> +security revision, then the device will not load that section of firmware.
> +
> +The ``ice`` driver implements driver-specific parameters for reading the
> +minimum security revision fields associated those two sections of the device
> +flash. Note that the device will not allow lowering a minimum security
> +revision, nor will it allow increasing the security revision higher than the
> +associated security revision of the active flash image.
> +
> +.. list-table:: Minimum security revision parameters
> +      :widths: 5 5 5 85
> +
> +   * - Name
> +     - Type
> +     - Mode
> +     - Description
> +   * - ``fw.undi.minsrev``
> +     - u32
> +     - permanent
> +     - The device's minimum security revision for the ``fw.undi`` section of
> +       the flash.
> +   * - ``fw.mgmt.minsrev``
> +     - u32
> +     - permanent
> +     - The device's minimum security revision for the ``fw.mgmt`` section of
> +       the flash.
> +
>   Flash Update
>   ============
>   
> diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
> index 9939238573a4..4d46f91adbdc 100644
> --- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
> +++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
> @@ -1509,6 +1509,23 @@ struct ice_aqc_nvm_checksum {
>   	u8 rsvd2[12];
>   };
>   
> +#define ICE_AQC_NVM_MINSREV_MOD_ID		0x130
> +
> +/* Used for reading and writing MinSRev using 0x0701 and 0x0703. Note that the
> + * type field is excluded from the section when reading and writing from
> + * a module using the module_typeid field with these AQ commands.
> + */
> +struct ice_aqc_nvm_minsrev {
> +	__le16 length;
> +	__le16 validity;
> +#define ICE_AQC_NVM_MINSREV_NVM_VALID          BIT(0)
> +#define ICE_AQC_NVM_MINSREV_OROM_VALID         BIT(1)
> +	__le16 nvm_minsrev_l;
> +	__le16 nvm_minsrev_h;
> +	__le16 orom_minsrev_l;
> +	__le16 orom_minsrev_h;
> +};
> +
>   /* Used for NVM Set Package Data command - 0x070A */
>   struct ice_aqc_nvm_pkg_data {
>   	u8 reserved[3];
> diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
> index 3337314a7b35..95f1653306d5 100644
> --- a/drivers/net/ethernet/intel/ice/ice_devlink.c
> +++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
> @@ -372,6 +372,83 @@ static int ice_devlink_info_get(struct devlink *devlink,
>   	return err;
>   }
>   
> +enum ice_devlink_param_id {
> +	ICE_DEVLINK_PARAM_ID_BASE = DEVLINK_PARAM_GENERIC_ID_MAX,
> +	ICE_DEVLINK_PARAM_ID_FW_MGMT_MINSREV,
> +	ICE_DEVLINK_PARAM_ID_FW_UNDI_MINSREV,
> +};
> +
> +/**
> + * ice_devlink_minsrev_get - Get the current minimum security revision
> + * @devlink: pointer to the devlink instance
> + * @id: the parameter ID to get
> + * @ctx: context to return the parameter value
> + *
> + * Returns: zero on success, or an error code on failure.
> + */
> +static int
> +ice_devlink_minsrev_get(struct devlink *devlink, u32 id,
> +			struct devlink_param_gset_ctx *ctx)
> +{
> +	struct ice_pf *pf = devlink_priv(devlink);
> +	struct device *dev = ice_pf_to_dev(pf);
> +	struct ice_minsrev_info minsrevs = {};
> +
> +	if (id != ICE_DEVLINK_PARAM_ID_FW_MGMT_MINSREV &&
> +	    id != ICE_DEVLINK_PARAM_ID_FW_UNDI_MINSREV)
> +		return -EINVAL;
> +
> +	if (ice_get_nvm_minsrevs(&pf->hw, &minsrevs)) {
> +		dev_warn(dev, "Failed to read minimum security revision data from flash\n");
> +		return -EIO;
> +	}
> +
> +	/* We report zero if the device has not yet had a valid minimum
> +	 * security revision programmed for the associated module. This makes
> +	 * sense because it is not possible to have a security revision of
> +	 * less than zero. Thus, all images will be able to load if the
> +	 * minimum security revision is zero, the same as the case where the
> +	 * minimum value is indicated as invalid.
> +	 */
> +	switch (id) {
> +	case ICE_DEVLINK_PARAM_ID_FW_MGMT_MINSREV:
> +		if (minsrevs.nvm_valid)
> +			ctx->val.vu32 = minsrevs.nvm;
> +		else
> +			ctx->val.vu32 = 0;
> +		break;
> +	case ICE_DEVLINK_PARAM_ID_FW_UNDI_MINSREV:
> +		if (minsrevs.orom_valid)
> +			ctx->val.vu32 = minsrevs.orom;
> +		else
> +			ctx->val.vu32 = 0;
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * ice_devlink_minsrev_set - Set the minimum security revision
> + * @devlink: pointer to the devlink instance
> + * @id: the parameter ID to set
> + * @ctx: context to return the parameter value
> + *
> + * Currently manually changing minimum security revision is not supported.
> + *
> + * Returns: EINVAL.
> + */
> +static int
> +ice_devlink_minsrev_set(struct devlink *devlink, u32 id,
> +			struct devlink_param_gset_ctx *ctx)
> +{
> +	struct ice_pf *pf = devlink_priv(devlink);
> +
> +	dev_err(ice_pf_to_dev(pf), "Setting minimum security revision is not available\n");
> +
> +	return -EINVAL;
> +}
> +
>   /**
>    * ice_devlink_reload_empr_start - Start EMP reset to activate new firmware
>    * @devlink: pointer to the devlink instance to reload
> @@ -589,6 +666,19 @@ static const struct devlink_param ice_devlink_params[] = {
>   			      ice_devlink_enable_iw_get,
>   			      ice_devlink_enable_iw_set,
>   			      ice_devlink_enable_iw_validate),
> +	DEVLINK_PARAM_DRIVER(ICE_DEVLINK_PARAM_ID_FW_MGMT_MINSREV,
> +			     "fw.mgmt.minsrev",
> +			     DEVLINK_PARAM_TYPE_U32,
> +			     BIT(DEVLINK_PARAM_CMODE_PERMANENT),
> +			     ice_devlink_minsrev_get,
> +			     ice_devlink_minsrev_set, NULL),
> +	DEVLINK_PARAM_DRIVER(ICE_DEVLINK_PARAM_ID_FW_UNDI_MINSREV,
> +			     "fw.undi.minsrev",
> +			     DEVLINK_PARAM_TYPE_U32,
> +			     BIT(DEVLINK_PARAM_CMODE_PERMANENT),
> +			     ice_devlink_minsrev_get,
> +			     ice_devlink_minsrev_set, NULL),
> +
>   
>   };
>   
> diff --git a/drivers/net/ethernet/intel/ice/ice_nvm.c b/drivers/net/ethernet/intel/ice/ice_nvm.c
> index 13cdb5ea594d..1c3fa733387d 100644
> --- a/drivers/net/ethernet/intel/ice/ice_nvm.c
> +++ b/drivers/net/ethernet/intel/ice/ice_nvm.c
> @@ -1139,6 +1139,59 @@ int ice_nvm_write_activate(struct ice_hw *hw, u8 cmd_flags, u8 *response_flags)
>   	return err;
>   }
>   
> +/**
> + * ice_get_nvm_minsrevs - Get the Minimum Security Revision values from flash
> + * @hw: pointer to the HW struct
> + * @minsrevs: structure to store NVM and OROM minsrev values
> + *
> + * Read the Minimum Security Revision TLV and extract the revision values from
> + * the flash image into a readable structure for processing.
> + */
> +int ice_get_nvm_minsrevs(struct ice_hw *hw, struct ice_minsrev_info *minsrevs)
> +{
> +	struct ice_aqc_nvm_minsrev data;
> +	int status;
> +	u16 valid;
> +
> +	status = ice_acquire_nvm(hw, ICE_RES_READ);
> +	if (status)
> +		return status;
> +
> +	status = ice_aq_read_nvm(hw, ICE_AQC_NVM_MINSREV_MOD_ID, 0,
> +				 sizeof(data), &data, true, false, NULL);
> +
> +	ice_release_nvm(hw);
> +
> +	if (status)
> +		return status;
> +
> +	valid = le16_to_cpu(data.validity);
> +
> +	/* Extract NVM minimum security revision */
> +	if (valid & ICE_AQC_NVM_MINSREV_NVM_VALID) {
> +		u16 minsrev_l, minsrev_h;
> +
> +		minsrev_l = le16_to_cpu(data.nvm_minsrev_l);
> +		minsrev_h = le16_to_cpu(data.nvm_minsrev_h);
> +
> +		minsrevs->nvm = minsrev_h << 16 | minsrev_l;
> +		minsrevs->nvm_valid = true;
> +	}
> +
> +	/* Extract the OROM minimum security revision */
> +	if (valid & ICE_AQC_NVM_MINSREV_OROM_VALID) {
> +		u16 minsrev_l, minsrev_h;
> +
> +		minsrev_l = le16_to_cpu(data.orom_minsrev_l);
> +		minsrev_h = le16_to_cpu(data.orom_minsrev_h);
> +
> +		minsrevs->orom = minsrev_h << 16 | minsrev_l;
> +		minsrevs->orom_valid = true;
> +	}
> +
> +	return 0;
> +}
> +
>   /**
>    * ice_aq_nvm_update_empr
>    * @hw: pointer to the HW struct
> diff --git a/drivers/net/ethernet/intel/ice/ice_nvm.h b/drivers/net/ethernet/intel/ice/ice_nvm.h
> index 856d1ad4398b..b44ecb8b9341 100644
> --- a/drivers/net/ethernet/intel/ice/ice_nvm.h
> +++ b/drivers/net/ethernet/intel/ice/ice_nvm.h
> @@ -20,6 +20,7 @@ ice_read_flat_nvm(struct ice_hw *hw, u32 offset, u32 *length, u8 *data,
>   int
>   ice_get_pfa_module_tlv(struct ice_hw *hw, u16 *module_tlv, u16 *module_tlv_len,
>   		       u16 module_type);
> +int ice_get_nvm_minsrevs(struct ice_hw *hw, struct ice_minsrev_info *minsrevs);
>   int ice_get_inactive_orom_ver(struct ice_hw *hw, struct ice_orom_info *orom);
>   int ice_get_inactive_nvm_ver(struct ice_hw *hw, struct ice_nvm_info *nvm);
>   int
> diff --git a/drivers/net/ethernet/intel/ice/ice_type.h b/drivers/net/ethernet/intel/ice/ice_type.h
> index 861b64322959..c14fa57b1cb7 100644
> --- a/drivers/net/ethernet/intel/ice/ice_type.h
> +++ b/drivers/net/ethernet/intel/ice/ice_type.h
> @@ -428,6 +428,14 @@ struct ice_nvm_info {
>   	u8 minor;
>   };
>   
> +/* Minimum Security Revision information */
> +struct ice_minsrev_info {
> +	u32 nvm;
> +	u32 orom;
> +	u8 nvm_valid : 1;
> +	u8 orom_valid : 1;
> +};
> +
>   /* netlist version information */
>   struct ice_netlist_info {
>   	u32 major;			/* major high/low */
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net v1] ice: Fix inventory failed error during flash update

[Tony Nguyen]

On 8/11/2022 4:45 AM, Mateusz Palczewski wrote:
> From: Sylwester Dziedziuch <sylwesterx.dziedziuch@intel.com>
> 
> After updating flash image on E810 card with NVM update tool
> there was an error: The inventory process failed.
> 
> This was reported at bugzilla thread #2114483 and caused by the tool
> trying to read devlink parameters fw.mgmt.minsrev and fw.undi.minsrev
> but those parameters were not registered by the driver.

Pointing to an issue when using with our userspace tool is not a good 
justification of why this should be accepted into the kernel.

> The ice NVM flash has a security revision field for the main NVM bank
> and the Option ROM bank. In addition to the revision within the module,
> the device also has a minimum security revision TLV area. This minimum
> security revision field indicates the minimum value that will be
> accepted for the associated security revision when loading the NVM bank.
> 
> These parameters are permanent (i.e. stored in flash), and are used to
> indicate the minimum security revision of the associated NVM bank. If
> the image in the bank has a lower security revision, then the flash
> loader will not continue loading that flash bank.
> 
> Fix this by adding two new devlink parameters fw.mgmt.minsrev
> and fw.undi.minsrev and function to read they respective values.
> 
> This idea was proposed before with both write and read funcionality
> but was rejected by community. This patch focuses on read only.

How is this different/addresses the issues that caused it to be rejected 
initially? What makes it acceptable now?

> Fixes: 1adf7ead8204 ("ice: enable initial devlink support")

#1 this is too big for net. #2 This is not fixing a bug for a tool that 
the community is concerned about.

> Signed-off-by: Sylwester Dziedziuch <sylwesterx.dziedziuch@intel.com>
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> Signed-off-by: Mateusz Palczewski <mateusz.palczewski@intel.com>
> Link: https://lore.kernel.org/netdev/20210129004332.3004826-5-anthony.l.nguyen@intel.com/
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net v1] ice: Fix inventory failed error during flash update

[Dziedziuch, SylwesterX]

> On 8/11/2022 4:45 AM, Mateusz Palczewski wrote:
> > From: Sylwester Dziedziuch <sylwesterx.dziedziuch@intel.com>
> >
> > After updating flash image on E810 card with NVM update tool there was
> > an error: The inventory process failed.
> >
> > This was reported at bugzilla thread #2114483 and caused by the tool
> > trying to read devlink parameters fw.mgmt.minsrev and fw.undi.minsrev
> > but those parameters were not registered by the driver.
> 
> Pointing to an issue when using with our userspace tool is not a good
> justification of why this should be accepted into the kernel.
> 
> > The ice NVM flash has a security revision field for the main NVM bank
> > and the Option ROM bank. In addition to the revision within the
> > module, the device also has a minimum security revision TLV area. This
> > minimum security revision field indicates the minimum value that will
> > be accepted for the associated security revision when loading the NVM
> bank.
> >
> > These parameters are permanent (i.e. stored in flash), and are used to
> > indicate the minimum security revision of the associated NVM bank. If
> > the image in the bank has a lower security revision, then the flash
> > loader will not continue loading that flash bank.
> >
> > Fix this by adding two new devlink parameters fw.mgmt.minsrev and
> > fw.undi.minsrev and function to read they respective values.
> >
> > This idea was proposed before with both write and read funcionality
> > but was rejected by community. This patch focuses on read only.
> 
> How is this different/addresses the issues that caused it to be rejected
> initially? What makes it acceptable now?

One of the concerns in the previous review was that we give the ability to change those values manually which might cause security issues. So in this change we are not allowing to modify those values only to read them for the update process to finish without errors.

> 
> > Fixes: 1adf7ead8204 ("ice: enable initial devlink support")
> 
> #1 this is too big for net. #2 This is not fixing a bug for a tool that the
> community is concerned about.

This issue is actually reported by Red Hat and is fixing the Red Hat Bugzilla mentioned in the commit message.

> 
> > Signed-off-by: Sylwester Dziedziuch <sylwesterx.dziedziuch@intel.com>
> > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> > Signed-off-by: Mateusz Palczewski <mateusz.palczewski@intel.com>
> > Link:
> > https://lore.kernel.org/netdev/20210129004332.3004826-5-anthony.l.nguy
> > en@intel.com/
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net v1] ice: Fix inventory failed error during flash update

[Tony Nguyen]

On 8/19/2022 12:21 AM, Dziedziuch, SylwesterX wrote:
>> On 8/11/2022 4:45 AM, Mateusz Palczewski wrote:
>>> From: Sylwester Dziedziuch <sylwesterx.dziedziuch@intel.com>
>>>
>>> After updating flash image on E810 card with NVM update tool there was
>>> an error: The inventory process failed.
>>>
>>> This was reported at bugzilla thread #2114483 and caused by the tool
>>> trying to read devlink parameters fw.mgmt.minsrev and fw.undi.minsrev
>>> but those parameters were not registered by the driver.
>>
>> Pointing to an issue when using with our userspace tool is not a good
>> justification of why this should be accepted into the kernel.
>>
>>> The ice NVM flash has a security revision field for the main NVM bank
>>> and the Option ROM bank. In addition to the revision within the
>>> module, the device also has a minimum security revision TLV area. This
>>> minimum security revision field indicates the minimum value that will
>>> be accepted for the associated security revision when loading the NVM
>> bank.
>>>
>>> These parameters are permanent (i.e. stored in flash), and are used to
>>> indicate the minimum security revision of the associated NVM bank. If
>>> the image in the bank has a lower security revision, then the flash
>>> loader will not continue loading that flash bank.
>>>
>>> Fix this by adding two new devlink parameters fw.mgmt.minsrev and
>>> fw.undi.minsrev and function to read they respective values.
>>>
>>> This idea was proposed before with both write and read funcionality
>>> but was rejected by community. This patch focuses on read only.
>>
>> How is this different/addresses the issues that caused it to be rejected
>> initially? What makes it acceptable now?
> 
> One of the concerns in the previous review was that we give the ability to change those values manually which might cause security issues. So in this change we are not allowing to modify those values only to read them for the update process to finish without errors.

Let's put this patch on pause for the moment and discuss internally. I 
think, at the very least, the commit message would need some updates to 
it so review could continue on a v2.

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan