diff mbox

[Xen-devel,v8,09/13] xen/arm: second irq injection while the first irq is still inflight

Message ID alpine.DEB.2.02.1405231545300.14596@kaball.uk.xensource.com
State New
Headers show

Commit Message

Stefano Stabellini May 23, 2014, 2:50 p.m. UTC 
On Thu, 22 May 2014, Julien Grall wrote:
> On 22/05/14 18:39, Stefano Stabellini wrote:
> > On Thu, 22 May 2014, Julien Grall wrote:
> > > > while the first one is still active.
> > > > If the first irq is already pending (not active), clear
> > > > GIC_IRQ_GUEST_QUEUED because the guest doesn't need a second
> > > > notification.If the irq has already been EOI'ed then just clear the
> > > > GICH_LR right away and move the interrupt to lr_pending so that it is
> > > > going to be reinjected by gic_restore_pending_irqs on return to guest.
> > > > 
> > > > If the target cpu is not the current cpu, then set GIC_IRQ_GUEST_QUEUED
> > > > and send an SGI. The target cpu is going to be interrupted and call
> > > > gic_clear_lrs, that is going to take the same actions.
> > > > 
> > > > Do not call vgic_vcpu_inject_irq from gic_inject if
> > > > evtchn_upcall_pending is set. If we remove that call, we don't need to
> > > > special case evtchn_irq in vgic_vcpu_inject_irq anymore.
> > > > We need to force the first injection of evtchn_irq (call
> > > > gic_vcpu_inject_irq) from vgic_enable_irqs because evtchn_upcall_pending
> > > > is already set by common code on vcpu creation.
> > > 
> > > If you only need it for the first time. Why can't you call vgic_inject_irq
> > > with the IRQ evtchn when the VCPU is turn on?
> > > 
> > > This would remove every hack with this IRQ in the GIC code.
> > 
> > In principle sounds nice, but in practice it is difficult and risks
> > being racy. In vgic_vcpu_inject_irq we have:
> > 
> >      /* vcpu offline */
> >      if ( test_bit(_VPF_down, &v->pause_flags) )
> >      {
> >          spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> >          return;
> >      }
> > 
> > So we can only inject the irq once the vcpu is properly up, that is
> > certainly later than vcpu_initialise.
> 
> If we call vcpu_vgic_inject right before vcpu_wake (the _VPF_down flags has
> been cleared) we won't have any race condition.
> 
> This can be done in both arch/arm/vpsci.c and common/domain.c (VCPUOP_up).
>
> It may require an arch specific function. Smth like arch_vcpu_prepare_up.

The following change works:

Comments

Julien Grall May 23, 2014, 3:14 p.m. UTC | #1 
On 05/23/2014 03:50 PM, Stefano Stabellini wrote:
> The following change works:
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 33141e3..2a8456f 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -644,6 +644,8 @@ int arch_set_info_guest(
>      else
>          set_bit(_VPF_down, &v->pause_flags);
>  
> +    vgic_vcpu_inject_irq(v, v->domain->arch.evtchn_irq);
> +

This is racy, we may not clear the _VPF_down bit in this function
(depending if VGCF_online is set or not).

Hopefully for ARM, libxc is setting this flags by default but it's not
always true.

>      return 0;
>  }
>  
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index af5cd6c..d597f63 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1087,6 +1087,8 @@ int construct_dom0(struct domain *d)
>      }
>  #endif
>  
> +    vgic_vcpu_inject_irq(v, v->domain->arch.evtchn_irq);
> +

I think it needs a comment in code.

