| 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: <d2192fec-4edf-4943-8045-517d8bae390f@os.amperecomputing.com>
Date: Fri, 14 Feb 2025 17:59:44 -0500
From: Paul Benoit <paul@...amperecomputing.com>
To: Sudeep Holla <sudeep.holla@....com>
Cc: linux-kernel@...r.kernel.org, Mark Rutland <mark.rutland@....com>,
Lorenzo Pieralisi <lpieralisi@...nel.org>,
linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v3] firmware: smccc: Support optional Arm SMC SOC_ID name
Hi Sudeep,
Thank you for your feedback.
I hope to submit the suggested changes as v4 of the patch early next week.
On 2/10/2025 7:13 AM, Sudeep Holla wrote:
> Mostly minor coding style comments from me, otherwise LGTM.
>
> On Tue, Dec 17, 2024 at 04:13:38PM -0800, Paul Benoit wrote:
>
> Split the commit into multiple paragraphs, it looks too crowded 😄 >
>> Issue Number 1.6 of the Arm SMC Calling Convention introduces an
>> optional SOC_ID name string.
>
>> If available, point the 'machine' field of
>
> ^^^^ I prefer implemented instead of available.
I changed the wording to use "implemented" rather than "available".
>
>> the SoC Device Attributes at this string so that it will appear under
>> /sys/bus/soc/devices/soc0/machine.
>
> Break into new paragraph here.
I made the split, and adjusted the line lengths/breaks so as to not
exceed 75 columns/characters.
>
>> 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
>> ---
>>
>> v2->v3: Add conditionalization to exclude SOC_ID Name from 32-bit builds.
>> v1->v2: Address code review identified issues.
>>
>> drivers/firmware/smccc/soc_id.c | 79 +++++++++++++++++++++++++++++++++
>> include/linux/arm-smccc.h | 37 +++++++++++++++
>> 2 files changed, 116 insertions(+)
>>
>> diff --git a/drivers/firmware/smccc/soc_id.c b/drivers/firmware/smccc/soc_id.c
>> index 1990263fbba0..3b50ff5d2cbd 100644
>> --- a/drivers/firmware/smccc/soc_id.c
>> +++ b/drivers/firmware/smccc/soc_id.c
>> @@ -32,6 +32,12 @@
>> static struct soc_device *soc_dev;
>> static struct soc_device_attribute *soc_dev_attr;
>>
>> +static char __init *smccc_soc_name_init(void);
>> +
>
> Not really needed if you move you code before smccc_soc_init()
>
It has been eliminated.
>> +#ifdef CONFIG_ARM64
>> +static char __ro_after_init smccc_soc_id_name[136] = "";
>
> Move all in one block under #ifdef, details below.
>
I have made that change.
>> +#endif
>> +
>> static int __init smccc_soc_init(void)
>> {
>> int soc_id_rev, soc_id_version;
>> @@ -72,6 +78,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 = smccc_soc_name_init();
>>
>> soc_dev = soc_device_register(soc_dev_attr);
>> if (IS_ERR(soc_dev)) {
>> @@ -93,3 +100,75 @@ static void __exit smccc_soc_exit(void)
>> kfree(soc_dev_attr);
>> }
>> module_exit(smccc_soc_exit);
>
> Generally it good to have module_{init,exit} at the end of the file.
> Move you additions above these.
>
Agreed. I have made that change as part of your suggestion to "Move all
in one block under #ifdef".
>> +
>> +
>> +#ifdef CONFIG_ARM64
>> +static inline void str_fragment_from_reg(char *dst, unsigned long 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;
>> +}
>> +#endif
>> +
>> +static char __init *smccc_soc_name_init(void)
>> +{
>> +#ifdef CONFIG_ARM64
>> + struct arm_smccc_1_2_regs args;
>> + struct arm_smccc_1_2_regs res;
>> + size_t len;
>> +
>> + /*
>> + * 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.
>> + */
>> + args.a0 = ARM_SMCCC_ARCH_SOC_ID;
>> + args.a1 = 2; /* SOC_ID name */
>> + arm_smccc_1_2_invoke(&args, &res);
>> + if ((u32)res.a0 == 0) {
>> + const unsigned int regsize = sizeof(res.a1);
>> +
>> + /*
>> + * Copy res.a1..res.a17 to the smccc_soc_id_name string
>> + * 8 bytes at a time. 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.
>> + */
>> + str_fragment_from_reg(smccc_soc_id_name + 0*regsize, res.a1);
>> + str_fragment_from_reg(smccc_soc_id_name + 1*regsize, res.a2);
>> + str_fragment_from_reg(smccc_soc_id_name + 2*regsize, res.a3);
>> + str_fragment_from_reg(smccc_soc_id_name + 3*regsize, res.a4);
>> + str_fragment_from_reg(smccc_soc_id_name + 4*regsize, res.a5);
>> + str_fragment_from_reg(smccc_soc_id_name + 5*regsize, res.a6);
>> + str_fragment_from_reg(smccc_soc_id_name + 6*regsize, res.a7);
>> + str_fragment_from_reg(smccc_soc_id_name + 7*regsize, res.a8);
>> + str_fragment_from_reg(smccc_soc_id_name + 8*regsize, res.a9);
>> + str_fragment_from_reg(smccc_soc_id_name + 9*regsize, res.a10);
>> + str_fragment_from_reg(smccc_soc_id_name + 10*regsize, res.a11);
>> + str_fragment_from_reg(smccc_soc_id_name + 11*regsize, res.a12);
>> + str_fragment_from_reg(smccc_soc_id_name + 12*regsize, res.a13);
>> + str_fragment_from_reg(smccc_soc_id_name + 13*regsize, res.a14);
>> + str_fragment_from_reg(smccc_soc_id_name + 14*regsize, res.a15);
>> + str_fragment_from_reg(smccc_soc_id_name + 15*regsize, res.a16);
>> + str_fragment_from_reg(smccc_soc_id_name + 16*regsize, res.a17);
>> +
>> + len = strnlen(smccc_soc_id_name, sizeof(smccc_soc_id_name));
>> + if (len) {
>> + if (len == sizeof(smccc_soc_id_name))
>> + pr_warn(FW_BUG "Ignoring improperly formatted Name\n");
>> + else
>> + return smccc_soc_id_name;
>> + }
>> + }
>> +#endif
>> +
>> + return NULL;
>> +}
>
> Can we improve readability with
>
> #ifdef CONFIG_ARM64
>
> static char __ro_after_init smccc_soc_id_name[136] = "";
>
> <both str_fragment_from_reg and smccc_soc_name_init here>
>
> #else
> static char __init *smccc_soc_name_init(void)
> {
> return NULL;
> }
>
> #endif
>
I have made the change suggested above.
>> diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
>> index 67f6fdf2e7cd..9d444e5862fe 100644
>> --- a/include/linux/arm-smccc.h
>> +++ b/include/linux/arm-smccc.h
>> @@ -607,6 +607,12 @@ asmlinkage void __arm_smccc_hvc(unsigned long a0, unsigned long a1,
>> ___res->a0 = SMCCC_RET_NOT_SUPPORTED; \
>> } while (0)
>>
>> +#define __fail_smccc_1_2(___res) \
>> + do { \
>> + if (___res) \
>> + ___res->a0 = SMCCC_RET_NOT_SUPPORTED; \
>> + } while (0)
>> +
>> /*
>> * arm_smccc_1_1_invoke() - make an SMCCC v1.1 compliant call
>> *
>> @@ -639,5 +645,36 @@ asmlinkage void __arm_smccc_hvc(unsigned long a0, unsigned long a1,
>> method; \
>> })
>>
>> +/*
>> + * arm_smccc_1_2_invoke() - make an SMCCC v1.2 compliant call
>> + *
>> + * @args: SMC args are in the a0..a17 fields of the arm_smcc_1_2_regs structure
>> + * @res: result values from registers 0 to 17
>> + *
>> + * This macro will make either an HVC call or an SMC call depending on the
>> + * current SMCCC conduit. If no valid conduit is available then -1
>> + * (SMCCC_RET_NOT_SUPPORTED) is returned in @res.a0 (if supplied).
>> + *
>> + * The return value also provides the conduit that was used.
>> + */
>> +#define arm_smccc_1_2_invoke(args, res) ({ \
>> + struct arm_smccc_1_2_regs *__args = args; \
>
> I think we can move this macro and the above under CONFIG_ARM64 as
> arm_smccc_1_2_regs is defined only for ARM64 for now. Otherwise one
> could use this macro and get undefined compiler errors for the structure.
>
I added another #ifdef CONFIG_ARM64 around arm_smccc_1_2_invoke. It
seemed cleaner to keep arm_smccc_1_2_invoke located right after
arm_smccc_1_1_invoke rather than to move it amongst the other
CONFIG_ARM64 conditionalization for struct definitions, prototypes, etc.
Though, I'm open to moving it if you think that is better than having
it immediately follow arm_smccc_1_1_invoke.
Powered by blists - more mailing lists