Hello,
This series contains some small fixes for the host pKVM proxy code. I included
some of the patches that I already sent on the list as part of this series
to make it easier to keep track of them.
I verified the functionality with OPTEE as a TEE-OS.
Changelog:
* previously posted FFA_PARTITION_INFO_GET patch here:
https://lore.kernel.org/kvmarm/[email protected]/
-> minor changes from the previous version, look for the current
ffa_version in the host_buffer structure
* previously posted "Fix the identification range for the FF-A smcs" here:
https://lore.kernel.org/kvmarm/[email protected]/
Thank you,
Sebastian
Sebastian Ene (4):
KVM: arm64: Trap FFA_VERSION host call in pKVM
KVM: arm64: Add support for FFA_PARTITION_INFO_GET
KVM: arm64: Fix the identification range for the FF-A smcs
KVM: arm64: Use FF-A 1.1 with pKVM
arch/arm64/kvm/hyp/include/nvhe/ffa.h | 2 +-
arch/arm64/kvm/hyp/nvhe/ffa.c | 95 +++++++++++++++++++++++++--
2 files changed, 90 insertions(+), 7 deletions(-)
--
2.44.0.769.g3c40516874-goog
The pKVM hypervisor initializes with FF-A version 1.0. Keep the
supported version inside the host structure and prevent the host
drivers from overwriting the FF-A version with an increased version.
Without trapping the call, the host drivers can negotiate a higher
version number with TEE which can result in a different memory layout
described during the memory sharing calls.
Signed-off-by: Sebastian Ene <[email protected]>
---
arch/arm64/kvm/hyp/nvhe/ffa.c | 43 ++++++++++++++++++++++++++++++++---
1 file changed, 40 insertions(+), 3 deletions(-)
diff --git a/arch/arm64/kvm/hyp/nvhe/ffa.c b/arch/arm64/kvm/hyp/nvhe/ffa.c
index 320f2eaa14a9..023712e8beeb 100644
--- a/arch/arm64/kvm/hyp/nvhe/ffa.c
+++ b/arch/arm64/kvm/hyp/nvhe/ffa.c
@@ -58,6 +58,7 @@ struct kvm_ffa_buffers {
hyp_spinlock_t lock;
void *tx;
void *rx;
+ u32 ffa_version;
};
/*
@@ -640,6 +641,39 @@ static bool do_ffa_features(struct arm_smccc_res *res,
return true;
}
+static void do_ffa_version(struct arm_smccc_res *res,
+ struct kvm_cpu_context *ctxt)
+{
+ DECLARE_REG(u32, ffa_req_version, ctxt, 1);
+ u32 current_version;
+
+ hyp_spin_lock(&host_buffers.lock);
+ current_version = host_buffers.ffa_version;
+ if (FFA_MAJOR_VERSION(ffa_req_version) != FFA_MAJOR_VERSION(current_version)) {
+ res->a0 = FFA_RET_NOT_SUPPORTED;
+ goto unlock;
+ }
+
+ /*
+ * If the client driver tries to downgrade the version, we need to ask
+ * first if TEE supports it.
+ */
+ if (FFA_MINOR_VERSION(ffa_req_version) < FFA_MINOR_VERSION(current_version)) {
+ arm_smccc_1_1_smc(FFA_VERSION, ffa_req_version, 0,
+ 0, 0, 0, 0, 0,
+ res);
+ if (res->a0 == FFA_RET_NOT_SUPPORTED)
+ goto unlock;
+
+ host_buffers.ffa_version = ffa_req_version;
+ goto unlock;
+ }
+
+ res->a0 = current_version;
+unlock:
+ hyp_spin_unlock(&host_buffers.lock);
+}
+
bool kvm_host_ffa_handler(struct kvm_cpu_context *host_ctxt, u32 func_id)
{
struct arm_smccc_res res;
@@ -686,6 +720,9 @@ bool kvm_host_ffa_handler(struct kvm_cpu_context *host_ctxt, u32 func_id)
case FFA_MEM_FRAG_TX:
do_ffa_mem_frag_tx(&res, host_ctxt);
goto out_handled;
+ case FFA_VERSION:
+ do_ffa_version(&res, host_ctxt);
+ goto out_handled;
}
if (ffa_call_supported(func_id))
@@ -726,6 +763,8 @@ int hyp_ffa_init(void *pages)
if (FFA_MAJOR_VERSION(res.a0) != 1)
return -EOPNOTSUPP;
+ host_buffers.ffa_version = res.a0;
+
arm_smccc_1_1_smc(FFA_ID_GET, 0, 0, 0, 0, 0, 0, 0, &res);
if (res.a0 != FFA_SUCCESS)
return -EOPNOTSUPP;
@@ -772,9 +811,7 @@ int hyp_ffa_init(void *pages)
.rx = rx,
};
- host_buffers = (struct kvm_ffa_buffers) {
- .lock = __HYP_SPIN_LOCK_UNLOCKED,
- };
+ host_buffers.lock = __HYP_SPIN_LOCK_UNLOCKED;
return 0;
}
--
2.44.0.769.g3c40516874-goog
Handle the FFA_PARTITION_INFO_GET host call inside the pKVM hypervisor
and copy the response message back to the host buffers. Save the
returned FF-A version as we will need it later to interpret the response
from the TEE.
Signed-off-by: Sebastian Ene <[email protected]>
---
arch/arm64/kvm/hyp/nvhe/ffa.c | 46 +++++++++++++++++++++++++++++++++++
1 file changed, 46 insertions(+)
diff --git a/arch/arm64/kvm/hyp/nvhe/ffa.c b/arch/arm64/kvm/hyp/nvhe/ffa.c
index 023712e8beeb..d53f50c73acb 100644
--- a/arch/arm64/kvm/hyp/nvhe/ffa.c
+++ b/arch/arm64/kvm/hyp/nvhe/ffa.c
@@ -674,6 +674,49 @@ static void do_ffa_version(struct arm_smccc_res *res,
hyp_spin_unlock(&host_buffers.lock);
}
+static void do_ffa_part_get(struct arm_smccc_res *res,
+ struct kvm_cpu_context *ctxt)
+{
+ DECLARE_REG(u32, uuid0, ctxt, 1);
+ DECLARE_REG(u32, uuid1, ctxt, 2);
+ DECLARE_REG(u32, uuid2, ctxt, 3);
+ DECLARE_REG(u32, uuid3, ctxt, 4);
+ DECLARE_REG(u32, flags, ctxt, 5);
+ u32 off, count, sz, buf_sz;
+
+ hyp_spin_lock(&host_buffers.lock);
+ if (!host_buffers.rx) {
+ ffa_to_smccc_res(res, FFA_RET_INVALID_PARAMETERS);
+ goto out_unlock;
+ }
+
+ arm_smccc_1_1_smc(FFA_PARTITION_INFO_GET, uuid0, uuid1,
+ uuid2, uuid3, flags, 0, 0,
+ res);
+
+ if (res->a0 != FFA_SUCCESS)
+ goto out_unlock;
+
+ count = res->a2;
+ if (!count)
+ goto out_unlock;
+
+ if (host_buffers.ffa_version > FFA_VERSION_1_0) {
+ buf_sz = sz = res->a3;
+ if (sz > sizeof(struct ffa_partition_info))
+ buf_sz = sizeof(struct ffa_partition_info);
+ } else {
+ /* FFA_VERSION_1_0 lacks the size in the response */
+ buf_sz = sz = 8;
+ }
+
+ WARN_ON((count - 1) * sz + buf_sz > PAGE_SIZE);
+ for (off = 0; off < count * sz; off += sz)
+ memcpy(host_buffers.rx + off, hyp_buffers.rx + off, buf_sz);
+out_unlock:
+ hyp_spin_unlock(&host_buffers.lock);
+}
+
bool kvm_host_ffa_handler(struct kvm_cpu_context *host_ctxt, u32 func_id)
{
struct arm_smccc_res res;
@@ -723,6 +766,9 @@ bool kvm_host_ffa_handler(struct kvm_cpu_context *host_ctxt, u32 func_id)
case FFA_VERSION:
do_ffa_version(&res, host_ctxt);
goto out_handled;
+ case FFA_PARTITION_INFO_GET:
+ do_ffa_part_get(&res, host_ctxt);
+ goto out_handled;
}
if (ffa_call_supported(func_id))
--
2.44.0.769.g3c40516874-goog
Now that the layout of the structures is compatible with 1.1 it is time
to probe the 1.1 version of the FF-A protocol inside the hypervisor. If
the TEE doesn't support it, it should return the minimum supported
version.
Signed-off-by: Sebastian Ene <[email protected]>
---
arch/arm64/kvm/hyp/nvhe/ffa.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/arm64/kvm/hyp/nvhe/ffa.c b/arch/arm64/kvm/hyp/nvhe/ffa.c
index d53f50c73acb..8633c8e0b09b 100644
--- a/arch/arm64/kvm/hyp/nvhe/ffa.c
+++ b/arch/arm64/kvm/hyp/nvhe/ffa.c
@@ -455,7 +455,7 @@ static __always_inline void do_ffa_mem_xfer(const u64 func_id,
memcpy(buf, host_buffers.tx, fraglen);
ep_mem_access = (void *)buf +
- ffa_mem_desc_offset(buf, 0, FFA_VERSION_1_0);
+ ffa_mem_desc_offset(buf, 0, host_buffers.ffa_version);
offset = ep_mem_access->composite_off;
if (!offset || buf->ep_count != 1 || buf->sender_id != HOST_FFA_ID) {
ret = FFA_RET_INVALID_PARAMETERS;
@@ -534,7 +534,7 @@ static void do_ffa_mem_reclaim(struct arm_smccc_res *res,
fraglen = res->a2;
ep_mem_access = (void *)buf +
- ffa_mem_desc_offset(buf, 0, FFA_VERSION_1_0);
+ ffa_mem_desc_offset(buf, 0, host_buffers.ffa_version);
offset = ep_mem_access->composite_off;
/*
* We can trust the SPMD to get this right, but let's at least
@@ -789,7 +789,7 @@ int hyp_ffa_init(void *pages)
if (kvm_host_psci_config.smccc_version < ARM_SMCCC_VERSION_1_2)
return 0;
- arm_smccc_1_1_smc(FFA_VERSION, FFA_VERSION_1_0, 0, 0, 0, 0, 0, 0, &res);
+ arm_smccc_1_1_smc(FFA_VERSION, FFA_VERSION_1_1, 0, 0, 0, 0, 0, 0, &res);
if (res.a0 == FFA_RET_NOT_SUPPORTED)
return 0;
--
2.44.0.769.g3c40516874-goog
The FF-A spec 1.2 reserves the following ranges for identifying FF-A
calls:
0x84000060-0x840000FF: FF-A 32-bit calls
0xC4000060-0xC40000FF: FF-A 64-bit calls.
Use the range identification according to the spec and allow calls that
are currently out of the range(eg. FFA_MSG_SEND_DIRECT_REQ2) to be
identified correctly.
Signed-off-by: Sebastian Ene <[email protected]>
---
arch/arm64/kvm/hyp/include/nvhe/ffa.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm64/kvm/hyp/include/nvhe/ffa.h b/arch/arm64/kvm/hyp/include/nvhe/ffa.h
index d9fd5e6c7d3c..146e0aebfa1c 100644
--- a/arch/arm64/kvm/hyp/include/nvhe/ffa.h
+++ b/arch/arm64/kvm/hyp/include/nvhe/ffa.h
@@ -9,7 +9,7 @@
#include <asm/kvm_host.h>
#define FFA_MIN_FUNC_NUM 0x60
-#define FFA_MAX_FUNC_NUM 0x7F
+#define FFA_MAX_FUNC_NUM 0xFF
int hyp_ffa_init(void *pages);
bool kvm_host_ffa_handler(struct kvm_cpu_context *host_ctxt, u32 func_id);
--
2.44.0.769.g3c40516874-goog
Hi Seb,
On Thu, Apr 18, 2024 at 04:30:23PM +0000, Sebastian Ene wrote:
> The pKVM hypervisor initializes with FF-A version 1.0. Keep the
> supported version inside the host structure and prevent the host
> drivers from overwriting the FF-A version with an increased version.
> Without trapping the call, the host drivers can negotiate a higher
> version number with TEE which can result in a different memory layout
> described during the memory sharing calls.
>
> Signed-off-by: Sebastian Ene <[email protected]>
> ---
> arch/arm64/kvm/hyp/nvhe/ffa.c | 43 ++++++++++++++++++++++++++++++++---
> 1 file changed, 40 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm64/kvm/hyp/nvhe/ffa.c b/arch/arm64/kvm/hyp/nvhe/ffa.c
> index 320f2eaa14a9..023712e8beeb 100644
> --- a/arch/arm64/kvm/hyp/nvhe/ffa.c
> +++ b/arch/arm64/kvm/hyp/nvhe/ffa.c
> @@ -58,6 +58,7 @@ struct kvm_ffa_buffers {
> hyp_spinlock_t lock;
> void *tx;
> void *rx;
> + u32 ffa_version;
> };
Why should this be part of 'struct kvm_ffa_buffers'? The host, proxy and
Secure side will end up using the same version, so a simple global
variable would suffice, no?
> /*
> @@ -640,6 +641,39 @@ static bool do_ffa_features(struct arm_smccc_res *res,
> return true;
> }
>
> +static void do_ffa_version(struct arm_smccc_res *res,
> + struct kvm_cpu_context *ctxt)
> +{
> + DECLARE_REG(u32, ffa_req_version, ctxt, 1);
> + u32 current_version;
> +
> + hyp_spin_lock(&host_buffers.lock);
Why do you need to take the lock for this?
> + current_version = host_buffers.ffa_version;
> + if (FFA_MAJOR_VERSION(ffa_req_version) != FFA_MAJOR_VERSION(current_version)) {
> + res->a0 = FFA_RET_NOT_SUPPORTED;
> + goto unlock;
> + }
We won't have probed the proxy if the Secure side doesn't support 1.x
so I think you should just do:
if (FFA_MAJOR_VERSION(ffa_req_version) != 1)
...
> + /*
> + * If the client driver tries to downgrade the version, we need to ask
> + * first if TEE supports it.
> + */
> + if (FFA_MINOR_VERSION(ffa_req_version) < FFA_MINOR_VERSION(current_version)) {
Similarly here, I don't think 'current_version' is what we should expose.
Rather, we should be returning the highest version that the proxy
supports in the host, which is 1.0 at this point in the patch series.
> + arm_smccc_1_1_smc(FFA_VERSION, ffa_req_version, 0,
> + 0, 0, 0, 0, 0,
> + res);
Hmm, I'm struggling to see how this is supposed to work per the spec.
The FF-A spec says:
| ... negotiation of the version must happen before an invocation of
| any other FF-A ABI.
and:
| Once the caller invokes any FF-A ABI other than FFA_VERSION, the
| version negotiation phase is complete.
|
| Once an FF-A version has been negotiated between a caller and a
| callee, the version may not be changed for the lifetime of the
| calling component. The callee must treat the negotiated version as
| the only supported version for any subsequent interactions with the
| caller.
So by the time we get here, we've already settled on our version with
the Secure side and the host cannot downgrade.
That's a bit rubbish if you ask me, but I think it means we'll have to
defer some of the proxy initialisation until the host calls FFA_VERSION,
at which point we'll need to negotiate a common version between the host,
the proxy and Secure. Once we've done that, our FFA_VERSION handler will
just return that negotiated version.
Will
On Thu, Apr 18, 2024 at 04:30:24PM +0000, Sebastian Ene wrote:
> Handle the FFA_PARTITION_INFO_GET host call inside the pKVM hypervisor
> and copy the response message back to the host buffers. Save the
> returned FF-A version as we will need it later to interpret the response
> from the TEE.
>
> Signed-off-by: Sebastian Ene <[email protected]>
> ---
> arch/arm64/kvm/hyp/nvhe/ffa.c | 46 +++++++++++++++++++++++++++++++++++
> 1 file changed, 46 insertions(+)
>
> diff --git a/arch/arm64/kvm/hyp/nvhe/ffa.c b/arch/arm64/kvm/hyp/nvhe/ffa.c
> index 023712e8beeb..d53f50c73acb 100644
> --- a/arch/arm64/kvm/hyp/nvhe/ffa.c
> +++ b/arch/arm64/kvm/hyp/nvhe/ffa.c
> @@ -674,6 +674,49 @@ static void do_ffa_version(struct arm_smccc_res *res,
> hyp_spin_unlock(&host_buffers.lock);
> }
>
> +static void do_ffa_part_get(struct arm_smccc_res *res,
> + struct kvm_cpu_context *ctxt)
> +{
> + DECLARE_REG(u32, uuid0, ctxt, 1);
> + DECLARE_REG(u32, uuid1, ctxt, 2);
> + DECLARE_REG(u32, uuid2, ctxt, 3);
> + DECLARE_REG(u32, uuid3, ctxt, 4);
> + DECLARE_REG(u32, flags, ctxt, 5);
> + u32 off, count, sz, buf_sz;
> +
> + hyp_spin_lock(&host_buffers.lock);
> + if (!host_buffers.rx) {
> + ffa_to_smccc_res(res, FFA_RET_INVALID_PARAMETERS);
This should be FFA_RET_BUSY per the spec.
> + goto out_unlock;
> + }
> +
> + arm_smccc_1_1_smc(FFA_PARTITION_INFO_GET, uuid0, uuid1,
> + uuid2, uuid3, flags, 0, 0,
> + res);
> +
> + if (res->a0 != FFA_SUCCESS)
> + goto out_unlock;
> +
> + count = res->a2;
> + if (!count)
> + goto out_unlock;
> +
> + if (host_buffers.ffa_version > FFA_VERSION_1_0) {
The spec says that the size field is populated based on the flags
parameter. Why aren't you checking that instead of the version number?
> + buf_sz = sz = res->a3;
> + if (sz > sizeof(struct ffa_partition_info))
> + buf_sz = sizeof(struct ffa_partition_info);
I don't think this is right, as if the payload really is bigger than
'struct ffa_partition_info' we'll truncate the data (and you don't
adjust res->a3 afaict).
Can't we just copy the whole thing back to the host? We're not
interpreting the thing, so we can just treat it like a stream of bytes.
> + } else {
> + /* FFA_VERSION_1_0 lacks the size in the response */
> + buf_sz = sz = 8;
Can you define that as a constant in arm_ffa.h, please? It's the size of
a 1.0 partition info structure.
> + }
> +
> + WARN_ON((count - 1) * sz + buf_sz > PAGE_SIZE);
We should bounds-check against 'KVM_FFA_MBOX_NR_PAGES * PAGE_SIZE' and
return an error (FFA_RET_ABORTED) if the size is over that.
> + for (off = 0; off < count * sz; off += sz)
> +
> + memcpy(host_buffers.rx + off, hyp_buffers.rx + off, buf_sz);
I think this is wrong if bit 0 of 'flags' is set to 1. In that case, I
think you just get back the number of partitions and that's it, so we
shouldn't be going near the mailboxes.
Will
On Thu, Apr 18, 2024 at 04:30:25PM +0000, Sebastian Ene wrote:
> The FF-A spec 1.2 reserves the following ranges for identifying FF-A
> calls:
> 0x84000060-0x840000FF: FF-A 32-bit calls
> 0xC4000060-0xC40000FF: FF-A 64-bit calls.
>
> Use the range identification according to the spec and allow calls that
> are currently out of the range(eg. FFA_MSG_SEND_DIRECT_REQ2) to be
> identified correctly.
>
> Signed-off-by: Sebastian Ene <[email protected]>
> ---
> arch/arm64/kvm/hyp/include/nvhe/ffa.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm64/kvm/hyp/include/nvhe/ffa.h b/arch/arm64/kvm/hyp/include/nvhe/ffa.h
> index d9fd5e6c7d3c..146e0aebfa1c 100644
> --- a/arch/arm64/kvm/hyp/include/nvhe/ffa.h
> +++ b/arch/arm64/kvm/hyp/include/nvhe/ffa.h
> @@ -9,7 +9,7 @@
> #include <asm/kvm_host.h>
>
> #define FFA_MIN_FUNC_NUM 0x60
> -#define FFA_MAX_FUNC_NUM 0x7F
> +#define FFA_MAX_FUNC_NUM 0xFF
>
> int hyp_ffa_init(void *pages);
> bool kvm_host_ffa_handler(struct kvm_cpu_context *host_ctxt, u32 func_id);
> --
> 2.44.0.769.g3c40516874-goog
Acked-by: Will Deacon <[email protected]>
Will
On Fri, May 03, 2024 at 03:39:38PM +0100, Will Deacon wrote:
Hello Will,
> Hi Seb,
>
> On Thu, Apr 18, 2024 at 04:30:23PM +0000, Sebastian Ene wrote:
> > The pKVM hypervisor initializes with FF-A version 1.0. Keep the
> > supported version inside the host structure and prevent the host
> > drivers from overwriting the FF-A version with an increased version.
> > Without trapping the call, the host drivers can negotiate a higher
> > version number with TEE which can result in a different memory layout
> > described during the memory sharing calls.
> >
> > Signed-off-by: Sebastian Ene <[email protected]>
> > ---
> > arch/arm64/kvm/hyp/nvhe/ffa.c | 43 ++++++++++++++++++++++++++++++++---
> > 1 file changed, 40 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/arm64/kvm/hyp/nvhe/ffa.c b/arch/arm64/kvm/hyp/nvhe/ffa.c
> > index 320f2eaa14a9..023712e8beeb 100644
> > --- a/arch/arm64/kvm/hyp/nvhe/ffa.c
> > +++ b/arch/arm64/kvm/hyp/nvhe/ffa.c
> > @@ -58,6 +58,7 @@ struct kvm_ffa_buffers {
> > hyp_spinlock_t lock;
> > void *tx;
> > void *rx;
> > + u32 ffa_version;
> > };
>
> Why should this be part of 'struct kvm_ffa_buffers'? The host, proxy and
> Secure side will end up using the same version, so a simple global
> variable would suffice, no?
>
I prefer keeping it here as we will have more clients in the future /
different VMs and each one of them will have its own version and its own
pair of buffers.
> > /*
> > @@ -640,6 +641,39 @@ static bool do_ffa_features(struct arm_smccc_res *res,
> > return true;
> > }
> >
> > +static void do_ffa_version(struct arm_smccc_res *res,
> > + struct kvm_cpu_context *ctxt)
> > +{
> > + DECLARE_REG(u32, ffa_req_version, ctxt, 1);
> > + u32 current_version;
> > +
> > + hyp_spin_lock(&host_buffers.lock);
>
> Why do you need to take the lock for this?
>
Because we interpret the host buffer content based on the version that we
end up setting here and each time we are accessing these buffers we are
protected by this lock.
> > + current_version = host_buffers.ffa_version;
> > + if (FFA_MAJOR_VERSION(ffa_req_version) != FFA_MAJOR_VERSION(current_version)) {
> > + res->a0 = FFA_RET_NOT_SUPPORTED;
> > + goto unlock;
> > + }
>
> We won't have probed the proxy if the Secure side doesn't support 1.x
> so I think you should just do:
>
> if (FFA_MAJOR_VERSION(ffa_req_version) != 1)
> ...
>
Ack.
> > + /*
> > + * If the client driver tries to downgrade the version, we need to ask
> > + * first if TEE supports it.
> > + */
> > + if (FFA_MINOR_VERSION(ffa_req_version) < FFA_MINOR_VERSION(current_version)) {
>
> Similarly here, I don't think 'current_version' is what we should expose.
> Rather, we should be returning the highest version that the proxy
> supports in the host, which is 1.0 at this point in the patch series.
We already report the highest version that the proxy supports on line:
`res->a0 = current_version;`
'current_version' is assigned during proxy initialization.
This check allows us to downgrade the supported ffa_version if the Host
requested it and only if TF-A supports it.
>
> > + arm_smccc_1_1_smc(FFA_VERSION, ffa_req_version, 0,
> > + 0, 0, 0, 0, 0,
> > + res);
>
> Hmm, I'm struggling to see how this is supposed to work per the spec.
> The FF-A spec says:
>
> | ... negotiation of the version must happen before an invocation of
> | any other FF-A ABI.
I think that is a bit vague in my opinion but what I get is that the first call
executed should always be the get version ff-a call.
>
> and:
>
> | Once the caller invokes any FF-A ABI other than FFA_VERSION, the
> | version negotiation phase is complete.
> |
> | Once an FF-A version has been negotiated between a caller and a
> | callee, the version may not be changed for the lifetime of the
> | calling component. The callee must treat the negotiated version as
> | the only supported version for any subsequent interactions with the
> | caller.>
> So by the time we get here, we've already settled on our version with
> the Secure side and the host cannot downgrade.
At this stage I think the spec didn't take into account that there can be a hypervisor
in between.
>
> That's a bit rubbish if you ask me, but I think it means we'll have to
> defer some of the proxy initialisation until the host calls FFA_VERSION,
> at which point we'll need to negotiate a common version between the host,
> the proxy and Secure. Once we've done that, our FFA_VERSION handler will
> just return that negotiated version.
We are already doing this when the ARM driver is built as an external
module. If it is not as an external module and is builtin we have a
bigger issue because it loads before pKVM at subsys_initcall. This means
that we won't trap FFA_MAP* and other setup calls.
>
> Will
>
Thank you,
Seb
> To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
>
On Fri, May 03, 2024 at 03:29:12PM +0000, Sebastian Ene wrote:
> On Fri, May 03, 2024 at 03:39:38PM +0100, Will Deacon wrote:
> > On Thu, Apr 18, 2024 at 04:30:23PM +0000, Sebastian Ene wrote:
> > > The pKVM hypervisor initializes with FF-A version 1.0. Keep the
> > > supported version inside the host structure and prevent the host
> > > drivers from overwriting the FF-A version with an increased version.
> > > Without trapping the call, the host drivers can negotiate a higher
> > > version number with TEE which can result in a different memory layout
> > > described during the memory sharing calls.
> > >
> > > Signed-off-by: Sebastian Ene <[email protected]>
> > > ---
> > > arch/arm64/kvm/hyp/nvhe/ffa.c | 43 ++++++++++++++++++++++++++++++++---
> > > 1 file changed, 40 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/arch/arm64/kvm/hyp/nvhe/ffa.c b/arch/arm64/kvm/hyp/nvhe/ffa.c
> > > index 320f2eaa14a9..023712e8beeb 100644
> > > --- a/arch/arm64/kvm/hyp/nvhe/ffa.c
> > > +++ b/arch/arm64/kvm/hyp/nvhe/ffa.c
> > > @@ -58,6 +58,7 @@ struct kvm_ffa_buffers {
> > > hyp_spinlock_t lock;
> > > void *tx;
> > > void *rx;
> > > + u32 ffa_version;
> > > };
> >
> > Why should this be part of 'struct kvm_ffa_buffers'? The host, proxy and
> > Secure side will end up using the same version, so a simple global
> > variable would suffice, no?
> >
> I prefer keeping it here as we will have more clients in the future /
> different VMs and each one of them will have its own version and its own
> pair of buffers.
We can add that when we need to. Let's keep it simple for now, as the
idea of the proxy having to support multiple versions of the spec at
once sounds terrifying to me. I don't think we're going to want to
re-marshall the data structures between the 1.0 and 1.1 formats, are we?
> > > @@ -640,6 +641,39 @@ static bool do_ffa_features(struct arm_smccc_res *res,
> > > return true;
> > > }
> > >
> > > +static void do_ffa_version(struct arm_smccc_res *res,
> > > + struct kvm_cpu_context *ctxt)
> > > +{
> > > + DECLARE_REG(u32, ffa_req_version, ctxt, 1);
> > > + u32 current_version;
> > > +
> > > + hyp_spin_lock(&host_buffers.lock);
> >
> > Why do you need to take the lock for this?
> >
>
> Because we interpret the host buffer content based on the version that we
> end up setting here and each time we are accessing these buffers we are
> protected by this lock.
I think that's indicative of a broader issue, though, which is that you
allow for the version to be re-negotiated at runtime. The spec doesn't
allow that and I don't think we should either.
> > > + /*
> > > + * If the client driver tries to downgrade the version, we need to ask
> > > + * first if TEE supports it.
> > > + */
> > > + if (FFA_MINOR_VERSION(ffa_req_version) < FFA_MINOR_VERSION(current_version)) {
> >
> > Similarly here, I don't think 'current_version' is what we should expose.
> > Rather, we should be returning the highest version that the proxy
> > supports in the host, which is 1.0 at this point in the patch series.
>
> We already report the highest version that the proxy supports on line:
> `res->a0 = current_version;`
>
> 'current_version' is assigned during proxy initialization.
> This check allows us to downgrade the supported ffa_version if the Host
> requested it and only if TF-A supports it.
I don't think we want the host negotiating directly with the Secure side,
though, do we? 'current_version' is initialised to whatever the Secure
side gives us, so if we had something like:
1. Proxy initialises, issues FFA_VERSION(1.0)
2. Secure implements 1.2 and so returns 1.2 but remains in 1.0
compatability mode for the data structure formats.
3. The host issues FFA_VERSION(1.1)
4. The code above then issues FFA_VERSION(1.1) to Secure and it
switches over to the 1.1 data structures
Did I get that right?
I really think we need to settle on a single version for the host,
hypervisor and Secure and then stick with it following a single
negotiation stage.
> > > + arm_smccc_1_1_smc(FFA_VERSION, ffa_req_version, 0,
> > > + 0, 0, 0, 0, 0,
> > > + res);
> >
> > Hmm, I'm struggling to see how this is supposed to work per the spec.
> > The FF-A spec says:
> >
> > | ... negotiation of the version must happen before an invocation of
> > | any other FF-A ABI.
>
> I think that is a bit vague in my opinion but what I get is that the first call
> executed should always be the get version ff-a call.
>
> >
> > and:
> >
> > | Once the caller invokes any FF-A ABI other than FFA_VERSION, the
> > | version negotiation phase is complete.
> > |
> > | Once an FF-A version has been negotiated between a caller and a
> > | callee, the version may not be changed for the lifetime of the
> > | calling component. The callee must treat the negotiated version as
> > | the only supported version for any subsequent interactions with the
> > | caller.>
> > So by the time we get here, we've already settled on our version with
> > the Secure side and the host cannot downgrade.
>
> At this stage I think the spec didn't take into account that there can be a hypervisor
> in between.
Well, that's what the spec says and I think we need to follow it. I can
well imagine that the Secure side won't allow the version to be
re-negotiated on the fly and I don't think we'd want that in the proxy,
either.
> > That's a bit rubbish if you ask me, but I think it means we'll have to
> > defer some of the proxy initialisation until the host calls FFA_VERSION,
> > at which point we'll need to negotiate a common version between the host,
> > the proxy and Secure. Once we've done that, our FFA_VERSION handler will
> > just return that negotiated version.
>
> We are already doing this when the ARM driver is built as an external
> module. If it is not as an external module and is builtin we have a
> bigger issue because it loads before pKVM at subsys_initcall. This means
> that we won't trap FFA_MAP* and other setup calls.
Sorry, I don't follow. hyp_ffa_init() issues FFA_FEATURES immediately
after FFA_VERSION, so we terminate the negotiation right away.
Will
On Fri, May 03, 2024 at 05:21:14PM +0100, Will Deacon wrote:
> On Fri, May 03, 2024 at 03:29:12PM +0000, Sebastian Ene wrote:
> > On Fri, May 03, 2024 at 03:39:38PM +0100, Will Deacon wrote:
> > > On Thu, Apr 18, 2024 at 04:30:23PM +0000, Sebastian Ene wrote:
> > > > The pKVM hypervisor initializes with FF-A version 1.0. Keep the
> > > > supported version inside the host structure and prevent the host
> > > > drivers from overwriting the FF-A version with an increased version.
> > > > Without trapping the call, the host drivers can negotiate a higher
> > > > version number with TEE which can result in a different memory layout
> > > > described during the memory sharing calls.
> > > >
> > > > Signed-off-by: Sebastian Ene <[email protected]>
> > > > ---
> > > > arch/arm64/kvm/hyp/nvhe/ffa.c | 43 ++++++++++++++++++++++++++++++++---
> > > > 1 file changed, 40 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/arch/arm64/kvm/hyp/nvhe/ffa.c b/arch/arm64/kvm/hyp/nvhe/ffa.c
> > > > index 320f2eaa14a9..023712e8beeb 100644
> > > > --- a/arch/arm64/kvm/hyp/nvhe/ffa.c
> > > > +++ b/arch/arm64/kvm/hyp/nvhe/ffa.c
> > > > @@ -58,6 +58,7 @@ struct kvm_ffa_buffers {
> > > > hyp_spinlock_t lock;
> > > > void *tx;
> > > > void *rx;
> > > > + u32 ffa_version;
> > > > };
> > >
> > > Why should this be part of 'struct kvm_ffa_buffers'? The host, proxy and
> > > Secure side will end up using the same version, so a simple global
> > > variable would suffice, no?
> > >
> > I prefer keeping it here as we will have more clients in the future /
> > different VMs and each one of them will have its own version and its own
> > pair of buffers.
>
> We can add that when we need to. Let's keep it simple for now, as the
> idea of the proxy having to support multiple versions of the spec at
> once sounds terrifying to me. I don't think we're going to want to
> re-marshall the data structures between the 1.0 and 1.1 formats, are we?
>
I don't think we increase the complexity of the code by keeping this
argument in the structure. The code in nvhe/ffa.c supports marshalling
the structure as of [this
change](https://lore.kernel.org/r/[email protected]
) and that is why I was in favor of keeping the version where it belongs
to.
> > > > @@ -640,6 +641,39 @@ static bool do_ffa_features(struct arm_smccc_res *res,
> > > > return true;
> > > > }
> > > >
> > > > +static void do_ffa_version(struct arm_smccc_res *res,
> > > > + struct kvm_cpu_context *ctxt)
> > > > +{
> > > > + DECLARE_REG(u32, ffa_req_version, ctxt, 1);
> > > > + u32 current_version;
> > > > +
> > > > + hyp_spin_lock(&host_buffers.lock);
> > >
> > > Why do you need to take the lock for this?
> > >
> >
> > Because we interpret the host buffer content based on the version that we
> > end up setting here and each time we are accessing these buffers we are
> > protected by this lock.
>
> I think that's indicative of a broader issue, though, which is that you
> allow for the version to be re-negotiated at runtime. The spec doesn't
> allow that and I don't think we should either.
>
The spec talks about interopeartion in case two versions (x,y) and (a,b)
want to talk:
- given the pairs (x,y) and (a,b) x=major, y=minor if x == a and y > b
the versions are incompatible until y downgrades its version such that
y <= b.
From this I drew the conclusion that the spec allows the re-negotiation
at runtime, please let me know if you see things differently.
> > > > + /*
> > > > + * If the client driver tries to downgrade the version, we need to ask
> > > > + * first if TEE supports it.
> > > > + */
> > > > + if (FFA_MINOR_VERSION(ffa_req_version) < FFA_MINOR_VERSION(current_version)) {
> > >
> > > Similarly here, I don't think 'current_version' is what we should expose.
> > > Rather, we should be returning the highest version that the proxy
> > > supports in the host, which is 1.0 at this point in the patch series.
> >
> > We already report the highest version that the proxy supports on line:
> > `res->a0 = current_version;`
> >
> > 'current_version' is assigned during proxy initialization.
> > This check allows us to downgrade the supported ffa_version if the Host
> > requested it and only if TF-A supports it.
>
> I don't think we want the host negotiating directly with the Secure side,
> though, do we? 'current_version' is initialised to whatever the Secure
> side gives us, so if we had something like:
>
> 1. Proxy initialises, issues FFA_VERSION(1.0)
This will save 1.0 in host_buffers.ffa_version
> 2. Secure implements 1.2 and so returns 1.2 but remains in 1.0
> compatability mode for the data structure formats.
Ack.
> 3. The host issues FFA_VERSION(1.1)
The call is trapped and we verify if the requested version
(FFA_VERSION(1.1) is smaller than our current_version saved in step 1.
Given that is not smaller we only reply with our current supported
version which is FFA_VERSION(1.0) and we return to the host.
> 4. The code above then issues FFA_VERSION(1.1) to Secure and it
> switches over to the 1.1 data structures
This was happening prior to my patch, so in a way this patch is a bugfix.
We get this behavior without trapping and interpretting
of the FFA_VERSION API.
>
> Did I get that right?
>
> I really think we need to settle on a single version for the host,
> hypervisor and Secure and then stick with it following a single
> negotiation stage.
>
> > > > + arm_smccc_1_1_smc(FFA_VERSION, ffa_req_version, 0,
> > > > + 0, 0, 0, 0, 0,
> > > > + res);
> > >
> > > Hmm, I'm struggling to see how this is supposed to work per the spec.
> > > The FF-A spec says:
> > >
> > > | ... negotiation of the version must happen before an invocation of
> > > | any other FF-A ABI.
> >
> > I think that is a bit vague in my opinion but what I get is that the first call
> > executed should always be the get version ff-a call.
> >
> > >
> > > and:
> > >
> > > | Once the caller invokes any FF-A ABI other than FFA_VERSION, the
> > > | version negotiation phase is complete.
> > > |
> > > | Once an FF-A version has been negotiated between a caller and a
> > > | callee, the version may not be changed for the lifetime of the
> > > | calling component. The callee must treat the negotiated version as
> > > | the only supported version for any subsequent interactions with the
> > > | caller.>
> > > So by the time we get here, we've already settled on our version with
> > > the Secure side and the host cannot downgrade.
> >
> > At this stage I think the spec didn't take into account that there can be a hypervisor
> > in between.
>
> Well, that's what the spec says and I think we need to follow it. I can
> well imagine that the Secure side won't allow the version to be
> re-negotiated on the fly and I don't think we'd want that in the proxy,
> either.
>
> > > That's a bit rubbish if you ask me, but I think it means we'll have to
> > > defer some of the proxy initialisation until the host calls FFA_VERSION,
> > > at which point we'll need to negotiate a common version between the host,
> > > the proxy and Secure. Once we've done that, our FFA_VERSION handler will
> > > just return that negotiated version.
> >
> > We are already doing this when the ARM driver is built as an external
> > module. If it is not as an external module and is builtin we have a
> > bigger issue because it loads before pKVM at subsys_initcall. This means
> > that we won't trap FFA_MAP* and other setup calls.
>
> Sorry, I don't follow. hyp_ffa_init() issues FFA_FEATURES immediately
> after FFA_VERSION, so we terminate the negotiation right away.
Sorry I confused you, I am afraid I was trying to desribe a different issue
here which is related to how early the ARM FF-A driver initializes when
is builtin - it is before the hypervisor proxy is installed.
>
> Will
>
> To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
>
Thank you,
Seb
On Fri, May 03, 2024 at 04:01:16PM +0100, Will Deacon wrote:
> On Thu, Apr 18, 2024 at 04:30:24PM +0000, Sebastian Ene wrote:
> > Handle the FFA_PARTITION_INFO_GET host call inside the pKVM hypervisor
> > and copy the response message back to the host buffers. Save the
> > returned FF-A version as we will need it later to interpret the response
> > from the TEE.
> >
> > Signed-off-by: Sebastian Ene <[email protected]>
> > ---
> > arch/arm64/kvm/hyp/nvhe/ffa.c | 46 +++++++++++++++++++++++++++++++++++
> > 1 file changed, 46 insertions(+)
> >
> > diff --git a/arch/arm64/kvm/hyp/nvhe/ffa.c b/arch/arm64/kvm/hyp/nvhe/ffa.c
> > index 023712e8beeb..d53f50c73acb 100644
> > --- a/arch/arm64/kvm/hyp/nvhe/ffa.c
> > +++ b/arch/arm64/kvm/hyp/nvhe/ffa.c
> > @@ -674,6 +674,49 @@ static void do_ffa_version(struct arm_smccc_res *res,
> > hyp_spin_unlock(&host_buffers.lock);
> > }
Hello Will,
> >
> > +static void do_ffa_part_get(struct arm_smccc_res *res,
> > + struct kvm_cpu_context *ctxt)
> > +{
> > + DECLARE_REG(u32, uuid0, ctxt, 1);
> > + DECLARE_REG(u32, uuid1, ctxt, 2);
> > + DECLARE_REG(u32, uuid2, ctxt, 3);
> > + DECLARE_REG(u32, uuid3, ctxt, 4);
> > + DECLARE_REG(u32, flags, ctxt, 5);
> > + u32 off, count, sz, buf_sz;
> > +
> > + hyp_spin_lock(&host_buffers.lock);
> > + if (!host_buffers.rx) {
> > + ffa_to_smccc_res(res, FFA_RET_INVALID_PARAMETERS);
>
> This should be FFA_RET_BUSY per the spec.
>
Good catch, thank you ! I am fixing this.
> > + goto out_unlock;
> > + }
> > +
> > + arm_smccc_1_1_smc(FFA_PARTITION_INFO_GET, uuid0, uuid1,
> > + uuid2, uuid3, flags, 0, 0,
> > + res);
> > +
> > + if (res->a0 != FFA_SUCCESS)
> > + goto out_unlock;
> > +
> > + count = res->a2;
> > + if (!count)
> > + goto out_unlock;
> > +
> > + if (host_buffers.ffa_version > FFA_VERSION_1_0) {
>
> The spec says that the size field is populated based on the flags
> parameter. Why aren't you checking that instead of the version number?
>
Right but the flags were introduced in the latest version (they were not
in 1.0). In 1.0 the content of w5 is reserved and this is why I need the
version check.
> > + buf_sz = sz = res->a3;
> > + if (sz > sizeof(struct ffa_partition_info))
> > + buf_sz = sizeof(struct ffa_partition_info);
>
> I don't think this is right, as if the payload really is bigger than
> 'struct ffa_partition_info' we'll truncate the data (and you don't
> adjust res->a3 afaict).
>
> Can't we just copy the whole thing back to the host? We're not
> interpreting the thing, so we can just treat it like a stream of bytes.
>
Let me rewrite this. You are right, we should treat it like a stream of
bytes.
> > + } else {
> > + /* FFA_VERSION_1_0 lacks the size in the response */
> > + buf_sz = sz = 8;
>
> Can you define that as a constant in arm_ffa.h, please? It's the size of
> a 1.0 partition info structure.
>
Yup, I am adding this.
> > + }
> > +
> > + WARN_ON((count - 1) * sz + buf_sz > PAGE_SIZE);
>
> We should bounds-check against 'KVM_FFA_MBOX_NR_PAGES * PAGE_SIZE' and
> return an error (FFA_RET_ABORTED) if the size is over that.
>
Thanks, this should work I will do this.
> > + for (off = 0; off < count * sz; off += sz)
> > +
> > + memcpy(host_buffers.rx + off, hyp_buffers.rx + off, buf_sz);
>
> I think this is wrong if bit 0 of 'flags' is set to 1. In that case, I
> think you just get back the number of partitions and that's it, so we
> shouldn't be going near the mailboxes.
>
I removed this, thanks for the review !
> Will