[PATCH] firmware: smccc: Support optional Arm SMC SOC_ID name

[PATCH] firmware: smccc: Support optional Arm SMC SOC_ID name

[Paul Benoit]

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@os.amperecomputing.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Lorenzo Pieralisi <lpieralisi@kernel.org>
Cc: Sudeep Holla <sudeep.holla@arm.com>
Cc: linux-arm-kernel@lists.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.
+			 */
+			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 *)&regs_1_2,
+				(struct arm_smccc_1_2_regs *)&regs_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.
+				 * 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 *)&regs_1_2.a1);
+				*destination++ =
+				    cpu_to_le64p((const __u64 *)&regs_1_2.a2);
+				*destination++ =
+				    cpu_to_le64p((const __u64 *)&regs_1_2.a3);
+				*destination++ =
+				    cpu_to_le64p((const __u64 *)&regs_1_2.a4);
+				*destination++ =
+				    cpu_to_le64p((const __u64 *)&regs_1_2.a5);
+				*destination++ =
+				    cpu_to_le64p((const __u64 *)&regs_1_2.a6);
+				*destination++ =
+				    cpu_to_le64p((const __u64 *)&regs_1_2.a7);
+				*destination++ =
+				    cpu_to_le64p((const __u64 *)&regs_1_2.a8);
+				*destination++ =
+				    cpu_to_le64p((const __u64 *)&regs_1_2.a9);
+				*destination++ =
+				    cpu_to_le64p((const __u64 *)&regs_1_2.a10);
+				*destination++ =
+				    cpu_to_le64p((const __u64 *)&regs_1_2.a11);
+				*destination++ =
+				    cpu_to_le64p((const __u64 *)&regs_1_2.a12);
+				*destination++ =
+				    cpu_to_le64p((const __u64 *)&regs_1_2.a13);
+				*destination++ =
+				    cpu_to_le64p((const __u64 *)&regs_1_2.a14);
+				*destination++ =
+				    cpu_to_le64p((const __u64 *)&regs_1_2.a15);
+				*destination++ =
+				    cpu_to_le64p((const __u64 *)&regs_1_2.a16);
+				*destination++ =
+				    cpu_to_le64p((const __u64 *)&regs_1_2.a17);
+			}
 		}
 	}
 }
@@ -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;
+}
+
 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);
