linux-kernel - Re: [EXTERNAL] [PATCH v4 04/15] soc: qcom: ice: add hwkm support in ice

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Mon, 5 Feb 2024 00:02:07 +0530
From: Kamlesh Gurudasani <kamlesh@...com>
To: Gaurav Kashyap <quic_gaurkash@...cinc.com>,
        <linux-arm-msm@...r.kernel.org>, <linux-scsi@...r.kernel.org>,
        <andersson@...nel.org>, <ebiggers@...gle.com>,
        <neil.armstrong@...aro.org>, <srinivas.kandagatla@...aro.org>,
        <krzysztof.kozlowski+dt@...aro.org>, <conor+dt@...nel.org>,
        <robh+dt@...nel.org>
CC: <linux-kernel@...r.kernel.org>, <linux-mmc@...r.kernel.org>,
        <kernel@...cinc.com>, <linux-crypto@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <quic_omprsing@...cinc.com>,
        <quic_nguyenb@...cinc.com>, <bartosz.golaszewski@...aro.org>,
        <konrad.dybcio@...aro.org>, <ulf.hansson@...aro.org>,
        <jejb@...ux.ibm.com>, <martin.petersen@...cle.com>, <mani@...nel.org>,
        <davem@...emloft.net>, <herbert@...dor.apana.org.au>,
        Gaurav Kashyap <quic_gaurkash@...cinc.com>
Subject: Re: [EXTERNAL] [PATCH v4 04/15] soc: qcom: ice: add hwkm support in
 ice

Gaurav Kashyap <quic_gaurkash@...cinc.com> writes:

..

> +	/*
> +	 * When ICE is in standard (hwkm) mode, it supports HW wrapped
> +	 * keys, and when it is in legacy mode, it only supports standard
> +	 * (non HW wrapped) keys.
> +	 *
> +	 * Put ICE in standard mode, ICE defaults to legacy mode.
> +	 * Legacy mode - ICE HWKM slave not supported.
> +	 * Standard mode - ICE HWKM slave supported.
> +	 *
> +	 * Depending on the version of HWKM, it is controlled by different
> +	 * registers in ICE.
> +	 */
> +	if (ice->hwkm_version >= 2) {
> +		val = qcom_ice_readl(ice, QCOM_ICE_REG_CONTROL);
> +		val = val & 0xFFFFFFFE;
>From the code I understand that the last bit is used for setting the
mode.

Was wondering if it would make more sense to use ~BIT(0) or GENMASK
to generate this value and #define this value to express the work it
does.

In this case, something like #define xx_SET_MODE_MASK (~BIT(0)) or use GENMASK

This would make it easier for a person who is taking a look at the code
for first time. This is just my perspective.

If you do agree, you can change them at multiple places accross this
patch, wherever magic numbers are used for val and masking the value.

Regards,
Kamlesh

> +		qcom_ice_writel(ice, val, QCOM_ICE_REG_CONTROL);
> +	} else {
> +		qcom_ice_writel(ice, 0x7, HWKM_OFFSET(QCOM_ICE_REG_HWKM_TZ_KM_CTL));
> +	}
> +}
> +
> +static void qcom_ice_hwkm_init(struct qcom_ice *ice)
> +{
> +	/* Disable CRC checks. This HWKM feature is not used. */
> +	qcom_ice_writel(ice, 0x6,
> +			HWKM_OFFSET(QCOM_ICE_REG_HWKM_TZ_KM_CTL));
> +
> +	/*
> +	 * Give register bank of the HWKM slave access to read and modify
> +	 * the keyslots in ICE HWKM slave. Without this, trustzone will not
> +	 * be able to program keys into ICE.
> +	 */
> +	qcom_ice_writel(ice, 0xFFFFFFFF, HWKM_OFFSET(QCOM_ICE_REG_HWKM_BANK0_BBAC_0));
> +	qcom_ice_writel(ice, 0xFFFFFFFF, HWKM_OFFSET(QCOM_ICE_REG_HWKM_BANK0_BBAC_1));
> +	qcom_ice_writel(ice, 0xFFFFFFFF, HWKM_OFFSET(QCOM_ICE_REG_HWKM_BANK0_BBAC_2));
> +	qcom_ice_writel(ice, 0xFFFFFFFF, HWKM_OFFSET(QCOM_ICE_REG_HWKM_BANK0_BBAC_3));
> +	qcom_ice_writel(ice, 0xFFFFFFFF, HWKM_OFFSET(QCOM_ICE_REG_HWKM_BANK0_BBAC_4));
> +
> +	/* Clear HWKM response FIFO before doing anything */
> +	qcom_ice_writel(ice, 0x8, HWKM_OFFSET(QCOM_ICE_REG_HWKM_BANK0_BANKN_IRQ_STATUS));
> +	ice->hwkm_init_complete = true;
> +}

..