Messages in this thread | | | Subject | Re: [PATCH] firmware: arm_scmi: Pass shmem address to SMCCC call | From | Daniele Alessandrelli <> | Date | Fri, 17 Jul 2020 15:59:10 +0100 |
| |
On Fri, 2020-07-17 at 11:31 +0100, Sudeep Holla wrote: > On Thu, Jul 16, 2020 at 12:57:23PM -0700, Florian Fainelli wrote: > > > > On 7/16/2020 7:13 AM, Daniele Alessandrelli wrote: > > > Hi Florian, > > > > > > Thanks for you feedback. > > > > > > On Wed, 2020-07-15 at 15:43 -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? > > > > > > I see your point. After a quick look, I think that, practically, > > > the > > > issue is with ARM32 LPAE addresses, for which phys_addr_t is a > > > u64. So, > > > basically, for AArch32 systems with LPAE the 64-bit shmem_paddr > > > gets > > > truncated to 32-bit when it's passed to the SMC32/HVC32 call. > > > > > > To solve that, I would prefer splitting the address between two > > > SMC > > > parameters (a1 = addr_lo, a2 = addr_hi), instead of imposing an > > > arbitrary alignment. Would that be reasonable? > > > > The low/high part would only be relevant on a 32-bit LPAE platform > > which > > is probably a corner case, I would just pass the shmem_paddr / 4096 > > since that is the smallest granule size and alignment possible and > > it > > still allows you to map up to 36-bits of physical address, which is > > the > > maximum that the long descriptor in LPAE can support. For 64-bit we > > have > > no such problems since we have the full register width. > > > > OK, I will check if 32-bit identifier can be relaxed in the spec in > which > case we can avoid having DT binding for the identifier. If that is > possible > we could use addr/4k page size. > > > > > 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? > > > > > > The return code is actually checked at the end of the function: > > > https://elixir.bootlin.com/linux/v5.8-rc4/source/drivers/firmware/arm_scmi/smc.c#L118 > > > > > > But in the meantime scmi_rx_callback() has already been called. > > > Not > > > sure if that's intentional or a possible bug. > > > > > > > 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 share same opinion as Florian here. > > [...] > > > > > 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, that's a possible solution, but it looks more complicated to > > > me, > > > since it adds an additional component (the boot loader) to the > > > equation, while the goal of this patch was to reduce the coupling > > > between components (namely the DT/kernel and the trusted > > > firmware). > > > > > > I guess my question is: if we fix the handling of LPAE addresses > > > and > > > the SMC return code, what is the drawback of having the shmem > > > address > > > passed to the SMC? > > > > My only concern is that if somehow Linux gets assigned a shared > > memory > > range that is completely outside of what the trusted firmware has > > already mapped, or is capable of addressing, or any combination > > thereof, > > it could be challenging to debug what is going on, especially if > > INVALID > > PARAMETER must not be returned (assuming this is to avoid Linux > > discovering where other shared memory areas pertaining to the > > firmware > > reside?). > > > > Valid point. Again, I was planning to use this as identifier and > didn't > think of the usecase Daniele mentions here. > > > The other concern I have is that we are not documenting the various > > SMCCC calling conventions, soon enough it will be come out of > > control, > > and we are already allowing people to define their own function IDs > > and > > parameters to call into the trusted firmware. This sounds like > > something > > that is so basic that it should be standardized from the top, by > > ARM. > > > > Completely agreed. I have failed to achieve that myself. Unless there > is > demand from partners/vendors, it will be dismissed by architects. All > I managed to get in was the identifier as the argument for SMC/HVC to > support multiple channels. However 32-bit id will be issue to be used > with 64-bit address. I will see if we can drop that 32-bit > requirement > from the spec.
It seems that there is some demand already, but perhaps it needs to be requested in a more official way? What is the recommended way to do so?
> > > > Anyway, I should have mentioned this in the commit message (sorry > > > for > > > not doing so), but I submitted this patch because initial > > > feedback from > > > Sudeep was positive [1]; but if there is no consensus around it > > > I'm > > > fine with dropping it. > > > > > > [1] https://lore.kernel.org/lkml/20200710075931.GB1189@bogus/ > > > > My review is by no means authoritative however in deploying SCMI on > > our > > Broadcom STB platforms some experience was gained in the process > > which > > is how it piqued my interest. Thanks for providing more background > > to > > this patch, this does help. > > > > Your knowledge is important and please always share the same and > happy > to take all your review into consideration always. End goal is to > help > all or most of the users of the driver if possible. > > > We have opted for a solution where the boot loader knows about all > > possible reserved regions prior to booting/loading the trusted > > firmware > > as well as the kernel, therefore it can pass that information to > > both > > and we never really had a situation where the two need to evolve in > > an > > uncoordinated way. > > I agree and uncoordinated evolution may not be as simple as it is > presented by Daniele earlier and I am not confident to support that. > The main reason for giving positive response for this initially is > for the addition of identifier which I always thought is required for > supporting multiple channels. E.g. DVFS/Perf has dedicated channel.
I can see now that in the previous discussion there was some misunderstanding on the intended use case. That's fine, I will drop the patch for now, since hard-coding the shmem address in our trusted firmware is fine with us (and so we don't really need this patch).
> > -- > Regards, > Sudeep
| |