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

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

[Sudeep Holla]

Hi All,

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), 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.

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.

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.

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 ?

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 ?

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.

Sudeep Holla (3):
  dt-bindings: Add ARM PSA FF binding for non-secure VM partitions
  firmware: Add support for PSA FF-A transport for VM partitions
  firmware: Add example PSA FF-A non-secure VM partition

 .../devicetree/bindings/arm/arm,psa-ffa.txt   |  47 ++++
 drivers/firmware/Kconfig                      |   1 +
 drivers/firmware/Makefile                     |   1 +
 drivers/firmware/arm_psa_ffa/Kconfig          |  22 ++
 drivers/firmware/arm_psa_ffa/Makefile         |   3 +
 drivers/firmware/arm_psa_ffa/driver.c         | 250 ++++++++++++++++++
 drivers/firmware/arm_psa_ffa/partition.c      |  71 +++++
 include/linux/arm_psa_ffa.h                   |  42 +++
 8 files changed, 437 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/arm,psa-ffa.txt
 create mode 100644 drivers/firmware/arm_psa_ffa/Kconfig
 create mode 100644 drivers/firmware/arm_psa_ffa/Makefile
 create mode 100644 drivers/firmware/arm_psa_ffa/driver.c
 create mode 100644 drivers/firmware/arm_psa_ffa/partition.c
 create mode 100644 include/linux/arm_psa_ffa.h

-- 
2.17.1

[RFC PATCH 1/3] dt-bindings: Add ARM PSA FF binding for non-secure VM partitions

[Sudeep Holla]

Add devicetree bindings for a Arm PSA FF-A compliant non-secure partition
at virtual interface(VMs).

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 .../devicetree/bindings/arm/arm,psa-ffa.txt   | 47 +++++++++++++++++++
 1 file changed, 47 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/arm,psa-ffa.txt

