From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCH v15 01/11] multicall: add no preemption ability between two calls Date: Tue, 09 Sep 2014 14:15:59 +0100 Message-ID: <540F19AF0200007800032AD1@mail.emea.novell.com> References: <1409906249-6057-1-git-send-email-chao.p.peng@linux.intel.com> <1409906249-6057-2-git-send-email-chao.p.peng@linux.intel.com> <5409947C.3020402@citrix.com> <20140909064359.GF15872@pengc-linux> <540EF4EC0200007800032903@mail.emea.novell.com> <540EDB9B.6080908@citrix.com> <540F05ED020000780003299E@mail.emea.novell.com> <540EF624.4020200@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <540EF624.4020200@citrix.com> Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Andrew Cooper , Chao Peng Cc: keir@xen.org, Ian.Campbell@citrix.com, stefano.stabellini@eu.citrix.com, George.Dunlap@eu.citrix.com, Ian.Jackson@eu.citrix.com, xen-devel@lists.xen.org, dgdegra@tycho.nsa.gov List-Id: xen-devel@lists.xenproject.org >>> On 09.09.14 at 14:44, wrote: > On 09/09/14 12:51, Jan Beulich wrote: >>>>> On 09.09.14 at 12:51, wrote: >>> On 09/09/14 11:39, Jan Beulich wrote: >>>>>>> On 09.09.14 at 08:43, wrote: >>>>> 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 >>>>>>> --- >>>>>>> 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 = ¤t->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). >>>> The I suppose you didn't really understand Andrew's comment: I >>>> don't think he was suggesting to drop the approach, but instead >>>> to implement it properly (read: securely). >>> That is certainly one part of it. >>> >>> However, there is the other open question (dropped from this context) of >>> how to deal with a multicall which has NO_PREEMT set, which itself >>> preempts, and I don't have a good answer for this. >> The pretty natural answer to this is - the specific handler knows >> best what to do. > > Given our past history at retrofitting preempting into existing > hypercalls, the multicaller has no idea whether the ops they have > selected will preempt or not, and no way to guarentee that the behaviour > will stay the same in future. > > The multicall dispatches to the regular hypercall handlers, which > (cant?) They can - current->mc_state.flags has _MCSF_in_multicall set. > and certainly shouldn't distinguish between a regular hypercall > and multicall. I agree with this. Yet it's a bug in the caller to request no preemption at this layer for a constituent hypercall that can itself preempt. But that's only a problem for the caller, not for the hypervisor. > As I have been looking through this code, I have noticed that the NDEBUG > parameter corruption will break half of our existing preemption logic, > which does use some of the parameters to hold preemption information. Certainly not - call_list is being copied over a second time a few lines after that NDEBUG section. Jan