|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v5 6/6] xen/arm: ffa: Enable VM to VM without firmware
Hi Bertrand,
On Thu, May 22, 2025 at 11:11 AM Bertrand Marquis
<Bertrand.Marquis@xxxxxxx> wrote:
>
> Hi Jens,
>
> > On 22 May 2025, at 10:30, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote:
> >
> > 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.
>
> I now get what you mean and you would like the return false to be in a else.
>
> Relooking at the code, i actually do not like the fact that we do so much in
> exit and i think i can move a bit the code to be clearer:
> - put the fw init in a sub function
> - create a vm_to_vm init function
> - in probe call both functions and do the common init part if at least one
> succeded
I was starting to think along those lines, too. :-)
>
> Something like this:
> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
> index 57b648a22840..42dfc71a12d7 100644
> --- a/xen/arch/arm/tee/ffa.c
> +++ b/xen/arch/arm/tee/ffa.c
> @@ -478,38 +478,12 @@ static void ffa_init_secondary(void)
> ffa_notif_init_interrupt();
> }
>
> -static bool ffa_probe(void)
> +static bool ffa_probe_fw(void)
> {
> uint32_t vers;
> unsigned int major_vers;
> unsigned int minor_vers;
>
> - /*
> - * FF-A often works in units of 4K pages and currently it's assumed
> - * that we can map memory using that granularity. See also the comment
> - * above the FFA_PAGE_SIZE define.
> - *
> - * It is possible to support a PAGE_SIZE larger than 4K in Xen, but
> - * until that is fully handled in this code make sure that we only use
> - * 4K page sizes.
> - */
> - BUILD_BUG_ON(PAGE_SIZE != FFA_PAGE_SIZE);
> -
> - printk(XENLOG_INFO "ARM FF-A Mediator version %u.%u\n",
> - FFA_MY_VERSION_MAJOR, FFA_MY_VERSION_MINOR);
> -
> - if ( IS_ENABLED(CONFIG_FFA_VM_TO_VM) )
> - {
> - /*
> - * When FFA VM to VM is enabled, the current implementation does not
> - * offer any way to limit which VM can communicate with which VM
> using
> - * FF-A.
> - * Signal this in the xen console and taint the system as insecure.
> - * TODO: Introduce a solution to limit what a VM can do through FFA.
> - */
> - printk(XENLOG_ERR "ffa: VM to VM is enabled, system is insecure
> !!\n");
> - add_taint(TAINT_MACHINE_INSECURE);
> - }
> /*
> * psci_init_smccc() updates this value with what's reported by EL-3
> * or secure world.
> @@ -528,11 +502,6 @@ static bool ffa_probe(void)
> goto err_no_fw;
> }
>
> - /* Some sanity check in case we update the version we support */
> - BUILD_BUG_ON(FFA_MIN_SPMC_VERSION > FFA_MY_VERSION);
> - BUILD_BUG_ON(FFA_VERSION_MAJOR(FFA_MIN_SPMC_VERSION) !=
> - FFA_MY_VERSION_MAJOR);
> -
> major_vers = FFA_VERSION_MAJOR(vers);
> minor_vers = FFA_VERSION_MINOR(vers);
>
> @@ -584,7 +553,7 @@ static bool ffa_probe(void)
>
> ffa_notif_init();
>
> - goto exit;
> + return true;
>
> err_rxtx_destroy:
> ffa_rxtx_destroy();
> @@ -593,23 +562,59 @@ 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);
> - }
> + return false;
> +}
>
> - if ( IS_ENABLED(CONFIG_FFA_VM_TO_VM) )
> - {
> +static bool ffa_probe_vm_to_vm(void)
> +{
> + if ( !IS_ENABLED(CONFIG_FFA_VM_TO_VM) )
> + return false;
> +
> + /*
> + * When FFA VM to VM is enabled, the current implementation does not
> + * offer any way to limit which VM can communicate with which VM using
> + * FF-A.
> + * Signal this in the xen console and taint the system as insecure.
> + * TODO: Introduce a solution to limit what a VM can do through FFA.
> + */
> + printk(XENLOG_ERR "ffa: VM to VM is enabled, system is insecure !!\n");
> + add_taint(TAINT_MACHINE_INSECURE);
> +
> + return true;
> +}
> +
> +static bool ffa_probe(void)
> +{
> + /*
> + * FF-A often works in units of 4K pages and currently it's assumed
> + * that we can map memory using that granularity. See also the comment
> + * above the FFA_PAGE_SIZE define.
> + *
> + * It is possible to support a PAGE_SIZE larger than 4K in Xen, but
> + * until that is fully handled in this code make sure that we only use
> + * 4K page sizes.
> + */
> + BUILD_BUG_ON(PAGE_SIZE != FFA_PAGE_SIZE);
> +
> + /* Some sanity check in case we update the version we support */
> + BUILD_BUG_ON(FFA_MIN_SPMC_VERSION > FFA_MY_VERSION);
> + BUILD_BUG_ON(FFA_VERSION_MAJOR(FFA_MIN_SPMC_VERSION) !=
> + FFA_MY_VERSION_MAJOR);
> +
> + printk(XENLOG_INFO "ARM FF-A Mediator version %u.%u\n",
> + FFA_MY_VERSION_MAJOR, FFA_MY_VERSION_MINOR);
> +
> + if ( !ffa_probe_fw() && !ffa_probe_vm_to_vm() )
> + return false;
> +
> + if ( !ffa_fw_version )
> printk(XENLOG_INFO "ARM FF-A only available between VMs\n");
> - return true;
> - }
> - else if ( ffa_fw_version )
> - return true;
>
> - return false;
> + 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;
> }
>
> static const struct tee_mediator_ops ffa_ops =
>
> I think this makes the code cleaner as we also get the proper error handling
> with goto
> inside the fw probe instead of the previous one which was trying to handle
> both cases.
>
> What do you think ?
This is good. It's much easier to follow.
Cheers,
Jens
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |