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