diff --git a/Documentation/devicetree/bindings/arm/arm,psa-ffa.txt b/Documentation/devicetree/bindings/arm/arm,psa-ffa.txt
new file mode 100644
index 000000000000..ee543fb5b397
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/arm,psa-ffa.txt
@@ -0,0 +1,47 @@
+Arm Platform Security Architecture(PSA) Firmware Framework(FF) for Armv8-A
+--------------------------------------------------------------------------
+
+This binding is intended to define the interface the firmware framework
+implementing the Non-secure partitions/endpoints(mostly VMs) as described
+in ARM document ARM DEN 0077A ("Arm Platform Security Architecture
+Firmware Framework for Arm v8-A") [0]
+
+In the case of a Non-secure virtual FF-A instance, the hypervisor
+(e.g. Linux KVM) can use this binding to create and launch VM partitions.
+
+The SMCCC conduit available in the VM partition itself is used and hence
+there is no explicit binding to specify the conduit used for PSA FFA
+interface.
+
+Required properties:
+
+- compatible : Should be one of:
+	       "arm,psa-ffa"
+
+- One or more child nodes, each describing an PSA FFA partition using the
+  following required properties:
+
+  - compatible: Should be one of:
+		"arm,psa-ffa-partition"
+
+  - uuid : The 128-bit UUID [2] of the service implemented by this partition,
+	   represented as a string.
+
+[0] https://developer.arm.com/docs/den0077/latest
+[1] https://tools.ietf.org/html/rfc4122
+
+Example:
+
+	#address-cells = <2>;
+	#size-cells = <2>;
+
+	firmware {
+		psa-ffa {
+			compatible = "arm,psa-ffa";
+
+			partition0: psa_ffa_partition {
+				compatible = "arm,psa-ffa-partition";
+				uuid = "12345678-9abc-def0-1234-56789abcdef0";
+			};
+		};
+	};
-- 
2.17.1

Re: [RFC PATCH 1/3] dt-bindings: Add ARM PSA FF binding for non-secure VM partitions

[Rob Herring]

On Mon, Jun 01, 2020 at 10:45:10AM +0100, Sudeep Holla wrote:
> Add devicetree bindings for a Arm PSA FF-A compliant non-secure partition
> at virtual interface(VMs).
> 
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  .../devicetree/bindings/arm/arm,psa-ffa.txt   | 47 +++++++++++++++++++
>  1 file changed, 47 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/arm,psa-ffa.txt

I'm hoping this goes away if the firmware is discoverable, but if not DT 
bindings are DT schema now.

Rob

Re: [RFC PATCH 1/3] dt-bindings: Add ARM PSA FF binding for non-secure VM partitions

[Will Deacon]

On Tue, Jun 09, 2020 at 04:35:51PM -0600, Rob Herring wrote:
> On Mon, Jun 01, 2020 at 10:45:10AM +0100, Sudeep Holla wrote:
> > Add devicetree bindings for a Arm PSA FF-A compliant non-secure partition
> > at virtual interface(VMs).
> > 
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > ---
> >  .../devicetree/bindings/arm/arm,psa-ffa.txt   | 47 +++++++++++++++++++
> >  1 file changed, 47 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/arm/arm,psa-ffa.txt
> 
> I'm hoping this goes away if the firmware is discoverable, but if not DT 
> bindings are DT schema now.

We'll need the binding for the kvm host side, because there are plenty
of partition properties that are not discoverable (e.g. number of vCPUs).

I'll have a go a yamlifying what I have...

Will

Re: [RFC PATCH 1/3] dt-bindings: Add ARM PSA FF binding for non-secure VM partitions

[Rob Herring]

On Wed, Jun 10, 2020 at 1:43 AM Will Deacon <will@kernel.org> wrote:
>
> On Tue, Jun 09, 2020 at 04:35:51PM -0600, Rob Herring wrote:
> > On Mon, Jun 01, 2020 at 10:45:10AM +0100, Sudeep Holla wrote:
> > > Add devicetree bindings for a Arm PSA FF-A compliant non-secure partition
> > > at virtual interface(VMs).
> > >
> > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > > ---
> > >  .../devicetree/bindings/arm/arm,psa-ffa.txt   | 47 +++++++++++++++++++
> > >  1 file changed, 47 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/arm/arm,psa-ffa.txt
> >
> > I'm hoping this goes away if the firmware is discoverable, but if not DT
> > bindings are DT schema now.
>
> We'll need the binding for the kvm host side, because there are plenty
> of partition properties that are not discoverable (e.g. number of vCPUs).

BTW, there's already a partitioning binding for Power in ePAPR. I've
not paid much attention to it, but maybe it could be useful here.

Rob

Re: [RFC PATCH 1/3] dt-bindings: Add ARM PSA FF binding for non-secure VM partitions

[Achin Gupta]

Hi Will,

> On 10 Jun 2020, at 08:43, Will Deacon <will@kernel.org> wrote:
> 
> On Tue, Jun 09, 2020 at 04:35:51PM -0600, Rob Herring wrote:
>> On Mon, Jun 01, 2020 at 10:45:10AM +0100, Sudeep Holla wrote:
>>> Add devicetree bindings for a Arm PSA FF-A compliant non-secure partition
>>> at virtual interface(VMs).
>>> 
>>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>>> ---
>>> .../devicetree/bindings/arm/arm,psa-ffa.txt   | 47 +++++++++++++++++++
>>> 1 file changed, 47 insertions(+)
>>> create mode 100644 Documentation/devicetree/bindings/arm/arm,psa-ffa.txt
>> 
>> I'm hoping this goes away if the firmware is discoverable, but if not DT 
>> bindings are DT schema now.
> 
> We'll need the binding for the kvm host side, because there are plenty
> of partition properties that are not discoverable (e.g. number of vCPUs).

Just trying to understand the req. a bit better…

The FF-A driver in the host can use FFA_PARTITION_INFO_GET to determine the count of partitions and their vCPUs.

Is this about a guest being able to find out how many vCPUs it has? 

Cheers,
Achin

> 
> I'll have a go a yamlifying what I have...
> 
> Will
> 

Re: [RFC PATCH 1/3] dt-bindings: Add ARM PSA FF binding for non-secure VM partitions

[Will Deacon]

On Thu, Jun 11, 2020 at 03:46:35PM +0000, Achin Gupta wrote:
> > On 10 Jun 2020, at 08:43, Will Deacon <will@kernel.org> wrote:
> > On Tue, Jun 09, 2020 at 04:35:51PM -0600, Rob Herring wrote:
> >> On Mon, Jun 01, 2020 at 10:45:10AM +0100, Sudeep Holla wrote:
> >>> Add devicetree bindings for a Arm PSA FF-A compliant non-secure partition
> >>> at virtual interface(VMs).
> >>> 
> >>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> >>> ---
> >>> .../devicetree/bindings/arm/arm,psa-ffa.txt   | 47 +++++++++++++++++++
> >>> 1 file changed, 47 insertions(+)
> >>> create mode 100644 Documentation/devicetree/bindings/arm/arm,psa-ffa.txt
> >> 
> >> I'm hoping this goes away if the firmware is discoverable, but if not DT 
> >> bindings are DT schema now.
> > 
> > We'll need the binding for the kvm host side, because there are plenty
> > of partition properties that are not discoverable (e.g. number of vCPUs).
> 
> Just trying to understand the req. a bit better…
> 
> The FF-A driver in the host can use FFA_PARTITION_INFO_GET to determine
> the count of partitions and their vCPUs.
> 
> Is this about a guest being able to find out how many vCPUs it has? 

This is about KVM finding out the information it needs in order to spawn
non-secure partitions. I don't see how it can do that with
FFA_PARTITION_INFO_GET -- who would respond?

But you're right that number of vCPUs was a bad example. We also need
information such as the entry point.

Will

Re: [RFC PATCH 1/3] dt-bindings: Add ARM PSA FF binding for non-secure VM partitions

[Achin Gupta]

On Thu, Jun 11, 2020 at 06:12:23PM +0100, Will Deacon wrote:
> On Thu, Jun 11, 2020 at 03:46:35PM +0000, Achin Gupta wrote:
> > > On 10 Jun 2020, at 08:43, Will Deacon <will@kernel.org> wrote:
> > > On Tue, Jun 09, 2020 at 04:35:51PM -0600, Rob Herring wrote:
> > >> On Mon, Jun 01, 2020 at 10:45:10AM +0100, Sudeep Holla wrote:
> > >>> Add devicetree bindings for a Arm PSA FF-A compliant non-secure partition
> > >>> at virtual interface(VMs).
> > >>>
> > >>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > >>> ---
> > >>> .../devicetree/bindings/arm/arm,psa-ffa.txt   | 47 +++++++++++++++++++
> > >>> 1 file changed, 47 insertions(+)
> > >>> create mode 100644 Documentation/devicetree/bindings/arm/arm,psa-ffa.txt
> > >>
> > >> I'm hoping this goes away if the firmware is discoverable, but if not DT
> > >> bindings are DT schema now.
> > >
> > > We'll need the binding for the kvm host side, because there are plenty
> > > of partition properties that are not discoverable (e.g. number of vCPUs).
> >
> > Just trying to understand the req. a bit better…
> >
> > The FF-A driver in the host can use FFA_PARTITION_INFO_GET to determine
> > the count of partitions and their vCPUs.
> >
> > Is this about a guest being able to find out how many vCPUs it has?
>
> This is about KVM finding out the information it needs in order to spawn
> non-secure partitions. I don't see how it can do that with
> FFA_PARTITION_INFO_GET -- who would respond?

Right! FFA_PARTITION_INFO_GET is meant to help the FF-A driver in the kernel to
determine partition properties. It assumes that EL2 SW has already read each
partition's manifest and will reply to this ABI.

IIUC, with protected KVM, this information will have to be a part of the
manifest that the KVM host consumes.

But then, can this be made discoverable (use a SMC for discovery) at all as Rob
had originally suggested. Firmware (Secure world) has no clue and the bootloader
is long gone.

Separate topic, protected KVM does not get dibs on the manifest and it relies on
the KVM host to specify the address ranges for each partition? Does this not
mean that the KVM host can control the physical address space each partition
sees. This seems contrary to the isolation guarantees that protected KVM must
provide?

>
> But you're right that number of vCPUs was a bad example. We also need
> information such as the entry point.

Yes. From a spec perspective this should be specified in the partition manifest
unless the base address of the loaded image can be assummed to be the entry
point.

cheers,
Achin

>
> Will

Re: [RFC PATCH 1/3] dt-bindings: Add ARM PSA FF binding for non-secure VM partitions

[Will Deacon]

On Mon, Jun 15, 2020 at 10:16:39AM +0100, Achin Gupta wrote:
> On Thu, Jun 11, 2020 at 06:12:23PM +0100, Will Deacon wrote:
> > On Thu, Jun 11, 2020 at 03:46:35PM +0000, Achin Gupta wrote:
> > > > On 10 Jun 2020, at 08:43, Will Deacon <will@kernel.org> wrote:
> > > > On Tue, Jun 09, 2020 at 04:35:51PM -0600, Rob Herring wrote:
> > > >> On Mon, Jun 01, 2020 at 10:45:10AM +0100, Sudeep Holla wrote:
> > > >>> Add devicetree bindings for a Arm PSA FF-A compliant non-secure partition
> > > >>> at virtual interface(VMs).
> > > >>>
> > > >>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > > >>> ---
> > > >>> .../devicetree/bindings/arm/arm,psa-ffa.txt   | 47 +++++++++++++++++++
> > > >>> 1 file changed, 47 insertions(+)
> > > >>> create mode 100644 Documentation/devicetree/bindings/arm/arm,psa-ffa.txt
> > > >>
> > > >> I'm hoping this goes away if the firmware is discoverable, but if not DT
> > > >> bindings are DT schema now.
> > > >
> > > > We'll need the binding for the kvm host side, because there are plenty
> > > > of partition properties that are not discoverable (e.g. number of vCPUs).
> > >
> > > Just trying to understand the req. a bit better…
> > >
> > > The FF-A driver in the host can use FFA_PARTITION_INFO_GET to determine
> > > the count of partitions and their vCPUs.
> > >
> > > Is this about a guest being able to find out how many vCPUs it has?
> >
> > This is about KVM finding out the information it needs in order to spawn
> > non-secure partitions. I don't see how it can do that with
> > FFA_PARTITION_INFO_GET -- who would respond?
> 
> Right! FFA_PARTITION_INFO_GET is meant to help the FF-A driver in the kernel to
> determine partition properties. It assumes that EL2 SW has already read each
> partition's manifest and will reply to this ABI.
> 
> IIUC, with protected KVM, this information will have to be a part of the
> manifest that the KVM host consumes.

The host does not consume the manifest directly -- instead, the bootloader
will use the manifest to populate these DT nodes. Again, these are *only*
for non-secure virtual partitions which are to be managed by KVM.

> But then, can this be made discoverable (use a SMC for discovery) at all as Rob
> had originally suggested. Firmware (Secure world) has no clue and the bootloader
> is long gone.

Make what discoverable?

> Separate topic, protected KVM does not get dibs on the manifest and it relies on
> the KVM host to specify the address ranges for each partition? Does this not
> mean that the KVM host can control the physical address space each partition
> sees. This seems contrary to the isolation guarantees that protected KVM must
> provide?

The host is trusted during early boot, and gives up this trust after
initialising EL2 fully. So roughly speaking, we:

	* Boot at EL2 and install a shim
	* Drop down to EL2 and start the host kernel
	* Before some initialisation (DT parsing, SMP bringup, etc)
	* Init KVM by calling back up to EL2 to install the full hypervisor

At that point, the EL1 host is no longer trusted and the last call
effectively "locks it out" from EL2.

> > But you're right that number of vCPUs was a bad example. We also need
> > information such as the entry point.
> 
> Yes. From a spec perspective this should be specified in the partition manifest
> unless the base address of the loaded image can be assummed to be the entry
> point.

Right, but the format of the manifest isn't defined by the spec so I really
don't think it's something that Linux should be dealing with directly.

Will

Re: [RFC PATCH 1/3] dt-bindings: Add ARM PSA FF binding for non-secure VM partitions

[Achin Gupta]

On Mon, Jun 15, 2020 at 10:51:34AM +0100, Will Deacon wrote:
> On Mon, Jun 15, 2020 at 10:16:39AM +0100, Achin Gupta wrote:
> > On Thu, Jun 11, 2020 at 06:12:23PM +0100, Will Deacon wrote:
> > > On Thu, Jun 11, 2020 at 03:46:35PM +0000, Achin Gupta wrote:
> > > > > On 10 Jun 2020, at 08:43, Will Deacon <will@kernel.org> wrote:
> > > > > On Tue, Jun 09, 2020 at 04:35:51PM -0600, Rob Herring wrote:
> > > > >> On Mon, Jun 01, 2020 at 10:45:10AM +0100, Sudeep Holla wrote:
> > > > >>> Add devicetree bindings for a Arm PSA FF-A compliant non-secure partition
> > > > >>> at virtual interface(VMs).
> > > > >>>
> > > > >>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > > > >>> ---
> > > > >>> .../devicetree/bindings/arm/arm,psa-ffa.txt   | 47 +++++++++++++++++++
> > > > >>> 1 file changed, 47 insertions(+)
> > > > >>> create mode 100644 Documentation/devicetree/bindings/arm/arm,psa-ffa.txt
> > > > >>
> > > > >> I'm hoping this goes away if the firmware is discoverable, but if not DT
> > > > >> bindings are DT schema now.
> > > > >
> > > > > We'll need the binding for the kvm host side, because there are plenty
> > > > > of partition properties that are not discoverable (e.g. number of vCPUs).
> > > >
> > > > Just trying to understand the req. a bit better…
> > > >
> > > > The FF-A driver in the host can use FFA_PARTITION_INFO_GET to determine
> > > > the count of partitions and their vCPUs.
> > > >
> > > > Is this about a guest being able to find out how many vCPUs it has?
> > >
> > > This is about KVM finding out the information it needs in order to spawn
> > > non-secure partitions. I don't see how it can do that with
> > > FFA_PARTITION_INFO_GET -- who would respond?
> >
> > Right! FFA_PARTITION_INFO_GET is meant to help the FF-A driver in the kernel to
> > determine partition properties. It assumes that EL2 SW has already read each
> > partition's manifest and will reply to this ABI.
> >
> > IIUC, with protected KVM, this information will have to be a part of the
> > manifest that the KVM host consumes.
>
> The host does not consume the manifest directly -- instead, the bootloader
> will use the manifest to populate these DT nodes. Again, these are *only*
> for non-secure virtual partitions which are to be managed by KVM.

Yes. Understand and agree. Manifest is an overloaded term. I was using it to
describe the DT nodes that the host will consume.

>
> > But then, can this be made discoverable (use a SMC for discovery) at all as Rob
> > had originally suggested. Firmware (Secure world) has no clue and the bootloader
> > is long gone.
>
> Make what discoverable?

I thought the original question was to make partition properties discoverable
instead of relying on DT nodes. I might have misunderstood. Looks like we are on
the same page in any case :o)

