Re: [Xen-devel] [PATCH v7 1/2] xen: export do_block, add a parameter to enable local events delivery

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Fri, 19 Apr 2013, Keir Fraser wrote:
> On 19/04/2013 17:23, "Stefano Stabellini" <stefano.stabellini@xxxxxxxxxxxxx>
> wrote:
> 
> > On Fri, 19 Apr 2013, Jan Beulich wrote:
> >>>>> On 19.04.13 at 16:06, Stefano Stabellini
> >>>>> <stefano.stabellini@xxxxxxxxxxxxx> 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:


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.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel