[XEN PATCH v8 00/22] Xen FF-A mediator

[XEN PATCH v8 00/22] Xen FF-A mediator

[Jens Wiklander]

Hi,

This patch sets add an FF-A [1] mediator to the TEE mediator framework
already present in Xen.  The FF-A mediator implements the subset of the
FF-A 1.1 specification needed to communicate with OP-TEE using FF-A as
transport mechanism instead of SMC/HVC as with the TEE mediator. It allows
a similar design in OP-TEE as with the TEE mediator where OP-TEE presents
one virtual partition of itself to each guest in Xen.

The FF-A mediator is generic in the sense it has nothing OP-TEE specific
except that only the subset needed for OP-TEE is implemented so far. The
hooks needed to inform OP-TEE that a guest is created or destroyed are part
of the FF-A specification.

It should be possible to extend the FF-A mediator to implement a larger
portion of the FF-A 1.1 specification without breaking with the way OP-TEE
is communicated with here. So it should be possible to support any TEE or
Secure Partition using FF-A as transport with this mediator.

The patches are also available at https://github.com/jenswi-linaro/xen
branch "xen_ffa_v8".

With help from Bertrand I've integrated this in a test setup with OP-TEE.
Please check prerequisites at
https://optee.readthedocs.io/en/latest/building/prerequisites.html

My setup is duplicated using:
repo init -u https://github.com/jenswi-linaro/manifest.git -m qemu_v8.xml \
        -b qemu_xen_ffa
repo sync -j8
cd build
make -j8 toolchains
make -j8 all
make run-only

Test in dom0 with for instance:
xtest 1004

at the prompt.

To start up a domu and connect to it do:
cd /mnt/host/build/qemu_v8/xen
xl create guest_ffa.cfg
xl console domu

Then test as usual with "xtest 1004".

The setup uses the branch "ffa" from https://github.com/jenswi-linaro/xen.
That's currently the same as the "xen_ffa_v8" branch, but the "ffa" branch
may change later as I update for a new version of the patch set.

[1] https://developer.arm.com/documentation/den0077/latest

Thanks,
Jens

v7->v8:
* Adding "xen/arm: ffa: list current limitations" as requested
* Adding tags to "xen/arm: smccc: add support for SMCCCv1.2 extended
  input/output registers"
* Patch "xen/arm: tee: add a primitive FF-A mediator":
  - Changing license for ffa.h and ffa.c to GPL-2.0-only
  - Avoiding IS_ENABLED() in the constant FFA_NR_FUNCS
  - Accepting version 1.1 SPMC only to keep things simple
  - Removes 32bit and only supports 64bit to keep things simple
* Patch "tools: add Arm FF-A mediator"
  - Adding Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
  - Adding LIBXL_HAVE_BUILDINFO_ARCH_ARM_TEE_FFA for the "ffa" value 
    in arch_arm.tee
* Patch "docs: add Arm FF-A mediator"
  - Fixing a spell error
  - Moving the patch last in the series
* Patch "xen/arm: ffa: add remaining SMC function IDs"
  - Adding Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
  - Renaming the define FFA_MSG_RUN to FFA_RUN to match the specification
* Patch "xen/arm: ffa: add flags for FFA_PARTITION_INFO_GET"
  - Updating the comment describing the flags for FFA_PARTITION_INFO_GET
* Patch "xen/arm: ffa: add defines for framework direct request/response
  messages"
  - Updating the comment describing the flags for MSG_SEND_DIRECT_REQ/RESP
* Patch "xen/arm: ffa: enforce dependency on 4k pages"
  - Updating title of patch
  - Adding Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
* Patch "xen/arm: ffa: add support for FFA_ID_GET"
  - In ffa_domain_init(), check that domain_id isn't greater than
    UINT16_MAX to avoid a future potential integer overflow in get_vm_id()
* Patch "xen/arm: ffa: add direct request support"
  - Move preemption (interrupted) parts to a separate patch "xen/arm: ffa:
    support preemption of SP during direct request"
  - Remove loop in handle_msg_send_direct_req() to return eventual
    errors back to the VM instead of the SP.
* Patch "xen/arm: ffa: map SPMC rx/tx buffers"
  - Adding a FFA_RXTX_PAGE_COUNT define instead of using 1 directly
* New patch "xen/arm: ffa: support preemption of SP during direct request"
* Patch "xen/arm: ffa: send guest events to Secure Partitions"
  - Replacing unsigned int with uint16_t for subscr_vm_created_count and
    subscr_vm_destroyed_count plus the needed range check to see that
    they don't overflow.
* Patch "xen/arm: ffa: support mapping guest RX/TX buffers"
  - Limit the number of pages in VM RX/TX buffers to 32 using a new
    FFA_MAX_RXTX_PAGE_COUNT define.
* Patch "xen/arm: ffa: support guest FFA_PARTITION_INFO_GET"
  - Renaming tx_is_mine to rx_is_free as requested
  - Simplified the FFA_PARTITION_INFO_GET_COUNT_FLAG check in
    handle_partition_info_get()
  - Adding a comment on ownership of the RX buffer
  - Adding the patch "xen/arm: ffa: improve lock granularity" to address
    parts of the locking concerns.
* Patch "xen/arm: move regpair_to_uint64() and uint64_to_regpair() to regs.h"
  - Adding Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
* Patch "xen/arm: ffa: add defines for sharing memory"
  - Fixing reference for FFA_NORMAL_MEM_REG_ATTR and FFA_MEM_ACC_RW
  - Updating descirption for FFA_MAX_SHM_PAGE_COUNT
* Patch "xen/arm: ffa: add ABI structs for sharing memory"
  - Changing name of the "global_handle" memeber in struct
    ffa_mem_transaction_* to "handle".
* Patch "xen/arm: ffa: support sharing memory"
  - Use FFA_MEM_SHARE_64 only since we changed to only suporting ARM_64.
  - Rename struct ffa_mem_transaction_x to struct ffa_mem_transaction_int
    as requested.
  - Adding a check that shm->page_count isn't 0 before calling share_shm()
  - Masking return value from FFA_MEM_FRAG_RX to avoid an implic cast to
    the int32_t returned by ffa_mem_share().
* Patch "xen/arm: ffa: add support to reclaim shared memory"
  - Adding Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
* Patch "xen/arm: ffa: support sharing large memory ranges"
  - Adding commetns for struct ffa_ctx
  - Cleaning up and removing the fragmentation state if handle_mem_frag_tx()
    detects an error.
* Adding "xen/arm: ffa: improve lock granularity" to address some of the
  locking concerns.

v6->v7:
* Split some of the larger patches into smaller patches for easier review.
  For instance, the v6 patch "xen/arm: add a primitive FF-A mediator" has
  been replaced with:
  - "xen/arm: add a primitive FF-A mediator"
  - "tools: add Arm FF-A mediator"
  - "docs: add Arm FF-A mediator"
  - "xen/arm: ffa: add remaining SMC function IDs"
* Some small fixes in the error path for handle_mem_share()
* Switched to SPDX for license in new files.
* Fixed comment style issues in
  "xen/arm: smccc: add support for SMCCCv1.2 extended input/output registers"
* Made FFA support UNSUPPORTED in "xen/arm: add a primitive FF-A mediator"
* Replaced ffa_get_call_count() with FFA_NR_FUNCS
* Update the FFA_MAX_SHM_PAGE_COUNT with a formula instead of a value.
* Replaced XEN_ARM_FLAGS_FFA with XEN_DOMCTL_CONFIG_TEE_FFA to minimize impact
  on struct xen_arch_domainconfig. This works because the FF-A mediator and
  the OP-TEE mediator will not be used at the same time in by a guest.
* Replaced "ffa" boolean in the guest config with a new "ffa" value to the
  enumeration "tee_type".
* Integrated the FF-A mediator in the TEE mediator framework instead of
  being its own.
* Rebased on staging as of 2023-02-16

v5->v6:
* Updated "xen/arm: move regpair_to_uint64() and uint64_to_regpair() to regs.h"
  commit message and moved the patch right before the patch which needs it.
  Applied Michal Orzel's R-B tag.
* Renamed the guest configuration option "ffa_enabled" to "ffa" and
  updated the description.
* More tools update in "xen/arm: add a primitive FF-A mediator" with the "ffa"
  option, including golang and ocaml.
* Update ffa_domain_init() to return an error if communication with
  the SPMC can't be established.
* Factored out a ffa_domain_destroy() from ffa_relinquish_resources().
* Added ffa_get_call_count() to give an accurate number of FF-A function,
  updated in each patch as new FF-A functions are added.
* Added a flags field in struct xen_arch_domainconfig that replaces the
  ffa_enabled field.
* Made check_mandatory_feature() __init
* Replaced a few printk() calls with gprintk() where needed.
* Rebased on staging as of 2022-09-14

V4->v5:
* Added "xen/arm: move regpair_to_uint64() and uint64_to_regpair() to regs.h"
* Added documentation for the "ffa_enabled" guest config flag
* Changed to GPL license for xen/arch/arm/ffa.c
* Added __read_mostly and const where applicable
* Added more describing comments in the code
* Moved list of shared memory object ("ffa_mem_list") into the guest context
  as they are guest specific
* Simplified a few of the simple wrapper functions for SMC to SPMC
* Added a BUILD_BUG_ON(PAGE_SIZE != FFA_PAGE_SIZE) since the mediator
  currently depends on the page size to be same as FFA_PAGE_SIZE (4k).
* Added max number of shared memory object per guest and max number of
  size of each shared memory object
* Added helper macros to calculate offsets of different FF-A data structures
  in the communication buffer instead of relying on pointer arithmetic
* Addressed style issues and other comments
* Broke the commit "xen/arm: add FF-A mediator" into multiple parts, trying
  to add a few features at a time as requested
* Added a missing call to rxtx_unmap() in ffa_relinquish_resources()
* Assignment of "ffa_enabled" is kept as is until I have something definitive
  on the type etc.
* Tested with CONFIG_DEBUG=y

v3->v4:
* Missed v3 and sent a v4 instead by mistake.

v2->v3:
* Generates offsets into struct arm_smccc_1_2_regs with asm-offsets.c in
  order to avoid hard coded offsets in the assembly function
  arm_smccc_1_2_smc()
* Adds an entry in SUPPORT.md on the FF-A status
* Adds a configuration variable "ffa_enabled" to tell if FF-A should be
  enabled for a particular domu guest
* Moves the ffa_frag_list for fragmented memory share requests into
  struct ffa_ctx instead to keep it per guest in order to avoid mixups
  and simplify locking
* Adds a spinlock to struct ffa_ctx for per guest locking
* Addressing style issues and suggestions
* Uses FFA_FEATURES to check that all the needed features are available
  before initializing the mediator
* Rebased on staging as of 2022-06-20

v1->v2:
* Rebased on staging to resolve some merge conflicts as requested



Jens Wiklander (22):
  xen/arm: smccc: add support for SMCCCv1.2 extended input/output
    registers
  xen/arm: tee: add a primitive FF-A mediator
  tools: add Arm FF-A mediator
  xen/arm: ffa: add remaining SMC function IDs
  xen/arm: ffa: add flags for FFA_PARTITION_INFO_GET
  xen/arm: ffa: add defines for framework direct request/response
    messages
  xen/arm: ffa: enforce dependency on 4k pages
  xen/arm: ffa: add support for FFA_ID_GET
  xen/arm: ffa: add direct request support
  xen/arm: ffa: map SPMC rx/tx buffers
  xen/arm: ffa: send guest events to Secure Partitions
  xen/arm: ffa: support mapping guest RX/TX buffers
  xen/arm: ffa: support guest FFA_PARTITION_INFO_GET
  xen/arm: move regpair_to_uint64() and uint64_to_regpair() to regs.h
  xen/arm: ffa: add defines for sharing memory
  xen/arm: ffa: add ABI structs for sharing memory
  xen/arm: ffa: support sharing memory
  xen/arm: ffa: add support to reclaim shared memory
  xen/arm: ffa: support sharing large memory ranges
  xen/arm: ffa: improve lock granularity
  xen/arm: ffa: list current limitations
  docs: add Arm FF-A mediator

 SUPPORT.md                         |    8 +
 docs/man/xl.cfg.5.pod.in           |   15 +
 tools/include/libxl.h              |    5 +
 tools/libs/light/libxl_arm.c       |    3 +
 tools/libs/light/libxl_types.idl   |    3 +-
 xen/arch/arm/arm64/asm-offsets.c   |    9 +
 xen/arch/arm/arm64/smc.S           |   42 +
 xen/arch/arm/include/asm/psci.h    |    4 +
 xen/arch/arm/include/asm/regs.h    |   12 +
 xen/arch/arm/include/asm/smccc.h   |   40 +
 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             | 1950 ++++++++++++++++++++++++++++
 xen/arch/arm/tee/optee.c           |   11 -
 xen/arch/arm/vsmc.c                |   19 +-
 xen/include/public/arch-arm.h      |    1 +
 17 files changed, 2153 insertions(+), 16 deletions(-)
 create mode 100644 xen/arch/arm/include/asm/tee/ffa.h
 create mode 100644 xen/arch/arm/tee/ffa.c

-- 
2.34.1

[XEN PATCH v8 01/22] xen/arm: smccc: add support for SMCCCv1.2 extended input/output registers

[Jens Wiklander]

SMCCC v1.2 [1] AArch64 allows x0-x17 to be used as both parameter
registers and result registers for the SMC and HVC instructions.

Arm Firmware Framework for Armv8-A specification makes use of x0-x7 as
parameter and result registers.

Let us add new interface to support this extended set of input/output
registers.

