| 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: <627d2a20-3af3-4993-830d-741af2c13aa2@os.amperecomputing.com>
Date: Mon, 25 Nov 2024 17:52:41 -0500
From: Paul Benoit <paul@...amperecomputing.com>
To: Mark Rutland <mark.rutland@....com>
Cc: linux-kernel@...r.kernel.org, Lorenzo Pieralisi <lpieralisi@...nel.org>,
Sudeep Holla <sudeep.holla@....com>, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH] firmware: smccc: Support optional Arm SMC SOC_ID name
On 11/14/2024 7:22 AM, Mark Rutland wrote:
> Hi Paul,
>
> On Wed, Nov 13, 2024 at 07:04:52PM -0800, Paul Benoit wrote:
>> Issue Number 1.6 of the Arm SMC Calling Convention introduces an
>> optional SOC_ID name string. If available, point the 'machine' field of
>> the SoC Device Attributes at this string so that it will appear under
>> /sys/bus/soc/devices/soc0/machine. On Arm SMC compliant SoCs, this will
>> allow things like 'lscpu' to eventually get a SoC provider model name
>> from there rather than each tool/utility needing to get a possibly
>> inconsistent, obsolete, or incorrect model/machine name from its own
>> hardcoded model/machine name table.
>>
>> Signed-off-by: Paul Benoit <paul@...amperecomputing.com>
>> Cc: Mark Rutland <mark.rutland@....com>
>> Cc: Lorenzo Pieralisi <lpieralisi@...nel.org>
>> Cc: Sudeep Holla <sudeep.holla@....com>
>> Cc: linux-arm-kernel@...ts.infradead.org
>> ---
>> drivers/firmware/smccc/smccc.c | 70 +++++++++++++++++++++++++++++++++
>> drivers/firmware/smccc/soc_id.c | 1 +
>> include/linux/arm-smccc.h | 10 +++++
>> 3 files changed, 81 insertions(+)
>>
>> diff --git a/drivers/firmware/smccc/smccc.c b/drivers/firmware/smccc/smccc.c
>> index a74600d9f2d7..1c7084b0b8d7 100644
>> --- a/drivers/firmware/smccc/smccc.c
>> +++ b/drivers/firmware/smccc/smccc.c
>> @@ -18,10 +18,12 @@ static enum arm_smccc_conduit smccc_conduit = SMCCC_CONDUIT_NONE;
>> bool __ro_after_init smccc_trng_available = false;
>> s32 __ro_after_init smccc_soc_id_version = SMCCC_RET_NOT_SUPPORTED;
>> s32 __ro_after_init smccc_soc_id_revision = SMCCC_RET_NOT_SUPPORTED;
>> +char __ro_after_init smccc_soc_id_name[136] = "";
>>
>> void __init arm_smccc_version_init(u32 version, enum arm_smccc_conduit conduit)
>> {
>> struct arm_smccc_res res;
>> + struct arm_smccc_1_2_regs regs_1_2;
>>
>> smccc_version = version;
>> smccc_conduit = conduit;
>> @@ -37,6 +39,66 @@ void __init arm_smccc_version_init(u32 version, enum arm_smccc_conduit conduit)
>> smccc_soc_id_version = (s32)res.a0;
>> arm_smccc_1_1_invoke(ARM_SMCCC_ARCH_SOC_ID, 1, &res);
>> smccc_soc_id_revision = (s32)res.a0;
>> +
>> + /* Issue Number 1.6 of the Arm SMC Calling Convention
>> + * specification introduces an optional "name" string
>> + * to the ARM_SMCCC_ARCH_SOC_ID function. Fetch it if
>> + * available.
>> + */
>
> I think the code for the SOC_ID name should live under soc_id.c, since
> it *only* matters to the sysfs interface (and will not be used by other
> code in the kernel to identify the SOC). That should be initialised
> under smccc_soc_init(), ideally factored into a smccc_soc_name_init()
> helper.
>
> Nit: comments should have the leading '/*' on its own line, e.g.
>
> /*
> * Multi-line comments should be formatted like this, with a
> * leading and trailing line.
> */
>
>> + regs_1_2.a0 = ARM_SMCCC_ARCH_SOC_ID;
>> + regs_1_2.a1 = 2; /* SOC_ID name */
>> + arm_smccc_1_2_smc(
>> + (const struct arm_smccc_1_2_regs *)®s_1_2,
>> + (struct arm_smccc_1_2_regs *)®s_1_2);
>
> These casts shouldn't be necessary, and they look suspicious.
>
> Additionally, this should be using whichever conduit SMCCC happens to be
> using rather than assuming SMC. As with the rest of this code using
> arm_smccc_1_1_invoke(), we should add a arm_smccc_1_2_invoke() wrapper
> for that, with this looking like:
>
> arm_smccc_1_2_invoke(®s_1_2, ®s_1_2);
>> +
>> + if ((u32)regs_1_2.a0 == 0) {
>> + unsigned long *destination =
>> + (unsigned long *)smccc_soc_id_name;
>> +
>> + /*
>> + * Copy regs_1_2.a1..regs_1_2.a17 to the
>> + * smccc_soc_id_name string with consideration
>> + * to the endianness of the values in a1..a17.
>
> This indicates that we have to do *something* about endianness, but
> doesn't say *what* consideration is necessary, which is rather
> unhelpful -- it would be better to describe the format of the registers,
> which would indicate what specifically we need to do.
>
> For example:
>
> The string is packed into registers a1 to a17 such that each
> register contains 8 successive bytes, and within each register
> byte N of the string fragment is encoded into bits [8*N+7:8*N].
>
>> + * As per Issue 1.6 of the Arm SMC Calling
>> + * Convention, the string will be NUL terminated
>> + * and padded, from the end of the string to
>> + * the end of the 136 byte buffer, with NULs.
>> + */
>> + *destination++ =
>> + cpu_to_le64p((const __u64 *)®s_1_2.a1);
>
> If you used cpu_to_le64(), you wouldn't need a cast, though either way you will
> need a cast on the return value since 'destination' is not an array of __le64,
> and sparse should complain.
>
> This isn't really an endianness conversion since we're unpacking smaller
> elements out of a larger container, so I reckon it would be better to
> handle this explicitly, e.g.
>
> void str_fragment_from_reg(char *dst, u64 reg)
> {
> dst[0] = (reg >> 0) & 0xff;
> dst[1] = (reg >> 8) & 0xff;
> dst[2] = (reg >> 16) & 0xff;
> dst[3] = (reg >> 24) & 0xff;
> dst[4] = (reg >> 32) & 0xff;
> dst[5] = (reg >> 40) & 0xff;
> dst[6] = (reg >> 48) & 0xff;
> dst[7] = (reg >> 56) & 0xff;
> }
>
> ... and then using that:
>
> str_fragment_from_reg(destination + 0, regs_1_2.a1);
> str_fragment_from_reg(destination + 8, regs_1_2.a2);
> str_fragment_from_reg(destination + 16, regs_1_2.a3);
> str_fragment_from_reg(destination + 24, regs_1_2.a4);
> ...
>
> That way we avoid all the messy casting, and we can more clearly align
> this with the way the spec says this is packed.
>
> The generated code looks sane (with GCC 14.2.0, at least):
>
> // little-endian kernel
> 0000000000000000 <str_fragment_from_reg>:
> 0: f9000001 str x1, [x0]
> 4: d65f03c0 ret
>
> // big-endian kernel
> 0000000000000000 <str_fragment_from_reg>:
> 0: dac00c21 rev x1, x1
> 4: f9000001 str x1, [x0]
> 8: d65f03c0 ret
>
>> + *destination++ =
>> + cpu_to_le64p((const __u64 *)®s_1_2.a2);
>> + *destination++ =
>> + cpu_to_le64p((const __u64 *)®s_1_2.a3);
>> + *destination++ =
>> + cpu_to_le64p((const __u64 *)®s_1_2.a4);
>> + *destination++ =
>> + cpu_to_le64p((const __u64 *)®s_1_2.a5);
>> + *destination++ =
>> + cpu_to_le64p((const __u64 *)®s_1_2.a6);
>> + *destination++ =
>> + cpu_to_le64p((const __u64 *)®s_1_2.a7);
>> + *destination++ =
>> + cpu_to_le64p((const __u64 *)®s_1_2.a8);
>> + *destination++ =
>> + cpu_to_le64p((const __u64 *)®s_1_2.a9);
>> + *destination++ =
>> + cpu_to_le64p((const __u64 *)®s_1_2.a10);
>> + *destination++ =
>> + cpu_to_le64p((const __u64 *)®s_1_2.a11);
>> + *destination++ =
>> + cpu_to_le64p((const __u64 *)®s_1_2.a12);
>> + *destination++ =
>> + cpu_to_le64p((const __u64 *)®s_1_2.a13);
>> + *destination++ =
>> + cpu_to_le64p((const __u64 *)®s_1_2.a14);
>> + *destination++ =
>> + cpu_to_le64p((const __u64 *)®s_1_2.a15);
>> + *destination++ =
>> + cpu_to_le64p((const __u64 *)®s_1_2.a16);
>> + *destination++ =
>> + cpu_to_le64p((const __u64 *)®s_1_2.a17);
>
> We probably want to check that the string is actually NUL terminated
> here, and log a FW BUG message if it is not.
>
>> + }
>> }
>> }
>> }
>> @@ -67,6 +129,14 @@ s32 arm_smccc_get_soc_id_revision(void)
>> }
>> EXPORT_SYMBOL_GPL(arm_smccc_get_soc_id_revision);
>>
>> +char *arm_smccc_get_soc_id_name(void)
>> +{
>> + if (strnlen(smccc_soc_id_name, sizeof(smccc_soc_id_name)))
>> + return smccc_soc_id_name;
>> +
>> + return NULL;
>> +}
>
> As above, I think this can be folded into the probing routine.
>
>> +
>> static int __init smccc_devices_init(void)
>> {
>> struct platform_device *pdev;
>> diff --git a/drivers/firmware/smccc/soc_id.c b/drivers/firmware/smccc/soc_id.c
>> index 1990263fbba0..6f698c703868 100644
>> --- a/drivers/firmware/smccc/soc_id.c
>> +++ b/drivers/firmware/smccc/soc_id.c
>> @@ -72,6 +72,7 @@ static int __init smccc_soc_init(void)
>> soc_dev_attr->soc_id = soc_id_str;
>> soc_dev_attr->revision = soc_id_rev_str;
>> soc_dev_attr->family = soc_id_jep106_id_str;
>> + soc_dev_attr->machine = arm_smccc_get_soc_id_name();
>>
>> soc_dev = soc_device_register(soc_dev_attr);
>> if (IS_ERR(soc_dev)) {
>> diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
>> index 67f6fdf2e7cd..5935cf636135 100644
>> --- a/include/linux/arm-smccc.h
>> +++ b/include/linux/arm-smccc.h
>> @@ -333,6 +333,16 @@ s32 arm_smccc_get_soc_id_version(void);
>> */
>> s32 arm_smccc_get_soc_id_revision(void);
>>
>> +/**
>> + * arm_smccc_get_soc_id_name()
>> + *
>> + * Returns the SOC ID name.
>> + *
>> + * When ARM_SMCCC_ARCH_SOC_ID name is not present, returns NULL.
>> + */
>> +char *arm_smccc_get_soc_id_name(void);
>
> I don't think this needs to be exposed outside of the SOC ID code.
>
> Thanks,
> Mark.
Hi Mark,
Thanks for the quick response and detailed review. I'll soon be
submitting a v2 patch that addresses the issues that you have identified.
Powered by blists - more mailing lists