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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 4/18/2023 9:33 AM, Florian Fainelli wrote:
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.

Agreed!

[...]

@@ -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.

Okay. I will float another version, first parameter of smc/hvc call will be pfn and second the offset!




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux