[PATCH 0/2] Allow parameter in smc/hvc calls

[PATCH 0/2] Allow parameter in smc/hvc calls

[Nikunj Kela]

Currently, smc/hvc calls are made with parameters set
to zeros. We are using multiple scmi instances within
a VM and hypervisor associates a tag with each instance
and expects the tag in smc calls from that instance, while
sharing the same smc-id(func_id) among the instances.

This patch series introduces new optional dtb bindings which
can be used to pass parameters to smc/hvc calls.

Nikunj Kela (2):
  dt-bindings: firmware: arm,scmi: support parameter passing in smc/hvc
  firmware: arm_scmi: Augment SMC/HVC to allow optional parameters

 .../bindings/firmware/arm,scmi.yaml           | 16 +++++
 drivers/firmware/arm_scmi/smc.c               | 66 ++++++++++++++++++-
 2 files changed, 81 insertions(+), 1 deletion(-)

-- 
2.17.1

[PATCH 1/2] dt-bindings: firmware: arm,scmi: support parameter passing in smc/hvc

[Nikunj Kela]

Currently, smc/hvc calls are made with smc-id only. The parameters are
all set to zeros. This patch defines two optional device tree bindings,
that can be used to pass parameters in smc/hvc calls.

This is useful when multiple scmi instances are used with common smc-id.

Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
---
 .../devicetree/bindings/firmware/arm,scmi.yaml   | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
index 5824c43e9893..08c331a79b80 100644
--- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
+++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
@@ -115,6 +115,22 @@ properties:
     description:
       SMC id required when using smc or hvc transports
 
+  arm,smc32-params:
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    description:
+      An optional parameter list passed in smc32 or hvc32 calls
+    default: 0
+    minItems: 1
+    maxItems: 6
+
+  arm,smc64-params:
+    $ref: /schemas/types.yaml#/definitions/uint64-array
+    description:
+      An optional parameter list passed in smc64 or hvc64 calls
+    default: 0
+    minItems: 1
+    maxItems: 6
+
   linaro,optee-channel-id:
     $ref: /schemas/types.yaml#/definitions/uint32
     description:
-- 
2.17.1

Re: [PATCH 1/2] dt-bindings: firmware: arm,scmi: support parameter passing in smc/hvc

[Krzysztof Kozlowski]

On 09/04/2023 20:19, Nikunj Kela wrote:
> Currently, smc/hvc calls are made with smc-id only. The parameters are
> all set to zeros. This patch defines two optional device tree bindings,
> that can be used to pass parameters in smc/hvc calls.
> 
> This is useful when multiple scmi instances are used with common smc-id.
> 
> Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
> ---
>  .../devicetree/bindings/firmware/arm,scmi.yaml   | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> index 5824c43e9893..08c331a79b80 100644
> --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> @@ -115,6 +115,22 @@ properties:
>      description:
>        SMC id required when using smc or hvc transports
>  
> +  arm,smc32-params:
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    description:
> +      An optional parameter list passed in smc32 or hvc32 calls
> +    default: 0
> +    minItems: 1
> +    maxItems: 6
> +
> +  arm,smc64-params:
> +    $ref: /schemas/types.yaml#/definitions/uint64-array
> +    description:
> +      An optional parameter list passed in smc64 or hvc64 calls
> +    default: 0
> +    minItems: 1
> +    maxItems: 6

These do not look like hardware properties and the fact that you need
two properties for the same also points that you tied it to specific SW
interface.

Why this should be board-specific? Actually better question - why this
should be fixed per board? Doesn't my software want to have different
parameters, depending on some other condition?

You also did not provide any DTS user for this, so difficult to judge
usefulness.

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: firmware: arm,scmi: support parameter passing in smc/hvc

[Nikunj Kela]

On 4/10/2023 10:20 AM, Krzysztof Kozlowski wrote:
> On 09/04/2023 20:19, Nikunj Kela wrote:
>> Currently, smc/hvc calls are made with smc-id only. The parameters are
>> all set to zeros. This patch defines two optional device tree bindings,
>> that can be used to pass parameters in smc/hvc calls.
>>
>> This is useful when multiple scmi instances are used with common smc-id.
>>
>> Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
>> ---
>>   .../devicetree/bindings/firmware/arm,scmi.yaml   | 16 ++++++++++++++++
>>   1 file changed, 16 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
>> index 5824c43e9893..08c331a79b80 100644
>> --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
>> +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
>> @@ -115,6 +115,22 @@ properties:
>>       description:
>>         SMC id required when using smc or hvc transports
>>   
>> +  arm,smc32-params:
>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
>> +    description:
>> +      An optional parameter list passed in smc32 or hvc32 calls
>> +    default: 0
>> +    minItems: 1
>> +    maxItems: 6
>> +
>> +  arm,smc64-params:
>> +    $ref: /schemas/types.yaml#/definitions/uint64-array
>> +    description:
>> +      An optional parameter list passed in smc64 or hvc64 calls
>> +    default: 0
>> +    minItems: 1
>> +    maxItems: 6
> These do not look like hardware properties and the fact that you need
> two properties for the same also points that you tied it to specific SW
> interface.

This is certainly not the H/W property but then smc-id is also not H/W 
property either

but that gets passed via DTB. I could use the same property for both 
however I wasn't sure

which datatype should be used, uint32-array/uint64-array. Moreover, I 
thought if users are

passing parameters, they better know which SMC convention they are using 
hence used two

explicit properties.

> Why this should be board-specific? Actually better question - why this
> should be fixed per board? Doesn't my software want to have different
> parameters, depending on some other condition?

Not sure I follow, I didn't say this is board specific. People can use 
the parameters to pass

whatever their S/W demands. SMC/HVC calls are made by passing parameters 
which is what this patch is enabling.

>
> You also did not provide any DTS user for this, so difficult to judge
> usefulness.

The work is still on going and we will push the dts in few months, 
however that shouldn't stop

making changes in advance.

> Best regards,
> Krzysztof
>

Re: [PATCH 1/2] dt-bindings: firmware: arm,scmi: support parameter passing in smc/hvc

[Krzysztof Kozlowski]

On 10/04/2023 19:33, Nikunj Kela wrote:
> 
> On 4/10/2023 10:20 AM, Krzysztof Kozlowski wrote:
>> On 09/04/2023 20:19, Nikunj Kela wrote:
>>> Currently, smc/hvc calls are made with smc-id only. The parameters are
>>> all set to zeros. This patch defines two optional device tree bindings,
>>> that can be used to pass parameters in smc/hvc calls.
>>>
>>> This is useful when multiple scmi instances are used with common smc-id.
>>>
>>> Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
>>> ---
>>>   .../devicetree/bindings/firmware/arm,scmi.yaml   | 16 ++++++++++++++++
>>>   1 file changed, 16 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
>>> index 5824c43e9893..08c331a79b80 100644
>>> --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
>>> +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
>>> @@ -115,6 +115,22 @@ properties:
>>>       description:
>>>         SMC id required when using smc or hvc transports
>>>   
>>> +  arm,smc32-params:
>>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
>>> +    description:
>>> +      An optional parameter list passed in smc32 or hvc32 calls
>>> +    default: 0
>>> +    minItems: 1
>>> +    maxItems: 6
>>> +
>>> +  arm,smc64-params:
>>> +    $ref: /schemas/types.yaml#/definitions/uint64-array
>>> +    description:
>>> +      An optional parameter list passed in smc64 or hvc64 calls
>>> +    default: 0
>>> +    minItems: 1
>>> +    maxItems: 6
>> These do not look like hardware properties and the fact that you need
>> two properties for the same also points that you tied it to specific SW
>> interface.
> 
> This is certainly not the H/W property but then smc-id is also not H/W 
> property either

Yep, maybe it should be also removed?

> 
> but that gets passed via DTB. I could use the same property for both 
> however I wasn't sure
> 
> which datatype should be used, uint32-array/uint64-array. Moreover, I 
> thought if users are
> 
> passing parameters, they better know which SMC convention they are using 
> hence used two
> 
> explicit properties.

Does not solve my concern.

> 
>> Why this should be board-specific? Actually better question - why this
>> should be fixed per board? Doesn't my software want to have different
>> parameters, depending on some other condition?
> 
> Not sure I follow, I didn't say this is board specific.

But putting it in DTS which is customized per board is then board specific.

>  People can use 
> the parameters to pass
> 
> whatever their S/W demands. SMC/HVC calls are made by passing parameters 
> which is what this patch is enabling.

I cannot follow your paragraphs. You cut middle of sentence.

Anyway, if this is to match whatever their SW demands, it is not
Devicetree property.

> 
>>
>> You also did not provide any DTS user for this, so difficult to judge
>> usefulness.
> 
> The work is still on going and we will push the dts in few months, 
> however that shouldn't stop
> 
> making changes in advance.

With a proper DTS maybe it would be easier to convince us why SW
interface should be put to Devicetree... but sure, we can skip DTS and
answer is - this looks like SW property and not a DT.

Best regards,
Krzysztof