+
+
 /**
  * struct arm_smccc_res - Result from SMC/HVC call
  * @a0-a3 result values from registers 0 to 3
-- 
2.46.2

Re: [PATCH] firmware: smccc: Support optional Arm SMC SOC_ID name

[Mark Rutland]

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@os.amperecomputing.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Lorenzo Pieralisi <lpieralisi@kernel.org>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Cc: linux-arm-kernel@lists.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 *)&regs_1_2,
> +				(struct arm_smccc_1_2_regs *)&regs_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(&regs_1_2, &regs_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 *)&regs_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 *)&regs_1_2.a2);
> +				*destination++ =
> +				    cpu_to_le64p((const __u64 *)&regs_1_2.a3);
> +				*destination++ =
> +				    cpu_to_le64p((const __u64 *)&regs_1_2.a4);
> +				*destination++ =
> +				    cpu_to_le64p((const __u64 *)&regs_1_2.a5);
> +				*destination++ =
> +				    cpu_to_le64p((const __u64 *)&regs_1_2.a6);
> +				*destination++ =
> +				    cpu_to_le64p((const __u64 *)&regs_1_2.a7);
> +				*destination++ =
> +				    cpu_to_le64p((const __u64 *)&regs_1_2.a8);
> +				*destination++ =
> +				    cpu_to_le64p((const __u64 *)&regs_1_2.a9);
> +				*destination++ =
> +				    cpu_to_le64p((const __u64 *)&regs_1_2.a10);
> +				*destination++ =
> +				    cpu_to_le64p((const __u64 *)&regs_1_2.a11);
> +				*destination++ =
> +				    cpu_to_le64p((const __u64 *)&regs_1_2.a12);
> +				*destination++ =
> +				    cpu_to_le64p((const __u64 *)&regs_1_2.a13);
> +				*destination++ =
> +				    cpu_to_le64p((const __u64 *)&regs_1_2.a14);
> +				*destination++ =
> +				    cpu_to_le64p((const __u64 *)&regs_1_2.a15);
> +				*destination++ =
> +				    cpu_to_le64p((const __u64 *)&regs_1_2.a16);
> +				*destination++ =
> +				    cpu_to_le64p((const __u64 *)&regs_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.

Re: [PATCH] firmware: smccc: Support optional Arm SMC SOC_ID name

[kernel test robot]

Hi Paul,

kernel test robot noticed the following build errors:

[auto build test ERROR on soc/for-next]
[also build test ERROR on linus/master v6.12-rc7 next-20241115]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Paul-Benoit/firmware-smccc-Support-optional-Arm-SMC-SOC_ID-name/20241114-113039
base:   https://git.kernel.org/pub/scm/linux/kernel/git/soc/soc.git for-next
patch link:    https://lore.kernel.org/r/20241114030452.10149-1-paul%40os.amperecomputing.com
patch subject: [PATCH] firmware: smccc: Support optional Arm SMC SOC_ID name
config: arm-randconfig-002-20241117 (https://download.01.org/0day-ci/archive/20241117/202411170820.t1xuMJVL-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 592c0fe55f6d9a811028b5f3507be91458ab2713)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241117/202411170820.t1xuMJVL-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202411170820.t1xuMJVL-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/firmware/smccc/smccc.c:26:28: error: variable has incomplete type 'struct arm_smccc_1_2_regs'
      26 |         struct arm_smccc_1_2_regs regs_1_2;
         |                                   ^
   drivers/firmware/smccc/smccc.c:26:9: note: forward declaration of 'struct arm_smccc_1_2_regs'
      26 |         struct arm_smccc_1_2_regs regs_1_2;
         |                ^
>> drivers/firmware/smccc/smccc.c:50:4: error: call to undeclared function 'arm_smccc_1_2_smc'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
      50 |                         arm_smccc_1_2_smc(
         |                         ^
   2 errors generated.


vim +26 drivers/firmware/smccc/smccc.c

    22	
    23	void __init arm_smccc_version_init(u32 version, enum arm_smccc_conduit conduit)
    24	{
    25		struct arm_smccc_res res;
  > 26		struct arm_smccc_1_2_regs regs_1_2;
    27	
    28		smccc_version = version;
    29		smccc_conduit = conduit;
    30	
    31		smccc_trng_available = smccc_probe_trng();
    32	
    33		if ((smccc_version >= ARM_SMCCC_VERSION_1_2) &&
    34		    (smccc_conduit != SMCCC_CONDUIT_NONE)) {
    35			arm_smccc_1_1_invoke(ARM_SMCCC_ARCH_FEATURES_FUNC_ID,
    36					     ARM_SMCCC_ARCH_SOC_ID, &res);
    37			if ((s32)res.a0 >= 0) {
    38				arm_smccc_1_1_invoke(ARM_SMCCC_ARCH_SOC_ID, 0, &res);
    39				smccc_soc_id_version = (s32)res.a0;
    40				arm_smccc_1_1_invoke(ARM_SMCCC_ARCH_SOC_ID, 1, &res);
    41				smccc_soc_id_revision = (s32)res.a0;
    42	
    43				/* Issue Number 1.6 of the Arm SMC Calling Convention
    44				 * specification introduces an optional "name" string
    45				 * to the ARM_SMCCC_ARCH_SOC_ID function.  Fetch it if
    46				 * available.
    47				 */
    48				regs_1_2.a0 = ARM_SMCCC_ARCH_SOC_ID;
    49				regs_1_2.a1 = 2;	/* SOC_ID name */
  > 50				arm_smccc_1_2_smc(
    51					(const struct arm_smccc_1_2_regs *)&regs_1_2,
    52					(struct arm_smccc_1_2_regs *)&regs_1_2);
    53	
    54				if ((u32)regs_1_2.a0 == 0) {
    55					unsigned long *destination =
    56						(unsigned long *)smccc_soc_id_name;
    57	
    58					/*
    59					 * Copy regs_1_2.a1..regs_1_2.a17 to the
    60					 * smccc_soc_id_name string with consideration
    61					 * to the endianness of the values in a1..a17.
    62					 * As per Issue 1.6 of the Arm SMC Calling
    63					 * Convention, the string will be NUL terminated
    64					 * and padded, from the end of the string to
    65					 * the end of the 136 byte buffer, with NULs.
    66					 */
    67					*destination++ =
    68					    cpu_to_le64p((const __u64 *)&regs_1_2.a1);
    69					*destination++ =
    70					    cpu_to_le64p((const __u64 *)&regs_1_2.a2);
    71					*destination++ =
    72					    cpu_to_le64p((const __u64 *)&regs_1_2.a3);
    73					*destination++ =
    74					    cpu_to_le64p((const __u64 *)&regs_1_2.a4);
    75					*destination++ =
    76					    cpu_to_le64p((const __u64 *)&regs_1_2.a5);
    77					*destination++ =
    78					    cpu_to_le64p((const __u64 *)&regs_1_2.a6);
    79					*destination++ =
    80					    cpu_to_le64p((const __u64 *)&regs_1_2.a7);
    81					*destination++ =
    82					    cpu_to_le64p((const __u64 *)&regs_1_2.a8);
    83					*destination++ =
    84					    cpu_to_le64p((const __u64 *)&regs_1_2.a9);
    85					*destination++ =
    86					    cpu_to_le64p((const __u64 *)&regs_1_2.a10);
    87					*destination++ =
    88					    cpu_to_le64p((const __u64 *)&regs_1_2.a11);
    89					*destination++ =
    90					    cpu_to_le64p((const __u64 *)&regs_1_2.a12);
    91					*destination++ =
    92					    cpu_to_le64p((const __u64 *)&regs_1_2.a13);
    93					*destination++ =
    94					    cpu_to_le64p((const __u64 *)&regs_1_2.a14);
    95					*destination++ =
    96					    cpu_to_le64p((const __u64 *)&regs_1_2.a15);
    97					*destination++ =
    98					    cpu_to_le64p((const __u64 *)&regs_1_2.a16);
    99					*destination++ =
   100					    cpu_to_le64p((const __u64 *)&regs_1_2.a17);
   101				}
   102			}
   103		}
   104	}
   105	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH] firmware: smccc: Support optional Arm SMC SOC_ID name

[kernel test robot]

Hi Paul,

kernel test robot noticed the following build errors:

[auto build test ERROR on soc/for-next]
[also build test ERROR on linus/master v6.12-rc7 next-20241115]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Paul-Benoit/firmware-smccc-Support-optional-Arm-SMC-SOC_ID-name/20241114-113039
base:   https://git.kernel.org/pub/scm/linux/kernel/git/soc/soc.git for-next
patch link:    https://lore.kernel.org/r/20241114030452.10149-1-paul%40os.amperecomputing.com
patch subject: [PATCH] firmware: smccc: Support optional Arm SMC SOC_ID name
config: arm-randconfig-003-20241117 (https://download.01.org/0day-ci/archive/20241117/202411171059.1mPeDZMS-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241117/202411171059.1mPeDZMS-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202411171059.1mPeDZMS-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

   drivers/firmware/smccc/smccc.c: In function 'arm_smccc_version_init':
>> drivers/firmware/smccc/smccc.c:26:35: error: storage size of 'regs_1_2' isn't known
      26 |         struct arm_smccc_1_2_regs regs_1_2;
         |                                   ^~~~~~~~
>> drivers/firmware/smccc/smccc.c:50:25: error: implicit declaration of function 'arm_smccc_1_2_smc'; did you mean 'arm_smccc_1_1_smc'? [-Wimplicit-function-declaration]
      50 |                         arm_smccc_1_2_smc(
         |                         ^~~~~~~~~~~~~~~~~
         |                         arm_smccc_1_1_smc
>> drivers/firmware/smccc/smccc.c:26:35: warning: unused variable 'regs_1_2' [-Wunused-variable]
      26 |         struct arm_smccc_1_2_regs regs_1_2;
         |                                   ^~~~~~~~


vim +26 drivers/firmware/smccc/smccc.c

    22	
    23	void __init arm_smccc_version_init(u32 version, enum arm_smccc_conduit conduit)
    24	{
    25		struct arm_smccc_res res;
  > 26		struct arm_smccc_1_2_regs regs_1_2;
    27	
    28		smccc_version = version;
    29		smccc_conduit = conduit;
    30	
    31		smccc_trng_available = smccc_probe_trng();
    32	
    33		if ((smccc_version >= ARM_SMCCC_VERSION_1_2) &&
    34		    (smccc_conduit != SMCCC_CONDUIT_NONE)) {
    35			arm_smccc_1_1_invoke(ARM_SMCCC_ARCH_FEATURES_FUNC_ID,
    36					     ARM_SMCCC_ARCH_SOC_ID, &res);
    37			if ((s32)res.a0 >= 0) {
    38				arm_smccc_1_1_invoke(ARM_SMCCC_ARCH_SOC_ID, 0, &res);
    39				smccc_soc_id_version = (s32)res.a0;
    40				arm_smccc_1_1_invoke(ARM_SMCCC_ARCH_SOC_ID, 1, &res);
    41				smccc_soc_id_revision = (s32)res.a0;
    42	
    43				/* Issue Number 1.6 of the Arm SMC Calling Convention
    44				 * specification introduces an optional "name" string
    45				 * to the ARM_SMCCC_ARCH_SOC_ID function.  Fetch it if
    46				 * available.
    47				 */
    48				regs_1_2.a0 = ARM_SMCCC_ARCH_SOC_ID;
    49				regs_1_2.a1 = 2;	/* SOC_ID name */
  > 50				arm_smccc_1_2_smc(
    51					(const struct arm_smccc_1_2_regs *)&regs_1_2,
    52					(struct arm_smccc_1_2_regs *)&regs_1_2);
    53	
    54				if ((u32)regs_1_2.a0 == 0) {
    55					unsigned long *destination =
    56						(unsigned long *)smccc_soc_id_name;
    57	
    58					/*
    59					 * Copy regs_1_2.a1..regs_1_2.a17 to the
    60					 * smccc_soc_id_name string with consideration
    61					 * to the endianness of the values in a1..a17.
    62					 * As per Issue 1.6 of the Arm SMC Calling
    63					 * Convention, the string will be NUL terminated
    64					 * and padded, from the end of the string to
    65					 * the end of the 136 byte buffer, with NULs.
    66					 */
    67					*destination++ =
    68					    cpu_to_le64p((const __u64 *)&regs_1_2.a1);
    69					*destination++ =
    70					    cpu_to_le64p((const __u64 *)&regs_1_2.a2);
    71					*destination++ =
    72					    cpu_to_le64p((const __u64 *)&regs_1_2.a3);
    73					*destination++ =
    74					    cpu_to_le64p((const __u64 *)&regs_1_2.a4);
    75					*destination++ =
    76					    cpu_to_le64p((const __u64 *)&regs_1_2.a5);
    77					*destination++ =
    78					    cpu_to_le64p((const __u64 *)&regs_1_2.a6);
    79					*destination++ =
    80					    cpu_to_le64p((const __u64 *)&regs_1_2.a7);
    81					*destination++ =
    82					    cpu_to_le64p((const __u64 *)&regs_1_2.a8);
    83					*destination++ =
    84					    cpu_to_le64p((const __u64 *)&regs_1_2.a9);
    85					*destination++ =
    86					    cpu_to_le64p((const __u64 *)&regs_1_2.a10);
    87					*destination++ =
    88					    cpu_to_le64p((const __u64 *)&regs_1_2.a11);
    89					*destination++ =
    90					    cpu_to_le64p((const __u64 *)&regs_1_2.a12);
    91					*destination++ =
    92					    cpu_to_le64p((const __u64 *)&regs_1_2.a13);
    93					*destination++ =
    94					    cpu_to_le64p((const __u64 *)&regs_1_2.a14);
    95					*destination++ =
    96					    cpu_to_le64p((const __u64 *)&regs_1_2.a15);
    97					*destination++ =
    98					    cpu_to_le64p((const __u64 *)&regs_1_2.a16);
    99					*destination++ =
   100					    cpu_to_le64p((const __u64 *)&regs_1_2.a17);
   101				}
   102			}
   103		}
   104	}
   105	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH] firmware: smccc: Support optional Arm SMC SOC_ID name

[Paul Benoit]

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@os.amperecomputing.com>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: Lorenzo Pieralisi <lpieralisi@kernel.org>
>> Cc: Sudeep Holla <sudeep.holla@arm.com>
>> Cc: linux-arm-kernel@lists.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 *)&regs_1_2,
>> +				(struct arm_smccc_1_2_regs *)&regs_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(&regs_1_2, &regs_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 *)&regs_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 *)&regs_1_2.a2);
>> +				*destination++ =
>> +				    cpu_to_le64p((const __u64 *)&regs_1_2.a3);
>> +				*destination++ =
>> +				    cpu_to_le64p((const __u64 *)&regs_1_2.a4);
>> +				*destination++ =
>> +				    cpu_to_le64p((const __u64 *)&regs_1_2.a5);
>> +				*destination++ =
>> +				    cpu_to_le64p((const __u64 *)&regs_1_2.a6);
>> +				*destination++ =
>> +				    cpu_to_le64p((const __u64 *)&regs_1_2.a7);
>> +				*destination++ =
>> +				    cpu_to_le64p((const __u64 *)&regs_1_2.a8);
>> +				*destination++ =
>> +				    cpu_to_le64p((const __u64 *)&regs_1_2.a9);
>> +				*destination++ =
>> +				    cpu_to_le64p((const __u64 *)&regs_1_2.a10);
>> +				*destination++ =
>> +				    cpu_to_le64p((const __u64 *)&regs_1_2.a11);
>> +				*destination++ =
>> +				    cpu_to_le64p((const __u64 *)&regs_1_2.a12);
>> +				*destination++ =
>> +				    cpu_to_le64p((const __u64 *)&regs_1_2.a13);
>> +				*destination++ =
>> +				    cpu_to_le64p((const __u64 *)&regs_1_2.a14);
>> +				*destination++ =
>> +				    cpu_to_le64p((const __u64 *)&regs_1_2.a15);
>> +				*destination++ =
>> +				    cpu_to_le64p((const __u64 *)&regs_1_2.a16);
>> +				*destination++ =
>> +				    cpu_to_le64p((const __u64 *)&regs_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.

[PATCH v2] firmware: smccc: Support optional Arm SMC SOC_ID name

[Paul Benoit]

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@os.amperecomputing.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Lorenzo Pieralisi <lpieralisi@kernel.org>
Cc: Sudeep Holla <sudeep.holla@arm.com>
Cc: linux-arm-kernel@lists.infradead.org
---

v1->v2: Address code review identified issues.

 drivers/firmware/smccc/soc_id.c | 75 +++++++++++++++++++++++++++++++++
 include/linux/arm-smccc.h       | 37 ++++++++++++++++
 2 files changed, 112 insertions(+)

diff --git a/drivers/firmware/smccc/soc_id.c b/drivers/firmware/smccc/soc_id.c
index 1990263fbba0..b72d100bdf31 100644
--- a/drivers/firmware/smccc/soc_id.c
+++ b/drivers/firmware/smccc/soc_id.c
@@ -32,6 +32,10 @@
 static struct soc_device *soc_dev;
 static struct soc_device_attribute *soc_dev_attr;
 
+static char __init *smccc_soc_name_init(void);
+
+static char __ro_after_init smccc_soc_id_name[136] = "";
+
 static int __init smccc_soc_init(void)
 {
 	int soc_id_rev, soc_id_version;
@@ -72,6 +76,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 +98,73 @@ static void __exit smccc_soc_exit(void)
 	kfree(soc_dev_attr);
 }
 module_exit(smccc_soc_exit);
+
+
+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;
+}
+
+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;
+}
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;		\
+		struct arm_smccc_1_2_regs *__res = res;			\
+		int method = arm_smccc_1_1_get_conduit();		\
+		switch (method) {					\
+		case SMCCC_CONDUIT_HVC:					\
+			arm_smccc_1_2_hvc(__args, __res);		\
+			break;						\
+		case SMCCC_CONDUIT_SMC:					\
+			arm_smccc_1_2_smc(__args, __res);		\
+			break;						\
+		default:						\
+			__fail_smccc_1_2(__res);			\
+			method = SMCCC_CONDUIT_NONE;			\
+			break;						\
+		}							\
+		method;							\
+	})
+
 #endif /*__ASSEMBLY__*/
 #endif /*__LINUX_ARM_SMCCC_H*/
