mbox series

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

Message ID 20230410182058.8949-1-quic_nkela@quicinc.com
Headers
Series Allow parameter in smc/hvc calls |

Message

Nikunj Kela April 10, 2023, 6:20 p.m. UTC 
  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(-)

Comments

Sudeep Holla April 11, 2023, 1:01 p.m. UTC | #1 
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 ?
Nikunj Kela April 11, 2023, 2:42 p.m. UTC | #2 
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.
>
Sudeep Holla April 12, 2023, 8:37 a.m. UTC | #3 
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.
Nikunj Kela April 12, 2023, 2:54 p.m. UTC | #4 
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!