>
> > Separate topic, protected KVM does not get dibs on the manifest and it relies on
> > the KVM host to specify the address ranges for each partition? Does this not
> > mean that the KVM host can control the physical address space each partition
> > sees. This seems contrary to the isolation guarantees that protected KVM must
> > provide?
>
> The host is trusted during early boot, and gives up this trust after
> initialising EL2 fully. So roughly speaking, we:
>
> 	* Boot at EL2 and install a shim
> 	* Drop down to EL2 and start the host kernel
> 	* Before some initialisation (DT parsing, SMP bringup, etc)
> 	* Init KVM by calling back up to EL2 to install the full hypervisor
>
> At that point, the EL1 host is no longer trusted and the last call
> effectively "locks it out" from EL2.

Ok. Protected KVM (PKVM) must create S2 tables when asked to setup a partition
by the Host. My main concern is if PKVM must trust the Host to provide the
correct physical address space ranges for a partition?

I guess your point is this is not a problem since PKVM can lock the Host out of
those address ranges in any case?

It is a bit counter intuitive that the Host gets to see and potentially
manipulate information that was verified and extracted by the bootloader from
the partition's manifest. This hapens before PKVM sees the same
information. Can't put my finger on what could go wrong though. Depends upon the
threat model too!

>
> > > But you're right that number of vCPUs was a bad example. We also need
> > > information such as the entry point.
> >
> > Yes. From a spec perspective this should be specified in the partition manifest
> > unless the base address of the loaded image can be assummed to be the entry
> > point.
>
> Right, but the format of the manifest isn't defined by the spec so I really
> don't think it's something that Linux should be dealing with directly.

Agree.

cheers,
Achin
>
> Will

Re: [RFC PATCH 1/3] dt-bindings: Add ARM PSA FF binding for non-secure VM partitions

[Will Deacon]

On Mon, Jun 15, 2020 at 12:42:20PM +0100, Achin Gupta wrote:
> On Mon, Jun 15, 2020 at 10:51:34AM +0100, Will Deacon wrote:
> > On Mon, Jun 15, 2020 at 10:16:39AM +0100, Achin Gupta wrote:
> > > Right! FFA_PARTITION_INFO_GET is meant to help the FF-A driver in the kernel to
> > > determine partition properties. It assumes that EL2 SW has already read each
> > > partition's manifest and will reply to this ABI.
> > >
> > > IIUC, with protected KVM, this information will have to be a part of the
> > > manifest that the KVM host consumes.
> >
> > The host does not consume the manifest directly -- instead, the bootloader
> > will use the manifest to populate these DT nodes. Again, these are *only*
> > for non-secure virtual partitions which are to be managed by KVM.
> 
> Yes. Understand and agree. Manifest is an overloaded term. I was using it to
> describe the DT nodes that the host will consume.

Hmm, I think that conflates two things though because only the partitions
managed by KVM will have DT nodes.

> > > Separate topic, protected KVM does not get dibs on the manifest and it relies on
> > > the KVM host to specify the address ranges for each partition? Does this not
> > > mean that the KVM host can control the physical address space each partition
> > > sees. This seems contrary to the isolation guarantees that protected KVM must
> > > provide?
> >
> > The host is trusted during early boot, and gives up this trust after
> > initialising EL2 fully. So roughly speaking, we:
> >
> > 	* Boot at EL2 and install a shim
> > 	* Drop down to EL2 and start the host kernel
> > 	* Before some initialisation (DT parsing, SMP bringup, etc)
> > 	* Init KVM by calling back up to EL2 to install the full hypervisor
> >
> > At that point, the EL1 host is no longer trusted and the last call
> > effectively "locks it out" from EL2.
> 
> Ok. Protected KVM (PKVM) must create S2 tables when asked to setup a partition
> by the Host. My main concern is if PKVM must trust the Host to provide the
> correct physical address space ranges for a partition?

Yes, but that all happens as part of KVM initialisation: the host parses
the DT nodes and memory reservations, and then passes this information
up to EL2.

> I guess your point is this is not a problem since PKVM can lock the Host out of
> those address ranges in any case?

It has to do this, regardless of how they are probed. Once KVM has
initialised, the host will have a stage-2 which limits it to the memory that
it is allowed to access.

> It is a bit counter intuitive that the Host gets to see and potentially
> manipulate information that was verified and extracted by the bootloader from
> the partition's manifest. This hapens before PKVM sees the same
> information. Can't put my finger on what could go wrong though. Depends upon the
> threat model too!

I think you're trying too hard to separate the host from the EL2 code during
early boot. Don't forget -- this is all part of the same binary payload that
is loaded and initially run at EL2. Having the host take care of early boot
/significantly/ reduces the amount of code at EL2, which has a very
clear security benefit.

Will

Re: [RFC PATCH 1/3] dt-bindings: Add ARM PSA FF binding for non-secure VM partitions

[Achin Gupta]

On Mon, Jun 15, 2020 at 12:55:49PM +0100, Will Deacon wrote:
> On Mon, Jun 15, 2020 at 12:42:20PM +0100, Achin Gupta wrote:
> > On Mon, Jun 15, 2020 at 10:51:34AM +0100, Will Deacon wrote:
> > > On Mon, Jun 15, 2020 at 10:16:39AM +0100, Achin Gupta wrote:
> > > > Right! FFA_PARTITION_INFO_GET is meant to help the FF-A driver in the kernel to
> > > > determine partition properties. It assumes that EL2 SW has already read each
> > > > partition's manifest and will reply to this ABI.
> > > >
> > > > IIUC, with protected KVM, this information will have to be a part of the
> > > > manifest that the KVM host consumes.
> > >
> > > The host does not consume the manifest directly -- instead, the bootloader
> > > will use the manifest to populate these DT nodes. Again, these are *only*
> > > for non-secure virtual partitions which are to be managed by KVM.
> >
> > Yes. Understand and agree. Manifest is an overloaded term. I was using it to
> > describe the DT nodes that the host will consume.
>
> Hmm, I think that conflates two things though because only the partitions
> managed by KVM will have DT nodes.

Sure. I am realising the need to maintain the distinction :o)

>
> > > > Separate topic, protected KVM does not get dibs on the manifest and it relies on
> > > > the KVM host to specify the address ranges for each partition? Does this not
> > > > mean that the KVM host can control the physical address space each partition
> > > > sees. This seems contrary to the isolation guarantees that protected KVM must
> > > > provide?
> > >
> > > The host is trusted during early boot, and gives up this trust after
> > > initialising EL2 fully. So roughly speaking, we:
> > >
> > > 	* Boot at EL2 and install a shim
> > > 	* Drop down to EL2 and start the host kernel
> > > 	* Before some initialisation (DT parsing, SMP bringup, etc)
> > > 	* Init KVM by calling back up to EL2 to install the full hypervisor
> > >
> > > At that point, the EL1 host is no longer trusted and the last call
> > > effectively "locks it out" from EL2.
> >
> > Ok. Protected KVM (PKVM) must create S2 tables when asked to setup a partition
> > by the Host. My main concern is if PKVM must trust the Host to provide the
> > correct physical address space ranges for a partition?
>
> Yes, but that all happens as part of KVM initialisation: the host parses
> the DT nodes and memory reservations, and then passes this information
> up to EL2.

Ok. Good to know this.

>
> > I guess your point is this is not a problem since PKVM can lock the Host out of
> > those address ranges in any case?
>
> It has to do this, regardless of how they are probed. Once KVM has
> initialised, the host will have a stage-2 which limits it to the memory that
> it is allowed to access.

Agree.

>
> > It is a bit counter intuitive that the Host gets to see and potentially
> > manipulate information that was verified and extracted by the bootloader from
> > the partition's manifest. This hapens before PKVM sees the same
> > information. Can't put my finger on what could go wrong though. Depends upon the
> > threat model too!
>
> I think you're trying too hard to separate the host from the EL2 code during
> early boot. Don't forget -- this is all part of the same binary payload that
> is loaded and initially run at EL2. Having the host take care of early boot
> /significantly/ reduces the amount of code at EL2, which has a very
> clear security benefit.

Fair point!

cheers,
Achin

>
> Will

Re: [RFC PATCH 1/3] dt-bindings: Add ARM PSA FF binding for non-secure VM partitions

[Sudeep Holla]

On Tue, Jun 09, 2020 at 04:35:51PM -0600, Rob Herring wrote:
> On Mon, Jun 01, 2020 at 10:45:10AM +0100, Sudeep Holla wrote:
> > Add devicetree bindings for a Arm PSA FF-A compliant non-secure partition
> > at virtual interface(VMs).
> >
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > ---
> >  .../devicetree/bindings/arm/arm,psa-ffa.txt   | 47 +++++++++++++++++++
> >  1 file changed, 47 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/arm/arm,psa-ffa.txt
>
> I'm hoping this goes away if the firmware is discoverable, but if not DT
> bindings are DT schema now.
>

Yes, consider it gone. I introduced it to couple of reasons both of which
are now irrelevant. We may revisit this if we need it for Rx/Tx buffer
information. The KVM host may need the binding, this was for the guests/
partitions as Will has already mentioned.

--
Regards,
Sudeep

[RFC PATCH 2/3] firmware: Add support for PSA FF-A transport for VM partitions