-- 
2.46.2

Re: [PATCH v2] firmware: smccc: Support optional Arm SMC SOC_ID name

[kernel test robot]

Hi Paul,

kernel test robot noticed the following build warnings:

[auto build test WARNING on soc/for-next]
[also build test WARNING on linus/master v6.13-rc1 next-20241203]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Paul-Benoit/firmware-smccc-Support-optional-Arm-SMC-SOC_ID-name/20241204-124243
base:   https://git.kernel.org/pub/scm/linux/kernel/git/soc/soc.git for-next
patch link:    https://lore.kernel.org/r/20241203212854.5565-1-paul%40os.amperecomputing.com
patch subject: [PATCH v2] firmware: smccc: Support optional Arm SMC SOC_ID name
config: arm-sp7021_defconfig (https://download.01.org/0day-ci/archive/20241204/202412041926.d2p2hcQe-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241204/202412041926.d2p2hcQe-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202412041926.d2p2hcQe-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/firmware/smccc/soc_id.c: In function 'str_fragment_from_reg':
>> drivers/firmware/smccc/soc_id.c:109:23: warning: right shift count >= width of type [-Wshift-count-overflow]
     109 |         dst[4] = (reg >> 32) & 0xff;
         |                       ^~
   drivers/firmware/smccc/soc_id.c:110:23: warning: right shift count >= width of type [-Wshift-count-overflow]
     110 |         dst[5] = (reg >> 40) & 0xff;
         |                       ^~
   drivers/firmware/smccc/soc_id.c:111:23: warning: right shift count >= width of type [-Wshift-count-overflow]
     111 |         dst[6] = (reg >> 48) & 0xff;
         |                       ^~
   drivers/firmware/smccc/soc_id.c:112:23: warning: right shift count >= width of type [-Wshift-count-overflow]
     112 |         dst[7] = (reg >> 56) & 0xff;
         |                       ^~
   drivers/firmware/smccc/soc_id.c: At top level:
>> drivers/firmware/smccc/soc_id.c:37:29: warning: 'smccc_soc_id_name' defined but not used [-Wunused-variable]
      37 | static char __ro_after_init smccc_soc_id_name[136] = "";
         |                             ^~~~~~~~~~~~~~~~~


vim +109 drivers/firmware/smccc/soc_id.c

    36	
  > 37	static char __ro_after_init smccc_soc_id_name[136] = "";
    38	
    39	static int __init smccc_soc_init(void)
    40	{
    41		int soc_id_rev, soc_id_version;
    42		static char soc_id_str[20], soc_id_rev_str[12];
    43		static char soc_id_jep106_id_str[12];
    44	
    45		if (arm_smccc_get_version() < ARM_SMCCC_VERSION_1_2)
    46			return 0;
    47	
    48		soc_id_version = arm_smccc_get_soc_id_version();
    49		if (soc_id_version == SMCCC_RET_NOT_SUPPORTED) {
    50			pr_info("ARCH_SOC_ID not implemented, skipping ....\n");
    51			return 0;
    52		}
    53	
    54		if (soc_id_version < 0) {
    55			pr_err("Invalid SoC Version: %x\n", soc_id_version);
    56			return -EINVAL;
    57		}
    58	
    59		soc_id_rev = arm_smccc_get_soc_id_revision();
    60		if (soc_id_rev < 0) {
    61			pr_err("Invalid SoC Revision: %x\n", soc_id_rev);
    62			return -EINVAL;
    63		}
    64	
    65		soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
    66		if (!soc_dev_attr)
    67			return -ENOMEM;
    68	
    69		sprintf(soc_id_rev_str, "0x%08x", soc_id_rev);
    70		sprintf(soc_id_jep106_id_str, "jep106:%02x%02x",
    71			JEP106_BANK_CONT_CODE(soc_id_version),
    72			JEP106_ID_CODE(soc_id_version));
    73		sprintf(soc_id_str, "%s:%04x", soc_id_jep106_id_str,
    74			IMP_DEF_SOC_ID(soc_id_version));
    75	
    76		soc_dev_attr->soc_id = soc_id_str;
    77		soc_dev_attr->revision = soc_id_rev_str;
    78		soc_dev_attr->family = soc_id_jep106_id_str;
    79		soc_dev_attr->machine = smccc_soc_name_init();
    80	
    81		soc_dev = soc_device_register(soc_dev_attr);
    82		if (IS_ERR(soc_dev)) {
    83			kfree(soc_dev_attr);
    84			return PTR_ERR(soc_dev);
    85		}
    86	
    87		pr_info("ID = %s Revision = %s\n", soc_dev_attr->soc_id,
    88			soc_dev_attr->revision);
    89	
    90		return 0;
    91	}
    92	module_init(smccc_soc_init);
    93	
    94	static void __exit smccc_soc_exit(void)
    95	{
    96		if (soc_dev)
    97			soc_device_unregister(soc_dev);
    98		kfree(soc_dev_attr);
    99	}
   100	module_exit(smccc_soc_exit);
   101	
   102	
   103	static inline void str_fragment_from_reg(char *dst, unsigned long reg)
   104	{
   105		dst[0] = (reg >> 0)  & 0xff;
   106		dst[1] = (reg >> 8)  & 0xff;
   107		dst[2] = (reg >> 16) & 0xff;
   108		dst[3] = (reg >> 24) & 0xff;
 > 109		dst[4] = (reg >> 32) & 0xff;
   110		dst[5] = (reg >> 40) & 0xff;
   111		dst[6] = (reg >> 48) & 0xff;
   112		dst[7] = (reg >> 56) & 0xff;
   113	}
   114	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2] firmware: smccc: Support optional Arm SMC SOC_ID name