[PATCH 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional parameters

[Nikunj Kela]

This patch add support for passing parameters to smc/hvc calls.
This patch is useful when multiple scmi instances are using same
smc-id and firmware needs to distiguish among the instances.

Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
---
 drivers/firmware/arm_scmi/smc.c | 66 ++++++++++++++++++++++++++++++++-
 1 file changed, 65 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
index 93272e4bbd12..39582d1e2c9f 100644
--- a/drivers/firmware/arm_scmi/smc.c
+++ b/drivers/firmware/arm_scmi/smc.c
@@ -20,6 +20,16 @@
 
 #include "common.h"
 
+#define MAX_PARAM_COUNT 6
+
+/**
+ * scmi_smc_param_t - parameter type for SCMI smc/hvc call
+ */
+typedef union {
+	u64 x;
+	u32 w;
+} scmi_smc_param_t;
+
 /**
  * struct scmi_smc - Structure representing a SCMI smc transport
  *
@@ -30,6 +40,8 @@
  * @inflight: Atomic flag to protect access to Tx/Rx shared memory area.
  *	      Used when operating in atomic mode.
  * @func_id: smc/hvc call function id
+ * @is_smc64: A flag, indicating smc64 calling convention.
+ * @params: Optional, smc/hvc call parameters.
  */
 
 struct scmi_smc {
@@ -40,8 +52,51 @@ struct scmi_smc {
 #define INFLIGHT_NONE	MSG_TOKEN_MAX
 	atomic_t inflight;
 	u32 func_id;
+	bool is_smc64;
+	scmi_smc_param_t params[MAX_PARAM_COUNT];
 };
 
+static void populate_smc_params(struct device *dev, struct scmi_smc *scmi_info)
+{
+	struct device_node *np = dev->of_node;
+	u64 params64[MAX_PARAM_COUNT] = { 0 };
+	u32 params32[MAX_PARAM_COUNT] = { 0 };
+	int i, count;
+
+	if (scmi_info->is_smc64) {
+		count = of_property_read_variable_u64_array(np,
+							    "arm,smc64-params",
+							    &params64[0], 1,
+							    MAX_PARAM_COUNT);
+		if (count == -EINVAL)  /* if property is not defined */
+			return;
+
+		if (count > 0)	/* populate the parameters */
+			for (i = 0; i < count; i++)
+				scmi_info->params[i].x = params64[i];
+		else
+			goto param_err;
+	} else {
+		count = of_property_read_variable_u32_array(np,
+							    "arm,smc32-params",
+							    &params32[0], 1,
+							    MAX_PARAM_COUNT);
+		if (count == -EINVAL)  /* if property is not defined */
+			return;
+
+		if (count > 0)	/* populate the parameters */
+			for (i = 0; i < count; i++)
+				scmi_info->params[i].w = params32[i];
+		else
+			goto param_err;
+	}
+
+	return;
+
+param_err:
+	dev_warn(dev, "failed to read smc/hvc call parameters\n");
+}
+
 static irqreturn_t smc_msg_done_isr(int irq, void *data)
 {
 	struct scmi_smc *scmi_info = data;
@@ -156,6 +211,8 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
 	}
 
 	scmi_info->func_id = func_id;
+	scmi_info->is_smc64 = ARM_SMCCC_IS_64(func_id);
+	populate_smc_params(dev, scmi_info);
 	scmi_info->cinfo = cinfo;
 	smc_channel_lock_init(scmi_info);
 	cinfo->transport_info = scmi_info;
@@ -179,6 +236,7 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
 {
 	struct scmi_smc *scmi_info = cinfo->transport_info;
 	struct arm_smccc_res res;
+	scmi_smc_param_t *p = scmi_info->params;
 
 	/*
 	 * Channel will be released only once response has been
@@ -188,7 +246,13 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
 
 	shmem_tx_prepare(scmi_info->shmem, xfer, cinfo);
 
-	arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
+	if (scmi_info->is_smc64)
+		arm_smccc_1_1_invoke(scmi_info->func_id, p[0].x, p[1].x, p[2].x,
+				     p[3].x, p[4].x, p[5].x, 0, &res);
+	else
+		arm_smccc_1_1_invoke(scmi_info->func_id, p[0].w, p[1].w, p[2].w,
+				     p[3].w, p[4].w, p[5].w, 0, &res);
+
 
 	/* Only SMCCC_RET_NOT_SUPPORTED is valid error code */
 	if (res.a0) {
-- 
2.17.1

Re: [PATCH 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional parameters

[kernel test robot]

Hi Nikunj,

kernel test robot noticed the following build errors:

[auto build test ERROR on robh/for-next]
[also build test ERROR on linus/master v6.3-rc6 next-20230406]
[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/Nikunj-Kela/dt-bindings-firmware-arm-scmi-support-parameter-passing-in-smc-hvc/20230410-022129
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20230409181918.29270-3-quic_nkela%40quicinc.com
patch subject: [PATCH 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional parameters
config: arm-allyesconfig (https://download.01.org/0day-ci/archive/20230410/202304100606.kUjhsRYf-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/9c34de66aa243da9824e8bbe749b24e36b0db483
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Nikunj-Kela/dt-bindings-firmware-arm-scmi-support-parameter-passing-in-smc-hvc/20230410-022129
        git checkout 9c34de66aa243da9824e8bbe749b24e36b0db483
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arm olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304100606.kUjhsRYf-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from drivers/firmware/arm_scmi/smc.c:9:
   drivers/firmware/arm_scmi/smc.c: In function 'smc_send_message':
>> include/linux/arm-smccc.h:436:29: error: register specified for 'arg1' isn't suitable for data type
     436 |         register typeof(a1) arg1 asm("r1") = __a1;                      \
         |                             ^~~~
   include/linux/arm-smccc.h:442:9: note: in expansion of macro '__declare_arg_3'
     442 |         __declare_arg_3(a0, a1, a2, a3, res);                           \
         |         ^~~~~~~~~~~~~~~
   include/linux/arm-smccc.h:447:9: note: in expansion of macro '__declare_arg_4'
     447 |         __declare_arg_4(a0, a1, a2, a3, a4, res);                       \
         |         ^~~~~~~~~~~~~~~
   include/linux/arm-smccc.h:452:9: note: in expansion of macro '__declare_arg_5'
     452 |         __declare_arg_5(a0, a1, a2, a3, a4, a5, res);                   \
         |         ^~~~~~~~~~~~~~~
   include/linux/arm-smccc.h:457:9: note: in expansion of macro '__declare_arg_6'
     457 |         __declare_arg_6(a0, a1, a2, a3, a4, a5, a6, res);               \
         |         ^~~~~~~~~~~~~~~
   include/linux/arm-smccc.h:460:37: note: in expansion of macro '__declare_arg_7'
     460 | #define ___declare_args(count, ...) __declare_arg_ ## count(__VA_ARGS__)
         |                                     ^~~~~~~~~~~~~~
   include/linux/arm-smccc.h:461:37: note: in expansion of macro '___declare_args'
     461 | #define __declare_args(count, ...)  ___declare_args(count, __VA_ARGS__)
         |                                     ^~~~~~~~~~~~~~~
   include/linux/arm-smccc.h:479:17: note: in expansion of macro '__declare_args'
     479 |                 __declare_args(__count_args(__VA_ARGS__), __VA_ARGS__); \
         |                 ^~~~~~~~~~~~~~
   include/linux/arm-smccc.h:518:33: note: in expansion of macro '__arm_smccc_1_1'
     518 | #define arm_smccc_1_1_hvc(...)  __arm_smccc_1_1(SMCCC_HVC_INST, __VA_ARGS__)
         |                                 ^~~~~~~~~~~~~~~
   include/linux/arm-smccc.h:552:25: note: in expansion of macro 'arm_smccc_1_1_hvc'
     552 |                         arm_smccc_1_1_hvc(__VA_ARGS__);                 \
         |                         ^~~~~~~~~~~~~~~~~
   drivers/firmware/arm_scmi/smc.c:250:17: note: in expansion of macro 'arm_smccc_1_1_invoke'
     250 |                 arm_smccc_1_1_invoke(scmi_info->func_id, p[0].x, p[1].x, p[2].x,
         |                 ^~~~~~~~~~~~~~~~~~~~
>> include/linux/arm-smccc.h:438:29: error: register specified for 'arg3' isn't suitable for data type
     438 |         register typeof(a3) arg3 asm("r3") = __a3
         |                             ^~~~
   include/linux/arm-smccc.h:442:9: note: in expansion of macro '__declare_arg_3'
     442 |         __declare_arg_3(a0, a1, a2, a3, res);                           \
         |         ^~~~~~~~~~~~~~~
   include/linux/arm-smccc.h:447:9: note: in expansion of macro '__declare_arg_4'
     447 |         __declare_arg_4(a0, a1, a2, a3, a4, res);                       \
         |         ^~~~~~~~~~~~~~~
   include/linux/arm-smccc.h:452:9: note: in expansion of macro '__declare_arg_5'
     452 |         __declare_arg_5(a0, a1, a2, a3, a4, a5, res);                   \
         |         ^~~~~~~~~~~~~~~
   include/linux/arm-smccc.h:457:9: note: in expansion of macro '__declare_arg_6'
     457 |         __declare_arg_6(a0, a1, a2, a3, a4, a5, a6, res);               \
         |         ^~~~~~~~~~~~~~~
   include/linux/arm-smccc.h:460:37: note: in expansion of macro '__declare_arg_7'
     460 | #define ___declare_args(count, ...) __declare_arg_ ## count(__VA_ARGS__)
         |                                     ^~~~~~~~~~~~~~
   include/linux/arm-smccc.h:461:37: note: in expansion of macro '___declare_args'
     461 | #define __declare_args(count, ...)  ___declare_args(count, __VA_ARGS__)
         |                                     ^~~~~~~~~~~~~~~
   include/linux/arm-smccc.h:479:17: note: in expansion of macro '__declare_args'
     479 |                 __declare_args(__count_args(__VA_ARGS__), __VA_ARGS__); \
         |                 ^~~~~~~~~~~~~~
   include/linux/arm-smccc.h:518:33: note: in expansion of macro '__arm_smccc_1_1'
     518 | #define arm_smccc_1_1_hvc(...)  __arm_smccc_1_1(SMCCC_HVC_INST, __VA_ARGS__)
         |                                 ^~~~~~~~~~~~~~~
   include/linux/arm-smccc.h:552:25: note: in expansion of macro 'arm_smccc_1_1_hvc'
     552 |                         arm_smccc_1_1_hvc(__VA_ARGS__);                 \
         |                         ^~~~~~~~~~~~~~~~~
   drivers/firmware/arm_scmi/smc.c:250:17: note: in expansion of macro 'arm_smccc_1_1_invoke'
     250 |                 arm_smccc_1_1_invoke(scmi_info->func_id, p[0].x, p[1].x, p[2].x,
         |                 ^~~~~~~~~~~~~~~~~~~~
>> include/linux/arm-smccc.h:448:29: error: register specified for 'arg5' isn't suitable for data type
     448 |         register typeof(a5) arg5 asm("r5") = __a5
         |                             ^~~~
   include/linux/arm-smccc.h:452:9: note: in expansion of macro '__declare_arg_5'
     452 |         __declare_arg_5(a0, a1, a2, a3, a4, a5, res);                   \
         |         ^~~~~~~~~~~~~~~
   include/linux/arm-smccc.h:457:9: note: in expansion of macro '__declare_arg_6'
     457 |         __declare_arg_6(a0, a1, a2, a3, a4, a5, a6, res);               \
         |         ^~~~~~~~~~~~~~~
   include/linux/arm-smccc.h:460:37: note: in expansion of macro '__declare_arg_7'
     460 | #define ___declare_args(count, ...) __declare_arg_ ## count(__VA_ARGS__)
         |                                     ^~~~~~~~~~~~~~
   include/linux/arm-smccc.h:461:37: note: in expansion of macro '___declare_args'
     461 | #define __declare_args(count, ...)  ___declare_args(count, __VA_ARGS__)
         |                                     ^~~~~~~~~~~~~~~
   include/linux/arm-smccc.h:479:17: note: in expansion of macro '__declare_args'
     479 |                 __declare_args(__count_args(__VA_ARGS__), __VA_ARGS__); \
         |                 ^~~~~~~~~~~~~~
   include/linux/arm-smccc.h:518:33: note: in expansion of macro '__arm_smccc_1_1'
     518 | #define arm_smccc_1_1_hvc(...)  __arm_smccc_1_1(SMCCC_HVC_INST, __VA_ARGS__)
         |                                 ^~~~~~~~~~~~~~~
   include/linux/arm-smccc.h:552:25: note: in expansion of macro 'arm_smccc_1_1_hvc'
     552 |                         arm_smccc_1_1_hvc(__VA_ARGS__);                 \
         |                         ^~~~~~~~~~~~~~~~~
   drivers/firmware/arm_scmi/smc.c:250:17: note: in expansion of macro 'arm_smccc_1_1_invoke'
     250 |                 arm_smccc_1_1_invoke(scmi_info->func_id, p[0].x, p[1].x, p[2].x,
         |                 ^~~~~~~~~~~~~~~~~~~~
>> include/linux/arm-smccc.h:436:29: error: register specified for 'arg1' isn't suitable for data type
     436 |         register typeof(a1) arg1 asm("r1") = __a1;                      \
         |                             ^~~~
   include/linux/arm-smccc.h:442:9: note: in expansion of macro '__declare_arg_3'
     442 |         __declare_arg_3(a0, a1, a2, a3, res);                           \
         |         ^~~~~~~~~~~~~~~
   include/linux/arm-smccc.h:447:9: note: in expansion of macro '__declare_arg_4'
     447 |         __declare_arg_4(a0, a1, a2, a3, a4, res);                       \
         |         ^~~~~~~~~~~~~~~
   include/linux/arm-smccc.h:452:9: note: in expansion of macro '__declare_arg_5'
     452 |         __declare_arg_5(a0, a1, a2, a3, a4, a5, res);                   \
         |         ^~~~~~~~~~~~~~~
   include/linux/arm-smccc.h:457:9: note: in expansion of macro '__declare_arg_6'
     457 |         __declare_arg_6(a0, a1, a2, a3, a4, a5, a6, res);               \
         |         ^~~~~~~~~~~~~~~
   include/linux/arm-smccc.h:460:37: note: in expansion of macro '__declare_arg_7'
     460 | #define ___declare_args(count, ...) __declare_arg_ ## count(__VA_ARGS__)
         |                                     ^~~~~~~~~~~~~~
   include/linux/arm-smccc.h:461:37: note: in expansion of macro '___declare_args'
     461 | #define __declare_args(count, ...)  ___declare_args(count, __VA_ARGS__)
         |                                     ^~~~~~~~~~~~~~~
   include/linux/arm-smccc.h:479:17: note: in expansion of macro '__declare_args'
     479 |                 __declare_args(__count_args(__VA_ARGS__), __VA_ARGS__); \
         |                 ^~~~~~~~~~~~~~
   include/linux/arm-smccc.h:502:33: note: in expansion of macro '__arm_smccc_1_1'
     502 | #define arm_smccc_1_1_smc(...)  __arm_smccc_1_1(SMCCC_SMC_INST, __VA_ARGS__)
         |                                 ^~~~~~~~~~~~~~~
   include/linux/arm-smccc.h:555:25: note: in expansion of macro 'arm_smccc_1_1_smc'
     555 |                         arm_smccc_1_1_smc(__VA_ARGS__);                 \
         |                         ^~~~~~~~~~~~~~~~~
   drivers/firmware/arm_scmi/smc.c:250:17: note: in expansion of macro 'arm_smccc_1_1_invoke'
     250 |                 arm_smccc_1_1_invoke(scmi_info->func_id, p[0].x, p[1].x, p[2].x,
         |                 ^~~~~~~~~~~~~~~~~~~~
>> include/linux/arm-smccc.h:438:29: error: register specified for 'arg3' isn't suitable for data type
     438 |         register typeof(a3) arg3 asm("r3") = __a3
         |                             ^~~~
   include/linux/arm-smccc.h:442:9: note: in expansion of macro '__declare_arg_3'
     442 |         __declare_arg_3(a0, a1, a2, a3, res);                           \
         |         ^~~~~~~~~~~~~~~
   include/linux/arm-smccc.h:447:9: note: in expansion of macro '__declare_arg_4'
     447 |         __declare_arg_4(a0, a1, a2, a3, a4, res);                       \
         |         ^~~~~~~~~~~~~~~
   include/linux/arm-smccc.h:452:9: note: in expansion of macro '__declare_arg_5'
     452 |         __declare_arg_5(a0, a1, a2, a3, a4, a5, res);                   \
         |         ^~~~~~~~~~~~~~~
   include/linux/arm-smccc.h:457:9: note: in expansion of macro '__declare_arg_6'
     457 |         __declare_arg_6(a0, a1, a2, a3, a4, a5, a6, res);               \
         |         ^~~~~~~~~~~~~~~
   include/linux/arm-smccc.h:460:37: note: in expansion of macro '__declare_arg_7'
     460 | #define ___declare_args(count, ...) __declare_arg_ ## count(__VA_ARGS__)
         |                                     ^~~~~~~~~~~~~~
   include/linux/arm-smccc.h:461:37: note: in expansion of macro '___declare_args'
     461 | #define __declare_args(count, ...)  ___declare_args(count, __VA_ARGS__)
         |                                     ^~~~~~~~~~~~~~~
   include/linux/arm-smccc.h:479:17: note: in expansion of macro '__declare_args'
     479 |                 __declare_args(__count_args(__VA_ARGS__), __VA_ARGS__); \
         |                 ^~~~~~~~~~~~~~
   include/linux/arm-smccc.h:502:33: note: in expansion of macro '__arm_smccc_1_1'
     502 | #define arm_smccc_1_1_smc(...)  __arm_smccc_1_1(SMCCC_SMC_INST, __VA_ARGS__)
         |                                 ^~~~~~~~~~~~~~~
   include/linux/arm-smccc.h:555:25: note: in expansion of macro 'arm_smccc_1_1_smc'
     555 |                         arm_smccc_1_1_smc(__VA_ARGS__);                 \
         |                         ^~~~~~~~~~~~~~~~~
   drivers/firmware/arm_scmi/smc.c:250:17: note: in expansion of macro 'arm_smccc_1_1_invoke'
     250 |                 arm_smccc_1_1_invoke(scmi_info->func_id, p[0].x, p[1].x, p[2].x,
         |                 ^~~~~~~~~~~~~~~~~~~~
>> include/linux/arm-smccc.h:448:29: error: register specified for 'arg5' isn't suitable for data type
     448 |         register typeof(a5) arg5 asm("r5") = __a5
         |                             ^~~~
   include/linux/arm-smccc.h:452:9: note: in expansion of macro '__declare_arg_5'
     452 |         __declare_arg_5(a0, a1, a2, a3, a4, a5, res);                   \
         |         ^~~~~~~~~~~~~~~
   include/linux/arm-smccc.h:457:9: note: in expansion of macro '__declare_arg_6'
     457 |         __declare_arg_6(a0, a1, a2, a3, a4, a5, a6, res);               \
         |         ^~~~~~~~~~~~~~~
   include/linux/arm-smccc.h:460:37: note: in expansion of macro '__declare_arg_7'
     460 | #define ___declare_args(count, ...) __declare_arg_ ## count(__VA_ARGS__)
         |                                     ^~~~~~~~~~~~~~
   include/linux/arm-smccc.h:461:37: note: in expansion of macro '___declare_args'
     461 | #define __declare_args(count, ...)  ___declare_args(count, __VA_ARGS__)
         |                                     ^~~~~~~~~~~~~~~
   include/linux/arm-smccc.h:479:17: note: in expansion of macro '__declare_args'
     479 |                 __declare_args(__count_args(__VA_ARGS__), __VA_ARGS__); \
         |                 ^~~~~~~~~~~~~~
   include/linux/arm-smccc.h:502:33: note: in expansion of macro '__arm_smccc_1_1'
     502 | #define arm_smccc_1_1_smc(...)  __arm_smccc_1_1(SMCCC_SMC_INST, __VA_ARGS__)
         |                                 ^~~~~~~~~~~~~~~
   include/linux/arm-smccc.h:555:25: note: in expansion of macro 'arm_smccc_1_1_smc'
     555 |                         arm_smccc_1_1_smc(__VA_ARGS__);                 \
         |                         ^~~~~~~~~~~~~~~~~
   drivers/firmware/arm_scmi/smc.c:250:17: note: in expansion of macro 'arm_smccc_1_1_invoke'
     250 |                 arm_smccc_1_1_invoke(scmi_info->func_id, p[0].x, p[1].x, p[2].x,
         |                 ^~~~~~~~~~~~~~~~~~~~
>> include/linux/arm-smccc.h:436:29: error: register specified for 'arg1' isn't suitable for data type
     436 |         register typeof(a1) arg1 asm("r1") = __a1;                      \
         |                             ^~~~
   include/linux/arm-smccc.h:442:9: note: in expansion of macro '__declare_arg_3'
     442 |         __declare_arg_3(a0, a1, a2, a3, res);                           \
         |         ^~~~~~~~~~~~~~~
   include/linux/arm-smccc.h:447:9: note: in expansion of macro '__declare_arg_4'
     447 |         __declare_arg_4(a0, a1, a2, a3, a4, res);                       \
         |         ^~~~~~~~~~~~~~~
   include/linux/arm-smccc.h:452:9: note: in expansion of macro '__declare_arg_5'
     452 |         __declare_arg_5(a0, a1, a2, a3, a4, a5, res);                   \
         |         ^~~~~~~~~~~~~~~
   include/linux/arm-smccc.h:457:9: note: in expansion of macro '__declare_arg_6'
     457 |         __declare_arg_6(a0, a1, a2, a3, a4, a5, a6, res);               \
         |         ^~~~~~~~~~~~~~~
   include/linux/arm-smccc.h:460:37: note: in expansion of macro '__declare_arg_7'
     460 | #define ___declare_args(count, ...) __declare_arg_ ## count(__VA_ARGS__)
         |                                     ^~~~~~~~~~~~~~
   include/linux/arm-smccc.h:461:37: note: in expansion of macro '___declare_args'
     461 | #define __declare_args(count, ...)  ___declare_args(count, __VA_ARGS__)
         |                                     ^~~~~~~~~~~~~~~
   include/linux/arm-smccc.h:527:17: note: in expansion of macro '__declare_args'
     527 |                 __declare_args(__count_args(__VA_ARGS__), __VA_ARGS__); \
         |                 ^~~~~~~~~~~~~~
   include/linux/arm-smccc.h:558:25: note: in expansion of macro '__fail_smccc_1_1'
     558 |                         __fail_smccc_1_1(__VA_ARGS__);                  \
         |                         ^~~~~~~~~~~~~~~~
   drivers/firmware/arm_scmi/smc.c:250:17: note: in expansion of macro 'arm_smccc_1_1_invoke'
     250 |                 arm_smccc_1_1_invoke(scmi_info->func_id, p[0].x, p[1].x, p[2].x,
         |                 ^~~~~~~~~~~~~~~~~~~~
>> include/linux/arm-smccc.h:438:29: error: register specified for 'arg3' isn't suitable for data type
     438 |         register typeof(a3) arg3 asm("r3") = __a3
         |                             ^~~~
   include/linux/arm-smccc.h:442:9: note: in expansion of macro '__declare_arg_3'
     442 |         __declare_arg_3(a0, a1, a2, a3, res);                           \
         |         ^~~~~~~~~~~~~~~
   include/linux/arm-smccc.h:447:9: note: in expansion of macro '__declare_arg_4'
     447 |         __declare_arg_4(a0, a1, a2, a3, a4, res);                       \
         |         ^~~~~~~~~~~~~~~
   include/linux/arm-smccc.h:452:9: note: in expansion of macro '__declare_arg_5'
     452 |         __declare_arg_5(a0, a1, a2, a3, a4, a5, res);                   \
         |         ^~~~~~~~~~~~~~~
   include/linux/arm-smccc.h:457:9: note: in expansion of macro '__declare_arg_6'
     457 |         __declare_arg_6(a0, a1, a2, a3, a4, a5, a6, res);               \
         |         ^~~~~~~~~~~~~~~
   include/linux/arm-smccc.h:460:37: note: in expansion of macro '__declare_arg_7'
     460 | #define ___declare_args(count, ...) __declare_arg_ ## count(__VA_ARGS__)
         |                                     ^~~~~~~~~~~~~~
   include/linux/arm-smccc.h:461:37: note: in expansion of macro '___declare_args'
     461 | #define __declare_args(count, ...)  ___declare_args(count, __VA_ARGS__)
         |                                     ^~~~~~~~~~~~~~~
   include/linux/arm-smccc.h:527:17: note: in expansion of macro '__declare_args'
     527 |                 __declare_args(__count_args(__VA_ARGS__), __VA_ARGS__); \
         |                 ^~~~~~~~~~~~~~
   include/linux/arm-smccc.h:558:25: note: in expansion of macro '__fail_smccc_1_1'
     558 |                         __fail_smccc_1_1(__VA_ARGS__);                  \
         |                         ^~~~~~~~~~~~~~~~
   drivers/firmware/arm_scmi/smc.c:250:17: note: in expansion of macro 'arm_smccc_1_1_invoke'
     250 |                 arm_smccc_1_1_invoke(scmi_info->func_id, p[0].x, p[1].x, p[2].x,
         |                 ^~~~~~~~~~~~~~~~~~~~
>> include/linux/arm-smccc.h:448:29: error: register specified for 'arg5' isn't suitable for data type
     448 |         register typeof(a5) arg5 asm("r5") = __a5
         |                             ^~~~
   include/linux/arm-smccc.h:452:9: note: in expansion of macro '__declare_arg_5'
     452 |         __declare_arg_5(a0, a1, a2, a3, a4, a5, res);                   \
         |         ^~~~~~~~~~~~~~~
   include/linux/arm-smccc.h:457:9: note: in expansion of macro '__declare_arg_6'
     457 |         __declare_arg_6(a0, a1, a2, a3, a4, a5, a6, res);               \
         |         ^~~~~~~~~~~~~~~
   include/linux/arm-smccc.h:460:37: note: in expansion of macro '__declare_arg_7'
     460 | #define ___declare_args(count, ...) __declare_arg_ ## count(__VA_ARGS__)
         |                                     ^~~~~~~~~~~~~~
   include/linux/arm-smccc.h:461:37: note: in expansion of macro '___declare_args'
     461 | #define __declare_args(count, ...)  ___declare_args(count, __VA_ARGS__)
         |                                     ^~~~~~~~~~~~~~~
   include/linux/arm-smccc.h:527:17: note: in expansion of macro '__declare_args'
     527 |                 __declare_args(__count_args(__VA_ARGS__), __VA_ARGS__); \
         |                 ^~~~~~~~~~~~~~
   include/linux/arm-smccc.h:558:25: note: in expansion of macro '__fail_smccc_1_1'
     558 |                         __fail_smccc_1_1(__VA_ARGS__);                  \
         |                         ^~~~~~~~~~~~~~~~
   drivers/firmware/arm_scmi/smc.c:250:17: note: in expansion of macro 'arm_smccc_1_1_invoke'
     250 |                 arm_smccc_1_1_invoke(scmi_info->func_id, p[0].x, p[1].x, p[2].x,
         |                 ^~~~~~~~~~~~~~~~~~~~


vim +436 include/linux/arm-smccc.h

f2d3b2e8759a58 Marc Zyngier 2018-02-06  411  
f2d3b2e8759a58 Marc Zyngier 2018-02-06  412  #define __declare_arg_0(a0, res)					\
f2d3b2e8759a58 Marc Zyngier 2018-02-06  413  	struct arm_smccc_res   *___res = res;				\
0794a974d74dc7 Andrew Scull 2020-09-15  414  	register unsigned long arg0 asm("r0") = (u32)a0
f2d3b2e8759a58 Marc Zyngier 2018-02-06  415  
f2d3b2e8759a58 Marc Zyngier 2018-02-06  416  #define __declare_arg_1(a0, a1, res)					\
755a8bf5579d22 Marc Zyngier 2018-08-24  417  	typeof(a1) __a1 = a1;						\
f2d3b2e8759a58 Marc Zyngier 2018-02-06  418  	struct arm_smccc_res   *___res = res;				\
0794a974d74dc7 Andrew Scull 2020-09-15  419  	register unsigned long arg0 asm("r0") = (u32)a0;			\
0794a974d74dc7 Andrew Scull 2020-09-15  420  	register typeof(a1) arg1 asm("r1") = __a1
f2d3b2e8759a58 Marc Zyngier 2018-02-06  421  
f2d3b2e8759a58 Marc Zyngier 2018-02-06  422  #define __declare_arg_2(a0, a1, a2, res)				\
755a8bf5579d22 Marc Zyngier 2018-08-24  423  	typeof(a1) __a1 = a1;						\
755a8bf5579d22 Marc Zyngier 2018-08-24  424  	typeof(a2) __a2 = a2;						\
f2d3b2e8759a58 Marc Zyngier 2018-02-06  425  	struct arm_smccc_res   *___res = res;				\
0794a974d74dc7 Andrew Scull 2020-09-15  426  	register unsigned long arg0 asm("r0") = (u32)a0;			\
0794a974d74dc7 Andrew Scull 2020-09-15  427  	register typeof(a1) arg1 asm("r1") = __a1;			\
0794a974d74dc7 Andrew Scull 2020-09-15  428  	register typeof(a2) arg2 asm("r2") = __a2
f2d3b2e8759a58 Marc Zyngier 2018-02-06  429  
f2d3b2e8759a58 Marc Zyngier 2018-02-06  430  #define __declare_arg_3(a0, a1, a2, a3, res)				\
755a8bf5579d22 Marc Zyngier 2018-08-24  431  	typeof(a1) __a1 = a1;						\
755a8bf5579d22 Marc Zyngier 2018-08-24  432  	typeof(a2) __a2 = a2;						\
755a8bf5579d22 Marc Zyngier 2018-08-24  433  	typeof(a3) __a3 = a3;						\
f2d3b2e8759a58 Marc Zyngier 2018-02-06  434  	struct arm_smccc_res   *___res = res;				\
0794a974d74dc7 Andrew Scull 2020-09-15  435  	register unsigned long arg0 asm("r0") = (u32)a0;			\
0794a974d74dc7 Andrew Scull 2020-09-15 @436  	register typeof(a1) arg1 asm("r1") = __a1;			\
0794a974d74dc7 Andrew Scull 2020-09-15  437  	register typeof(a2) arg2 asm("r2") = __a2;			\
0794a974d74dc7 Andrew Scull 2020-09-15 @438  	register typeof(a3) arg3 asm("r3") = __a3
f2d3b2e8759a58 Marc Zyngier 2018-02-06  439  
f2d3b2e8759a58 Marc Zyngier 2018-02-06  440  #define __declare_arg_4(a0, a1, a2, a3, a4, res)			\
755a8bf5579d22 Marc Zyngier 2018-08-24  441  	typeof(a4) __a4 = a4;						\
f2d3b2e8759a58 Marc Zyngier 2018-02-06  442  	__declare_arg_3(a0, a1, a2, a3, res);				\
0794a974d74dc7 Andrew Scull 2020-09-15  443  	register typeof(a4) arg4 asm("r4") = __a4
f2d3b2e8759a58 Marc Zyngier 2018-02-06  444  
f2d3b2e8759a58 Marc Zyngier 2018-02-06  445  #define __declare_arg_5(a0, a1, a2, a3, a4, a5, res)			\
755a8bf5579d22 Marc Zyngier 2018-08-24  446  	typeof(a5) __a5 = a5;						\
f2d3b2e8759a58 Marc Zyngier 2018-02-06  447  	__declare_arg_4(a0, a1, a2, a3, a4, res);			\
0794a974d74dc7 Andrew Scull 2020-09-15 @448  	register typeof(a5) arg5 asm("r5") = __a5
f2d3b2e8759a58 Marc Zyngier 2018-02-06  449  

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

[PATCH v2 0/2] Allow parameter in smc/hvc calls

[Nikunj Kela]

Currently, smc/hvc calls are made with parameters set
to zeros. We are using multiple scmi instances within
a VM and hypervisor associates a tag with each instance
and expects the tag in hvc calls from that instance, while
sharing the same smc-id(func_id) among the instances.

This patch series introduces new optional dtb bindings which
can be used to pass parameters to smc/hvc calls.

---
v2 -> fix the compilation erros on 32bit platform(see below)
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202304100606.kUjhsRYf-lkp@intel.com/

v1 -> original patches

Nikunj Kela (2):
  dt-bindings: firmware: arm,scmi: support parameter passing in smc/hvc
  firmware: arm_scmi: Augment SMC/HVC to allow optional parameters

 .../bindings/firmware/arm,scmi.yaml           | 17 +++++
 drivers/firmware/arm_scmi/smc.c               | 72 ++++++++++++++++++-
 2 files changed, 88 insertions(+), 1 deletion(-)

-- 
2.17.1

[PATCH v2 1/2] dt-bindings: firmware: arm,scmi: support parameter passing in smc/hvc

[Nikunj Kela]

Currently, smc/hvc calls are made with smc-id only. The parameters are
all set to zeros. This patch defines two optional device tree bindings,
that can be used to pass parameters in smc/hvc calls.

This is useful when multiple scmi instances are used with common smc-id.

Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
---
 .../devicetree/bindings/firmware/arm,scmi.yaml | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
index 5824c43e9893..ecf76b763c8c 100644
--- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
+++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
@@ -115,6 +115,23 @@ properties:
     description:
       SMC id required when using smc or hvc transports
 
+  arm,smc32-params:
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    description:
+      An optional parameter list passed in smc32 or hvc32 calls
+    default: 0
+    minItems: 1
+    maxItems: 6
+
+  arm,smc64-params:
+    $ref: /schemas/types.yaml#/definitions/uint64-array
+    description:
+      An optional parameter list passed in smc64 or hvc64 calls.
+      This is valid only on ARM64 machines.
+    default: 0
+    minItems: 1
+    maxItems: 6
+
   linaro,optee-channel-id:
     $ref: /schemas/types.yaml#/definitions/uint32
     description:
@@ -427,6 +444,7 @@ examples:
             compatible = "arm,scmi-smc";
             shmem = <&cpu_scp_lpri0>, <&cpu_scp_lpri1>;
             arm,smc-id = <0xc3000001>;
+            arm,smc64-params = <0xcd974d6c 0x5ed97289>;
 
             #address-cells = <1>;
             #size-cells = <0>;
-- 
2.17.1

Re: [PATCH v2 1/2] dt-bindings: firmware: arm,scmi: support parameter passing in smc/hvc

[Sudeep Holla]

On Mon, Apr 10, 2023 at 11:20:57AM -0700, Nikunj Kela wrote:
> Currently, smc/hvc calls are made with smc-id only. The parameters are
> all set to zeros. This patch defines two optional device tree bindings,
> that can be used to pass parameters in smc/hvc calls.
>

Why 2 values ?

> This is useful when multiple scmi instances are used with common smc-id.
>

I really would like to avoid this binding. Because of lack of standard
SMC/HVC FID for SCMI we had to add this binding. Extending for newer use
case like this in a vendor specific way is something I would like to avoid.

> Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
> ---
>  .../devicetree/bindings/firmware/arm,scmi.yaml | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> index 5824c43e9893..ecf76b763c8c 100644
> --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> @@ -115,6 +115,23 @@ properties:
>      description:
>        SMC id required when using smc or hvc transports
>  
> +  arm,smc32-params:
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    description:
> +      An optional parameter list passed in smc32 or hvc32 calls
> +    default: 0
> +    minItems: 1
> +    maxItems: 6
> +
> +  arm,smc64-params:
> +    $ref: /schemas/types.yaml#/definitions/uint64-array
> +    description:
> +      An optional parameter list passed in smc64 or hvc64 calls.
> +      This is valid only on ARM64 machines.
> +    default: 0
> +    minItems: 1
> +    maxItems: 6
> +

Even if we end up adding(which I would very much like to avoid), I don't see
the need for 32 and 64 bit params like this. There must be ways to avoid that
used by some property in some other binding(I will look for one if we choose
this path)

-- 
Regards,
Sudeep

Re: [PATCH v2 1/2] dt-bindings: firmware: arm,scmi: support parameter passing in smc/hvc

[Nikunj Kela]

On 4/11/2023 5:54 AM, Sudeep Holla wrote:
> On Mon, Apr 10, 2023 at 11:20:57AM -0700, Nikunj Kela wrote:
>> Currently, smc/hvc calls are made with smc-id only. The parameters are
>> all set to zeros. This patch defines two optional device tree bindings,
>> that can be used to pass parameters in smc/hvc calls.
>>
> Why 2 values ?

I can do with one property if you prefer that. Its just I wanted to 
ensure that whosoever is setting

the parameter list, is mindful of 32bit vs 64bit convention. If we use 
one property, do you propose to add a new property like width to specify 
if the  parameter list is for 32bit vs 64bit?

>> This is useful when multiple scmi instances are used with common smc-id.
>>
> I really would like to avoid this binding. Because of lack of standard
> SMC/HVC FID for SCMI we had to add this binding. Extending for newer use
> case like this in a vendor specific way is something I would like to avoid.
If you have a solution to get rid of FID from DTB node, I will follow 
the same however until that happens, my view is that we put the 
parameters in dtb.
>
>> Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
>> ---
>>   .../devicetree/bindings/firmware/arm,scmi.yaml | 18 ++++++++++++++++++
>>   1 file changed, 18 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
>> index 5824c43e9893..ecf76b763c8c 100644
>> --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
>> +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
>> @@ -115,6 +115,23 @@ properties:
>>       description:
>>         SMC id required when using smc or hvc transports
>>   
>> +  arm,smc32-params:
>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
>> +    description:
>> +      An optional parameter list passed in smc32 or hvc32 calls
>> +    default: 0
>> +    minItems: 1
>> +    maxItems: 6
>> +
>> +  arm,smc64-params:
>> +    $ref: /schemas/types.yaml#/definitions/uint64-array
>> +    description:
>> +      An optional parameter list passed in smc64 or hvc64 calls.
>> +      This is valid only on ARM64 machines.
>> +    default: 0
>> +    minItems: 1
>> +    maxItems: 6
>> +
> Even if we end up adding(which I would very much like to avoid), I don't see
> the need for 32 and 64 bit params like this. There must be ways to avoid that
> used by some property in some other binding(I will look for one if we choose
> this path)
>

Re: [PATCH v2 1/2] dt-bindings: firmware: arm,scmi: support parameter passing in smc/hvc

[Krzysztof Kozlowski]

On 10/04/2023 20:20, Nikunj Kela wrote:
> Currently, smc/hvc calls are made with smc-id only. The parameters are
> all set to zeros. This patch defines two optional device tree bindings,
> that can be used to pass parameters in smc/hvc calls.
> 
> This is useful when multiple scmi instances are used with common smc-id.
> 
> Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>

Since you sent v2 before our discussion finished, let me answer here:
this does not look like property for DT. Do not send new patches without
giving reviewers chance to respond.

Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: firmware: arm,scmi: support parameter passing in smc/hvc

[Nikunj Kela]

On 4/11/2023 10:18 AM, Krzysztof Kozlowski wrote:
> On 10/04/2023 20:20, Nikunj Kela wrote:
>> Currently, smc/hvc calls are made with smc-id only. The parameters are
>> all set to zeros. This patch defines two optional device tree bindings,
>> that can be used to pass parameters in smc/hvc calls.
>>
>> This is useful when multiple scmi instances are used with common smc-id.
>>
>> Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
> Since you sent v2 before our discussion finished, let me answer here:
> this does not look like property for DT. Do not send new patches without
> giving reviewers chance to respond.
Ok. My rationale on that is, if we allow smc-id which goes in r0/w0/x0 
registers in smc/hvc call to be part of dtb, then we should allow 
parameters which go in r1/w1/x1 to r6/w6/x6 register to be part of dtb. 
If you have an alternative suggestion, I am all ears!
> Best regards,
> Krzysztof
>

[PATCH v2 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional parameters

[Nikunj Kela]

This patch add support for passing parameters to smc/hvc calls.
This patch is useful when multiple scmi instances are using same
smc-id and firmware needs to distiguish among the instances.

Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
---
 drivers/firmware/arm_scmi/smc.c | 72 ++++++++++++++++++++++++++++++++-
 1 file changed, 71 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
index 93272e4bbd12..c57d2f3fab87 100644
--- a/drivers/firmware/arm_scmi/smc.c
+++ b/drivers/firmware/arm_scmi/smc.c
@@ -20,6 +20,16 @@
 
 #include "common.h"
 
+#define MAX_PARAM_COUNT 6
+
+/**
+ * scmi_smc_param_t - parameter type for SCMI smc/hvc call
+ */
+typedef union {
+	u64 x;
+	u32 w;
+} scmi_smc_param_t;
+
 /**
  * struct scmi_smc - Structure representing a SCMI smc transport
  *
@@ -30,6 +40,8 @@
  * @inflight: Atomic flag to protect access to Tx/Rx shared memory area.
  *	      Used when operating in atomic mode.
  * @func_id: smc/hvc call function id
+ * @is_smc64: A flag, indicating smc64 calling convention.
+ * @params: Optional, smc/hvc call parameters.
  */
 
 struct scmi_smc {
@@ -40,8 +52,55 @@ struct scmi_smc {
 #define INFLIGHT_NONE	MSG_TOKEN_MAX
 	atomic_t inflight;
 	u32 func_id;
+	bool is_smc64;
+	scmi_smc_param_t params[MAX_PARAM_COUNT];
 };
 
+static void populate_smc_params(struct device *dev, struct scmi_smc *scmi_info)
+{
+	struct device_node *np = dev->of_node;
+	u64 params64[MAX_PARAM_COUNT] = { 0 };
+	u32 params32[MAX_PARAM_COUNT] = { 0 };
+	int i, count;
+
+#ifdef CONFIG_ARM64
+	if (scmi_info->is_smc64) {
+		count = of_property_read_variable_u64_array(np,
+							    "arm,smc64-params",
+							    &params64[0], 1,
+							    MAX_PARAM_COUNT);
+		if (count == -EINVAL)  /* if property is not defined */
+			return;
+
+		if (count > 0)	/* populate the parameters */
+			for (i = 0; i < count; i++)
+				scmi_info->params[i].x = params64[i];
+		else
+			goto param_err;
+	} else {
+#else
+	{
+#endif
+		count = of_property_read_variable_u32_array(np,
+							    "arm,smc32-params",
+							    &params32[0], 1,
+							    MAX_PARAM_COUNT);
+		if (count == -EINVAL)  /* if property is not defined */
+			return;
+
+		if (count > 0)	/* populate the parameters */
+			for (i = 0; i < count; i++)
+				scmi_info->params[i].w = params32[i];
+		else
+			goto param_err;
+	}
+
+	return;
+
+param_err:
+	dev_warn(dev, "failed to read smc/hvc call parameters\n");
+}
+
 static irqreturn_t smc_msg_done_isr(int irq, void *data)
 {
 	struct scmi_smc *scmi_info = data;
@@ -156,6 +215,8 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
 	}
 
 	scmi_info->func_id = func_id;
+	scmi_info->is_smc64 = ARM_SMCCC_IS_64(func_id);
+	populate_smc_params(dev, scmi_info);
 	scmi_info->cinfo = cinfo;
 	smc_channel_lock_init(scmi_info);
 	cinfo->transport_info = scmi_info;
@@ -179,6 +240,7 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
 {
 	struct scmi_smc *scmi_info = cinfo->transport_info;
 	struct arm_smccc_res res;
+	scmi_smc_param_t *p = scmi_info->params;
 
 	/*
 	 * Channel will be released only once response has been
@@ -188,7 +250,15 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
 
 	shmem_tx_prepare(scmi_info->shmem, xfer, cinfo);
 
-	arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
+#ifdef CONFIG_ARM64
+	if (scmi_info->is_smc64)
+		arm_smccc_1_1_invoke(scmi_info->func_id, p[0].x, p[1].x, p[2].x,
+				     p[3].x, p[4].x, p[5].x, 0, &res);
+	else
+#endif
+		arm_smccc_1_1_invoke(scmi_info->func_id, p[0].w, p[1].w, p[2].w,
+				     p[3].w, p[4].w, p[5].w, 0, &res);
+
 
 	/* Only SMCCC_RET_NOT_SUPPORTED is valid error code */
 	if (res.a0) {
-- 
2.17.1

Re: [PATCH v2 0/2] Allow parameter in smc/hvc calls

[Sudeep Holla]

On Mon, Apr 10, 2023 at 11:20:56AM -0700, Nikunj Kela wrote:
> Currently, smc/hvc calls are made with parameters set
> to zeros. We are using multiple scmi instances within
> a VM and hypervisor associates a tag with each instance
> and expects the tag in hvc calls from that instance, while
> sharing the same smc-id(func_id) among the instances.
> 
> This patch series introduces new optional dtb bindings which
> can be used to pass parameters to smc/hvc calls.
>

Just to be sure that I understood the problem(as your 2 parameters confused
me), this is just to help hypervisor/secure side to identify the right
channel/shared memory when the same SMC/HVC function id is shared right ?

If that is the case, why can't we just pass the shmem address as the
parameter ? I would like to avoid fragmentation here with every vendor
trying to do different things to achieve the same.

I would just change the driver to do that. Not sure if it breaks any existing
implementation or not. If it does, we can add another compatible to identify
one needing this fixed(shmem address) as additional parameter.

Does that make sense at all ? Or am I missing some/all of the requirements
here ?

-- 
Regards,
Sudeep

Re: [PATCH v2 0/2] Allow parameter in smc/hvc calls

[Nikunj Kela]

On 4/11/2023 6:01 AM, Sudeep Holla wrote:
> On Mon, Apr 10, 2023 at 11:20:56AM -0700, Nikunj Kela wrote:
>> Currently, smc/hvc calls are made with parameters set
>> to zeros. We are using multiple scmi instances within
>> a VM and hypervisor associates a tag with each instance
>> and expects the tag in hvc calls from that instance, while
>> sharing the same smc-id(func_id) among the instances.
>>
>> This patch series introduces new optional dtb bindings which
>> can be used to pass parameters to smc/hvc calls.
>>
> Just to be sure that I understood the problem(as your 2 parameters confused
> me), this is just to help hypervisor/secure side to identify the right
> channel/shared memory when the same SMC/HVC function id is shared right ?
that's somewhat correct. Our hypervisor allocates 64bit ids on every 
boot for each scmi instance which it uses to identify the scmi instance. 
Additionally, our hypervisor also categorizes events within each 
instance by an event-id that gets passed to hvc call as second 
parameter. So we can pass two parameters in addition to function_id.
>
> If that is the case, why can't we just pass the shmem address as the
> parameter ? I would like to avoid fragmentation here with every vendor
> trying to do different things to achieve the same.
that's a good suggestion. Any solution you propose shouldn't just limit 
to only one parameter. IMO, there should be some way to pass all 6 
parameters since we do have a use case of at least two parameters.
>
> I would just change the driver to do that. Not sure if it breaks any existing
> implementation or not. If it does, we can add another compatible to identify
> one needing this fixed(shmem address) as additional parameter.
>
> Does that make sense at all ? Or am I missing some/all of the requirements
> here ?
The shmem proposal is fine however please also incorporate passing of 
other parameters.
>

Re: [PATCH v2 0/2] Allow parameter in smc/hvc calls

[Sudeep Holla]

On Tue, Apr 11, 2023 at 07:42:50AM -0700, Nikunj Kela wrote:

> that's a good suggestion. Any solution you propose shouldn't just limit to
> only one parameter. IMO, there should be some way to pass all 6 parameters
> since we do have a use case of at least two parameters.

Please elaborate on your use-case.

> The shmem proposal is fine however please also incorporate passing of other
> parameters.

You are missing the point here. SMC/HVC is just a doorbell and the main point
I made earlier is that there is no need for vendors to try colourful things
here if it is not necessary. So no, I don't want any extra bindings or more
than one param is that is not needed. I will wait for the reason as requested
above.

-- 
Regards,
Sudeep

Re: [PATCH v2 0/2] Allow parameter in smc/hvc calls

[Nikunj Kela]

On 4/12/2023 1:37 AM, Sudeep Holla wrote:
> On Tue, Apr 11, 2023 at 07:42:50AM -0700, Nikunj Kela wrote:
>
>> that's a good suggestion. Any solution you propose shouldn't just limit to
>> only one parameter. IMO, there should be some way to pass all 6 parameters
>> since we do have a use case of at least two parameters.
> Please elaborate on your use-case.
Based on your comments below, we will change our hypervisor to make use 
of shmem.
>
>> The shmem proposal is fine however please also incorporate passing of other
>> parameters.
> You are missing the point here. SMC/HVC is just a doorbell and the main point
> I made earlier is that there is no need for vendors to try colourful things
> here if it is not necessary. So no, I don't want any extra bindings or more
> than one param is that is not needed. I will wait for the reason as requested
> above.
ok, understood. In that case, we will change our hypervisor to use shmem 
address as instance identifier. Please add support for one param, thanks!

[PATCH v3 0/2] Allow parameter in smc/hvc calls

[Nikunj Kela]

Currently, smc/hvc calls are made with parameters set
to zeros. We are using multiple scmi instances within
a VM. We are sharing the same smc-id(func_id) with all
scmi instance. The hypervisor needs a way to distinguish 
among hvc calls made from different instances.

This patch series introduces new compatible string which
can be used to pass shmem channel address as a parameter
to smc/hvc calls.

---
v3 -> pass shmem channel address as parameter

v2 -> fix the compilation erros on 32bit platform(see below)
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202304100606.kUjhsRYf-lkp@intel.com/

v1 -> original patches

Nikunj Kela (2):
  dt-bindings: firmware: arm,scmi: support for parameter in smc/hvc call
  firmware: arm_scmi: Augment SMC/HVC to allow optional parameter

 .../bindings/firmware/arm,scmi.yaml           |  4 +++
 drivers/firmware/arm_scmi/driver.c            |  1 +
 drivers/firmware/arm_scmi/smc.c               | 25 ++++++++++++++++++-
 3 files changed, 29 insertions(+), 1 deletion(-)

-- 
2.17.1

[PATCH v3 1/2] dt-bindings: firmware: arm,scmi: support for parameter in smc/hvc call

[Nikunj Kela]

Currently, smc/hvc calls are made with smc-id only. The parameters are
all set to zeros. This patch defines a new compatible string that can
be used to pass shmem address as one parameter in SMC/HVC doorbell.

This is useful when multiple scmi instances are used with common smc-id.

Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
---
 Documentation/devicetree/bindings/firmware/arm,scmi.yaml | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
index 5824c43e9893..e1f39a10d405 100644
--- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
+++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
@@ -34,6 +34,10 @@ properties:
       - description: SCMI compliant firmware with ARM SMC/HVC transport
         items:
           - const: arm,scmi-smc
+      - description: SCMI compliant firmware with ARM SMC/HVC transport
+                     with shmem address as parameter
+        items:
+          - const: arm,scmi-smc-param
       - description: SCMI compliant firmware with SCMI Virtio transport.
                      The virtio transport only supports a single device.
         items:
@@ -299,7 +303,9 @@ else:
     properties:
       compatible:
         contains:
-          const: arm,scmi-smc
+          enum:
+            - arm,scmi-smc
+            - arm,scmi-smc-param
   then:
     required:
       - arm,smc-id
-- 
2.17.1

[PATCH v3 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional parameter

[Nikunj Kela]

This patch add support for passing shmem channel address as parameter
in smc/hvc call. This patch is useful when multiple scmi instances are
using same smc-id and firmware needs to distiguish among the instances.

Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
---
 drivers/firmware/arm_scmi/driver.c |  1 +
 drivers/firmware/arm_scmi/smc.c    | 25 ++++++++++++++++++++++++-
 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index e7d97b59963b..b5957cc12fee 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -2914,6 +2914,7 @@ static const struct of_device_id scmi_of_match[] = {
 #endif
 #ifdef CONFIG_ARM_SCMI_TRANSPORT_SMC
 	{ .compatible = "arm,scmi-smc", .data = &scmi_smc_desc},
+	{ .compatible = "arm,scmi-smc-param", .data = &scmi_smc_desc},
 #endif
 #ifdef CONFIG_ARM_SCMI_TRANSPORT_VIRTIO
 	{ .compatible = "arm,scmi-virtio", .data = &scmi_virtio_desc},
diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
index 93272e4bbd12..e28387346d33 100644
--- a/drivers/firmware/arm_scmi/smc.c
+++ b/drivers/firmware/arm_scmi/smc.c
@@ -20,6 +20,9 @@
 
 #include "common.h"
 
+#define lower32(x)	((u32)((x) & 0xffffffff))
+#define upper32(x)	((u32)(((u64)(x) >> 32) & 0xffffffff))
+
 /**
  * struct scmi_smc - Structure representing a SCMI smc transport
  *
@@ -30,6 +33,8 @@
  * @inflight: Atomic flag to protect access to Tx/Rx shared memory area.
  *	      Used when operating in atomic mode.
  * @func_id: smc/hvc call function id
+ * @is_smc64: smc/hvc calling convention type 64 vs 32
+ * @param: physical address of the shmem channel
  */
 
 struct scmi_smc {
@@ -40,6 +45,8 @@ struct scmi_smc {
 #define INFLIGHT_NONE	MSG_TOKEN_MAX
 	atomic_t inflight;
 	u32 func_id;
+	bool is_smc64;
+	phys_addr_t param;
 };
 
 static irqreturn_t smc_msg_done_isr(int irq, void *data)
@@ -137,6 +144,8 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
 	if (ret < 0)
 		return ret;
 
+	if (of_device_is_compatible(dev->of_node, "arm,scmi-smc-param"))
+		scmi_info->param = res.start;
 	/*
 	 * If there is an interrupt named "a2p", then the service and
 	 * completion of a message is signaled by an interrupt rather than by
@@ -156,6 +165,7 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
 	}
 
 	scmi_info->func_id = func_id;
+	scmi_info->is_smc64 = ARM_SMCCC_IS_64(func_id);
 	scmi_info->cinfo = cinfo;
 	smc_channel_lock_init(scmi_info);
 	cinfo->transport_info = scmi_info;
@@ -188,7 +198,20 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
 
 	shmem_tx_prepare(scmi_info->shmem, xfer, cinfo);
 
-	arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
+#ifdef CONFIG_ARM64
+	/*
+	 * if SMC32 convention is used, pass 64 bit address in
+	 * two parameters
+	 */
+	if (!scmi_info->is_smc64)
+		arm_smccc_1_1_invoke(scmi_info->func_id,
+				     lower32(scmi_info->param),
+				     upper32(scmi_info->param),
+				     0, 0, 0, 0, 0, &res);
+	else
+#endif
+		arm_smccc_1_1_invoke(scmi_info->func_id, scmi_info->param,
+				     0, 0, 0, 0, 0, 0, &res);
 
 	/* Only SMCCC_RET_NOT_SUPPORTED is valid error code */
 	if (res.a0) {
-- 
2.17.1

Re: [PATCH v3 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional parameter

[Florian Fainelli]

On 4/17/23 10:44, Nikunj Kela wrote:
> This patch add support for passing shmem channel address as parameter
> in smc/hvc call. This patch is useful when multiple scmi instances are
> using same smc-id and firmware needs to distiguish among the instances.

Typo: distinguish.

It really would have been a lot clearer and made a whole lot more sense 
to encode a VM ID/channel number within some of the SMCCC parameters, 
possibly as part of the function ID itself.

> 
> Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
> ---
>   drivers/firmware/arm_scmi/driver.c |  1 +
>   drivers/firmware/arm_scmi/smc.c    | 25 ++++++++++++++++++++++++-
>   2 files changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index e7d97b59963b..b5957cc12fee 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -2914,6 +2914,7 @@ static const struct of_device_id scmi_of_match[] = {
>   #endif
>   #ifdef CONFIG_ARM_SCMI_TRANSPORT_SMC
>   	{ .compatible = "arm,scmi-smc", .data = &scmi_smc_desc},
> +	{ .compatible = "arm,scmi-smc-param", .data = &scmi_smc_desc},
>   #endif
>   #ifdef CONFIG_ARM_SCMI_TRANSPORT_VIRTIO
>   	{ .compatible = "arm,scmi-virtio", .data = &scmi_virtio_desc},
> diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
> index 93272e4bbd12..e28387346d33 100644
> --- a/drivers/firmware/arm_scmi/smc.c
> +++ b/drivers/firmware/arm_scmi/smc.c
> @@ -20,6 +20,9 @@
>   
>   #include "common.h"
>   
> +#define lower32(x)	((u32)((x) & 0xffffffff))
> +#define upper32(x)	((u32)(((u64)(x) >> 32) & 0xffffffff))

Cannot you use the existing lower_32_bits and upper_32_bits macros from 
kernel.h here?

> +
>   /**
>    * struct scmi_smc - Structure representing a SCMI smc transport
>    *
> @@ -30,6 +33,8 @@
>    * @inflight: Atomic flag to protect access to Tx/Rx shared memory area.
>    *	      Used when operating in atomic mode.
>    * @func_id: smc/hvc call function id
> + * @is_smc64: smc/hvc calling convention type 64 vs 32
> + * @param: physical address of the shmem channel
>    */
>   
>   struct scmi_smc {
> @@ -40,6 +45,8 @@ struct scmi_smc {
>   #define INFLIGHT_NONE	MSG_TOKEN_MAX
>   	atomic_t inflight;
>   	u32 func_id;
> +	bool is_smc64;
> +	phys_addr_t param;
>   };
>   
>   static irqreturn_t smc_msg_done_isr(int irq, void *data)
> @@ -137,6 +144,8 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
>   	if (ret < 0)
>   		return ret;
>   
> +	if (of_device_is_compatible(dev->of_node, "arm,scmi-smc-param"))
> +		scmi_info->param = res.start;

There is not even a check that this is going to be part of the kernel's 
view of memory, that seems a bit brittle and possibly a security hole, 
too. Your hypervisor presumably needs to have carved out some amount of 
memory in order for the messages to be written to/read from, and so 
would the VM kernel, so eventually we should have a 'reserved-memory' 
entry of some sort, no?

>   	/*
>   	 * If there is an interrupt named "a2p", then the service and
>   	 * completion of a message is signaled by an interrupt rather than by
> @@ -156,6 +165,7 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
>   	}
>   
>   	scmi_info->func_id = func_id;
> +	scmi_info->is_smc64 = ARM_SMCCC_IS_64(func_id);
>   	scmi_info->cinfo = cinfo;
>   	smc_channel_lock_init(scmi_info);
>   	cinfo->transport_info = scmi_info;
> @@ -188,7 +198,20 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
>   
>   	shmem_tx_prepare(scmi_info->shmem, xfer, cinfo);
>   
> -	arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
> +#ifdef CONFIG_ARM64
> +	/*
> +	 * if SMC32 convention is used, pass 64 bit address in
> +	 * two parameters
> +	 */
> +	if (!scmi_info->is_smc64)

There is no need for scmi_info to store is_smc64, just check the func_id 
here and declare is_smc64 as a local variable to the function.

Also, another way to approach this would be to encode the parameters 
region in 4KB units such that event on a 32-bit system with LPAE you are 
guaranteed to fit the region into a 32-bit unsigned long. AFAIR 
virtualization and LPAE are indistinguishable on real CPUs?

> +		arm_smccc_1_1_invoke(scmi_info->func_id,
> +				     lower32(scmi_info->param),
> +				     upper32(scmi_info->param),
> +				     0, 0, 0, 0, 0, &res);
> +	else
> +#endif
> +		arm_smccc_1_1_invoke(scmi_info->func_id, scmi_info->param,
> +				     0, 0, 0, 0, 0, 0, &res);
>   
>   	/* Only SMCCC_RET_NOT_SUPPORTED is valid error code */
>   	if (res.a0) {

-- 
Florian

Re: [PATCH v3 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional parameter

[Nikunj Kela]

On 4/17/2023 11:01 AM, Florian Fainelli wrote:
> On 4/17/23 10:44, Nikunj Kela wrote:
>> This patch add support for passing shmem channel address as parameter
>> in smc/hvc call. This patch is useful when multiple scmi instances are
>> using same smc-id and firmware needs to distiguish among the instances.
>
> Typo: distinguish.
>
Will fix it.
> It really would have been a lot clearer and made a whole lot more 
> sense to encode a VM ID/channel number within some of the SMCCC 
> parameters, possibly as part of the function ID itself.
smc-id(func-id) is 32 bit long and the spec doesn't define any such 
provisions in it. Having said that, there are optional parameters as 
session-id and client-id(secureOS-id) that can be passed in w6/r6 and 
w7/r7 registers. If maintainers are OK to use dtb to pass them instead, 
I can rework the patches.
>
>>
>> Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
>> ---
>>   drivers/firmware/arm_scmi/driver.c |  1 +
>>   drivers/firmware/arm_scmi/smc.c    | 25 ++++++++++++++++++++++++-
>>   2 files changed, 25 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/firmware/arm_scmi/driver.c 
>> b/drivers/firmware/arm_scmi/driver.c
>> index e7d97b59963b..b5957cc12fee 100644
>> --- a/drivers/firmware/arm_scmi/driver.c
>> +++ b/drivers/firmware/arm_scmi/driver.c
>> @@ -2914,6 +2914,7 @@ static const struct of_device_id 
>> scmi_of_match[] = {
>>   #endif
>>   #ifdef CONFIG_ARM_SCMI_TRANSPORT_SMC
>>       { .compatible = "arm,scmi-smc", .data = &scmi_smc_desc},
>> +    { .compatible = "arm,scmi-smc-param", .data = &scmi_smc_desc},
>>   #endif
>>   #ifdef CONFIG_ARM_SCMI_TRANSPORT_VIRTIO
>>       { .compatible = "arm,scmi-virtio", .data = &scmi_virtio_desc},
>> diff --git a/drivers/firmware/arm_scmi/smc.c 
>> b/drivers/firmware/arm_scmi/smc.c
>> index 93272e4bbd12..e28387346d33 100644
>> --- a/drivers/firmware/arm_scmi/smc.c
>> +++ b/drivers/firmware/arm_scmi/smc.c
>> @@ -20,6 +20,9 @@
>>     #include "common.h"
>>   +#define lower32(x)    ((u32)((x) & 0xffffffff))
>> +#define upper32(x)    ((u32)(((u64)(x) >> 32) & 0xffffffff))
>
> Cannot you use the existing lower_32_bits and upper_32_bits macros 
> from kernel.h here?
>
>> +
>>   /**
>>    * struct scmi_smc - Structure representing a SCMI smc transport
>>    *
>> @@ -30,6 +33,8 @@
>>    * @inflight: Atomic flag to protect access to Tx/Rx shared memory 
>> area.
>>    *          Used when operating in atomic mode.
>>    * @func_id: smc/hvc call function id
>> + * @is_smc64: smc/hvc calling convention type 64 vs 32
>> + * @param: physical address of the shmem channel
>>    */
>>     struct scmi_smc {
>> @@ -40,6 +45,8 @@ struct scmi_smc {
>>   #define INFLIGHT_NONE    MSG_TOKEN_MAX
>>       atomic_t inflight;
>>       u32 func_id;
>> +    bool is_smc64;
>> +    phys_addr_t param;
>>   };
>>     static irqreturn_t smc_msg_done_isr(int irq, void *data)
>> @@ -137,6 +144,8 @@ static int smc_chan_setup(struct scmi_chan_info 
>> *cinfo, struct device *dev,
>>       if (ret < 0)
>>           return ret;
>>   +    if (of_device_is_compatible(dev->of_node, "arm,scmi-smc-param"))
>> +        scmi_info->param = res.start;
>
> There is not even a check that this is going to be part of the 
> kernel's view of memory, that seems a bit brittle and possibly a 
> security hole, too. Your hypervisor presumably needs to have carved 
> out some amount of memory in order for the messages to be written 
> to/read from, and so would the VM kernel, so eventually we should have 
> a 'reserved-memory' entry of some sort, no?
>
>>       /*
>>        * If there is an interrupt named "a2p", then the service and
>>        * completion of a message is signaled by an interrupt rather 
>> than by
>> @@ -156,6 +165,7 @@ static int smc_chan_setup(struct scmi_chan_info 
>> *cinfo, struct device *dev,
>>       }
>>         scmi_info->func_id = func_id;
>> +    scmi_info->is_smc64 = ARM_SMCCC_IS_64(func_id);
>>       scmi_info->cinfo = cinfo;
>>       smc_channel_lock_init(scmi_info);
>>       cinfo->transport_info = scmi_info;
>> @@ -188,7 +198,20 @@ static int smc_send_message(struct 
>> scmi_chan_info *cinfo,
>>         shmem_tx_prepare(scmi_info->shmem, xfer, cinfo);
>>   -    arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, 
>> &res);
>> +#ifdef CONFIG_ARM64
>> +    /*
>> +     * if SMC32 convention is used, pass 64 bit address in
>> +     * two parameters
>> +     */
>> +    if (!scmi_info->is_smc64)
>
> There is no need for scmi_info to store is_smc64, just check the 
> func_id here and declare is_smc64 as a local variable to the function.
>
> Also, another way to approach this would be to encode the parameters 
> region in 4KB units such that event on a 32-bit system with LPAE you 
> are guaranteed to fit the region into a 32-bit unsigned long. AFAIR 
> virtualization and LPAE are indistinguishable on real CPUs?
>
>> + arm_smccc_1_1_invoke(scmi_info->func_id,
>> +                     lower32(scmi_info->param),
>> +                     upper32(scmi_info->param),
>> +                     0, 0, 0, 0, 0, &res);
>> +    else
>> +#endif
>> +        arm_smccc_1_1_invoke(scmi_info->func_id, scmi_info->param,
>> +                     0, 0, 0, 0, 0, 0, &res);
>>         /* Only SMCCC_RET_NOT_SUPPORTED is valid error code */
>>       if (res.a0) {
>

Re: [PATCH v3 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional parameter

[Sudeep Holla]

On Mon, Apr 17, 2023 at 11:01:13AM -0700, Florian Fainelli wrote:
> On 4/17/23 10:44, Nikunj Kela wrote:
> > This patch add support for passing shmem channel address as parameter
> > in smc/hvc call. This patch is useful when multiple scmi instances are
> > using same smc-id and firmware needs to distiguish among the instances.
> 
> Typo: distinguish.
> 
> It really would have been a lot clearer and made a whole lot more sense to
> encode a VM ID/channel number within some of the SMCCC parameters, possibly
> as part of the function ID itself.
>

Yes I was about to suggest this but then remembered one main reason I have
been given in the past against that:
If the system launches high number of VMs then that means loads of FID
needs to be reserved for SCMI and the hypervisor needs to support it.
Basically it is not scalable which I agree but not sure if such large
number of VMs are used in reality with SCMI. But I agree with the technical
reasoning.

The other reason why I preferred the shmem address itself instead of some
custom VM ID/channel number is that it can easily becomes vendor specific
for no real good reason and then we need to add support for each such
different parameters. Nikunj suggested getting them from DT which I really
don't like if the sole reason is just to identify the channel. We don't
have standard SCMI SMC/HVC but allowing such deviations with params from
DT will just explode with various combinations for silly/no reason.


[...]

> > @@ -137,6 +144,8 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
> >   	if (ret < 0)
> >   		return ret;
> > +	if (of_device_is_compatible(dev->of_node, "arm,scmi-smc-param"))
> > +		scmi_info->param = res.start;
>
> There is not even a check that this is going to be part of the kernel's view
> of memory, that seems a bit brittle and possibly a security hole, too. Your
> hypervisor presumably needs to have carved out some amount of memory in
> order for the messages to be written to/read from, and so would the VM
> kernel, so eventually we should have a 'reserved-memory' entry of some sort,
> no?
>

Not disagreeing here. Just checking if my understanding is correct or not.
IIUC, we need reserved-memory if it is part of the memory presented to the
OS right ? You don't need that if it is dedicated memory like part of SRAM
or something similar.

> >   	/*
> >   	 * If there is an interrupt named "a2p", then the service and
> >   	 * completion of a message is signaled by an interrupt rather than by
> > @@ -156,6 +165,7 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
> >   	}
> >   	scmi_info->func_id = func_id;
> > +	scmi_info->is_smc64 = ARM_SMCCC_IS_64(func_id);
> >   	scmi_info->cinfo = cinfo;
> >   	smc_channel_lock_init(scmi_info);
> >   	cinfo->transport_info = scmi_info;
> > @@ -188,7 +198,20 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
> >   	shmem_tx_prepare(scmi_info->shmem, xfer, cinfo);
> > -	arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
> > +#ifdef CONFIG_ARM64
> > +	/*
> > +	 * if SMC32 convention is used, pass 64 bit address in
> > +	 * two parameters
> > +	 */
> > +	if (!scmi_info->is_smc64)
> 
> There is no need for scmi_info to store is_smc64, just check the func_id
> here and declare is_smc64 as a local variable to the function.
>

+1

> Also, another way to approach this would be to encode the parameters region
> in 4KB units such that event on a 32-bit system with LPAE you are guaranteed
> to fit the region into a 32-bit unsigned long. AFAIR virtualization and LPAE
> are indistinguishable on real CPUs?
>

Agree with the idea. But can a single 4kB be used for multiple shmem across
VMs ? IIUC the hypervisor can deal with that, so I prefer it if it is possible
practically.

--
Regards,
Sudeep

Re: [PATCH v3 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional parameter

[Nikunj Kela]

On 4/18/2023 2:58 AM, Sudeep Holla wrote:
> On Mon, Apr 17, 2023 at 11:01:13AM -0700, Florian Fainelli wrote:
>> On 4/17/23 10:44, Nikunj Kela wrote:
>>> This patch add support for passing shmem channel address as parameter
>>> in smc/hvc call. This patch is useful when multiple scmi instances are
>>> using same smc-id and firmware needs to distiguish among the instances.
>> Typo: distinguish.
>>
>> It really would have been a lot clearer and made a whole lot more sense to
>> encode a VM ID/channel number within some of the SMCCC parameters, possibly
>> as part of the function ID itself.
>>
> Yes I was about to suggest this but then remembered one main reason I have
> been given in the past against that:
> If the system launches high number of VMs then that means loads of FID
> needs to be reserved for SCMI and the hypervisor needs to support it.
> Basically it is not scalable which I agree but not sure if such large
> number of VMs are used in reality with SCMI. But I agree with the technical
> reasoning.
>
> The other reason why I preferred the shmem address itself instead of some
> custom VM ID/channel number is that it can easily becomes vendor specific
> for no real good reason and then we need to add support for each such
> different parameters. Nikunj suggested getting them from DT which I really
> don't like if the sole reason is just to identify the channel. We don't
> have standard SCMI SMC/HVC but allowing such deviations with params from
> DT will just explode with various combinations for silly/no reason.
>
Would you be ok to pass the smc/hvc parameters via kernel parameters in 
commandline? If so, I can implement that. I just wanted to keep 
everything in one place hence suggested using DTB node.

> [...]
>
>>> @@ -137,6 +144,8 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
>>>    	if (ret < 0)
>>>    		return ret;
>>> +	if (of_device_is_compatible(dev->of_node, "arm,scmi-smc-param"))
>>> +		scmi_info->param = res.start;
>> There is not even a check that this is going to be part of the kernel's view
>> of memory, that seems a bit brittle and possibly a security hole, too. Your
>> hypervisor presumably needs to have carved out some amount of memory in
>> order for the messages to be written to/read from, and so would the VM
>> kernel, so eventually we should have a 'reserved-memory' entry of some sort,
>> no?
>>
> Not disagreeing here. Just checking if my understanding is correct or not.
> IIUC, we need reserved-memory if it is part of the memory presented to the
> OS right ? You don't need that if it is dedicated memory like part of SRAM
> or something similar.
We are not using reserved-memory node. Instead using sram-mmio to carve 
out shmem for scmi instances.
>>>    	/*
>>>    	 * If there is an interrupt named "a2p", then the service and
>>>    	 * completion of a message is signaled by an interrupt rather than by
>>> @@ -156,6 +165,7 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
>>>    	}
>>>    	scmi_info->func_id = func_id;
>>> +	scmi_info->is_smc64 = ARM_SMCCC_IS_64(func_id);
>>>    	scmi_info->cinfo = cinfo;
>>>    	smc_channel_lock_init(scmi_info);
>>>    	cinfo->transport_info = scmi_info;
>>> @@ -188,7 +198,20 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
>>>    	shmem_tx_prepare(scmi_info->shmem, xfer, cinfo);
>>> -	arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
>>> +#ifdef CONFIG_ARM64
>>> +	/*
>>> +	 * if SMC32 convention is used, pass 64 bit address in
>>> +	 * two parameters
>>> +	 */
>>> +	if (!scmi_info->is_smc64)
>> There is no need for scmi_info to store is_smc64, just check the func_id
>> here and declare is_smc64 as a local variable to the function.
>>
> +1
ACK!
>> Also, another way to approach this would be to encode the parameters region
>> in 4KB units such that event on a 32-bit system with LPAE you are guaranteed
>> to fit the region into a 32-bit unsigned long. AFAIR virtualization and LPAE
>> are indistinguishable on real CPUs?
>>
> Agree with the idea. But can a single 4kB be used for multiple shmem across
> VMs ? IIUC the hypervisor can deal with that, so I prefer it if it is possible
> practically.
We have multiple VMs and each VM has multiple instances. We will have 
quite a few domains and I am not sure if single page will suffice.
> --
> Regards,
> Sudeep

Re: [PATCH v3 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional parameter

[Florian Fainelli]

On 4/18/23 07:20, Nikunj Kela wrote:
> 
> On 4/18/2023 2:58 AM, Sudeep Holla wrote:
>> On Mon, Apr 17, 2023 at 11:01:13AM -0700, Florian Fainelli wrote:
>>> On 4/17/23 10:44, Nikunj Kela wrote:
>>>> This patch add support for passing shmem channel address as parameter
>>>> in smc/hvc call. This patch is useful when multiple scmi instances are
>>>> using same smc-id and firmware needs to distiguish among the instances.
>>> Typo: distinguish.
>>>
>>> It really would have been a lot clearer and made a whole lot more 
>>> sense to
>>> encode a VM ID/channel number within some of the SMCCC parameters, 
>>> possibly
>>> as part of the function ID itself.
>>>
>> Yes I was about to suggest this but then remembered one main reason I 
>> have
>> been given in the past against that:
>> If the system launches high number of VMs then that means loads of FID
>> needs to be reserved for SCMI and the hypervisor needs to support it.
>> Basically it is not scalable which I agree but not sure if such large
>> number of VMs are used in reality with SCMI. But I agree with the 
>> technical
>> reasoning.
>>
>> The other reason why I preferred the shmem address itself instead of some
>> custom VM ID/channel number is that it can easily becomes vendor specific
>> for no real good reason and then we need to add support for each such
>> different parameters. Nikunj suggested getting them from DT which I 
>> really
>> don't like if the sole reason is just to identify the channel. We don't
>> have standard SCMI SMC/HVC but allowing such deviations with params from
>> DT will just explode with various combinations for silly/no reason.
>>
> Would you be ok to pass the smc/hvc parameters via kernel parameters in 
> commandline? If so, I can implement that. I just wanted to keep 
> everything in one place hence suggested using DTB node.

Command line arguments seem a bit unnecessary here and it would force 
you to invent a scheme to control per SCMI device/instance parameters.

> 
>> [...]
>>
>>>> @@ -137,6 +144,8 @@ static int smc_chan_setup(struct scmi_chan_info 
>>>> *cinfo, struct device *dev,
>>>>        if (ret < 0)
>>>>            return ret;
>>>> +    if (of_device_is_compatible(dev->of_node, "arm,scmi-smc-param"))
>>>> +        scmi_info->param = res.start;
>>> There is not even a check that this is going to be part of the 
>>> kernel's view
>>> of memory, that seems a bit brittle and possibly a security hole, 
>>> too. Your
>>> hypervisor presumably needs to have carved out some amount of memory in
>>> order for the messages to be written to/read from, and so would the VM
>>> kernel, so eventually we should have a 'reserved-memory' entry of 
>>> some sort,
>>> no?
>>>
>> Not disagreeing here. Just checking if my understanding is correct or 
>> not.
>> IIUC, we need reserved-memory if it is part of the memory presented to 
>> the
>> OS right ? You don't need that if it is dedicated memory like part of 
>> SRAM
>> or something similar.
> We are not using reserved-memory node. Instead using sram-mmio to carve 
> out shmem for scmi instances.

OK, that works too.

>>>>        /*
>>>>         * If there is an interrupt named "a2p", then the service and
>>>>         * completion of a message is signaled by an interrupt rather 
>>>> than by
>>>> @@ -156,6 +165,7 @@ static int smc_chan_setup(struct scmi_chan_info 
>>>> *cinfo, struct device *dev,
>>>>        }
>>>>        scmi_info->func_id = func_id;
>>>> +    scmi_info->is_smc64 = ARM_SMCCC_IS_64(func_id);
>>>>        scmi_info->cinfo = cinfo;
>>>>        smc_channel_lock_init(scmi_info);
>>>>        cinfo->transport_info = scmi_info;
>>>> @@ -188,7 +198,20 @@ static int smc_send_message(struct 
>>>> scmi_chan_info *cinfo,
>>>>        shmem_tx_prepare(scmi_info->shmem, xfer, cinfo);
>>>> -    arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, 
>>>> &res);
>>>> +#ifdef CONFIG_ARM64
>>>> +    /*
>>>> +     * if SMC32 convention is used, pass 64 bit address in
>>>> +     * two parameters
>>>> +     */
>>>> +    if (!scmi_info->is_smc64)
>>> There is no need for scmi_info to store is_smc64, just check the func_id
>>> here and declare is_smc64 as a local variable to the function.
>>>
>> +1
> ACK!
>>> Also, another way to approach this would be to encode the parameters 
>>> region
>>> in 4KB units such that event on a 32-bit system with LPAE you are 
>>> guaranteed
>>> to fit the region into a 32-bit unsigned long. AFAIR virtualization 
>>> and LPAE
>>> are indistinguishable on real CPUs?
>>>
>> Agree with the idea. But can a single 4kB be used for multiple shmem 
>> across
>> VMs ? IIUC the hypervisor can deal with that, so I prefer it if it is 
>> possible
>> practically.
> We have multiple VMs and each VM has multiple instances. We will have 
> quite a few domains and I am not sure if single page will suffice.

I did not make myself clear enough, you can encode an offset into the 
shared memory area, and however big that shared memory area will be, 
that offset can be in a 4KB size. So for instance if you have your MMIO 
SRAM at 0x8000_0000, the first VM can use 0x8000_0ffff, the second VM 
can use 0x8000_1000 to 0x8000_1fff and so on and so forth.

Even if you need more than 4KB per VM, you can put that information into 
the two additional parameters you pass through the SMC/HVC call.
-- 
Florian

Re: [PATCH v3 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional parameter

[Nikunj Kela]

On 4/18/2023 9:33 AM, Florian Fainelli wrote:
> On 4/18/23 07:20, Nikunj Kela wrote:
>>
>> On 4/18/2023 2:58 AM, Sudeep Holla wrote:
>>> On Mon, Apr 17, 2023 at 11:01:13AM -0700, Florian Fainelli wrote:
>>>> On 4/17/23 10:44, Nikunj Kela wrote:
>>>>> This patch add support for passing shmem channel address as parameter
>>>>> in smc/hvc call. This patch is useful when multiple scmi instances 
>>>>> are
>>>>> using same smc-id and firmware needs to distiguish among the 
>>>>> instances.
>>>> Typo: distinguish.
>>>>
>>>> It really would have been a lot clearer and made a whole lot more 
>>>> sense to
>>>> encode a VM ID/channel number within some of the SMCCC parameters, 
>>>> possibly
>>>> as part of the function ID itself.
>>>>
>>> Yes I was about to suggest this but then remembered one main reason 
>>> I have
>>> been given in the past against that:
>>> If the system launches high number of VMs then that means loads of FID
>>> needs to be reserved for SCMI and the hypervisor needs to support it.
>>> Basically it is not scalable which I agree but not sure if such large
>>> number of VMs are used in reality with SCMI. But I agree with the 
>>> technical
>>> reasoning.
>>>
>>> The other reason why I preferred the shmem address itself instead of 
>>> some
>>> custom VM ID/channel number is that it can easily becomes vendor 
>>> specific
>>> for no real good reason and then we need to add support for each such
>>> different parameters. Nikunj suggested getting them from DT which I 
>>> really
>>> don't like if the sole reason is just to identify the channel. We don't
>>> have standard SCMI SMC/HVC but allowing such deviations with params 
>>> from
>>> DT will just explode with various combinations for silly/no reason.
>>>
>> Would you be ok to pass the smc/hvc parameters via kernel parameters 
>> in commandline? If so, I can implement that. I just wanted to keep 
>> everything in one place hence suggested using DTB node.
>
> Command line arguments seem a bit unnecessary here and it would force 
> you to invent a scheme to control per SCMI device/instance parameters.
>
Agreed!
>>
>>> [...]
>>>
>>>>> @@ -137,6 +144,8 @@ static int smc_chan_setup(struct 
>>>>> scmi_chan_info *cinfo, struct device *dev,
>>>>>        if (ret < 0)
>>>>>            return ret;
>>>>> +    if (of_device_is_compatible(dev->of_node, "arm,scmi-smc-param"))
>>>>> +        scmi_info->param = res.start;
>>>> There is not even a check that this is going to be part of the 
>>>> kernel's view
>>>> of memory, that seems a bit brittle and possibly a security hole, 
>>>> too. Your
>>>> hypervisor presumably needs to have carved out some amount of 
>>>> memory in
>>>> order for the messages to be written to/read from, and so would the VM
>>>> kernel, so eventually we should have a 'reserved-memory' entry of 
>>>> some sort,
>>>> no?
>>>>
>>> Not disagreeing here. Just checking if my understanding is correct 
>>> or not.
>>> IIUC, we need reserved-memory if it is part of the memory presented 
>>> to the
>>> OS right ? You don't need that if it is dedicated memory like part 
>>> of SRAM
>>> or something similar.
>> We are not using reserved-memory node. Instead using sram-mmio to 
>> carve out shmem for scmi instances.
>
> OK, that works too.
>
>>>>>        /*
>>>>>         * If there is an interrupt named "a2p", then the service and
>>>>>         * completion of a message is signaled by an interrupt 
>>>>> rather than by
>>>>> @@ -156,6 +165,7 @@ static int smc_chan_setup(struct 
>>>>> scmi_chan_info *cinfo, struct device *dev,
>>>>>        }
>>>>>        scmi_info->func_id = func_id;
>>>>> +    scmi_info->is_smc64 = ARM_SMCCC_IS_64(func_id);
>>>>>        scmi_info->cinfo = cinfo;
>>>>>        smc_channel_lock_init(scmi_info);
>>>>>        cinfo->transport_info = scmi_info;
>>>>> @@ -188,7 +198,20 @@ static int smc_send_message(struct 
>>>>> scmi_chan_info *cinfo,
>>>>>        shmem_tx_prepare(scmi_info->shmem, xfer, cinfo);
>>>>> -    arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, 
>>>>> &res);
>>>>> +#ifdef CONFIG_ARM64
>>>>> +    /*
>>>>> +     * if SMC32 convention is used, pass 64 bit address in
>>>>> +     * two parameters
>>>>> +     */
>>>>> +    if (!scmi_info->is_smc64)
>>>> There is no need for scmi_info to store is_smc64, just check the 
>>>> func_id
>>>> here and declare is_smc64 as a local variable to the function.
>>>>
>>> +1
>> ACK!
>>>> Also, another way to approach this would be to encode the 
>>>> parameters region
>>>> in 4KB units such that event on a 32-bit system with LPAE you are 
>>>> guaranteed
>>>> to fit the region into a 32-bit unsigned long. AFAIR virtualization 
>>>> and LPAE
>>>> are indistinguishable on real CPUs?
>>>>
>>> Agree with the idea. But can a single 4kB be used for multiple shmem 
>>> across
>>> VMs ? IIUC the hypervisor can deal with that, so I prefer it if it 
>>> is possible
>>> practically.
>> We have multiple VMs and each VM has multiple instances. We will have 
>> quite a few domains and I am not sure if single page will suffice.
>
> I did not make myself clear enough, you can encode an offset into the 
> shared memory area, and however big that shared memory area will be, 
> that offset can be in a 4KB size. So for instance if you have your 
> MMIO SRAM at 0x8000_0000, the first VM can use 0x8000_0ffff, the 
> second VM can use 0x8000_1000 to 0x8000_1fff and so on and so forth.
>
> Even if you need more than 4KB per VM, you can put that information 
> into the two additional parameters you pass through the SMC/HVC call.

Okay. I will float another version, first parameter of smc/hvc call will 
be pfn and second the offset!

[PATCH v4 0/2] Allow parameter in smc/hvc calls

[Nikunj Kela]

Currently, smc/hvc calls are made with parameters set
to zeros. We are using multiple scmi instances within
a VM. We are sharing the same smc-id(func_id) with all
scmi instance. The hypervisor needs a way to distinguish
among hvc calls made from different instances.

This patch series introduces new compatible string which
can be used to pass shmem channel address as parameters
to smc/hvc calls.

---
v4 -> split shmem address into 4KB-pages and offset

v3 -> pass shmem channel address as parameter

v2 -> fix the compilation erros on 32bit platform(see below)
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202304100606.kUjhsRYf-lkp@intel.com/

v1 -> original patches

Nikunj Kela (2):
  dt-bindings: firmware: arm,scmi: support for parameter in smc/hvc call
  firmware: arm_scmi: Augment SMC/HVC to allow optional parameters

 .../devicetree/bindings/firmware/arm,scmi.yaml     |  8 +++++++-
 drivers/firmware/arm_scmi/driver.c                 |  1 +
 drivers/firmware/arm_scmi/smc.c                    | 14 +++++++++++++-
 3 files changed, 21 insertions(+), 2 deletions(-)

-- 
2.17.1

[PATCH v4 1/2] dt-bindings: firmware: arm,scmi: support for parameter in smc/hvc call

[Nikunj Kela]

Currently, smc/hvc calls are made with smc-id only. The parameters are
all set to zeros. This patch defines a new compatible string that can
be used to pass shmem address(4KB-page, offset) as two parameters in
SMC/HVC doorbell.

This is useful when multiple scmi instances are used with common smc-id.

Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
---
 Documentation/devicetree/bindings/firmware/arm,scmi.yaml | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
index 5824c43e9893..ad776911f990 100644
--- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
+++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
@@ -34,6 +34,10 @@ properties:
       - description: SCMI compliant firmware with ARM SMC/HVC transport
         items:
           - const: arm,scmi-smc
+      - description: SCMI compliant firmware with ARM SMC/HVC transport
+                     with shmem address(4KB-page, offset) as parameters
+        items:
+          - const: arm,scmi-smc-param
       - description: SCMI compliant firmware with SCMI Virtio transport.
                      The virtio transport only supports a single device.
         items:
@@ -299,7 +303,9 @@ else:
     properties:
       compatible:
         contains:
-          const: arm,scmi-smc
+          enum:
+            - arm,scmi-smc
+            - arm,scmi-smc-param
   then:
     required:
       - arm,smc-id
-- 
2.17.1

Re: [PATCH v4 1/2] dt-bindings: firmware: arm,scmi: support for parameter in smc/hvc call

[Rob Herring]

On Tue, 18 Apr 2023 11:56:58 -0700, Nikunj Kela wrote:
> Currently, smc/hvc calls are made with smc-id only. The parameters are
> all set to zeros. This patch defines a new compatible string that can
> be used to pass shmem address(4KB-page, offset) as two parameters in
> SMC/HVC doorbell.
> 
> This is useful when multiple scmi instances are used with common smc-id.
> 
> Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
> ---
>  Documentation/devicetree/bindings/firmware/arm,scmi.yaml | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

[PATCH v4 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional parameters

[Nikunj Kela]

This patch add support for passing shmem channel address as parameters
in smc/hvc call. The address is split into 4KB-page and offset.
This patch is useful when multiple scmi instances are using same smc-id
and firmware needs to distinguish among the instances.

Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
---
 drivers/firmware/arm_scmi/driver.c |  1 +
 drivers/firmware/arm_scmi/smc.c    | 14 +++++++++++++-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index e7d97b59963b..b5957cc12fee 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -2914,6 +2914,7 @@ static const struct of_device_id scmi_of_match[] = {
 #endif
 #ifdef CONFIG_ARM_SCMI_TRANSPORT_SMC
 	{ .compatible = "arm,scmi-smc", .data = &scmi_smc_desc},
+	{ .compatible = "arm,scmi-smc-param", .data = &scmi_smc_desc},
 #endif
 #ifdef CONFIG_ARM_SCMI_TRANSPORT_VIRTIO
 	{ .compatible = "arm,scmi-virtio", .data = &scmi_virtio_desc},
diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
index 93272e4bbd12..71e080b70df5 100644
--- a/drivers/firmware/arm_scmi/smc.c
+++ b/drivers/firmware/arm_scmi/smc.c
@@ -20,6 +20,11 @@
 
 #include "common.h"
 
+#define SHMEM_SHIFT 12
+#define SHMEM_SIZE (_AC(1, UL) << SHMEM_SHIFT)
+#define SHMEM_PAGE(x) ((unsigned long)((x) >> SHMEM_SHIFT))
+#define SHMEM_OFFSET(x) ((x) & (SHMEM_SIZE - 1))
+
 /**
  * struct scmi_smc - Structure representing a SCMI smc transport
  *
@@ -30,6 +35,7 @@
  * @inflight: Atomic flag to protect access to Tx/Rx shared memory area.
  *	      Used when operating in atomic mode.
  * @func_id: smc/hvc call function id
+ * @param: physical address of the shmem channel
  */
 
 struct scmi_smc {
@@ -40,6 +46,7 @@ struct scmi_smc {
 #define INFLIGHT_NONE	MSG_TOKEN_MAX
 	atomic_t inflight;
 	u32 func_id;
+	phys_addr_t param;
 };
 
 static irqreturn_t smc_msg_done_isr(int irq, void *data)
@@ -137,6 +144,8 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
 	if (ret < 0)
 		return ret;
 
+	if (of_device_is_compatible(dev->of_node, "arm,scmi-smc-param"))
+		scmi_info->param = res.start;
 	/*
 	 * If there is an interrupt named "a2p", then the service and
 	 * completion of a message is signaled by an interrupt rather than by
@@ -179,6 +188,8 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
 {
 	struct scmi_smc *scmi_info = cinfo->transport_info;
 	struct arm_smccc_res res;
+	unsigned long page = SHMEM_PAGE(scmi_info->param);
+	unsigned long offset = SHMEM_OFFSET(scmi_info->param);
 
 	/*
 	 * Channel will be released only once response has been
@@ -188,7 +199,8 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
 
 	shmem_tx_prepare(scmi_info->shmem, xfer, cinfo);
 
-	arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
+	arm_smccc_1_1_invoke(scmi_info->func_id, page, offset, 0, 0, 0, 0, 0,
+			     &res);
 
 	/* Only SMCCC_RET_NOT_SUPPORTED is valid error code */
 	if (res.a0) {
-- 
2.17.1

Re: [PATCH v4 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional parameters

[Nikunj Kela]

Reminder: Please review this patch! Thanks

On 4/18/2023 11:56 AM, Nikunj Kela wrote:
> This patch add support for passing shmem channel address as parameters
> in smc/hvc call. The address is split into 4KB-page and offset.
> This patch is useful when multiple scmi instances are using same smc-id
> and firmware needs to distinguish among the instances.
>
> Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
> ---
>   drivers/firmware/arm_scmi/driver.c |  1 +
>   drivers/firmware/arm_scmi/smc.c    | 14 +++++++++++++-
>   2 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index e7d97b59963b..b5957cc12fee 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -2914,6 +2914,7 @@ static const struct of_device_id scmi_of_match[] = {
>   #endif
>   #ifdef CONFIG_ARM_SCMI_TRANSPORT_SMC
>   	{ .compatible = "arm,scmi-smc", .data = &scmi_smc_desc},
> +	{ .compatible = "arm,scmi-smc-param", .data = &scmi_smc_desc},
>   #endif
>   #ifdef CONFIG_ARM_SCMI_TRANSPORT_VIRTIO
>   	{ .compatible = "arm,scmi-virtio", .data = &scmi_virtio_desc},
> diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
> index 93272e4bbd12..71e080b70df5 100644
> --- a/drivers/firmware/arm_scmi/smc.c
> +++ b/drivers/firmware/arm_scmi/smc.c
> @@ -20,6 +20,11 @@
>   
>   #include "common.h"
>   
> +#define SHMEM_SHIFT 12
> +#define SHMEM_SIZE (_AC(1, UL) << SHMEM_SHIFT)
> +#define SHMEM_PAGE(x) ((unsigned long)((x) >> SHMEM_SHIFT))
> +#define SHMEM_OFFSET(x) ((x) & (SHMEM_SIZE - 1))
> +
>   /**
>    * struct scmi_smc - Structure representing a SCMI smc transport
>    *
> @@ -30,6 +35,7 @@
>    * @inflight: Atomic flag to protect access to Tx/Rx shared memory area.
>    *	      Used when operating in atomic mode.
>    * @func_id: smc/hvc call function id
> + * @param: physical address of the shmem channel
>    */
>   
>   struct scmi_smc {
> @@ -40,6 +46,7 @@ struct scmi_smc {
>   #define INFLIGHT_NONE	MSG_TOKEN_MAX
>   	atomic_t inflight;
>   	u32 func_id;
> +	phys_addr_t param;
>   };
>   
>   static irqreturn_t smc_msg_done_isr(int irq, void *data)
> @@ -137,6 +144,8 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
>   	if (ret < 0)
>   		return ret;
>   
> +	if (of_device_is_compatible(dev->of_node, "arm,scmi-smc-param"))
> +		scmi_info->param = res.start;
>   	/*
>   	 * If there is an interrupt named "a2p", then the service and
>   	 * completion of a message is signaled by an interrupt rather than by
> @@ -179,6 +188,8 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
>   {
>   	struct scmi_smc *scmi_info = cinfo->transport_info;
>   	struct arm_smccc_res res;
> +	unsigned long page = SHMEM_PAGE(scmi_info->param);
> +	unsigned long offset = SHMEM_OFFSET(scmi_info->param);
>   
>   	/*
>   	 * Channel will be released only once response has been
> @@ -188,7 +199,8 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
>   
>   	shmem_tx_prepare(scmi_info->shmem, xfer, cinfo);
>   
> -	arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
> +	arm_smccc_1_1_invoke(scmi_info->func_id, page, offset, 0, 0, 0, 0, 0,
> +			     &res);
>   
>   	/* Only SMCCC_RET_NOT_SUPPORTED is valid error code */
>   	if (res.a0) {

Re: [PATCH v4 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional parameters

[Sudeep Holla]

On Mon, May 01, 2023 at 07:39:29AM -0700, Nikunj Kela wrote:
> Reminder: Please review this patch! Thanks
>

Since the current merge window is open, there is no rush and hence I had put
this on hold until merge window close. Anyways, it looks good in general.
Couple of minor nits below:

> On 4/18/2023 11:56 AM, Nikunj Kela wrote:
> > This patch add support for passing shmem channel address as parameters
> > in smc/hvc call. The address is split into 4KB-page and offset.
> > This patch is useful when multiple scmi instances are using same smc-id
> > and firmware needs to distinguish among the instances.
> >

Drop the term "patch". You can refer it as change. It is not match after
it is applied to the git.

> > Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
> > ---
> >   drivers/firmware/arm_scmi/driver.c |  1 +
> >   drivers/firmware/arm_scmi/smc.c    | 14 +++++++++++++-
> >   2 files changed, 14 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> > index e7d97b59963b..b5957cc12fee 100644
> > --- a/drivers/firmware/arm_scmi/driver.c
> > +++ b/drivers/firmware/arm_scmi/driver.c
> > @@ -2914,6 +2914,7 @@ static const struct of_device_id scmi_of_match[] = {
> >   #endif
> >   #ifdef CONFIG_ARM_SCMI_TRANSPORT_SMC
> >   	{ .compatible = "arm,scmi-smc", .data = &scmi_smc_desc},
> > +	{ .compatible = "arm,scmi-smc-param", .data = &scmi_smc_desc},
> >   #endif
> >   #ifdef CONFIG_ARM_SCMI_TRANSPORT_VIRTIO
> >   	{ .compatible = "arm,scmi-virtio", .data = &scmi_virtio_desc},
> > diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
> > index 93272e4bbd12..71e080b70df5 100644
> > --- a/drivers/firmware/arm_scmi/smc.c
> > +++ b/drivers/firmware/arm_scmi/smc.c
> > @@ -20,6 +20,11 @@
> >   #include "common.h"
> > +#define SHMEM_SHIFT 12
> > +#define SHMEM_SIZE (_AC(1, UL) << SHMEM_SHIFT)
> > +#define SHMEM_PAGE(x) ((unsigned long)((x) >> SHMEM_SHIFT))

Since we are dealing with 4kB pages only, I prefer to do:
#define SHMEM_SIZE     (SZ_4K)
#define SHMEM_SHIFT    12
#define SHMEM_PAGE(x) (_UL((x) >> SHMEM_SHIFT))

> > +#define SHMEM_OFFSET(x) ((x) & (SHMEM_SIZE - 1))
> > +

Also it is definitely worth adding comment about supporting just 4kB pages
and limitations associated with it here(e.g. we can support up to 44 bit
address) and also parameters to the SMC call so that others get clear
idea on how to use it if they need that in the future.

> >   /**
> >    * struct scmi_smc - Structure representing a SCMI smc transport
> >    *
> > @@ -30,6 +35,7 @@
> >    * @inflight: Atomic flag to protect access to Tx/Rx shared memory area.
> >    *	      Used when operating in atomic mode.
> >    * @func_id: smc/hvc call function id
> > + * @param: physical address of the shmem channel
> >    */
> >   struct scmi_smc {
> > @@ -40,6 +46,7 @@ struct scmi_smc {
> >   #define INFLIGHT_NONE	MSG_TOKEN_MAX
> >   	atomic_t inflight;
> >   	u32 func_id;
> > +	phys_addr_t param;
> >   };
> >   static irqreturn_t smc_msg_done_isr(int irq, void *data)
> > @@ -137,6 +144,8 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
> >   	if (ret < 0)
> >   		return ret;
> > +	if (of_device_is_compatible(dev->of_node, "arm,scmi-smc-param"))
> > +		scmi_info->param = res.start;
> >   	/*
> >   	 * If there is an interrupt named "a2p", then the service and
> >   	 * completion of a message is signaled by an interrupt rather than by
> > @@ -179,6 +188,8 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
> >   {
> >   	struct scmi_smc *scmi_info = cinfo->transport_info;
> >   	struct arm_smccc_res res;
> > +	unsigned long page = SHMEM_PAGE(scmi_info->param);
> > +	unsigned long offset = SHMEM_OFFSET(scmi_info->param);

While I see you initialise param in smc_chan_setup, I wonder why the page
and offset itself be initialised once and reused instead of computing the
same fixed value on every smc_send_message. You can probably have param_page
and param_offset stashed instead of just single param value ?

> >   	/*
> >   	 * Channel will be released only once response has been
> > @@ -188,7 +199,8 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
> >   	shmem_tx_prepare(scmi_info->shmem, xfer, cinfo);
> > -	arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
> > +	arm_smccc_1_1_invoke(scmi_info->func_id, page, offset, 0, 0, 0, 0, 0,
> > +			     &res);
> >   	/* Only SMCCC_RET_NOT_SUPPORTED is valid error code */
> >   	if (res.a0) {

-- 
Regards,
Sudeep

Re: [PATCH v4 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional parameters

[Nikunj Kela]

On 5/2/2023 3:46 AM, Sudeep Holla wrote:
> On Mon, May 01, 2023 at 07:39:29AM -0700, Nikunj Kela wrote:
>> Reminder: Please review this patch! Thanks
>>
> Since the current merge window is open, there is no rush and hence I had put
> this on hold until merge window close. Anyways, it looks good in general.
> Couple of minor nits below:
Sure, thanks!
>> On 4/18/2023 11:56 AM, Nikunj Kela wrote:
>>> This patch add support for passing shmem channel address as parameters
>>> in smc/hvc call. The address is split into 4KB-page and offset.
>>> This patch is useful when multiple scmi instances are using same smc-id
>>> and firmware needs to distinguish among the instances.
>>>
> Drop the term "patch". You can refer it as change. It is not match after
> it is applied to the git.
ACK!
>>> Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
>>> ---
>>>    drivers/firmware/arm_scmi/driver.c |  1 +
>>>    drivers/firmware/arm_scmi/smc.c    | 14 +++++++++++++-
>>>    2 files changed, 14 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
>>> index e7d97b59963b..b5957cc12fee 100644
>>> --- a/drivers/firmware/arm_scmi/driver.c
>>> +++ b/drivers/firmware/arm_scmi/driver.c
>>> @@ -2914,6 +2914,7 @@ static const struct of_device_id scmi_of_match[] = {
>>>    #endif
>>>    #ifdef CONFIG_ARM_SCMI_TRANSPORT_SMC
>>>    	{ .compatible = "arm,scmi-smc", .data = &scmi_smc_desc},
>>> +	{ .compatible = "arm,scmi-smc-param", .data = &scmi_smc_desc},
>>>    #endif
>>>    #ifdef CONFIG_ARM_SCMI_TRANSPORT_VIRTIO
>>>    	{ .compatible = "arm,scmi-virtio", .data = &scmi_virtio_desc},
>>> diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
>>> index 93272e4bbd12..71e080b70df5 100644
>>> --- a/drivers/firmware/arm_scmi/smc.c
>>> +++ b/drivers/firmware/arm_scmi/smc.c
>>> @@ -20,6 +20,11 @@
>>>    #include "common.h"
>>> +#define SHMEM_SHIFT 12
>>> +#define SHMEM_SIZE (_AC(1, UL) << SHMEM_SHIFT)
>>> +#define SHMEM_PAGE(x) ((unsigned long)((x) >> SHMEM_SHIFT))
> Since we are dealing with 4kB pages only, I prefer to do:
> #define SHMEM_SIZE     (SZ_4K)
> #define SHMEM_SHIFT    12
> #define SHMEM_PAGE(x) (_UL((x) >> SHMEM_SHIFT))
ACK!
>>> +#define SHMEM_OFFSET(x) ((x) & (SHMEM_SIZE - 1))
>>> +
> Also it is definitely worth adding comment about supporting just 4kB pages
> and limitations associated with it here(e.g. we can support up to 44 bit
> address) and also parameters to the SMC call so that others get clear
> idea on how to use it if they need that in the future.
ACK!
>>>    /**
>>>     * struct scmi_smc - Structure representing a SCMI smc transport
>>>     *
>>> @@ -30,6 +35,7 @@
>>>     * @inflight: Atomic flag to protect access to Tx/Rx shared memory area.
>>>     *	      Used when operating in atomic mode.
>>>     * @func_id: smc/hvc call function id
>>> + * @param: physical address of the shmem channel
>>>     */
>>>    struct scmi_smc {
>>> @@ -40,6 +46,7 @@ struct scmi_smc {
>>>    #define INFLIGHT_NONE	MSG_TOKEN_MAX
>>>    	atomic_t inflight;
>>>    	u32 func_id;
>>> +	phys_addr_t param;
>>>    };
>>>    static irqreturn_t smc_msg_done_isr(int irq, void *data)
>>> @@ -137,6 +144,8 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
>>>    	if (ret < 0)
>>>    		return ret;
>>> +	if (of_device_is_compatible(dev->of_node, "arm,scmi-smc-param"))
>>> +		scmi_info->param = res.start;
>>>    	/*
>>>    	 * If there is an interrupt named "a2p", then the service and
>>>    	 * completion of a message is signaled by an interrupt rather than by
>>> @@ -179,6 +188,8 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
>>>    {
>>>    	struct scmi_smc *scmi_info = cinfo->transport_info;
>>>    	struct arm_smccc_res res;
>>> +	unsigned long page = SHMEM_PAGE(scmi_info->param);
>>> +	unsigned long offset = SHMEM_OFFSET(scmi_info->param);
> While I see you initialise param in smc_chan_setup, I wonder why the page
> and offset itself be initialised once and reused instead of computing the
> same fixed value on every smc_send_message. You can probably have param_page
> and param_offset stashed instead of just single param value ?
Yeah, I did think of that but then I dropped it since in the earlier 
versions of patches when I was using a flag to identify smc32/smc64 
convention used, I was told to not include it in the scmi_info struct, 
instead compute using local variable. Anyway, I will use the two values 
as advised!
>>>    	/*
>>>    	 * Channel will be released only once response has been
>>> @@ -188,7 +199,8 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
>>>    	shmem_tx_prepare(scmi_info->shmem, xfer, cinfo);
>>> -	arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
>>> +	arm_smccc_1_1_invoke(scmi_info->func_id, page, offset, 0, 0, 0, 0, 0,
>>> +			     &res);
>>>    	/* Only SMCCC_RET_NOT_SUPPORTED is valid error code */
>>>    	if (res.a0) {

Re: [PATCH v4 0/2] Allow parameter in smc/hvc calls

[Nikunj Kela]

Hi Sudeep, could you please review v4 patches. Thanks!


On 4/18/2023 11:56 AM, Nikunj Kela wrote:
> Currently, smc/hvc calls are made with parameters set
> to zeros. We are using multiple scmi instances within
> a VM. We are sharing the same smc-id(func_id) with all
> scmi instance. The hypervisor needs a way to distinguish
> among hvc calls made from different instances.
>
> This patch series introduces new compatible string which
> can be used to pass shmem channel address as parameters
> to smc/hvc calls.
>
> ---
> v4 -> split shmem address into 4KB-pages and offset
>
> v3 -> pass shmem channel address as parameter
>
> v2 -> fix the compilation erros on 32bit platform(see below)
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202304100606.kUjhsRYf-lkp@intel.com/
>
> v1 -> original patches
>
> Nikunj Kela (2):
>    dt-bindings: firmware: arm,scmi: support for parameter in smc/hvc call
>    firmware: arm_scmi: Augment SMC/HVC to allow optional parameters
>
>   .../devicetree/bindings/firmware/arm,scmi.yaml     |  8 +++++++-
>   drivers/firmware/arm_scmi/driver.c                 |  1 +
>   drivers/firmware/arm_scmi/smc.c                    | 14 +++++++++++++-
>   3 files changed, 21 insertions(+), 2 deletions(-)
>