Re: [RFC PATCH 0/3] firmware: Add support for PSA FF-A interface

Jens Wiklander
2020-06-15T11:38:50Z

Hi,

On Tue, Jun 09, 2020 at 06:41:23PM +0100, Sudeep Holla wrote:
> (Sorry for the delay, got distracted with some other bug fix.)
>
> On Thu, Jun 04, 2020 at 02:37:46PM +0100, Will Deacon wrote:
> > Hi Sudeep, [+Fuad, Andrew and Ard]
> >
> > (To other interested readers: if you haven't seen it, the FF-A spec is here:
> > https://static.docs.arm.com/den0077/a/DEN0077A_PSA_Firmware_Framework_Arm_v8-A_1.0_EAC.pdf
> > since this discussion makes no sense without that, and a tiny bit of sense
> > with it. It used to be called "SPCI" but it was recently renamed.)
> >
>
> Thanks for adding all interested parties.
>
> > On Mon, Jun 01, 2020 at 10:45:09AM +0100, Sudeep Holla wrote:
> > > Sorry for posting in the middle of merge window and I must have done
> > > this last week itself. This is not the driver I had thought about posting
> > > last week. After I started cleaning up and looking at Will's KVM prototype[1]
> > > for PSA FF-A (previously known as SPCI),
> >
> > Yes, I need to do the Big Rename at some point. Joy.
> >
>
> 😁
>
> > > I got more doubts on alignment and dropped huge chunk of interface APIs in
> > > the driver in order to keep it simple, and get aligned more with that
> > > prototype and avoid scanning lots of code unnecessary.
> >
> > You also dropped most of the code, so this doesn't really do anything in
> > its current form ;)
> >
>
> Yes, it was intentional 😉
>
> > > Here are few things to clarify:
> > >
> > > 1. DT bindings
> > > ---------------
> > > I was initially against adding bindings for Tx/Rx buffers for
> > > partitions. As per the spec, an endpoint could allocate the
> > > buffer pair and use the FFA_RXTX_MAP interface to map it with the
> > > Hypervisor(KVM here). However looking at the prototype and also
> > > I remember you mentioning that it is not possible to manage buffers
> > > in that way. Please confirm if you plan to add the buffer details
> > > fetcthing them through ioctls in KVM and adding them to VM DT nodes
> > > in KVM userspace. I will update the bindings accordingly.
> >
> > I think it's useful to have a mode of operation where the hypervisor
> > allocates the RX/TX buffers and advertises them in the DT. However, we
> > can always add this later, so there's no need to have it in the binding
> > from the start. Best start as simple as possible, I reckon.
> >
>
> OK
>
> > Setting the static RX/TX buffer allocation aside, why is a DT node needed
> > at all for the case where Linux is running purely as an FF-A client? I
> > thought everything should be discoverable via FFA_VERSION, FFA_FEATURES,
> > FFA_PARTITION_INFO_GET and FFA_ID_GET? That should mean we can get away
> > without a binding at all for the client case.
> >
>
> Agreed, I added for RxTx buffers and initially to build the parent/child
> hierarchy for all users of the driver. Initially I was assuming only
> in-kernel users and now I agree we should avoid any in kernel users if
> possible.
>
> One thing to note FFA_PARTITION_INFO_GET relies on Rx buffers to send the
> information to the caller. So we need to have established buffers before
> that and one of the reason you don't find that in this RFC. I dropped that
> too which I wanted initially.
>
> > > 2. Driver
> > > ---------
> > > a. Support for multiple partitions in a VM
> > > ------------------------------------------
> > > I am not sure if there is need for supporting multiple partitions
> > > within a VM. It should be possible to do so as I expect to create
> > > device for each partition entry under arm-psa-ffa devicetree node.
> > > However, I don't want to assume something that will never be a
> > > usecase. However I don't think this will change must of the
> > > abstraction as we need to keep the interface API implementation
> > > separate to support different partitions on various platforms.
> >
> > I think Ard has a case for something like this, where a VM actually consists
> > of multiple partitions so that S-EL0 services can be provided from NS-EL0.
> > However, he probably wants that for a dynamically created VM, so we'd
> > need a way to instantiate an FFA namespace for the VM. Maybe that can be
> > done entirely in userspace by the VMM...
> >
>
> Interesting...
>
> > > b. SMCCC interface
> > > ------------------
> > > This is something I messed up completely while trying to add
> > > support for SMCCC v1.2. It now supports x0-x17 as parameter
> > > registers(input) and return registers(output). I started simple
> > > with x0-x7 as both input and output as PSA FF-A needs that at
> > > most. But extending to x0-x17 then became with messy in my
> > > implementation. That's the reason I dropped it completely
> > > here and thought of checking it first.
> > >
> > > Do we need to extend the optimisations that were done to handle
> > > ARCH_WORKAROUND_{1,2}. Or should be just use a version with x0-x7
> > > as both input and ouput. Hyper-V guys need full x0-x17 support.
> > >
> > > I need some guidance as what is the approach preferred ?
> >
> > I think we can start off with x0-x7 and extend if later if we need to.
> >
>
> Sure
>
> > > 3. Partitions
> > > -------------
> > > I am not sure if we have a full define partition that we plan to
> > > push upstream. Without one, we can have a sample/example partition
> > > to test all the interface APIs, but is that fine with respect to
> > > what we want upstream ? Any other thoughts that helps to test the
> > > driver ?
> >
> > I think that's the best you can do for now. We can probably help with
> > testing as our stuff gets off the ground.
> >
>
> OK
>
> > > Sorry for long email and too many questions, but I thought it is easier
> > > this way to begin with than throwing huge code implementing loads of APIs
> > > with no users(expect example partition) especially that I am posting this
> > > during merge window.
> >
> > No problem. Maybe it would help if I described roughly what we were thinking
> > of doing for KVM (this is open for discussion, of course):
> >
> > 1. Describe KVM-managed partitions in the DT, along the lines of [1]
> > 2. Expose each partition as a file to userspace. E.g.:
> >
> > /dev/spci/:
> >
> > self
> > e3a48fa5-dc54-4a8b-898b-bdc4dfeeb7b8
> > 49f65057-d002-4ae2-b4ee-d31c7940a13d
> >
> > Here, self would be a symlink to the host uuid. The host uuid file
> > would implement FFA_MEM operations using an ioctl(), so you could,
> > for example, share a user buffer with multiple partitions by issuing
> > a MEM_SHARE ioctl() on self, passing the fds for the borrower partitions
> > as arguments. Messaging would be implemented as ioctl()s on the
> > partition uuid files themselves.
> >
>
> OK, IIUC that covers mostly KVM implementation. We still need a way to
> share the RxTx buffer info to the partitions and DT/ACPI(?) is one
> possible way. Based on you comment about not needing DT node, do you have
> any other way to communicate the buffer info to the partitions ?
>
> > 3. We'll need some (all?) of these patches to unmap memory from the host
> > when necessary:
> >
> > https://lwn.net/Articles/821215/
> >
> > (for nVHE, we'll have a stage-2 for the host so we can unmap there as
> > well)
> >
>
> Sounds more fun.
>
> > For communicating with partitions that are not managed by KVM (e.g. trusted
> > applications), it's not clear to me how much of that will be handled in
> > kernel or user. I think it would still be worth exposing the partitions as
> > files, but perhaps having them root only or just returning -EPERM for the
> > ioctl() if a kernel driver has claimed the partition as its own? Ideally,
> > FF-A would allow us to transition some of the Trusted OS interfacing code
> > out to userspace, but I don't know how realistic that is.
> >
>
> Ah good, so we can still manage in-kernel users this way but we need to
> provide interface to such a driver which I agree that we need to avoid
> if possible.

The OP-TEE driver is an in-kernel user, I don't see that we can migrate
that to user space in the nearest future. In fact I'm not sure it would
make sense since we have a kernel internal interface which is used by
some drivers.

Cheers,
Jens