[kernel test robot]

Hi Paul,

kernel test robot noticed the following build warnings:

[auto build test WARNING on soc/for-next]
[also build test WARNING on linus/master v6.13-rc1 next-20241203]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Paul-Benoit/firmware-smccc-Support-optional-Arm-SMC-SOC_ID-name/20241204-124243
base:   https://git.kernel.org/pub/scm/linux/kernel/git/soc/soc.git for-next
patch link:    https://lore.kernel.org/r/20241203212854.5565-1-paul%40os.amperecomputing.com
patch subject: [PATCH v2] firmware: smccc: Support optional Arm SMC SOC_ID name
config: arm-qcom_defconfig (https://download.01.org/0day-ci/archive/20241204/202412042020.U9EEV8mE-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 592c0fe55f6d9a811028b5f3507be91458ab2713)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241204/202412042020.U9EEV8mE-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202412042020.U9EEV8mE-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/firmware/smccc/soc_id.c:109:16: warning: shift count >= width of type [-Wshift-count-overflow]
     109 |         dst[4] = (reg >> 32) & 0xff;
         |                       ^  ~~
   drivers/firmware/smccc/soc_id.c:110:16: warning: shift count >= width of type [-Wshift-count-overflow]
     110 |         dst[5] = (reg >> 40) & 0xff;
         |                       ^  ~~
   drivers/firmware/smccc/soc_id.c:111:16: warning: shift count >= width of type [-Wshift-count-overflow]
     111 |         dst[6] = (reg >> 48) & 0xff;
         |                       ^  ~~
   drivers/firmware/smccc/soc_id.c:112:16: warning: shift count >= width of type [-Wshift-count-overflow]
     112 |         dst[7] = (reg >> 56) & 0xff;
         |                       ^  ~~
>> drivers/firmware/smccc/soc_id.c:37:29: warning: unused variable 'smccc_soc_id_name' [-Wunused-variable]
      37 | static char __ro_after_init smccc_soc_id_name[136] = "";
         |                             ^~~~~~~~~~~~~~~~~
   drivers/firmware/smccc/soc_id.c:103:20: warning: unused function 'str_fragment_from_reg' [-Wunused-function]
     103 | static inline void str_fragment_from_reg(char *dst, unsigned long reg)
         |                    ^~~~~~~~~~~~~~~~~~~~~
   6 warnings generated.


vim +109 drivers/firmware/smccc/soc_id.c

    36	
  > 37	static char __ro_after_init smccc_soc_id_name[136] = "";
    38	
    39	static int __init smccc_soc_init(void)
    40	{
    41		int soc_id_rev, soc_id_version;
    42		static char soc_id_str[20], soc_id_rev_str[12];
    43		static char soc_id_jep106_id_str[12];
    44	
    45		if (arm_smccc_get_version() < ARM_SMCCC_VERSION_1_2)
    46			return 0;
    47	
    48		soc_id_version = arm_smccc_get_soc_id_version();
    49		if (soc_id_version == SMCCC_RET_NOT_SUPPORTED) {
    50			pr_info("ARCH_SOC_ID not implemented, skipping ....\n");
    51			return 0;
    52		}
    53	
    54		if (soc_id_version < 0) {
    55			pr_err("Invalid SoC Version: %x\n", soc_id_version);
    56			return -EINVAL;
    57		}
    58	
    59		soc_id_rev = arm_smccc_get_soc_id_revision();
    60		if (soc_id_rev < 0) {
    61			pr_err("Invalid SoC Revision: %x\n", soc_id_rev);
    62			return -EINVAL;
    63		}
    64	
    65		soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
    66		if (!soc_dev_attr)
    67			return -ENOMEM;
    68	
    69		sprintf(soc_id_rev_str, "0x%08x", soc_id_rev);
    70		sprintf(soc_id_jep106_id_str, "jep106:%02x%02x",
    71			JEP106_BANK_CONT_CODE(soc_id_version),
    72			JEP106_ID_CODE(soc_id_version));
    73		sprintf(soc_id_str, "%s:%04x", soc_id_jep106_id_str,
    74			IMP_DEF_SOC_ID(soc_id_version));
    75	
    76		soc_dev_attr->soc_id = soc_id_str;
    77		soc_dev_attr->revision = soc_id_rev_str;
    78		soc_dev_attr->family = soc_id_jep106_id_str;
    79		soc_dev_attr->machine = smccc_soc_name_init();
    80	
    81		soc_dev = soc_device_register(soc_dev_attr);
    82		if (IS_ERR(soc_dev)) {
    83			kfree(soc_dev_attr);
    84			return PTR_ERR(soc_dev);
    85		}
    86	
    87		pr_info("ID = %s Revision = %s\n", soc_dev_attr->soc_id,
    88			soc_dev_attr->revision);
    89	
    90		return 0;
    91	}
    92	module_init(smccc_soc_init);
    93	
    94	static void __exit smccc_soc_exit(void)
    95	{
    96		if (soc_dev)
    97			soc_device_unregister(soc_dev);
    98		kfree(soc_dev_attr);
    99	}
   100	module_exit(smccc_soc_exit);
   101	
   102	
   103	static inline void str_fragment_from_reg(char *dst, unsigned long reg)
   104	{
   105		dst[0] = (reg >> 0)  & 0xff;
   106		dst[1] = (reg >> 8)  & 0xff;
   107		dst[2] = (reg >> 16) & 0xff;
   108		dst[3] = (reg >> 24) & 0xff;
 > 109		dst[4] = (reg >> 32) & 0xff;
 > 110		dst[5] = (reg >> 40) & 0xff;
   111		dst[6] = (reg >> 48) & 0xff;
   112		dst[7] = (reg >> 56) & 0xff;
   113	}
   114	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH v3] firmware: smccc: Support optional Arm SMC SOC_ID name