This is based on 3fdc0cb59d97 ("arm64: smccc: Add support for SMCCCv1.2
extended input/output registers") by Sudeep Holla from the Linux kernel

The SMCCC version reported to the VM is bumped to 1.2 in order to support
handling FF-A messages.

[1] https://developer.arm.com/documentation/den0028/c/?lang=en

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
Acked-by: Julien Grall <jgrall@amazon.com>
---
 xen/arch/arm/arm64/asm-offsets.c |  9 +++++++
 xen/arch/arm/arm64/smc.S         | 42 ++++++++++++++++++++++++++++++++
 xen/arch/arm/include/asm/smccc.h | 40 ++++++++++++++++++++++++++++++
 xen/arch/arm/vsmc.c              |  2 +-
 4 files changed, 92 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/arm64/asm-offsets.c b/xen/arch/arm/arm64/asm-offsets.c
index 7226cd9b2eb0..7adb67a1b81a 100644
--- a/xen/arch/arm/arm64/asm-offsets.c
+++ b/xen/arch/arm/arm64/asm-offsets.c
@@ -57,6 +57,15 @@ void __dummy__(void)
    BLANK();
    OFFSET(SMCCC_RES_a0, struct arm_smccc_res, a0);
    OFFSET(SMCCC_RES_a2, struct arm_smccc_res, a2);
+   OFFSET(ARM_SMCCC_1_2_REGS_X0_OFFS, struct arm_smccc_1_2_regs, a0);
+   OFFSET(ARM_SMCCC_1_2_REGS_X2_OFFS, struct arm_smccc_1_2_regs, a2);
+   OFFSET(ARM_SMCCC_1_2_REGS_X4_OFFS, struct arm_smccc_1_2_regs, a4);
+   OFFSET(ARM_SMCCC_1_2_REGS_X6_OFFS, struct arm_smccc_1_2_regs, a6);
+   OFFSET(ARM_SMCCC_1_2_REGS_X8_OFFS, struct arm_smccc_1_2_regs, a8);
+   OFFSET(ARM_SMCCC_1_2_REGS_X10_OFFS, struct arm_smccc_1_2_regs, a10);
+   OFFSET(ARM_SMCCC_1_2_REGS_X12_OFFS, struct arm_smccc_1_2_regs, a12);
+   OFFSET(ARM_SMCCC_1_2_REGS_X14_OFFS, struct arm_smccc_1_2_regs, a14);
+   OFFSET(ARM_SMCCC_1_2_REGS_X16_OFFS, struct arm_smccc_1_2_regs, a16);
 }
 
 /*
diff --git a/xen/arch/arm/arm64/smc.S b/xen/arch/arm/arm64/smc.S
index 91bae62dd4d2..fc6b676e2ee3 100644
--- a/xen/arch/arm/arm64/smc.S
+++ b/xen/arch/arm/arm64/smc.S
@@ -27,3 +27,45 @@ ENTRY(__arm_smccc_1_0_smc)
         stp     x2, x3, [x4, #SMCCC_RES_a2]
 1:
         ret
+
+/*
+ * void arm_smccc_1_2_smc(const struct arm_smccc_1_2_regs *args,
+ *                        struct arm_smccc_1_2_regs *res)
+ */
+ENTRY(arm_smccc_1_2_smc)
+    /* Save `res` and free a GPR that won't be clobbered by SMC call */
+    stp     x1, x19, [sp, #-16]!
+
+    /* Ensure `args` won't be clobbered while loading regs in next step */
+    mov	x19, x0
+
+    /* Load the registers x0 - x17 from the struct arm_smccc_1_2_regs */
+    ldp	x0, x1, [x19, #ARM_SMCCC_1_2_REGS_X0_OFFS]
+    ldp	x2, x3, [x19, #ARM_SMCCC_1_2_REGS_X2_OFFS]
+    ldp	x4, x5, [x19, #ARM_SMCCC_1_2_REGS_X4_OFFS]
+    ldp	x6, x7, [x19, #ARM_SMCCC_1_2_REGS_X6_OFFS]
+    ldp	x8, x9, [x19, #ARM_SMCCC_1_2_REGS_X8_OFFS]
+    ldp	x10, x11, [x19, #ARM_SMCCC_1_2_REGS_X10_OFFS]
+    ldp	x12, x13, [x19, #ARM_SMCCC_1_2_REGS_X12_OFFS]
+    ldp	x14, x15, [x19, #ARM_SMCCC_1_2_REGS_X14_OFFS]
+    ldp	x16, x17, [x19, #ARM_SMCCC_1_2_REGS_X16_OFFS]
+
+    smc #0
+
+    /* Load the `res` from the stack */
+    ldr	x19, [sp]
+
+    /* Store the registers x0 - x17 into the result structure */
+    stp	x0, x1, [x19, #ARM_SMCCC_1_2_REGS_X0_OFFS]
+    stp	x2, x3, [x19, #ARM_SMCCC_1_2_REGS_X2_OFFS]
+    stp	x4, x5, [x19, #ARM_SMCCC_1_2_REGS_X4_OFFS]
+    stp	x6, x7, [x19, #ARM_SMCCC_1_2_REGS_X6_OFFS]
+    stp	x8, x9, [x19, #ARM_SMCCC_1_2_REGS_X8_OFFS]
+    stp	x10, x11, [x19, #ARM_SMCCC_1_2_REGS_X10_OFFS]
+    stp	x12, x13, [x19, #ARM_SMCCC_1_2_REGS_X12_OFFS]
+    stp	x14, x15, [x19, #ARM_SMCCC_1_2_REGS_X14_OFFS]
+    stp	x16, x17, [x19, #ARM_SMCCC_1_2_REGS_X16_OFFS]
+
+    /* Restore original x19 */
+    ldp     xzr, x19, [sp], #16
+    ret
diff --git a/xen/arch/arm/include/asm/smccc.h b/xen/arch/arm/include/asm/smccc.h
index b3dbeecc90ad..1adcd37443c7 100644
--- a/xen/arch/arm/include/asm/smccc.h
+++ b/xen/arch/arm/include/asm/smccc.h
@@ -33,6 +33,7 @@
 
 #define ARM_SMCCC_VERSION_1_0   SMCCC_VERSION(1, 0)
 #define ARM_SMCCC_VERSION_1_1   SMCCC_VERSION(1, 1)
+#define ARM_SMCCC_VERSION_1_2   SMCCC_VERSION(1, 2)
 
 /*
  * This file provides common defines for ARM SMC Calling Convention as
@@ -265,6 +266,45 @@ void __arm_smccc_1_0_smc(register_t a0, register_t a1, register_t a2,
         else                                                    \
             arm_smccc_1_0_smc(__VA_ARGS__);                     \
     } while ( 0 )
+
+/*
+ * struct arm_smccc_1_2_regs - Arguments for or Results from SMC call
+ * @a0-a17 argument values from registers 0 to 17
+ */
+struct arm_smccc_1_2_regs {
+    unsigned long a0;
+    unsigned long a1;
+    unsigned long a2;
+    unsigned long a3;
+    unsigned long a4;
+    unsigned long a5;
+    unsigned long a6;
+    unsigned long a7;
+    unsigned long a8;
+    unsigned long a9;
+    unsigned long a10;
+    unsigned long a11;
+    unsigned long a12;
+    unsigned long a13;
+    unsigned long a14;
+    unsigned long a15;
+    unsigned long a16;
+    unsigned long a17;
+};
+
+/*
+ * arm_smccc_1_2_smc() - make SMC calls
+ * @args: arguments passed via struct arm_smccc_1_2_regs
+ * @res: result values via struct arm_smccc_1_2_regs
+ *
+ * This function is used to make SMC calls following SMC Calling Convention
+ * v1.2 or above. The content of the supplied param are copied from the
+ * structure to registers prior to the SMC instruction. The return values
+ * are updated with the content from registers on return from the SMC
+ * instruction.
+ */
+void arm_smccc_1_2_smc(const struct arm_smccc_1_2_regs *args,
+                       struct arm_smccc_1_2_regs *res);
 #endif /* CONFIG_ARM_64 */
 
 #endif /* __ASSEMBLY__ */
diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
index 7335276f3fa1..cd68fa80e98a 100644
--- a/xen/arch/arm/vsmc.c
+++ b/xen/arch/arm/vsmc.c
@@ -85,7 +85,7 @@ static bool handle_arch(struct cpu_user_regs *regs)
     switch ( fid )
     {
     case ARM_SMCCC_VERSION_FID:
-        set_user_reg(regs, 0, ARM_SMCCC_VERSION_1_1);
+        set_user_reg(regs, 0, ARM_SMCCC_VERSION_1_2);
         return true;
 
     case ARM_SMCCC_ARCH_FEATURES_FID:
-- 
2.34.1

[XEN PATCH v8 02/22] xen/arm: tee: add a primitive FF-A mediator

[Jens Wiklander]

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 <jens.wiklander@linaro.org>
---
 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 */
+#define	PSCI_FNUM_MIN_VALUE                 _AC(0,U)
+#define	PSCI_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 000000000000..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 <xen/const.h>
+#include <xen/kconfig.h>
+
+#include <asm/smccc.h>
+#include <asm/types.h>
+
+#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_FUNCS    12
+#else
+#define FFA_NR_FUNCS    0
+#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 000000000000..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 <xen/bitops.h>
+#include <xen/domain_page.h>
+#include <xen/errno.h>
+#include <xen/init.h>
+#include <xen/lib.h>
+#include <xen/sched.h>
+#include <xen/sizes.h>
+#include <xen/types.h>
+
+#include <asm/event.h>
+#include <asm/regs.h>
+#include <asm/smccc.h>
+#include <asm/tee/ffa.h>
+#include <asm/tee/tee.h>
+
+/* 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          0xFFFFU
+#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_VERSION    FFA_VERSION_1_1
+
+/*
+ * This is the version we want to use in communication with guests and SPs.
+ * During negotiation with a guest or a SP we may need to lower it for
+ * that particular guest or SP.
+ */
+#define FFA_MY_VERSION_MAJOR    1U
+#define FFA_MY_VERSION_MINOR    1U
+#define FFA_MY_VERSION          MAKE_FFA_VERSION(FFA_MY_VERSION_MAJOR, \
+                                                 FFA_MY_VERSION_MINOR)
+
+/* Function IDs */
+#define FFA_ERROR                       0x84000060U
+#define FFA_SUCCESS_32                  0x84000061U
+#define FFA_VERSION                     0x84000063U
+
+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;
+
+static bool ffa_get_version(uint32_t *vers)
+{
+    const struct arm_smccc_1_2_regs arg = {
+        .a0 = FFA_VERSION,
+        .a1 = FFA_MY_VERSION,
+    };
+    struct arm_smccc_1_2_regs resp;
+
+    arm_smccc_1_2_smc(&arg, &resp);
+    if ( resp.a0 == FFA_RET_NOT_SUPPORTED )
+    {
+        gprintk(XENLOG_ERR, "ffa: FFA_VERSION returned not supported\n");
+        return false;
+    }
+
+    *vers = resp.a0;
+
+    return true;
+}
+
+static void set_regs(struct cpu_user_regs *regs, register_t v0, register_t v1,
+                     register_t v2, register_t v3, register_t v4, register_t v5,
+                     register_t v6, register_t v7)
+{
+        set_user_reg(regs, 0, v0);
+        set_user_reg(regs, 1, v1);
+        set_user_reg(regs, 2, v2);
+        set_user_reg(regs, 3, v3);
+        set_user_reg(regs, 4, v4);
+        set_user_reg(regs, 5, v5);
+        set_user_reg(regs, 6, v6);
+        set_user_reg(regs, 7, v7);
+}
+
+static void handle_version(struct cpu_user_regs *regs)
+{
+    struct domain *d = current->domain;
+    struct ffa_ctx *ctx = d->arch.tee;
+    uint32_t vers = get_user_reg(regs, 1);
+
+    if ( vers < FFA_VERSION_1_1 )
+        vers = FFA_VERSION_1_0;
+    else
+        vers = FFA_VERSION_1_1;
+
+    ctx->guest_vers = vers;
+    set_regs(regs, vers, 0, 0, 0, 0, 0, 0, 0);
+}
+
+static bool ffa_handle_call(struct cpu_user_regs *regs)
+{
+    uint32_t fid = get_user_reg(regs, 0);
+    struct domain *d = current->domain;
+    struct ffa_ctx *ctx = d->arch.tee;
+
+    if ( !ctx )
+        return false;
+
+    switch ( fid )
+    {
+    case FFA_VERSION:
+        handle_version(regs);
+        return true;
+
+    default:
+        gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid);
+        return false;
+    }
+}
+
+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 */
+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;
+}
+
+static bool ffa_probe(void)
+{
+    uint32_t vers;
+    unsigned int major_vers;
+    unsigned int minor_vers;
+
+    /*
+     * psci_init_smccc() updates this value with what's reported by EL-3
+     * or secure world.
+     */
+    if ( smccc_ver < ARM_SMCCC_VERSION_1_2 )
+    {
+        printk(XENLOG_ERR
+               "ffa: unsupported SMCCC version %#x (need at least %#x)\n",
+               smccc_ver, ARM_SMCCC_VERSION_1_2);
+        return false;
+    }
+
+    if ( !ffa_get_version(&vers) )
+        return false;
+
+    if ( vers < FFA_MIN_SPMC_VERSION || vers > FFA_MY_VERSION )
+    {
+        printk(XENLOG_ERR "ffa: Incompatible version %#x found\n", vers);
+        return false;
+    }
+
+    major_vers = (vers >> FFA_VERSION_MAJOR_SHIFT) & FFA_VERSION_MAJOR_MASK;
+    minor_vers = vers & FFA_VERSION_MINOR_MASK;
+    printk(XENLOG_INFO "ARM FF-A Mediator version %u.%u\n",
+           FFA_MY_VERSION_MAJOR, FFA_MY_VERSION_MINOR);
+    printk(XENLOG_INFO "ARM FF-A Firmware version %u.%u\n",
+           major_vers, minor_vers);
+
+    ffa_version = vers;
+
+    return true;
+}
+
+static const struct tee_mediator_ops ffa_ops =
+{
+    .probe = ffa_probe,
+    .domain_init = ffa_domain_init,
+    .relinquish_resources = ffa_relinquish_resources,
+    .handle_call = ffa_handle_call,
+};
+
+REGISTER_TEE_MEDIATOR(ffa, "FF-A", XEN_DOMCTL_CONFIG_TEE_FFA, &ffa_ops);
diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
index cd68fa80e98a..7f2f5eb9ce3d 100644
--- a/xen/arch/arm/vsmc.c
+++ b/xen/arch/arm/vsmc.c
@@ -15,6 +15,7 @@
 #include <asm/monitor.h>
 #include <asm/regs.h>
 #include <asm/smccc.h>
+#include <asm/tee/ffa.h>
 #include <asm/tee/tee.h>
 #include <asm/traps.h>
 #include <asm/vpsci.h>
@@ -24,7 +25,7 @@
 #define XEN_SMCCC_FUNCTION_COUNT 3
 
 /* Number of functions currently supported by Standard Service Service Calls. */
-#define SSSC_SMCCC_FUNCTION_COUNT (3 + VPSCI_NR_FUNCS)
+#define SSSC_SMCCC_FUNCTION_COUNT (3 + VPSCI_NR_FUNCS + FFA_NR_FUNCS)
 
 static bool fill_uid(struct cpu_user_regs *regs, xen_uuid_t uuid)
 {
@@ -188,13 +189,23 @@ static bool handle_existing_apis(struct cpu_user_regs *regs)
     return do_vpsci_0_1_call(regs, fid);
 }
 
+static bool is_psci_fid(uint32_t fid)
+{
+    uint32_t fn = fid & ARM_SMCCC_FUNC_MASK;
+
+    return fn >= PSCI_FNUM_MIN_VALUE && fn <= PSCI_FNUM_MAX_VALUE;
+}
+
 /* PSCI 0.2 interface and other Standard Secure Calls */
 static bool handle_sssc(struct cpu_user_regs *regs)
 {
     uint32_t fid = (uint32_t)get_user_reg(regs, 0);
 
-    if ( do_vpsci_0_2_call(regs, fid) )
-        return true;
+    if ( is_psci_fid(fid) )
+        return do_vpsci_0_2_call(regs, fid);
+
+    if ( is_ffa_fid(fid) )
+        return tee_handle_call(regs);
 
     switch ( fid )
     {
diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index 1528ced5097a..92aff923056a 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -296,6 +296,7 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_guest_context_t);
 
 #define XEN_DOMCTL_CONFIG_TEE_NONE      0
 #define XEN_DOMCTL_CONFIG_TEE_OPTEE     1
+#define XEN_DOMCTL_CONFIG_TEE_FFA       2
 
 struct xen_arch_domainconfig {
     /* IN/OUT */
-- 
2.34.1

[XEN PATCH v8 03/22] tools: add Arm FF-A mediator

[Jens Wiklander]

Adds a new "ffa" value to the Enumeration "tee_type" to indicate if a
guest is trusted to use FF-A.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
---
 tools/include/libxl.h            | 5 +++++
 tools/libs/light/libxl_arm.c     | 3 +++
 tools/libs/light/libxl_types.idl | 3 ++-
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/tools/include/libxl.h b/tools/include/libxl.h
index cfa1a191318c..7c48e8d8472e 100644
--- a/tools/include/libxl.h
+++ b/tools/include/libxl.h
@@ -283,6 +283,11 @@
  */
 #define LIBXL_HAVE_BUILDINFO_ARCH_ARM_TEE 1
 
+/*
+ * arch_arm.tee field in libxl_domain_build_info has ffa value.
+ */
+#define LIBXL_HAVE_BUILDINFO_ARCH_ARM_TEE_FFA 1
+
 /*
  * LIBXL_HAVE_SOFT_RESET indicates that libxl supports performing
  * 'soft reset' for domains and there is 'soft_reset' shutdown reason
diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
index ddc7b2a15975..601890dda1ce 100644
--- a/tools/libs/light/libxl_arm.c
+++ b/tools/libs/light/libxl_arm.c
@@ -205,6 +205,9 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
     case LIBXL_TEE_TYPE_OPTEE:
         config->arch.tee_type = XEN_DOMCTL_CONFIG_TEE_OPTEE;
         break;
+    case LIBXL_TEE_TYPE_FFA:
+        config->arch.tee_type = XEN_DOMCTL_CONFIG_TEE_FFA;
+        break;
     default:
         LOG(ERROR, "Unknown TEE type %d",
             d_config->b_info.tee);
diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
index c10292e0d7e3..1a680d0f8839 100644
--- a/tools/libs/light/libxl_types.idl
+++ b/tools/libs/light/libxl_types.idl
@@ -520,7 +520,8 @@ libxl_gic_version = Enumeration("gic_version", [
 
 libxl_tee_type = Enumeration("tee_type", [
     (0, "none"),
-    (1, "optee")
+    (1, "optee"),
+    (2, "ffa"),
     ], init_val = "LIBXL_TEE_TYPE_NONE")
 
 libxl_rdm_reserve = Struct("rdm_reserve", [
-- 
2.34.1

[XEN PATCH v8 04/22] xen/arm: ffa: add remaining SMC function IDs

[Jens Wiklander]

Adds the remaining SMC function IDs from FF-A 1.1 specification.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
---
 xen/arch/arm/tee/ffa.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
index aaf74c287aef..ba0942e76993 100644
--- a/xen/arch/arm/tee/ffa.c
+++ b/xen/arch/arm/tee/ffa.c
@@ -60,7 +60,41 @@
 /* Function IDs */
 #define FFA_ERROR                       0x84000060U
 #define FFA_SUCCESS_32                  0x84000061U
+#define FFA_SUCCESS_64                  0xC4000061U
+#define FFA_INTERRUPT                   0x84000062U
 #define FFA_VERSION                     0x84000063U
+#define FFA_FEATURES                    0x84000064U
+#define FFA_RX_ACQUIRE                  0x84000084U
+#define FFA_RX_RELEASE                  0x84000065U
+#define FFA_RXTX_MAP_32                 0x84000066U
+#define FFA_RXTX_MAP_64                 0xC4000066U
+#define FFA_RXTX_UNMAP                  0x84000067U
+#define FFA_PARTITION_INFO_GET          0x84000068U
+#define FFA_ID_GET                      0x84000069U
+#define FFA_SPM_ID_GET                  0x84000085U
+#define FFA_MSG_WAIT                    0x8400006BU
+#define FFA_MSG_YIELD                   0x8400006CU
+#define FFA_RUN                         0x8400006DU
+#define FFA_MSG_SEND2                   0x84000086U
+#define FFA_MSG_SEND_DIRECT_REQ_32      0x8400006FU
+#define FFA_MSG_SEND_DIRECT_REQ_64      0xC400006FU
+#define FFA_MSG_SEND_DIRECT_RESP_32     0x84000070U
+#define FFA_MSG_SEND_DIRECT_RESP_64     0xC4000070U
+#define FFA_MEM_DONATE_32               0x84000071U
+#define FFA_MEM_DONATE_64               0xC4000071U
+#define FFA_MEM_LEND_32                 0x84000072U
+#define FFA_MEM_LEND_64                 0xC4000072U
+#define FFA_MEM_SHARE_32                0x84000073U
+#define FFA_MEM_SHARE_64                0xC4000073U
+#define FFA_MEM_RETRIEVE_REQ_32         0x84000074U
+#define FFA_MEM_RETRIEVE_REQ_64         0xC4000074U
+#define FFA_MEM_RETRIEVE_RESP           0x84000075U
+#define FFA_MEM_RELINQUISH              0x84000076U
+#define FFA_MEM_RECLAIM                 0x84000077U
+#define FFA_MEM_FRAG_RX                 0x8400007AU
+#define FFA_MEM_FRAG_TX                 0x8400007BU
+#define FFA_MSG_SEND                    0x8400006EU
+#define FFA_MSG_POLL                    0x8400006AU
 
 struct ffa_ctx {
     /* FF-A version used by the guest */
-- 
2.34.1

[XEN PATCH v8 05/22] xen/arm: ffa: add flags for FFA_PARTITION_INFO_GET

[Jens Wiklander]

Defines flags used for the function FFA_PARTITION_INFO_GET.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
 xen/arch/arm/tee/ffa.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
index ba0942e76993..72e7d0575de5 100644
--- a/xen/arch/arm/tee/ffa.c
+++ b/xen/arch/arm/tee/ffa.c
@@ -57,6 +57,40 @@
 #define FFA_MY_VERSION          MAKE_FFA_VERSION(FFA_MY_VERSION_MAJOR, \
                                                  FFA_MY_VERSION_MINOR)
 
+/*
+ * Flags to determine partition properties in FFA_PARTITION_INFO_GET return
+ * message:
+ * BIT(0): Supports receipt of direct requests
+ * BIT(1): Can send direct requests
+ * BIT(2): Can send and receive indirect messages
+ * BIT(3): Supports receipt of notifications
+ * BIT(4-5): Partition ID is a PE endpoint ID
+ * BIT(6): Partition must be informed about each VM that is created by
+ *         the Hypervisor
+ * BIT(7): Partition must be informed about each VM that is destroyed by
+ *         the Hypervisor
+ * BIT(8): Partition runs in the AArch64 execution state else AArch32
+ *         execution state
+ */
+#define FFA_PART_PROP_DIRECT_REQ_RECV   BIT(0, U)
+#define FFA_PART_PROP_DIRECT_REQ_SEND   BIT(1, U)
+#define FFA_PART_PROP_INDIRECT_MSGS     BIT(2, U)
+#define FFA_PART_PROP_RECV_NOTIF        BIT(3, U)
+#define FFA_PART_PROP_IS_MASK           (3U << 4)
+#define FFA_PART_PROP_IS_PE_ID          (0U << 4)
+#define FFA_PART_PROP_IS_SEPID_INDEP    (1U << 4)
+#define FFA_PART_PROP_IS_SEPID_DEP      (2U << 4)
+#define FFA_PART_PROP_IS_AUX_ID         (3U << 4)
+#define FFA_PART_PROP_NOTIF_CREATED     BIT(6, U)
+#define FFA_PART_PROP_NOTIF_DESTROYED   BIT(7, U)
+#define FFA_PART_PROP_AARCH64_STATE     BIT(8, U)
+
+/*
+ * Flag used as parameter to FFA_PARTITION_INFO_GET to return partition
+ * count only.
+ */
+#define FFA_PARTITION_INFO_GET_COUNT_FLAG BIT(0, U)
+
 /* Function IDs */
 #define FFA_ERROR                       0x84000060U
 #define FFA_SUCCESS_32                  0x84000061U
-- 
2.34.1

[XEN PATCH v8 06/22] xen/arm: ffa: add defines for framework direct request/response messages

[Jens Wiklander]

Adds defines for framework direct request/response messages.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
 xen/arch/arm/tee/ffa.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
index 72e7d0575de5..cf7b7545aa03 100644
--- a/xen/arch/arm/tee/ffa.c
+++ b/xen/arch/arm/tee/ffa.c
@@ -57,6 +57,19 @@
 #define FFA_MY_VERSION          MAKE_FFA_VERSION(FFA_MY_VERSION_MAJOR, \
                                                  FFA_MY_VERSION_MINOR)
 
+/*
+ * Flags and field values used for the MSG_SEND_DIRECT_REQ/RESP:
+ * BIT(31): Framework or partition message
+ * BIT(7-0): Message type for frameworks messages
+ */
+#define FFA_MSG_FLAG_FRAMEWORK          BIT(31, U)
+#define FFA_MSG_TYPE_MASK               0xFFU;
+#define FFA_MSG_PSCI                    0x0U
+#define FFA_MSG_SEND_VM_CREATED         0x4U
+#define FFA_MSG_RESP_VM_CREATED         0x5U
+#define FFA_MSG_SEND_VM_DESTROYED       0x6U
+#define FFA_MSG_RESP_VM_DESTROYED       0x7U
+
 /*
  * Flags to determine partition properties in FFA_PARTITION_INFO_GET return
  * message:
-- 
2.34.1

[XEN PATCH v8 07/22] xen/arm: ffa: enforce dependency on 4k pages

[Jens Wiklander]

Adds a BUILD_BUG_ON() to assert the dependency on 4k pages in the FF-A
mediator since the current implementation only works if Xen page size is
the same as the FFA page size.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
---
 xen/arch/arm/tee/ffa.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
index cf7b7545aa03..90ed71cbfda3 100644
--- a/xen/arch/arm/tee/ffa.c
+++ b/xen/arch/arm/tee/ffa.c
@@ -57,6 +57,16 @@
 #define FFA_MY_VERSION          MAKE_FFA_VERSION(FFA_MY_VERSION_MAJOR, \
                                                  FFA_MY_VERSION_MINOR)
 
+/*
+ * The FF-A specification explicitly works with 4K pages as a measure of
+ * memory size, for example, FFA_RXTX_MAP takes one parameter "RX/TX page
+ * count" which is the number of contiguous 4K pages allocated. Xen may use
+ * a different page size depending on the configuration to avoid confusion
+ * with PAGE_SIZE use a special define when it's a page size as in the FF-A
+ * specification.
+ */
+#define FFA_PAGE_SIZE                   SZ_4K
+
 /*
  * Flags and field values used for the MSG_SEND_DIRECT_REQ/RESP:
  * BIT(31): Framework or partition message
@@ -256,6 +266,17 @@ static bool ffa_probe(void)
     unsigned int major_vers;
     unsigned int minor_vers;
 
+    /*
+     * FF-A often works in units of 4K pages and currently it's assumed
+     * that we can map memory using that granularity. See also the comment
+     * above the FFA_PAGE_SIZE define.
+     *
+     * It is possible to support a PAGE_SIZE larger than 4K in Xen, but
+     * until that is fully handled in this code make sure that we only use
+     * 4K page sizes.
+     */
+    BUILD_BUG_ON(PAGE_SIZE != FFA_PAGE_SIZE);
+
     /*
      * psci_init_smccc() updates this value with what's reported by EL-3
      * or secure world.
-- 
2.34.1

[XEN PATCH v8 08/22] xen/arm: ffa: add support for FFA_ID_GET

[Jens Wiklander]

Adds support for the FF-A function FFA_ID_GET to return the ID of the
calling client.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
 xen/arch/arm/tee/ffa.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
index 90ed71cbfda3..f129879c5b81 100644
--- a/xen/arch/arm/tee/ffa.c
+++ b/xen/arch/arm/tee/ffa.c
@@ -181,6 +181,12 @@ static bool ffa_get_version(uint32_t *vers)
     return true;
 }
 
+static uint16_t get_vm_id(const struct domain *d)
+{
+    /* +1 since 0 is reserved for the hypervisor in FF-A */
+    return d->domain_id + 1;
+}
+
 static void set_regs(struct cpu_user_regs *regs, register_t v0, register_t v1,
                      register_t v2, register_t v3, register_t v4, register_t v5,
                      register_t v6, register_t v7)
@@ -195,6 +201,12 @@ static void set_regs(struct cpu_user_regs *regs, register_t v0, register_t v1,
         set_user_reg(regs, 7, v7);
 }
 
+static void set_regs_success(struct cpu_user_regs *regs, uint32_t w2,
+                             uint32_t w3)
+{
+    set_regs(regs, FFA_SUCCESS_32, 0, w2, w3, 0, 0, 0, 0);
+}
+
 static void handle_version(struct cpu_user_regs *regs)
 {
     struct domain *d = current->domain;
@@ -224,6 +236,9 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
     case FFA_VERSION:
         handle_version(regs);
         return true;
+    case FFA_ID_GET:
+        set_regs_success(regs, get_vm_id(d), 0);
+        return true;
 
     default:
         gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid);
@@ -237,6 +252,12 @@ static int ffa_domain_init(struct domain *d)
 
     if ( !ffa_version )
         return -ENODEV;
+     /*
+      * We can't use that last possible domain ID or get_vm_id() would cause
+      * an overflow.
+      */
+    if ( d->domain_id >= UINT16_MAX)
+        return -ERANGE;
 
     ctx = xzalloc(struct ffa_ctx);
     if ( !ctx )
-- 
2.34.1

[XEN PATCH v8 09/22] xen/arm: ffa: add direct request support

[Jens Wiklander]

Adds support for sending a FF-A direct request. Checks that the SP also
supports handling a 32-bit direct request. 64-bit direct requests are
not used by the mediator itself so there is not need to check for that.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
 xen/arch/arm/tee/ffa.c | 112 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 112 insertions(+)

diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
index f129879c5b81..f2cce955d981 100644
--- a/xen/arch/arm/tee/ffa.c
+++ b/xen/arch/arm/tee/ffa.c
@@ -181,6 +181,56 @@ static bool ffa_get_version(uint32_t *vers)
     return true;
 }
 
+static int32_t get_ffa_ret_code(const struct arm_smccc_1_2_regs *resp)
+{
+    switch ( resp->a0 )
+    {
+    case FFA_ERROR:
+        if ( resp->a2 )
+            return resp->a2;
+        else
+            return FFA_RET_NOT_SUPPORTED;
+    case FFA_SUCCESS_32:
+    case FFA_SUCCESS_64:
+        return FFA_RET_OK;
+    default:
+        return FFA_RET_NOT_SUPPORTED;
+    }
+}
+
+static int32_t ffa_simple_call(uint32_t fid, register_t a1, register_t a2,
+                               register_t a3, register_t a4)
+{
+    const struct arm_smccc_1_2_regs arg = {
+        .a0 = fid,
+        .a1 = a1,
+        .a2 = a2,
+        .a3 = a3,
+        .a4 = a4,
+    };
+    struct arm_smccc_1_2_regs resp;
+
+    arm_smccc_1_2_smc(&arg, &resp);
+
+    return get_ffa_ret_code(&resp);
+}
+
+static int32_t ffa_features(uint32_t id)
+{
+    return ffa_simple_call(FFA_FEATURES, id, 0, 0, 0);
+}
+
+static bool check_mandatory_feature(uint32_t id)
+{
+    int32_t ret = ffa_features(id);
+
+    if (ret)
+        printk(XENLOG_ERR "ffa: mandatory feature id %#x missing: error %d\n",
+               id, ret);
+
+    return !ret;
+}
+
 static uint16_t get_vm_id(const struct domain *d)
 {
     /* +1 since 0 is reserved for the hypervisor in FF-A */
@@ -222,6 +272,57 @@ static void handle_version(struct cpu_user_regs *regs)
     set_regs(regs, vers, 0, 0, 0, 0, 0, 0, 0);
 }
 
+static void handle_msg_send_direct_req(struct cpu_user_regs *regs, uint32_t fid)
+{
+    struct arm_smccc_1_2_regs arg = { .a0 = fid, };
+    struct arm_smccc_1_2_regs resp = { };
+    struct domain *d = current->domain;
+    uint32_t src_dst;
+    uint64_t mask;
+
+    if ( smccc_is_conv_64(fid) )
+        mask = GENMASK_ULL(63, 0);
+    else
+        mask = GENMASK_ULL(31, 0);
+
+    src_dst = get_user_reg(regs, 1);
+    if ( (src_dst >> 16) != get_vm_id(d) )
+    {
+        resp.a0 = FFA_ERROR;
+        resp.a2 = FFA_RET_INVALID_PARAMETERS;
+        goto out;
+    }
+
+    arg.a1 = src_dst;
+    arg.a2 = get_user_reg(regs, 2) & mask;
+    arg.a3 = get_user_reg(regs, 3) & mask;
+    arg.a4 = get_user_reg(regs, 4) & mask;
+    arg.a5 = get_user_reg(regs, 5) & mask;
+    arg.a6 = get_user_reg(regs, 6) & mask;
+    arg.a7 = get_user_reg(regs, 7) & mask;
+
+    arm_smccc_1_2_smc(&arg, &resp);
+    switch ( resp.a0 )
+    {
+    case FFA_ERROR:
+    case FFA_SUCCESS_32:
+    case FFA_SUCCESS_64:
+    case FFA_MSG_SEND_DIRECT_RESP_32:
+    case FFA_MSG_SEND_DIRECT_RESP_64:
+        break;
+    default:
+        /* Bad fid, report back. */
+        memset(&arg, 0, sizeof(arg));
+        arg.a0 = FFA_ERROR;
+        arg.a1 = src_dst;
+        arg.a2 = FFA_RET_ABORTED;
+    }
+
+out:
+    set_regs(regs, resp.a0, resp.a1 & mask, resp.a2 & mask, resp.a3 & mask,
+             resp.a4 & mask, resp.a5 & mask, resp.a6 & mask, resp.a7 & mask);
+}
+
 static bool ffa_handle_call(struct cpu_user_regs *regs)
 {
     uint32_t fid = get_user_reg(regs, 0);
@@ -239,6 +340,10 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
     case FFA_ID_GET:
         set_regs_success(regs, get_vm_id(d), 0);
         return true;
+    case FFA_MSG_SEND_DIRECT_REQ_32:
+    case FFA_MSG_SEND_DIRECT_REQ_64:
+        handle_msg_send_direct_req(regs, fid);
+        return true;
 
     default:
         gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid);
@@ -326,6 +431,13 @@ static bool ffa_probe(void)
     printk(XENLOG_INFO "ARM FF-A Firmware version %u.%u\n",
            major_vers, minor_vers);
 
+    /*
+     * TODO save result of checked features and use that information to
+     * accept or reject requests from guests.
+     */
+    if ( !check_mandatory_feature(FFA_MSG_SEND_DIRECT_REQ_32) )
+        return false;
+
     ffa_version = vers;
 
     return true;
-- 
2.34.1

[XEN PATCH v8 10/22] xen/arm: ffa: map SPMC rx/tx buffers

[Jens Wiklander]

When initializing the FF-A mediator map the RX and TX buffers shared with
the SPMC.

These buffer are later used to to transmit data that cannot be passed in
registers only.

Adds a check that the SP supports the needed FF-A features
FFA_RXTX_MAP_64 and FFA_RXTX_UNMAP.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
 xen/arch/arm/tee/ffa.c | 50 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 49 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
index f2cce955d981..054121fe4321 100644
--- a/xen/arch/arm/tee/ffa.c
+++ b/xen/arch/arm/tee/ffa.c
@@ -12,6 +12,7 @@
 #include <xen/errno.h>
 #include <xen/init.h>
 #include <xen/lib.h>
+#include <xen/mm.h>
 #include <xen/sched.h>
 #include <xen/sizes.h>
 #include <xen/types.h>
@@ -67,6 +68,12 @@
  */
 #define FFA_PAGE_SIZE                   SZ_4K
 
+/*
+ * The number of pages used for each of the RX and TX buffers shared with
+ * the SPMC.
+ */
+#define FFA_RXTX_PAGE_COUNT             1
+
 /*
  * Flags and field values used for the MSG_SEND_DIRECT_REQ/RESP:
  * BIT(31): Framework or partition message
@@ -161,6 +168,13 @@ struct ffa_ctx {
 /* Negotiated FF-A version to use with the SPMC */
 static uint32_t ffa_version __ro_after_init;
 
+/*
+ * Our rx/tx buffers shared with the SPMC. FFA_RXTX_PAGE_COUNT is the
+ * number of pages used in each of these buffers.
+ */
+static void *ffa_rx __read_mostly;
+static void *ffa_tx __read_mostly;
+
 static bool ffa_get_version(uint32_t *vers)
 {
     const struct arm_smccc_1_2_regs arg = {
@@ -231,6 +245,12 @@ static bool check_mandatory_feature(uint32_t id)
     return !ret;
 }
 
+static int32_t ffa_rxtx_map(paddr_t tx_addr, paddr_t rx_addr,
+                            uint32_t page_count)
+{
+    return ffa_simple_call(FFA_RXTX_MAP_64, tx_addr, rx_addr, page_count, 0);
+}
+
 static uint16_t get_vm_id(const struct domain *d)
 {
     /* +1 since 0 is reserved for the hypervisor in FF-A */
@@ -389,6 +409,7 @@ static int ffa_relinquish_resources(struct domain *d)
 static bool ffa_probe(void)
 {
     uint32_t vers;
+    int e;
     unsigned int major_vers;
     unsigned int minor_vers;
 
@@ -435,12 +456,39 @@ static bool ffa_probe(void)
      * TODO save result of checked features and use that information to
      * accept or reject requests from guests.
      */
-    if ( !check_mandatory_feature(FFA_MSG_SEND_DIRECT_REQ_32) )
+    if (
+         !check_mandatory_feature(FFA_RXTX_MAP_64) ||
+         !check_mandatory_feature(FFA_RXTX_UNMAP) ||
+         !check_mandatory_feature(FFA_MSG_SEND_DIRECT_REQ_32) )
+        return false;
+
+    ffa_rx = alloc_xenheap_pages(get_order_from_pages(FFA_RXTX_PAGE_COUNT), 0);
+    if ( !ffa_rx )
         return false;
 
+    ffa_tx = alloc_xenheap_pages(get_order_from_pages(FFA_RXTX_PAGE_COUNT), 0);
+    if ( !ffa_tx )
+        goto err_free_ffa_rx;
+
+    e = ffa_rxtx_map(__pa(ffa_tx), __pa(ffa_rx), FFA_RXTX_PAGE_COUNT);
+    if ( e )
+    {
+        printk(XENLOG_ERR "ffa: Failed to map rxtx: error %d\n", e);
+        goto err_free_ffa_tx;
+    }
     ffa_version = vers;
 
     return true;
+
+err_free_ffa_tx:
+    free_xenheap_pages(ffa_tx, 0);
+    ffa_tx = NULL;
+err_free_ffa_rx:
+    free_xenheap_pages(ffa_rx, 0);
+    ffa_rx = NULL;
+    ffa_version = 0;
+
+    return false;
 }
 
 static const struct tee_mediator_ops ffa_ops =
-- 
2.34.1

[XEN PATCH v8 11/22] xen/arm: ffa: send guest events to Secure Partitions

[Jens Wiklander]

The FF-A specification defines framework messages sent as direct
requests when certain events occurs. For instance when a VM (guest) is
created or destroyed. Only SPs which have subscribed to these events
will receive them. An SP can subscribe to these messages in its
partition properties.

Adds a check that the SP supports the needed FF-A features
FFA_PARTITION_INFO_GET and FFA_RX_RELEASE.

The partition properties of each SP is retrieved with
FFA_PARTITION_INFO_GET which returns the information in our RX buffer.
Using FFA_PARTITION_INFO_GET changes the owner of the RX buffer to the
caller (us), so once we're done with the buffer it must be released
using FFA_RX_RELEASE before another call can be made.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
 xen/arch/arm/tee/ffa.c | 200 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 199 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
index 054121fe4321..b4fea65ce31d 100644
--- a/xen/arch/arm/tee/ffa.c
+++ b/xen/arch/arm/tee/ffa.c
@@ -160,6 +160,14 @@
 #define FFA_MSG_SEND                    0x8400006EU
 #define FFA_MSG_POLL                    0x8400006AU
 
+/* Partition information descriptor */
+struct ffa_partition_info_1_1 {
+    uint16_t id;
+    uint16_t execution_context;
+    uint32_t partition_properties;
+    uint8_t uuid[16];
+};
+
 struct ffa_ctx {
     /* FF-A version used by the guest */
     uint32_t guest_vers;
@@ -168,6 +176,12 @@ struct ffa_ctx {
 /* Negotiated FF-A version to use with the SPMC */
 static uint32_t ffa_version __ro_after_init;
 
+/* SPs subscribing to VM_CREATE and VM_DESTROYED events */
+static uint16_t *subscr_vm_created __read_mostly;
+static uint16_t subscr_vm_created_count __read_mostly;
+static uint16_t *subscr_vm_destroyed __read_mostly;
+static uint16_t subscr_vm_destroyed_count __read_mostly;
+
 /*
  * Our rx/tx buffers shared with the SPMC. FFA_RXTX_PAGE_COUNT is the
  * number of pages used in each of these buffers.
@@ -251,6 +265,72 @@ static int32_t ffa_rxtx_map(paddr_t tx_addr, paddr_t rx_addr,
     return ffa_simple_call(FFA_RXTX_MAP_64, tx_addr, rx_addr, page_count, 0);
 }
 
+static int32_t ffa_partition_info_get(uint32_t w1, uint32_t w2, uint32_t w3,
+                                      uint32_t w4, uint32_t w5,
+                                      uint32_t *count)
+{
+    const struct arm_smccc_1_2_regs arg = {
+        .a0 = FFA_PARTITION_INFO_GET,
+        .a1 = w1,
+        .a2 = w2,
+        .a3 = w3,
+        .a4 = w4,
+        .a5 = w5,
+    };
+    struct arm_smccc_1_2_regs resp;
+    uint32_t ret;
+
+    arm_smccc_1_2_smc(&arg, &resp);
+
+    ret = get_ffa_ret_code(&resp);
+    if ( !ret )
+        *count = resp.a2;
+
+    return ret;
+}
+
+static int32_t ffa_rx_release(void)
+{
+    return ffa_simple_call(FFA_RX_RELEASE, 0, 0, 0, 0);
+}
+
+static int32_t ffa_direct_req_send_vm(uint16_t sp_id, uint16_t vm_id,
+                                      uint8_t msg)
+{
+    uint32_t exp_resp = FFA_MSG_FLAG_FRAMEWORK;
+    int32_t res;
+
+    if ( msg == FFA_MSG_SEND_VM_CREATED )
+        exp_resp |= FFA_MSG_RESP_VM_CREATED;
+    else if ( msg == FFA_MSG_SEND_VM_DESTROYED )
+        exp_resp |= FFA_MSG_RESP_VM_DESTROYED;
+    else
+        return FFA_RET_INVALID_PARAMETERS;
+
+    do {
+        const struct arm_smccc_1_2_regs arg = {
+            .a0 = FFA_MSG_SEND_DIRECT_REQ_32,
+            .a1 = sp_id,
+            .a2 = FFA_MSG_FLAG_FRAMEWORK | msg,
+            .a5 = vm_id,
+        };
+        struct arm_smccc_1_2_regs resp;
+
+        arm_smccc_1_2_smc(&arg, &resp);
+        if ( resp.a0 != FFA_MSG_SEND_DIRECT_RESP_32 || resp.a2 != exp_resp )
+        {
+            /*
+             * This is an invalid response, likely due to some error in the
+             * implementation of the ABI.
+             */
+            return FFA_RET_INVALID_PARAMETERS;
+        }
+        res = resp.a3;
+    } while ( res == FFA_RET_INTERRUPTED || res == FFA_RET_RETRY );
+
+    return res;
+}
+
 static uint16_t get_vm_id(const struct domain *d)
 {
     /* +1 since 0 is reserved for the hypervisor in FF-A */
@@ -374,6 +454,10 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
 static int ffa_domain_init(struct domain *d)
 {
     struct ffa_ctx *ctx;
+    unsigned int n;
+    unsigned int m;
+    unsigned int c_pos;
+    int32_t res;
 
     if ( !ffa_version )
         return -ENODEV;
@@ -388,24 +472,134 @@ static int ffa_domain_init(struct domain *d)
     if ( !ctx )
         return -ENOMEM;
 
+    for ( n = 0; n < subscr_vm_created_count; n++ )
+    {
+        res = ffa_direct_req_send_vm(subscr_vm_created[n], get_vm_id(d),
+                                     FFA_MSG_SEND_VM_CREATED);
+        if ( res )
+        {
+            printk(XENLOG_ERR "ffa: Failed to report creation of vm_id %u to  %u: res %d\n",
+                   get_vm_id(d), subscr_vm_created[n], res);
+            c_pos = n;
+            goto err;
+        }
+    }
+
     d->arch.tee = ctx;
 
     return 0;
+
+err:
+    /* Undo any already sent vm created messaged */
+    for ( n = 0; n < c_pos; n++ )
+        for ( m = 0; m < subscr_vm_destroyed_count; m++ )
+            if ( subscr_vm_destroyed[m] == subscr_vm_created[n] )
+                ffa_direct_req_send_vm(subscr_vm_destroyed[n], get_vm_id(d),
+                                       FFA_MSG_SEND_VM_DESTROYED);
+
+    return -EIO;
 }
 
 /* This function is supposed to undo what ffa_domain_init() has done */
 static int ffa_relinquish_resources(struct domain *d)
 {
     struct ffa_ctx *ctx = d->arch.tee;
+    unsigned int n;
+    int32_t res;
 
     if ( !ctx )
         return 0;
 
+    for ( n = 0; n < subscr_vm_destroyed_count; n++ )
+    {
+        res = ffa_direct_req_send_vm(subscr_vm_destroyed[n], get_vm_id(d),
+                                     FFA_MSG_SEND_VM_DESTROYED);
+
+        if ( res )
+            printk(XENLOG_ERR "ffa: Failed to report destruction of vm_id %u to  %u: res %d\n",
+                   get_vm_id(d), subscr_vm_destroyed[n], res);
+    }
+
     XFREE(d->arch.tee);
 
     return 0;
 }
 
+static void uninit_subscribers(void)
+{
+        subscr_vm_created_count = 0;
+        subscr_vm_destroyed_count = 0;
+        XFREE(subscr_vm_created);
+        XFREE(subscr_vm_destroyed);
+}
+
+static bool init_subscribers(struct ffa_partition_info_1_1 *fpi, uint16_t count)
+{
+    uint16_t n;
+    uint16_t c_pos;
+    uint16_t d_pos;
+
+    subscr_vm_created_count = 0;
+    subscr_vm_destroyed_count = 0;
+    for ( n = 0; n < count; n++ )
+    {
+        if (fpi[n].partition_properties & FFA_PART_PROP_NOTIF_CREATED)
+            subscr_vm_created_count++;
+        if (fpi[n].partition_properties & FFA_PART_PROP_NOTIF_DESTROYED)
+            subscr_vm_destroyed_count++;
+    }
+
+    if ( subscr_vm_created_count )
+        subscr_vm_created = xzalloc_array(uint16_t, subscr_vm_created_count);
+    if ( subscr_vm_destroyed_count )
+        subscr_vm_destroyed = xzalloc_array(uint16_t,
+                                            subscr_vm_destroyed_count);
+    if ( (subscr_vm_created_count && !subscr_vm_created) ||
+         (subscr_vm_destroyed_count && !subscr_vm_destroyed) )
+    {
+        printk(XENLOG_ERR "ffa: Failed to allocate subscription lists\n");
+        uninit_subscribers();
+        return false;
+    }
+
+    for ( c_pos = 0, d_pos = 0, n = 0; n < count; n++ )
+    {
+        if ( fpi[n].partition_properties & FFA_PART_PROP_NOTIF_CREATED )
+            subscr_vm_created[c_pos++] = fpi[n].id;
+        if ( fpi[n].partition_properties & FFA_PART_PROP_NOTIF_DESTROYED )
+            subscr_vm_destroyed[d_pos++] = fpi[n].id;
+    }
+
+    return true;
+}
+
+static bool init_sps(void)
+{
+    bool ret = false;
+    uint32_t count;
+    int e;
+
+    e = ffa_partition_info_get(0, 0, 0, 0, 0, &count);
+    if ( e )
+    {
+        printk(XENLOG_ERR "ffa: Failed to get list of SPs: %d\n", e);
+        goto out;
+    }
+
+    if ( count >= UINT16_MAX )
+    {
+        printk(XENLOG_ERR "ffa: Impossible number of SPs: %u\n", count);
+        goto out;
+    }
+
+    ret = init_subscribers(ffa_rx, count);
+
+out:
+    ffa_rx_release();
+
+    return ret;
+}
+
 static bool ffa_probe(void)
 {
     uint32_t vers;
@@ -456,7 +650,8 @@ static bool ffa_probe(void)
      * TODO save result of checked features and use that information to
      * accept or reject requests from guests.
      */
-    if (
+    if ( !check_mandatory_feature(FFA_PARTITION_INFO_GET) ||
+         !check_mandatory_feature(FFA_RX_RELEASE) ||
          !check_mandatory_feature(FFA_RXTX_MAP_64) ||
          !check_mandatory_feature(FFA_RXTX_UNMAP) ||
          !check_mandatory_feature(FFA_MSG_SEND_DIRECT_REQ_32) )
@@ -478,6 +673,9 @@ static bool ffa_probe(void)
     }
     ffa_version = vers;
 
+    if ( !init_sps() )
+        goto err_free_ffa_tx;
+
     return true;
 
 err_free_ffa_tx:
-- 
2.34.1

[XEN PATCH v8 12/22] xen/arm: ffa: support mapping guest RX/TX buffers

[Jens Wiklander]

Adds support in the mediator to map and unmap the RX and TX buffers
provided by the guest using the two FF-A functions FFA_RXTX_MAP and
FFA_RXTX_UNMAP.

These buffer are later used to transmit data that cannot be passed in
registers only.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
 xen/arch/arm/tee/ffa.c | 137 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 137 insertions(+)

diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
index b4fea65ce31d..127397d8e448 100644
--- a/xen/arch/arm/tee/ffa.c
+++ b/xen/arch/arm/tee/ffa.c
@@ -74,6 +74,12 @@
  */
 #define FFA_RXTX_PAGE_COUNT             1
 
+/*
+ * Limits the number of pages RX/TX buffers guests can map. This value has
+ * been chosen arbitrary.
+ */
+#define FFA_MAX_RXTX_PAGE_COUNT         32
+
 /*
  * Flags and field values used for the MSG_SEND_DIRECT_REQ/RESP:
  * BIT(31): Framework or partition message
@@ -169,8 +175,15 @@ struct ffa_partition_info_1_1 {
 };
 
 struct ffa_ctx {
+    void *rx;
+    const void *tx;
+    struct page_info *rx_pg;
+    struct page_info *tx_pg;
+    /* Number of 4kB pages in each of rx/rx_pg and tx/tx_pg */
+    unsigned int page_count;
     /* FF-A version used by the guest */
     uint32_t guest_vers;
+    bool tx_is_free;
 };
 
 /* Negotiated FF-A version to use with the SPMC */
@@ -351,6 +364,11 @@ static void set_regs(struct cpu_user_regs *regs, register_t v0, register_t v1,
         set_user_reg(regs, 7, v7);
 }
 
+static void set_regs_error(struct cpu_user_regs *regs, uint32_t error_code)
+{
+    set_regs(regs, FFA_ERROR, 0, error_code, 0, 0, 0, 0, 0);
+}
+
 static void set_regs_success(struct cpu_user_regs *regs, uint32_t w2,
                              uint32_t w3)
 {
@@ -372,6 +390,105 @@ static void handle_version(struct cpu_user_regs *regs)
     set_regs(regs, vers, 0, 0, 0, 0, 0, 0, 0);
 }
 
+static uint32_t handle_rxtx_map(uint32_t fid, register_t tx_addr,
+                                register_t rx_addr, uint32_t page_count)
+{
+    uint32_t ret = FFA_RET_INVALID_PARAMETERS;
+    struct domain *d = current->domain;
+    struct ffa_ctx *ctx = d->arch.tee;
+    struct page_info *tx_pg;
+    struct page_info *rx_pg;
+    p2m_type_t t;
+    void *rx;
+    void *tx;
+
+    if ( !smccc_is_conv_64(fid) )
+    {
+        /*
+         * Calls using the 32-bit calling convention must ignore the upper
+         * 32 bits in the argument registers.
+         */
+        tx_addr &= UINT32_MAX;
+        rx_addr &= UINT32_MAX;
+    }
+
+    if ( page_count > FFA_MAX_RXTX_PAGE_COUNT ) {
+        printk(XENLOG_ERR "ffa: RXTX_MAP: error: %u pages requested (limit %u)\n",
+               page_count, FFA_MAX_RXTX_PAGE_COUNT);
+        return FFA_RET_NOT_SUPPORTED;
+    }
+
+    /* Already mapped */
+    if ( ctx->rx )
+        return FFA_RET_DENIED;
+
+    tx_pg = get_page_from_gfn(d, gfn_x(gaddr_to_gfn(tx_addr)), &t, P2M_ALLOC);
+    if ( !tx_pg )
+        return FFA_RET_INVALID_PARAMETERS;
+    /* Only normal RAM for now */
+    if ( !p2m_is_ram(t) )
+        goto err_put_tx_pg;
+
+    rx_pg = get_page_from_gfn(d, gfn_x(gaddr_to_gfn(rx_addr)), &t, P2M_ALLOC);
+    if ( !tx_pg )
+        goto err_put_tx_pg;
+    /* Only normal RAM for now */
+    if ( !p2m_is_ram(t) )
+        goto err_put_rx_pg;
+
+    tx = __map_domain_page_global(tx_pg);
+    if ( !tx )
+        goto err_put_rx_pg;
+
+    rx = __map_domain_page_global(rx_pg);
+    if ( !rx )
+        goto err_unmap_tx;
+
+    ctx->rx = rx;
+    ctx->tx = tx;
+    ctx->rx_pg = rx_pg;
+    ctx->tx_pg = tx_pg;
+    ctx->page_count = page_count;
+    ctx->tx_is_free = true;
+    return FFA_RET_OK;
+
+err_unmap_tx:
+    unmap_domain_page_global(tx);
+err_put_rx_pg:
+    put_page(rx_pg);
+err_put_tx_pg:
+    put_page(tx_pg);
+
+    return ret;
+}
+
+static void rxtx_unmap(struct ffa_ctx *ctx)
+{
+    unmap_domain_page_global(ctx->rx);
+    unmap_domain_page_global(ctx->tx);
+    put_page(ctx->rx_pg);
+    put_page(ctx->tx_pg);
+    ctx->rx = NULL;
+    ctx->tx = NULL;
+    ctx->rx_pg = NULL;
+    ctx->tx_pg = NULL;
+    ctx->page_count = 0;
+    ctx->tx_is_free = false;
+}
+
+static uint32_t handle_rxtx_unmap(void)
+{
+    struct domain *d = current->domain;
+    struct ffa_ctx *ctx = d->arch.tee;
+
+    if ( !ctx->rx )
+        return FFA_RET_INVALID_PARAMETERS;
+
+    rxtx_unmap(ctx);
+
+    return FFA_RET_OK;
+}
+
 static void handle_msg_send_direct_req(struct cpu_user_regs *regs, uint32_t fid)
 {
     struct arm_smccc_1_2_regs arg = { .a0 = fid, };
@@ -428,6 +545,7 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
     uint32_t fid = get_user_reg(regs, 0);
     struct domain *d = current->domain;
     struct ffa_ctx *ctx = d->arch.tee;
+    int e;
 
     if ( !ctx )
         return false;
@@ -440,6 +558,22 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
     case FFA_ID_GET:
         set_regs_success(regs, get_vm_id(d), 0);
         return true;
+    case FFA_RXTX_MAP_32:
+    case FFA_RXTX_MAP_64:
+        e = handle_rxtx_map(fid, get_user_reg(regs, 1), get_user_reg(regs, 2),
+                            get_user_reg(regs, 3));
+        if ( e )
+            set_regs_error(regs, e);
+        else
+            set_regs_success(regs, 0, 0);
+        return true;
+    case FFA_RXTX_UNMAP:
+        e = handle_rxtx_unmap();
+        if ( e )
+            set_regs_error(regs, e);
+        else
+            set_regs_success(regs, 0, 0);
+        return true;
     case FFA_MSG_SEND_DIRECT_REQ_32:
     case FFA_MSG_SEND_DIRECT_REQ_64:
         handle_msg_send_direct_req(regs, fid);
@@ -520,6 +654,9 @@ static int ffa_relinquish_resources(struct domain *d)
                    get_vm_id(d), subscr_vm_destroyed[n], res);
     }
 
+    if ( ctx->rx )
+        rxtx_unmap(ctx);
+
     XFREE(d->arch.tee);
 
     return 0;
-- 
2.34.1

[XEN PATCH v8 13/22] xen/arm: ffa: support guest FFA_PARTITION_INFO_GET

[Jens Wiklander]

Adds support in the mediator to handle FFA_PARTITION_INFO_GET requests
from a guest. The requests are forwarded to the SPMC and the response is
translated according to the FF-A version in use by the guest.

Using FFA_PARTITION_INFO_GET changes the owner of the RX buffer to the
caller (the guest in this case), so once it is done with the buffer it
must be released using FFA_RX_RELEASE before another call can be made.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
 xen/arch/arm/tee/ffa.c | 137 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 134 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
index 127397d8e448..74b8c517afb8 100644
--- a/xen/arch/arm/tee/ffa.c
+++ b/xen/arch/arm/tee/ffa.c
@@ -166,7 +166,18 @@
 #define FFA_MSG_SEND                    0x8400006EU
 #define FFA_MSG_POLL                    0x8400006AU
 
+/*
+ * Structs below ending with _1_0 are defined in FF-A-1.0-REL and
+ * struct ending with _1_1 are defined in FF-A-1.1-REL0.
+ */
+
 /* Partition information descriptor */
+struct ffa_partition_info_1_0 {
+    uint16_t id;
+    uint16_t execution_context;
+    uint32_t partition_properties;
+};
+
 struct ffa_partition_info_1_1 {
     uint16_t id;
     uint16_t execution_context;
@@ -183,7 +194,8 @@ struct ffa_ctx {
     unsigned int page_count;
     /* FF-A version used by the guest */
     uint32_t guest_vers;
-    bool tx_is_free;
+    bool rx_is_free;
+    spinlock_t lock;
 };
 
 /* Negotiated FF-A version to use with the SPMC */
@@ -198,9 +210,15 @@ static uint16_t subscr_vm_destroyed_count __read_mostly;
 /*
  * Our rx/tx buffers shared with the SPMC. FFA_RXTX_PAGE_COUNT is the
  * number of pages used in each of these buffers.
+ *
+ * The RX buffer is protected from concurrent usage with ffa_rx_buffer_lock.
+ * Note that the SPMC is also tracking the ownership of our RX buffer so
+ * for calls which uses our RX buffer to deliver a result we must call
+ * ffa_rx_release() to let the SPMC know that we're done with the buffer.
  */
 static void *ffa_rx __read_mostly;
 static void *ffa_tx __read_mostly;
+static DEFINE_SPINLOCK(ffa_rx_buffer_lock);
 
 static bool ffa_get_version(uint32_t *vers)
 {
@@ -449,7 +467,7 @@ static uint32_t handle_rxtx_map(uint32_t fid, register_t tx_addr,
     ctx->rx_pg = rx_pg;
     ctx->tx_pg = tx_pg;
     ctx->page_count = page_count;
-    ctx->tx_is_free = true;
+    ctx->rx_is_free = true;
     return FFA_RET_OK;
 
 err_unmap_tx:
@@ -473,7 +491,7 @@ static void rxtx_unmap(struct ffa_ctx *ctx)
     ctx->rx_pg = NULL;
     ctx->tx_pg = NULL;
     ctx->page_count = 0;
-    ctx->tx_is_free = false;
+    ctx->rx_is_free = false;
 }
 
 static uint32_t handle_rxtx_unmap(void)
@@ -489,6 +507,100 @@ static uint32_t handle_rxtx_unmap(void)
     return FFA_RET_OK;
 }
 
+static int32_t handle_partition_info_get(uint32_t w1, uint32_t w2, uint32_t w3,
+                                         uint32_t w4, uint32_t w5,
+                                         uint32_t *count)
+{
+    int32_t ret = FFA_RET_DENIED;
+    struct domain *d = current->domain;
+    struct ffa_ctx *ctx = d->arch.tee;
+
+    /*
+     * FF-A v1.0 has w5 MBZ while v1.1 allows
+     * FFA_PARTITION_INFO_GET_COUNT_FLAG to be non-zero.
+     */
+    if ( w5 == FFA_PARTITION_INFO_GET_COUNT_FLAG &&
+         ctx->guest_vers == FFA_VERSION_1_1 )
+        return ffa_partition_info_get(w1, w2, w3, w4, w5, count);
+    if ( w5 )
+        return FFA_RET_INVALID_PARAMETERS;
+
+    if ( !ffa_rx )
+        return FFA_RET_DENIED;
+
+    spin_lock(&ctx->lock);
+    if ( !ctx->page_count || !ctx->rx_is_free )
+        goto out;
+    spin_lock(&ffa_rx_buffer_lock);
+    ret = ffa_partition_info_get(w1, w2, w3, w4, w5, count);
+    if ( ret )
+        goto out_rx_buf_unlock;
+    /*
+     * ffa_partition_info_get() succeeded so we now own the RX buffer we
+     * share with the SPMC. We must give it back using ffa_rx_release()
+     * once we've copied the content.
+     */
+
+    if ( ctx->guest_vers == FFA_VERSION_1_0 )
+    {
+        size_t n;
+        struct ffa_partition_info_1_1 *src = ffa_rx;
+        struct ffa_partition_info_1_0 *dst = ctx->rx;
+
+        if ( ctx->page_count * FFA_PAGE_SIZE < *count * sizeof(*dst) )
+        {
+            ret = FFA_RET_NO_MEMORY;
+            goto out_rx_release;
+        }
+
+        for ( n = 0; n < *count; n++ )
+        {
+            dst[n].id = src[n].id;
+            dst[n].execution_context = src[n].execution_context;
+            dst[n].partition_properties = src[n].partition_properties;
+        }
+    }
+    else
+    {
+        size_t sz = *count * sizeof(struct ffa_partition_info_1_1);
+
+        if ( ctx->page_count * FFA_PAGE_SIZE < sz )
+        {
+            ret = FFA_RET_NO_MEMORY;
+            goto out_rx_release;
+        }
+
+
+        memcpy(ctx->rx, ffa_rx, sz);
+    }
+    ctx->rx_is_free = false;
+out_rx_release:
+    ffa_rx_release();
+out_rx_buf_unlock:
+    spin_unlock(&ffa_rx_buffer_lock);
+out:
+    spin_unlock(&ctx->lock);
+
+    return ret;
+}
+
+static int32_t handle_rx_release(void)
+{
+    int32_t ret = FFA_RET_DENIED;
+    struct domain *d = current->domain;
+    struct ffa_ctx *ctx = d->arch.tee;
+
+    spin_lock(&ctx->lock);
+    if ( !ctx->page_count || ctx->rx_is_free )
+        goto out;
+    ret = FFA_RET_OK;
+    ctx->rx_is_free = true;
+out:
+    spin_unlock(&ctx->lock);
+
+    return ret;
+}
+
 static void handle_msg_send_direct_req(struct cpu_user_regs *regs, uint32_t fid)
 {
     struct arm_smccc_1_2_regs arg = { .a0 = fid, };
@@ -545,6 +657,7 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
     uint32_t fid = get_user_reg(regs, 0);
     struct domain *d = current->domain;
     struct ffa_ctx *ctx = d->arch.tee;
+    uint32_t count;
     int e;
 
     if ( !ctx )
@@ -574,6 +687,24 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
         else
             set_regs_success(regs, 0, 0);
         return true;
+    case FFA_PARTITION_INFO_GET:
+        e = handle_partition_info_get(get_user_reg(regs, 1),
+                                      get_user_reg(regs, 2),
+                                      get_user_reg(regs, 3),
+                                      get_user_reg(regs, 4),
+                                      get_user_reg(regs, 5), &count);
+        if ( e )
+            set_regs_error(regs, e);
+        else
+            set_regs_success(regs, count, 0);
+        return true;
+    case FFA_RX_RELEASE:
+        e = handle_rx_release();
+        if ( e )
+            set_regs_error(regs, e);
+        else
+            set_regs_success(regs, 0, 0);
+        return true;
     case FFA_MSG_SEND_DIRECT_REQ_32:
     case FFA_MSG_SEND_DIRECT_REQ_64:
         handle_msg_send_direct_req(regs, fid);
-- 
2.34.1

[XEN PATCH v8 14/22] xen/arm: move regpair_to_uint64() and uint64_to_regpair() to regs.h

[Jens Wiklander]

Moves the two helper functions regpair_to_uint64() and
uint64_to_regpair() from xen/arch/arm/tee/optee.c to the common arm
specific regs.h. This enables reuse of these functions in the FF-A
mediator in a subsequent patch.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
---
 xen/arch/arm/include/asm/regs.h | 12 ++++++++++++
 xen/arch/arm/tee/optee.c        | 11 -----------
 2 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/xen/arch/arm/include/asm/regs.h b/xen/arch/arm/include/asm/regs.h
index 0693a681315f..aa39e83ee5f4 100644
--- a/xen/arch/arm/include/asm/regs.h
+++ b/xen/arch/arm/include/asm/regs.h
@@ -60,6 +60,18 @@ static inline bool guest_mode(const struct cpu_user_regs *r)
 register_t get_user_reg(struct cpu_user_regs *regs, int reg);
 void set_user_reg(struct cpu_user_regs *regs, int reg, register_t val);
 
+static inline uint64_t regpair_to_uint64(register_t reg0, register_t reg1)
+{
+    return ((uint64_t)reg0 << 32) | (uint32_t)reg1;
+}
+
+static inline void uint64_to_regpair(register_t *reg0, register_t *reg1,
+                                     uint64_t val)
+{
+    *reg0 = val >> 32;
+    *reg1 = (uint32_t)val;
+}
+
 #endif
 
 #endif /* __ARM_REGS_H__ */
diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
index 9cb9f16d43cb..47027ecef47c 100644
--- a/xen/arch/arm/tee/optee.c
+++ b/xen/arch/arm/tee/optee.c
@@ -268,17 +268,6 @@ static int optee_domain_init(struct domain *d)
     return 0;
 }
 
-static uint64_t regpair_to_uint64(register_t reg0, register_t reg1)
-{
-    return ((uint64_t)reg0 << 32) | (uint32_t)reg1;
-}
-
-static void uint64_to_regpair(register_t *reg0, register_t *reg1, uint64_t val)
-{
-    *reg0 = val >> 32;
-    *reg1 = (uint32_t)val;
-}
-
 static struct page_info *get_domain_ram_page(gfn_t gfn)
 {
     struct page_info *page;
-- 
2.34.1

[XEN PATCH v8 15/22] xen/arm: ffa: add defines for sharing memory

[Jens Wiklander]

Adds defines needed for sharing using the function FFA_MEM_SHARE and
friends.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
 xen/arch/arm/tee/ffa.c | 60 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
index 74b8c517afb8..58c581c8ffc7 100644
--- a/xen/arch/arm/tee/ffa.c
+++ b/xen/arch/arm/tee/ffa.c
@@ -5,6 +5,14 @@
  * Arm Firmware Framework for ARMv8-A (FF-A) mediator
  *
  * Copyright (C) 2023  Linaro Limited
+ *
+ * References:
+ * FF-A-1.0-REL: FF-A specification version 1.0 available at
+ *               https://developer.arm.com/documentation/den0077/a
+ * FF-A-1.1-REL0: FF-A specification version 1.1 available at
+ *                https://developer.arm.com/documentation/den0077/e
+ * TEEC-1.0C: TEE Client API Specification version 1.0c available at
+ *            https://globalplatform.org/specs-library/tee-client-api-specification/
  */
 
 #include <xen/bitops.h>
@@ -80,6 +88,58 @@
  */
 #define FFA_MAX_RXTX_PAGE_COUNT         32
 
+/*
+ * Limit for shared buffer size. Please note that this define limits
+ * number of pages. But user buffer can be not aligned to a page
+ * boundary. So it is possible that user would not be able to share
+ * exactly FFA_MAX_SHM_BUFFER_PG * FFA_PAGE_SIZE bytes.
+ *
+ * FF-A doesn't have any direct requirments on GlobalPlatform or vice
+ * versa, but an implementation can very well use FF-A in order to provide
+ * a GlobalPlatform interface on top.
+ *
+ * Global Platform specification for TEE requires that any TEE
+ * implementation should allow to share buffers with size of at least
+ * 512KB, defined in TEEC-1.0C page 24, Table 4-1,
+ * TEEC_CONFIG_SHAREDMEM_MAX_SIZE.
+ * Due to align issue mentioned above, we need to increase this
+ * value with one.
+ */
+#define FFA_MAX_SHM_PAGE_COUNT          (SZ_512K / FFA_PAGE_SIZE + 1)
+
+/*
+ * Limits the number of shared buffers that guest can have at once. This
+ * is to prevent case, when guests tricks XEN into exhausting its own
+ * memory by allocating many small buffers. This value has been chosen
+ * arbitrary.
+ */
+#define FFA_MAX_SHM_COUNT               32
+
+/* FF-A-1.1-REL0 section 10.9.2 Memory region handle, page 167 */
+#define FFA_HANDLE_HYP_FLAG             BIT(63, ULL)
+#define FFA_HANDLE_INVALID              0xffffffffffffffffULL
+
+/*
+ * Memory attributes: Normal memory, Write-Back cacheable, Inner shareable
+ * Defined in FF-A-1.1-REL0 Table 10.18 at page 175.
+ */
+#define FFA_NORMAL_MEM_REG_ATTR         0x2fU
+/*
+ * Memory access permissions: Read-write
+ * Defined in FF-A-1.1-REL0 Table 10.15 at page 168.
+ */
+#define FFA_MEM_ACC_RW                  0x2U
+
+/* FF-A-1.1-REL0 section 10.11.4 Flags usage, page 184-187 */
+/* Clear memory before mapping in receiver */
+#define FFA_MEMORY_REGION_FLAG_CLEAR            BIT(0, U)
+/* Relayer may time slice this operation */
+#define FFA_MEMORY_REGION_FLAG_TIME_SLICE       BIT(1, U)
+/* Clear memory after receiver relinquishes it */
+#define FFA_MEMORY_REGION_FLAG_CLEAR_RELINQUISH BIT(2, U)
+/* Share memory transaction */
+#define FFA_MEMORY_REGION_TRANSACTION_TYPE_SHARE (1U << 3)
+
 /*
  * Flags and field values used for the MSG_SEND_DIRECT_REQ/RESP:
  * BIT(31): Framework or partition message
-- 
2.34.1

[XEN PATCH v8 16/22] xen/arm: ffa: add ABI structs for sharing memory

[Jens Wiklander]

Adds the ABI structs used by function FFA_MEM_SHARE and friends for
sharing memory.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
---
 xen/arch/arm/tee/ffa.c | 69 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)

diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
index 58c581c8ffc7..f3e05911e16e 100644
--- a/xen/arch/arm/tee/ffa.c
+++ b/xen/arch/arm/tee/ffa.c
@@ -245,6 +245,75 @@ struct ffa_partition_info_1_1 {
     uint8_t uuid[16];
 };
 
+/* Constituent memory region descriptor */
+struct ffa_address_range {
+    uint64_t address;
+    uint32_t page_count;
+    uint32_t reserved;
+};
+
+/* Composite memory region descriptor */
+struct ffa_mem_region {
+    uint32_t total_page_count;
+    uint32_t address_range_count;
+    uint64_t reserved;
+    struct ffa_address_range address_range_array[];
+};
+
+/* Memory access permissions descriptor */
+struct ffa_mem_access_perm {
+    uint16_t endpoint_id;
+    uint8_t perm;
+    uint8_t flags;
+};
+
+/* Endpoint memory access descriptor */
+struct ffa_mem_access {
+    struct ffa_mem_access_perm access_perm;
+    uint32_t region_offs;
+    uint64_t reserved;
+};
+
+/* Lend, donate or share memory transaction descriptor */
+struct ffa_mem_transaction_1_0 {
+    uint16_t sender_id;
+    uint8_t mem_reg_attr;
+    uint8_t reserved0;
+    uint32_t flags;
+    uint64_t handle;
+    uint64_t tag;
+    uint32_t reserved1;
+    uint32_t mem_access_count;
+    struct ffa_mem_access mem_access_array[];
+};
+
+struct ffa_mem_transaction_1_1 {
+    uint16_t sender_id;
+    uint16_t mem_reg_attr;
+    uint32_t flags;
+    uint64_t handle;
+    uint64_t tag;
+    uint32_t mem_access_size;
+    uint32_t mem_access_count;
+    uint32_t mem_access_offs;
+    uint8_t reserved[12];
+};
+
+/* Endpoint RX/TX descriptor */
+struct ffa_endpoint_rxtx_descriptor_1_0 {
+    uint16_t sender_id;
+    uint16_t reserved;
+    uint32_t rx_range_count;
+    uint32_t tx_range_count;
+};
+
+struct ffa_endpoint_rxtx_descriptor_1_1 {
+    uint16_t sender_id;
+    uint16_t reserved;
+    uint32_t rx_region_offs;
+    uint32_t tx_region_offs;
+};
+
 struct ffa_ctx {
     void *rx;
     const void *tx;
-- 
2.34.1

[XEN PATCH v8 17/22] xen/arm: ffa: support sharing memory

[Jens Wiklander]

Adds support for a guest to share memory with an SP using FFA_MEM_SHARE.
Only memory regions small enough to be shared with a single call to
FFA_MEM_SHARE are supported.

With this commit we have a FF-A version 1.1 [1] mediator able to
communicate with a Secure Partition in secure world using shared memory.
The secure world must use FF-A version 1.1, but the guest is free to use
version 1.0 or version 1.1.

Adds a check that the SP supports the needed FF-A features
FFA_MEM_SHARE_64 or FFA_MEM_SHARE_32.

[1] https://developer.arm.com/documentation/den0077/latest
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
 xen/arch/arm/tee/ffa.c | 483 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 483 insertions(+)

diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
index f3e05911e16e..438e0b21d1ea 100644
--- a/xen/arch/arm/tee/ffa.c
+++ b/xen/arch/arm/tee/ffa.c
@@ -299,6 +299,38 @@ struct ffa_mem_transaction_1_1 {
     uint8_t reserved[12];
 };
 
+/* Calculate offset of struct ffa_mem_access from start of buffer */
+#define MEM_ACCESS_OFFSET(access_idx) \
+    ( sizeof(struct ffa_mem_transaction_1_1) + \
+      ( access_idx ) * sizeof(struct ffa_mem_access) )
+
+/* Calculate offset of struct ffa_mem_region from start of buffer */
+#define REGION_OFFSET(access_count, region_idx) \
+    ( MEM_ACCESS_OFFSET(access_count) + \
+      ( region_idx ) * sizeof(struct ffa_mem_region) )
+
+/* Calculate offset of struct ffa_address_range from start of buffer */
+#define ADDR_RANGE_OFFSET(access_count, region_count, range_idx) \
+    ( REGION_OFFSET(access_count, region_count) + \
+      ( range_idx ) * sizeof(struct ffa_address_range) )
+
+/*
+ * The parts needed from struct ffa_mem_transaction_1_0 or struct
+ * ffa_mem_transaction_1_1, used to provide an abstraction of difference in
+ * data structures between version 1.0 and 1.1. This is just an internal
+ * interface and can be changed without changing any ABI.
+ */
+struct ffa_mem_transaction_int {
+    uint16_t sender_id;
+    uint8_t mem_reg_attr;
+    uint8_t flags;
+    uint8_t mem_access_size;
+    uint8_t mem_access_count;
+    uint16_t mem_access_offs;
+    uint64_t handle;
+    uint64_t tag;
+};
+
 /* Endpoint RX/TX descriptor */
 struct ffa_endpoint_rxtx_descriptor_1_0 {
     uint16_t sender_id;
@@ -324,9 +356,22 @@ struct ffa_ctx {
     /* FF-A version used by the guest */
     uint32_t guest_vers;
     bool rx_is_free;
+    /* Used shared memory objects, struct ffa_shm_mem */
+    struct list_head shm_list;
+    /* Number of allocated shared memory object */
+    unsigned int shm_count;
     spinlock_t lock;
 };
 
+struct ffa_shm_mem {
+    struct list_head list;
+    uint16_t sender_id;
+    uint16_t ep_id;     /* endpoint, the one lending */
+    uint64_t handle;    /* FFA_HANDLE_INVALID if not set yet */
+    unsigned int page_count;
+    struct page_info *pages[];
+};
+
 /* Negotiated FF-A version to use with the SPMC */
 static uint32_t ffa_version __ro_after_init;
 
@@ -348,6 +393,7 @@ static uint16_t subscr_vm_destroyed_count __read_mostly;
 static void *ffa_rx __read_mostly;
 static void *ffa_tx __read_mostly;
 static DEFINE_SPINLOCK(ffa_rx_buffer_lock);
+static DEFINE_SPINLOCK(ffa_tx_buffer_lock);
 
 static bool ffa_get_version(uint32_t *vers)
 {
@@ -454,6 +500,41 @@ static int32_t ffa_rx_release(void)
     return ffa_simple_call(FFA_RX_RELEASE, 0, 0, 0, 0);
 }
 
+static int32_t ffa_mem_share(uint32_t tot_len, uint32_t frag_len,
+                             register_t addr, uint32_t pg_count,
+                             uint64_t *handle)
+{
+    struct arm_smccc_1_2_regs arg = {
+        .a0 = FFA_MEM_SHARE_64,
+        .a1 = tot_len,
+        .a2 = frag_len,
+        .a3 = addr,
+        .a4 = pg_count,
+    };
+    struct arm_smccc_1_2_regs resp;
+
+    arm_smccc_1_2_smc(&arg, &resp);
+
+    switch ( resp.a0 )
+    {
+    case FFA_ERROR:
+        if ( resp.a2 )
+            return resp.a2;
+        else
+            return FFA_RET_NOT_SUPPORTED;
+    case FFA_SUCCESS_32:
+        *handle = regpair_to_uint64(resp.a3, resp.a2);
+        return FFA_RET_OK;
+    case FFA_MEM_FRAG_RX:
+        *handle = regpair_to_uint64(resp.a2, resp.a1);
+        if ( resp.a3 > INT32_MAX ) /* Impossible value */
+            return FFA_RET_ABORTED;
+        return resp.a3 & INT32_MAX;
+    default:
+        return FFA_RET_NOT_SUPPORTED;
+    }
+}
+
 static int32_t ffa_direct_req_send_vm(uint16_t sp_id, uint16_t vm_id,
                                       uint8_t msg)
 {
@@ -781,6 +862,400 @@ out:
              resp.a4 & mask, resp.a5 & mask, resp.a6 & mask, resp.a7 & mask);
 }
 
+/*
+ * Gets all page and assigns them to the supplied shared memory object. If
+ * this function fails then the caller is still expected to call
+ * put_shm_pages() as a cleanup.
+ */
+static int get_shm_pages(struct domain *d, struct ffa_shm_mem *shm,
+                         const struct ffa_address_range *range,
+                         uint32_t range_count, unsigned int start_page_idx,
+                         unsigned int *last_page_idx)
+{
+    unsigned int pg_idx = start_page_idx;
+    gfn_t gfn;
+    unsigned int n;
+    unsigned int m;
+    p2m_type_t t;
+    uint64_t addr;
+
+    for ( n = 0; n < range_count; n++ )
+    {
+        for ( m = 0; m < range[n].page_count; m++ )
+        {
+            if ( pg_idx >= shm->page_count )
+                return FFA_RET_INVALID_PARAMETERS;
+
+            addr = read_atomic(&range[n].address);
+            gfn = gaddr_to_gfn(addr + m * FFA_PAGE_SIZE);
+            shm->pages[pg_idx] = get_page_from_gfn(d, gfn_x(gfn), &t,
+						   P2M_ALLOC);
+            if ( !shm->pages[pg_idx] )
+                return FFA_RET_DENIED;
+            /* Only normal RAM for now */
+            if ( !p2m_is_ram(t) )
+                return FFA_RET_DENIED;
+            pg_idx++;
+        }
+    }
+
+    *last_page_idx = pg_idx;
+
+    return FFA_RET_OK;
+}
+
+static void put_shm_pages(struct ffa_shm_mem *shm)
+{
+    unsigned int n;
+
+    for ( n = 0; n < shm->page_count && shm->pages[n]; n++ )
+    {
+        put_page(shm->pages[n]);
+        shm->pages[n] = NULL;
+    }
+}
+
+static struct ffa_shm_mem *alloc_ffa_shm_mem(struct ffa_ctx *ctx,
+                                             unsigned int page_count)
+{
+    struct ffa_shm_mem *shm;
+
+    if ( page_count >= FFA_MAX_SHM_PAGE_COUNT ||
+         ctx->shm_count >= FFA_MAX_SHM_COUNT )
+        return NULL;
+
+    shm = xzalloc_flex_struct(struct ffa_shm_mem, pages, page_count);
+    if ( shm )
+    {
+        ctx->shm_count++;
+        shm->page_count = page_count;
+    }
+
+    return shm;
+}
+
+static void free_ffa_shm_mem(struct ffa_ctx *ctx, struct ffa_shm_mem *shm)
+{
+    if ( shm ) {
+        ASSERT(ctx->shm_count > 0);
+        ctx->shm_count--;
+        put_shm_pages(shm);
+        xfree(shm);
+    }
+}
+
+static void init_range(struct ffa_address_range *addr_range,
+                       paddr_t pa)
+{
+    memset(addr_range, 0, sizeof(*addr_range));
+    addr_range->address = pa;
+    addr_range->page_count = 1;
+}
+
+/*
+ * This function uses the ffa_tx buffer to transmit the memory transaction
+ * descriptor. The function depends ffa_tx_buffer_lock to be used to guard
+ * the buffer from concurrent use.
+ */
+static int share_shm(struct ffa_shm_mem *shm)
+{
+    const uint32_t max_frag_len = FFA_RXTX_PAGE_COUNT * FFA_PAGE_SIZE;
+    struct ffa_mem_access *mem_access_array;
+    struct ffa_mem_transaction_1_1 *descr;
+    struct ffa_address_range *addr_range;
+    struct ffa_mem_region *region_descr;
+    const unsigned int region_count = 1;
+    void *buf = ffa_tx;
+    uint32_t frag_len;
+    uint32_t tot_len;
+    paddr_t last_pa;
+    unsigned int n;
+    paddr_t pa;
+
+    ASSERT(spin_is_locked(&ffa_tx_buffer_lock));
+    ASSERT(shm->page_count);
+
+    descr = buf;
+    memset(descr, 0, sizeof(*descr));
+    descr->sender_id = shm->sender_id;
+    descr->handle = shm->handle;
+    descr->mem_reg_attr = FFA_NORMAL_MEM_REG_ATTR;
+    descr->mem_access_count = 1;
+    descr->mem_access_size = sizeof(*mem_access_array);
+    descr->mem_access_offs = MEM_ACCESS_OFFSET(0);
+
+    mem_access_array = buf + descr->mem_access_offs;
+    memset(mem_access_array, 0, sizeof(*mem_access_array));
+    mem_access_array[0].access_perm.endpoint_id = shm->ep_id;
+    mem_access_array[0].access_perm.perm = FFA_MEM_ACC_RW;
+    mem_access_array[0].region_offs = REGION_OFFSET(descr->mem_access_count, 0);
+
+    region_descr = buf + mem_access_array[0].region_offs;
+    memset(region_descr, 0, sizeof(*region_descr));
+    region_descr->total_page_count = shm->page_count;
+
+    region_descr->address_range_count = 1;
+    last_pa = page_to_maddr(shm->pages[0]);
+    for ( n = 1; n < shm->page_count; last_pa = pa, n++ )
+    {
+        pa = page_to_maddr(shm->pages[n]);
+        if ( last_pa + FFA_PAGE_SIZE == pa )
+            continue;
+        region_descr->address_range_count++;
+    }
+
+    tot_len = ADDR_RANGE_OFFSET(descr->mem_access_count, region_count,
+                                region_descr->address_range_count);
+    if ( tot_len > max_frag_len )
+        return FFA_RET_NOT_SUPPORTED;
+
+    addr_range = region_descr->address_range_array;
+    frag_len = ADDR_RANGE_OFFSET(descr->mem_access_count, region_count, 1);
+    last_pa = page_to_maddr(shm->pages[0]);
+    init_range(addr_range, last_pa);
+    for ( n = 1; n < shm->page_count; last_pa = pa, n++ )
+    {
+        pa = page_to_maddr(shm->pages[n]);
+        if ( last_pa + FFA_PAGE_SIZE == pa )
+        {
+            addr_range->page_count++;
+            continue;
+        }
+
+        frag_len += sizeof(*addr_range);
+        addr_range++;
+        init_range(addr_range, pa);
+    }
+
+    return ffa_mem_share(tot_len, frag_len, 0, 0, &shm->handle);
+}
+
+static int read_mem_transaction(uint32_t ffa_vers, const void *buf, size_t blen,
+                                struct ffa_mem_transaction_int *trans)
+{
+    uint16_t mem_reg_attr;
+    uint32_t flags;
+    uint32_t count;
+    uint32_t offs;
+    uint32_t size;
+
+    if ( ffa_vers >= FFA_VERSION_1_1 )
+    {
+        const struct ffa_mem_transaction_1_1 *descr;
+
+        if ( blen < sizeof(*descr) )
+            return FFA_RET_INVALID_PARAMETERS;
+
+        descr = buf;
+        trans->sender_id = descr->sender_id;
+        mem_reg_attr = descr->mem_reg_attr;
+        flags = descr->flags;
+        trans->handle = descr->handle;
+        trans->tag = descr->tag;
+
+        count = descr->mem_access_count;
+        size = descr->mem_access_size;
+        offs = descr->mem_access_offs;
+    }
+    else
+    {
+        const struct ffa_mem_transaction_1_0 *descr;
+
+        if ( blen < sizeof(*descr) )
+            return FFA_RET_INVALID_PARAMETERS;
+
+        descr = buf;
+        trans->sender_id = descr->sender_id;
+        mem_reg_attr = descr->mem_reg_attr;
+        flags = descr->flags;
+        trans->handle = descr->handle;
+        trans->tag = descr->tag;
+
+        count = descr->mem_access_count;
+        size = sizeof(struct ffa_mem_access);
+        offs = offsetof(struct ffa_mem_transaction_1_0, mem_access_array);
+    }
+    /*
+     * Make sure that "descr" which is shared with the guest isn't accessed
+     * again after this point.
+     */
+    barrier();
+
+    /*
+     * We're doing a rough check to see that no information is lost when
+     * tranfering the values into a struct ffa_mem_transaction_int below.
+     * The fields in struct ffa_mem_transaction_int are wide enough to hold
+     * any valid value so being out of range means that something is wrong.
+     */
+    if ( mem_reg_attr > UINT8_MAX || flags > UINT8_MAX || size > UINT8_MAX ||
+        count > UINT8_MAX || offs > UINT16_MAX )
+        return FFA_RET_INVALID_PARAMETERS;
+
+    /* Check that the endpoint memory access descriptor array fits */
+    if ( size * count + offs > blen )
+        return FFA_RET_INVALID_PARAMETERS;
+
+    trans->mem_reg_attr = mem_reg_attr;
+    trans->flags = flags;
+    trans->mem_access_size = size;
+    trans->mem_access_count = count;
+    trans->mem_access_offs = offs;
+
+    return 0;
+}
+
+static void handle_mem_share(struct cpu_user_regs *regs)
+{
+    uint32_t tot_len = get_user_reg(regs, 1);
+    uint32_t frag_len = get_user_reg(regs, 2);
+    uint64_t addr = get_user_reg(regs, 3);
+    uint32_t page_count = get_user_reg(regs, 4);
+    const struct ffa_mem_region *region_descr;
+    const struct ffa_mem_access *mem_access;
+    struct ffa_mem_transaction_int trans;
+    struct domain *d = current->domain;
+    struct ffa_ctx *ctx = d->arch.tee;
+    struct ffa_shm_mem *shm = NULL;
+    unsigned int last_page_idx = 0;
+    register_t handle_hi = 0;
+    register_t handle_lo = 0;
+    int ret = FFA_RET_DENIED;
+    uint32_t range_count;
+    uint32_t region_offs;
+
+    /*
+     * We're only accepting memory transaction descriptors via the rx/tx
+     * buffer.
+     */
+    if ( addr )
+    {
+        ret = FFA_RET_NOT_SUPPORTED;
+        goto out_set_ret;
+    }
+
+    /* Check that fragment length doesn't exceed total length */
+    if ( frag_len > tot_len )
+    {
+        ret = FFA_RET_INVALID_PARAMETERS;
+        goto out_set_ret;
+    }
+
+    /* We currently only support a single fragment */
+    if ( frag_len != tot_len )
+    {
+        ret = FFA_RET_NOT_SUPPORTED;
+        goto out_set_ret;
+    }
+
+    spin_lock(&ctx->lock);
+
+    if ( frag_len > ctx->page_count * FFA_PAGE_SIZE )
+        goto out_unlock;
+
+    ret = read_mem_transaction(ctx->guest_vers, ctx->tx, frag_len, &trans);
+    if ( ret )
+        goto out_unlock;
+
+    if ( trans.mem_reg_attr != FFA_NORMAL_MEM_REG_ATTR )
+    {
+        ret = FFA_RET_NOT_SUPPORTED;
+        goto out_unlock;
+    }
+
+    /* Only supports sharing it with one SP for now */
+    if ( trans.mem_access_count != 1 )
+    {
+        ret = FFA_RET_NOT_SUPPORTED;
+        goto out_unlock;
+    }
+
+    if ( trans.sender_id != get_vm_id(d) )
+    {
+        ret = FFA_RET_INVALID_PARAMETERS;
+        goto out_unlock;
+    }
+
+    /* Check that it fits in the supplied data */
+    if ( trans.mem_access_offs + trans.mem_access_size > frag_len )
+        goto out_unlock;
+
+    mem_access = ctx->tx + trans.mem_access_offs;
+    if ( read_atomic(&mem_access->access_perm.perm) != FFA_MEM_ACC_RW )
+    {
+        ret = FFA_RET_NOT_SUPPORTED;
+        goto out_unlock;
+    }
+
+    region_offs = read_atomic(&mem_access->region_offs);
+    if ( sizeof(*region_descr) + region_offs > frag_len )
+    {
+        ret = FFA_RET_NOT_SUPPORTED;
+        goto out_unlock;
+    }
+
+    region_descr = ctx->tx + region_offs;
+    range_count = read_atomic(&region_descr->address_range_count);
+    page_count = read_atomic(&region_descr->total_page_count);
+
+    if ( !page_count )
+    {
+        ret = FFA_RET_INVALID_PARAMETERS;
+        goto out_unlock;
+    }
+
+    shm = alloc_ffa_shm_mem(ctx, page_count);
+    if ( !shm )
+    {
+        ret = FFA_RET_NO_MEMORY;
+        goto out_unlock;
+    }
+    shm->sender_id = trans.sender_id;
+    shm->ep_id = read_atomic(&mem_access->access_perm.endpoint_id);
+
+    /*
+     * Check that the Composite memory region descriptor fits.
+     */
+    if ( sizeof(*region_descr) + region_offs +
+         range_count * sizeof(struct ffa_address_range) > frag_len )
+    {
+        ret = FFA_RET_INVALID_PARAMETERS;
+        goto out;
+    }
+
+    ret = get_shm_pages(d, shm, region_descr->address_range_array, range_count,
+                        0, &last_page_idx);
+    if ( ret )
+        goto out;
+    if ( last_page_idx != shm->page_count )
+    {
+        ret = FFA_RET_INVALID_PARAMETERS;
+        goto out;
+    }
+
+    /* Note that share_shm() uses our tx buffer */
+    spin_lock(&ffa_tx_buffer_lock);
+    ret = share_shm(shm);
+    spin_unlock(&ffa_tx_buffer_lock);
+    if ( ret )
+        goto out;
+
+    list_add_tail(&shm->list, &ctx->shm_list);
+
+    uint64_to_regpair(&handle_hi, &handle_lo, shm->handle);
+
+out:
+    if ( ret )
+        free_ffa_shm_mem(ctx, shm);
+out_unlock:
+    spin_unlock(&ctx->lock);
+
+out_set_ret:
+    if ( ret == 0)
+            set_regs_success(regs, handle_lo, handle_hi);
+    else
+            set_regs_error(regs, ret);
+}
+
 static bool ffa_handle_call(struct cpu_user_regs *regs)
 {
     uint32_t fid = get_user_reg(regs, 0);
@@ -838,6 +1313,10 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
     case FFA_MSG_SEND_DIRECT_REQ_64:
         handle_msg_send_direct_req(regs, fid);
         return true;
+    case FFA_MEM_SHARE_32:
+    case FFA_MEM_SHARE_64:
+        handle_mem_share(regs);
+        return true;
 
     default:
         gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid);
@@ -879,6 +1358,8 @@ static int ffa_domain_init(struct domain *d)
         }
     }
 
+    INIT_LIST_HEAD(&ctx->shm_list);
+
     d->arch.tee = ctx;
 
     return 0;
@@ -1050,7 +1531,9 @@ static bool ffa_probe(void)
     if ( !check_mandatory_feature(FFA_PARTITION_INFO_GET) ||
          !check_mandatory_feature(FFA_RX_RELEASE) ||
          !check_mandatory_feature(FFA_RXTX_MAP_64) ||
+         !check_mandatory_feature(FFA_MEM_SHARE_64) ||
          !check_mandatory_feature(FFA_RXTX_UNMAP) ||
+         !check_mandatory_feature(FFA_MEM_SHARE_32) ||
          !check_mandatory_feature(FFA_MSG_SEND_DIRECT_REQ_32) )
         return false;
 
-- 
2.34.1

[XEN PATCH v8 18/22] xen/arm: ffa: add support to reclaim shared memory

[Jens Wiklander]

Adds support to reclaim memory previously shared with FFA_MEM_SHARE.

A memory region that doesn't need to be shared any longer can be
reclaimed with FFA_MEM_RECLAIM once the SP doesn't use it any longer.
This is checked by the SPMC and not in control of the mediator.

Adds a check that the SP supports the needed FF-A feature
FFA_MEM_RECLAIM.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
---
 xen/arch/arm/tee/ffa.c | 53 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
index 438e0b21d1ea..47ff899eca32 100644
--- a/xen/arch/arm/tee/ffa.c
+++ b/xen/arch/arm/tee/ffa.c
@@ -535,6 +535,12 @@ static int32_t ffa_mem_share(uint32_t tot_len, uint32_t frag_len,
     }
 }
 
+static int32_t ffa_mem_reclaim(uint32_t handle_lo, uint32_t handle_hi,
+                               uint32_t flags)
+{
+    return ffa_simple_call(FFA_MEM_RECLAIM, handle_lo, handle_hi, flags, 0);
+}
+
 static int32_t ffa_direct_req_send_vm(uint16_t sp_id, uint16_t vm_id,
                                       uint8_t msg)
 {
@@ -1256,6 +1262,43 @@ out_set_ret:
             set_regs_error(regs, ret);
 }
 
+static int handle_mem_reclaim(uint64_t handle, uint32_t flags)
+{
+    struct domain *d = current->domain;
+    struct ffa_ctx *ctx = d->arch.tee;
+    struct ffa_shm_mem *shm;
+    register_t handle_hi;
+    register_t handle_lo;
+    int ret;
+
+    spin_lock(&ctx->lock);
+    list_for_each_entry(shm, &ctx->shm_list, list)
+    {
+        if ( shm->handle == handle )
+            goto found_it;
+    }
+    shm = NULL;
+    ret = FFA_RET_INVALID_PARAMETERS;
+    goto out;
+found_it:
+
+    uint64_to_regpair(&handle_hi, &handle_lo, handle);
+    ret = ffa_mem_reclaim(handle_lo, handle_hi, flags);
+    if ( ret )
+    {
+        shm = NULL;
+        goto out;
+    }
+
+    list_del(&shm->list);
+
+out:
+    free_ffa_shm_mem(ctx, shm);
+    spin_unlock(&ctx->lock);
+
+    return ret;
+}
+
 static bool ffa_handle_call(struct cpu_user_regs *regs)
 {
     uint32_t fid = get_user_reg(regs, 0);
@@ -1317,6 +1360,15 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
     case FFA_MEM_SHARE_64:
         handle_mem_share(regs);
         return true;
+    case FFA_MEM_RECLAIM:
+        e = handle_mem_reclaim(regpair_to_uint64(get_user_reg(regs, 2),
+                                                 get_user_reg(regs, 1)),
+                               get_user_reg(regs, 3));
+        if ( e )
+            set_regs_error(regs, e);
+        else
+            set_regs_success(regs, 0, 0);
+        return true;
 
     default:
         gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid);
@@ -1534,6 +1586,7 @@ static bool ffa_probe(void)
          !check_mandatory_feature(FFA_MEM_SHARE_64) ||
          !check_mandatory_feature(FFA_RXTX_UNMAP) ||
          !check_mandatory_feature(FFA_MEM_SHARE_32) ||
+         !check_mandatory_feature(FFA_MEM_RECLAIM) ||
          !check_mandatory_feature(FFA_MSG_SEND_DIRECT_REQ_32) )
         return false;
 
-- 
2.34.1

[XEN PATCH v8 19/22] xen/arm: ffa: support sharing large memory ranges

[Jens Wiklander]

Adds support for sharing large memory ranges transmitted in fragments
using FFA_MEM_FRAG_TX.

The implementation is the bare minimum to be able to communicate with
OP-TEE running as an SPMC at S-EL1.

Adds a check that the SP supports the needed FF-A feature
FFA_MEM_FRAG_TX.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
 xen/arch/arm/tee/ffa.c | 253 ++++++++++++++++++++++++++++++++++++++---
 1 file changed, 240 insertions(+), 13 deletions(-)

diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
index 47ff899eca32..888e3f9265c2 100644
--- a/xen/arch/arm/tee/ffa.c
+++ b/xen/arch/arm/tee/ffa.c
@@ -356,6 +356,8 @@ struct ffa_ctx {
     /* FF-A version used by the guest */
     uint32_t guest_vers;
     bool rx_is_free;
+    /* Currently used fragment states, struct mem_frag_state */
+    struct list_head frag_list;
     /* Used shared memory objects, struct ffa_shm_mem */
     struct list_head shm_list;
     /* Number of allocated shared memory object */
@@ -372,6 +374,18 @@ struct ffa_shm_mem {
     struct page_info *pages[];
 };
 
+struct mem_frag_state {
+    struct list_head list;
+    struct ffa_shm_mem *shm;
+    uint32_t range_count;
+    unsigned int current_page_idx;
+    unsigned int frag_offset;
+    unsigned int range_offset;
+    const uint8_t *buf;
+    unsigned int buf_size;
+    struct ffa_address_range range;
+};
+
 /* Negotiated FF-A version to use with the SPMC */
 static uint32_t ffa_version __ro_after_init;
 
@@ -535,6 +549,36 @@ static int32_t ffa_mem_share(uint32_t tot_len, uint32_t frag_len,
     }
 }
 
+static int32_t ffa_mem_frag_tx(uint64_t handle, uint32_t frag_len,
+                               uint16_t sender_id)
+{
+    struct arm_smccc_1_2_regs arg = {
+        .a0 = FFA_MEM_FRAG_TX,
+        .a1 = handle & UINT32_MAX,
+        .a2 = handle >> 32,
+        .a3 = frag_len,
+        .a4 = (uint32_t)sender_id << 16,
+    };
+    struct arm_smccc_1_2_regs resp;
+
+    arm_smccc_1_2_smc(&arg, &resp);
+
+    switch ( resp.a0 )
+    {
+    case FFA_ERROR:
+        if ( resp.a2 )
+            return resp.a2;
+        else
+            return FFA_RET_NOT_SUPPORTED;
+    case FFA_SUCCESS_32:
+        return FFA_RET_OK;
+    case FFA_MEM_FRAG_RX:
+        return resp.a3;
+    default:
+            return FFA_RET_NOT_SUPPORTED;
+    }
+}
+
 static int32_t ffa_mem_reclaim(uint32_t handle_lo, uint32_t handle_hi,
                                uint32_t flags)
 {
@@ -609,6 +653,14 @@ static void set_regs_success(struct cpu_user_regs *regs, uint32_t w2,
     set_regs(regs, FFA_SUCCESS_32, 0, w2, w3, 0, 0, 0, 0);
 }
 
+static void set_regs_frag_rx(struct cpu_user_regs *regs, uint32_t handle_lo,
+                             uint32_t handle_hi, uint32_t frag_offset,
+                             uint16_t sender_id)
+{
+    set_regs(regs, FFA_MEM_FRAG_RX, handle_lo, handle_hi, frag_offset,
+             (uint32_t)sender_id << 16, 0, 0, 0);
+}
+
 static void handle_version(struct cpu_user_regs *regs)
 {
     struct domain *d = current->domain;
@@ -977,6 +1029,8 @@ static int share_shm(struct ffa_shm_mem *shm)
     paddr_t last_pa;
     unsigned int n;
     paddr_t pa;
+    bool first;
+    int ret;
 
     ASSERT(spin_is_locked(&ffa_tx_buffer_lock));
     ASSERT(shm->page_count);
@@ -1012,13 +1066,23 @@ static int share_shm(struct ffa_shm_mem *shm)
 
     tot_len = ADDR_RANGE_OFFSET(descr->mem_access_count, region_count,
                                 region_descr->address_range_count);
-    if ( tot_len > max_frag_len )
-        return FFA_RET_NOT_SUPPORTED;
 
+    /*
+     * Sharing memory with secure world may have to be done with multiple
+     * calls depending on how many address ranges will be needed. If we're
+     * sharing physically contiguous memory we will only need one range but
+     * we will also need to deal with the worst case where all physical
+     * pages are non-contiguous. For the first batch of address ranges we
+     * call ffa_mem_share() and for all that follows ffa_mem_frag_tx().
+     *
+     * We use frag_len to keep track of how far into the transmit buffer we
+     * have gone.
+     */
     addr_range = region_descr->address_range_array;
     frag_len = ADDR_RANGE_OFFSET(descr->mem_access_count, region_count, 1);
     last_pa = page_to_maddr(shm->pages[0]);
     init_range(addr_range, last_pa);
+    first = true;
     for ( n = 1; n < shm->page_count; last_pa = pa, n++ )
     {
         pa = page_to_maddr(shm->pages[n]);
@@ -1028,12 +1092,34 @@ static int share_shm(struct ffa_shm_mem *shm)
             continue;
         }
 
-        frag_len += sizeof(*addr_range);
-        addr_range++;
+        if ( frag_len == max_frag_len )
+        {
+            if ( first )
+            {
+                ret = ffa_mem_share(tot_len, frag_len, 0, 0, &shm->handle);
+                first = false;
+            }
+            else
+            {
+                ret = ffa_mem_frag_tx(shm->handle, frag_len, shm->sender_id);
+            }
+            if ( ret <= 0 )
+                return ret;
+            frag_len = sizeof(*addr_range);
+            addr_range = buf;
+        }
+        else
+        {
+            frag_len += sizeof(*addr_range);
+            addr_range++;
+        }
         init_range(addr_range, pa);
     }
 
-    return ffa_mem_share(tot_len, frag_len, 0, 0, &shm->handle);
+    if ( first )
+        return ffa_mem_share(tot_len, frag_len, 0, 0, &shm->handle);
+    else
+        return ffa_mem_frag_tx(shm->handle, frag_len, shm->sender_id);
 }
 
 static int read_mem_transaction(uint32_t ffa_vers, const void *buf, size_t blen,
@@ -1110,8 +1196,53 @@ static int read_mem_transaction(uint32_t ffa_vers, const void *buf, size_t blen,
     return 0;
 }
 
+static int add_mem_share_frag(struct mem_frag_state *s, unsigned int offs,
+                              unsigned int frag_len)
+{
+    struct domain *d = current->domain;
+    unsigned int o = offs;
+    unsigned int l;
+    int ret;
+
+    if ( frag_len < o )
+        return FFA_RET_INVALID_PARAMETERS;
+
+    /* Fill up the first struct ffa_address_range */
+    l = min_t(unsigned int, frag_len - o, sizeof(s->range) - s->range_offset);
+    memcpy((uint8_t *)&s->range + s->range_offset, s->buf + o, l);
+    s->range_offset += l;
+    o += l;
+    if ( s->range_offset != sizeof(s->range) )
+        goto out;
+    s->range_offset = 0;
+
+    while ( true )
+    {
+        ret = get_shm_pages(d, s->shm, &s->range, 1, s->current_page_idx,
+                            &s->current_page_idx);
+        if ( ret )
+            return ret;
+        if ( s->range_count == 1 )
+            return 0;
+        s->range_count--;
+        if ( frag_len - o < sizeof(s->range) )
+            break;
+        memcpy(&s->range, s->buf + o, sizeof(s->range));
+        o += sizeof(s->range);
+    }
+
+    /* Collect any remaining bytes for the next struct ffa_address_range */
+    s->range_offset = frag_len - o;
+    memcpy(&s->range, s->buf + o, frag_len - o);
+out:
+    s->frag_offset += frag_len;
+
+    return s->frag_offset;
+}
+
 static void handle_mem_share(struct cpu_user_regs *regs)
 {
+    static uint64_t next_handle = FFA_HANDLE_HYP_FLAG;
     uint32_t tot_len = get_user_reg(regs, 1);
     uint32_t frag_len = get_user_reg(regs, 2);
     uint64_t addr = get_user_reg(regs, 3);
@@ -1146,13 +1277,6 @@ static void handle_mem_share(struct cpu_user_regs *regs)
         goto out_set_ret;
     }
 
-    /* We currently only support a single fragment */
-    if ( frag_len != tot_len )
-    {
-        ret = FFA_RET_NOT_SUPPORTED;
-        goto out_set_ret;
-    }
-
     spin_lock(&ctx->lock);
 
     if ( frag_len > ctx->page_count * FFA_PAGE_SIZE )
@@ -1218,6 +1342,36 @@ static void handle_mem_share(struct cpu_user_regs *regs)
     shm->sender_id = trans.sender_id;
     shm->ep_id = read_atomic(&mem_access->access_perm.endpoint_id);
 
+    if ( frag_len != tot_len )
+    {
+        struct mem_frag_state *s = xzalloc(struct mem_frag_state);
+
+        if ( !s )
+        {
+            ret = FFA_RET_NO_MEMORY;
+            goto out;
+        }
+        s->shm = shm;
+        s->range_count = range_count;
+        s->buf = ctx->tx;
+        s->buf_size = FFA_RXTX_PAGE_COUNT * FFA_PAGE_SIZE;
+        ret = add_mem_share_frag(s, sizeof(*region_descr)  + region_offs,
+                                 frag_len);
+        if ( ret <= 0 )
+        {
+            xfree(s);
+            if ( ret < 0 )
+                goto out;
+        }
+        else
+        {
+            shm->handle = next_handle++;
+            uint64_to_regpair(&handle_hi, &handle_lo, shm->handle);
+            list_add_tail(&s->list, &ctx->frag_list);
+        }
+        goto out_unlock;
+    }
+
     /*
      * Check that the Composite memory region descriptor fits.
      */
@@ -1256,7 +1410,75 @@ out_unlock:
     spin_unlock(&ctx->lock);
 
 out_set_ret:
-    if ( ret == 0)
+    if ( ret > 0 )
+            set_regs_frag_rx(regs, handle_lo, handle_hi, ret, trans.sender_id);
+    else if ( ret == 0)
+            set_regs_success(regs, handle_lo, handle_hi);
+    else
+            set_regs_error(regs, ret);
+}
+
+static struct mem_frag_state *find_frag_state(struct ffa_ctx *ctx,
+                                              uint64_t handle)
+{
+    struct mem_frag_state *s;
+
+    list_for_each_entry(s, &ctx->frag_list, list)
+        if ( s->shm->handle == handle )
+            return s;
+
+    return NULL;
+}
+
+static void handle_mem_frag_tx(struct cpu_user_regs *regs)
+{
+    struct domain *d = current->domain;
+    struct ffa_ctx *ctx = d->arch.tee;
+    uint32_t frag_len = get_user_reg(regs, 3);
+    uint32_t handle_lo = get_user_reg(regs, 1);
+    uint32_t handle_hi = get_user_reg(regs, 2);
+    uint64_t handle = regpair_to_uint64(handle_hi, handle_lo);
+    struct mem_frag_state *s;
+    uint16_t sender_id = 0;
+    int ret;
+
+    spin_lock(&ctx->lock);
+    s = find_frag_state(ctx, handle);
+    if ( !s )
+    {
+        ret = FFA_RET_INVALID_PARAMETERS;
+        goto out;
+    }
+    sender_id = s->shm->sender_id;
+
+    if ( frag_len > s->buf_size )
+    {
+        ret = FFA_RET_INVALID_PARAMETERS;
+        goto out_free_s;
+    }
+
+    ret = add_mem_share_frag(s, 0, frag_len);
+    if ( ret < 0 )
+        goto out_free_s;
+
+    /* Note that share_shm() uses our tx buffer */
+    spin_lock(&ffa_tx_buffer_lock);
+    ret = share_shm(s->shm);
+    spin_unlock(&ffa_tx_buffer_lock);
+    if ( ret < 0 )
+        goto out_free_s;
+    list_add_tail(&s->shm->list, &ctx->shm_list);
+out_free_s:
+    if ( ret < 0 )
+        free_ffa_shm_mem(ctx, s->shm);
+    list_del(&s->list);
+    xfree(s);
+out:
+    spin_unlock(&ctx->lock);
+
+    if ( ret > 0 )
+            set_regs_frag_rx(regs, handle_lo, handle_hi, ret, sender_id);
+    else if ( ret == 0)
             set_regs_success(regs, handle_lo, handle_hi);
     else
             set_regs_error(regs, ret);
@@ -1369,6 +1591,9 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
         else
             set_regs_success(regs, 0, 0);
         return true;
+    case FFA_MEM_FRAG_TX:
+        handle_mem_frag_tx(regs);
+        return true;
 
     default:
         gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid);
@@ -1410,6 +1635,7 @@ static int ffa_domain_init(struct domain *d)
         }
     }
 
+    INIT_LIST_HEAD(&ctx->frag_list);
     INIT_LIST_HEAD(&ctx->shm_list);
 
     d->arch.tee = ctx;
@@ -1586,6 +1812,7 @@ static bool ffa_probe(void)
          !check_mandatory_feature(FFA_MEM_SHARE_64) ||
          !check_mandatory_feature(FFA_RXTX_UNMAP) ||
          !check_mandatory_feature(FFA_MEM_SHARE_32) ||
+         !check_mandatory_feature(FFA_MEM_FRAG_TX) ||
          !check_mandatory_feature(FFA_MEM_RECLAIM) ||
          !check_mandatory_feature(FFA_MSG_SEND_DIRECT_REQ_32) )
         return false;
-- 
2.34.1

[XEN PATCH v8 20/22] xen/arm: ffa: improve lock granularity

[Jens Wiklander]

The single lock in struct ffa_ctx is complemented with rx_lock and tx_lock.

The old lock is used for small critical sections, like increasing
shm_count or adding another shm to shm_list.

rx_lock and tx_lock are only acquired using spin_trylock() which for
well-behaving guests should always succeed. Guests using the RX and TX
buffers are expected to serialize accesses before doing the FF-A
request.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
 xen/arch/arm/tee/ffa.c | 124 ++++++++++++++++++++++++++++++-----------
 1 file changed, 91 insertions(+), 33 deletions(-)

diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
index 888e3f9265c2..0948cc636871 100644
--- a/xen/arch/arm/tee/ffa.c
+++ b/xen/arch/arm/tee/ffa.c
@@ -362,6 +362,13 @@ struct ffa_ctx {
     struct list_head shm_list;
     /* Number of allocated shared memory object */
     unsigned int shm_count;
+    /*
+     * tx_lock is used to serialize access to tx
+     * rx_lock is used to serialize access to rx
+     * lock is used for the rest in this struct
+     */
+    spinlock_t tx_lock;
+    spinlock_t rx_lock;
     spinlock_t lock;
 };
 
@@ -796,7 +803,9 @@ static int32_t handle_partition_info_get(uint32_t w1, uint32_t w2, uint32_t w3,
     if ( !ffa_rx )
         return FFA_RET_DENIED;
 
-    spin_lock(&ctx->lock);
+    if ( !spin_trylock(&ctx->rx_lock) )
+        return FFA_RET_BUSY;
+
     if ( !ctx->page_count || !ctx->rx_is_free )
         goto out;
     spin_lock(&ffa_rx_buffer_lock);
@@ -847,7 +856,7 @@ out_rx_release:
 out_rx_buf_unlock:
     spin_unlock(&ffa_rx_buffer_lock);
 out:
-    spin_unlock(&ctx->lock);
+    spin_unlock(&ctx->rx_lock);
 
     return ret;
 }
@@ -858,13 +867,15 @@ static int32_t handle_rx_release(void)
     struct domain *d = current->domain;
     struct ffa_ctx *ctx = d->arch.tee;
 
-    spin_lock(&ctx->lock);
+    if ( !spin_trylock(&ctx->rx_lock) )
+        return FFA_RET_BUSY;
+
     if ( !ctx->page_count || ctx->rx_is_free )
         goto out;
     ret = FFA_RET_OK;
     ctx->rx_is_free = true;
 out:
-    spin_unlock(&ctx->lock);
+    spin_unlock(&ctx->rx_lock);
 
     return ret;
 }
@@ -973,30 +984,52 @@ static void put_shm_pages(struct ffa_shm_mem *shm)
     }
 }
 
+static bool inc_ctx_shm_count(struct ffa_ctx *ctx)
+{
+    bool ret = true;
+
+    spin_lock(&ctx->lock);
+    if (ctx->shm_count >= FFA_MAX_SHM_COUNT)
+        ret = false;
+    else
+        ctx->shm_count++;
+    spin_unlock(&ctx->lock);
+
+    return ret;
+}
+
+static void dec_ctx_shm_count(struct ffa_ctx *ctx)
+{
+    spin_lock(&ctx->lock);
+    ASSERT(ctx->shm_count > 0);
+    ctx->shm_count--;
+    spin_unlock(&ctx->lock);
+}
+
 static struct ffa_shm_mem *alloc_ffa_shm_mem(struct ffa_ctx *ctx,
                                              unsigned int page_count)
 {
     struct ffa_shm_mem *shm;
 
-    if ( page_count >= FFA_MAX_SHM_PAGE_COUNT ||
-         ctx->shm_count >= FFA_MAX_SHM_COUNT )
+    if ( page_count >= FFA_MAX_SHM_PAGE_COUNT )
+        return NULL;
+    if ( !inc_ctx_shm_count(ctx) )
         return NULL;
 
     shm = xzalloc_flex_struct(struct ffa_shm_mem, pages, page_count);
     if ( shm )
-    {
-        ctx->shm_count++;
         shm->page_count = page_count;
-    }
+    else
+        dec_ctx_shm_count(ctx);
 
     return shm;
 }
 
 static void free_ffa_shm_mem(struct ffa_ctx *ctx, struct ffa_shm_mem *shm)
 {
-    if ( shm ) {
-        ASSERT(ctx->shm_count > 0);
-        ctx->shm_count--;
+    if ( shm )
+    {
+        dec_ctx_shm_count(ctx);
         put_shm_pages(shm);
         xfree(shm);
     }
@@ -1277,7 +1310,11 @@ static void handle_mem_share(struct cpu_user_regs *regs)
         goto out_set_ret;
     }
 
-    spin_lock(&ctx->lock);
+    if ( !spin_trylock(&ctx->tx_lock) )
+    {
+        ret = FFA_RET_BUSY;
+        goto out_set_ret;
+    }
 
     if ( frag_len > ctx->page_count * FFA_PAGE_SIZE )
         goto out_unlock;
@@ -1399,7 +1436,9 @@ static void handle_mem_share(struct cpu_user_regs *regs)
     if ( ret )
         goto out;
 
+    spin_lock(&ctx->lock);
     list_add_tail(&shm->list, &ctx->shm_list);
+    spin_unlock(&ctx->lock);
 
     uint64_to_regpair(&handle_hi, &handle_lo, shm->handle);
 
@@ -1407,7 +1446,7 @@ out:
     if ( ret )
         free_ffa_shm_mem(ctx, shm);
 out_unlock:
-    spin_unlock(&ctx->lock);
+    spin_unlock(&ctx->tx_lock);
 
 out_set_ret:
     if ( ret > 0 )
@@ -1442,7 +1481,12 @@ static void handle_mem_frag_tx(struct cpu_user_regs *regs)
     uint16_t sender_id = 0;
     int ret;
 
-    spin_lock(&ctx->lock);
+    if ( !spin_trylock(&ctx->tx_lock) )
+    {
+        ret = FFA_RET_BUSY;
+        goto out_set_ret;
+    }
+
     s = find_frag_state(ctx, handle);
     if ( !s )
     {
@@ -1467,15 +1511,20 @@ static void handle_mem_frag_tx(struct cpu_user_regs *regs)
     spin_unlock(&ffa_tx_buffer_lock);
     if ( ret < 0 )
         goto out_free_s;
+
+    spin_lock(&ctx->lock);
     list_add_tail(&s->shm->list, &ctx->shm_list);
+    spin_unlock(&ctx->lock);
+
 out_free_s:
     if ( ret < 0 )
         free_ffa_shm_mem(ctx, s->shm);
     list_del(&s->list);
     xfree(s);
 out:
-    spin_unlock(&ctx->lock);
+    spin_unlock(&ctx->tx_lock);
 
+out_set_ret:
     if ( ret > 0 )
             set_regs_frag_rx(regs, handle_lo, handle_hi, ret, sender_id);
     else if ( ret == 0)
@@ -1484,6 +1533,18 @@ out:
             set_regs_error(regs, ret);
 }
 
+/* Must only be called with ctx->lock held */
+static struct ffa_shm_mem *find_shm_mem(struct ffa_ctx *ctx, uint64_t handle)
+{
+    struct ffa_shm_mem *shm;
+
+    list_for_each_entry(shm, &ctx->shm_list, list)
+        if ( shm->handle == handle )
+            return shm;
+
+    return NULL;
+}
+
 static int handle_mem_reclaim(uint64_t handle, uint32_t flags)
 {
     struct domain *d = current->domain;
@@ -1494,29 +1555,26 @@ static int handle_mem_reclaim(uint64_t handle, uint32_t flags)
     int ret;
 
     spin_lock(&ctx->lock);
-    list_for_each_entry(shm, &ctx->shm_list, list)
-    {
-        if ( shm->handle == handle )
-            goto found_it;
-    }
-    shm = NULL;
-    ret = FFA_RET_INVALID_PARAMETERS;
-    goto out;
-found_it:
+    shm = find_shm_mem(ctx, handle);
+    if ( shm )
+        list_del(&shm->list);
+    spin_unlock(&ctx->lock);
+    if ( !shm )
+        return FFA_RET_INVALID_PARAMETERS;
 
     uint64_to_regpair(&handle_hi, &handle_lo, handle);
     ret = ffa_mem_reclaim(handle_lo, handle_hi, flags);
+
     if ( ret )
     {
-        shm = NULL;
-        goto out;
+        spin_lock(&ctx->lock);
+        list_add_tail(&shm->list, &ctx->shm_list);
+        spin_unlock(&ctx->lock);
+    }
+    else
+    {
+        free_ffa_shm_mem(ctx, shm);
     }
-
-    list_del(&shm->list);
-
-out:
-    free_ffa_shm_mem(ctx, shm);
-    spin_unlock(&ctx->lock);
 
     return ret;
 }
-- 
2.34.1

[XEN PATCH v8 21/22] xen/arm: ffa: list current limitations

[Jens Wiklander]

Adds a comments with a list of unsupported FF-A interfaces and
limitations in the implemented FF-A interfaces.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
 xen/arch/arm/tee/ffa.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
index 0948cc636871..6424c222c885 100644
--- a/xen/arch/arm/tee/ffa.c
+++ b/xen/arch/arm/tee/ffa.c
@@ -13,6 +13,38 @@
  *                https://developer.arm.com/documentation/den0077/e
  * TEEC-1.0C: TEE Client API Specification version 1.0c available at
  *            https://globalplatform.org/specs-library/tee-client-api-specification/
+ *
+ * Notes on the the current implementstion.
+ *
+ * Unsupported FF-A interfaces:
+ * o FFA_MSG_POLL and FFA_MSG_SEND - deprecated in FF-A-1.1-REL0
+ * o FFA_MEM_RETRIEVE_* - Used when sharing memory from an SP to a VM
+ * o FFA_MEM_DONATE_* and FFA_MEM_LEND_* - Used when tranferring ownership
+ *   or access of a memory readion
+ * o FFA_MSG_SEND2 and FFA_MSG_WAIT - Used for indirect messaging
+ * o FFA_MSG_YIELD
+ * o FFA_INTERRUPT - Used to report preemption
+ * o FFA_RUN
+ *
+ * Limitations in the implemented FF-A interfaces:
+ * o FFA_RXTX_MAP_*:
+ *   - Maps at most 32 4k pages large RX and TX buffers
+ *   - RT/TX buffers must be normal RAM
+ *   - Doesn't support forwarding this call on behalf of an endpoint
+ * o FFA_MEM_SHARE_*: only supports sharing
+ *   - from a VM to an SP
+ *   - with one borrower
+ *   - with the memory transaction descriptor in the RX/TX buffer
+ *   - normal memory
+ *   - at most 512 kB large memory regions
+ *   - at most 32 shared memory regions per guest
+ * o FFA_MSG_SEND_DIRECT_REQ:
+ *   - only supported from a VM to an SP
+ *
+ * There are some large locked sections with ffa_tx_buffer_lock and
+ * ffa_rx_buffer_lock. Especially the ffa_tx_buffer_lock spinlock used
+ * around share_shm() is a very large locked section which can let one VM
+ * affect another VM.
  */
 
 #include <xen/bitops.h>
-- 
2.34.1

[XEN PATCH v8 22/22] docs: add Arm FF-A mediator

[Jens Wiklander]

Describes a FF-A version 1.1 [1] mediator to communicate with a Secure
Partition in secure world.

[1] https://developer.arm.com/documentation/den0077/latest
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
 SUPPORT.md               |  8 ++++++++
 docs/man/xl.cfg.5.pod.in | 15 +++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/SUPPORT.md b/SUPPORT.md
index aa1940e55f09..1fd746f7f7f2 100644
--- a/SUPPORT.md
+++ b/SUPPORT.md
@@ -818,6 +818,14 @@ that covers the DMA of the device to be passed through.
 
 No support for QEMU backends in a 16K or 64K domain.
 
+### ARM: Firmware Framework for Arm A-profile (FF-A) Mediator
+
+    Status, Arm64: Tech Preview
+
+There are still some code paths where a vCPU may hog a pCPU longer than
+necessary. The FF-A mediator is not yet implemented for Arm32. Part of the
+FF-A specification is not supported.
+
 ### ARM: Guest Device Tree support
 
     Status: Supported
diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
index 10f37990be57..bba99c576b48 100644
--- a/docs/man/xl.cfg.5.pod.in
+++ b/docs/man/xl.cfg.5.pod.in
@@ -1645,6 +1645,21 @@ in OP-TEE.
 
 This feature is a B<technology preview>.
 
+=item B<ffa>
+
+B<Arm only.> Allow a guest to communicate via FF-A with Secure Partitions
+(SP), default false.
+
+Currently is only a small subset of the FF-A specification supported. Just
+enough to communicate with OP-TEE. In general only direct messaging and
+sharing memory with one SP. More advanced use cases where memory might be
+shared or donated to multple SPs are not supported.
+
+See L<https://developer.arm.com/documentation/den0077/latest> for more
+informantion about FF-A.
+
+This feature is a B<technology preview>.
+
 =back
 
 =back
-- 
2.34.1

RE: [XEN PATCH v8 04/22] xen/arm: ffa: add remaining SMC function IDs

[Henry Wang]

Hi Jens,

> -----Original Message-----
> Subject: [XEN PATCH v8 04/22] xen/arm: ffa: add remaining SMC function IDs
> 
> Adds the remaining SMC function IDs from FF-A 1.1 specification.

Nit: I would suggest that in commit message you can mention the documentation
number you used. During my review of this patch I am using
DEN0077A version 1.1 REL0.

> 
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

I also confirm that the macro values introduced by this patch is consistent with
the spec in commit message, hence:

Reviewed-by: Henry Wang <Henry.Wang@arm.com>

Kind regards,
Henry

RE: [XEN PATCH v8 05/22] xen/arm: ffa: add flags for FFA_PARTITION_INFO_GET

[Henry Wang]

Hi Jens,

> -----Original Message-----
> Subject: [XEN PATCH v8 05/22] xen/arm: ffa: add flags for
> FFA_PARTITION_INFO_GET
> 
> Defines flags used for the function FFA_PARTITION_INFO_GET.

Nit: Similarly as my comment for patch #4, I would suggest that in
commit message you can mention the documentation number and
the chapter of FFA_PARTITION_INFO_GET. Something like:
"According to DEN0077A version 1.1 REL0, section 13.8, defines
flags used for the function FFA_PARTITION_INFO_GET"

> 
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> ---
>  xen/arch/arm/tee/ffa.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
> index ba0942e76993..72e7d0575de5 100644
> --- a/xen/arch/arm/tee/ffa.c
> +++ b/xen/arch/arm/tee/ffa.c
> @@ -57,6 +57,40 @@
>  #define FFA_MY_VERSION
> MAKE_FFA_VERSION(FFA_MY_VERSION_MAJOR, \
>                                                   FFA_MY_VERSION_MINOR)
> 
> +/*
> + * Flags to determine partition properties in FFA_PARTITION_INFO_GET
> return
> + * message:
> + * BIT(0): Supports receipt of direct requests
> + * BIT(1): Can send direct requests
> + * BIT(2): Can send and receive indirect messages
> + * BIT(3): Supports receipt of notifications
> + * BIT(4-5): Partition ID is a PE endpoint ID
> + * BIT(6): Partition must be informed about each VM that is created by
> + *         the Hypervisor
> + * BIT(7): Partition must be informed about each VM that is destroyed by
> + *         the Hypervisor
> + * BIT(8): Partition runs in the AArch64 execution state else AArch32
> + *         execution state
> + */
> +#define FFA_PART_PROP_DIRECT_REQ_RECV   BIT(0, U)
> +#define FFA_PART_PROP_DIRECT_REQ_SEND   BIT(1, U)
> +#define FFA_PART_PROP_INDIRECT_MSGS     BIT(2, U)
> +#define FFA_PART_PROP_RECV_NOTIF        BIT(3, U)
> +#define FFA_PART_PROP_IS_MASK           (3U << 4)

I am a bit confused here, here (3U<<4) is "IS_MASK" but...

> +#define FFA_PART_PROP_IS_PE_ID          (0U << 4)
> +#define FFA_PART_PROP_IS_SEPID_INDEP    (1U << 4)
> +#define FFA_PART_PROP_IS_SEPID_DEP      (2U << 4)
> +#define FFA_PART_PROP_IS_AUX_ID         (3U << 4)

...here the same value is used for "IS_AUX_ID". According to
the spec that I referred to, bit[5:4] has the following encoding:
b'11: Partition ID is an auxiliary ID. Hence I guess the above
"IS_MASK" should be removed?

I confirm the values of other fields are consistent with the spec.

Kind regards,
Henry

RE: [XEN PATCH v8 06/22] xen/arm: ffa: add defines for framework direct request/response messages

[Henry Wang]

Hi Jens,

> -----Original Message-----
> Subject: [XEN PATCH v8 06/22] xen/arm: ffa: add defines for framework direct
> request/response messages
> 
> Adds defines for framework direct request/response messages.

Same here, it actually took me a while to find the related chapters
about this patch, so I would suggest that we can add more details
about where the values are from.

From the spec that I referred to (DEN0077A version 1.1 REL0), they are
in section 18.3, Table 18.{21, 25, 26, 27,28}.

> 
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

I confirm the values introduced by this patch are consistent with the
above spec, hence:

Reviewed-by: Henry Wang <Henry.Wang@arm.com>

Kind regards,
Henry

RE: [XEN PATCH v8 07/22] xen/arm: ffa: enforce dependency on 4k pages

[Henry Wang]

Hi Jens,

> -----Original Message-----
> Subject: [XEN PATCH v8 07/22] xen/arm: ffa: enforce dependency on 4k pages
> 
> Adds a BUILD_BUG_ON() to assert the dependency on 4k pages in the FF-A
> mediator since the current implementation only works if Xen page size is
> the same as the FFA page size.
> 
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Reviewed-by: Henry Wang <Henry.Wang@arm.com>

Kind regards,
Henry

RE: [XEN PATCH v8 08/22] xen/arm: ffa: add support for FFA_ID_GET

[Henry Wang]

Hi Jens,

> -----Original Message-----
> Subject: [XEN PATCH v8 08/22] xen/arm: ffa: add support for FFA_ID_GET
> 
> Adds support for the FF-A function FFA_ID_GET to return the ID of the
> calling client.
> 
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> ---
>  xen/arch/arm/tee/ffa.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
> index 90ed71cbfda3..f129879c5b81 100644
> --- a/xen/arch/arm/tee/ffa.c
> +++ b/xen/arch/arm/tee/ffa.c
> @@ -181,6 +181,12 @@ static bool ffa_get_version(uint32_t *vers)
>      return true;
>  }
> 
> +static uint16_t get_vm_id(const struct domain *d)
> +{
> +    /* +1 since 0 is reserved for the hypervisor in FF-A */
> +    return d->domain_id + 1;

Since here you want 0 to be reserved, I think maybe you can use
the "d->arch.p2m.vmid"? According to the logic in p2m_alloc_vmid(),
the "d->arch.p2m.vmid" is also a per-domain u16 value that starts
from 1.

Kind regards,
Henry

RE: [XEN PATCH v8 09/22] xen/arm: ffa: add direct request support

[Henry Wang]

Hi Jens,

> -----Original Message-----
> Subject: [XEN PATCH v8 09/22] xen/arm: ffa: add direct request support
> 
> Adds support for sending a FF-A direct request. Checks that the SP also
> supports handling a 32-bit direct request. 64-bit direct requests are
> not used by the mediator itself so there is not need to check for that.
> 
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> ---
>  xen/arch/arm/tee/ffa.c | 112
> +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 112 insertions(+)
> 
> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
> index f129879c5b81..f2cce955d981 100644
> --- a/xen/arch/arm/tee/ffa.c
> +++ b/xen/arch/arm/tee/ffa.c
> @@ -181,6 +181,56 @@ static bool ffa_get_version(uint32_t *vers)
>      return true;
>  }
> 
> +static int32_t get_ffa_ret_code(const struct arm_smccc_1_2_regs *resp)
> +{
> +    switch ( resp->a0 )
> +    {
> +    case FFA_ERROR:
> +        if ( resp->a2 )
> +            return resp->a2;
> +        else
> +            return FFA_RET_NOT_SUPPORTED;
> +    case FFA_SUCCESS_32:
> +    case FFA_SUCCESS_64:
> +        return FFA_RET_OK;
> +    default:
> +        return FFA_RET_NOT_SUPPORTED;
> +    }
> +}
> +
> +static int32_t ffa_simple_call(uint32_t fid, register_t a1, register_t a2,
> +                               register_t a3, register_t a4)
> +{
> +    const struct arm_smccc_1_2_regs arg = {
> +        .a0 = fid,
> +        .a1 = a1,
> +        .a2 = a2,
> +        .a3 = a3,
> +        .a4 = a4,
> +    };
> +    struct arm_smccc_1_2_regs resp;
> +
> +    arm_smccc_1_2_smc(&arg, &resp);
> +
> +    return get_ffa_ret_code(&resp);
> +}
> +
> +static int32_t ffa_features(uint32_t id)
> +{
> +    return ffa_simple_call(FFA_FEATURES, id, 0, 0, 0);
> +}
> +
> +static bool check_mandatory_feature(uint32_t id)
> +{
> +    int32_t ret = ffa_features(id);
> +
> +    if (ret)

Coding style nit: You need spaces before and after "ret", i.e.
if ( ret )

With this fixed:

Reviewed-by: Henry Wang <Henry.Wang@arm.com>

Kind regards,
Henry

RE: [XEN PATCH v8 22/22] docs: add Arm FF-A mediator

[Henry Wang]

Hi Jens,

> -----Original Message-----
> Subject: [XEN PATCH v8 22/22] docs: add Arm FF-A mediator
> 
> Describes a FF-A version 1.1 [1] mediator to communicate with a Secure
> Partition in secure world.
> 
> [1] https://developer.arm.com/documentation/den0077/latest
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> ---
>  SUPPORT.md               |  8 ++++++++
>  docs/man/xl.cfg.5.pod.in | 15 +++++++++++++++
>  2 files changed, 23 insertions(+)
>
> +B<Arm only.> Allow a guest to communicate via FF-A with Secure Partitions
> +(SP), default false.
> +
> +Currently is only a small subset of the FF-A specification supported. Just

I am not a native English speaker but I think this sentence would better be:
"Currently only a small subset of the FF-A specification is supported."

Other parts look good to me, so:

Reviewed-by: Henry Wang <Henry.Wang@arm.com>

Kind regards,
Henry

RE: [XEN PATCH v8 21/22] xen/arm: ffa: list current limitations

[Henry Wang]

Hi Jens,

> -----Original Message-----
> Subject: [XEN PATCH v8 21/22] xen/arm: ffa: list current limitations
> 
> Adds a comments with a list of unsupported FF-A interfaces and

Typo: s/a comments/comments/

> limitations in the implemented FF-A interfaces.
> 
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> ---
>  xen/arch/arm/tee/ffa.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
> index 0948cc636871..6424c222c885 100644
> --- a/xen/arch/arm/tee/ffa.c
> +++ b/xen/arch/arm/tee/ffa.c
> @@ -13,6 +13,38 @@
>   *                https://developer.arm.com/documentation/den0077/e
>   * TEEC-1.0C: TEE Client API Specification version 1.0c available at
>   *            https://globalplatform.org/specs-library/tee-client-api-specification/
> + *
> + * Notes on the the current implementstion.

Typo: s/implementstion/implementation/

> + *
> + * Unsupported FF-A interfaces:
> + * o FFA_MSG_POLL and FFA_MSG_SEND - deprecated in FF-A-1.1-REL0
> + * o FFA_MEM_RETRIEVE_* - Used when sharing memory from an SP to a
> VM
> + * o FFA_MEM_DONATE_* and FFA_MEM_LEND_* - Used when tranferring
> ownership
> + *   or access of a memory readion

Typo "readion"? Maybe I am wrong but I cannot find this word in the spec.

With above typos corrected:

Reviewed-by: Henry Wang <Henry.Wang@arm.com>

Kind regards,
Henry

RE: [XEN PATCH v8 14/22] xen/arm: move regpair_to_uint64() and uint64_to_regpair() to regs.h

[Henry Wang]

Hi Jens,

> -----Original Message-----
> Subject: [XEN PATCH v8 14/22] xen/arm: move regpair_to_uint64() and
> uint64_to_regpair() to regs.h
> 
> Moves the two helper functions regpair_to_uint64() and
> uint64_to_regpair() from xen/arch/arm/tee/optee.c to the common arm
> specific regs.h. This enables reuse of these functions in the FF-A
> mediator in a subsequent patch.
> 
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Reviewed-by: Henry Wang <Henry.Wang@arm.com>

Kind regards,
Henry

RE: [XEN PATCH v8 15/22] xen/arm: ffa: add defines for sharing memory

[Henry Wang]

Hi Jens,

> -----Original Message-----
> Subject: [XEN PATCH v8 15/22] xen/arm: ffa: add defines for sharing memory
> 
> Adds defines needed for sharing using the function FFA_MEM_SHARE and
> friends.

Same as my comments in previous patches, I would suggest to also mention
the references in commit message.

> 
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> ---
>  xen/arch/arm/tee/ffa.c | 60
> ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 60 insertions(+)
> 
> + * FF-A doesn't have any direct requirments on GlobalPlatform or vice

Typo: s/requirements/requirements/

> + * versa, but an implementation can very well use FF-A in order to provide
> + * a GlobalPlatform interface on top.
> + *
> + * Global Platform specification for TEE requires that any TEE
> + * implementation should allow to share buffers with size of at least
> + * 512KB, defined in TEEC-1.0C page 24, Table 4-1,
> + * TEEC_CONFIG_SHAREDMEM_MAX_SIZE.
> + * Due to align issue mentioned above, we need to increase this

s/align issue/alignment issue/ ?

> + * value with one.
> + */
> +#define FFA_MAX_SHM_PAGE_COUNT          (SZ_512K / FFA_PAGE_SIZE + 1)
> +
> +/*
> + * Limits the number of shared buffers that guest can have at once. This
> + * is to prevent case, when guests tricks XEN into exhausting its own

Typo: s/tricks/trick/

> + * memory by allocating many small buffers. This value has been chosen
> + * arbitrary.

Typo: s/ arbitrary/arbitrarily/

> + */
> +#define FFA_MAX_SHM_COUNT               32
> +
> +/* FF-A-1.1-REL0 section 10.9.2 Memory region handle, page 167 */
> +#define FFA_HANDLE_HYP_FLAG             BIT(63, ULL)
> +#define FFA_HANDLE_INVALID              0xffffffffffffffffULL
> +
> +/*
> + * Memory attributes: Normal memory, Write-Back cacheable, Inner
> shareable
> + * Defined in FF-A-1.1-REL0 Table 10.18 at page 175.
> + */
> +#define FFA_NORMAL_MEM_REG_ATTR         0x2fU
> +/*
> + * Memory access permissions: Read-write
> + * Defined in FF-A-1.1-REL0 Table 10.15 at page 168.
> + */
> +#define FFA_MEM_ACC_RW                  0x2U
> +
> +/* FF-A-1.1-REL0 section 10.11.4 Flags usage, page 184-187 */
> +/* Clear memory before mapping in receiver */
> +#define FFA_MEMORY_REGION_FLAG_CLEAR            BIT(0, U)
> +/* Relayer may time slice this operation */
> +#define FFA_MEMORY_REGION_FLAG_TIME_SLICE       BIT(1, U)
> +/* Clear memory after receiver relinquishes it */
> +#define FFA_MEMORY_REGION_FLAG_CLEAR_RELINQUISH BIT(2, U)
> +/* Share memory transaction */
> +#define FFA_MEMORY_REGION_TRANSACTION_TYPE_SHARE (1U << 3)
> +

I confirm the values introduced in this patch are consistent with in-code
comments on top of them. Thanks for the pointer :)

With the typos corrected:
Reviewed-by: Henry Wang <Henry.Wang@arm.com>

Kind regards,
Henry


>  /*
>   * Flags and field values used for the MSG_SEND_DIRECT_REQ/RESP:
>   * BIT(31): Framework or partition message
> --
> 2.34.1
> 

RE: [XEN PATCH v8 16/22] xen/arm: ffa: add ABI structs for sharing memory

[Henry Wang]

Hi Jens,

> -----Original Message-----
> Subject: [XEN PATCH v8 16/22] xen/arm: ffa: add ABI structs for sharing
> memory
> 
> Adds the ABI structs used by function FFA_MEM_SHARE and friends for
> sharing memory.
> 
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Reviewed-by: Henry Wang <Henry.Wang@arm.com>

Kind regards,
Henry

RE: [XEN PATCH v8 03/22] tools: add Arm FF-A mediator

[Henry Wang]

Hi Jens,

> -----Original Message-----
> Subject: [XEN PATCH v8 03/22] tools: add Arm FF-A mediator
> 
> Adds a new "ffa" value to the Enumeration "tee_type" to indicate if a
> guest is trusted to use FF-A.
> 
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Reviewed-by: Henry Wang <Henry.Wang@arm.com>

Kind regards,
Henry

Re: [XEN PATCH v8 02/22] xen/arm: tee: add a primitive FF-A mediator

[Julien Grall]

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

Re: [XEN PATCH v8 08/22] xen/arm: ffa: add support for FFA_ID_GET

[Julien Grall]

On 13/04/2023 12:07, Henry Wang wrote:
> Hi Jens,
> 
>> -----Original Message-----
>> Subject: [XEN PATCH v8 08/22] xen/arm: ffa: add support for FFA_ID_GET
>>
>> Adds support for the FF-A function FFA_ID_GET to return the ID of the
>> calling client.
>>
>> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
>> ---
>>   xen/arch/arm/tee/ffa.c | 21 +++++++++++++++++++++
>>   1 file changed, 21 insertions(+)
>>
>> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
>> index 90ed71cbfda3..f129879c5b81 100644
>> --- a/xen/arch/arm/tee/ffa.c
>> +++ b/xen/arch/arm/tee/ffa.c
>> @@ -181,6 +181,12 @@ static bool ffa_get_version(uint32_t *vers)
>>       return true;
>>   }
>>
>> +static uint16_t get_vm_id(const struct domain *d)
>> +{
>> +    /* +1 since 0 is reserved for the hypervisor in FF-A */
>> +    return d->domain_id + 1;
> 
> Since here you want 0 to be reserved, I think maybe you can use
> the "d->arch.p2m.vmid"? According to the logic in p2m_alloc_vmid(),
> the "d->arch.p2m.vmid" is also a per-domain u16 value that starts
> from 1.

I would rather not do that for a few reasons:
  1) This is assuming the P2M code is initialized first
  2) We should not rely on the VMID to be fixed. We may need to change 
that if we want to run more VMs than the number of VMIDs (we may even 
need multiple VMIDs per domain...).

Cheers,

-- 
Julien Grall

RE: [XEN PATCH v8 08/22] xen/arm: ffa: add support for FFA_ID_GET

[Henry Wang]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Subject: Re: [XEN PATCH v8 08/22] xen/arm: ffa: add support for FFA_ID_GET
> >> +static uint16_t get_vm_id(const struct domain *d)
> >> +{
> >> +    /* +1 since 0 is reserved for the hypervisor in FF-A */
> >> +    return d->domain_id + 1;
> >
> > Since here you want 0 to be reserved, I think maybe you can use
> > the "d->arch.p2m.vmid"? According to the logic in p2m_alloc_vmid(),
> > the "d->arch.p2m.vmid" is also a per-domain u16 value that starts
> > from 1.
> 
> I would rather not do that for a few reasons:
>   1) This is assuming the P2M code is initialized first
>   2) We should not rely on the VMID to be fixed. We may need to change
> that if we want to run more VMs than the number of VMIDs (we may even
> need multiple VMIDs per domain...).

Yeah these arguments are reasonable. Forget about my comments then.

Kind regards,
Henry

> 
> Cheers,
> 
> --
> Julien Grall

Re: [XEN PATCH v8 09/22] xen/arm: ffa: add direct request support

[Julien Grall]

Hi,

On 13/04/2023 08:14, Jens Wiklander wrote:
> Adds support for sending a FF-A direct request. Checks that the SP also
> supports handling a 32-bit direct request. 64-bit direct requests are
> not used by the mediator itself so there is not need to check for that.
> 
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> ---
>   xen/arch/arm/tee/ffa.c | 112 +++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 112 insertions(+)
> 
> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
> index f129879c5b81..f2cce955d981 100644
> --- a/xen/arch/arm/tee/ffa.c
> +++ b/xen/arch/arm/tee/ffa.c
> @@ -181,6 +181,56 @@ static bool ffa_get_version(uint32_t *vers)
>       return true;
>   }
>   
> +static int32_t get_ffa_ret_code(const struct arm_smccc_1_2_regs *resp)
> +{
> +    switch ( resp->a0 )
> +    {
> +    case FFA_ERROR:
> +        if ( resp->a2 )
> +            return resp->a2;
> +        else
> +            return FFA_RET_NOT_SUPPORTED;
> +    case FFA_SUCCESS_32:
> +    case FFA_SUCCESS_64:
> +        return FFA_RET_OK;
> +    default:
> +        return FFA_RET_NOT_SUPPORTED;
> +    }
> +}
> +
> +static int32_t ffa_simple_call(uint32_t fid, register_t a1, register_t a2,
> +                               register_t a3, register_t a4)
> +{
> +    const struct arm_smccc_1_2_regs arg = {
> +        .a0 = fid,
> +        .a1 = a1,
> +        .a2 = a2,
> +        .a3 = a3,
> +        .a4 = a4,
> +    };
> +    struct arm_smccc_1_2_regs resp;
> +
> +    arm_smccc_1_2_smc(&arg, &resp);
> +
> +    return get_ffa_ret_code(&resp);
> +}
> +
> +static int32_t ffa_features(uint32_t id)
> +{
> +    return ffa_simple_call(FFA_FEATURES, id, 0, 0, 0);
> +}
> +
> +static bool check_mandatory_feature(uint32_t id)
> +{
> +    int32_t ret = ffa_features(id);
> +
> +    if (ret)
> +        printk(XENLOG_ERR "ffa: mandatory feature id %#x missing: error %d\n",
> +               id, ret);
> +
> +    return !ret;
> +}
> +
>   static uint16_t get_vm_id(const struct domain *d)
>   {
>       /* +1 since 0 is reserved for the hypervisor in FF-A */
> @@ -222,6 +272,57 @@ static void handle_version(struct cpu_user_regs *regs)
>       set_regs(regs, vers, 0, 0, 0, 0, 0, 0, 0);
>   }
>   
> +static void handle_msg_send_direct_req(struct cpu_user_regs *regs, uint32_t fid)
> +{
> +    struct arm_smccc_1_2_regs arg = { .a0 = fid, };
> +    struct arm_smccc_1_2_regs resp = { };
> +    struct domain *d = current->domain;
> +    uint32_t src_dst;
> +    uint64_t mask;
> +
> +    if ( smccc_is_conv_64(fid) )
> +        mask = GENMASK_ULL(63, 0);
> +    else
> +        mask = GENMASK_ULL(31, 0);
> +
> +    src_dst = get_user_reg(regs, 1);
> +    if ( (src_dst >> 16) != get_vm_id(d) )
> +    {
> +        resp.a0 = FFA_ERROR;
> +        resp.a2 = FFA_RET_INVALID_PARAMETERS;
> +        goto out;
> +    }
> +
> +    arg.a1 = src_dst;
> +    arg.a2 = get_user_reg(regs, 2) & mask;
> +    arg.a3 = get_user_reg(regs, 3) & mask;
> +    arg.a4 = get_user_reg(regs, 4) & mask;
> +    arg.a5 = get_user_reg(regs, 5) & mask;
> +    arg.a6 = get_user_reg(regs, 6) & mask;
> +    arg.a7 = get_user_reg(regs, 7) & mask;
> +
> +    arm_smccc_1_2_smc(&arg, &resp);
> +    switch ( resp.a0 )
> +    {
> +    case FFA_ERROR:
> +    case FFA_SUCCESS_32:
> +    case FFA_SUCCESS_64:
> +    case FFA_MSG_SEND_DIRECT_RESP_32:
> +    case FFA_MSG_SEND_DIRECT_RESP_64:
> +        break;
> +    default:
> +        /* Bad fid, report back. */
> +        memset(&arg, 0, sizeof(arg));
> +        arg.a0 = FFA_ERROR;
> +        arg.a1 = src_dst;
> +        arg.a2 = FFA_RET_ABORTED;
> +    }
> +
> +out:
> +    set_regs(regs, resp.a0, resp.a1 & mask, resp.a2 & mask, resp.a3 & mask,
> +             resp.a4 & mask, resp.a5 & mask, resp.a6 & mask, resp.a7 & mask);
> +}
> +
>   static bool ffa_handle_call(struct cpu_user_regs *regs)
>   {
>       uint32_t fid = get_user_reg(regs, 0);
> @@ -239,6 +340,10 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
>       case FFA_ID_GET:
>           set_regs_success(regs, get_vm_id(d), 0);
>           return true;
> +    case FFA_MSG_SEND_DIRECT_REQ_32:
> +    case FFA_MSG_SEND_DIRECT_REQ_64:
> +        handle_msg_send_direct_req(regs, fid);
> +        return true;
>   
>       default:
>           gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid);
> @@ -326,6 +431,13 @@ static bool ffa_probe(void)
>       printk(XENLOG_INFO "ARM FF-A Firmware version %u.%u\n",
>              major_vers, minor_vers);
>   
> +    /*
> +     * TODO save result of checked features and use that information to
> +     * accept or reject requests from guests.
> +     */

I am not entirely sure I understand this TODO. Does it mean a guest can 
currently use a request that is not supported by FFA?

> +    if ( !check_mandatory_feature(FFA_MSG_SEND_DIRECT_REQ_32) )
> +        return false;
> +
>       ffa_version = vers;
>   
>       return true;

Cheers,

-- 
Julien Grall

Re: [XEN PATCH v8 09/22] xen/arm: ffa: add direct request support

[Bertrand Marquis]

Hi Julien,

> On 13 Apr 2023, at 15:15, Julien Grall <julien@xen.org> wrote:
> 
> Hi,
> 
> On 13/04/2023 08:14, Jens Wiklander wrote:
>> Adds support for sending a FF-A direct request. Checks that the SP also
>> supports handling a 32-bit direct request. 64-bit direct requests are
>> not used by the mediator itself so there is not need to check for that.
>> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
>> ---
>>  xen/arch/arm/tee/ffa.c | 112 +++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 112 insertions(+)
>> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
>> index f129879c5b81..f2cce955d981 100644
>> --- a/xen/arch/arm/tee/ffa.c
>> +++ b/xen/arch/arm/tee/ffa.c
>> @@ -181,6 +181,56 @@ static bool ffa_get_version(uint32_t *vers)
>>      return true;
>>  }
>>  +static int32_t get_ffa_ret_code(const struct arm_smccc_1_2_regs *resp)
>> +{
>> +    switch ( resp->a0 )
>> +    {
>> +    case FFA_ERROR:
>> +        if ( resp->a2 )
>> +            return resp->a2;
>> +        else
>> +            return FFA_RET_NOT_SUPPORTED;
>> +    case FFA_SUCCESS_32:
>> +    case FFA_SUCCESS_64:
>> +        return FFA_RET_OK;
>> +    default:
>> +        return FFA_RET_NOT_SUPPORTED;
>> +    }
>> +}
>> +
>> +static int32_t ffa_simple_call(uint32_t fid, register_t a1, register_t a2,
>> +                               register_t a3, register_t a4)
>> +{
>> +    const struct arm_smccc_1_2_regs arg = {
>> +        .a0 = fid,
>> +        .a1 = a1,
>> +        .a2 = a2,
>> +        .a3 = a3,
>> +        .a4 = a4,
>> +    };
>> +    struct arm_smccc_1_2_regs resp;
>> +
>> +    arm_smccc_1_2_smc(&arg, &resp);
>> +
>> +    return get_ffa_ret_code(&resp);
>> +}
>> +
>> +static int32_t ffa_features(uint32_t id)
>> +{
>> +    return ffa_simple_call(FFA_FEATURES, id, 0, 0, 0);
>> +}
>> +
>> +static bool check_mandatory_feature(uint32_t id)
>> +{
>> +    int32_t ret = ffa_features(id);
>> +
>> +    if (ret)
>> +        printk(XENLOG_ERR "ffa: mandatory feature id %#x missing: error %d\n",
>> +               id, ret);
>> +
>> +    return !ret;
>> +}
>> +
>>  static uint16_t get_vm_id(const struct domain *d)
>>  {
>>      /* +1 since 0 is reserved for the hypervisor in FF-A */
>> @@ -222,6 +272,57 @@ static void handle_version(struct cpu_user_regs *regs)
>>      set_regs(regs, vers, 0, 0, 0, 0, 0, 0, 0);
>>  }
>>  +static void handle_msg_send_direct_req(struct cpu_user_regs *regs, uint32_t fid)
>> +{
>> +    struct arm_smccc_1_2_regs arg = { .a0 = fid, };
>> +    struct arm_smccc_1_2_regs resp = { };
>> +    struct domain *d = current->domain;
>> +    uint32_t src_dst;
>> +    uint64_t mask;
>> +
>> +    if ( smccc_is_conv_64(fid) )
>> +        mask = GENMASK_ULL(63, 0);
>> +    else
>> +        mask = GENMASK_ULL(31, 0);
>> +
>> +    src_dst = get_user_reg(regs, 1);
>> +    if ( (src_dst >> 16) != get_vm_id(d) )
>> +    {
>> +        resp.a0 = FFA_ERROR;
>> +        resp.a2 = FFA_RET_INVALID_PARAMETERS;
>> +        goto out;
>> +    }
>> +
>> +    arg.a1 = src_dst;
>> +    arg.a2 = get_user_reg(regs, 2) & mask;
>> +    arg.a3 = get_user_reg(regs, 3) & mask;
>> +    arg.a4 = get_user_reg(regs, 4) & mask;
>> +    arg.a5 = get_user_reg(regs, 5) & mask;
>> +    arg.a6 = get_user_reg(regs, 6) & mask;
>> +    arg.a7 = get_user_reg(regs, 7) & mask;
>> +
>> +    arm_smccc_1_2_smc(&arg, &resp);
>> +    switch ( resp.a0 )
>> +    {
>> +    case FFA_ERROR:
>> +    case FFA_SUCCESS_32:
>> +    case FFA_SUCCESS_64:
>> +    case FFA_MSG_SEND_DIRECT_RESP_32:
>> +    case FFA_MSG_SEND_DIRECT_RESP_64:
>> +        break;
>> +    default:
>> +        /* Bad fid, report back. */
>> +        memset(&arg, 0, sizeof(arg));
>> +        arg.a0 = FFA_ERROR;
>> +        arg.a1 = src_dst;
>> +        arg.a2 = FFA_RET_ABORTED;
>> +    }
>> +
>> +out:
>> +    set_regs(regs, resp.a0, resp.a1 & mask, resp.a2 & mask, resp.a3 & mask,
>> +             resp.a4 & mask, resp.a5 & mask, resp.a6 & mask, resp.a7 & mask);
>> +}
>> +
>>  static bool ffa_handle_call(struct cpu_user_regs *regs)
>>  {
>>      uint32_t fid = get_user_reg(regs, 0);
>> @@ -239,6 +340,10 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
>>      case FFA_ID_GET:
>>          set_regs_success(regs, get_vm_id(d), 0);
>>          return true;
>> +    case FFA_MSG_SEND_DIRECT_REQ_32:
>> +    case FFA_MSG_SEND_DIRECT_REQ_64:
>> +        handle_msg_send_direct_req(regs, fid);
>> +        return true;
>>        default:
>>          gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid);
>> @@ -326,6 +431,13 @@ static bool ffa_probe(void)
>>      printk(XENLOG_INFO "ARM FF-A Firmware version %u.%u\n",
>>             major_vers, minor_vers);
>>  +    /*
>> +     * TODO save result of checked features and use that information to
>> +     * accept or reject requests from guests.
>> +     */
> 
> I am not entirely sure I understand this TODO. Does it mean a guest can currently use a request that is not supported by FFA?

In fact this is a bit the opposite: in the following patch we check that all features we could need are supported but if a guest is only using a subset we might not need to have all of them.
Idea of this TODO would be to save the features supported and refuse guest requests depending on the features needed for them.

Cheers
Bertrand

> 
>> +    if ( !check_mandatory_feature(FFA_MSG_SEND_DIRECT_REQ_32) )
>> +        return false;
>> +
>>      ffa_version = vers;
>>        return true;
> 
> Cheers,
> 
> -- 
> Julien Grall

Re: [XEN PATCH v8 11/22] xen/arm: ffa: send guest events to Secure Partitions

[Julien Grall]

Hi,

On 13/04/2023 08:14, Jens Wiklander wrote:
> +static int32_t ffa_direct_req_send_vm(uint16_t sp_id, uint16_t vm_id,
> +                                      uint8_t msg)
> +{
> +    uint32_t exp_resp = FFA_MSG_FLAG_FRAMEWORK;
> +    int32_t res;
> +
> +    if ( msg == FFA_MSG_SEND_VM_CREATED )
> +        exp_resp |= FFA_MSG_RESP_VM_CREATED;
> +    else if ( msg == FFA_MSG_SEND_VM_DESTROYED )
> +        exp_resp |= FFA_MSG_RESP_VM_DESTROYED;
> +    else
> +        return FFA_RET_INVALID_PARAMETERS;
> +
> +    do {
> +        const struct arm_smccc_1_2_regs arg = {
> +            .a0 = FFA_MSG_SEND_DIRECT_REQ_32,
> +            .a1 = sp_id,
> +            .a2 = FFA_MSG_FLAG_FRAMEWORK | msg,
> +            .a5 = vm_id,
> +        };
> +        struct arm_smccc_1_2_regs resp;
> +
> +        arm_smccc_1_2_smc(&arg, &resp);
> +        if ( resp.a0 != FFA_MSG_SEND_DIRECT_RESP_32 || resp.a2 != exp_resp )
> +        {
> +            /*
> +             * This is an invalid response, likely due to some error in the
> +             * implementation of the ABI.
> +             */
> +            return FFA_RET_INVALID_PARAMETERS;
> +        }
> +        res = resp.a3;
> +    } while ( res == FFA_RET_INTERRUPTED || res == FFA_RET_RETRY );

This loop seems potentially unbounded to me. Can you add a comment 
explaining why this is fine?

> +
> +    return res;
> +}
> +

Cheers,

-- 
Julien Grall

Re: [XEN PATCH v8 02/22] xen/arm: tee: add a primitive FF-A mediator

[Andrew Cooper]

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 09/22] xen/arm: ffa: add direct request support

[Julien Grall]

On 13/04/2023 14:20, Bertrand Marquis wrote:
> Hi Julien,
> 
>> On 13 Apr 2023, at 15:15, Julien Grall <julien@xen.org> wrote:
>>
>> Hi,
>>
>> On 13/04/2023 08:14, Jens Wiklander wrote:
>>> Adds support for sending a FF-A direct request. Checks that the SP also
>>> supports handling a 32-bit direct request. 64-bit direct requests are
>>> not used by the mediator itself so there is not need to check for that.
>>> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
>>> ---
>>>   xen/arch/arm/tee/ffa.c | 112 +++++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 112 insertions(+)
>>> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
>>> index f129879c5b81..f2cce955d981 100644
>>> --- a/xen/arch/arm/tee/ffa.c
>>> +++ b/xen/arch/arm/tee/ffa.c
>>> @@ -181,6 +181,56 @@ static bool ffa_get_version(uint32_t *vers)
>>>       return true;
>>>   }
>>>   +static int32_t get_ffa_ret_code(const struct arm_smccc_1_2_regs *resp)
>>> +{
>>> +    switch ( resp->a0 )
>>> +    {
>>> +    case FFA_ERROR:
>>> +        if ( resp->a2 )
>>> +            return resp->a2;
>>> +        else
>>> +            return FFA_RET_NOT_SUPPORTED;
>>> +    case FFA_SUCCESS_32:
>>> +    case FFA_SUCCESS_64:
>>> +        return FFA_RET_OK;
>>> +    default:
>>> +        return FFA_RET_NOT_SUPPORTED;
>>> +    }
>>> +}
>>> +
>>> +static int32_t ffa_simple_call(uint32_t fid, register_t a1, register_t a2,
>>> +                               register_t a3, register_t a4)
>>> +{
>>> +    const struct arm_smccc_1_2_regs arg = {
>>> +        .a0 = fid,
>>> +        .a1 = a1,
>>> +        .a2 = a2,
>>> +        .a3 = a3,
>>> +        .a4 = a4,
>>> +    };
>>> +    struct arm_smccc_1_2_regs resp;
>>> +
>>> +    arm_smccc_1_2_smc(&arg, &resp);
>>> +
>>> +    return get_ffa_ret_code(&resp);
>>> +}
>>> +
>>> +static int32_t ffa_features(uint32_t id)
>>> +{
>>> +    return ffa_simple_call(FFA_FEATURES, id, 0, 0, 0);
>>> +}
>>> +
>>> +static bool check_mandatory_feature(uint32_t id)
>>> +{
>>> +    int32_t ret = ffa_features(id);
>>> +
>>> +    if (ret)
>>> +        printk(XENLOG_ERR "ffa: mandatory feature id %#x missing: error %d\n",
>>> +               id, ret);
>>> +
>>> +    return !ret;
>>> +}
>>> +
>>>   static uint16_t get_vm_id(const struct domain *d)
>>>   {
>>>       /* +1 since 0 is reserved for the hypervisor in FF-A */
>>> @@ -222,6 +272,57 @@ static void handle_version(struct cpu_user_regs *regs)
>>>       set_regs(regs, vers, 0, 0, 0, 0, 0, 0, 0);
>>>   }
>>>   +static void handle_msg_send_direct_req(struct cpu_user_regs *regs, uint32_t fid)
>>> +{
>>> +    struct arm_smccc_1_2_regs arg = { .a0 = fid, };
>>> +    struct arm_smccc_1_2_regs resp = { };
>>> +    struct domain *d = current->domain;
>>> +    uint32_t src_dst;
>>> +    uint64_t mask;
>>> +
>>> +    if ( smccc_is_conv_64(fid) )
>>> +        mask = GENMASK_ULL(63, 0);
>>> +    else
>>> +        mask = GENMASK_ULL(31, 0);
>>> +
>>> +    src_dst = get_user_reg(regs, 1);
>>> +    if ( (src_dst >> 16) != get_vm_id(d) )
>>> +    {
>>> +        resp.a0 = FFA_ERROR;
>>> +        resp.a2 = FFA_RET_INVALID_PARAMETERS;
>>> +        goto out;
>>> +    }
>>> +
>>> +    arg.a1 = src_dst;
>>> +    arg.a2 = get_user_reg(regs, 2) & mask;
>>> +    arg.a3 = get_user_reg(regs, 3) & mask;
>>> +    arg.a4 = get_user_reg(regs, 4) & mask;
>>> +    arg.a5 = get_user_reg(regs, 5) & mask;
>>> +    arg.a6 = get_user_reg(regs, 6) & mask;
>>> +    arg.a7 = get_user_reg(regs, 7) & mask;
>>> +
>>> +    arm_smccc_1_2_smc(&arg, &resp);
>>> +    switch ( resp.a0 )
>>> +    {
>>> +    case FFA_ERROR:
>>> +    case FFA_SUCCESS_32:
>>> +    case FFA_SUCCESS_64:
>>> +    case FFA_MSG_SEND_DIRECT_RESP_32:
>>> +    case FFA_MSG_SEND_DIRECT_RESP_64:
>>> +        break;
>>> +    default:
>>> +        /* Bad fid, report back. */
>>> +        memset(&arg, 0, sizeof(arg));
>>> +        arg.a0 = FFA_ERROR;
>>> +        arg.a1 = src_dst;
>>> +        arg.a2 = FFA_RET_ABORTED;
>>> +    }
>>> +
>>> +out:
>>> +    set_regs(regs, resp.a0, resp.a1 & mask, resp.a2 & mask, resp.a3 & mask,
>>> +             resp.a4 & mask, resp.a5 & mask, resp.a6 & mask, resp.a7 & mask);
>>> +}
>>> +
>>>   static bool ffa_handle_call(struct cpu_user_regs *regs)
>>>   {
>>>       uint32_t fid = get_user_reg(regs, 0);
>>> @@ -239,6 +340,10 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
>>>       case FFA_ID_GET:
>>>           set_regs_success(regs, get_vm_id(d), 0);
>>>           return true;
>>> +    case FFA_MSG_SEND_DIRECT_REQ_32:
>>> +    case FFA_MSG_SEND_DIRECT_REQ_64:
>>> +        handle_msg_send_direct_req(regs, fid);
>>> +        return true;
>>>         default:
>>>           gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid);
>>> @@ -326,6 +431,13 @@ static bool ffa_probe(void)
>>>       printk(XENLOG_INFO "ARM FF-A Firmware version %u.%u\n",
>>>              major_vers, minor_vers);
>>>   +    /*
>>> +     * TODO save result of checked features and use that information to
>>> +     * accept or reject requests from guests.
>>> +     */
>>
>> I am not entirely sure I understand this TODO. Does it mean a guest can currently use a request that is not supported by FFA?
> 
> In fact this is a bit the opposite: in the following patch we check that all features we could need are supported but if a guest is only using a subset we might not need to have all of them.
> Idea of this TODO would be to save the features supported and refuse guest requests depending on the features needed for them.

Thanks. I would suggest the following comment:

/*
  * At the moment domains must supports the same features used by Xen.
  * TODO: Rework the code to allow domain to use a subset of the features
  * supported.
  */

Note that I am using "domains" rather than "guests" because the latter 
doesn't include dom0.

Cheers,

-- 
Julien Grall

Re: [XEN PATCH v8 04/22] xen/arm: ffa: add remaining SMC function IDs

[Jens Wiklander]

Hi Henry,

On Thu, Apr 13, 2023 at 12:18 PM Henry Wang <Henry.Wang@arm.com> wrote:
>
> Hi Jens,
>
> > -----Original Message-----
> > Subject: [XEN PATCH v8 04/22] xen/arm: ffa: add remaining SMC function IDs
> >
> > Adds the remaining SMC function IDs from FF-A 1.1 specification.
>
> Nit: I would suggest that in commit message you can mention the documentation
> number you used. During my review of this patch I am using
> DEN0077A version 1.1 REL0.

OK, I'll add that.

>
> >
> > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
>
> I also confirm that the macro values introduced by this patch is consistent with
> the spec in commit message, hence:
>
> Reviewed-by: Henry Wang <Henry.Wang@arm.com>

Thanks,
Jens

>
> Kind regards,
> Henry

Re: [XEN PATCH v8 09/22] xen/arm: ffa: add direct request support

[Bertrand Marquis]

Hi,

> On 13 Apr 2023, at 15:27, Julien Grall <julien@xen.org> wrote:
> 
> 
> 
> On 13/04/2023 14:20, Bertrand Marquis wrote:
>> Hi Julien,
>>> On 13 Apr 2023, at 15:15, Julien Grall <julien@xen.org> wrote:
>>> 
>>> Hi,
>>> 
>>> On 13/04/2023 08:14, Jens Wiklander wrote:
>>>> Adds support for sending a FF-A direct request. Checks that the SP also
>>>> supports handling a 32-bit direct request. 64-bit direct requests are
>>>> not used by the mediator itself so there is not need to check for that.
>>>> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
>>>> ---
>>>>  xen/arch/arm/tee/ffa.c | 112 +++++++++++++++++++++++++++++++++++++++++
>>>>  1 file changed, 112 insertions(+)
>>>> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
>>>> index f129879c5b81..f2cce955d981 100644
>>>> --- a/xen/arch/arm/tee/ffa.c
>>>> +++ b/xen/arch/arm/tee/ffa.c
>>>> @@ -181,6 +181,56 @@ static bool ffa_get_version(uint32_t *vers)
>>>>      return true;
>>>>  }
>>>>  +static int32_t get_ffa_ret_code(const struct arm_smccc_1_2_regs *resp)
>>>> +{
>>>> +    switch ( resp->a0 )
>>>> +    {
>>>> +    case FFA_ERROR:
>>>> +        if ( resp->a2 )
>>>> +            return resp->a2;
>>>> +        else
>>>> +            return FFA_RET_NOT_SUPPORTED;
>>>> +    case FFA_SUCCESS_32:
>>>> +    case FFA_SUCCESS_64:
>>>> +        return FFA_RET_OK;
>>>> +    default:
>>>> +        return FFA_RET_NOT_SUPPORTED;
>>>> +    }
>>>> +}
>>>> +
>>>> +static int32_t ffa_simple_call(uint32_t fid, register_t a1, register_t a2,
>>>> +                               register_t a3, register_t a4)
>>>> +{
>>>> +    const struct arm_smccc_1_2_regs arg = {
>>>> +        .a0 = fid,
>>>> +        .a1 = a1,
>>>> +        .a2 = a2,
>>>> +        .a3 = a3,
>>>> +        .a4 = a4,
>>>> +    };
>>>> +    struct arm_smccc_1_2_regs resp;
>>>> +
>>>> +    arm_smccc_1_2_smc(&arg, &resp);
>>>> +
>>>> +    return get_ffa_ret_code(&resp);
>>>> +}
>>>> +
>>>> +static int32_t ffa_features(uint32_t id)
>>>> +{
>>>> +    return ffa_simple_call(FFA_FEATURES, id, 0, 0, 0);
>>>> +}
>>>> +
>>>> +static bool check_mandatory_feature(uint32_t id)
>>>> +{
>>>> +    int32_t ret = ffa_features(id);
>>>> +
>>>> +    if (ret)
>>>> +        printk(XENLOG_ERR "ffa: mandatory feature id %#x missing: error %d\n",
>>>> +               id, ret);
>>>> +
>>>> +    return !ret;
>>>> +}
>>>> +
>>>>  static uint16_t get_vm_id(const struct domain *d)
>>>>  {
>>>>      /* +1 since 0 is reserved for the hypervisor in FF-A */
>>>> @@ -222,6 +272,57 @@ static void handle_version(struct cpu_user_regs *regs)
>>>>      set_regs(regs, vers, 0, 0, 0, 0, 0, 0, 0);
>>>>  }
>>>>  +static void handle_msg_send_direct_req(struct cpu_user_regs *regs, uint32_t fid)
>>>> +{
>>>> +    struct arm_smccc_1_2_regs arg = { .a0 = fid, };
>>>> +    struct arm_smccc_1_2_regs resp = { };
>>>> +    struct domain *d = current->domain;
>>>> +    uint32_t src_dst;
>>>> +    uint64_t mask;
>>>> +
>>>> +    if ( smccc_is_conv_64(fid) )
>>>> +        mask = GENMASK_ULL(63, 0);
>>>> +    else
>>>> +        mask = GENMASK_ULL(31, 0);
>>>> +
>>>> +    src_dst = get_user_reg(regs, 1);
>>>> +    if ( (src_dst >> 16) != get_vm_id(d) )
>>>> +    {
>>>> +        resp.a0 = FFA_ERROR;
>>>> +        resp.a2 = FFA_RET_INVALID_PARAMETERS;
>>>> +        goto out;
>>>> +    }
>>>> +
>>>> +    arg.a1 = src_dst;
>>>> +    arg.a2 = get_user_reg(regs, 2) & mask;
>>>> +    arg.a3 = get_user_reg(regs, 3) & mask;
>>>> +    arg.a4 = get_user_reg(regs, 4) & mask;
>>>> +    arg.a5 = get_user_reg(regs, 5) & mask;
>>>> +    arg.a6 = get_user_reg(regs, 6) & mask;
>>>> +    arg.a7 = get_user_reg(regs, 7) & mask;
>>>> +
>>>> +    arm_smccc_1_2_smc(&arg, &resp);
>>>> +    switch ( resp.a0 )
>>>> +    {
>>>> +    case FFA_ERROR:
>>>> +    case FFA_SUCCESS_32:
>>>> +    case FFA_SUCCESS_64:
>>>> +    case FFA_MSG_SEND_DIRECT_RESP_32:
>>>> +    case FFA_MSG_SEND_DIRECT_RESP_64:
>>>> +        break;
>>>> +    default:
>>>> +        /* Bad fid, report back. */
>>>> +        memset(&arg, 0, sizeof(arg));
>>>> +        arg.a0 = FFA_ERROR;
>>>> +        arg.a1 = src_dst;
>>>> +        arg.a2 = FFA_RET_ABORTED;
>>>> +    }
>>>> +
>>>> +out:
>>>> +    set_regs(regs, resp.a0, resp.a1 & mask, resp.a2 & mask, resp.a3 & mask,
>>>> +             resp.a4 & mask, resp.a5 & mask, resp.a6 & mask, resp.a7 & mask);
>>>> +}
>>>> +
>>>>  static bool ffa_handle_call(struct cpu_user_regs *regs)
>>>>  {
>>>>      uint32_t fid = get_user_reg(regs, 0);
>>>> @@ -239,6 +340,10 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
>>>>      case FFA_ID_GET:
>>>>          set_regs_success(regs, get_vm_id(d), 0);
>>>>          return true;
>>>> +    case FFA_MSG_SEND_DIRECT_REQ_32:
>>>> +    case FFA_MSG_SEND_DIRECT_REQ_64:
>>>> +        handle_msg_send_direct_req(regs, fid);
>>>> +        return true;
>>>>        default:
>>>>          gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid);
>>>> @@ -326,6 +431,13 @@ static bool ffa_probe(void)
>>>>      printk(XENLOG_INFO "ARM FF-A Firmware version %u.%u\n",
>>>>             major_vers, minor_vers);
>>>>  +    /*
>>>> +     * TODO save result of checked features and use that information to
>>>> +     * accept or reject requests from guests.
>>>> +     */
>>> 
>>> I am not entirely sure I understand this TODO. Does it mean a guest can currently use a request that is not supported by FFA?
>> In fact this is a bit the opposite: in the following patch we check that all features we could need are supported but if a guest is only using a subset we might not need to have all of them.
>> Idea of this TODO would be to save the features supported and refuse guest requests depending on the features needed for them.
> 
> Thanks. I would suggest the following comment:
> 
> /*
> * At the moment domains must supports the same features used by Xen.
> * TODO: Rework the code to allow domain to use a subset of the features
> * supported.
> */
> 
> Note that I am using "domains" rather than "guests" because the latter doesn't include dom0.

Makes sense and new comment is nice.

Up to Jens to say if he is ok with it.

Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall

Re: [XEN PATCH v8 05/22] xen/arm: ffa: add flags for FFA_PARTITION_INFO_GET

[Jens Wiklander]

On Thu, Apr 13, 2023 at 12:28 PM Henry Wang <Henry.Wang@arm.com> wrote:
>
> Hi Jens,
>
> > -----Original Message-----
> > Subject: [XEN PATCH v8 05/22] xen/arm: ffa: add flags for
> > FFA_PARTITION_INFO_GET
> >
> > Defines flags used for the function FFA_PARTITION_INFO_GET.
>
> Nit: Similarly as my comment for patch #4, I would suggest that in
> commit message you can mention the documentation number and
> the chapter of FFA_PARTITION_INFO_GET. Something like:
> "According to DEN0077A version 1.1 REL0, section 13.8, defines
> flags used for the function FFA_PARTITION_INFO_GET"

I'll add that.

>
> >
> > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > ---
> >  xen/arch/arm/tee/ffa.c | 34 ++++++++++++++++++++++++++++++++++
> >  1 file changed, 34 insertions(+)
> >
> > diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
> > index ba0942e76993..72e7d0575de5 100644
> > --- a/xen/arch/arm/tee/ffa.c
> > +++ b/xen/arch/arm/tee/ffa.c
> > @@ -57,6 +57,40 @@
> >  #define FFA_MY_VERSION
> > MAKE_FFA_VERSION(FFA_MY_VERSION_MAJOR, \
> >                                                   FFA_MY_VERSION_MINOR)
> >
> > +/*
> > + * Flags to determine partition properties in FFA_PARTITION_INFO_GET
> > return
> > + * message:
> > + * BIT(0): Supports receipt of direct requests
> > + * BIT(1): Can send direct requests
> > + * BIT(2): Can send and receive indirect messages
> > + * BIT(3): Supports receipt of notifications
> > + * BIT(4-5): Partition ID is a PE endpoint ID
> > + * BIT(6): Partition must be informed about each VM that is created by
> > + *         the Hypervisor
> > + * BIT(7): Partition must be informed about each VM that is destroyed by
> > + *         the Hypervisor
> > + * BIT(8): Partition runs in the AArch64 execution state else AArch32
> > + *         execution state
> > + */
> > +#define FFA_PART_PROP_DIRECT_REQ_RECV   BIT(0, U)
> > +#define FFA_PART_PROP_DIRECT_REQ_SEND   BIT(1, U)
> > +#define FFA_PART_PROP_INDIRECT_MSGS     BIT(2, U)
> > +#define FFA_PART_PROP_RECV_NOTIF        BIT(3, U)
> > +#define FFA_PART_PROP_IS_MASK           (3U << 4)
>
> I am a bit confused here, here (3U<<4) is "IS_MASK" but...
>
> > +#define FFA_PART_PROP_IS_PE_ID          (0U << 4)
> > +#define FFA_PART_PROP_IS_SEPID_INDEP    (1U << 4)
> > +#define FFA_PART_PROP_IS_SEPID_DEP      (2U << 4)
> > +#define FFA_PART_PROP_IS_AUX_ID         (3U << 4)
>
> ...here the same value is used for "IS_AUX_ID". According to
> the spec that I referred to, bit[5:4] has the following encoding:
> b'11: Partition ID is an auxiliary ID. Hence I guess the above
> "IS_MASK" should be removed?

FFA_PART_PROP_IS_MASK is supposed to be used when extracting the bits
to compare with one of the other  FFA_PART_PROP_IS_* defines. For
example:
if ((props & FFA_PART_PROP_IS_MASK) == FFA_PART_PROP_IS_PE_ID)

using
if ((props & FFA_PART_PROP_IS_AUX_ID) == FFA_PART_PROP_IS_PE_ID)

doesn't seem right.

>
> I confirm the values of other fields are consistent with the spec.

Thanks,
Jens

>
> Kind regards,
> Henry

Re: [XEN PATCH v8 06/22] xen/arm: ffa: add defines for framework direct request/response messages

[Jens Wiklander]

Hi Henry,

On Thu, Apr 13, 2023 at 12:51 PM Henry Wang <Henry.Wang@arm.com> wrote:
>
> Hi Jens,
>
> > -----Original Message-----
> > Subject: [XEN PATCH v8 06/22] xen/arm: ffa: add defines for framework direct
> > request/response messages
> >
> > Adds defines for framework direct request/response messages.
>
> Same here, it actually took me a while to find the related chapters
> about this patch, so I would suggest that we can add more details
> about where the values are from.
>
> From the spec that I referred to (DEN0077A version 1.1 REL0), they are
> in section 18.3, Table 18.{21, 25, 26, 27,28}.

OK, I'll add it.

>
> >
> > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
>
> I confirm the values introduced by this patch are consistent with the
> above spec, hence:
>
> Reviewed-by: Henry Wang <Henry.Wang@arm.com>

Thanks,
Jens

>
> Kind regards,
> Henry

RE: [XEN PATCH v8 05/22] xen/arm: ffa: add flags for FFA_PARTITION_INFO_GET

[Henry Wang]

Hi Jens,

> -----Original Message-----
> From: Jens Wiklander <jens.wiklander@linaro.org>
> Subject: Re: [XEN PATCH v8 05/22] xen/arm: ffa: add flags for
> FFA_PARTITION_INFO_GET
> > > +#define FFA_PART_PROP_DIRECT_REQ_RECV   BIT(0, U)
> > > +#define FFA_PART_PROP_DIRECT_REQ_SEND   BIT(1, U)
> > > +#define FFA_PART_PROP_INDIRECT_MSGS     BIT(2, U)
> > > +#define FFA_PART_PROP_RECV_NOTIF        BIT(3, U)
> > > +#define FFA_PART_PROP_IS_MASK           (3U << 4)
> >
> > I am a bit confused here, here (3U<<4) is "IS_MASK" but...
> >
> > > +#define FFA_PART_PROP_IS_PE_ID          (0U << 4)
> > > +#define FFA_PART_PROP_IS_SEPID_INDEP    (1U << 4)
> > > +#define FFA_PART_PROP_IS_SEPID_DEP      (2U << 4)
> > > +#define FFA_PART_PROP_IS_AUX_ID         (3U << 4)
> >
> > ...here the same value is used for "IS_AUX_ID". According to
> > the spec that I referred to, bit[5:4] has the following encoding:
> > b'11: Partition ID is an auxiliary ID. Hence I guess the above
> > "IS_MASK" should be removed?
> 
> FFA_PART_PROP_IS_MASK is supposed to be used when extracting the bits
> to compare with one of the other  FFA_PART_PROP_IS_* defines. For
> example:
> if ((props & FFA_PART_PROP_IS_MASK) == FFA_PART_PROP_IS_PE_ID)

Ohh I now understand, the naming does not mean it "is a mask" but actually
means "this is a mask for FFA_PART_PROP_IS_". That makes a lot of sense.

To avoid this kind of ambiguity, do you think changing the name to something
like "FFA_PART_PROP_IS_TYPE_MASK" makes sense here? Note that this
is just my suggestion, you can decide to change or not, I am asking just
because I downloaded the whole series and found that currently
FFA_PART_PROP_IS_MASK is not used anywhere, so before it is used everywhere
in the code, it might be good to use a more clear name.

> 
> using
> if ((props & FFA_PART_PROP_IS_AUX_ID) == FFA_PART_PROP_IS_PE_ID)
> 
> doesn't seem right.

Indeed. Please see my above reply.

Personally after the above clarification, I am good with the patch, so:

Reviewed-by: Henry Wang <Henry.Wang@arm.com>

Kind regards,
Henry

> 
> >
> > I confirm the values of other fields are consistent with the spec.
> 
> Thanks,
> Jens
> 
> >
> > Kind regards,
> > Henry

Re: [XEN PATCH v8 07/22] xen/arm: ffa: enforce dependency on 4k pages

[Jens Wiklander]

Hi Henry,

On Thu, Apr 13, 2023 at 12:55 PM Henry Wang <Henry.Wang@arm.com> wrote:
>
> Hi Jens,
>
> > -----Original Message-----
> > Subject: [XEN PATCH v8 07/22] xen/arm: ffa: enforce dependency on 4k pages
> >
> > Adds a BUILD_BUG_ON() to assert the dependency on 4k pages in the FF-A
> > mediator since the current implementation only works if Xen page size is
> > the same as the FFA page size.
> >
> > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
>
> Reviewed-by: Henry Wang <Henry.Wang@arm.com>

I'll add that.

Thanks,
Jens

>
> Kind regards,
> Henry

Re: [XEN PATCH v8 22/22] docs: add Arm FF-A mediator

[Jens Wiklander]

Hi Henry,

On Thu, Apr 13, 2023 at 1:20 PM Henry Wang <Henry.Wang@arm.com> wrote:
>
> Hi Jens,
>
> > -----Original Message-----
> > Subject: [XEN PATCH v8 22/22] docs: add Arm FF-A mediator
> >
> > Describes a FF-A version 1.1 [1] mediator to communicate with a Secure
> > Partition in secure world.
> >
> > [1] https://developer.arm.com/documentation/den0077/latest
> > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > ---
> >  SUPPORT.md               |  8 ++++++++
> >  docs/man/xl.cfg.5.pod.in | 15 +++++++++++++++
> >  2 files changed, 23 insertions(+)
> >
> > +B<Arm only.> Allow a guest to communicate via FF-A with Secure Partitions
> > +(SP), default false.
> > +
> > +Currently is only a small subset of the FF-A specification supported. Just
>
> I am not a native English speaker but I think this sentence would better be:
> "Currently only a small subset of the FF-A specification is supported."

I'll update it.

>
> Other parts look good to me, so:
>
> Reviewed-by: Henry Wang <Henry.Wang@arm.com>

Thanks,
Jens

>
> Kind regards,
> Henry
>

Re: [XEN PATCH v8 21/22] xen/arm: ffa: list current limitations

[Jens Wiklander]

Hi Henry,

On Thu, Apr 13, 2023 at 1:24 PM Henry Wang <Henry.Wang@arm.com> wrote:
>
> Hi Jens,
>
> > -----Original Message-----
> > Subject: [XEN PATCH v8 21/22] xen/arm: ffa: list current limitations
> >
> > Adds a comments with a list of unsupported FF-A interfaces and
>
> Typo: s/a comments/comments/

OK

>
> > limitations in the implemented FF-A interfaces.
> >
> > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > ---
> >  xen/arch/arm/tee/ffa.c | 32 ++++++++++++++++++++++++++++++++
> >  1 file changed, 32 insertions(+)
> >
> > diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
> > index 0948cc636871..6424c222c885 100644
> > --- a/xen/arch/arm/tee/ffa.c
> > +++ b/xen/arch/arm/tee/ffa.c
> > @@ -13,6 +13,38 @@
> >   *                https://developer.arm.com/documentation/den0077/e
> >   * TEEC-1.0C: TEE Client API Specification version 1.0c available at
> >   *            https://globalplatform.org/specs-library/tee-client-api-specification/
> > + *
> > + * Notes on the the current implementstion.
>
> Typo: s/implementstion/implementation/

OK

>
> > + *
> > + * Unsupported FF-A interfaces:
> > + * o FFA_MSG_POLL and FFA_MSG_SEND - deprecated in FF-A-1.1-REL0
> > + * o FFA_MEM_RETRIEVE_* - Used when sharing memory from an SP to a
> > VM
> > + * o FFA_MEM_DONATE_* and FFA_MEM_LEND_* - Used when tranferring
> > ownership
> > + *   or access of a memory readion
>
> Typo "readion"? Maybe I am wrong but I cannot find this word in the spec.

I'll fix it.

>
> With above typos corrected:
>
> Reviewed-by: Henry Wang <Henry.Wang@arm.com>

Thanks,
Jens

>
> Kind regards,
> Henry

Re: [XEN PATCH v8 14/22] xen/arm: move regpair_to_uint64() and uint64_to_regpair() to regs.h

[Jens Wiklander]

Hi Henry,

On Thu, Apr 13, 2023 at 1:31 PM Henry Wang <Henry.Wang@arm.com> wrote:
>
> Hi Jens,
>
> > -----Original Message-----
> > Subject: [XEN PATCH v8 14/22] xen/arm: move regpair_to_uint64() and
> > uint64_to_regpair() to regs.h
> >
> > Moves the two helper functions regpair_to_uint64() and
> > uint64_to_regpair() from xen/arch/arm/tee/optee.c to the common arm
> > specific regs.h. This enables reuse of these functions in the FF-A
> > mediator in a subsequent patch.
> >
> > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > Reviewed-by: Michal Orzel <michal.orzel@amd.com>
> > Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
>
> Reviewed-by: Henry Wang <Henry.Wang@arm.com>

I'll add it.

Thanks,
Jens

>
> Kind regards,
> Henry

Re: [XEN PATCH v8 15/22] xen/arm: ffa: add defines for sharing memory

[Jens Wiklander]

Hi Henry,

On Thu, Apr 13, 2023 at 1:47 PM Henry Wang <Henry.Wang@arm.com> wrote:
>
> Hi Jens,
>
> > -----Original Message-----
> > Subject: [XEN PATCH v8 15/22] xen/arm: ffa: add defines for sharing memory
> >
> > Adds defines needed for sharing using the function FFA_MEM_SHARE and
> > friends.
>
> Same as my comments in previous patches, I would suggest to also mention
> the references in commit message.

OK, I'll add something.

>
> >
> > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > ---
> >  xen/arch/arm/tee/ffa.c | 60
> > ++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 60 insertions(+)
> >
> > + * FF-A doesn't have any direct requirments on GlobalPlatform or vice
>
> Typo: s/requirements/requirements/

OK

>
> > + * versa, but an implementation can very well use FF-A in order to provide
> > + * a GlobalPlatform interface on top.
> > + *
> > + * Global Platform specification for TEE requires that any TEE
> > + * implementation should allow to share buffers with size of at least
> > + * 512KB, defined in TEEC-1.0C page 24, Table 4-1,
> > + * TEEC_CONFIG_SHAREDMEM_MAX_SIZE.
> > + * Due to align issue mentioned above, we need to increase this
>
> s/align issue/alignment issue/ ?

Yes, I'll make it "the alignment issue".

>
> > + * value with one.
> > + */
> > +#define FFA_MAX_SHM_PAGE_COUNT          (SZ_512K / FFA_PAGE_SIZE + 1)
> > +
> > +/*
> > + * Limits the number of shared buffers that guest can have at once. This
> > + * is to prevent case, when guests tricks XEN into exhausting its own
>
> Typo: s/tricks/trick/

OK

>
> > + * memory by allocating many small buffers. This value has been chosen
> > + * arbitrary.
>
> Typo: s/ arbitrary/arbitrarily/

OK

>
> > + */
> > +#define FFA_MAX_SHM_COUNT               32
> > +
> > +/* FF-A-1.1-REL0 section 10.9.2 Memory region handle, page 167 */
> > +#define FFA_HANDLE_HYP_FLAG             BIT(63, ULL)
> > +#define FFA_HANDLE_INVALID              0xffffffffffffffffULL
> > +
> > +/*
> > + * Memory attributes: Normal memory, Write-Back cacheable, Inner
> > shareable
> > + * Defined in FF-A-1.1-REL0 Table 10.18 at page 175.
> > + */
> > +#define FFA_NORMAL_MEM_REG_ATTR         0x2fU
> > +/*
> > + * Memory access permissions: Read-write
> > + * Defined in FF-A-1.1-REL0 Table 10.15 at page 168.
> > + */
> > +#define FFA_MEM_ACC_RW                  0x2U
> > +
> > +/* FF-A-1.1-REL0 section 10.11.4 Flags usage, page 184-187 */
> > +/* Clear memory before mapping in receiver */
> > +#define FFA_MEMORY_REGION_FLAG_CLEAR            BIT(0, U)
> > +/* Relayer may time slice this operation */
> > +#define FFA_MEMORY_REGION_FLAG_TIME_SLICE       BIT(1, U)
> > +/* Clear memory after receiver relinquishes it */
> > +#define FFA_MEMORY_REGION_FLAG_CLEAR_RELINQUISH BIT(2, U)
> > +/* Share memory transaction */
> > +#define FFA_MEMORY_REGION_TRANSACTION_TYPE_SHARE (1U << 3)
> > +
>
> I confirm the values introduced in this patch are consistent with in-code
> comments on top of them. Thanks for the pointer :)

:-)

