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