[Paul Benoit]

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@os.amperecomputing.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Lorenzo Pieralisi <lpieralisi@kernel.org>
Cc: Sudeep Holla <sudeep.holla@arm.com>
Cc: linux-arm-kernel@lists.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);
+
+#ifdef CONFIG_ARM64
+static char __ro_after_init smccc_soc_id_name[136] = "";
+#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);
+
+
+#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;
+}
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;		\
+		struct arm_smccc_1_2_regs *__res = res;			\
+		int method = arm_smccc_1_1_get_conduit();		\
+		switch (method) {					\
+		case SMCCC_CONDUIT_HVC:					\
+			arm_smccc_1_2_hvc(__args, __res);		\
+			break;						\
+		case SMCCC_CONDUIT_SMC:					\
+			arm_smccc_1_2_smc(__args, __res);		\
+			break;						\
+		default:						\
+			__fail_smccc_1_2(__res);			\
+			method = SMCCC_CONDUIT_NONE;			\
+			break;						\
+		}							\
+		method;							\
+	})
+
 #endif /*__ASSEMBLY__*/
 #endif /*__LINUX_ARM_SMCCC_H*/
-- 
2.47.0

Re: [PATCH v3] firmware: smccc: Support optional Arm SMC SOC_ID name

[Sudeep Holla]

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.

> the SoC Device Attributes at this string so that it will appear under
> /sys/bus/soc/devices/soc0/machine.

Break into new paragraph here.

> 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@os.amperecomputing.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Lorenzo Pieralisi <lpieralisi@kernel.org>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Cc: linux-arm-kernel@lists.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()

> +#ifdef CONFIG_ARM64
> +static char __ro_after_init smccc_soc_id_name[136] = "";

Move all in one block under #ifdef, details below.

> +#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.

> +
> +
> +#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

> 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.

-- 
Regards,
Sudeep

Re: [PATCH v3] firmware: smccc: Support optional Arm SMC SOC_ID name

[Paul Benoit]

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@os.amperecomputing.com>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: Lorenzo Pieralisi <lpieralisi@kernel.org>
>> Cc: Sudeep Holla <sudeep.holla@arm.com>
>> Cc: linux-arm-kernel@lists.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.

[PATCH v4] firmware: smccc: Support optional Arm SMC SOC_ID name

[Paul Benoit]

