Message-ID: <40e91346-19d7-c0b4-7fcc-d5a2081fe8dd@gmail.com>
Date:   Tue, 18 Apr 2023 09:33:06 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Nikunj Kela <quic_nkela@...cinc.com>,
        Sudeep Holla <sudeep.holla@....com>
Cc:     cristian.marussi@....com, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org,
        linux-arm-kernel@...ts.infradead.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/2] firmware: arm_scmi: Augment SMC/HVC to allow
 optional parameter

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

Powered by blists - more mailing lists