From mboxrd@z Thu Jan 1 00:00:00 1970 From: Keir Fraser Subject: Re: [PATCH v7 1/2] xen: export do_block, add a parameter to enable local events delivery Date: Mon, 22 Apr 2013 12:38:48 +0100 Message-ID: References: Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Stefano Stabellini Cc: Julien Grall , "Tim (Xen.org)" , Ian Campbell , Jan Beulich , xen-devel List-Id: xen-devel@lists.xenproject.org On 22/04/2013 11:20, "Stefano Stabellini" wrote: > On Fri, 19 Apr 2013, Keir Fraser wrote: >> On 19/04/2013 17:23, "Stefano Stabellini" >> wrote: >> >>> On Fri, 19 Apr 2013, Jan Beulich wrote: >>>>>>> On 19.04.13 at 16:06, Stefano Stabellini >>>>>>> wrote: >>>>> -static long do_block(void) >>>>> +int do_block(int event_delivery_enable) >>>> >>>> In the previous version you didn't need this new parameter, so >>>> what changed since then? >>> >>> I changed the ARM implementation of local_event_delivery_enable: now it >>> clears PSR_IRQ_MASK (the interrupt flag). That is the correct behavior >>> for the implementation of the SCHEDOP_block hypercall but it is not the >>> right behavior for the WFI trap handler. In order to reuse this code, I >>> had to add the event_delivery_enable parameter. >> >> Given that basically you need special versions of both local_events_*() >> calls in do_block() (in one case you nop it out, and in the other it is a >> special case which ignores the irq mask), I think perhaps you are better >> with your own version of do_block() rather than code sharing. It's only a >> small function and it skanks up every caller in common code, and you're >> practically abusing do_block() by trying to play nicely with it. > > It is a small function, but the implementation is not trivial. > What if I do something like: Yes, I like this well enough. -- Keir > > diff --git a/xen/common/schedule.c b/xen/common/schedule.c > index c1cd3d0..489c23d 100644 > --- a/xen/common/schedule.c > +++ b/xen/common/schedule.c > @@ -677,11 +677,10 @@ int vcpu_set_affinity(struct vcpu *v, const cpumask_t > *affinity) > } > > /* Block the currently-executing domain until a pertinent event occurs. */ > -static long do_block(void) > +long vcpu_block(void) > { > struct vcpu *v = current; > > - local_event_delivery_enable(); > set_bit(_VPF_blocked, &v->pause_flags); > > /* Check for events /after/ blocking: avoids wakeup waiting race. */ > @@ -698,6 +697,12 @@ static long do_block(void) > return 0; > } > > +long vcpu_block_enable_events(void) > +{ > + local_event_delivery_enable(); > + return vcpu_block(); > +} > + > static long do_poll(struct sched_poll *sched_poll) > { > struct vcpu *v = current; > @@ -870,7 +875,7 @@ long do_sched_op_compat(int cmd, unsigned long arg) > > case SCHEDOP_block: > { > - ret = do_block(); > + ret = vcpu_block_enable_events(); > break; > } > > @@ -907,7 +912,7 @@ ret_t do_sched_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) > arg) > > case SCHEDOP_block: > { > - ret = do_block(); > + ret = vcpu_block_enable_events(); > break; > } > > > > >> I would also suggest that _local_events_need_delivery() takes no parameter, >> always ignores the mask, and instead push the mask check into >> local_events_need_delivery(). Perhaps rename the former to >> local_events_need_delivery_ignore_mask() or something. > > Yeah, this is a good idea.