Re: [PATCH v15 01/11] multicall: add no preemption ability between two calls

[Chao Peng <chao.p.peng@linux.intel.com>]

On Fri, Sep 05, 2014 at 11:46:20AM +0100, Andrew Cooper wrote:
> On 05/09/14 09:37, Chao Peng wrote:
> > Add a flag to indicate if the execution can be preempted between two
> > calls. If not specified, stay preemptable.
> >
> > Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
> > ---
> >  xen/common/multicall.c   |    5 ++++-
> >  xen/include/public/xen.h |    4 ++++
> >  2 files changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/common/multicall.c b/xen/common/multicall.c
> > index fa9d910..83b96eb 100644
> > --- a/xen/common/multicall.c
> > +++ b/xen/common/multicall.c
> > @@ -40,6 +40,7 @@ do_multicall(
> >      struct mc_state *mcs = &current->mc_state;
> >      uint32_t         i;
> >      int              rc = 0;
> > +    bool_t           preemptable = 0;
> >  
> >      if ( unlikely(__test_and_set_bit(_MCSF_in_multicall, &mcs->flags)) )
> >      {
> > @@ -52,7 +53,7 @@ do_multicall(
> >  
> >      for ( i = 0; !rc && i < nr_calls; i++ )
> >      {
> > -        if ( i && hypercall_preempt_check() )
> > +        if ( preemptable && hypercall_preempt_check() )
> >              goto preempted;
> >  
> >          if ( unlikely(__copy_from_guest(&mcs->call, call_list, 1)) )
> > @@ -61,6 +62,8 @@ do_multicall(
> >              break;
> >          }
> >  
> > +        preemptable = mcs->call.flags & MC_NO_PREEMPT;
> > +
> 
> Please consider what would happen if a malicious guest set NO_PREEMPT on
> every multicall entry.

OK, I see. My direct purpose here is to support batch operations for
XENPF_resource_op added in next patch. Recall what Jan suggested in v14
comments, we have 3 possible ways to support XENPF_resource_op batch:
1) Add a field in the xenpf_resource_op to indicate the iteration;
2) Fiddle multicall mechanism, just like this patch;
3) Add a brand new hypercall.

So perhaps I will give up option 2) before I can see any improvement
here. While option 3) is aggressive, so I'd go option 1) through I also
don't quite like it (Totally because the iteration is transparent for user).

Chao
> 
> IMO, two back-to-back entries whith NO_PREEMPT set is grounds for an
> immediate hypercall failure.
> 
> Furthermore, what happens if one of the multicall operations pre-empts
> of its own accord?
> 
> ~Andrew
> 
> >          trace_multicall_call(&mcs->call);
> >  
> >          do_multicall_call(&mcs->call);
> > diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
> > index a6a2092..3f8b908 100644
> > --- a/xen/include/public/xen.h
> > +++ b/xen/include/public/xen.h
> > @@ -550,10 +550,14 @@ DEFINE_XEN_GUEST_HANDLE(mmu_update_t);
> >  struct multicall_entry {
> >      xen_ulong_t op, result;
> >      xen_ulong_t args[6];
> > +    uint32_t flags;
> >  };
> >  typedef struct multicall_entry multicall_entry_t;
> >  DEFINE_XEN_GUEST_HANDLE(multicall_entry_t);
> >  
> > +/* These flags passed in the 'flags' field of multicall_entry_t. */
> > +#define MC_NO_PREEMPT    (1<<0)  /* Can't be preempted before next entry? */
> > +
> >  #if __XEN_INTERFACE_VERSION__ < 0x00040400
> >  /*
> >   * Event channel endpoints per domain (when using the 2-level ABI):
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel