Re: [XEN PATCH v8 02/22] xen/arm: tee: add a primitive FF-A mediator
Hi Andrew,
> On 4 May 2023, at 15:14, Bertrand Marquis wrote:
>
> Hi Andrew,
>
>> On 14 Apr 2023, at 10:58, Jens Wiklander wrote:
>>
>> Hi,
>>
>> On Thu, Apr 13, 2023 at 3:27 PM Andrew Cooper
>> wrote:
>>>
>>> On 13/04/2023 1:26 pm, Julien Grall wrote:
> +static int ffa_domain_init(struct domain *d)
> +{
> +struct ffa_ctx *ctx;
> +
> +if ( !ffa_version )
> +return -ENODEV;
> +
> +ctx = xzalloc(struct ffa_ctx);
> +if ( !ctx )
> +return -ENOMEM;
> +
> +d->arch.tee = ctx;
> +
> +return 0;
> +}
> +
> +/* This function is supposed to undo what ffa_domain_init() has done */
I think there is a problem in the TEE framework. The callback
.relinquish_resources() will not be called if domain_create() failed.
So this will result to a memory leak.
We also can't call .relinquish_resources() on early domain creation
failure because relinquishing resources can take time and therefore
needs to be preemptible.
So I think we need to introduce a new callback domain_free() that will
be called arch_domain_destroy(). Is this something you can look at?
>>>
>>>
>>> Cleanup of an early domain creation failure, however you do it, is at
>>> most "the same amount of time again". It cannot (absent of development
>>> errors) take the same indefinite time periods of time that a full
>>> domain_destroy() can.
>>>
>>> The error path in domain_create() explicitly does call domain_teardown()
>>> so we can (eventually) purge these duplicate cleanup paths. There are
>>> far too many easy errors to be made which occur from having split
>>> cleanup, and we have had to issue XSAs in the past to address some of
>>> them. (Hence the effort to try and specifically change things, and
>>> remove the ability to introduce the errors in the first place.)
>>>
>>>
>>> Right now, it is specifically awkward to do this nicely because
>>> domain_teardown() doesn't call into a suitable arch hook.
>>>
>>> IMO the best option here is extend domain_teardown() with an
>>> arch_domain_teardown() state/hook, and wire in the TEE cleanup path into
>>> this too.
>>>
>>> Anything else is explicitly adding to technical debt that I (or someone
>>> else) is going to have to revert further down the line.
>>>
>>> If you want, I am happy to prototype the arch_domain_teardown() bit of
>>> the fix, but I will have to defer wiring in the TEE part to someone
>>> capable of testing it.
>>
>> You're more than welcome to prototype the fix, I can test it and add
>> it to the next version of the patch set when we're happy with the
>> result.
>
>
> Could you tell us if you are still happy to work on the prototype for
> arch_domain_teardown and when you would be able to give a first prototype ?
Could you answer to this question ?
Cheers
Bertrand
Re: [XEN PATCH v8 02/22] xen/arm: tee: add a primitive FF-A mediator
Hi Andrew,
> On 14 Apr 2023, at 10:58, Jens Wiklander wrote:
>
> Hi,
>
> On Thu, Apr 13, 2023 at 3:27 PM Andrew Cooper
> wrote:
>>
>> On 13/04/2023 1:26 pm, Julien Grall wrote:
+static int ffa_domain_init(struct domain *d)
+{
+struct ffa_ctx *ctx;
+
+if ( !ffa_version )
+return -ENODEV;
+
+ctx = xzalloc(struct ffa_ctx);
+if ( !ctx )
+return -ENOMEM;
+
+d->arch.tee = ctx;
+
+return 0;
+}
+
+/* This function is supposed to undo what ffa_domain_init() has done */
>>>
>>> I think there is a problem in the TEE framework. The callback
>>> .relinquish_resources() will not be called if domain_create() failed.
>>> So this will result to a memory leak.
>>>
>>> We also can't call .relinquish_resources() on early domain creation
>>> failure because relinquishing resources can take time and therefore
>>> needs to be preemptible.
>>>
>>> So I think we need to introduce a new callback domain_free() that will
>>> be called arch_domain_destroy(). Is this something you can look at?
>>
>>
>> Cleanup of an early domain creation failure, however you do it, is at
>> most "the same amount of time again". It cannot (absent of development
>> errors) take the same indefinite time periods of time that a full
>> domain_destroy() can.
>>
>> The error path in domain_create() explicitly does call domain_teardown()
>> so we can (eventually) purge these duplicate cleanup paths. There are
>> far too many easy errors to be made which occur from having split
>> cleanup, and we have had to issue XSAs in the past to address some of
>> them. (Hence the effort to try and specifically change things, and
>> remove the ability to introduce the errors in the first place.)
>>
>>
>> Right now, it is specifically awkward to do this nicely because
>> domain_teardown() doesn't call into a suitable arch hook.
>>
>> IMO the best option here is extend domain_teardown() with an
>> arch_domain_teardown() state/hook, and wire in the TEE cleanup path into
>> this too.
>>
>> Anything else is explicitly adding to technical debt that I (or someone
>> else) is going to have to revert further down the line.
>>
>> If you want, I am happy to prototype the arch_domain_teardown() bit of
>> the fix, but I will have to defer wiring in the TEE part to someone
>> capable of testing it.
>
> You're more than welcome to prototype the fix, I can test it and add
> it to the next version of the patch set when we're happy with the
> result.
Could you tell us if you are still happy to work on the prototype for
arch_domain_teardown and when you would be able to give a first prototype ?
Regards
Bertrand
Re: [XEN PATCH v8 02/22] xen/arm: tee: add a primitive FF-A mediator
Hi,
On Thu, Apr 13, 2023 at 3:27 PM Andrew Cooper wrote:
>
> On 13/04/2023 1:26 pm, Julien Grall wrote:
> >> +static int ffa_domain_init(struct domain *d)
> >> +{
> >> +struct ffa_ctx *ctx;
> >> +
> >> +if ( !ffa_version )
> >> +return -ENODEV;
> >> +
> >> +ctx = xzalloc(struct ffa_ctx);
> >> +if ( !ctx )
> >> +return -ENOMEM;
> >> +
> >> +d->arch.tee = ctx;
> >> +
> >> +return 0;
> >> +}
> >> +
> >> +/* This function is supposed to undo what ffa_domain_init() has done */
> >
> > I think there is a problem in the TEE framework. The callback
> > .relinquish_resources() will not be called if domain_create() failed.
> > So this will result to a memory leak.
> >
> > We also can't call .relinquish_resources() on early domain creation
> > failure because relinquishing resources can take time and therefore
> > needs to be preemptible.
> >
> > So I think we need to introduce a new callback domain_free() that will
> > be called arch_domain_destroy(). Is this something you can look at?
>
>
> Cleanup of an early domain creation failure, however you do it, is at
> most "the same amount of time again". It cannot (absent of development
> errors) take the same indefinite time periods of time that a full
> domain_destroy() can.
>
> The error path in domain_create() explicitly does call domain_teardown()
> so we can (eventually) purge these duplicate cleanup paths. There are
> far too many easy errors to be made which occur from having split
> cleanup, and we have had to issue XSAs in the past to address some of
> them. (Hence the effort to try and specifically change things, and
> remove the ability to introduce the errors in the first place.)
>
>
> Right now, it is specifically awkward to do this nicely because
> domain_teardown() doesn't call into a suitable arch hook.
>
> IMO the best option here is extend domain_teardown() with an
> arch_domain_teardown() state/hook, and wire in the TEE cleanup path into
> this too.
>
> Anything else is explicitly adding to technical debt that I (or someone
> else) is going to have to revert further down the line.
>
> If you want, I am happy to prototype the arch_domain_teardown() bit of
> the fix, but I will have to defer wiring in the TEE part to someone
> capable of testing it.
You're more than welcome to prototype the fix, I can test it and add
it to the next version of the patch set when we're happy with the
result.
Thanks,
Jens
>
> ~Andrew
Re: [XEN PATCH v8 02/22] xen/arm: tee: add a primitive FF-A mediator
On 13/04/2023 1:26 pm, Julien Grall wrote:
>> +static int ffa_domain_init(struct domain *d)
>> +{
>> + struct ffa_ctx *ctx;
>> +
>> + if ( !ffa_version )
>> + return -ENODEV;
>> +
>> + ctx = xzalloc(struct ffa_ctx);
>> + if ( !ctx )
>> + return -ENOMEM;
>> +
>> + d->arch.tee = ctx;
>> +
>> + return 0;
>> +}
>> +
>> +/* This function is supposed to undo what ffa_domain_init() has done */
>
> I think there is a problem in the TEE framework. The callback
> .relinquish_resources() will not be called if domain_create() failed.
> So this will result to a memory leak.
>
> We also can't call .relinquish_resources() on early domain creation
> failure because relinquishing resources can take time and therefore
> needs to be preemptible.
>
> So I think we need to introduce a new callback domain_free() that will
> be called arch_domain_destroy(). Is this something you can look at?
Cleanup of an early domain creation failure, however you do it, is at
most "the same amount of time again". It cannot (absent of development
errors) take the same indefinite time periods of time that a full
domain_destroy() can.
The error path in domain_create() explicitly does call domain_teardown()
so we can (eventually) purge these duplicate cleanup paths. There are
far too many easy errors to be made which occur from having split
cleanup, and we have had to issue XSAs in the past to address some of
them. (Hence the effort to try and specifically change things, and
remove the ability to introduce the errors in the first place.)
Right now, it is specifically awkward to do this nicely because
domain_teardown() doesn't call into a suitable arch hook.
IMO the best option here is extend domain_teardown() with an
arch_domain_teardown() state/hook, and wire in the TEE cleanup path into
this too.
Anything else is explicitly adding to technical debt that I (or someone
else) is going to have to revert further down the line.
If you want, I am happy to prototype the arch_domain_teardown() bit of
the fix, but I will have to defer wiring in the TEE part to someone
capable of testing it.
~Andrew
Re: [XEN PATCH v8 02/22] xen/arm: tee: add a primitive FF-A mediator
Hi Jens,
Mainly reviewing this patch from a Xen PoV. I will leave the others to
review the patch from a spec-compliance PoV.
On 13/04/2023 08:14, Jens Wiklander wrote:
+struct ffa_ctx {
+/* FF-A version used by the guest */
+uint32_t guest_vers;
+};
+
+/* Negotiated FF-A version to use with the SPMC */
+static uint32_t ffa_version __ro_after_init;
Coding style: We tend to add attributes just after the type. E.g.:
static uint32_t __ro_after_init ffa_version;
I can deal with this one on commit if there is nothing else to change.
[...]
+static int ffa_domain_init(struct domain *d)
+{
+struct ffa_ctx *ctx;
+
+if ( !ffa_version )
+return -ENODEV;
+
+ctx = xzalloc(struct ffa_ctx);
+if ( !ctx )
+return -ENOMEM;
+
+d->arch.tee = ctx;
+
+return 0;
+}
+
+/* This function is supposed to undo what ffa_domain_init() has done */
I think there is a problem in the TEE framework. The callback
.relinquish_resources() will not be called if domain_create() failed. So
this will result to a memory leak.
We also can't call .relinquish_resources() on early domain creation
failure because relinquishing resources can take time and therefore
needs to be preemptible.
So I think we need to introduce a new callback domain_free() that will
be called arch_domain_destroy(). Is this something you can look at?
+static int ffa_relinquish_resources(struct domain *d)
+{
+struct ffa_ctx *ctx = d->arch.tee;
+
+if ( !ctx )
+return 0;
+
+XFREE(d->arch.tee);
+
+return 0;
+}
Cheers,
--
Julien Grall
[XEN PATCH v8 02/22] xen/arm: tee: add a primitive FF-A mediator
Adds a FF-A version 1.1 [1] mediator to communicate with a Secure
Partition in secure world.
This commit brings in only the parts needed to negotiate FF-A version
number with guest and SPMC.
[1] https://developer.arm.com/documentation/den0077/e
Signed-off-by: Jens Wiklander
---
xen/arch/arm/include/asm/psci.h| 4 +
xen/arch/arm/include/asm/tee/ffa.h | 35 +
xen/arch/arm/tee/Kconfig | 11 ++
xen/arch/arm/tee/Makefile | 1 +
xen/arch/arm/tee/ffa.c | 219 +
xen/arch/arm/vsmc.c| 17 ++-
xen/include/public/arch-arm.h | 1 +
7 files changed, 285 insertions(+), 3 deletions(-)
create mode 100644 xen/arch/arm/include/asm/tee/ffa.h
create mode 100644 xen/arch/arm/tee/ffa.c
diff --git a/xen/arch/arm/include/asm/psci.h b/xen/arch/arm/include/asm/psci.h
index 832f77afff3a..4780972621bb 100644
--- a/xen/arch/arm/include/asm/psci.h
+++ b/xen/arch/arm/include/asm/psci.h
@@ -24,6 +24,10 @@ void call_psci_cpu_off(void);
void call_psci_system_off(void);
void call_psci_system_reset(void);
+/* Range of allocated PSCI function numbers */
+#definePSCI_FNUM_MIN_VALUE _AC(0,U)
+#definePSCI_FNUM_MAX_VALUE _AC(0x1f,U)
+
/* PSCI v0.2 interface */
#define PSCI_0_2_FN32(nr) ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
ARM_SMCCC_CONV_32, \
diff --git a/xen/arch/arm/include/asm/tee/ffa.h
b/xen/arch/arm/include/asm/tee/ffa.h
new file mode 100644
index ..44361a4e78e4
--- /dev/null
+++ b/xen/arch/arm/include/asm/tee/ffa.h
@@ -0,0 +1,35 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * xen/arch/arm/include/asm/tee/ffa.h
+ *
+ * Arm Firmware Framework for ARMv8-A(FFA) mediator
+ *
+ * Copyright (C) 2023 Linaro Limited
+ */
+
+#ifndef __ASM_ARM_TEE_FFA_H__
+#define __ASM_ARM_TEE_FFA_H__
+
+#include
+#include
+
+#include
+#include
+
+#define FFA_FNUM_MIN_VALUE _AC(0x60,U)
+#define FFA_FNUM_MAX_VALUE _AC(0x86,U)
+
+static inline bool is_ffa_fid(uint32_t fid)
+{
+uint32_t fn = fid & ARM_SMCCC_FUNC_MASK;
+
+return fn >= FFA_FNUM_MIN_VALUE && fn <= FFA_FNUM_MAX_VALUE;
+}
+
+#ifdef CONFIG_FFA
+#define FFA_NR_FUNCS12
+#else
+#define FFA_NR_FUNCS0
+#endif
+
+#endif /*__ASM_ARM_TEE_FFA_H__*/
diff --git a/xen/arch/arm/tee/Kconfig b/xen/arch/arm/tee/Kconfig
index 392169b2559d..923f08ba8cb7 100644
--- a/xen/arch/arm/tee/Kconfig
+++ b/xen/arch/arm/tee/Kconfig
@@ -8,3 +8,14 @@ config OPTEE
virtualization-enabled OP-TEE present. You can learn more
about virtualization for OP-TEE at
https://optee.readthedocs.io/architecture/virtualization.html
+
+config FFA
+ bool "Enable FF-A mediator support (UNSUPPORTED)" if UNSUPPORTED
+ default n
+ depends on ARM_64
+ help
+ This option enables a minimal FF-A mediator. The mediator is
+ generic as it follows the FF-A specification [1], but it only
+ implements a small subset of the specification.
+
+ [1] https://developer.arm.com/documentation/den0077/latest
diff --git a/xen/arch/arm/tee/Makefile b/xen/arch/arm/tee/Makefile
index 982c87968447..58a1015e40e0 100644
--- a/xen/arch/arm/tee/Makefile
+++ b/xen/arch/arm/tee/Makefile
@@ -1,2 +1,3 @@
+obj-$(CONFIG_FFA) += ffa.o
obj-y += tee.o
obj-$(CONFIG_OPTEE) += optee.o
diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
new file mode 100644
index ..aaf74c287aef
--- /dev/null
+++ b/xen/arch/arm/tee/ffa.c
@@ -0,0 +1,219 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * xen/arch/arm/tee/ffa.c
+ *
+ * Arm Firmware Framework for ARMv8-A (FF-A) mediator
+ *
+ * Copyright (C) 2023 Linaro Limited
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+#include
+#include
+#include
+
+/* Error codes */
+#define FFA_RET_OK 0
+#define FFA_RET_NOT_SUPPORTED -1
+#define FFA_RET_INVALID_PARAMETERS -2
+#define FFA_RET_NO_MEMORY -3
+#define FFA_RET_BUSY-4
+#define FFA_RET_INTERRUPTED -5
+#define FFA_RET_DENIED -6
+#define FFA_RET_RETRY -7
+#define FFA_RET_ABORTED -8
+
+/* FFA_VERSION helpers */
+#define FFA_VERSION_MAJOR_SHIFT 16U
+#define FFA_VERSION_MAJOR_MASK 0x7FFFU
+#define FFA_VERSION_MINOR_SHIFT 0U
+#define FFA_VERSION_MINOR_MASK 0xU
+#define MAKE_FFA_VERSION(major, minor) \
+major) & FFA_VERSION_MAJOR_MASK) << FFA_VERSION_MAJOR_SHIFT) | \
+ ((minor) & FFA_VERSION_MINOR_MASK))
+
+#define FFA_VERSION_1_0 MAKE_FFA_VERSION(1, 0)
+#define FFA_VERSION_1_1 MAKE_FFA_VERSION(1, 1)
+/* The minimal FF-A version of the SPMC that can be supported */
+#define FFA_MIN_SPMC_VERSIONFFA_VERSION_1_1
+
+/*
+ * This is the version