Regards,
Stefano Stabellini May 23, 2014, 5:24 p.m. UTC | #2 
On Fri, 23 May 2014, Julien Grall wrote:
> On 05/23/2014 03:50 PM, Stefano Stabellini wrote:
> > The following change works:
> > 
> > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> > index 33141e3..2a8456f 100644
> > --- a/xen/arch/arm/domain.c
> > +++ b/xen/arch/arm/domain.c
> > @@ -644,6 +644,8 @@ int arch_set_info_guest(
> >      else
> >          set_bit(_VPF_down, &v->pause_flags);
> >  
> > +    vgic_vcpu_inject_irq(v, v->domain->arch.evtchn_irq);
> > +
> 
> This is racy, we may not clear the _VPF_down bit in this function
> (depending if VGCF_online is set or not).
> 
> Hopefully for ARM, libxc is setting this flags by default but it's not
> always true.

I could change the code to call vgic_vcpu_inject_irq only if VGCF_online
is set, but on second thought, would the code actually be more readable?
Or less error prone?

I think that the original patch is better. At least the hack is present
in a single very obvious place (vgic_enable_irqs).


> >      return 0;
> >  }
> >  
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index af5cd6c..d597f63 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -1087,6 +1087,8 @@ int construct_dom0(struct domain *d)
> >      }
> >  #endif
> >  
> > +    vgic_vcpu_inject_irq(v, v->domain->arch.evtchn_irq);
> > +
> 
> I think it needs a comment in code.
> 
> Regards,
> 
> -- 
> Julien Grall
>
Julien Grall May 25, 2014, 6:46 p.m. UTC | #3 
On 23/05/14 18:24, Stefano Stabellini wrote:
> On Fri, 23 May 2014, Julien Grall wrote:
>> On 05/23/2014 03:50 PM, Stefano Stabellini wrote:
>>> The following change works:
>>>
>>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>>> index 33141e3..2a8456f 100644
>>> --- a/xen/arch/arm/domain.c
>>> +++ b/xen/arch/arm/domain.c
>>> @@ -644,6 +644,8 @@ int arch_set_info_guest(
>>>       else
>>>           set_bit(_VPF_down, &v->pause_flags);
>>>
>>> +    vgic_vcpu_inject_irq(v, v->domain->arch.evtchn_irq);
>>> +
>>
>> This is racy, we may not clear the _VPF_down bit in this function
>> (depending if VGCF_online is set or not).
>>
>> Hopefully for ARM, libxc is setting this flags by default but it's not
>> always true.
>
> I could change the code to call vgic_vcpu_inject_irq only if VGCF_online
> is set, but on second thought, would the code actually be more readable?
> Or less error prone?
>
> I think that the original patch is better. At least the hack is present
> in a single very obvious place (vgic_enable_irqs).

Hmmm ... right. I know that this code will likely change (with GICv3 
support). Can you add a comment in the code explain this issue?

With this change:

Acked-by: Julien Grall <julien.grall@linaro.org>

Regards,
Stefano Stabellini May 27, 2014, 4:53 p.m. UTC | #4 
On Sun, 25 May 2014, Julien Grall wrote:
> On 23/05/14 18:24, Stefano Stabellini wrote:
> > On Fri, 23 May 2014, Julien Grall wrote:
> > > On 05/23/2014 03:50 PM, Stefano Stabellini wrote:
> > > > The following change works:
> > > > 
> > > > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> > > > index 33141e3..2a8456f 100644
> > > > --- a/xen/arch/arm/domain.c
> > > > +++ b/xen/arch/arm/domain.c
> > > > @@ -644,6 +644,8 @@ int arch_set_info_guest(
> > > >       else
> > > >           set_bit(_VPF_down, &v->pause_flags);
> > > > 
> > > > +    vgic_vcpu_inject_irq(v, v->domain->arch.evtchn_irq);
> > > > +
> > > 
> > > This is racy, we may not clear the _VPF_down bit in this function
> > > (depending if VGCF_online is set or not).
> > > 
> > > Hopefully for ARM, libxc is setting this flags by default but it's not
> > > always true.
> > 
> > I could change the code to call vgic_vcpu_inject_irq only if VGCF_online
> > is set, but on second thought, would the code actually be more readable?
> > Or less error prone?
> > 
> > I think that the original patch is better. At least the hack is present
> > in a single very obvious place (vgic_enable_irqs).
> 
> Hmmm ... right. I know that this code will likely change (with GICv3 support).
> Can you add a comment in the code explain this issue?

done


> With this change:
> 
> Acked-by: Julien Grall <julien.grall@linaro.org>
> 
> Regards,
> 
> -- 
> Julien Grall
>
diff mbox

Patch

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 33141e3..2a8456f 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -644,6 +644,8 @@  int arch_set_info_guest(
     else
         set_bit(_VPF_down, &v->pause_flags);
 
+    vgic_vcpu_inject_irq(v, v->domain->arch.evtchn_irq);
+
     return 0;
 }
 
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index af5cd6c..d597f63 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1087,6 +1087,8 @@  int construct_dom0(struct domain *d)
     }
 #endif
 
+    vgic_vcpu_inject_irq(v, v->domain->arch.evtchn_irq);
+
     for ( i = 1, cpu = 0; i < d->max_vcpus; i++ )
     {
         cpu = cpumask_cycle(cpu, &cpu_online_map);
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 4869b87..2f86de1 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -404,17 +404,10 @@  static void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
         irq = i + (32 * n);
         p = irq_to_pending(v, irq);
         set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
-        if ( irq == v->domain->arch.evtchn_irq &&
-             vcpu_info(current, evtchn_upcall_pending) &&
-             list_empty(&p->inflight) )
-            vgic_vcpu_inject_irq(v, irq);
-        else {
-            unsigned long flags;
-            spin_lock_irqsave(&v->arch.vgic.lock, flags);
-            if ( !list_empty(&p->inflight) && !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
-                gic_raise_guest_irq(v, irq, p->priority);
-            spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
-        }
+        spin_lock_irqsave(&v->arch.vgic.lock, flags);
+        if ( !list_empty(&p->inflight) && !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
+            gic_raise_guest_irq(v, irq, p->priority);
+        spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
         if ( p->desc != NULL )
         {
             spin_lock_irqsave(&p->desc->lock, flags);