[Sudeep Holla]

Initial support for PSA FF-A interface providing APIs for non-secure VM
partitions.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/Kconfig              |   1 +
 drivers/firmware/Makefile             |   1 +
 drivers/firmware/arm_psa_ffa/Kconfig  |  15 ++
 drivers/firmware/arm_psa_ffa/Makefile |   2 +
 drivers/firmware/arm_psa_ffa/driver.c | 250 ++++++++++++++++++++++++++
 include/linux/arm_psa_ffa.h           |  42 +++++
 6 files changed, 311 insertions(+)
 create mode 100644 drivers/firmware/arm_psa_ffa/Kconfig
 create mode 100644 drivers/firmware/arm_psa_ffa/Makefile
 create mode 100644 drivers/firmware/arm_psa_ffa/driver.c
 create mode 100644 include/linux/arm_psa_ffa.h

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index 4843e94713a4..1ee421974cba 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -295,6 +295,7 @@ config TURRIS_MOX_RWTM
 	  other manufacturing data and also utilize the Entropy Bit Generator
 	  for hardware random number generation.
 
+source "drivers/firmware/arm_psa_ffa/Kconfig"
 source "drivers/firmware/broadcom/Kconfig"
 source "drivers/firmware/google/Kconfig"
 source "drivers/firmware/efi/Kconfig"
diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index 99510be9f5ed..1c0b5f130d7d 100644
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -22,6 +22,7 @@ obj-$(CONFIG_TI_SCI_PROTOCOL)	+= ti_sci.o
 obj-$(CONFIG_TRUSTED_FOUNDATIONS) += trusted_foundations.o
 obj-$(CONFIG_TURRIS_MOX_RWTM)	+= turris-mox-rwtm.o
 
+obj-y				+= arm_psa_ffa/
 obj-$(CONFIG_ARM_SCMI_PROTOCOL)	+= arm_scmi/
 obj-y				+= broadcom/
 obj-y				+= meson/
