Re: Bug report and patch about IRQ freezing after gic_restore_state

[Jaeyong Yoo <jaeyong.yoo@samsung.com>]

> On Tue, 2013-05-21 at 11:13 +0000, 유재용 wrote:
> > > > Signed-off-by: Jaeyong Yoo 
> > > > --- 
> > > >  xen/arch/arm/domain.c |    4 ++-- 
> > > >  xen/arch/arm/gic.c    |    4 ++-- 
> > > >  2 files changed, 4 insertions(+), 4 deletions(-) 
> > > > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c 
> > > > index f71b582..2c3b132 100644 
> > > > --- a/xen/arch/arm/domain.c 
> > > > +++ b/xen/arch/arm/domain.c 
> > > > @@ -141,6 +141,8 @@ static void ctxt_switch_to(struct vcpu *n) 
> > > >      /* VGIC */ 
> > > >      gic_restore_state(n); 
> > > > +    local_irq_enable(); 
> > > > +
> > > 
> > > Could you move the local_irq_enable right after ctxt_switch_to?
> > 
> > Of course.
> > Just one small concern:
> > Would it be more efficient (i.e., less scheduling latency) if we
> > enable irq right after gic_restore state similar to lock-breaking
> > effect?
> 
> Perhaps, but I think at this stage we should aim for obvious
> correctness.
> 
> Optimising things for IRQ/scheduler latency is a complete project in its
> own right.

I got it.
Here goes the updated patch:

Signed-off-by: Jaeyong Yoo <jaeyong.yoo@samsung.com>
___
 xen/arch/arm/domain.c |    4 ++--
 xen/arch/arm/gic.c    |    2 --
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 9ca44ea..ee12b5f 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -226,10 +226,10 @@ static void schedule_tail(struct vcpu *prev)
 {
     ctxt_switch_from(prev);
 
-    local_irq_enable();
-
     ctxt_switch_to(current);
 
+    local_irq_enable();
+
     if ( prev != current )
         update_runstate_area(current);
 }
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 30bf8d1..d9940ea 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -93,11 +93,9 @@ void gic_restore_state(struct vcpu *v)
     if ( is_idle_vcpu(v) )
         return;
 
-    spin_lock_irq(&gic.lock);
     this_cpu(lr_mask) = v->arch.lr_mask;
     for ( i=0; i<nr_lrs; i++)
         GICH[GICH_LR + i] = v->arch.gic_lr[i];
-    spin_unlock_irq(&gic.lock);
     GICH[GICH_APR] = v->arch.gic_apr;
     GICH[GICH_HCR] = GICH_HCR_EN;
     isb();

best,
Jaeyong
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel