[PATCH v2 0/4] KVM: arm64: Separate the hyp FF-A buffers init from the host

[PATCH v2 0/4] KVM: arm64: Separate the hyp FF-A buffers init from the host

[Sebastian Ene]

Hello,

This moves the initialization of the hypervisor FF-A buffers
away from the host FF-A map calling path. If the hypervisor
cannot map the buffers with Trustzone, it rejects any FF-A call
when it runs under protected mode. 
Other than that it moves the definitions of the ffa_to_linux_err
map from the arm_ffa driver to the ffa header so that the hyp code
can make use of it.

*** Changelog ***

v1 -> this version:

Split the patch that maps the ff-a buffers of ffa init and introduce
"Move the ffa_to_linux definition to the ffa header".

Thanks,

Sebastian Ene (4):
  KVM: arm64: Use the static initializer for the vesion lock
  KVM: arm64: Move the ffa_to_linux definition to the ffa header
  KVM: arm64: Map the hypervisor FF-A buffers on ffa init
  KVM: arm64: Release the ownership of the hyp rx buffer to Trustzone

 arch/arm64/kvm/hyp/nvhe/ffa.c     | 56 +++++++++++++------------------
 drivers/firmware/arm_ffa/driver.c | 24 -------------
 include/linux/arm_ffa.h           | 24 +++++++++++++
 3 files changed, 47 insertions(+), 57 deletions(-)

-- 
2.48.1.711.g2feabab25a-goog

[PATCH v2 1/4] KVM: arm64: Use the static initializer for the vesion lock

[Sebastian Ene]

Replace the definition of the hypervisor version lock
with a static initializer.

Signed-off-by: Sebastian Ene <sebastianene@google.com>
---
 arch/arm64/kvm/hyp/nvhe/ffa.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/hyp/nvhe/ffa.c b/arch/arm64/kvm/hyp/nvhe/ffa.c
index e433dfab882a..6df6131f1107 100644
--- a/arch/arm64/kvm/hyp/nvhe/ffa.c
+++ b/arch/arm64/kvm/hyp/nvhe/ffa.c
@@ -69,7 +69,7 @@ static struct kvm_ffa_buffers hyp_buffers;
 static struct kvm_ffa_buffers host_buffers;
 static u32 hyp_ffa_version;
 static bool has_version_negotiated;
-static hyp_spinlock_t version_lock;
+static DEFINE_HYP_SPINLOCK(version_lock);
 
 static void ffa_to_smccc_error(struct arm_smccc_res *res, u64 ffa_errno)
 {
@@ -911,6 +911,5 @@ int hyp_ffa_init(void *pages)
 		.lock	= __HYP_SPIN_LOCK_UNLOCKED,
 	};
 
-	version_lock = __HYP_SPIN_LOCK_UNLOCKED;
 	return 0;
 }
-- 
2.48.1.711.g2feabab25a-goog

Re: [PATCH v2 1/4] KVM: arm64: Use the static initializer for the vesion lock

[Will Deacon]

On Thu, Feb 27, 2025 at 06:17:46PM +0000, Sebastian Ene wrote:
> Replace the definition of the hypervisor version lock
> with a static initializer.
> 
> Signed-off-by: Sebastian Ene <sebastianene@google.com>
> ---
>  arch/arm64/kvm/hyp/nvhe/ffa.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp/nvhe/ffa.c b/arch/arm64/kvm/hyp/nvhe/ffa.c
> index e433dfab882a..6df6131f1107 100644
> --- a/arch/arm64/kvm/hyp/nvhe/ffa.c
> +++ b/arch/arm64/kvm/hyp/nvhe/ffa.c
> @@ -69,7 +69,7 @@ static struct kvm_ffa_buffers hyp_buffers;
>  static struct kvm_ffa_buffers host_buffers;
>  static u32 hyp_ffa_version;
>  static bool has_version_negotiated;
> -static hyp_spinlock_t version_lock;
> +static DEFINE_HYP_SPINLOCK(version_lock);
>  
>  static void ffa_to_smccc_error(struct arm_smccc_res *res, u64 ffa_errno)
>  {
> @@ -911,6 +911,5 @@ int hyp_ffa_init(void *pages)
>  		.lock	= __HYP_SPIN_LOCK_UNLOCKED,
>  	};
>  
> -	version_lock = __HYP_SPIN_LOCK_UNLOCKED;
>  	return 0;

nit: typo in the subject (s/vesion/version/).

With that fixed:

Acked-by: Will Deacon <will@kernel.org>

Will

[PATCH v2 2/4] KVM: arm64: Move the ffa_to_linux definition to the ffa header

[Sebastian Ene]

Keep the ffa_to_linux error map in the header and move it away
from the arm ffa driver to make it accessible for other components.

Signed-off-by: Sebastian Ene <sebastianene@google.com>
---
 drivers/firmware/arm_ffa/driver.c | 24 ------------------------
 include/linux/arm_ffa.h           | 24 ++++++++++++++++++++++++
 2 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
index 2c2ec3c35f15..b02b80f3dfe8 100644
--- a/drivers/firmware/arm_ffa/driver.c
+++ b/drivers/firmware/arm_ffa/driver.c
@@ -61,30 +61,6 @@
 
 static ffa_fn *invoke_ffa_fn;
 
-static const int ffa_linux_errmap[] = {
-	/* better than switch case as long as return value is continuous */
-	0,		/* FFA_RET_SUCCESS */
-	-EOPNOTSUPP,	/* FFA_RET_NOT_SUPPORTED */
-	-EINVAL,	/* FFA_RET_INVALID_PARAMETERS */
-	-ENOMEM,	/* FFA_RET_NO_MEMORY */
-	-EBUSY,		/* FFA_RET_BUSY */
-	-EINTR,		/* FFA_RET_INTERRUPTED */
-	-EACCES,	/* FFA_RET_DENIED */
-	-EAGAIN,	/* FFA_RET_RETRY */
-	-ECANCELED,	/* FFA_RET_ABORTED */
-	-ENODATA,	/* FFA_RET_NO_DATA */
-	-EAGAIN,	/* FFA_RET_NOT_READY */
-};
-
-static inline int ffa_to_linux_errno(int errno)
-{
-	int err_idx = -errno;
-
-	if (err_idx >= 0 && err_idx < ARRAY_SIZE(ffa_linux_errmap))
-		return ffa_linux_errmap[err_idx];
-	return -EINVAL;
-}
-
 struct ffa_pcpu_irq {
 	struct ffa_drv_info *info;
 };
diff --git a/include/linux/arm_ffa.h b/include/linux/arm_ffa.h
index 74169dd0f659..850577edadbc 100644
--- a/include/linux/arm_ffa.h
+++ b/include/linux/arm_ffa.h
@@ -475,4 +475,28 @@ struct ffa_ops {
 	const struct ffa_notifier_ops *notifier_ops;
 };
 
+static const int ffa_linux_errmap[] = {
+	/* better than switch case as long as return value is continuous */
+	0,		/* FFA_RET_SUCCESS */
+	-EOPNOTSUPP,	/* FFA_RET_NOT_SUPPORTED */
+	-EINVAL,	/* FFA_RET_INVALID_PARAMETERS */
+	-ENOMEM,	/* FFA_RET_NO_MEMORY */
+	-EBUSY,		/* FFA_RET_BUSY */
+	-EINTR,		/* FFA_RET_INTERRUPTED */
+	-EACCES,	/* FFA_RET_DENIED */
+	-EAGAIN,	/* FFA_RET_RETRY */
+	-ECANCELED,	/* FFA_RET_ABORTED */
+	-ENODATA,	/* FFA_RET_NO_DATA */
+	-EAGAIN,	/* FFA_RET_NOT_READY */
+};
+
+static inline int ffa_to_linux_errno(int errno)
+{
+	int err_idx = -errno;
+
+	if (err_idx >= 0 && err_idx < ARRAY_SIZE(ffa_linux_errmap))
+		return ffa_linux_errmap[err_idx];
+	return -EINVAL;
+}
+
 #endif /* _LINUX_ARM_FFA_H */
-- 
2.48.1.711.g2feabab25a-goog

Re: [PATCH v2 2/4] KVM: arm64: Move the ffa_to_linux definition to the ffa header

[Sudeep Holla]

On Thu, Feb 27, 2025 at 06:17:47PM +0000, Sebastian Ene wrote:
> Keep the ffa_to_linux error map in the header and move it away
> from the arm ffa driver to make it accessible for other components.

Do you plan to push/target these changes for v6.15 ? If not, I can take
this patch with other FF-A changes in my tree for v6.15. Otherwise, it
is must go along with other changes.

--
Regards,
Sudeep

Re: [PATCH v2 2/4] KVM: arm64: Move the ffa_to_linux definition to the ffa header

[Sebastian Ene]

On Thu, Feb 27, 2025 at 08:25:57PM +0000, Sudeep Holla wrote:
> On Thu, Feb 27, 2025 at 06:17:47PM +0000, Sebastian Ene wrote:
> > Keep the ffa_to_linux error map in the header and move it away
> > from the arm ffa driver to make it accessible for other components.
> 
> Do you plan to push/target these changes for v6.15 ? If not, I can take
> this patch with other FF-A changes in my tree for v6.15. Otherwise, it
> is must go along with other changes.
> 

Yes, feel free to pick them with your changes and we can push them
later.

> --
> Regards,
> Sudeep

Thanks,
Sebastian

Re: [PATCH v2 2/4] KVM: arm64: Move the ffa_to_linux definition to the ffa header

[Marc Zyngier]

On Thu, 27 Feb 2025 23:12:37 +0000,
Sebastian Ene <sebastianene@google.com> wrote:
> 
> On Thu, Feb 27, 2025 at 08:25:57PM +0000, Sudeep Holla wrote:
> > On Thu, Feb 27, 2025 at 06:17:47PM +0000, Sebastian Ene wrote:
> > > Keep the ffa_to_linux error map in the header and move it away
> > > from the arm ffa driver to make it accessible for other components.
> > 
> > Do you plan to push/target these changes for v6.15 ? If not, I can take
> > this patch with other FF-A changes in my tree for v6.15. Otherwise, it
> > is must go along with other changes.
> > 
> 
> Yes, feel free to pick them with your changes and we can push them
> later.

So this series is not a 6.15 candidate?

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v2 2/4] KVM: arm64: Move the ffa_to_linux definition to the ffa header

[Will Deacon]

On Fri, Feb 28, 2025 at 10:09:19AM +0000, Marc Zyngier wrote:
> On Thu, 27 Feb 2025 23:12:37 +0000,
> Sebastian Ene <sebastianene@google.com> wrote:
> > 
> > On Thu, Feb 27, 2025 at 08:25:57PM +0000, Sudeep Holla wrote:
> > > On Thu, Feb 27, 2025 at 06:17:47PM +0000, Sebastian Ene wrote:
> > > > Keep the ffa_to_linux error map in the header and move it away
> > > > from the arm ffa driver to make it accessible for other components.
> > > 
> > > Do you plan to push/target these changes for v6.15 ? If not, I can take
> > > this patch with other FF-A changes in my tree for v6.15. Otherwise, it
> > > is must go along with other changes.
> > > 
> > 
> > Yes, feel free to pick them with your changes and we can push them
> > later.
> 
> So this series is not a 6.15 candidate?

I think this is 6.15 stuff once it's been reviewed. Sudeep's message is
a little confusing as it refers to 6.15 twice (I guess he meant 6.14 the
first time?).

Will

Re: [PATCH v2 2/4] KVM: arm64: Move the ffa_to_linux definition to the ffa header

[Sebastian Ene]

On Mon, Mar 03, 2025 at 11:44:28PM +0000, Will Deacon wrote:
> On Fri, Feb 28, 2025 at 10:09:19AM +0000, Marc Zyngier wrote:
> > On Thu, 27 Feb 2025 23:12:37 +0000,
> > Sebastian Ene <sebastianene@google.com> wrote:
> > > 
> > > On Thu, Feb 27, 2025 at 08:25:57PM +0000, Sudeep Holla wrote:
> > > > On Thu, Feb 27, 2025 at 06:17:47PM +0000, Sebastian Ene wrote:
> > > > > Keep the ffa_to_linux error map in the header and move it away
> > > > > from the arm ffa driver to make it accessible for other components.
> > > > 
> > > > Do you plan to push/target these changes for v6.15 ? If not, I can take
> > > > this patch with other FF-A changes in my tree for v6.15. Otherwise, it
> > > > is must go along with other changes.
> > > > 
> > > 
> > > Yes, feel free to pick them with your changes and we can push them
> > > later.
> > 
> > So this series is not a 6.15 candidate?
> 
> I think this is 6.15 stuff once it's been reviewed. Sudeep's message is
> a little confusing as it refers to 6.15 twice (I guess he meant 6.14 the
> first time?).
> 

Right, I think it should be for 6.15 after it ticks the review boxes.

> Will

Thanks,
Seb

Re: [PATCH v2 2/4] KVM: arm64: Move the ffa_to_linux definition to the ffa header

[Sudeep Holla]

On Mon, Mar 03, 2025 at 11:44:28PM +0000, Will Deacon wrote:
> On Fri, Feb 28, 2025 at 10:09:19AM +0000, Marc Zyngier wrote:
> > On Thu, 27 Feb 2025 23:12:37 +0000,
> > Sebastian Ene <sebastianene@google.com> wrote:
> > >
> > > On Thu, Feb 27, 2025 at 08:25:57PM +0000, Sudeep Holla wrote:
> > > > On Thu, Feb 27, 2025 at 06:17:47PM +0000, Sebastian Ene wrote:
> > > > > Keep the ffa_to_linux error map in the header and move it away
> > > > > from the arm ffa driver to make it accessible for other components.
> > > >
> > > > Do you plan to push/target these changes for v6.15 ? If not, I can take
> > > > this patch with other FF-A changes in my tree for v6.15. Otherwise, it
> > > > is must go along with other changes.
> > > >
> > >
> > > Yes, feel free to pick them with your changes and we can push them
> > > later.
> >
> > So this series is not a 6.15 candidate?
>
> I think this is 6.15 stuff once it's been reviewed. Sudeep's message is
> a little confusing as it refers to 6.15 twice (I guess he meant 6.14 the
> first time?).
>

No I meant v6.15 both times 😉. Since not much time before v6.15, I was
thinking if this is not v6.15 material, I can just take the driver change
for v6.15 itself via my tree. Now that, you have clarified, I will leave
it to you. I think it doesn't conflict with any of driver changes in -next.

In short, just to avoid any further confusion, please take it via arm64/kvm
tree.

--
Regards,
Sudeep

Re: [PATCH v2 2/4] KVM: arm64: Move the ffa_to_linux definition to the ffa header

[Sudeep Holla]

On Thu, Feb 27, 2025 at 06:17:47PM +0000, Sebastian Ene wrote:
> Keep the ffa_to_linux error map in the header and move it away
> from the arm ffa driver to make it accessible for other components.
>

If you are reposting the series(which seems to be the case), please

s/KVM: arm64:/firmware: arm_ffa:/

 as it is not strictly KVM specific. With that, you can add

Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>

-- 
Regards,
Sudeep

[PATCH v2 3/4] KVM: arm64: Map the hypervisor FF-A buffers on ffa init

[Sebastian Ene]

Map the hypervisor's buffers irrespective to the host and return
a linux error code from the FF-A error code on failure. Remove
the unmap ff-a buffers calls from the hypervisor as it will
never be called.
Prevent the host from using FF-A directly with Trustzone
if the hypervisor could not map its own buffers.

Signed-off-by: Sebastian Ene <sebastianene@google.com>
---
 arch/arm64/kvm/hyp/nvhe/ffa.c | 46 +++++++++++++----------------------
 1 file changed, 17 insertions(+), 29 deletions(-)

diff --git a/arch/arm64/kvm/hyp/nvhe/ffa.c b/arch/arm64/kvm/hyp/nvhe/ffa.c
index 6df6131f1107..861f24de97cb 100644
--- a/arch/arm64/kvm/hyp/nvhe/ffa.c
+++ b/arch/arm64/kvm/hyp/nvhe/ffa.c
@@ -69,6 +69,7 @@ static struct kvm_ffa_buffers hyp_buffers;
 static struct kvm_ffa_buffers host_buffers;
 static u32 hyp_ffa_version;
 static bool has_version_negotiated;
+static bool has_ffa_supported;
 static DEFINE_HYP_SPINLOCK(version_lock);
 
 static void ffa_to_smccc_error(struct arm_smccc_res *res, u64 ffa_errno)
@@ -111,30 +112,18 @@ static bool is_ffa_call(u64 func_id)
 	       ARM_SMCCC_FUNC_NUM(func_id) <= FFA_MAX_FUNC_NUM;
 }
 
