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

[Jens Wiklander <jens.wiklander@linaro.org>]

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