>
> With the typos corrected:
> Reviewed-by: Henry Wang <Henry.Wang@arm.com>

Great.

Thanks,
Jens

>
> Kind regards,
> Henry
>
>
> >  /*
> >   * Flags and field values used for the MSG_SEND_DIRECT_REQ/RESP:
> >   * BIT(31): Framework or partition message
> > --
> > 2.34.1
> >
>

Re: [XEN PATCH v8 12/22] xen/arm: ffa: support mapping guest RX/TX buffers

[Julien Grall]

Hi Jens,

On 13/04/2023 08:14, Jens Wiklander wrote:
> +static uint32_t handle_rxtx_map(uint32_t fid, register_t tx_addr,
> +                                register_t rx_addr, uint32_t page_count)
> +{
> +    uint32_t ret = FFA_RET_INVALID_PARAMETERS;
> +    struct domain *d = current->domain;
> +    struct ffa_ctx *ctx = d->arch.tee;
> +    struct page_info *tx_pg;
> +    struct page_info *rx_pg;
> +    p2m_type_t t;
> +    void *rx;
> +    void *tx;
> +
> +    if ( !smccc_is_conv_64(fid) )
> +    {
> +        /*
> +         * Calls using the 32-bit calling convention must ignore the upper
> +         * 32 bits in the argument registers.
> +         */
> +        tx_addr &= UINT32_MAX;
> +        rx_addr &= UINT32_MAX;
> +    }
> +
> +    if ( page_count > FFA_MAX_RXTX_PAGE_COUNT ) {

Coding style:

if ( ... )
{

> +        printk(XENLOG_ERR "ffa: RXTX_MAP: error: %u pages requested (limit %u)\n",
> +               page_count, FFA_MAX_RXTX_PAGE_COUNT);
> +        return FFA_RET_NOT_SUPPORTED;
> +    }
> +
> +    /* Already mapped */
> +    if ( ctx->rx )
> +        return FFA_RET_DENIED;
> +
> +    tx_pg = get_page_from_gfn(d, gfn_x(gaddr_to_gfn(tx_addr)), &t, P2M_ALLOC);

I might be missing something. Here you only get the reference on one 
page. Per the value of FFA_MAX_RXTX_PAGE_COUNT, it looks like the buffer 
can be up to 32 pages.

Can you clarify?

> +    if ( !tx_pg )
> +        return FFA_RET_INVALID_PARAMETERS;
> +    /* Only normal RAM for now */
> +    if ( !p2m_is_ram(t) )

p2m_is_ram() would allow RAM page marked read-only in stage-2. Is it 
intended?

If not, then I think you want to use t != p2m_ram_rw.

> +        goto err_put_tx_pg;
> +
> +    rx_pg = get_page_from_gfn(d, gfn_x(gaddr_to_gfn(rx_addr)), &t, P2M_ALLOC);
> +    if ( !tx_pg )
> +        goto err_put_tx_pg;
> +    /* Only normal RAM for now */
> +    if ( !p2m_is_ram(t) )

Same here.

> +        goto err_put_rx_pg;
> +
> +    tx = __map_domain_page_global(tx_pg);
> +    if ( !tx )
> +        goto err_put_rx_pg;
> +
> +    rx = __map_domain_page_global(rx_pg);
> +    if ( !rx )
> +        goto err_unmap_tx;
> +
> +    ctx->rx = rx;
> +    ctx->tx = tx;
> +    ctx->rx_pg = rx_pg;
> +    ctx->tx_pg = tx_pg;
> +    ctx->page_count = page_count;
> +    ctx->tx_is_free = true;
> +    return FFA_RET_OK;

Cheers,

-- 
Julien Grall

Re: [XEN PATCH v8 13/22] xen/arm: ffa: support guest FFA_PARTITION_INFO_GET

[Julien Grall]

Hi Jens,

On 13/04/2023 08:14, Jens Wiklander wrote:
> Adds support in the mediator to handle FFA_PARTITION_INFO_GET requests
> from a guest. The requests are forwarded to the SPMC and the response is
> translated according to the FF-A version in use by the guest.
> 
> Using FFA_PARTITION_INFO_GET changes the owner of the RX buffer to the
> caller (the guest in this case), so once it is done with the buffer it
> must be released using FFA_RX_RELEASE before another call can be made.
> 
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> ---
>   xen/arch/arm/tee/ffa.c | 137 ++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 134 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
> index 127397d8e448..74b8c517afb8 100644
> --- a/xen/arch/arm/tee/ffa.c
> +++ b/xen/arch/arm/tee/ffa.c
> @@ -166,7 +166,18 @@
>   #define FFA_MSG_SEND                    0x8400006EU
>   #define FFA_MSG_POLL                    0x8400006AU
>   
> +/*
> + * Structs below ending with _1_0 are defined in FF-A-1.0-REL and
> + * struct ending with _1_1 are defined in FF-A-1.1-REL0.
> + */
> +
>   /* Partition information descriptor */
> +struct ffa_partition_info_1_0 {
> +    uint16_t id;
> +    uint16_t execution_context;
> +    uint32_t partition_properties;
> +};
> +
>   struct ffa_partition_info_1_1 {
>       uint16_t id;
>       uint16_t execution_context;
> @@ -183,7 +194,8 @@ struct ffa_ctx {
>       unsigned int page_count;
>       /* FF-A version used by the guest */
>       uint32_t guest_vers;
> -    bool tx_is_free;
> +    bool rx_is_free;

I am a bit confused why this is renamed. Did you introduce tx_is_free by 
mistake? If not, can you name the field correctly from when it is 
introduced?

Cheers,

-- 
Julien Grall

Re: [XEN PATCH v8 17/22] xen/arm: ffa: support sharing memory

[Julien Grall]

Hi Jens,

On 13/04/2023 08:14, Jens Wiklander wrote:
>   static int32_t ffa_direct_req_send_vm(uint16_t sp_id, uint16_t vm_id,
>                                         uint8_t msg)
>   {
> @@ -781,6 +862,400 @@ out:
>                resp.a4 & mask, resp.a5 & mask, resp.a6 & mask, resp.a7 & mask);
>   }
>   
> +/*
> + * Gets all page and assigns them to the supplied shared memory object. If
> + * this function fails then the caller is still expected to call
> + * put_shm_pages() as a cleanup.
> + */
> +static int get_shm_pages(struct domain *d, struct ffa_shm_mem *shm,
> +                         const struct ffa_address_range *range,
> +                         uint32_t range_count, unsigned int start_page_idx,
> +                         unsigned int *last_page_idx)
> +{
> +    unsigned int pg_idx = start_page_idx;
> +    gfn_t gfn;
> +    unsigned int n;
> +    unsigned int m;
> +    p2m_type_t t;
> +    uint64_t addr;
> +
> +    for ( n = 0; n < range_count; n++ )
> +    {
> +        for ( m = 0; m < range[n].page_count; m++ )
> +        {
> +            if ( pg_idx >= shm->page_count )
> +                return FFA_RET_INVALID_PARAMETERS;
> +
> +            addr = read_atomic(&range[n].address);

I am confused with the use of read_atomic() here. Is this part of the 
guest memory? If so, why don't page_count is also not read atomically?

Also, it looks like you are using you will read the same address 
atomically. Shouldn't this be moved just before the loop?

> +            gfn = gaddr_to_gfn(addr + m * FFA_PAGE_SIZE);
> +            shm->pages[pg_idx] = get_page_from_gfn(d, gfn_x(gfn), &t,
> +						   P2M_ALLOC);
> +            if ( !shm->pages[pg_idx] )
> +                return FFA_RET_DENIED;
> +            /* Only normal RAM for now */
> +            if ( !p2m_is_ram(t) )
> +                return FFA_RET_DENIED;
> +            pg_idx++;
> +        }
> +    }
> +
> +    *last_page_idx = pg_idx;
> +
> +    return FFA_RET_OK;
> +}
> +
> +static void put_shm_pages(struct ffa_shm_mem *shm)
> +{
> +    unsigned int n;
> +
> +    for ( n = 0; n < shm->page_count && shm->pages[n]; n++ )
> +    {
> +        put_page(shm->pages[n]);
> +        shm->pages[n] = NULL;
> +    }
> +}
> +
> +static struct ffa_shm_mem *alloc_ffa_shm_mem(struct ffa_ctx *ctx,
> +                                             unsigned int page_count)
> +{
> +    struct ffa_shm_mem *shm;
> +
> +    if ( page_count >= FFA_MAX_SHM_PAGE_COUNT ||
> +         ctx->shm_count >= FFA_MAX_SHM_COUNT )
> +        return NULL;
> +
> +    shm = xzalloc_flex_struct(struct ffa_shm_mem, pages, page_count);
> +    if ( shm )
> +    {
> +        ctx->shm_count++;
> +        shm->page_count = page_count;
> +    }
> +
> +    return shm;
> +}
> +
> +static void free_ffa_shm_mem(struct ffa_ctx *ctx, struct ffa_shm_mem *shm)
> +{
> +    if ( shm ) {

Coding style:

if ( ... )
{

but I would prefer if we remove one level of indentation and use:

if ( !shm )
   return;

> +        ASSERT(ctx->shm_count > 0);
> +        ctx->shm_count--;
> +        put_shm_pages(shm);
> +        xfree(shm);
> +    }
> +}

Cheers,

-- 
Julien Grall

Re: [XEN PATCH v8 21/22] xen/arm: ffa: list current limitations

[Julien Grall]

Hi Jens,

On 13/04/2023 08:14, Jens Wiklander wrote:
> Adds a comments with a list of unsupported FF-A interfaces and
> limitations in the implemented FF-A interfaces.
> 
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> ---
>   xen/arch/arm/tee/ffa.c | 32 ++++++++++++++++++++++++++++++++
>   1 file changed, 32 insertions(+)
> 
> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
> index 0948cc636871..6424c222c885 100644
> --- a/xen/arch/arm/tee/ffa.c
> +++ b/xen/arch/arm/tee/ffa.c
> @@ -13,6 +13,38 @@
>    *                https://developer.arm.com/documentation/den0077/e
>    * TEEC-1.0C: TEE Client API Specification version 1.0c available at
>    *            https://globalplatform.org/specs-library/tee-client-api-specification/
> + *
> + * Notes on the the current implementstion.
> + *
> + * Unsupported FF-A interfaces:
> + * o FFA_MSG_POLL and FFA_MSG_SEND - deprecated in FF-A-1.1-REL0
> + * o FFA_MEM_RETRIEVE_* - Used when sharing memory from an SP to a VM
> + * o FFA_MEM_DONATE_* and FFA_MEM_LEND_* - Used when tranferring ownership
> + *   or access of a memory readion
> + * o FFA_MSG_SEND2 and FFA_MSG_WAIT - Used for indirect messaging
> + * o FFA_MSG_YIELD
> + * o FFA_INTERRUPT - Used to report preemption
> + * o FFA_RUN
> + *
> + * Limitations in the implemented FF-A interfaces:
> + * o FFA_RXTX_MAP_*:
> + *   - Maps at most 32 4k pages large RX and TX buffers
> + *   - RT/TX buffers must be normal RAM

Can you explain why this is a problem?

> + *   - Doesn't support forwarding this call on behalf of an endpoint
> + * o FFA_MEM_SHARE_*: only supports sharing
> + *   - from a VM to an SP
> + *   - with one borrower
> + *   - with the memory transaction descriptor in the RX/TX buffer
> + *   - normal memory
> + *   - at most 512 kB large memory regions
> + *   - at most 32 shared memory regions per guest
> + * o FFA_MSG_SEND_DIRECT_REQ:
> + *   - only supported from a VM to an SP
> + *
> + * There are some large locked sections with ffa_tx_buffer_lock and
> + * ffa_rx_buffer_lock. Especially the ffa_tx_buffer_lock spinlock used
> + * around share_shm() is a very large locked section which can let one VM
> + * affect another VM.
>    */
>   
>   #include <xen/bitops.h>

Cheers,

-- 
Julien Grall

Re: [XEN PATCH v8 22/22] docs: add Arm FF-A mediator

[Julien Grall]

Hi Jens,

On 13/04/2023 08:14, Jens Wiklander wrote:
> Describes a FF-A version 1.1 [1] mediator to communicate with a Secure
> Partition in secure world.
> 
> [1] https://developer.arm.com/documentation/den0077/latest
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> ---
>   SUPPORT.md               |  8 ++++++++
>   docs/man/xl.cfg.5.pod.in | 15 +++++++++++++++
>   2 files changed, 23 insertions(+)
> 
> diff --git a/SUPPORT.md b/SUPPORT.md
> index aa1940e55f09..1fd746f7f7f2 100644
> --- a/SUPPORT.md
> +++ b/SUPPORT.md
> @@ -818,6 +818,14 @@ that covers the DMA of the device to be passed through.
>   
>   No support for QEMU backends in a 16K or 64K domain.
>   
> +### ARM: Firmware Framework for Arm A-profile (FF-A) Mediator
> +
> +    Status, Arm64: Tech Preview
> +
> +There are still some code paths where a vCPU may hog a pCPU longer than
> +necessary. The FF-A mediator is not yet implemented for Arm32. Part of the
> +FF-A specification is not supported.

NIT: You would suggest to add: "(See the top comment in ...)". So one 
can easily find the limitation.

> +
>   ### ARM: Guest Device Tree support
>   
>       Status: Supported
> diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
> index 10f37990be57..bba99c576b48 100644
> --- a/docs/man/xl.cfg.5.pod.in
> +++ b/docs/man/xl.cfg.5.pod.in
> @@ -1645,6 +1645,21 @@ in OP-TEE.
>   
>   This feature is a B<technology preview>.
>   
> +=item B<ffa>
> +
> +B<Arm only.> Allow a guest to communicate via FF-A with Secure Partitions
> +(SP), default false.
> +
> +Currently is only a small subset of the FF-A specification supported. Just
> +enough to communicate with OP-TEE. In general only direct messaging and
> +sharing memory with one SP. More advanced use cases where memory might be
> +shared or donated to multple SPs are not supported.

Typo: s/multple/multiple/

> +
> +See L<https://developer.arm.com/documentation/den0077/latest> for more
> +informantion about FF-A.

Typo: s/informantion/information/

> +
> +This feature is a B<technology preview>.
> +
>   =back
>   
>   =back

-- 
Julien Grall

Re: [XEN PATCH v8 16/22] xen/arm: ffa: add ABI structs for sharing memory

[Jens Wiklander]

Hi Henry,

On Thu, Apr 13, 2023 at 1:49 PM Henry Wang <Henry.Wang@arm.com> wrote:
>
> Hi Jens,
>
> > -----Original Message-----
> > Subject: [XEN PATCH v8 16/22] xen/arm: ffa: add ABI structs for sharing
> > memory
> >
> > Adds the ABI structs used by function FFA_MEM_SHARE and friends for
> > sharing memory.
> >
> > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
>
> Reviewed-by: Henry Wang <Henry.Wang@arm.com>

I'll add that.

Thanks,
Jens

>
> Kind regards,
> Henry

Re: [XEN PATCH v8 03/22] tools: add Arm FF-A mediator

[Jens Wiklander]

Hi Henry,

On Thu, Apr 13, 2023 at 1:53 PM Henry Wang <Henry.Wang@arm.com> wrote:
>
> Hi Jens,
>
> > -----Original Message-----
> > Subject: [XEN PATCH v8 03/22] tools: add Arm FF-A mediator
> >
> > Adds a new "ffa" value to the Enumeration "tee_type" to indicate if a
> > guest is trusted to use FF-A.
> >
> > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
>
> Reviewed-by: Henry Wang <Henry.Wang@arm.com>

I'll add that.

Thanks,
Jens

>
> Kind regards,
> Henry

Re: [XEN PATCH v8 11/22] xen/arm: ffa: send guest events to Secure Partitions

[Jens Wiklander]

Hi,

On Thu, Apr 13, 2023 at 3:24 PM Julien Grall <julien@xen.org> wrote:
>
> Hi,
>
> On 13/04/2023 08:14, Jens Wiklander wrote:
> > +static int32_t ffa_direct_req_send_vm(uint16_t sp_id, uint16_t vm_id,
> > +                                      uint8_t msg)
> > +{
> > +    uint32_t exp_resp = FFA_MSG_FLAG_FRAMEWORK;
> > +    int32_t res;
> > +
> > +    if ( msg == FFA_MSG_SEND_VM_CREATED )
> > +        exp_resp |= FFA_MSG_RESP_VM_CREATED;
> > +    else if ( msg == FFA_MSG_SEND_VM_DESTROYED )
> > +        exp_resp |= FFA_MSG_RESP_VM_DESTROYED;
> > +    else
> > +        return FFA_RET_INVALID_PARAMETERS;
> > +
> > +    do {
> > +        const struct arm_smccc_1_2_regs arg = {
> > +            .a0 = FFA_MSG_SEND_DIRECT_REQ_32,
> > +            .a1 = sp_id,
> > +            .a2 = FFA_MSG_FLAG_FRAMEWORK | msg,
> > +            .a5 = vm_id,
> > +        };
> > +        struct arm_smccc_1_2_regs resp;
> > +
> > +        arm_smccc_1_2_smc(&arg, &resp);
> > +        if ( resp.a0 != FFA_MSG_SEND_DIRECT_RESP_32 || resp.a2 != exp_resp )
> > +        {
> > +            /*
> > +             * This is an invalid response, likely due to some error in the
> > +             * implementation of the ABI.
> > +             */
> > +            return FFA_RET_INVALID_PARAMETERS;
> > +        }
> > +        res = resp.a3;
> > +    } while ( res == FFA_RET_INTERRUPTED || res == FFA_RET_RETRY );
>
> This loop seems potentially unbounded to me. Can you add a comment
> explaining why this is fine?

In the FF-A 1.1 specification
(https://developer.arm.com/documentation/den0077/e/?lang=en) Table
18.26 at page 330 it says that FFA_RET_INTERRUPTED and FFA_RET_RETRY
should be handled in this way. When looking at this from the
hypervisor's point of view it is troublesome since there isn't any
guarantee that we're progressing.

We should be able to rule out FFA_RET_INTERRUPTED since non-secure
interrupts should be masked at this point. I'm not sure if
FFA_RET_RETRY can be avoided entirely, but we should be able to put a
limit on how many times we're prepared to retry.

How about setting a limit of max 10 retries and treating
FFA_RET_INTERRUPTED as an error? Or is it better to not loop at all
and treat all but FFA_RET_OK as errors? What do others think?

Thanks,
Jens

>
> > +
> > +    return res;
> > +}
> > +
>
> Cheers,
>
> --
> Julien Grall

Re: [XEN PATCH v8 11/22] xen/arm: ffa: send guest events to Secure Partitions

[Bertrand Marquis]

Hi Jens,

> On 14 Apr 2023, at 10:19, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> 
> Hi,
> 
> On Thu, Apr 13, 2023 at 3:24 PM Julien Grall <julien@xen.org> wrote:
>> 
>> Hi,
>> 
>> On 13/04/2023 08:14, Jens Wiklander wrote:
>>> +static int32_t ffa_direct_req_send_vm(uint16_t sp_id, uint16_t vm_id,
>>> +                                      uint8_t msg)
>>> +{
>>> +    uint32_t exp_resp = FFA_MSG_FLAG_FRAMEWORK;
>>> +    int32_t res;
>>> +
>>> +    if ( msg == FFA_MSG_SEND_VM_CREATED )
>>> +        exp_resp |= FFA_MSG_RESP_VM_CREATED;
>>> +    else if ( msg == FFA_MSG_SEND_VM_DESTROYED )
>>> +        exp_resp |= FFA_MSG_RESP_VM_DESTROYED;
>>> +    else
>>> +        return FFA_RET_INVALID_PARAMETERS;
>>> +
>>> +    do {
>>> +        const struct arm_smccc_1_2_regs arg = {
>>> +            .a0 = FFA_MSG_SEND_DIRECT_REQ_32,
>>> +            .a1 = sp_id,
>>> +            .a2 = FFA_MSG_FLAG_FRAMEWORK | msg,
>>> +            .a5 = vm_id,
>>> +        };
>>> +        struct arm_smccc_1_2_regs resp;
>>> +
>>> +        arm_smccc_1_2_smc(&arg, &resp);
>>> +        if ( resp.a0 != FFA_MSG_SEND_DIRECT_RESP_32 || resp.a2 != exp_resp )
>>> +        {
>>> +            /*
>>> +             * This is an invalid response, likely due to some error in the
>>> +             * implementation of the ABI.
>>> +             */
>>> +            return FFA_RET_INVALID_PARAMETERS;
>>> +        }
>>> +        res = resp.a3;
>>> +    } while ( res == FFA_RET_INTERRUPTED || res == FFA_RET_RETRY );
>> 
>> This loop seems potentially unbounded to me. Can you add a comment
>> explaining why this is fine?
> 
> In the FF-A 1.1 specification
> (https://developer.arm.com/documentation/den0077/e/?lang=en) Table
> 18.26 at page 330 it says that FFA_RET_INTERRUPTED and FFA_RET_RETRY
> should be handled in this way. When looking at this from the
> hypervisor's point of view it is troublesome since there isn't any
> guarantee that we're progressing.
> 
> We should be able to rule out FFA_RET_INTERRUPTED since non-secure
> interrupts should be masked at this point. I'm not sure if
> FFA_RET_RETRY can be avoided entirely, but we should be able to put a
> limit on how many times we're prepared to retry.

The fact that interrupts are masked in Xen does not mean they will be in secure.
In fact what we should do when INTERRUPTED is received is something we have
to clear up but we should unmask interrupt to process them in Xen before retrying I think.

> 
> How about setting a limit of max 10 retries and treating
> FFA_RET_INTERRUPTED as an error? Or is it better to not loop at all
> and treat all but FFA_RET_OK as errors? What do others think?

I would suggest to do a max retry for both cases and add a TODO in the code.
We will need to define a generic way to handle those cases but at this stage
INTERRUPTED should be considered TODO.
RETRY will probably stay with a limit here, in the case of a guest message both
of those possibilities could just be returned to the guest.


Do you agree ?

Cheers
Bertrand

> 
> Thanks,
> Jens
> 
>> 
>>> +
>>> +    return res;
>>> +}
>>> +
>> 
>> Cheers,
>> 
>> --
>> Julien Grall

Re: [XEN PATCH v8 02/22] xen/arm: tee: add a primitive FF-A mediator

[Jens Wiklander]

Hi,

On Thu, Apr 13, 2023 at 3:27 PM Andrew Cooper <andrew.cooper3@citrix.com> 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 09/22] xen/arm: ffa: add direct request support

[Jens Wiklander]

Hi Henry,

On Thu, Apr 13, 2023 at 1:16 PM Henry Wang <Henry.Wang@arm.com> wrote:
>
> Hi Jens,
>
> > -----Original Message-----
> > Subject: [XEN PATCH v8 09/22] xen/arm: ffa: add direct request support
> >
> > Adds support for sending a FF-A direct request. Checks that the SP also
> > supports handling a 32-bit direct request. 64-bit direct requests are
> > not used by the mediator itself so there is not need to check for that.
> >
> > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > ---
> >  xen/arch/arm/tee/ffa.c | 112
> > +++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 112 insertions(+)
> >
> > diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
> > index f129879c5b81..f2cce955d981 100644
> > --- a/xen/arch/arm/tee/ffa.c
> > +++ b/xen/arch/arm/tee/ffa.c
> > @@ -181,6 +181,56 @@ static bool ffa_get_version(uint32_t *vers)
> >      return true;
> >  }
> >
> > +static int32_t get_ffa_ret_code(const struct arm_smccc_1_2_regs *resp)
> > +{
> > +    switch ( resp->a0 )
> > +    {
> > +    case FFA_ERROR:
> > +        if ( resp->a2 )
> > +            return resp->a2;
> > +        else
> > +            return FFA_RET_NOT_SUPPORTED;
> > +    case FFA_SUCCESS_32:
> > +    case FFA_SUCCESS_64:
> > +        return FFA_RET_OK;
> > +    default:
> > +        return FFA_RET_NOT_SUPPORTED;
> > +    }
> > +}
> > +
> > +static int32_t ffa_simple_call(uint32_t fid, register_t a1, register_t a2,
> > +                               register_t a3, register_t a4)
> > +{
> > +    const struct arm_smccc_1_2_regs arg = {
> > +        .a0 = fid,
> > +        .a1 = a1,
> > +        .a2 = a2,
> > +        .a3 = a3,
> > +        .a4 = a4,
> > +    };
> > +    struct arm_smccc_1_2_regs resp;
> > +
> > +    arm_smccc_1_2_smc(&arg, &resp);
> > +
> > +    return get_ffa_ret_code(&resp);
> > +}
> > +
> > +static int32_t ffa_features(uint32_t id)
> > +{
> > +    return ffa_simple_call(FFA_FEATURES, id, 0, 0, 0);
> > +}
> > +
> > +static bool check_mandatory_feature(uint32_t id)
> > +{
> > +    int32_t ret = ffa_features(id);
> > +
> > +    if (ret)
>
> Coding style nit: You need spaces before and after "ret", i.e.
> if ( ret )
>
> With this fixed:
>
> Reviewed-by: Henry Wang <Henry.Wang@arm.com>

I'll fix it.

Thanks,
Jens

>
> Kind regards,
> Henry

Re: [XEN PATCH v8 09/22] xen/arm: ffa: add direct request support

[Jens Wiklander]

On Thu, Apr 13, 2023 at 3:44 PM Bertrand Marquis
<Bertrand.Marquis@arm.com> wrote:
>
> Hi,
>
> > On 13 Apr 2023, at 15:27, Julien Grall <julien@xen.org> wrote:
> >
> >
> >
> > On 13/04/2023 14:20, Bertrand Marquis wrote:
> >> Hi Julien,
> >>> On 13 Apr 2023, at 15:15, Julien Grall <julien@xen.org> wrote:
> >>>
> >>> Hi,
> >>>
> >>> On 13/04/2023 08:14, Jens Wiklander wrote:
> >>>> Adds support for sending a FF-A direct request. Checks that the SP also
> >>>> supports handling a 32-bit direct request. 64-bit direct requests are
> >>>> not used by the mediator itself so there is not need to check for that.
> >>>> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> >>>> ---
> >>>>  xen/arch/arm/tee/ffa.c | 112 +++++++++++++++++++++++++++++++++++++++++
> >>>>  1 file changed, 112 insertions(+)
> >>>> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
> >>>> index f129879c5b81..f2cce955d981 100644
> >>>> --- a/xen/arch/arm/tee/ffa.c
> >>>> +++ b/xen/arch/arm/tee/ffa.c
> >>>> @@ -181,6 +181,56 @@ static bool ffa_get_version(uint32_t *vers)
> >>>>      return true;
> >>>>  }
> >>>>  +static int32_t get_ffa_ret_code(const struct arm_smccc_1_2_regs *resp)
> >>>> +{
> >>>> +    switch ( resp->a0 )
> >>>> +    {
> >>>> +    case FFA_ERROR:
> >>>> +        if ( resp->a2 )
> >>>> +            return resp->a2;
> >>>> +        else
> >>>> +            return FFA_RET_NOT_SUPPORTED;
> >>>> +    case FFA_SUCCESS_32:
> >>>> +    case FFA_SUCCESS_64:
> >>>> +        return FFA_RET_OK;
> >>>> +    default:
> >>>> +        return FFA_RET_NOT_SUPPORTED;
> >>>> +    }
> >>>> +}
> >>>> +
> >>>> +static int32_t ffa_simple_call(uint32_t fid, register_t a1, register_t a2,
> >>>> +                               register_t a3, register_t a4)
> >>>> +{
> >>>> +    const struct arm_smccc_1_2_regs arg = {
> >>>> +        .a0 = fid,
> >>>> +        .a1 = a1,
> >>>> +        .a2 = a2,
> >>>> +        .a3 = a3,
> >>>> +        .a4 = a4,
> >>>> +    };
> >>>> +    struct arm_smccc_1_2_regs resp;
> >>>> +
> >>>> +    arm_smccc_1_2_smc(&arg, &resp);
> >>>> +
> >>>> +    return get_ffa_ret_code(&resp);
> >>>> +}
> >>>> +
> >>>> +static int32_t ffa_features(uint32_t id)
> >>>> +{
> >>>> +    return ffa_simple_call(FFA_FEATURES, id, 0, 0, 0);
> >>>> +}
> >>>> +
> >>>> +static bool check_mandatory_feature(uint32_t id)
> >>>> +{
> >>>> +    int32_t ret = ffa_features(id);
> >>>> +
> >>>> +    if (ret)
> >>>> +        printk(XENLOG_ERR "ffa: mandatory feature id %#x missing: error %d\n",
> >>>> +               id, ret);
> >>>> +
> >>>> +    return !ret;
> >>>> +}
> >>>> +
> >>>>  static uint16_t get_vm_id(const struct domain *d)
> >>>>  {
> >>>>      /* +1 since 0 is reserved for the hypervisor in FF-A */
> >>>> @@ -222,6 +272,57 @@ static void handle_version(struct cpu_user_regs *regs)
> >>>>      set_regs(regs, vers, 0, 0, 0, 0, 0, 0, 0);
> >>>>  }
> >>>>  +static void handle_msg_send_direct_req(struct cpu_user_regs *regs, uint32_t fid)
> >>>> +{
> >>>> +    struct arm_smccc_1_2_regs arg = { .a0 = fid, };
> >>>> +    struct arm_smccc_1_2_regs resp = { };
> >>>> +    struct domain *d = current->domain;
> >>>> +    uint32_t src_dst;
> >>>> +    uint64_t mask;
> >>>> +
> >>>> +    if ( smccc_is_conv_64(fid) )
> >>>> +        mask = GENMASK_ULL(63, 0);
> >>>> +    else
> >>>> +        mask = GENMASK_ULL(31, 0);
> >>>> +
> >>>> +    src_dst = get_user_reg(regs, 1);
> >>>> +    if ( (src_dst >> 16) != get_vm_id(d) )
> >>>> +    {
> >>>> +        resp.a0 = FFA_ERROR;
> >>>> +        resp.a2 = FFA_RET_INVALID_PARAMETERS;
> >>>> +        goto out;
> >>>> +    }
> >>>> +
> >>>> +    arg.a1 = src_dst;
> >>>> +    arg.a2 = get_user_reg(regs, 2) & mask;
> >>>> +    arg.a3 = get_user_reg(regs, 3) & mask;
> >>>> +    arg.a4 = get_user_reg(regs, 4) & mask;
> >>>> +    arg.a5 = get_user_reg(regs, 5) & mask;
> >>>> +    arg.a6 = get_user_reg(regs, 6) & mask;
> >>>> +    arg.a7 = get_user_reg(regs, 7) & mask;
> >>>> +
> >>>> +    arm_smccc_1_2_smc(&arg, &resp);
> >>>> +    switch ( resp.a0 )
> >>>> +    {
> >>>> +    case FFA_ERROR:
> >>>> +    case FFA_SUCCESS_32:
> >>>> +    case FFA_SUCCESS_64:
> >>>> +    case FFA_MSG_SEND_DIRECT_RESP_32:
> >>>> +    case FFA_MSG_SEND_DIRECT_RESP_64:
> >>>> +        break;
> >>>> +    default:
> >>>> +        /* Bad fid, report back. */
> >>>> +        memset(&arg, 0, sizeof(arg));
> >>>> +        arg.a0 = FFA_ERROR;
> >>>> +        arg.a1 = src_dst;
> >>>> +        arg.a2 = FFA_RET_ABORTED;
> >>>> +    }
> >>>> +
> >>>> +out:
> >>>> +    set_regs(regs, resp.a0, resp.a1 & mask, resp.a2 & mask, resp.a3 & mask,
> >>>> +             resp.a4 & mask, resp.a5 & mask, resp.a6 & mask, resp.a7 & mask);
> >>>> +}
> >>>> +
> >>>>  static bool ffa_handle_call(struct cpu_user_regs *regs)
> >>>>  {
> >>>>      uint32_t fid = get_user_reg(regs, 0);
> >>>> @@ -239,6 +340,10 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
> >>>>      case FFA_ID_GET:
> >>>>          set_regs_success(regs, get_vm_id(d), 0);
> >>>>          return true;
> >>>> +    case FFA_MSG_SEND_DIRECT_REQ_32:
> >>>> +    case FFA_MSG_SEND_DIRECT_REQ_64:
> >>>> +        handle_msg_send_direct_req(regs, fid);
> >>>> +        return true;
> >>>>        default:
> >>>>          gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid);
> >>>> @@ -326,6 +431,13 @@ static bool ffa_probe(void)
> >>>>      printk(XENLOG_INFO "ARM FF-A Firmware version %u.%u\n",
> >>>>             major_vers, minor_vers);
> >>>>  +    /*
> >>>> +     * TODO save result of checked features and use that information to
> >>>> +     * accept or reject requests from guests.
> >>>> +     */
> >>>
> >>> I am not entirely sure I understand this TODO. Does it mean a guest can currently use a request that is not supported by FFA?
> >> In fact this is a bit the opposite: in the following patch we check that all features we could need are supported but if a guest is only using a subset we might not need to have all of them.
> >> Idea of this TODO would be to save the features supported and refuse guest requests depending on the features needed for them.
> >
> > Thanks. I would suggest the following comment:
> >
> > /*
> > * At the moment domains must supports the same features used by Xen.
> > * TODO: Rework the code to allow domain to use a subset of the features
> > * supported.
> > */
> >
> > Note that I am using "domains" rather than "guests" because the latter doesn't include dom0.
>
> Makes sense and new comment is nice.

That's better, I'll update it.

Thanks,
Jens

>
> Up to Jens to say if he is ok with it.
>
> Cheers
> Bertrand
>
> >
> > Cheers,
> >
> > --
> > Julien Grall
>
>

Re: [XEN PATCH v8 05/22] xen/arm: ffa: add flags for FFA_PARTITION_INFO_GET

[Jens Wiklander]

Hi Henry,

On Thu, Apr 13, 2023 at 3:53 PM Henry Wang <Henry.Wang@arm.com> wrote:
>
> Hi Jens,
>
> > -----Original Message-----
> > From: Jens Wiklander <jens.wiklander@linaro.org>
> > Subject: Re: [XEN PATCH v8 05/22] xen/arm: ffa: add flags for
> > FFA_PARTITION_INFO_GET
> > > > +#define FFA_PART_PROP_DIRECT_REQ_RECV   BIT(0, U)
> > > > +#define FFA_PART_PROP_DIRECT_REQ_SEND   BIT(1, U)
> > > > +#define FFA_PART_PROP_INDIRECT_MSGS     BIT(2, U)
> > > > +#define FFA_PART_PROP_RECV_NOTIF        BIT(3, U)
> > > > +#define FFA_PART_PROP_IS_MASK           (3U << 4)
> > >
> > > I am a bit confused here, here (3U<<4) is "IS_MASK" but...
> > >
> > > > +#define FFA_PART_PROP_IS_PE_ID          (0U << 4)
> > > > +#define FFA_PART_PROP_IS_SEPID_INDEP    (1U << 4)
> > > > +#define FFA_PART_PROP_IS_SEPID_DEP      (2U << 4)
> > > > +#define FFA_PART_PROP_IS_AUX_ID         (3U << 4)
> > >
> > > ...here the same value is used for "IS_AUX_ID". According to
> > > the spec that I referred to, bit[5:4] has the following encoding:
> > > b'11: Partition ID is an auxiliary ID. Hence I guess the above
> > > "IS_MASK" should be removed?
> >
> > FFA_PART_PROP_IS_MASK is supposed to be used when extracting the bits
> > to compare with one of the other  FFA_PART_PROP_IS_* defines. For
> > example:
> > if ((props & FFA_PART_PROP_IS_MASK) == FFA_PART_PROP_IS_PE_ID)
>
> Ohh I now understand, the naming does not mean it "is a mask" but actually
> means "this is a mask for FFA_PART_PROP_IS_". That makes a lot of sense.
>
> To avoid this kind of ambiguity, do you think changing the name to something
> like "FFA_PART_PROP_IS_TYPE_MASK" makes sense here? Note that this
> is just my suggestion, you can decide to change or not, I am asking just
> because I downloaded the whole series and found that currently
> FFA_PART_PROP_IS_MASK is not used anywhere, so before it is used everywhere
> in the code, it might be good to use a more clear name.
>
> >
> > using
> > if ((props & FFA_PART_PROP_IS_AUX_ID) == FFA_PART_PROP_IS_PE_ID)
> >
> > doesn't seem right.
>
> Indeed. Please see my above reply.
>
> Personally after the above clarification, I am good with the patch, so:
>
> Reviewed-by: Henry Wang <Henry.Wang@arm.com>

I'll update it as you suggest.

Thanks,
Jens

>
> Kind regards,
> Henry
>
> >
> > >
> > > I confirm the values of other fields are consistent with the spec.
> >
> > Thanks,
> > Jens
> >
> > >
> > > Kind regards,
> > > Henry

Re: [XEN PATCH v8 11/22] xen/arm: ffa: send guest events to Secure Partitions

[Jens Wiklander]

Hi Bertrand,

On Fri, Apr 14, 2023 at 10:29 AM Bertrand Marquis
<Bertrand.Marquis@arm.com> wrote:
>
> Hi Jens,
>
> > On 14 Apr 2023, at 10:19, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> >
> > Hi,
> >
> > On Thu, Apr 13, 2023 at 3:24 PM Julien Grall <julien@xen.org> wrote:
> >>
> >> Hi,
> >>
> >> On 13/04/2023 08:14, Jens Wiklander wrote:
> >>> +static int32_t ffa_direct_req_send_vm(uint16_t sp_id, uint16_t vm_id,
> >>> +                                      uint8_t msg)
> >>> +{
> >>> +    uint32_t exp_resp = FFA_MSG_FLAG_FRAMEWORK;
> >>> +    int32_t res;
> >>> +
> >>> +    if ( msg == FFA_MSG_SEND_VM_CREATED )
> >>> +        exp_resp |= FFA_MSG_RESP_VM_CREATED;
> >>> +    else if ( msg == FFA_MSG_SEND_VM_DESTROYED )
> >>> +        exp_resp |= FFA_MSG_RESP_VM_DESTROYED;
> >>> +    else
> >>> +        return FFA_RET_INVALID_PARAMETERS;
> >>> +
> >>> +    do {
> >>> +        const struct arm_smccc_1_2_regs arg = {
> >>> +            .a0 = FFA_MSG_SEND_DIRECT_REQ_32,
> >>> +            .a1 = sp_id,
> >>> +            .a2 = FFA_MSG_FLAG_FRAMEWORK | msg,
> >>> +            .a5 = vm_id,
> >>> +        };
> >>> +        struct arm_smccc_1_2_regs resp;
> >>> +
> >>> +        arm_smccc_1_2_smc(&arg, &resp);
> >>> +        if ( resp.a0 != FFA_MSG_SEND_DIRECT_RESP_32 || resp.a2 != exp_resp )
> >>> +        {
> >>> +            /*
> >>> +             * This is an invalid response, likely due to some error in the
> >>> +             * implementation of the ABI.
> >>> +             */
> >>> +            return FFA_RET_INVALID_PARAMETERS;
> >>> +        }
> >>> +        res = resp.a3;
> >>> +    } while ( res == FFA_RET_INTERRUPTED || res == FFA_RET_RETRY );
> >>
> >> This loop seems potentially unbounded to me. Can you add a comment
> >> explaining why this is fine?
> >
> > In the FF-A 1.1 specification
> > (https://developer.arm.com/documentation/den0077/e/?lang=en) Table
> > 18.26 at page 330 it says that FFA_RET_INTERRUPTED and FFA_RET_RETRY
> > should be handled in this way. When looking at this from the
> > hypervisor's point of view it is troublesome since there isn't any
> > guarantee that we're progressing.
> >
> > We should be able to rule out FFA_RET_INTERRUPTED since non-secure
> > interrupts should be masked at this point. I'm not sure if
> > FFA_RET_RETRY can be avoided entirely, but we should be able to put a
> > limit on how many times we're prepared to retry.
>
> The fact that interrupts are masked in Xen does not mean they will be in secure.
> In fact what we should do when INTERRUPTED is received is something we have
> to clear up but we should unmask interrupt to process them in Xen before retrying I think.
>
> >
> > How about setting a limit of max 10 retries and treating
> > FFA_RET_INTERRUPTED as an error? Or is it better to not loop at all
> > and treat all but FFA_RET_OK as errors? What do others think?
>
> I would suggest to do a max retry for both cases and add a TODO in the code.
> We will need to define a generic way to handle those cases but at this stage
> INTERRUPTED should be considered TODO.
> RETRY will probably stay with a limit here, in the case of a guest message both
> of those possibilities could just be returned to the guest.
>
>
> Do you agree ?

Sounds good to me, I'll do that.

Thanks,
Jens

>
> Cheers
> Bertrand
>
> >
> > Thanks,
> > Jens
> >
> >>
> >>> +
> >>> +    return res;
> >>> +}
> >>> +
> >>
> >> Cheers,
> >>
> >> --
> >> Julien Grall
>
>

Re: [XEN PATCH v8 22/22] docs: add Arm FF-A mediator

[Jens Wiklander]

Hi Julien,

On Thu, Apr 13, 2023 at 11:00 PM Julien Grall <julien@xen.org> wrote:
>
> Hi Jens,
>
> On 13/04/2023 08:14, Jens Wiklander wrote:
> > Describes a FF-A version 1.1 [1] mediator to communicate with a Secure
> > Partition in secure world.
> >
> > [1] https://developer.arm.com/documentation/den0077/latest
> > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > ---
> >   SUPPORT.md               |  8 ++++++++
> >   docs/man/xl.cfg.5.pod.in | 15 +++++++++++++++
> >   2 files changed, 23 insertions(+)
> >
> > diff --git a/SUPPORT.md b/SUPPORT.md
> > index aa1940e55f09..1fd746f7f7f2 100644
> > --- a/SUPPORT.md
> > +++ b/SUPPORT.md
> > @@ -818,6 +818,14 @@ that covers the DMA of the device to be passed through.
> >
> >   No support for QEMU backends in a 16K or 64K domain.
> >
> > +### ARM: Firmware Framework for Arm A-profile (FF-A) Mediator
> > +
> > +    Status, Arm64: Tech Preview
> > +
> > +There are still some code paths where a vCPU may hog a pCPU longer than
> > +necessary. The FF-A mediator is not yet implemented for Arm32. Part of the
> > +FF-A specification is not supported.
>
> NIT: You would suggest to add: "(See the top comment in ...)". So one
> can easily find the limitation.

Good point, I'll fix that.

>
> > +
> >   ### ARM: Guest Device Tree support
> >
> >       Status: Supported
> > diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
> > index 10f37990be57..bba99c576b48 100644
> > --- a/docs/man/xl.cfg.5.pod.in
> > +++ b/docs/man/xl.cfg.5.pod.in
> > @@ -1645,6 +1645,21 @@ in OP-TEE.
> >
> >   This feature is a B<technology preview>.
> >
> > +=item B<ffa>
> > +
> > +B<Arm only.> Allow a guest to communicate via FF-A with Secure Partitions
> > +(SP), default false.
> > +
> > +Currently is only a small subset of the FF-A specification supported. Just
> > +enough to communicate with OP-TEE. In general only direct messaging and
> > +sharing memory with one SP. More advanced use cases where memory might be
> > +shared or donated to multple SPs are not supported.
>
> Typo: s/multple/multiple/
>
> > +
> > +See L<https://developer.arm.com/documentation/den0077/latest> for more
> > +informantion about FF-A.
>
> Typo: s/informantion/information/

I'll fix the typos.

Thanks,
Jens

>
> > +
> > +This feature is a B<technology preview>.
> > +
> >   =back
> >
> >   =back
>
> --
> Julien Grall

Re: [XEN PATCH v8 21/22] xen/arm: ffa: list current limitations

[Jens Wiklander]

Hi Julien,

On Thu, Apr 13, 2023 at 10:57 PM Julien Grall <julien@xen.org> wrote:
>
> Hi Jens,
>
> On 13/04/2023 08:14, Jens Wiklander wrote:
> > Adds a comments with a list of unsupported FF-A interfaces and
> > limitations in the implemented FF-A interfaces.
> >
> > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > ---
> >   xen/arch/arm/tee/ffa.c | 32 ++++++++++++++++++++++++++++++++
> >   1 file changed, 32 insertions(+)
> >
> > diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
> > index 0948cc636871..6424c222c885 100644
> > --- a/xen/arch/arm/tee/ffa.c
> > +++ b/xen/arch/arm/tee/ffa.c
> > @@ -13,6 +13,38 @@
> >    *                https://developer.arm.com/documentation/den0077/e
> >    * TEEC-1.0C: TEE Client API Specification version 1.0c available at
> >    *            https://globalplatform.org/specs-library/tee-client-api-specification/
> > + *
> > + * Notes on the the current implementstion.
> > + *
> > + * Unsupported FF-A interfaces:
> > + * o FFA_MSG_POLL and FFA_MSG_SEND - deprecated in FF-A-1.1-REL0
> > + * o FFA_MEM_RETRIEVE_* - Used when sharing memory from an SP to a VM
> > + * o FFA_MEM_DONATE_* and FFA_MEM_LEND_* - Used when tranferring ownership
> > + *   or access of a memory readion
> > + * o FFA_MSG_SEND2 and FFA_MSG_WAIT - Used for indirect messaging
> > + * o FFA_MSG_YIELD
> > + * o FFA_INTERRUPT - Used to report preemption
> > + * o FFA_RUN
> > + *
> > + * Limitations in the implemented FF-A interfaces:
> > + * o FFA_RXTX_MAP_*:
> > + *   - Maps at most 32 4k pages large RX and TX buffers
> > + *   - RT/TX buffers must be normal RAM
>
> Can you explain why this is a problem?

Good catch, I can't. I must have added it by mistake. I'll remove it.

Thanks,
Jens

>
> > + *   - Doesn't support forwarding this call on behalf of an endpoint
> > + * o FFA_MEM_SHARE_*: only supports sharing
> > + *   - from a VM to an SP
> > + *   - with one borrower
> > + *   - with the memory transaction descriptor in the RX/TX buffer
> > + *   - normal memory
> > + *   - at most 512 kB large memory regions
> > + *   - at most 32 shared memory regions per guest
> > + * o FFA_MSG_SEND_DIRECT_REQ:
> > + *   - only supported from a VM to an SP
> > + *
> > + * There are some large locked sections with ffa_tx_buffer_lock and
> > + * ffa_rx_buffer_lock. Especially the ffa_tx_buffer_lock spinlock used
> > + * around share_shm() is a very large locked section which can let one VM
> > + * affect another VM.
> >    */
> >
> >   #include <xen/bitops.h>
>
> Cheers,
>
> --
> Julien Grall

Re: [XEN PATCH v8 12/22] xen/arm: ffa: support mapping guest RX/TX buffers

[Jens Wiklander]

Hi Julien,

On Thu, Apr 13, 2023 at 10:31 PM Julien Grall <julien@xen.org> wrote:
>
> Hi Jens,
>
> On 13/04/2023 08:14, Jens Wiklander wrote:
> > +static uint32_t handle_rxtx_map(uint32_t fid, register_t tx_addr,
> > +                                register_t rx_addr, uint32_t page_count)
> > +{
> > +    uint32_t ret = FFA_RET_INVALID_PARAMETERS;
> > +    struct domain *d = current->domain;
> > +    struct ffa_ctx *ctx = d->arch.tee;
> > +    struct page_info *tx_pg;
> > +    struct page_info *rx_pg;
> > +    p2m_type_t t;
> > +    void *rx;
> > +    void *tx;
> > +
> > +    if ( !smccc_is_conv_64(fid) )
> > +    {
> > +        /*
> > +         * Calls using the 32-bit calling convention must ignore the upper
> > +         * 32 bits in the argument registers.
> > +         */
> > +        tx_addr &= UINT32_MAX;
> > +        rx_addr &= UINT32_MAX;
> > +    }
> > +
> > +    if ( page_count > FFA_MAX_RXTX_PAGE_COUNT ) {
>
> Coding style:

OK

>
> if ( ... )
> {
>
> > +        printk(XENLOG_ERR "ffa: RXTX_MAP: error: %u pages requested (limit %u)\n",
> > +               page_count, FFA_MAX_RXTX_PAGE_COUNT);
> > +        return FFA_RET_NOT_SUPPORTED;
> > +    }
> > +
> > +    /* Already mapped */
> > +    if ( ctx->rx )
> > +        return FFA_RET_DENIED;
> > +
> > +    tx_pg = get_page_from_gfn(d, gfn_x(gaddr_to_gfn(tx_addr)), &t, P2M_ALLOC);
>
> I might be missing something. Here you only get the reference on one
> page. Per the value of FFA_MAX_RXTX_PAGE_COUNT, it looks like the buffer
> can be up to 32 pages.
>
> Can you clarify?

Good catch. I'll reduce FFA_MAX_RXTX_PAGE_COUNT to 1 since that's what
I've been testing with. I'll add a TODO for supporting a larger
number.

>
> > +    if ( !tx_pg )
> > +        return FFA_RET_INVALID_PARAMETERS;
> > +    /* Only normal RAM for now */
> > +    if ( !p2m_is_ram(t) )
>
> p2m_is_ram() would allow RAM page marked read-only in stage-2. Is it
> intended?
>
> If not, then I think you want to use t != p2m_ram_rw.

Thanks, I'll update it.

>
> > +        goto err_put_tx_pg;
> > +
> > +    rx_pg = get_page_from_gfn(d, gfn_x(gaddr_to_gfn(rx_addr)), &t, P2M_ALLOC);
> > +    if ( !tx_pg )
> > +        goto err_put_tx_pg;
> > +    /* Only normal RAM for now */
> > +    if ( !p2m_is_ram(t) )
>
> Same here.

OK

Thanks,
Jens

>
> > +        goto err_put_rx_pg;
> > +
> > +    tx = __map_domain_page_global(tx_pg);
> > +    if ( !tx )
> > +        goto err_put_rx_pg;
> > +
> > +    rx = __map_domain_page_global(rx_pg);
> > +    if ( !rx )
> > +        goto err_unmap_tx;
> > +
> > +    ctx->rx = rx;
> > +    ctx->tx = tx;
> > +    ctx->rx_pg = rx_pg;
> > +    ctx->tx_pg = tx_pg;
> > +    ctx->page_count = page_count;
> > +    ctx->tx_is_free = true;
> > +    return FFA_RET_OK;
>
> Cheers,
>
> --
> Julien Grall

Re: [XEN PATCH v8 13/22] xen/arm: ffa: support guest FFA_PARTITION_INFO_GET

[Jens Wiklander]

Hi Julien,

On Thu, Apr 13, 2023 at 10:33 PM Julien Grall <julien@xen.org> wrote:
>
> Hi Jens,
>
> On 13/04/2023 08:14, Jens Wiklander wrote:
> > Adds support in the mediator to handle FFA_PARTITION_INFO_GET requests
> > from a guest. The requests are forwarded to the SPMC and the response is
> > translated according to the FF-A version in use by the guest.
> >
> > Using FFA_PARTITION_INFO_GET changes the owner of the RX buffer to the
> > caller (the guest in this case), so once it is done with the buffer it
> > must be released using FFA_RX_RELEASE before another call can be made.
> >
> > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > ---
> >   xen/arch/arm/tee/ffa.c | 137 ++++++++++++++++++++++++++++++++++++++++-
> >   1 file changed, 134 insertions(+), 3 deletions(-)
> >
> > diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
> > index 127397d8e448..74b8c517afb8 100644
> > --- a/xen/arch/arm/tee/ffa.c
> > +++ b/xen/arch/arm/tee/ffa.c
> > @@ -166,7 +166,18 @@
> >   #define FFA_MSG_SEND                    0x8400006EU
> >   #define FFA_MSG_POLL                    0x8400006AU
> >
> > +/*
> > + * Structs below ending with _1_0 are defined in FF-A-1.0-REL and
> > + * struct ending with _1_1 are defined in FF-A-1.1-REL0.
> > + */
> > +
> >   /* Partition information descriptor */
> > +struct ffa_partition_info_1_0 {
> > +    uint16_t id;
> > +    uint16_t execution_context;
> > +    uint32_t partition_properties;
> > +};
> > +
> >   struct ffa_partition_info_1_1 {
> >       uint16_t id;
> >       uint16_t execution_context;
> > @@ -183,7 +194,8 @@ struct ffa_ctx {
> >       unsigned int page_count;
> >       /* FF-A version used by the guest */
> >       uint32_t guest_vers;
> > -    bool tx_is_free;
> > +    bool rx_is_free;
>
> I am a bit confused why this is renamed. Did you introduce tx_is_free by
> mistake? If not, can you name the field correctly from when it is
> introduced?

I'll fix it.

Thanks,
Jens

>
> Cheers,
>
> --
> Julien Grall

Re: [XEN PATCH v8 17/22] xen/arm: ffa: support sharing memory

[Jens Wiklander]

Hi Julien,

On Thu, Apr 13, 2023 at 10:53 PM Julien Grall <julien@xen.org> wrote:
>
> Hi Jens,
>
> On 13/04/2023 08:14, Jens Wiklander wrote:
> >   static int32_t ffa_direct_req_send_vm(uint16_t sp_id, uint16_t vm_id,
> >                                         uint8_t msg)
> >   {
> > @@ -781,6 +862,400 @@ out:
> >                resp.a4 & mask, resp.a5 & mask, resp.a6 & mask, resp.a7 & mask);
> >   }
> >
> > +/*
> > + * Gets all page and assigns them to the supplied shared memory object. If
> > + * this function fails then the caller is still expected to call
> > + * put_shm_pages() as a cleanup.
> > + */
> > +static int get_shm_pages(struct domain *d, struct ffa_shm_mem *shm,
> > +                         const struct ffa_address_range *range,
> > +                         uint32_t range_count, unsigned int start_page_idx,
> > +                         unsigned int *last_page_idx)
> > +{
> > +    unsigned int pg_idx = start_page_idx;
> > +    gfn_t gfn;
> > +    unsigned int n;
> > +    unsigned int m;
> > +    p2m_type_t t;
> > +    uint64_t addr;
> > +
> > +    for ( n = 0; n < range_count; n++ )
> > +    {
> > +        for ( m = 0; m < range[n].page_count; m++ )
> > +        {
> > +            if ( pg_idx >= shm->page_count )
> > +                return FFA_RET_INVALID_PARAMETERS;
> > +
> > +            addr = read_atomic(&range[n].address);
>
> I am confused with the use of read_atomic() here. Is this part of the
> guest memory? If so, why don't page_count is also not read atomically?
>
> Also, it looks like you are using you will read the same address
> atomically. Shouldn't this be moved just before the loop?

You're right it is from guest memory and we should use read_atomoc()
only once. I'll fix it.

>
> > +            gfn = gaddr_to_gfn(addr + m * FFA_PAGE_SIZE);
> > +            shm->pages[pg_idx] = get_page_from_gfn(d, gfn_x(gfn), &t,
> > +                                                P2M_ALLOC);
> > +            if ( !shm->pages[pg_idx] )
> > +                return FFA_RET_DENIED;
> > +            /* Only normal RAM for now */
> > +            if ( !p2m_is_ram(t) )
> > +                return FFA_RET_DENIED;
> > +            pg_idx++;
> > +        }
> > +    }
> > +
> > +    *last_page_idx = pg_idx;
> > +
> > +    return FFA_RET_OK;
> > +}
> > +
> > +static void put_shm_pages(struct ffa_shm_mem *shm)
> > +{
> > +    unsigned int n;
> > +
> > +    for ( n = 0; n < shm->page_count && shm->pages[n]; n++ )
> > +    {
> > +        put_page(shm->pages[n]);
> > +        shm->pages[n] = NULL;
> > +    }
> > +}
> > +
> > +static struct ffa_shm_mem *alloc_ffa_shm_mem(struct ffa_ctx *ctx,
> > +                                             unsigned int page_count)
> > +{
> > +    struct ffa_shm_mem *shm;
> > +
> > +    if ( page_count >= FFA_MAX_SHM_PAGE_COUNT ||
> > +         ctx->shm_count >= FFA_MAX_SHM_COUNT )
> > +        return NULL;
> > +
> > +    shm = xzalloc_flex_struct(struct ffa_shm_mem, pages, page_count);
> > +    if ( shm )
> > +    {
> > +        ctx->shm_count++;
> > +        shm->page_count = page_count;
> > +    }
> > +
> > +    return shm;
> > +}
> > +
> > +static void free_ffa_shm_mem(struct ffa_ctx *ctx, struct ffa_shm_mem *shm)
> > +{
> > +    if ( shm ) {
>
> Coding style:
>
> if ( ... )
> {
>
> but I would prefer if we remove one level of indentation and use:
>
> if ( !shm )
>    return;

OK, I'll change it.

Thanks,
Jens

>
> > +        ASSERT(ctx->shm_count > 0);
> > +        ctx->shm_count--;
> > +        put_shm_pages(shm);
> > +        xfree(shm);
> > +    }
> > +}
>
> Cheers,
>
> --
> Julien Grall

Re: [XEN PATCH v8 02/22] xen/arm: tee: add a primitive FF-A mediator

[Bertrand Marquis]

Hi Andrew,

> On 14 Apr 2023, at 10:58, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> 
> Hi,
> 
> On Thu, Apr 13, 2023 at 3:27 PM Andrew Cooper <andrew.cooper3@citrix.com> 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 03/22] tools: add Arm FF-A mediator

[Anthony PERARD]

On Thu, Apr 13, 2023 at 09:14:05AM +0200, Jens Wiklander wrote:
> Adds a new "ffa" value to the Enumeration "tee_type" to indicate if a
> guest is trusted to use FF-A.

Is "ffa" working yet in the hypervisor? (At this point in the patch
series) I'm asking because the doc change is at the end of the patch
series and this patch at the beginning.

I feel like this patch would be better at the end of the series, just
before the doc change, when the hypervisor is ready for it.

In any case:
Acked-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD

Re: [XEN PATCH v8 03/22] tools: add Arm FF-A mediator

[Jens Wiklander]

On Thu, May 18, 2023 at 4:35 PM Anthony PERARD
<anthony.perard@citrix.com> wrote:
>
> On Thu, Apr 13, 2023 at 09:14:05AM +0200, Jens Wiklander wrote:
> > Adds a new "ffa" value to the Enumeration "tee_type" to indicate if a
> > guest is trusted to use FF-A.
>
> Is "ffa" working yet in the hypervisor? (At this point in the patch
> series) I'm asking because the doc change is at the end of the patch
> series and this patch at the beginning.
>
> I feel like this patch would be better at the end of the series, just
> before the doc change, when the hypervisor is ready for it.

Makes sense, I'll move it.

>
> In any case:
> Acked-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,
Jens

>
> Thanks,
>
> --
> Anthony PERARD

Re: [XEN PATCH v8 02/22] xen/arm: tee: add a primitive FF-A mediator

[Bertrand Marquis]

Hi Andrew,

> On 4 May 2023, at 15:14, Bertrand Marquis <Bertrand.Marquis@arm.com> wrote:
> 
> Hi Andrew,
> 
>> On 14 Apr 2023, at 10:58, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>> 
>> Hi,
>> 
>> On Thu, Apr 13, 2023 at 3:27 PM Andrew Cooper <andrew.cooper3@citrix.com> 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