Re: [PATCH v5 6/6] xen/arm: ffa: Enable VM to VM without firmware

[Jens Wiklander <jens.wiklander@xxxxxxxxxx>]

Hi,

On Thu, May 22, 2025 at 10:18 AM Bertrand Marquis
<Bertrand.Marquis@xxxxxxx> wrote:
>
> Hi Jens,
>
> > On 22 May 2025, at 10:00, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote:
> >
> > Hi Bertrand,
> >
> > On Wed, Apr 16, 2025 at 9:40 AM 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 OP-TEE running in the secure world and using the non FF-A
> >> communication system, having CONFIG_FFA_VM_TO_VM could be non functional
> >> (if optee is probed first) or OP-TEE 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 no
> >> firmware, rework the notification handling and modify the global flag to
> >> only be used as check for firmware notification support instead.
> >>
> >> Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> >> ---
> >> Changes in v5:
> >> - init ctx list when there is no firmware
> >> - rework init a bit to prevent duplicates
> >> - Remove Jens R-b due to changes done
> >> Changes in v4:
> >> - Fix Optee to OP-TEE in commit message
> >> - Add Jens R-b
> >> Changes in v3:
> >> - fix typos in commit message
> >> - add spaces around <<
> >> - move notification id fix back into buffer full patch
> >> - fix | position in if
> >> Changes in v2:
> >> - replace ifdef with IS_ENABLED when possible
> >> ---
> >> xen/arch/arm/tee/ffa.c       |  24 ++++++--
> >> xen/arch/arm/tee/ffa_notif.c | 104 ++++++++++++++++-------------------
> >> 2 files changed, 67 insertions(+), 61 deletions(-)
> >>
> >> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
> >> index c1c4c0957091..b86c88cefa8c 100644
> >> --- a/xen/arch/arm/tee/ffa.c
> >> +++ b/xen/arch/arm/tee/ffa.c
> >> @@ -342,8 +342,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
> >> @@ -579,11 +580,8 @@ static bool ffa_probe(void)
> >>         goto err_rxtx_destroy;
> >>
> >>     ffa_notif_init();
> >> -    INIT_LIST_HEAD(&ffa_teardown_head);
> >> -    INIT_LIST_HEAD(&ffa_ctx_head);
> >> -    init_timer(&ffa_teardown_timer, ffa_teardown_timer_callback, NULL, 0);
> >>
> >> -    return true;
> >> +    goto exit;
> >>
> >> err_rxtx_destroy:
> >>     ffa_rxtx_destroy();
> >> @@ -592,6 +590,22 @@ err_no_fw:
> >>     bitmap_zero(ffa_fw_abi_supported, FFA_ABI_BITMAP_SIZE);
> >>     printk(XENLOG_WARNING "ARM FF-A No firmware support\n");
> >>
> >> +exit:
> >> +    if ( IS_ENABLED(CONFIG_FFA_VM_TO_VM) || ffa_fw_version )
> >> +    {
> >> +        INIT_LIST_HEAD(&ffa_teardown_head);
> >> +        INIT_LIST_HEAD(&ffa_ctx_head);
> >> +        init_timer(&ffa_teardown_timer, ffa_teardown_timer_callback, 
> >> NULL, 0);
> >> +    }
> >> +
> >> +    if ( IS_ENABLED(CONFIG_FFA_VM_TO_VM) )
> >> +    {
> >> +        printk(XENLOG_INFO "ARM FF-A only available between VMs\n");
> >
> > This should only be printed if ffa_fw_version == 0
>
> Right i will fix but ...
>
> >
> >> +        return true;
> >> +    }
> >> +    else if ( ffa_fw_version )
> >
> > The else isn't needed.
>
> the else is needed so that we return true and not false.

I meant the "else" isn't needed, the "if" is still needed, as you explain.

>
> We have 3 cases:
> - firmware is there: return true
> - firmware not there but vm to vm enable: return true
> - otherwise: return false
>
> I will modify it like this to make it clearer:
> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
> index 57b648a22840..768b4e9ec968 100644
> --- a/xen/arch/arm/tee/ffa.c
> +++ b/xen/arch/arm/tee/ffa.c
> @@ -601,13 +601,13 @@ exit:
>          init_timer(&ffa_teardown_timer, ffa_teardown_timer_callback, NULL, 
> 0);
>      }
>
> -    if ( IS_ENABLED(CONFIG_FFA_VM_TO_VM) )
> +    if ( ffa_fw_version )
> +        return true;
> +    else if ( IS_ENABLED(CONFIG_FFA_VM_TO_VM) )
>      {
>          printk(XENLOG_INFO "ARM FF-A only available between VMs\n");
>          return true;
>      }
> -    else if ( ffa_fw_version )
> -        return true;
>
>      return false;
>  }
>
> Tell me if you agree.

Yes, this is an improvement. A return at the end of an if block makes
the eventual following "else" redundant. I suppose there are coding
styles where it's still preferred. I'm not sure if that applies in
Xen, though.

Cheers,
Jens

>
> Cheers
> Bertrand
>