mbox series

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

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

Message

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

Comments

Nikunj Kela April 25, 2023, 2:01 p.m. UTC | #1 
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(-)
>
Nikunj Kela May 1, 2023, 2:39 p.m. UTC | #2 
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) {
Sudeep Holla May 2, 2023, 10:46 a.m. UTC | #3 
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) {
Nikunj Kela May 2, 2023, 2:41 p.m. UTC | #4 
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) {