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

[Nikunj Kela]

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!