Issue Number 1.6 of the Arm SMC Calling Convention introduces an optional
SOC_ID name string.  If implemented, 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@os.amperecomputing.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Lorenzo Pieralisi <lpieralisi@kernel.org>
Cc: Sudeep Holla <sudeep.holla@arm.com>
Cc: linux-arm-kernel@lists.infradead.org
---

v3->v4: Address code review identified issues of v3.
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 | 81 +++++++++++++++++++++++++++++++++
 include/linux/arm-smccc.h       | 40 ++++++++++++++++
 2 files changed, 121 insertions(+)

diff --git a/drivers/firmware/smccc/soc_id.c b/drivers/firmware/smccc/soc_id.c
index 1990263fbba0..7b0688d78dab 100644
--- a/drivers/firmware/smccc/soc_id.c
+++ b/drivers/firmware/smccc/soc_id.c
@@ -32,6 +32,86 @@
 static struct soc_device *soc_dev;
 static struct soc_device_attribute *soc_dev_attr;
 
+#ifdef CONFIG_ARM64
+
+static char __ro_after_init smccc_soc_id_name[136] = "";
+
+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;
+}
+
+static char __init *smccc_soc_name_init(void)
+{
+	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;
+		}
+	}
+
+	return NULL;
+}
+
+#else
+
+static char __init *smccc_soc_name_init(void)
+{
+	return NULL;
+}
+
+#endif
+
 static int __init smccc_soc_init(void)
 {
 	int soc_id_rev, soc_id_version;
@@ -72,6 +152,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)) {
diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
index 67f6fdf2e7cd..eb7eab04755a 100644
--- a/include/linux/arm-smccc.h
+++ b/include/linux/arm-smccc.h
@@ -639,5 +639,45 @@ asmlinkage void __arm_smccc_hvc(unsigned long a0, unsigned long a1,
 		method;							\
 	})
 
+#ifdef CONFIG_ARM64
+
+#define __fail_smccc_1_2(___res)					\
+	do {								\
+		if (___res)						\
+			___res->a0 = SMCCC_RET_NOT_SUPPORTED;		\
+	} while (0)
+
+/*
+ * 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;		\
+		struct arm_smccc_1_2_regs *__res = res;			\
+		int method = arm_smccc_1_1_get_conduit();		\
+		switch (method) {					\
+		case SMCCC_CONDUIT_HVC:					\
+			arm_smccc_1_2_hvc(__args, __res);		\
+			break;						\
+		case SMCCC_CONDUIT_SMC:					\
+			arm_smccc_1_2_smc(__args, __res);		\
+			break;						\
+		default:						\
+			__fail_smccc_1_2(__res);			\
+			method = SMCCC_CONDUIT_NONE;			\
+			break;						\
+		}							\
+		method;							\
+	})
+#endif /*CONFIG_ARM64*/
+
 #endif /*__ASSEMBLY__*/
 #endif /*__LINUX_ARM_SMCCC_H*/
-- 
2.48.1

Re: [PATCH v4] firmware: smccc: Support optional Arm SMC SOC_ID name

[Mark Rutland]

Hi Paul,

This looks generally good with a couple of minor nits.

On Tue, Feb 18, 2025 at 04:59:32PM -0800, Paul Benoit wrote:
> Issue Number 1.6 of the Arm SMC Calling Convention introduces an optional
> SOC_ID name string.  If implemented, 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@os.amperecomputing.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Lorenzo Pieralisi <lpieralisi@kernel.org>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Cc: linux-arm-kernel@lists.infradead.org
> ---
> 
> v3->v4: Address code review identified issues of v3.
> 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 | 81 +++++++++++++++++++++++++++++++++
>  include/linux/arm-smccc.h       | 40 ++++++++++++++++
>  2 files changed, 121 insertions(+)
> 
> diff --git a/drivers/firmware/smccc/soc_id.c b/drivers/firmware/smccc/soc_id.c
> index 1990263fbba0..7b0688d78dab 100644
> --- a/drivers/firmware/smccc/soc_id.c
> +++ b/drivers/firmware/smccc/soc_id.c
> @@ -32,6 +32,86 @@
>  static struct soc_device *soc_dev;
>  static struct soc_device_attribute *soc_dev_attr;
>  
> +#ifdef CONFIG_ARM64
> +
> +static char __ro_after_init smccc_soc_id_name[136] = "";
> +
> +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;
> +}
> +
> +static char __init *smccc_soc_name_init(void)
> +{
> +	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);

Please get rid of 'regsize' and use '8' directly. This only exists for
arm64, where the registeres are 8 bytes, and the comment immediately
above refers to "8 bytes" specifically anyway, so 'regsize' only serves
to make this harder to read.

It'd be a bit clearer as:

	str_fragment_from_reg(smccc_soc_id_name + 8 * 0,  res.a1);
	str_fragment_from_reg(smccc_soc_id_name + 8 * 1,  res.a2);
	...
	str_fragment_from_reg(smccc_soc_id_name + 8 * 15, res.a16);
	str_fragment_from_reg(smccc_soc_id_name + 8 * 16, res.a17);

Sudeep, are you happy to fix that up when applying?

> +
> +		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");

It's odd that 'Name' is capitalized here. Not a big deal, but it doesn't
look quite right.

