Re: [Xen-devel] [PATCH v4 05/10] xen/arm: optee: add std call handling

[Julien Grall <julien.grall@xxxxxxx>]

Hi Volodymyr,

On 20/03/2019 17:42, Volodymyr Babchuk wrote:
> [...]
>
> > > > > +static void put_std_call(struct optee_domain *ctx, struct optee_std_call *call)
> > > > > +{
> > > > > +    spin_lock(&ctx->lock);
> > > > > +    ASSERT(call->in_flight);
> > > > > +    unmap_xen_arg(call);
> > > > Same question for the unmap.
> > > Yeah, in normal circumstances guest should not try to resume call on
> > > another vCPU, because we didn't returned from the original call on
> > > current vCPU. But what if gust will try to do this? There are chances,
> > > that current CPU will unmap buffer that was mapped by other CPU an
> > > instance ago.
> > Wasn't it the point to have the in_flight field? As soon as you set
> > in_flight to true, then only one CPU can have the optee_std_call
> > structure in hand.
> >
> > So, as long as you map/unmap within the section protected by
> > "in_flight", you protected against any race. The lock is only here to
> > protect the field in_flight and the list. Did I miss anything?
> This is partially true. I can call map_xen_arg() after spin_unlock(), yes.
>
> But then I need to call unmap_xen_arg() before spin_lock() in
> put_std_call() function. If there were no ASSERT() that would be okay. But with
> ASSERT() it is looking weird: firstly we are unmapping buffer and the we are
> asserting that we had a right to do this. This is sort of "use before check"
If you hit the ASSERT (or if it where a BUG_ON) then something has already got 
horribly wrong. You will crash in any case, it is not going to matter much 
whether you crash before unmapping or after unmapping.
> And obviously, I can't call ASSERT() without holding the the spinlock. >
> On other hand, ASSERT() is a debugging feature, so we can pretend that is
> not there... If you okay with this, I'll move mapping/unmapping out of
> the spinlocks.
ASSERT/BUG_ON are only here to catch against anything would be horribly wrong if 
we continue (the severity will drive the choice between ASSERT and BUG_ON).
In normal case, this should never be hit. So I would prefer if lock section are 
kept to minimal.
Cheers,

--
Julien Grall

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