|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2 5/5] xen/arm: ffa: Enable VM to VM without firmware
Hi Bertrand,
On Fri, Mar 21, 2025 at 2:47 PM Bertrand Marquis
<Bertrand.Marquis@xxxxxxx> wrote:
>
> Hi Jens,
>
> > On 21 Mar 2025, at 11:09, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote:
> >
> > Hi,
> >
> > On Fri, Mar 21, 2025 at 10:25 AM Bertrand Marquis
> > <Bertrand.Marquis@xxxxxxx> wrote:
> >>
> >> Hi Jens,
> >>
> >>> On 21 Mar 2025, at 09:55, Jens Wiklander <jens.wiklander@xxxxxxxxxx>
> >>> wrote:
> >>>
> >>> Hi Bertrand,
> >>>
> >>> On Mon, Mar 10, 2025 at 3:51 PM Bertrand Marquis
> >>> <bertrand.marquis@xxxxxxx> wrote:
> >>>>
> >>>> When VM to VM support is activated and there is no suitable FF-A support
> >>>> in the firmware, enable FF-A support for VMs to allow using it for VM to
> >>>> VM communications.
> >>>> If there is Optee running in the secure world and using the non FF-A
> >>>
> >>> It's spelled OP-TEE in text, and optee or OPTEE in identifiers.
> >>
> >> ack
> >>
> >>>
> >>>> communication system, having CONFIG_FFA_VM_TO_VM could be non functional
> >>>> (if optee is probed first) or Optee could be non functional (if FF-A is
> >>>> probed first) so it is not recommended to activate the configuration
> >>>> option for such systems.
> >>>>
> >>>> To make buffer full notification work between VMs when there is not
> >>>
> >>> s/not/no/
> >>
> >> ack
> >>
> >>>
> >>>> firmware, rework the notification handling and modify the global flag to
> >>>> only be used as check for firmware notification support instead.
> >>>>
> >>>> Modify part_info_get to return the list of VMs when there is no firmware
> >>>> support.
> >>>>
> >>>> Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> >>>> ---
> >>>> Changes in v2:
> >>>> - replace ifdef with IS_ENABLED when possible
> >>>> ---
> >>>> xen/arch/arm/tee/ffa.c | 12 +++-
> >>>> xen/arch/arm/tee/ffa_notif.c | 114 ++++++++++++++++----------------
> >>>> xen/arch/arm/tee/ffa_partinfo.c | 3 +-
> >>>> 3 files changed, 69 insertions(+), 60 deletions(-)
> >>>>
> >>>> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
> >>>> index 3bbdd7168a6b..f6582d2e855a 100644
> >>>> --- a/xen/arch/arm/tee/ffa.c
> >>>> +++ b/xen/arch/arm/tee/ffa.c
> >>>> @@ -324,8 +324,9 @@ static int ffa_domain_init(struct domain *d)
> >>>> struct ffa_ctx *ctx;
> >>>> int ret;
> >>>>
> >>>> - if ( !ffa_fw_version )
> >>>> + if ( !IS_ENABLED(CONFIG_FFA_VM_TO_VM) && !ffa_fw_version )
> >>>> return -ENODEV;
> >>>> +
> >>>> /*
> >>>> * We are using the domain_id + 1 as the FF-A ID for VMs as FF-A ID 0
> >>>> is
> >>>> * reserved for the hypervisor and we only support secure endpoints
> >>>> using
> >>>> @@ -549,6 +550,15 @@ err_no_fw:
> >>>> bitmap_zero(ffa_fw_abi_supported, FFA_ABI_BITMAP_SIZE);
> >>>> printk(XENLOG_WARNING "ARM FF-A No firmware support\n");
> >>>>
> >>>> + if ( IS_ENABLED(CONFIG_FFA_VM_TO_VM) )
> >>>> + {
> >>>> + INIT_LIST_HEAD(&ffa_teardown_head);
> >>>> + init_timer(&ffa_teardown_timer, ffa_teardown_timer_callback,
> >>>> NULL, 0);
> >>>> +
> >>>> + printk(XENLOG_INFO "ARM FF-A only available between VMs\n");
> >>>> + return true;
> >>>> + }
> >>>> +
> >>>> return false;
> >>>> }
> >>>>
> >>>> diff --git a/xen/arch/arm/tee/ffa_notif.c b/xen/arch/arm/tee/ffa_notif.c
> >>>> index d19aa5c5bef6..0673e53a9def 100644
> >>>> --- a/xen/arch/arm/tee/ffa_notif.c
> >>>> +++ b/xen/arch/arm/tee/ffa_notif.c
> >>>> @@ -16,7 +16,7 @@
> >>>>
> >>>> #include "ffa_private.h"
> >>>>
> >>>> -static bool __ro_after_init notif_enabled;
> >>>> +static bool __ro_after_init fw_notif_enabled;
> >>>> static unsigned int __ro_after_init notif_sri_irq;
> >>>>
> >>>> int ffa_handle_notification_bind(struct cpu_user_regs *regs)
> >>>> @@ -27,21 +27,17 @@ int ffa_handle_notification_bind(struct
> >>>> cpu_user_regs *regs)
> >>>> uint32_t bitmap_lo = get_user_reg(regs, 3);
> >>>> uint32_t bitmap_hi = get_user_reg(regs, 4);
> >>>>
> >>>> - if ( !notif_enabled )
> >>>> - return FFA_RET_NOT_SUPPORTED;
> >>>> -
> >>>> if ( (src_dst & 0xFFFFU) != ffa_get_vm_id(d) )
> >>>> return FFA_RET_INVALID_PARAMETERS;
> >>>>
> >>>> if ( flags ) /* Only global notifications are supported */
> >>>> return FFA_RET_DENIED;
> >>>>
> >>>> - /*
> >>>> - * We only support notifications from SP so no need to check the
> >>>> sender
> >>>> - * endpoint ID, the SPMC will take care of that for us.
> >>>> - */
> >>>> - return ffa_simple_call(FFA_NOTIFICATION_BIND, src_dst, flags,
> >>>> bitmap_lo,
> >>>> - bitmap_hi);
> >>>> + if ( FFA_ID_IS_SECURE(src_dst>>16) && fw_notif_enabled )
> >>>
> >>> Please add space before and after '>>', here and in the function below
> >>> in this patch.
> >>
> >> ack
> >>
> >>>
> >>>> + return ffa_simple_call(FFA_NOTIFICATION_BIND, src_dst, flags,
> >>>> + bitmap_lo, bitmap_hi);
> >>>> +
> >>>> + return FFA_RET_NOT_SUPPORTED;
> >>>> }
> >>>>
> >>>> int ffa_handle_notification_unbind(struct cpu_user_regs *regs)
> >>>> @@ -51,32 +47,34 @@ int ffa_handle_notification_unbind(struct
> >>>> cpu_user_regs *regs)
> >>>> uint32_t bitmap_lo = get_user_reg(regs, 3);
> >>>> uint32_t bitmap_hi = get_user_reg(regs, 4);
> >>>>
> >>>> - if ( !notif_enabled )
> >>>> - return FFA_RET_NOT_SUPPORTED;
> >>>> -
> >>>> if ( (src_dst & 0xFFFFU) != ffa_get_vm_id(d) )
> >>>> return FFA_RET_INVALID_PARAMETERS;
> >>>>
> >>>> - /*
> >>>> - * We only support notifications from SP so no need to check the
> >>>> - * destination endpoint ID, the SPMC will take care of that for us.
> >>>> - */
> >>>> - return ffa_simple_call(FFA_NOTIFICATION_UNBIND, src_dst, 0,
> >>>> bitmap_lo,
> >>>> - bitmap_hi);
> >>>> + if ( FFA_ID_IS_SECURE(src_dst>>16) && fw_notif_enabled )
> >>>> + return ffa_simple_call(FFA_NOTIFICATION_UNBIND, src_dst, 0,
> >>>> bitmap_lo,
> >>>> + bitmap_hi);
> >>>> +
> >>>> + return FFA_RET_NOT_SUPPORTED;
> >>>> }
> >>>>
> >>>> void ffa_handle_notification_info_get(struct cpu_user_regs *regs)
> >>>> {
> >>>> struct domain *d = current->domain;
> >>>> struct ffa_ctx *ctx = d->arch.tee;
> >>>> + bool notif_pending = false;
> >>>>
> >>>> - if ( !notif_enabled )
> >>>> + if ( !IS_ENABLED(CONFIG_FFA_VM_TO_VM) && !fw_notif_enabled )
> >>>> {
> >>>> ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
> >>>> return;
> >>>> }
> >>>>
> >>>> - if ( test_and_clear_bool(ctx->notif.secure_pending) )
> >>>> + notif_pending = ctx->notif.secure_pending;
> >>>> +#ifdef CONFIG_FFA_VM_TO_VM
> >>>> + notif_pending |= ctx->notif.buff_full_pending;
> >>>> +#endif
> >>>
> >>> Shouldn't ctx->notif.secure_pending and ctx->notif.secure_pending be
> >>> cleared also, like:
> >>> notif_pending = test_and_clear_bool(ctx->notif.secure_pending) ||
> >>> test_and_clear_bool(ctx->notif.buff_full_pending);
> >>
> >> Notification info get is returning who has pending notification but only
> >> notification get should erase pending notifications.
> >
> > FFA_NOTIFICATION_INFO_GET can return a "More pending notifications
> > flag" in w2/x2 to inform the caller that it should call
> > FFA_NOTIFICATION_INFO_GET again to get the remaining receiver
> > endpoints. How can the ABI know where to resume the next time if the
> > previous pending receivers aren't cleared?
>
> I just checked the specification and you are right.
> It is explicitly saying that "Information about pending notifications is
> returned only once".
>
> >
> > The more pending notifications flag will not be needed here as we're
> > dealing with a single endpoint at a time so it might be more of a
> > philosophical question. I don't think it causes problems for the guest
> > to keep secure_pending unchanged for FFA_NOTIFICATION_INFO_GET, but we
> > should mention the changed behaviour in the commit message.
> >
>
> I agree I should discard the secure_pending flag in the implementation but
> I need to find a solution for the buffer full notification as I still need to
> signal
> it in notification get even if notification info get was called.
>
> I will do the following:
> - change secure_pending into pending_notif.
> - set pending_notif when current secure_pending is set
> - set pending_notif and buff_full_pending on indirect message
> - clean pending_notif in notif_info_get
> - clean pending_notif and buff_full in notif_get
>
> Do you agree this is the right path ?
Yes, this is the way. :-)
Cheers,
Jens
>
> Cheers
> Bertrand
>
> > Cheers,
> > Jens
> >
> >>
> >>>
> >>>> +
> >>>> + if ( notif_pending )
> >>>> {
> >>>> /* A pending global notification for the guest */
> >>>> ffa_set_regs(regs, FFA_SUCCESS_64, 0,
> >>>> @@ -103,7 +101,7 @@ void ffa_handle_notification_get(struct
> >>>> cpu_user_regs *regs)
> >>>> uint32_t w6 = 0;
> >>>> uint32_t w7 = 0;
> >>>>
> >>>> - if ( !notif_enabled )
> >>>> + if ( !IS_ENABLED(CONFIG_FFA_VM_TO_VM) && !fw_notif_enabled )
> >>>> {
> >>>> ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
> >>>> return;
> >>>> @@ -115,7 +113,8 @@ void ffa_handle_notification_get(struct
> >>>> cpu_user_regs *regs)
> >>>> return;
> >>>> }
> >>>>
> >>>> - if ( flags & ( FFA_NOTIF_FLAG_BITMAP_SP | FFA_NOTIF_FLAG_BITMAP_SPM
> >>>> ) )
> >>>> + if ( fw_notif_enabled && (flags & ( FFA_NOTIF_FLAG_BITMAP_SP
> >>>> + | FFA_NOTIF_FLAG_BITMAP_SPM )) )
> >>>
> >>> Please end the previous line with the '|' operator.
> >>
> >> ack
> >>
> >>>
> >>>> {
> >>>> struct arm_smccc_1_2_regs arg = {
> >>>> .a0 = FFA_NOTIFICATION_GET,
> >>>> @@ -170,15 +169,14 @@ int ffa_handle_notification_set(struct
> >>>> cpu_user_regs *regs)
> >>>> uint32_t bitmap_lo = get_user_reg(regs, 3);
> >>>> uint32_t bitmap_hi = get_user_reg(regs, 4);
> >>>>
> >>>> - if ( !notif_enabled )
> >>>> - return FFA_RET_NOT_SUPPORTED;
> >>>> -
> >>>> if ( (src_dst >> 16) != ffa_get_vm_id(d) )
> >>>> return FFA_RET_INVALID_PARAMETERS;
> >>>>
> >>>> - /* Let the SPMC check the destination of the notification */
> >>>> - return ffa_simple_call(FFA_NOTIFICATION_SET, src_dst, flags,
> >>>> bitmap_lo,
> >>>> - bitmap_hi);
> >>>> + if ( FFA_ID_IS_SECURE(src_dst>>16) && fw_notif_enabled )
> >>>> + return ffa_simple_call(FFA_NOTIFICATION_SET, src_dst, flags,
> >>>> bitmap_lo,
> >>>> + bitmap_hi);
> >>>> +
> >>>> + return FFA_RET_NOT_SUPPORTED;
> >>>> }
> >>>>
> >>>> #ifdef CONFIG_FFA_VM_TO_VM
> >>>> @@ -190,7 +188,7 @@ void ffa_raise_rx_buffer_full(struct domain *d)
> >>>> return;
> >>>>
> >>>> if ( !test_and_set_bool(ctx->notif.buff_full_pending) )
> >>>> - vgic_inject_irq(d, d->vcpu[0], notif_sri_irq, true);
> >>>> + vgic_inject_irq(d, d->vcpu[0], GUEST_FFA_NOTIF_PEND_INTR_ID,
> >>>> true);
> >>>
> >>> Please move this to the patch "xen/arm: ffa: Add buffer full
> >>> notification support"
> >>
> >> Ack
> >>
> >>>
> >>>> }
> >>>> #endif
> >>>>
> >>>> @@ -363,7 +361,7 @@ void ffa_notif_init_interrupt(void)
> >>>> {
> >>>> int ret;
> >>>>
> >>>> - if ( notif_enabled && notif_sri_irq < NR_GIC_SGI )
> >>>> + if ( fw_notif_enabled && notif_sri_irq < NR_GIC_SGI )
> >>>> {
> >>>> /*
> >>>> * An error here is unlikely since the primary CPU has already
> >>>> @@ -394,41 +392,41 @@ void ffa_notif_init(void)
> >>>> int ret;
> >>>>
> >>>> /* Only enable fw notification if all ABIs we need are supported */
> >>>> - if ( !(ffa_fw_supports_fid(FFA_NOTIFICATION_BITMAP_CREATE) &&
> >>>> - ffa_fw_supports_fid(FFA_NOTIFICATION_BITMAP_DESTROY) &&
> >>>> - ffa_fw_supports_fid(FFA_NOTIFICATION_GET) &&
> >>>> - ffa_fw_supports_fid(FFA_NOTIFICATION_INFO_GET_64)) )
> >>>> - return;
> >>>> -
> >>>> - arm_smccc_1_2_smc(&arg, &resp);
> >>>> - if ( resp.a0 != FFA_SUCCESS_32 )
> >>>> - return;
> >>>> -
> >>>> - irq = resp.a2;
> >>>> - notif_sri_irq = irq;
> >>>> - if ( irq >= NR_GIC_SGI )
> >>>> - irq_set_type(irq, IRQ_TYPE_EDGE_RISING);
> >>>> - ret = request_irq(irq, 0, notif_irq_handler, "FF-A notif", NULL);
> >>>> - if ( ret )
> >>>> + if ( ffa_fw_supports_fid(FFA_NOTIFICATION_BITMAP_CREATE) &&
> >>>> + ffa_fw_supports_fid(FFA_NOTIFICATION_BITMAP_DESTROY) &&
> >>>> + ffa_fw_supports_fid(FFA_NOTIFICATION_GET) &&
> >>>> + ffa_fw_supports_fid(FFA_NOTIFICATION_INFO_GET_64) )
> >>>> {
> >>>> - printk(XENLOG_ERR "ffa: request_irq irq %u failed: error %d\n",
> >>>> - irq, ret);
> >>>> - return;
> >>>> - }
> >>>> + arm_smccc_1_2_smc(&arg, &resp);
> >>>> + if ( resp.a0 != FFA_SUCCESS_32 )
> >>>> + return;
> >>>>
> >>>> - notif_enabled = true;
> >>>> + irq = resp.a2;
> >>>> + notif_sri_irq = irq;
> >>>> + if ( irq >= NR_GIC_SGI )
> >>>> + irq_set_type(irq, IRQ_TYPE_EDGE_RISING);
> >>>> + ret = request_irq(irq, 0, notif_irq_handler, "FF-A notif",
> >>>> NULL);
> >>>> + if ( ret )
> >>>> + {
> >>>> + printk(XENLOG_ERR "ffa: request_irq irq %u failed: error
> >>>> %d\n",
> >>>> + irq, ret);
> >>>> + return;
> >>>> + }
> >>>> + fw_notif_enabled = true;
> >>>> + }
> >>>> }
> >>>>
> >>>> int ffa_notif_domain_init(struct domain *d)
> >>>> {
> >>>> int32_t res;
> >>>>
> >>>> - if ( !notif_enabled )
> >>>> - return 0;
> >>>> + if ( fw_notif_enabled )
> >>>> + {
> >>>>
> >>>> - res = ffa_notification_bitmap_create(ffa_get_vm_id(d),
> >>>> d->max_vcpus);
> >>>> - if ( res )
> >>>> - return -ENOMEM;
> >>>> + res = ffa_notification_bitmap_create(ffa_get_vm_id(d),
> >>>> d->max_vcpus);
> >>>> + if ( res )
> >>>> + return -ENOMEM;
> >>>> + }
> >>>>
> >>>> return 0;
> >>>> }
> >>>> @@ -439,6 +437,6 @@ void ffa_notif_domain_destroy(struct domain *d)
> >>>> * Call bitmap_destroy even if bitmap create failed as the SPMC will
> >>>> * return a DENIED error that we will ignore.
> >>>> */
> >>>> - if ( notif_enabled )
> >>>> + if ( fw_notif_enabled )
> >>>> ffa_notification_bitmap_destroy(ffa_get_vm_id(d));
> >>>> }
> >>>> diff --git a/xen/arch/arm/tee/ffa_partinfo.c
> >>>> b/xen/arch/arm/tee/ffa_partinfo.c
> >>>> index 7af1eca2d0c4..291396c8f635 100644
> >>>> --- a/xen/arch/arm/tee/ffa_partinfo.c
> >>>> +++ b/xen/arch/arm/tee/ffa_partinfo.c
> >>>> @@ -130,7 +130,8 @@ void ffa_handle_partition_info_get(struct
> >>>> cpu_user_regs *regs)
> >>>> goto out;
> >>>> }
> >>>>
> >>>> - if ( !ffa_fw_supports_fid(FFA_PARTITION_INFO_GET) )
> >>>> + if ( !IS_ENABLED(CONFIG_FFA_VM_TO_VM) &&
> >>>
> >>> Doesn't this belong in the patch "xen/arm: ffa: Introduce VM to VM
> >>> support"?
> >>> And wouldn't ffa_vm_count make more sense?
> >>
> >> ack. I will modify that and fold it into the VM to VM support patch.
> >>
> >> Cheers
> >> Bertrand
> >>
> >>>
> >>> Cheers,
> >>> Jens
> >>>
> >>>> + !ffa_fw_supports_fid(FFA_PARTITION_INFO_GET) )
> >>>> {
> >>>> /* Just give an empty partition list to the caller */
> >>>> ret = FFA_RET_OK;
> >>>> --
> >>>> 2.47.1
>
>
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |