Re: [Xen-devel] [PATCH 2/5] xen/arm: optee: check for preemption while freeing shared buffers

[Julien Grall <julien.grall@arm.com>]

Hi Volodymyr,

On 8/23/19 7:48 PM, Volodymyr Babchuk wrote:
> Now we have limit for one shared buffer size, so we can be sure that
> one call to free_optee_shm_buf() will not free all
> MAX_TOTAL_SMH_BUF_PG pages at once. Thus, we now can check for
> hypercall_preempt_check() in the loop inside
> optee_relinquish_resources() and this will ensure that we are not
> missing preemption.

I am not sure to understand the correlation between the two sentences. 
Even if previously the guest could pin up to MAX_TOTAL_SHM_BUF_PG in one 
call, a well-behaved guest would result to do multiple calls and 
therefore preemption would have been useful.

So I think the commit message needs some rewording.

> 
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> ---
>   xen/arch/arm/tee/optee.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
> index f4fa8a7758..a84ffa3089 100644
> --- a/xen/arch/arm/tee/optee.c
> +++ b/xen/arch/arm/tee/optee.c
> @@ -634,14 +634,14 @@ static int optee_relinquish_resources(struct domain *d)
>       if ( hypercall_preempt_check() )
>           return -ERESTART;
>   
> -    /*
> -     * TODO: Guest can pin up to MAX_TOTAL_SMH_BUF_PG pages and all of
> -     * them will be put in this loop. It is worth considering to
> -     * check for preemption inside the loop.
> -     */
>       list_for_each_entry_safe( optee_shm_buf, optee_shm_buf_tmp,
>                                 &ctx->optee_shm_buf_list, list )
> +    {
> +        if ( hypercall_preempt_check() )

So on the first iteration, you will check twice preemption (one before 
the loop and just entering). hypercall_preempt_check(). The function is 
not entirely free on Arm because of the implementation of 
vgic_vcpu_pending_irq(). So preventing pointless call would be nice.

In this case, the hypercall_preempt_check() before the loop could be 
dropped.

> +            return -ERESTART;
> +
>           free_optee_shm_buf(ctx, optee_shm_buf->cookie);
> +    }
>   
>       if ( hypercall_preempt_check() )
>           return -ERESTART;
> 

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel