Re: [PATCH] firmware: arm_scmi: Pass shmem address to SMCCC call

[Sudeep Holla <sudeep.holla@arm.com>]

On Wed, Jul 15, 2020 at 03:43:24PM -0700, Florian Fainelli wrote:
>
>
> On 7/15/2020 9:55 AM, Daniele Alessandrelli wrote:
> > From: Daniele Alessandrelli <daniele.alessandrelli@intel.com>
> >
> > Currently, when SMC/HVC is used as transport, the base address of the
> > shared memory used for communication is not passed to the SMCCC call.
> > This means that such an address must be hard-coded into the bootloader.
> >
> > In order to increase flexibility and allow the memory layout to be
> > changed without modifying the bootloader, this patch adds the shared
> > memory base address to the a1 argument of the SMCCC call.
> >
> > On the Secure Monitor side, the service call implementation can
> > therefore read the a1 argument in order to know the location of the
> > shared memory to use. This change is backward compatible to existing
> > service call implementations as long as they don't check for a1 to be
> > zero.
>
> resource_size_t being defined after phys_addr_t, its size is different
> between 32-bit, 32-bit with PAE and 64-bit so it would probably make
> more sense to define an physical address alignment, or maybe an address
> that is in multiple of 4KBytes so you can address up to 36-bits of
> physical address even on a 32-bit only system?
>

Good point, I had forgotten about LPAE. Thanks for pointing it out.

> What discovery mechanism does the OS have that the specified address
> within the SMCCC call has been accepted by the firmware given the return
> value of that SMCCC call does not appear to be used or checked? Do we
> just expect a timeout initializing the SCMI subsystem?
>

Agreed, we need to add the check for proper return value then and
definitely document it very clearly as we are trying to standardise
a call to vendor SiP FID space of SMCCC.

> Given that the kernel must somehow reserve this memory as a shared
> memory area for obvious reasons, and the trusted firmware must also
> ensure it treats this memory region with specific permissions in its
> translation regime, does it really make sense to give that much flexibility?
>

I expect so and this comes as shmem property from DT already. We are
just passing the value obtained from there as is. This is just to help
TFA or the firmware to identify the specific channel/shmem as SMC/HVC
otherwise has no way to do so.

> If your boot loader has FDT patching capability, maybe it can also do a
> SMC call to provide the address to your trusted firmware, prior to
> loading the Linux kernel, and then they both agree, prior to boot about
> the shared memory address?
>

Yes, but we definitely can't rely on such mechanism in the kernel. It is
more a platform choice as they run different bootloaders.

--
Regards,
Sudeep