diff --git a/drivers/firmware/arm_psa_ffa/Kconfig b/drivers/firmware/arm_psa_ffa/Kconfig
new file mode 100644
index 000000000000..ba699ec68ec4
--- /dev/null
+++ b/drivers/firmware/arm_psa_ffa/Kconfig
@@ -0,0 +1,15 @@
+# SPDX-License-Identifier: GPL-2.0-only
+config ARM_PSA_FFA_TRANSPORT
+	tristate "Arm Platform Security Architecture Firmware Framework for Armv8-A"
+	depends on ARM64 && HAVE_ARM_SMCCC_DISCOVERY
+	help
+	  This Firmware Framework(FF) of the Platform Security Architecture
+	  (PSA) for Arm A-profile processors describes interfaces that
+	  standardize communication between the various software images which
+	  includes communication between images in the Secure world and
+	  Normal world. It also leverages the virtualization extension to
+	  isolate software images provided by an ecosystem of vendors from
+	  each other.
+
+	  This driver provides interface for all the client drivers making
+	  use of the features offered by ARM PSA-FF-A.
diff --git a/drivers/firmware/arm_psa_ffa/Makefile b/drivers/firmware/arm_psa_ffa/Makefile
new file mode 100644
index 000000000000..ac0455ff71a4
--- /dev/null
+++ b/drivers/firmware/arm_psa_ffa/Makefile
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0-only
+obj-$(CONFIG_ARM_PSA_FFA_TRANSPORT) += driver.o
diff --git a/drivers/firmware/arm_psa_ffa/driver.c b/drivers/firmware/arm_psa_ffa/driver.c
new file mode 100644
index 000000000000..700bd5850746
--- /dev/null
+++ b/drivers/firmware/arm_psa_ffa/driver.c
@@ -0,0 +1,250 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Arm Platform Security Architecture(PSA) Firmware Framework for ARMv8-A(FFA)
+ * interface driver
+ *
+ * The Arm PSA FFA specification[1] describes a software architecture to
+ * leverages the virtualization extension to isolate software images
+ * provided by an ecosystem of vendors from each other and describes
+ * interfaces that standardize communication between the various software
+ * images including communication between images in the Secure world and
+ * Normal world. Any Hypervisor could use the PSA FFA interfaces to enable
+ * communication between VMs it manages.
+ *
+ * The Hypervisor a.k.a Partition managers in FFA terminology can assign
+ * system resources(Memory regions, Devices, CPU cycles) to the partitions
+ * and manage isolation amongst them.
+ *
+ * [1] https://developer.arm.com/docs/den0077/latest
+ *
+ * Copyright (C) 2020 Arm Ltd.
+ */
+
+#define pr_fmt(fmt) "ARM PSA FF-A: " fmt
+
+#include <linux/arm_psa_ffa.h>
+#include <linux/arm-smccc.h>
+#include <linux/bitfield.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <linux/types.h>
+
+#define PSA_FFA_SMC(calling_convention, func_num)			\
+	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, (calling_convention),	\
+			   ARM_SMCCC_OWNER_STANDARD, (func_num))
+
+#define PSA_FFA_SMC_32(func_num)	PSA_FFA_SMC(ARM_SMCCC_SMC_32, (func_num))
+#define PSA_FFA_SMC_64(func_num)	PSA_FFA_SMC(ARM_SMCCC_SMC_64, (func_num))
+
+#define PSA_FFA_ERROR				PSA_FFA_SMC_32(0x60)
+#define PSA_FFA_SUCCESS				PSA_FFA_SMC_32(0x61)
+#define PSA_FFA_INTERRUPT			PSA_FFA_SMC_32(0x62)
+#define PSA_FFA_VERSION				PSA_FFA_SMC_32(0x63)
+#define PSA_FFA_FEATURES			PSA_FFA_SMC_32(0x64)
+#define PSA_FFA_PARTITION_INFO_GET		PSA_FFA_SMC_32(0x68)
+#define PSA_FFA_ID_GET				PSA_FFA_SMC_32(0x69)
+
+/* PSA_FFA error codes. */
+#define PSA_FFA_RET_SUCCESS            (0)
+#define PSA_FFA_RET_NOT_SUPPORTED      (-1)
+#define PSA_FFA_RET_INVALID_PARAMETERS (-2)
+#define PSA_FFA_RET_NO_MEMORY          (-3)
+#define PSA_FFA_RET_BUSY               (-4)
+#define PSA_FFA_RET_INTERRUPTED        (-5)
+#define PSA_FFA_RET_DENIED             (-6)
+#define PSA_FFA_RET_RETRY              (-7)
+#define PSA_FFA_RET_ABORTED            (-8)
+
+#define MAJOR_VERSION_MASK	GENMASK(30, 16)
+#define MINOR_VERSION_MASK	GENMASK(15, 0)
+#define MAJOR_VERSION(x)	(u16)(FIELD_GET(MAJOR_VERSION_MASK, (x)))
+#define MINOR_VERSION(x)	(u16)(FIELD_GET(MINOR_VERSION_MASK, (x)))
+#define PACK_VERSION_INFO(major, minor)			\
+	(FIELD_PREP(MAJOR_VERSION_MASK, (major)) | 	\
+	FIELD_PREP(MINOR_VERSION_MASK, (minor)))
+#define PSA_FFA_VERSION_1_0	PACK_VERSION_INFO(1, 0)
+#define PSA_FFA_MIN_VERSION	PSA_FFA_VERSION_1_0
+#define PSA_FFA_DRIVER_VERSION	PSA_FFA_VERSION_1_0
+
+#define SENDER_ID_MASK		GENMASK(31, 16)
+#define RECEIVER_ID_MASK	GENMASK(15, 0)
+#define SENDER_ID(x)		(u16)(FIELD_GET(SENDER_ID_MASK, (x)))
+#define RECEIVER_ID(x)		(u16)(FIELD_GET(RECEIVER_ID_MASK, (x)))
+#define PACK_TARGET_INFO(s, r)		\
+	(FIELD_PREP(SENDER_ID_MASK, (s)) | FIELD_PREP(RECEIVER_ID_MASK, (r)))
+
+typedef struct arm_smccc_res
+(arm_psa_ffa_fn)(unsigned long, unsigned long, unsigned long, unsigned long,
+		 unsigned long, unsigned long, unsigned long, unsigned long);
+static arm_psa_ffa_fn *invoke_arm_psa_ffa_fn;
+
+static struct device *psa_ffa_dev;
+
+static const int psa_ffa_linux_errmap[] = {
+	/* better than switch case as long as return value is continuous */
+	0,		/* PSA_FFA_RET_SUCCESS */
+	-EOPNOTSUPP,	/* PSA_FFA_RET_NOT_SUPPORTED */
+	-EINVAL,	/* PSA_FFA_RET_INVALID_PARAMETERS */
+	-ENOMEM,	/* PSA_FFA_RET_NO_MEMORY */
+	-EBUSY,		/* PSA_FFA_RET_BUSY */
+	-EINTR,		/* PSA_FFA_RET_INTERRUPTED */
+	-EACCES,	/* PSA_FFA_RET_DENIED */
+	-EAGAIN,	/* PSA_FFA_RET_RETRY */
+	-ECANCELED,	/* PSA_FFA_RET_ABORTED */
+};
+
+static inline int psa_ffa_to_linux_errno(int errno)
+{
+	if (errno < PSA_FFA_RET_SUCCESS && errno >= PSA_FFA_RET_ABORTED)
+		return psa_ffa_linux_errmap[-errno];
+	return -EINVAL;
+}
+
+struct arm_smccc_res
+__arm_psa_ffa_fn_smc(unsigned long function_id,unsigned long arg0,
+		     unsigned long arg1, unsigned long arg2,
+		     unsigned long arg3, unsigned long arg4,
+		     unsigned long arg5, unsigned long arg6)
+{
+	struct arm_smccc_res res;
+
+	arm_smccc_smc(function_id, arg0, arg1, arg2, arg3, arg4, arg5, arg6,
+		      &res);
+
+	return res;
+}
+
+struct arm_smccc_res
+__arm_psa_ffa_fn_hvc(unsigned long function_id,unsigned long arg0,
+		     unsigned long arg1, unsigned long arg2,
+		     unsigned long arg3, unsigned long arg4,
+		     unsigned long arg5, unsigned long arg6)
+{
+	struct arm_smccc_res res;
+
+	arm_smccc_hvc(function_id, arg0, arg1, arg2, arg3, arg4, arg5, arg6,
+		      &res);
+
+	return res;
+}
+
+static u16 psa_ffa_id_get(void)
+{
+	struct arm_smccc_res id;
+
+	id = invoke_arm_psa_ffa_fn(PSA_FFA_ID_GET, 0, 0, 0, 0, 0, 0, 0);
+
+	if (id.a0 == PSA_FFA_ERROR)
+		return psa_ffa_to_linux_errno((int)id.a2);
+	else
+		return id.a2 & 0xffff;
+}
+
+static int psa_ffa_partition_info_get(u32 uuid0, u32 uuid1, u32 uuid2, u32 uuid3,
+				      struct psa_ffa_partition_info **buffer)
+{
+	struct arm_smccc_res partition_info;
+
+	partition_info = invoke_arm_psa_ffa_fn(PSA_FFA_PARTITION_INFO_GET,
+					       uuid0, uuid1, uuid2, uuid3,
+					       0, 0, 0);
+
+	if (partition_info.a0 == PSA_FFA_ERROR)
+		return psa_ffa_to_linux_errno((int)partition_info.a2);
+
+	/* TODO: read data from RX buffers */
+	return partition_info.a2;
+}
+
+static struct arm_psa_ffa_handle drv_handle = {
+	.id_get = psa_ffa_id_get,
+	.partition_info_get = psa_ffa_partition_info_get,
+};
+
+struct arm_psa_ffa_handle *arm_psa_ffa_handle_get(struct device *dev)
+{
+	 struct arm_psa_ffa_handle *handle = NULL;
+
+	if (dev->parent == psa_ffa_dev)
+		handle = &drv_handle;
+
+	return handle;
+}
+
+EXPORT_SYMBOL_GPL(arm_psa_ffa_handle_get);
+
+static int psa_ffa_version_check(void)
+{
+	struct arm_smccc_res version;
+
+	version = invoke_arm_psa_ffa_fn(PSA_FFA_VERSION, PSA_FFA_DRIVER_VERSION,
+					0, 0, 0, 0, 0, 0);
+
+	if (version.a0 == PSA_FFA_RET_NOT_SUPPORTED) {
+		pr_info("PSA_FFA_VERSION returned not supported\n");
+		return -ENOTSUPP;
+	}
+
+	if (version.a0 < PSA_FFA_MIN_VERSION ||
+	    version.a0 > PSA_FFA_DRIVER_VERSION) {
+		pr_err("Incompatible version %d.%d found\n",
+		       MAJOR_VERSION(version.a0), MINOR_VERSION(version.a0));
+		return -EINVAL;
+	}
+
+	pr_info("Found version %d.%d found\n", MAJOR_VERSION(version.a0),
+		MINOR_VERSION(version.a0));
+	return 0;
+}
+
+static int psa_ffa_probe(struct platform_device *pdev)
+{
+	int ret;
+	enum arm_smccc_conduit conduit;
+
+	if (arm_smccc_get_version() < ARM_SMCCC_VERSION_1_2)
+		return 0;
+
+	conduit = arm_smccc_1_1_get_conduit();
+	if (conduit == SMCCC_CONDUIT_NONE) {
+		pr_err("%s: invalid SMCCC conduit\n", __func__);
+		return -EOPNOTSUPP;
+	}
+
+	if (conduit == SMCCC_CONDUIT_SMC)
+		invoke_arm_psa_ffa_fn = __arm_psa_ffa_fn_smc;
+	else
+		invoke_arm_psa_ffa_fn = __arm_psa_ffa_fn_hvc;
+
+	ret = psa_ffa_version_check();
+	if (ret)
+		return ret;
+
+	psa_ffa_dev = &pdev->dev;
+
+	return devm_of_platform_populate(psa_ffa_dev);
+}
+
+static const struct of_device_id psa_ffa_of_match[] = {
+	{.compatible = "arm,psa-ffa"},
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, psa_ffa_of_match);
+
+static struct platform_driver psa_ffa_driver = {
+	.driver = {
+		   .name = "arm-psa-ffa",
+		   .of_match_table = psa_ffa_of_match,
+		   },
+	.probe = psa_ffa_probe,
+};
+
+module_platform_driver(psa_ffa_driver);
+
+MODULE_ALIAS("platform: arm-psa-ffa");
+MODULE_AUTHOR("Sudeep Holla <sudeep.holla@arm.com>");
+MODULE_DESCRIPTION("Arm PSA FF-A interface driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/arm_psa_ffa.h b/include/linux/arm_psa_ffa.h
new file mode 100644
index 000000000000..03a4ff559fa3
--- /dev/null
+++ b/include/linux/arm_psa_ffa.h
@@ -0,0 +1,42 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2019, 2020 Arm Ltd.
+ */
+
+#ifndef __LINUX_ARM_PSA_FFA_H
+#define __LINUX_ARM_PSA_FFA_H
+
+#include <linux/device.h>
+#include <linux/types.h>
+
+struct psa_ffa_partition_info {
+	/* The ID of the VM the information is about */
+	uint16_t id;
+	/* The number of execution contexts implemented by the partition */
+	uint16_t execution_context;
+	/* The Partition's properties, e.g. supported messaging methods */
+	uint32_t partition_properties;
+};
+
+
+/**
+ * struct psa_ffa_ops - represents the various PSA_FFA protocol operations
+ * available for an endpoint.
+ */
+struct arm_psa_ffa_handle {
+	u16 (*id_get)(void);
+	int (*partition_info_get)(u32 uuid0, u32 uuid1, u32 uuid2, u32 uuid3,
+				  struct psa_ffa_partition_info**);
+};
+
+#if IS_REACHABLE(CONFIG_ARM_PSA_FFA_TRANSPORT)
+struct arm_psa_ffa_handle *arm_psa_ffa_handle_get(struct device *dev);
+#else
+static inline
+struct arm_psa_ffa_handle * arm_psa_ffa_handle_get(struct device *dev)
+{
+	return NULL;
+}
+#endif
+
+#endif /*__LINUX_ARM_PSA_FFA_H*/
-- 
2.17.1

Re: [RFC PATCH 2/3] firmware: Add support for PSA FF-A transport for VM partitions

[Arve Hjønnevåg]

On Mon, Jun 1, 2020 at 2:45 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> Initial support for PSA FF-A interface providing APIs for non-secure VM
> partitions.
>
...
> diff --git a/drivers/firmware/arm_psa_ffa/Kconfig b/drivers/firmware/arm_psa_ffa/Kconfig
> new file mode 100644
> index 000000000000..ba699ec68ec4
> --- /dev/null
> +++ b/drivers/firmware/arm_psa_ffa/Kconfig
> @@ -0,0 +1,15 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config ARM_PSA_FFA_TRANSPORT
> +       tristate "Arm Platform Security Architecture Firmware Framework for Armv8-A"
> +       depends on ARM64 && HAVE_ARM_SMCCC_DISCOVERY

Most of this driver should be usable on any platform, so it would be
better to only depend on ARM64 in the component that has the arm64
specific implementation of your low level conduit.

...
> diff --git a/drivers/firmware/arm_psa_ffa/driver.c b/drivers/firmware/arm_psa_ffa/driver.c
> new file mode 100644
> index 000000000000..700bd5850746
> --- /dev/null
> +++ b/drivers/firmware/arm_psa_ffa/driver.c
...
> +
> +typedef struct arm_smccc_res
> +(arm_psa_ffa_fn)(unsigned long, unsigned long, unsigned long, unsigned long,
> +                unsigned long, unsigned long, unsigned long, unsigned long);
> +static arm_psa_ffa_fn *invoke_arm_psa_ffa_fn;
> +
...
> +struct arm_smccc_res
> +__arm_psa_ffa_fn_smc(unsigned long function_id,unsigned long arg0,
...
> +struct arm_smccc_res
> +__arm_psa_ffa_fn_hvc(unsigned long function_id,unsigned long arg0,

Can these two functions move out of this file so this driver only
depends on a function matching the arm_psa_ffa_fn type?

...
> +static int psa_ffa_probe(struct platform_device *pdev)
> +{
> +       int ret;
> +       enum arm_smccc_conduit conduit;
> +
> +       if (arm_smccc_get_version() < ARM_SMCCC_VERSION_1_2)
> +               return 0;
> +
> +       conduit = arm_smccc_1_1_get_conduit();

If you make this device a child device of the conduit, then you don't
need this here. Other conduits can be added to for instance support
other architectures without adding entries to this enum and modifying
this driver.

--
Arve Hjønnevåg

[RFC PATCH 3/3] firmware: Add example PSA FF-A non-secure VM partition

[Sudeep Holla]

This is just an example non-secure VM partition to show how to create
the device and use the PSA FF-A interface APIs.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_psa_ffa/Kconfig     |  7 +++
 drivers/firmware/arm_psa_ffa/Makefile    |  1 +
 drivers/firmware/arm_psa_ffa/partition.c | 71 ++++++++++++++++++++++++
 3 files changed, 79 insertions(+)
 create mode 100644 drivers/firmware/arm_psa_ffa/partition.c

diff --git a/drivers/firmware/arm_psa_ffa/Kconfig b/drivers/firmware/arm_psa_ffa/Kconfig
index ba699ec68ec4..34ad61e79234 100644
--- a/drivers/firmware/arm_psa_ffa/Kconfig
+++ b/drivers/firmware/arm_psa_ffa/Kconfig
@@ -13,3 +13,10 @@ config ARM_PSA_FFA_TRANSPORT
 
 	  This driver provides interface for all the client drivers making
 	  use of the features offered by ARM PSA-FF-A.
+
+config ARM_PSA_FFA_PARTITION
+	tristate "Arm PSA FF-A compliant partition"
+	depends on ARM_PSA_FFA_TRANSPORT
+	help
+	  This driver provides example for ARM PSA-FF-A client driver
+	  making use of the interfaces offered by ARM PSA-FF-A driver.
diff --git a/drivers/firmware/arm_psa_ffa/Makefile b/drivers/firmware/arm_psa_ffa/Makefile
index ac0455ff71a4..8eb03898baf7 100644
--- a/drivers/firmware/arm_psa_ffa/Makefile
+++ b/drivers/firmware/arm_psa_ffa/Makefile
@@ -1,2 +1,3 @@
 # SPDX-License-Identifier: GPL-2.0-only
 obj-$(CONFIG_ARM_PSA_FFA_TRANSPORT) += driver.o
+obj-$(CONFIG_ARM_PSA_FFA_PARTITION) += partition.o
diff --git a/drivers/firmware/arm_psa_ffa/partition.c b/drivers/firmware/arm_psa_ffa/partition.c
new file mode 100644
index 000000000000..8549f8d61454
--- /dev/null
+++ b/drivers/firmware/arm_psa_ffa/partition.c
@@ -0,0 +1,71 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Arm PSA FFA example partition driver
+ *
+ * Copyright (C) 2020 Arm Ltd.
+ */
+
+#include <linux/arm_psa_ffa.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <linux/types.h>
+#include <linux/uuid.h>
+
+static int psa_ffa_partition_probe(struct platform_device *pdev)
+{
+	u16 vm_id;
+	uuid_t uuid;
+	const char *uuid_str;
+	u32 uuid0_4[4];
+	struct device *dev = &pdev->dev;
+	const struct device_node *np = dev->of_node;
+	struct arm_psa_ffa_handle *handle;
+	struct psa_ffa_partition_info **buffer;
+
+	handle = arm_psa_ffa_handle_get(dev);
+	if (!handle)
+		return -ENODEV;
+
+	if (of_property_read_string(np, "uuid", &uuid_str)) {
+		dev_err(dev, "failed to parse \"uuid\" property in '%pOF'\n", np);
+		return -ENODEV;
+	}
+
+	if (uuid_parse(uuid_str, &uuid)) {
+		dev_err(dev, "invalid \"uuid\" property (%s)\n", uuid_str);
+		return -ENODEV;
+	}
+
+	export_uuid((u8 *)uuid0_4, &uuid);
+
+	vm_id = handle->id_get();
+
+	handle->partition_info_get(uuid0_4[0], uuid0_4[1], uuid0_4[2],
+				   uuid0_4[3], buffer);
+
+	return 0;
+}
+
+static const struct of_device_id psa_ffa_partition_of_match[] = {
+	{.compatible = "arm,psa-ffa-partition"},
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, psa_ffa_partition_of_match);
+
+static struct platform_driver psa_ffa_partition_driver = {
+	.driver = {
+		   .name = "psa-ffa-partition",
+		   .of_match_table = psa_ffa_partition_of_match,
+		   },
+	.probe = psa_ffa_partition_probe,
+};
+
+module_platform_driver(psa_ffa_partition_driver);
+
+MODULE_ALIAS("platform: arm-psa-ffa");
+MODULE_AUTHOR("Sudeep Holla <sudeep.holla@arm.com>");
+MODULE_DESCRIPTION("Arm PSA FF-A example partition driver");
+MODULE_LICENSE("GPL v2");
-- 
2.17.1

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

[Will Deacon]

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.)

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 ;)

> 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.

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.

> 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...

> 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.

> 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.

> 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.

 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)

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.

Anyway, to enable this, I think we need a clear separation in the kernel
between the FF-A code and the users: KVM will want to expose things as
above, but if drivers need to use this stuff as well then they can plug in
as additional users and we don't have to worry about tripping over the
RX/TX buffers etc.

What do you think, and do you reckon you can spin a cut-down driver that
implements the common part of the logic (since I know you've written much
of this code already)?

Cheers,

Will

[1] https://android-kvm.googlesource.com/linux/+/8632a5723ef106017c4ab57e95d9ce7630d35522%5E%21/#F0

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

[Sudeep Holla]

(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.

> Anyway, to enable this, I think we need a clear separation in the kernel
> between the FF-A code and the users:
Agreed.

> KVM will want to expose things as above, but if drivers need to use this
> stuff as well then they can plug in as additional users and we don't have to
> worry about tripping over the RX/TX buffers etc.
>

I am confused a bit. When you refer drivers above, are you referring to
drivers in host kernel(hypervisor) or in the partitions. I fail to
imagine need for the former.

> What do you think, and do you reckon you can spin a cut-down driver that
> implements the common part of the logic (since I know you've written much
> of this code already)?
>

I am not sure if I am aligned with your thoughts on the buffer sharing
yet.

--
Regards,
Sudeep

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

[Will Deacon]

Hi Sudeep,

On Tue, Jun 09, 2020 at 06:41:23PM +0100, Sudeep Holla wrote:
> On Thu, Jun 04, 2020 at 02:37:46PM +0100, Will Deacon wrote:
> > 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.
> >
> 
> 😁 

Renamed version here:

https://android-kvm.googlesource.com/linux/+/refs/heads/willdeacon/psa-ffa

although I haven't psyched myself up to write yaml yet.

> > 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.

Ok, sounds like we should at least get to a position where we can enumerate
things, though.

> > > 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 ?

This is only a concern if KVM chooses to provide the Rx/Tx buffer pair
though, right? If we punt that down the road for the moment, then we can
just rely on FFA_RXTX_MAP for now.

> > 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.
> 
> > Anyway, to enable this, I think we need a clear separation in the kernel
> > between the FF-A code and the users:
> Agreed.
> 
> > KVM will want to expose things as above, but if drivers need to use this
> > stuff as well then they can plug in as additional users and we don't have to
> > worry about tripping over the RX/TX buffers etc.
> >
> 
> I am confused a bit. When you refer drivers above, are you referring to
> drivers in host kernel(hypervisor) or in the partitions. I fail to
> imagine need for the former.

I'm referring to in-kernel users in the host kernel. For KVM-managed guests,
we may not need these, although signalling things like system shutdown might
be better off done without relying on userspace. But my point is really that
separating the buffer management from the users means we can serialise
consumers, whether they are in-kernel or out in userspace.

> > What do you think, and do you reckon you can spin a cut-down driver that
> > implements the common part of the logic (since I know you've written much
> > of this code already)?
> >
> 
> I am not sure if I am aligned with your thoughts on the buffer sharing
> yet.

Ok, please let me know if you have any more questions.

Will

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

[Sudeep Holla]

Hi Will,

On Wed, Jun 10, 2020 at 08:57:12AM +0100, Will Deacon wrote:
> Hi Sudeep,
>
> On Tue, Jun 09, 2020 at 06:41:23PM +0100, Sudeep Holla wrote:

[...]

> >
> > 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.
>
> Ok, sounds like we should at least get to a position where we can enumerate
> things, though.
>

Yes.

[...]

> >
> > 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 ?
>
> This is only a concern if KVM chooses to provide the Rx/Tx buffer pair
> though, right? If we punt that down the road for the moment, then we can
> just rely on FFA_RXTX_MAP for now.
>

Ah OK, I was under the assumption that we didn't want to use FFA_RXTX_{,UN}MAP

[...]

> >
> > I am confused a bit. When you refer drivers above, are you referring to
> > drivers in host kernel(hypervisor) or in the partitions. I fail to
> > imagine need for the former.
>
> I'm referring to in-kernel users in the host kernel. For KVM-managed guests,
> we may not need these, although signalling things like system shutdown might
> be better off done without relying on userspace. But my point is really that
> separating the buffer management from the users means we can serialise
> consumers, whether they are in-kernel or out in userspace.
>

Understood.

> > > What do you think, and do you reckon you can spin a cut-down driver that
> > > implements the common part of the logic (since I know you've written much
> > > of this code already)?
> > >
> >
> > I am not sure if I am aligned with your thoughts on the buffer sharing
> > yet.
>
> Ok, please let me know if you have any more questions.
>

None ATM. As I mentioned I had ruled out RXTX_{,UN}MAP which was my
misunderstanding.

--
Regards,
Sudeep

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

[Jens Wiklander]

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