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

[Nikunj Kela <quic_nkela@quicinc.com>]

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