> +			else
> +				return smccc_soc_id_name;
> +		}
> +	}
> +
> +	return NULL;
> +}
> +
> +#else
> +
> +static char __init *smccc_soc_name_init(void)
> +{
> +	return NULL;
> +}
> +
> +#endif
> +
>  static int __init smccc_soc_init(void)
>  {
>  	int soc_id_rev, soc_id_version;
> @@ -72,6 +152,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)) {
> diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
> index 67f6fdf2e7cd..eb7eab04755a 100644
> --- a/include/linux/arm-smccc.h
> +++ b/include/linux/arm-smccc.h
> @@ -639,5 +639,45 @@ asmlinkage void __arm_smccc_hvc(unsigned long a0, unsigned long a1,
>  		method;							\
>  	})
>  
> +#ifdef CONFIG_ARM64
> +
> +#define __fail_smccc_1_2(___res)					\
> +	do {								\
> +		if (___res)						\
> +			___res->a0 = SMCCC_RET_NOT_SUPPORTED;		\
> +	} while (0)
> +
> +/*
> + * 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;		\
> +		struct arm_smccc_1_2_regs *__res = res;			\
> +		int method = arm_smccc_1_1_get_conduit();		\
> +		switch (method) {					\
> +		case SMCCC_CONDUIT_HVC:					\
> +			arm_smccc_1_2_hvc(__args, __res);		\
> +			break;						\
> +		case SMCCC_CONDUIT_SMC:					\
> +			arm_smccc_1_2_smc(__args, __res);		\
> +			break;						\
> +		default:						\
> +			__fail_smccc_1_2(__res);			\
> +			method = SMCCC_CONDUIT_NONE;			\
> +			break;						\
> +		}							\
> +		method;							\
> +	})
> +#endif /*CONFIG_ARM64*/

At some point I intend to rework the existing helpers to not return the
conduit, but this is fine for now.

With the fixups above:

Acked-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> +
>  #endif /*__ASSEMBLY__*/
>  #endif /*__LINUX_ARM_SMCCC_H*/
> -- 
> 2.48.1
> 

Re: [PATCH v4] firmware: smccc: Support optional Arm SMC SOC_ID name

[Sudeep Holla]

On Mon, Mar 03, 2025 at 02:18:05PM +0000, Mark Rutland wrote:
> Hi Paul,
>
> This looks generally good with a couple of minor nits.
>
> On Tue, Feb 18, 2025 at 04:59:32PM -0800, Paul Benoit wrote:
> > Issue Number 1.6 of the Arm SMC Calling Convention introduces an optional
> > SOC_ID name string.  If implemented, 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@os.amperecomputing.com>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: Lorenzo Pieralisi <lpieralisi@kernel.org>
> > Cc: Sudeep Holla <sudeep.holla@arm.com>
> > Cc: linux-arm-kernel@lists.infradead.org
> > ---

[...]

> > +static char __init *smccc_soc_name_init(void)
> > +{
> > +	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);
> 
> Please get rid of 'regsize' and use '8' directly. This only exists for
> arm64, where the registeres are 8 bytes, and the comment immediately
> above refers to "8 bytes" specifically anyway, so 'regsize' only serves
> to make this harder to read.
> 
> It'd be a bit clearer as:
> 
> 	str_fragment_from_reg(smccc_soc_id_name + 8 * 0,  res.a1);
> 	str_fragment_from_reg(smccc_soc_id_name + 8 * 1,  res.a2);
> 	...
> 	str_fragment_from_reg(smccc_soc_id_name + 8 * 15, res.a16);
> 	str_fragment_from_reg(smccc_soc_id_name + 8 * 16, res.a17);
> 
> Sudeep, are you happy to fix that up when applying?
> 
> > +
> > +		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");
> 
> It's odd that 'Name' is capitalized here. Not a big deal, but it doesn't
> look quite right.
>

I can fix both of these and apply. No need to repost.

-- 
Regards,
Sudeep

Re: [PATCH v4] firmware: smccc: Support optional Arm SMC SOC_ID name

[Paul Benoit]

Thank you Sudeep and Mark.

On 3/3/2025 9:45 AM, Sudeep Holla wrote:
> On Mon, Mar 03, 2025 at 02:18:05PM +0000, Mark Rutland wrote:
>> Hi Paul,
>>
>> This looks generally good with a couple of minor nits.
>>
>> On Tue, Feb 18, 2025 at 04:59:32PM -0800, Paul Benoit wrote:
>>> Issue Number 1.6 of the Arm SMC Calling Convention introduces an optional
>>> SOC_ID name string.  If implemented, 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@os.amperecomputing.com>
>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>> Cc: Lorenzo Pieralisi <lpieralisi@kernel.org>
>>> Cc: Sudeep Holla <sudeep.holla@arm.com>
>>> Cc: linux-arm-kernel@lists.infradead.org
>>> ---
> 
> [...]
> 
>>> +static char __init *smccc_soc_name_init(void)
>>> +{
>>> +	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);
>>
>> Please get rid of 'regsize' and use '8' directly. This only exists for
>> arm64, where the registeres are 8 bytes, and the comment immediately
>> above refers to "8 bytes" specifically anyway, so 'regsize' only serves
>> to make this harder to read.
>>
>> It'd be a bit clearer as:
>>
>> 	str_fragment_from_reg(smccc_soc_id_name + 8 * 0,  res.a1);
>> 	str_fragment_from_reg(smccc_soc_id_name + 8 * 1,  res.a2);
>> 	...
>> 	str_fragment_from_reg(smccc_soc_id_name + 8 * 15, res.a16);
>> 	str_fragment_from_reg(smccc_soc_id_name + 8 * 16, res.a17);
>>
>> Sudeep, are you happy to fix that up when applying?
>>
>>> +
>>> +		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");
>>
>> It's odd that 'Name' is capitalized here. Not a big deal, but it doesn't
>> look quite right.
>>
> 
> I can fix both of these and apply. No need to repost.
> 

Re: [PATCH v4] firmware: smccc: Support optional Arm SMC SOC_ID name

[Sudeep Holla]

On Tue, 18 Feb 2025 16:59:32 -0800, Paul Benoit wrote:
> Issue Number 1.6 of the Arm SMC Calling Convention introduces an optional
> SOC_ID name string.  If implemented, 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.
>
> [...]

Applied to sudeep.holla/linux (for-linux-next), thanks!

[1/1] firmware: smccc: Support optional Arm SMC SOC_ID name
      https://git.kernel.org/sudeep.holla/c/5f9c23abc477
--
Regards,
Sudeep