Hi Jens,
I haven't checked whether the FFA driver is complaint with the spec. I
mainly checked whether the code makes sense from Xen PoV.
This is a fairly long patch to review. So I will split my review in
multiple session/e-mail.
On 22/06/2022 14:42, Jens Wiklander wrote:
Adds a FF-A version 1.1 [1] mediator to communicate with a Secure
Partition in secure world.
The implementation is the bare minimum to be able to communicate with
OP-TEE running as an SPMC at S-EL1.
This is loosely based on the TEE mediator framework and the OP-TEE
mediator.
[1] https://developer.arm.com/documentation/den0077/latest
Signed-off-by: Jens Wiklander <jens.wiklan...@linaro.org>
---
SUPPORT.md | 7 +
tools/libs/light/libxl_arm.c | 3 +
tools/libs/light/libxl_types.idl | 1 +
tools/xl/xl_parse.c | 3 +
These changes would need an ack from the toolstack maintainers.
xen/arch/arm/Kconfig | 11 +
xen/arch/arm/Makefile | 1 +
xen/arch/arm/domain.c | 10 +
xen/arch/arm/domain_build.c | 1 +
xen/arch/arm/ffa.c | 1683 +++++++++++++++++++++++++++++
xen/arch/arm/include/asm/domain.h | 4 +
xen/arch/arm/include/asm/ffa.h | 71 ++
xen/arch/arm/vsmc.c | 17 +-
xen/include/public/arch-arm.h | 2 +
13 files changed, 1811 insertions(+), 3 deletions(-)
create mode 100644 xen/arch/arm/ffa.c
create mode 100644 xen/arch/arm/include/asm/ffa.h
diff --git a/SUPPORT.md b/SUPPORT.md
index 70e98964cbc0..215bb3c9043b 100644
--- a/SUPPORT.md
+++ b/SUPPORT.md
@@ -785,6 +785,13 @@ 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.
+
### ARM: Guest Device Tree support
Status: Supported
diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
index eef1de093914..a985609861c7 100644
--- a/tools/libs/light/libxl_arm.c
+++ b/tools/libs/light/libxl_arm.c
@@ -101,6 +101,9 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
return ERROR_FAIL;
}
+ config->arch.ffa_enabled =
+ libxl_defbool_val(d_config->b_info.arch_arm.ffa_enabled);
+
return 0;
}
diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
index 2a42da2f7d78..bf4544bef399 100644
--- a/tools/libs/light/libxl_types.idl
+++ b/tools/libs/light/libxl_types.idl
@@ -646,6 +646,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
("vuart", libxl_vuart_type),
+ ("ffa_enabled", libxl_defbool),
This needs to be accompagnied with a define LIBXL_HAVE_* in
tools/include/libxl.h. Please see the examples in the header.
])),
("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
])),
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index b98c0de378b6..e0e99ed8d2b1 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -2746,6 +2746,9 @@ skip_usbdev:
exit(-ERROR_FAIL);
}
}
+ libxl_defbool_setdefault(&b_info->arch_arm.ffa_enabled, false);
+ xlu_cfg_get_defbool(config, "ffa_enabled",
+ &b_info->arch_arm.ffa_enabled, 0);
This option needs to be documented in docs/man/xl.cfg.5.pod.in.
parse_vkb_list(config, d_config);
diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index be9eff014120..e57e1d3757e2 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -139,6 +139,17 @@ config TEE
source "arch/arm/tee/Kconfig"
+config FFA
+ bool "Enable FF-A mediator support" if EXPERT
+ 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.
Where would a user be able to find which subset of the specification
that Xen implements?
+
+ [1] https://developer.arm.com/documentation/den0077/latest
+
endmenu
menu "ARM errata workaround via the alternative framework"
diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index bb7a6151c13c..af0c69f793d4 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -20,6 +20,7 @@ obj-y += domain_build.init.o
obj-y += domctl.o
obj-$(CONFIG_EARLY_PRINTK) += early_printk.o
obj-y += efi/
+obj-$(CONFIG_FFA) += ffa.o
obj-y += gic.o
obj-y += gic-v2.o
obj-$(CONFIG_GICV3) += gic-v3.o
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 8110c1df8638..a3f00e7e234d 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -27,6 +27,7 @@
#include <asm/cpufeature.h>
#include <asm/current.h>
#include <asm/event.h>
+#include <asm/ffa.h>
#include <asm/gic.h>
#include <asm/guest_atomics.h>
#include <asm/irq.h>
@@ -756,6 +757,9 @@ int arch_domain_create(struct domain *d,
if ( (rc = tee_domain_init(d, config->arch.tee_type)) != 0 )
goto fail;
+ if ( (rc = ffa_domain_init(d, config->arch.ffa_enabled)) != 0 )
+ goto fail;
+
update_domain_wallclock_time(d);
/*
@@ -998,6 +1002,7 @@ static int relinquish_memory(struct domain *d, struct
page_list_head *list)
enum {
PROG_pci = 1,
PROG_tee,
+ PROG_ffa,
PROG_xen,
PROG_page,
PROG_mapping,
@@ -1043,6 +1048,11 @@ int domain_relinquish_resources(struct domain *d)
PROGRESS(tee):
ret = tee_relinquish_resources(d);
+ if ( ret )
+ return ret;
+
+ PROGRESS(ffa):
+ ret = ffa_relinquish_resources(d);
if (ret )
return ret;
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 7ddd16c26da5..d708f76356f7 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -3450,6 +3450,7 @@ void __init create_dom0(void)
if ( gic_number_lines() > 992 )
printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded.\n");
dom0_cfg.arch.tee_type = tee_get_type();
+ dom0_cfg.arch.ffa_enabled = true;
AFAICT, ffa_enabled is a uint8_t. So we should use 1. However, I am not
convinced that using a uint8_t for what looks like a boolean is warrant.
I will detail more on the definition.
dom0_cfg.max_vcpus = dom0_max_vcpus();
if ( iommu_enabled )
diff --git a/xen/arch/arm/ffa.c b/xen/arch/arm/ffa.c
new file mode 100644
index 000000000000..3117ce5cec4d
--- /dev/null
+++ b/xen/arch/arm/ffa.c
@@ -0,0 +1,1683 @@
+/*
+ * xen/arch/arm/ffa.c
+ *
+ * Arm Firmware Framework for ARMv8-A (FF-A) mediator
+ *
+ * Copyright (C) 2022 Linaro Limited
+ *
+ * Permission is hereby granted, free of charge, to any person
+ * obtaining a copy of this software and associated documentation
+ * files (the "Software"), to deal in the Software without restriction,
+ * including without limitation the rights to use, copy, modify, merge,
+ * publish, distribute, sublicense, and/or sell copies of the Software,
+ * and to permit persons to whom the Software is furnished to do so,
+ * subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be
+ * included in all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.
+ * IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY
+ * CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
+ * TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE
+ * SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
This doesn't look like to be a GPLv2 license. Can you clarify which
license you are using?
+ */
+
+#include <xen/domain_page.h>
+#include <xen/errno.h>
+#include <xen/init.h>
+#include <xen/lib.h>
+#include <xen/sched.h>
+#include <xen/types.h>
+#include <xen/sizes.h>
+#include <xen/bitops.h>
+
+#include <asm/smccc.h>
+#include <asm/event.h>
+#include <asm/ffa.h>
+#include <asm/regs.h>
+
+/* Error codes */
All the #define below are using hard tab. Please use soft tab.
+#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 _AC(1,U)
NIT: The use of _AC() is a bit pointless given that you are only use the
values in C code.
+#define FFA_VERSION_MAJOR_SHIFT _AC(16,U)
+#define FFA_VERSION_MAJOR_MASK _AC(0x7FFF,U)
+#define FFA_VERSION_MINOR _AC(1,U)
+#define FFA_VERSION_MINOR_SHIFT _AC(0,U)
+#define FFA_VERSION_MINOR_MASK _AC(0xFFFF,U)
+#define MAKE_FFA_VERSION(major, minor) \
+ ((((major) & FFA_VERSION_MAJOR_MASK) << FFA_VERSION_MAJOR_SHIFT) | \
+ ((minor) & FFA_VERSION_MINOR_MASK))
+
+#define FFA_MIN_VERSION MAKE_FFA_VERSION(1, 0)
+#define FFA_VERSION_1_0 MAKE_FFA_VERSION(1, 0)
+#define FFA_VERSION_1_1 MAKE_FFA_VERSION(1, 1)
+#define FFA_MY_VERSION MAKE_FFA_VERSION(FFA_VERSION_MAJOR, \
+ FFA_VERSION_MINOR)
NIT: I think it would be better that FFA_VERSION_MAJOR AND
FFA_VERSION_MINOR are defined closer to FFA_MY_VERSION and they are
renamed to FFA_MY_VERSION_*.
I would also potentially move the 3 defines past all the definition
related to the specification. This would make clearer that this is what
Xen supports.
+
+
+#define FFA_HANDLE_HYP_FLAG BIT(63,ULL)
Coding style: You seem to use a mix of FOO(... , ...) and FOO(...,...).
At mimimum, please be consistent within the file.
For Xen, we usually add a space after the comma.
[...]
+/*
+ * Our rx/rx buffer shared with the SPMC
+ */
Hmmm... The comment seems to be misplaced.
+static uint32_t ffa_version;
This probably can be __read_mostly.
+static uint16_t *subsr_vm_created;
What subsr stand for?
+static unsigned int subsr_vm_created_count;
+static uint16_t *subsr_vm_destroyed;
+static unsigned int subsr_vm_destroyed_count;
+static void *ffa_rx;
+static void *ffa_tx;
subsr_vm_created, subsr_vm_destroyed, ffa_rx and ffa_tx can probably be
__read_mostly.
+static unsigned int ffa_page_count;
Is this related to ffa_rx?
+static DEFINE_SPINLOCK(ffa_buffer_lock);
+
+static LIST_HEAD(ffa_mem_list);
+static DEFINE_SPINLOCK(ffa_mem_list_lock);
+
+static uint64_t next_handle = FFA_HANDLE_HYP_FLAG;
next_handle only seems to be used handle_mem_share(). So I think it
would be better to move the definition (as static) within the function.
This will reduce the number of global variables.
+
+static inline uint64_t reg_pair_to_64(uint32_t reg0, uint32_t reg1)
+{
+ return (uint64_t)reg0 << 32 | reg1;
+}
+
+static void inline reg_pair_from_64(uint32_t *reg0, uint32_t *reg1,
+ uint64_t val)
+{
+ *reg0 = val >> 32;
+ *reg1 = val;
+}
Those two functions are the same as optee.c but with a different. I
would rather prefer if we avoid the duplication and provide generic
helpers in a pre-requisite.
+
+static bool ffa_get_version(uint32_t *vers)
+{
+ const struct arm_smccc_1_2_regs arg = {
+ .a0 = FFA_VERSION, .a1 = FFA_MY_VERSION,
Coding sytle: Please set each field on their own line.
+ };
+ struct arm_smccc_1_2_regs resp;
+
+ arm_smccc_1_2_smc(&arg, &resp);
+ if ( resp.a0 == FFA_RET_NOT_SUPPORTED )
+ {
+ printk(XENLOG_ERR "ffa: FFA_VERSION returned not supported\n");
+ return false;
+ }
+
+ *vers = resp.a0;
Coding style: We tend to add a newline before the last return. I am not
necessarily going to comment about this on all the instances. So please
have a look through the code.
+ return true;
+}
+
+static uint32_t get_ffa_ret_code(const struct arm_smccc_1_2_regs *resp)
FFA_RET_* will be negative, so shouldn't this return int32_t?
+{
+ 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 uint32_t ffa_features(uint32_t id)
+{
+ const struct arm_smccc_1_2_regs arg = { .a0 = FFA_FEATURES, .a1 = id, };
Coding style. Please split each field on their own line.
+ struct arm_smccc_1_2_regs resp;
+
+ arm_smccc_1_2_smc(&arg, &resp);
+ return get_ffa_ret_code(&resp);
+}
+
+static bool check_mandatory_feature(uint32_t id)
+{
+ uint32_t ret = ffa_features(id);
+
+ if (ret)
+ printk(XENLOG_ERR "ffa: mandatory feature id %#x missing\n", id);
+ return !ret;
+}
+
+static uint32_t ffa_rxtx_map(register_t tx_addr, register_t rx_addr,
+ uint32_t page_count)
Aside the parameters, the helper looks very similar to ffa_features(),
ffa_rxtx_unmap(). Can this be abstracted? Maybe using macro if you still
want to have separate helper name.
+{
+ const struct arm_smccc_1_2_regs arg = {
+#ifdef CONFIG_ARM_64
+ .a0 = FFA_RXTX_MAP_64,
+#endif
+#ifdef CONFIG_ARM_32
+ .a0 = FFA_RXTX_MAP_32,
+#endif
+ .a1 = tx_addr, .a2 = rx_addr,
Coding: Please don't use hard tab and put each field on their own line.
+ .a3 = page_count,
+ };
+ struct arm_smccc_1_2_regs resp;
+
+ arm_smccc_1_2_smc(&arg, &resp);
+
+ return get_ffa_ret_code(&resp);
+}
+
+static uint32_t ffa_rxtx_unmap(uint16_t vm_id)
+{
+ const struct arm_smccc_1_2_regs arg = {
+ .a0 = FFA_RXTX_UNMAP, .a1 = vm_id,
Coding style. Please add each field on their own line.
+ };
+ struct arm_smccc_1_2_regs resp;
+
+ arm_smccc_1_2_smc(&arg, &resp);
+
+ return get_ffa_ret_code(&resp);
+}
+
+static uint32_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,
Ditto.
+ .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 uint32_t ffa_rx_release(void)
+{
+ const struct arm_smccc_1_2_regs arg = { .a0 = FFA_RX_RELEASE, };
+ struct arm_smccc_1_2_regs resp;
+
+ arm_smccc_1_2_smc(&arg, &resp);
+
+ return get_ffa_ret_code(&resp);
+}
+
+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_32, .a1 = tot_len, .a2 = frag_len, .a3 = addr,
Ditto.
+ .a4 = pg_count,
+ };
+ struct arm_smccc_1_2_regs resp;
+
+ /*
+ * For arm64 we must use 64-bit calling convention if the buffer isn't
+ * passed in our tx buffer.
+ */
Can you explain why we would want to use the 32-bit calling convention
if addr is 0?
+ if (sizeof(addr) > 4 && addr)
sizeof(addr) > 4 is a bit odd to read. I think it would be better to
check that CONFIG_ARM_64 is set (i.e. IS_ENABLED()).
+ arg.a0 = FFA_MEM_SHARE_64;
+
+ 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 = reg_pair_to_64(resp.a3, resp.a2);
+ return FFA_RET_OK;
+ case FFA_MEM_FRAG_RX:
+ *handle = reg_pair_to_64(resp.a2, resp.a1);
+ return resp.a3;
+ default:
+ return FFA_RET_NOT_SUPPORTED;
+ }
+}
+
+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 uint32_t ffa_mem_reclaim(uint32_t handle_lo, uint32_t handle_hi,
+ uint32_t flags)
+{
+ const struct arm_smccc_1_2_regs arg = {
+ .a0 = FFA_MEM_RECLAIM, .a1 = handle_lo, .a2 = handle_hi, .a3 = flags,
+ };
+ struct arm_smccc_1_2_regs resp;
+
+ arm_smccc_1_2_smc(&arg, &resp);
+
+ return get_ffa_ret_code(&resp);
+}
+
+static int32_t ffa_direct_req_send_vm(uint16_t sp_id, uint16_t vm_id,
OOI, why is this call returns int32_t when all the other use uint32_t
(even if may returned negative values)?
+ uint8_t msg)
+{
+ uint32_t exp_resp = FFA_MSG_FLAG_FRAMEWORK;
+ int32_t res;
+
+ if ( msg != FFA_MSG_SEND_VM_CREATED && msg !=FFA_MSG_SEND_VM_DESTROYED )
+ return FFA_RET_INVALID_PARAMETERS;
+
+ if ( msg == FFA_MSG_SEND_VM_CREATED )
+ exp_resp |= FFA_MSG_RESP_VM_CREATED;
+ else
+ exp_resp |= FFA_MSG_RESP_VM_DESTROYED;
NIT: I think it would be easier to read if you drop the first 'if' and
instead write:
if ( msg == ..._CREATED )
...
else if ( msg == ..._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 u16 get_vm_id(struct domain *d)
d is not meant to be modified by the helper. So please use 'const'.
+{
+ /* +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)
+{
+ 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 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)
+{
+ 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;
+ struct ffa_ctx *ctx = d->arch.ffa;
+ 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;
I find a bit strange to 'cap' the version provided by the user. Is this
part of the spec?
+
+ ctx->guest_vers = vers;
+ 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)
Xen, Linux, and the firmware may use different page size. Below, you
seem to have the page size will always be 4KB. Is this guaranteed by the
spec? If not, how do all the 3 components agree on it?
If yes, then I think this should be written down and we should have a
BUILD_BUG_ON(PAGE_SIZE != SZ_4K).
+{
+ uint32_t ret = FFA_RET_INVALID_PARAMETERS;
+ struct domain *d = current->domain;
+ struct ffa_ctx *ctx = d->arch.ffa;
+ struct page_info *tx_pg;
+ struct page_info *rx_pg;
+ p2m_type_t t;
+ void *rx;
+ void *tx;
+
+ if ( !smccc_is_conv_64(fid) )
+ {
+ tx_addr &= UINT32_MAX;
+ rx_addr &= UINT32_MAX;
+ }
+
+ /* For now to keep things simple, only deal with a single page */
+ if ( page_count != 1 )
+ return FFA_RET_NOT_SUPPORTED;
+
+ /* Already mapped */
+ if ( ctx->rx )
+ return FFA_RET_DENIED;
+
+ tx_pg = get_page_from_gfn(d, gaddr_to_gfn(tx_addr), &t, P2M_ALLOC);
+ if ( !tx_pg )
+ return FFA_RET_INVALID_PARAMETERS;
+ /* Only normal RAM for now */
This comment suggests the check below should be p2m_is_ram() but you are
using p2m_ram_rw.
+ if (t != p2m_ram_rw)
Coding style: if ( ... )
+ goto err_put_tx_pg;
+
+ rx_pg = get_page_from_gfn(d, gaddr_to_gfn(rx_addr), &t, P2M_ALLOC);
+ if ( !tx_pg )
+ goto err_put_tx_pg;
+ /* Only normal RAM for now */
Same about the comment.
+ if ( t != p2m_ram_rw )
+ 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 = 1;
+ ctx->tx_is_mine = 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 uint32_t handle_rxtx_unmap(void)
+{
+ struct domain *d = current->domain;
+ struct ffa_ctx *ctx = d->arch.ffa;
+ uint32_t ret;
+
+ if ( !ctx->rx )
+ return FFA_RET_INVALID_PARAMETERS;
+
+ ret = ffa_rxtx_unmap(get_vm_id(d));
+ if ( ret )
+ return ret;
+
+ 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_mine = false;
+
+ return FFA_RET_OK;
+}
+
+static uint32_t handle_partition_info_get(uint32_t w1, uint32_t w2, uint32_t
w3,
+ uint32_t w4, uint32_t w5,
+ uint32_t *count)
+{
+ uint32_t ret = FFA_RET_DENIED;
+ struct domain *d = current->domain;
+ struct ffa_ctx *ctx = d->arch.ffa;
+
+ if ( !ffa_page_count )
+ return FFA_RET_DENIED;
+
+ spin_lock(&ctx->lock);
+ if ( !ctx->page_count || !ctx->tx_is_mine )
+ goto out;
+ ret = ffa_partition_info_get(w1, w2, w3, w4, w5, count);
+ if ( ret )
+ goto out;
+ 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;
+
+ for ( n = 0; n < *count; n++ )
Who is going to sanitize 'count' and how do you make sure that...
+ {
+ dst[n].id = src[n].id;
... this will still be written within the page provided by the domain?
+ 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);
+
+ memcpy(ctx->rx, ffa_rx, sz);
+ }
+ ffa_rx_release();
I saw above that you are expecting the ffa_rx to be "locked". Will it be
the firmware to block another thread that may need ffa_rx?
+ ctx->tx_is_mine = false;
+out:
+ spin_unlock(&ctx->lock);
+
+ return ret;
+}
+
+static uint32_t handle_rx_release(void)
+{
+ uint32_t ret = FFA_RET_DENIED;
+ struct domain *d = current->domain;
+ struct ffa_ctx *ctx = d->arch.ffa;
+
+ spin_lock(&ctx->lock);
+ if ( !ctx->page_count || ctx->tx_is_mine )
+ goto out;
+ ret = FFA_RET_OK;
+ ctx->tx_is_mine = 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, };
+ struct arm_smccc_1_2_regs resp = { };
+ struct domain *d = current->domain;
+ struct ffa_ctx *ctx = d->arch.ffa;
+ uint32_t src_dst;
+ uint64_t mask;
+
+ if ( smccc_is_conv_64(fid) )
+ mask = 0xffffffffffffffff;
+ else
+ mask = 0xffffffff;
Please use GENMASK() (or similar). So it is easier to confirm the number
of 'f' is correct.
+
+ 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;
+
+ while ( true )
+ {
+ arm_smccc_1_2_smc(&arg, &resp);
+
+ switch ( resp.a0 )
+ {
+ case FFA_INTERRUPT:
+ ctx->interrupted = true;
+ goto out;
+ 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:
+ goto out;
+ default:
+ /* Bad fid, report back. */
+ memset(&arg, 0, sizeof(arg));
+ arg.a0 = FFA_ERROR;
+ arg.a1 = src_dst;
+ arg.a2 = FFA_RET_NOT_SUPPORTED;
+ continue;
+ }
+ }
+
+out:
+ set_user_reg(regs, 0, resp.a0);
+ set_user_reg(regs, 1, resp.a1 & mask);
+ set_user_reg(regs, 2, resp.a2 & mask);
+ set_user_reg(regs, 3, resp.a3 & mask);
+ set_user_reg(regs, 4, resp.a4 & mask);
+ set_user_reg(regs, 5, resp.a5 & mask);
+ set_user_reg(regs, 6, resp.a6 & mask);
+ set_user_reg(regs, 7, resp.a7 & mask);
You have already an helper to set all the registers. Why not using it?
+}
I will continue the rest of the review at a later point.
Cheers,
--
Julien Grall