-static int ffa_map_hyp_buffers(u64 ffa_page_count)
+static int ffa_map_hyp_buffers(void)
 {
 	struct arm_smccc_res res;
 
 	arm_smccc_1_1_smc(FFA_FN64_RXTX_MAP,
 			  hyp_virt_to_phys(hyp_buffers.tx),
 			  hyp_virt_to_phys(hyp_buffers.rx),
-			  ffa_page_count,
+			  KVM_FFA_MBOX_NR_PAGES,
 			  0, 0, 0, 0,
 			  &res);
 
-	return res.a0 == FFA_SUCCESS ? FFA_RET_SUCCESS : res.a2;
-}
-
-static int ffa_unmap_hyp_buffers(void)
-{
-	struct arm_smccc_res res;
-
-	arm_smccc_1_1_smc(FFA_RXTX_UNMAP,
-			  HOST_FFA_ID,
-			  0, 0, 0, 0, 0, 0,
-			  &res);
-
-	return res.a0 == FFA_SUCCESS ? FFA_RET_SUCCESS : res.a2;
+	return res.a0 == FFA_SUCCESS ? 0 : ffa_to_linux_errno(res.a2);
 }
 
 static void ffa_mem_frag_tx(struct arm_smccc_res *res, u32 handle_lo,
@@ -213,18 +202,10 @@ static void do_ffa_rxtx_map(struct arm_smccc_res *res,
 		goto out_unlock;
 	}
 
-	/*
-	 * Map our hypervisor buffers into the SPMD before mapping and
-	 * pinning the host buffers in our own address space.
-	 */
-	ret = ffa_map_hyp_buffers(npages);
-	if (ret)
-		goto out_unlock;
-
 	ret = __pkvm_host_share_hyp(hyp_phys_to_pfn(tx));
 	if (ret) {
 		ret = FFA_RET_INVALID_PARAMETERS;
-		goto err_unmap;
+		goto out_unlock;
 	}
 
 	ret = __pkvm_host_share_hyp(hyp_phys_to_pfn(rx));
@@ -262,8 +243,6 @@ static void do_ffa_rxtx_map(struct arm_smccc_res *res,
 	__pkvm_host_unshare_hyp(hyp_phys_to_pfn(rx));
 err_unshare_tx:
 	__pkvm_host_unshare_hyp(hyp_phys_to_pfn(tx));
-err_unmap:
-	ffa_unmap_hyp_buffers();
 	goto out_unlock;
 }
 
@@ -291,9 +270,6 @@ static void do_ffa_rxtx_unmap(struct arm_smccc_res *res,
 	hyp_unpin_shared_mem(host_buffers.rx, host_buffers.rx + 1);
 	WARN_ON(__pkvm_host_unshare_hyp(hyp_virt_to_pfn(host_buffers.rx)));
 	host_buffers.rx = NULL;
-
-	ffa_unmap_hyp_buffers();
-
 out_unlock:
 	hyp_spin_unlock(&host_buffers.lock);
 out:
@@ -809,6 +785,11 @@ bool kvm_host_ffa_handler(struct kvm_cpu_context *host_ctxt, u32 func_id)
 	if (!is_ffa_call(func_id))
 		return false;
 
+	if (!has_ffa_supported) {
+		ffa_to_smccc_error(&res, FFA_RET_NOT_SUPPORTED);
+		goto out_handled;
+	}
+
 	if (!has_version_negotiated && func_id != FFA_VERSION) {
 		ffa_to_smccc_error(&res, FFA_RET_INVALID_PARAMETERS);
 		goto out_handled;
@@ -861,6 +842,7 @@ int hyp_ffa_init(void *pages)
 {
 	struct arm_smccc_res res;
 	void *tx, *rx;
+	int ret;
 
 	if (kvm_host_psci_config.smccc_version < ARM_SMCCC_VERSION_1_2)
 		return 0;
@@ -911,5 +893,11 @@ int hyp_ffa_init(void *pages)
 		.lock	= __HYP_SPIN_LOCK_UNLOCKED,
 	};
 
+	/* Map our hypervisor buffers into the SPMD */
+	ret = ffa_map_hyp_buffers();
+	if (ret)
+		return ret;
+
+	has_ffa_supported = true;
 	return 0;
 }
-- 
2.48.1.711.g2feabab25a-goog

Re: [PATCH v2 3/4] KVM: arm64: Map the hypervisor FF-A buffers on ffa init

[Will Deacon]

On Thu, Feb 27, 2025 at 06:17:48PM +0000, Sebastian Ene wrote:
> Map the hypervisor's buffers irrespective to the host and return
> a linux error code from the FF-A error code on failure. Remove
> the unmap ff-a buffers calls from the hypervisor as it will
> never be called.
> Prevent the host from using FF-A directly with Trustzone
> if the hypervisor could not map its own buffers.
> 
> Signed-off-by: Sebastian Ene <sebastianene@google.com>
> ---
>  arch/arm64/kvm/hyp/nvhe/ffa.c | 46 +++++++++++++----------------------
>  1 file changed, 17 insertions(+), 29 deletions(-)

[...]

> @@ -861,6 +842,7 @@ int hyp_ffa_init(void *pages)
>  {
>  	struct arm_smccc_res res;
>  	void *tx, *rx;
> +	int ret;
>  
>  	if (kvm_host_psci_config.smccc_version < ARM_SMCCC_VERSION_1_2)
>  		return 0;
> @@ -911,5 +893,11 @@ int hyp_ffa_init(void *pages)
>  		.lock	= __HYP_SPIN_LOCK_UNLOCKED,
>  	};
>  
> +	/* Map our hypervisor buffers into the SPMD */
> +	ret = ffa_map_hyp_buffers();
> +	if (ret)
> +		return ret;

Doesn't calling RXTX_MAP here undo the fix from c9c012625e12 ("KVM:
arm64: Trap FFA_VERSION host call in pKVM") where we want to allow for
the host to negotiate the version lazily?

Will

Re: [PATCH v2 3/4] KVM: arm64: Map the hypervisor FF-A buffers on ffa init

[Sebastian Ene]

On Mon, Mar 03, 2025 at 11:43:03PM +0000, Will Deacon wrote:
> On Thu, Feb 27, 2025 at 06:17:48PM +0000, Sebastian Ene wrote:
> > Map the hypervisor's buffers irrespective to the host and return
> > a linux error code from the FF-A error code on failure. Remove
> > the unmap ff-a buffers calls from the hypervisor as it will
> > never be called.
> > Prevent the host from using FF-A directly with Trustzone
> > if the hypervisor could not map its own buffers.
> > 
> > Signed-off-by: Sebastian Ene <sebastianene@google.com>
> > ---
> >  arch/arm64/kvm/hyp/nvhe/ffa.c | 46 +++++++++++++----------------------
> >  1 file changed, 17 insertions(+), 29 deletions(-)
> 
> [...]
> 
> > @@ -861,6 +842,7 @@ int hyp_ffa_init(void *pages)
> >  {
> >  	struct arm_smccc_res res;
> >  	void *tx, *rx;
> > +	int ret;
> >  
> >  	if (kvm_host_psci_config.smccc_version < ARM_SMCCC_VERSION_1_2)
> >  		return 0;
> > @@ -911,5 +893,11 @@ int hyp_ffa_init(void *pages)
> >  		.lock	= __HYP_SPIN_LOCK_UNLOCKED,
> >  	};
> >  
> > +	/* Map our hypervisor buffers into the SPMD */
> > +	ret = ffa_map_hyp_buffers();
> > +	if (ret)
> > +		return ret;
> 
> Doesn't calling RXTX_MAP here undo the fix from c9c012625e12 ("KVM:
> arm64: Trap FFA_VERSION host call in pKVM") where we want to allow for
> the host to negotiate the version lazily?

We still have the same behaviour where we don't allow memory
sharing to happen until the version is negotiated but this
separates the hypervisor buffer mapping part from the host.

> 
> Will

Thanks,
Sebastian

Re: [PATCH v2 3/4] KVM: arm64: Map the hypervisor FF-A buffers on ffa init

[Will Deacon]

On Tue, Mar 04, 2025 at 12:53:25AM +0000, Sebastian Ene wrote:
> On Mon, Mar 03, 2025 at 11:43:03PM +0000, Will Deacon wrote:
> > On Thu, Feb 27, 2025 at 06:17:48PM +0000, Sebastian Ene wrote:
> > > Map the hypervisor's buffers irrespective to the host and return
> > > a linux error code from the FF-A error code on failure. Remove
> > > the unmap ff-a buffers calls from the hypervisor as it will
> > > never be called.
> > > Prevent the host from using FF-A directly with Trustzone
> > > if the hypervisor could not map its own buffers.
> > > 
> > > Signed-off-by: Sebastian Ene <sebastianene@google.com>
> > > ---
> > >  arch/arm64/kvm/hyp/nvhe/ffa.c | 46 +++++++++++++----------------------
> > >  1 file changed, 17 insertions(+), 29 deletions(-)
> > 
> > [...]
> > 
> > > @@ -861,6 +842,7 @@ int hyp_ffa_init(void *pages)
> > >  {
> > >  	struct arm_smccc_res res;
> > >  	void *tx, *rx;
> > > +	int ret;
> > >  
> > >  	if (kvm_host_psci_config.smccc_version < ARM_SMCCC_VERSION_1_2)
> > >  		return 0;
> > > @@ -911,5 +893,11 @@ int hyp_ffa_init(void *pages)
> > >  		.lock	= __HYP_SPIN_LOCK_UNLOCKED,
> > >  	};
> > >  
> > > +	/* Map our hypervisor buffers into the SPMD */
> > > +	ret = ffa_map_hyp_buffers();
> > > +	if (ret)
> > > +		return ret;
> > 
> > Doesn't calling RXTX_MAP here undo the fix from c9c012625e12 ("KVM:
> > arm64: Trap FFA_VERSION host call in pKVM") where we want to allow for
> > the host to negotiate the version lazily?
> 
> We still have the same behaviour where we don't allow memory
> sharing to happen until the version is negotiated but this
> separates the hypervisor buffer mapping part from the host.

Sadly, the spec doesn't restrict this to the memory sharing calls:

  | [...] negotiation of the version must happen before an invocation of
  | any other FF-A ABI

We're also probing the minimum rxtx size in hyp_ffa_post_init() so doing
this here is doubly wrong.

So I think we should probably just drop this patch.

Will

Re: [PATCH v2 3/4] KVM: arm64: Map the hypervisor FF-A buffers on ffa init

[Sebastian Ene]

On Tue, Mar 04, 2025 at 01:56:35AM +0000, Will Deacon wrote:
> On Tue, Mar 04, 2025 at 12:53:25AM +0000, Sebastian Ene wrote:
> > On Mon, Mar 03, 2025 at 11:43:03PM +0000, Will Deacon wrote:
> > > On Thu, Feb 27, 2025 at 06:17:48PM +0000, Sebastian Ene wrote:
> > > > Map the hypervisor's buffers irrespective to the host and return
> > > > a linux error code from the FF-A error code on failure. Remove
> > > > the unmap ff-a buffers calls from the hypervisor as it will
> > > > never be called.
> > > > Prevent the host from using FF-A directly with Trustzone
> > > > if the hypervisor could not map its own buffers.
> > > > 
> > > > Signed-off-by: Sebastian Ene <sebastianene@google.com>
> > > > ---
> > > >  arch/arm64/kvm/hyp/nvhe/ffa.c | 46 +++++++++++++----------------------
> > > >  1 file changed, 17 insertions(+), 29 deletions(-)
> > > 
> > > [...]
> > > 
> > > > @@ -861,6 +842,7 @@ int hyp_ffa_init(void *pages)
> > > >  {
> > > >  	struct arm_smccc_res res;
> > > >  	void *tx, *rx;
> > > > +	int ret;
> > > >  
> > > >  	if (kvm_host_psci_config.smccc_version < ARM_SMCCC_VERSION_1_2)
> > > >  		return 0;
> > > > @@ -911,5 +893,11 @@ int hyp_ffa_init(void *pages)
> > > >  		.lock	= __HYP_SPIN_LOCK_UNLOCKED,
> > > >  	};
> > > >  
> > > > +	/* Map our hypervisor buffers into the SPMD */
> > > > +	ret = ffa_map_hyp_buffers();
> > > > +	if (ret)
> > > > +		return ret;
> > > 
> > > Doesn't calling RXTX_MAP here undo the fix from c9c012625e12 ("KVM:
> > > arm64: Trap FFA_VERSION host call in pKVM") where we want to allow for
> > > the host to negotiate the version lazily?
> > 
> > We still have the same behaviour where we don't allow memory
> > sharing to happen until the version is negotiated but this
> > separates the hypervisor buffer mapping part from the host.
> 
> Sadly, the spec doesn't restrict this to the memory sharing calls:
> 
>   | [...] negotiation of the version must happen before an invocation of
>   | any other FF-A ABI
> 

We do that, as the hypervisor negotiates its own version in
hyp_ffa_init. I think the host shouldn't be allowed to overwrite the
hyp_ffa_version obtained from _init, this feels wrong as you
can have a driver that forcefully downgrades the hypervisor to an old
version.

We need to do three things, Sudeep & Will please correct me if I am
wrong, but this is how I see it:

- the hypervisor should act as a separate entity (it has a different ID and
in the current implementation we don't do a distinction between host/hyp) and
it should be able to lock its own version from init.
- keep a separate version negotiated for the host
- trap FFA_ID_GET from the host and return ID=1 because
  currently we forward the call to the TZ and it returns the same ID
  as the (hypervisor == 0).

> We're also probing the minimum rxtx size in hyp_ffa_post_init() so doing
> this here is doubly wrong.
> 

Those operations should happen before the current ffa_map_hyp_buffers()
call, I agree.

Thanks,
Sebastian

> So I think we should probably just drop this patch.
> 
> Will

Re: [PATCH v2 3/4] KVM: arm64: Map the hypervisor FF-A buffers on ffa init

[Will Deacon]

On Tue, Mar 04, 2025 at 05:38:02PM +0000, Sebastian Ene wrote:
> On Tue, Mar 04, 2025 at 01:56:35AM +0000, Will Deacon wrote:
> > On Tue, Mar 04, 2025 at 12:53:25AM +0000, Sebastian Ene wrote:
> > > On Mon, Mar 03, 2025 at 11:43:03PM +0000, Will Deacon wrote:
> > > > On Thu, Feb 27, 2025 at 06:17:48PM +0000, Sebastian Ene wrote:
> > > > > Map the hypervisor's buffers irrespective to the host and return
> > > > > a linux error code from the FF-A error code on failure. Remove
> > > > > the unmap ff-a buffers calls from the hypervisor as it will
> > > > > never be called.
> > > > > Prevent the host from using FF-A directly with Trustzone
> > > > > if the hypervisor could not map its own buffers.
> > > > > 
> > > > > Signed-off-by: Sebastian Ene <sebastianene@google.com>
> > > > > ---
> > > > >  arch/arm64/kvm/hyp/nvhe/ffa.c | 46 +++++++++++++----------------------
> > > > >  1 file changed, 17 insertions(+), 29 deletions(-)
> > > > 
> > > > [...]
> > > > 
> > > > > @@ -861,6 +842,7 @@ int hyp_ffa_init(void *pages)
> > > > >  {
> > > > >  	struct arm_smccc_res res;
> > > > >  	void *tx, *rx;
> > > > > +	int ret;
> > > > >  
> > > > >  	if (kvm_host_psci_config.smccc_version < ARM_SMCCC_VERSION_1_2)
> > > > >  		return 0;
> > > > > @@ -911,5 +893,11 @@ int hyp_ffa_init(void *pages)
> > > > >  		.lock	= __HYP_SPIN_LOCK_UNLOCKED,
> > > > >  	};
> > > > >  
> > > > > +	/* Map our hypervisor buffers into the SPMD */
> > > > > +	ret = ffa_map_hyp_buffers();
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > 
> > > > Doesn't calling RXTX_MAP here undo the fix from c9c012625e12 ("KVM:
> > > > arm64: Trap FFA_VERSION host call in pKVM") where we want to allow for
> > > > the host to negotiate the version lazily?
> > > 
> > > We still have the same behaviour where we don't allow memory
> > > sharing to happen until the version is negotiated but this
> > > separates the hypervisor buffer mapping part from the host.
> > 
> > Sadly, the spec doesn't restrict this to the memory sharing calls:
> > 
> >   | [...] negotiation of the version must happen before an invocation of
> >   | any other FF-A ABI
> > 
> 
> We do that, as the hypervisor negotiates its own version in
> hyp_ffa_init.

hyp_ffa_init() only issues FFA_VERSION afaict, which is the one call
that you're allowed to make during negotiation. So the existing code is
fine.

> I think the host shouldn't be allowed to overwrite the
> hyp_ffa_version obtained from _init, this feels wrong as you
> can have a driver that forcefully downgrades the hypervisor to an old
> version.

I think that's also fine. The FFA code in the hypervisor exists solely
to proxy requests from the host; it's not used for anything else and so,
from the host's persective, FFA should behave identically to the case in
which the proxy is not present (e.g. if we were just using VHE). That
means that we're doing the right thing by deferring to the host for
version negotation.

Are you saying there's a bug in the current code if the host negotiates
the downgrade?

> We need to do three things, Sudeep & Will please correct me if I am
> wrong, but this is how I see it:
> 
> - the hypervisor should act as a separate entity (it has a different ID and
> in the current implementation we don't do a distinction between host/hyp) and
> it should be able to lock its own version from init.

I strongly disagree with that. The hypervisor isn't using FFA for
anything other than proxying the host and so we don't need to negotiate
a separate version.

What would we gain by doing this? Is there a bug with what we're doing
at the moment?

> - keep a separate version negotiated for the host
> - trap FFA_ID_GET from the host and return ID=1 because
>   currently we forward the call to the TZ and it returns the same ID
>   as the (hypervisor == 0).

Why is this beneficial? It just looks like complexity at EL2 for no gain
to me, but maybe I'm missing something.

Will

Re: [PATCH v2 3/4] KVM: arm64: Map the hypervisor FF-A buffers on ffa init

[Sebastian Ene]

On Wed, Mar 05, 2025 at 12:38:08AM +0000, Will Deacon wrote:
> On Tue, Mar 04, 2025 at 05:38:02PM +0000, Sebastian Ene wrote:
> > On Tue, Mar 04, 2025 at 01:56:35AM +0000, Will Deacon wrote:
> > > On Tue, Mar 04, 2025 at 12:53:25AM +0000, Sebastian Ene wrote:
> > > > On Mon, Mar 03, 2025 at 11:43:03PM +0000, Will Deacon wrote:
> > > > > On Thu, Feb 27, 2025 at 06:17:48PM +0000, Sebastian Ene wrote:
> > > > > > Map the hypervisor's buffers irrespective to the host and return
> > > > > > a linux error code from the FF-A error code on failure. Remove
> > > > > > the unmap ff-a buffers calls from the hypervisor as it will
> > > > > > never be called.
> > > > > > Prevent the host from using FF-A directly with Trustzone
> > > > > > if the hypervisor could not map its own buffers.
> > > > > > 
> > > > > > Signed-off-by: Sebastian Ene <sebastianene@google.com>
> > > > > > ---
> > > > > >  arch/arm64/kvm/hyp/nvhe/ffa.c | 46 +++++++++++++----------------------
> > > > > >  1 file changed, 17 insertions(+), 29 deletions(-)
> > > > > 
> > > > > [...]
> > > > > 
> > > > > > @@ -861,6 +842,7 @@ int hyp_ffa_init(void *pages)
> > > > > >  {
> > > > > >  	struct arm_smccc_res res;
> > > > > >  	void *tx, *rx;
> > > > > > +	int ret;
> > > > > >  
> > > > > >  	if (kvm_host_psci_config.smccc_version < ARM_SMCCC_VERSION_1_2)
> > > > > >  		return 0;
> > > > > > @@ -911,5 +893,11 @@ int hyp_ffa_init(void *pages)
> > > > > >  		.lock	= __HYP_SPIN_LOCK_UNLOCKED,
> > > > > >  	};
> > > > > >  
> > > > > > +	/* Map our hypervisor buffers into the SPMD */
> > > > > > +	ret = ffa_map_hyp_buffers();
> > > > > > +	if (ret)
> > > > > > +		return ret;
> > > > > 
> > > > > Doesn't calling RXTX_MAP here undo the fix from c9c012625e12 ("KVM:
> > > > > arm64: Trap FFA_VERSION host call in pKVM") where we want to allow for
> > > > > the host to negotiate the version lazily?
> > > > 
> > > > We still have the same behaviour where we don't allow memory
> > > > sharing to happen until the version is negotiated but this
> > > > separates the hypervisor buffer mapping part from the host.
> > > 
> > > Sadly, the spec doesn't restrict this to the memory sharing calls:
> > > 
> > >   | [...] negotiation of the version must happen before an invocation of
> > >   | any other FF-A ABI
> > > 
> > 
> > We do that, as the hypervisor negotiates its own version in
> > hyp_ffa_init.
> 
> hyp_ffa_init() only issues FFA_VERSION afaict, which is the one call
> that you're allowed to make during negotiation. So the existing code is
> fine.
> 
> > I think the host shouldn't be allowed to overwrite the
> > hyp_ffa_version obtained from _init, this feels wrong as you
> > can have a driver that forcefully downgrades the hypervisor to an old
> > version.
> 
> I think that's also fine. The FFA code in the hypervisor exists solely
> to proxy requests from the host; it's not used for anything else and so,
> from the host's persective, FFA should behave identically to the case in
> which the proxy is not present (e.g. if we were just using VHE). That
> means that we're doing the right thing by deferring to the host for
> version negotation.
> 
> Are you saying there's a bug in the current code if the host negotiates
> the downgrade?

It is an issue *only* for doing guest-ffa (which isn't posted here yet).
If we allow the host to dictate the version & there is an issue with TZ
FF-A dispatcher in that version => the guests will be affected by this
as well.

> 
> > We need to do three things, Sudeep & Will please correct me if I am
> > wrong, but this is how I see it:
> > 
> > - the hypervisor should act as a separate entity (it has a different ID and
> > in the current implementation we don't do a distinction between host/hyp) and
> > it should be able to lock its own version from init.
> 
> I strongly disagree with that. The hypervisor isn't using FFA for
> anything other than proxying the host and so we don't need to negotiate
> a separate version.
> 
> What would we gain by doing this? Is there a bug with what we're doing
> at the moment?

I think we need to make a distinction between the host and the
hypervisor when we are adding support for guest-ffa. We currently have
the same id (== 0) for both of them.

> 
> > - keep a separate version negotiated for the host
> > - trap FFA_ID_GET from the host and return ID=1 because
> >   currently we forward the call to the TZ and it returns the same ID
> >   as the (hypervisor == 0).
> 
> Why is this beneficial? It just looks like complexity at EL2 for no gain
> to me, but maybe I'm missing something.
>

Because the host can impersonate the hypervisor using ff-a direct calls atm.
and we are in a position to restrict the host from playing nasty games
with TZ.

> Will

Thanks,
Sebastian

Re: [PATCH v2 3/4] KVM: arm64: Map the hypervisor FF-A buffers on ffa init

[Will Deacon]

On Wed, Mar 05, 2025 at 06:36:01PM +0000, Sebastian Ene wrote:
> On Wed, Mar 05, 2025 at 12:38:08AM +0000, Will Deacon wrote:
> > On Tue, Mar 04, 2025 at 05:38:02PM +0000, Sebastian Ene wrote:
> > > On Tue, Mar 04, 2025 at 01:56:35AM +0000, Will Deacon wrote:
> > > >   | [...] negotiation of the version must happen before an invocation of
> > > >   | any other FF-A ABI
> > > > 
> > > 
> > > We do that, as the hypervisor negotiates its own version in
> > > hyp_ffa_init.
> > 
> > hyp_ffa_init() only issues FFA_VERSION afaict, which is the one call
> > that you're allowed to make during negotiation. So the existing code is
> > fine.
> > 
> > > I think the host shouldn't be allowed to overwrite the
> > > hyp_ffa_version obtained from _init, this feels wrong as you
> > > can have a driver that forcefully downgrades the hypervisor to an old
> > > version.
> > 
> > I think that's also fine. The FFA code in the hypervisor exists solely
> > to proxy requests from the host; it's not used for anything else and so,
> > from the host's persective, FFA should behave identically to the case in
> > which the proxy is not present (e.g. if we were just using VHE). That
> > means that we're doing the right thing by deferring to the host for
> > version negotation.
> > 
> > Are you saying there's a bug in the current code if the host negotiates
> > the downgrade?
> 
> It is an issue *only* for doing guest-ffa (which isn't posted here yet).
> If we allow the host to dictate the version & there is an issue with TZ
> FF-A dispatcher in that version => the guests will be affected by this
> as well.

When we get to guests doing FF-A, I still think the host should be
responsible for the version negotiation and guests should just be given
whatever has been negotiated. We could extend the hypervisor to marshall
between different versions, but I'd rather do that only if we actually
need it.

> > > We need to do three things, Sudeep & Will please correct me if I am
> > > wrong, but this is how I see it:
> > > 
> > > - the hypervisor should act as a separate entity (it has a different ID and
> > > in the current implementation we don't do a distinction between host/hyp) and
> > > it should be able to lock its own version from init.
> > 
> > I strongly disagree with that. The hypervisor isn't using FFA for
> > anything other than proxying the host and so we don't need to negotiate
> > a separate version.
> > 
> > What would we gain by doing this? Is there a bug with what we're doing
> > at the moment?
> 
> I think we need to make a distinction between the host and the
> hypervisor when we are adding support for guest-ffa. We currently have
> the same id (== 0) for both of them.

Right, and we currently don't support guest-ffa, so no problem :)

> > > - keep a separate version negotiated for the host
> > > - trap FFA_ID_GET from the host and return ID=1 because
> > >   currently we forward the call to the TZ and it returns the same ID
> > >   as the (hypervisor == 0).
> > 
> > Why is this beneficial? It just looks like complexity at EL2 for no gain
> > to me, but maybe I'm missing something.
> >
> 
> Because the host can impersonate the hypervisor using ff-a direct calls atm.
> and we are in a position to restrict the host from playing nasty games
> with TZ.

Can you give a specific example of why impersonating a direct call is a
problem? I agree that it sounds bad, but the hypervisor is still in the
middle and so it can check what the call is requesting.

Will

[PATCH v2 4/4] KVM: arm64: Release the ownership of the hyp rx buffer to Trustzone

[Sebastian Ene]

Introduce the release FF-A call to notify Trustzone that the hypervisor
has finished copying the data from the buffer shared with Trustzone to
the non-secure partition.

Reported-by: Andrei Homescu <ahomescu@google.com>
Signed-off-by: Sebastian Ene <sebastianene@google.com>
---
 arch/arm64/kvm/hyp/nvhe/ffa.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/hyp/nvhe/ffa.c b/arch/arm64/kvm/hyp/nvhe/ffa.c
index 861f24de97cb..7da0203f1ee9 100644
--- a/arch/arm64/kvm/hyp/nvhe/ffa.c
+++ b/arch/arm64/kvm/hyp/nvhe/ffa.c
@@ -725,6 +725,7 @@ static void do_ffa_part_get(struct arm_smccc_res *res,
 	DECLARE_REG(u32, uuid3, ctxt, 4);
 	DECLARE_REG(u32, flags, ctxt, 5);
 	u32 count, partition_sz, copy_sz;
+	struct arm_smccc_res _res;
 
 	hyp_spin_lock(&host_buffers.lock);
 	if (!host_buffers.rx) {
@@ -741,7 +742,7 @@ static void do_ffa_part_get(struct arm_smccc_res *res,
 
 	count = res->a2;
 	if (!count)
-		goto out_unlock;
+		goto release_rx;
 
 	if (hyp_ffa_version > FFA_VERSION_1_0) {
 		/* Get the number of partitions deployed in the system */
@@ -757,10 +758,12 @@ static void do_ffa_part_get(struct arm_smccc_res *res,
 	copy_sz = partition_sz * count;
 	if (copy_sz > KVM_FFA_MBOX_NR_PAGES * PAGE_SIZE) {
 		ffa_to_smccc_res(res, FFA_RET_ABORTED);
-		goto out_unlock;
+		goto release_rx;
 	}
 
 	memcpy(host_buffers.rx, hyp_buffers.rx, copy_sz);
+release_rx:
+	ffa_rx_release(&_res);
 out_unlock:
 	hyp_spin_unlock(&host_buffers.lock);
 }
-- 
2.48.1.711.g2feabab25a-goog

Re: [PATCH v2 4/4] KVM: arm64: Release the ownership of the hyp rx buffer to Trustzone

[Will Deacon]

On Thu, Feb 27, 2025 at 06:17:49PM +0000, Sebastian Ene wrote:
> Introduce the release FF-A call to notify Trustzone that the hypervisor
> has finished copying the data from the buffer shared with Trustzone to
> the non-secure partition.
> 
> Reported-by: Andrei Homescu <ahomescu@google.com>
> Signed-off-by: Sebastian Ene <sebastianene@google.com>
> ---
>  arch/arm64/kvm/hyp/nvhe/ffa.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp/nvhe/ffa.c b/arch/arm64/kvm/hyp/nvhe/ffa.c
> index 861f24de97cb..7da0203f1ee9 100644
> --- a/arch/arm64/kvm/hyp/nvhe/ffa.c
> +++ b/arch/arm64/kvm/hyp/nvhe/ffa.c
> @@ -725,6 +725,7 @@ static void do_ffa_part_get(struct arm_smccc_res *res,
>  	DECLARE_REG(u32, uuid3, ctxt, 4);
>  	DECLARE_REG(u32, flags, ctxt, 5);
>  	u32 count, partition_sz, copy_sz;
> +	struct arm_smccc_res _res;
>  
>  	hyp_spin_lock(&host_buffers.lock);
>  	if (!host_buffers.rx) {
> @@ -741,7 +742,7 @@ static void do_ffa_part_get(struct arm_smccc_res *res,
>  
>  	count = res->a2;
>  	if (!count)
> -		goto out_unlock;
> +		goto release_rx;
>  
>  	if (hyp_ffa_version > FFA_VERSION_1_0) {
>  		/* Get the number of partitions deployed in the system */
> @@ -757,10 +758,12 @@ static void do_ffa_part_get(struct arm_smccc_res *res,
>  	copy_sz = partition_sz * count;
>  	if (copy_sz > KVM_FFA_MBOX_NR_PAGES * PAGE_SIZE) {
>  		ffa_to_smccc_res(res, FFA_RET_ABORTED);
> -		goto out_unlock;
> +		goto release_rx;
>  	}
>  
>  	memcpy(host_buffers.rx, hyp_buffers.rx, copy_sz);
> +release_rx:
> +	ffa_rx_release(&_res);

Hmm, the FFA spec is characteristically unclear as to whether or not we
need to release the rx buffer in the case that the flags indicate use of
the rx buffer but the returned partition count is 0.

Sudeep -- do you know what we should be doing in that case?

Will

Re: [PATCH v2 4/4] KVM: arm64: Release the ownership of the hyp rx buffer to Trustzone

[Sudeep Holla]

On Wed, Mar 05, 2025 at 12:45:23AM +0000, Will Deacon wrote:
> On Thu, Feb 27, 2025 at 06:17:49PM +0000, Sebastian Ene wrote:
> > Introduce the release FF-A call to notify Trustzone that the hypervisor
> > has finished copying the data from the buffer shared with Trustzone to
> > the non-secure partition.
> > 
> > Reported-by: Andrei Homescu <ahomescu@google.com>
> > Signed-off-by: Sebastian Ene <sebastianene@google.com>
> > ---
> >  arch/arm64/kvm/hyp/nvhe/ffa.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm64/kvm/hyp/nvhe/ffa.c b/arch/arm64/kvm/hyp/nvhe/ffa.c
> > index 861f24de97cb..7da0203f1ee9 100644
> > --- a/arch/arm64/kvm/hyp/nvhe/ffa.c
> > +++ b/arch/arm64/kvm/hyp/nvhe/ffa.c
> > @@ -725,6 +725,7 @@ static void do_ffa_part_get(struct arm_smccc_res *res,
> >  	DECLARE_REG(u32, uuid3, ctxt, 4);
> >  	DECLARE_REG(u32, flags, ctxt, 5);
> >  	u32 count, partition_sz, copy_sz;
> > +	struct arm_smccc_res _res;
> >  
> >  	hyp_spin_lock(&host_buffers.lock);
> >  	if (!host_buffers.rx) {
> > @@ -741,7 +742,7 @@ static void do_ffa_part_get(struct arm_smccc_res *res,
> >  
> >  	count = res->a2;
> >  	if (!count)
> > -		goto out_unlock;
> > +		goto release_rx;
> >  
> >  	if (hyp_ffa_version > FFA_VERSION_1_0) {
> >  		/* Get the number of partitions deployed in the system */
> > @@ -757,10 +758,12 @@ static void do_ffa_part_get(struct arm_smccc_res *res,
> >  	copy_sz = partition_sz * count;
> >  	if (copy_sz > KVM_FFA_MBOX_NR_PAGES * PAGE_SIZE) {
> >  		ffa_to_smccc_res(res, FFA_RET_ABORTED);
> > -		goto out_unlock;
> > +		goto release_rx;
> >  	}
> >  
> >  	memcpy(host_buffers.rx, hyp_buffers.rx, copy_sz);
> > +release_rx:
> > +	ffa_rx_release(&_res);
> 
> Hmm, the FFA spec is characteristically unclear as to whether or not we
> need to release the rx buffer in the case that the flags indicate use of
> the rx buffer but the returned partition count is 0.
> 
> Sudeep -- do you know what we should be doing in that case?
> 

We need to call RX_RELEASE here. I went back to the spec to confirm the
same again.

v1.2 EAC0 spec Section 7.2.2.4.2 Transfer of buffer ownership
(Or just look for the section title in any version of the spec)
"
2. Ownership transfer for the RX buffer takes place as follows.
    2. For a framework message,
       1. Completion of the FFA_PARTITION_INFO_GET ABI transfers the ownership
       of the caller’s RX buffer from the Producer to the Consumer.
3. For both types of messages, an invocation of the following FF-A ABIs
    transfers the ownership from the Consumer to the Producer.
       1. FFA_MSG_WAIT ...
       2. FFA_RX_RELEASE.
"

Hope that helps, can dig deeper if there are any ambiguities around this.

-- 
Regards,
Sudeep

Re: [PATCH v2 4/4] KVM: arm64: Release the ownership of the hyp rx buffer to Trustzone

[Will Deacon]

On Wed, Mar 05, 2025 at 09:41:04AM +0000, Sudeep Holla wrote:
> On Wed, Mar 05, 2025 at 12:45:23AM +0000, Will Deacon wrote:
> > Hmm, the FFA spec is characteristically unclear as to whether or not we
> > need to release the rx buffer in the case that the flags indicate use of
> > the rx buffer but the returned partition count is 0.
> > 
> > Sudeep -- do you know what we should be doing in that case?
> > 
> 
> We need to call RX_RELEASE here. I went back to the spec to confirm the
> same again.
> 
> v1.2 EAC0 spec Section 7.2.2.4.2 Transfer of buffer ownership
> (Or just look for the section title in any version of the spec)
> "
> 2. Ownership transfer for the RX buffer takes place as follows.
>     2. For a framework message,
>        1. Completion of the FFA_PARTITION_INFO_GET ABI transfers the ownership
>        of the caller’s RX buffer from the Producer to the Consumer.
> 3. For both types of messages, an invocation of the following FF-A ABIs
>     transfers the ownership from the Consumer to the Producer.
>        1. FFA_MSG_WAIT ...
>        2. FFA_RX_RELEASE.
> "
> 
> Hope that helps, can dig deeper if there are any ambiguities around this.

Thanks Sudeep, but that also makes it sound like we need the RX_RELEASE
even if we're not using the RX buffer per the input flags. :/

Will

Re: [PATCH v2 4/4] KVM: arm64: Release the ownership of the hyp rx buffer to Trustzone

[Sudeep Holla]

On Wed, Mar 05, 2025 at 07:34:26PM +0000, Will Deacon wrote:
> On Wed, Mar 05, 2025 at 09:41:04AM +0000, Sudeep Holla wrote:
> > On Wed, Mar 05, 2025 at 12:45:23AM +0000, Will Deacon wrote:
> > > Hmm, the FFA spec is characteristically unclear as to whether or not we
> > > need to release the rx buffer in the case that the flags indicate use of
> > > the rx buffer but the returned partition count is 0.
> > > 
> > > Sudeep -- do you know what we should be doing in that case?
> > > 
> > 
> > We need to call RX_RELEASE here. I went back to the spec to confirm the
> > same again.
> > 
> > v1.2 EAC0 spec Section 7.2.2.4.2 Transfer of buffer ownership
> > (Or just look for the section title in any version of the spec)
> > "
> > 2. Ownership transfer for the RX buffer takes place as follows.
> >     2. For a framework message,
> >        1. Completion of the FFA_PARTITION_INFO_GET ABI transfers the ownership
> >        of the caller’s RX buffer from the Producer to the Consumer.
> > 3. For both types of messages, an invocation of the following FF-A ABIs
> >     transfers the ownership from the Consumer to the Producer.
> >        1. FFA_MSG_WAIT ...
> >        2. FFA_RX_RELEASE.
> > "
> > 
> > Hope that helps, can dig deeper if there are any ambiguities around this.
> 
> Thanks Sudeep, but that also makes it sound like we need the RX_RELEASE
> even if we're not using the RX buffer per the input flags. :/
> 

Good spot, I had forgotten about the input flags that can avoid using the
buffer. I will see if we can improve the spec in that regards.

-- 
Regards,
Sudeep

Re: [PATCH v2 4/4] KVM: arm64: Release the ownership of the hyp rx buffer to Trustzone

[Will Deacon]

On Thu, Mar 06, 2025 at 09:40:43AM +0000, Sudeep Holla wrote:
> On Wed, Mar 05, 2025 at 07:34:26PM +0000, Will Deacon wrote:
> > On Wed, Mar 05, 2025 at 09:41:04AM +0000, Sudeep Holla wrote:
> > > On Wed, Mar 05, 2025 at 12:45:23AM +0000, Will Deacon wrote:
> > > > Hmm, the FFA spec is characteristically unclear as to whether or not we
> > > > need to release the rx buffer in the case that the flags indicate use of
> > > > the rx buffer but the returned partition count is 0.
> > > > 
> > > > Sudeep -- do you know what we should be doing in that case?
> > > > 
> > > 
> > > We need to call RX_RELEASE here. I went back to the spec to confirm the
> > > same again.
> > > 
> > > v1.2 EAC0 spec Section 7.2.2.4.2 Transfer of buffer ownership
> > > (Or just look for the section title in any version of the spec)
> > > "
> > > 2. Ownership transfer for the RX buffer takes place as follows.
> > >     2. For a framework message,
> > >        1. Completion of the FFA_PARTITION_INFO_GET ABI transfers the ownership
> > >        of the caller’s RX buffer from the Producer to the Consumer.
> > > 3. For both types of messages, an invocation of the following FF-A ABIs
> > >     transfers the ownership from the Consumer to the Producer.
> > >        1. FFA_MSG_WAIT ...
> > >        2. FFA_RX_RELEASE.
> > > "
> > > 
> > > Hope that helps, can dig deeper if there are any ambiguities around this.
> > 
> > Thanks Sudeep, but that also makes it sound like we need the RX_RELEASE
> > even if we're not using the RX buffer per the input flags. :/
> > 
> 
> Good spot, I had forgotten about the input flags that can avoid using the
> buffer. I will see if we can improve the spec in that regards.

Thanks. In the meantime, what do you think is the correct behaviour in that
case? I guess _not_ doing the release when the flags don't request the RX
buffer? In other words:


	if (flags & PARTITION_INFO_GET_RETURN_COUNT_ONLY)
		goto out_unlock;

	if (!count)
		goto release_rx;

	[...]

	if (copy_sz > KVM_FFA_MBOX_NR_PAGES * PAGE_SIZE) {
		ffa_to_smccc_res(res, FFA_RET_ABORTED);
		goto release_rx;
	}

	memcpy(host_buffers.rx, hyp_buffers.rx, copy_sz);
release_rx:
	ffa_rx_release(&_res);
out_unlock:
	hyp_spin_unlock(&host_buffers.lock);
}


What do you reckon?

Will

Re: [PATCH v2 4/4] KVM: arm64: Release the ownership of the hyp rx buffer to Trustzone

[Sudeep Holla]

On Thu, Mar 13, 2025 at 12:15:59PM +0000, Will Deacon wrote:
> On Thu, Mar 06, 2025 at 09:40:43AM +0000, Sudeep Holla wrote:
> > On Wed, Mar 05, 2025 at 07:34:26PM +0000, Will Deacon wrote:
> > > On Wed, Mar 05, 2025 at 09:41:04AM +0000, Sudeep Holla wrote:
> > > > On Wed, Mar 05, 2025 at 12:45:23AM +0000, Will Deacon wrote:
> > > > > Hmm, the FFA spec is characteristically unclear as to whether or not we
> > > > > need to release the rx buffer in the case that the flags indicate use of
> > > > > the rx buffer but the returned partition count is 0.
> > > > >
> > > > > Sudeep -- do you know what we should be doing in that case?
> > > > >
> > > >
> > > > We need to call RX_RELEASE here. I went back to the spec to confirm the
> > > > same again.
> > > >
> > > > v1.2 EAC0 spec Section 7.2.2.4.2 Transfer of buffer ownership
> > > > (Or just look for the section title in any version of the spec)
> > > > "
> > > > 2. Ownership transfer for the RX buffer takes place as follows.
> > > >     2. For a framework message,
> > > >        1. Completion of the FFA_PARTITION_INFO_GET ABI transfers the ownership
> > > >        of the caller’s RX buffer from the Producer to the Consumer.
> > > > 3. For both types of messages, an invocation of the following FF-A ABIs
> > > >     transfers the ownership from the Consumer to the Producer.
> > > >        1. FFA_MSG_WAIT ...
> > > >        2. FFA_RX_RELEASE.
> > > > "
> > > >
> > > > Hope that helps, can dig deeper if there are any ambiguities around this.
> > >
> > > Thanks Sudeep, but that also makes it sound like we need the RX_RELEASE
> > > even if we're not using the RX buffer per the input flags. :/
> > >
> >
> > Good spot, I had forgotten about the input flags that can avoid using the
> > buffer. I will see if we can improve the spec in that regards.
>
> Thanks. In the meantime, what do you think is the correct behaviour in that
> case? I guess _not_ doing the release when the flags don't request the RX
> buffer? In other words:
>
>
> 	if (flags & PARTITION_INFO_GET_RETURN_COUNT_ONLY)
> 		goto out_unlock;
>
> 	if (!count)
> 		goto release_rx;
>
> 	[...]
>
> 	if (copy_sz > KVM_FFA_MBOX_NR_PAGES * PAGE_SIZE) {
> 		ffa_to_smccc_res(res, FFA_RET_ABORTED);
> 		goto release_rx;
> 	}
>
> 	memcpy(host_buffers.rx, hyp_buffers.rx, copy_sz);
> release_rx:
> 	ffa_rx_release(&_res);
> out_unlock:
> 	hyp_spin_unlock(&host_buffers.lock);
> }
>
>
> What do you reckon?

Yes matches my understanding. I also cross checked with FF-A spec authors
to be sure. Now I got to fix that in the driver, currently it releases
buffer unconditionally which is wrong 🙁.

-- 
Regards,
Sudeep