| Message ID | 1403633514-8853-1-git-send-email-stefano.stabellini@eu.citrix.com |
|---|---|
| State | New |
| Headers | show |
Hi Stefano, On 06/24/2014 07:11 PM, Stefano Stabellini wrote: > evtchn_upcall_pending is already set by common code at vcpu creation, > therefore on ARM we also need to call vgic_vcpu_inject_irq for it. > Currently we do that from vgic_enable_irqs as a workaround. > > Do this properly by calling vgic_vcpu_inject_irq in the appropriate > places at vcpu creation time, making sure to call it after the vcpu is > up (_VPF_down has been cleared). While it's works perfectly on common case, as the toolstack is always setting VGCF_online. It would be possible to call the hypercall DOMCTL_vcpusetcontext without this flags enable. If so, the new VCPU will never receive event channel interrupt. > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > --- > xen/arch/arm/domain.c | 4 +++- > xen/arch/arm/domain_build.c | 2 ++ > xen/arch/arm/vgic.c | 18 ++++-------------- > 3 files changed, 9 insertions(+), 15 deletions(-) > > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c > index e20ba0b..c29b063 100644 > --- a/xen/arch/arm/domain.c > +++ b/xen/arch/arm/domain.c > @@ -651,8 +651,10 @@ int arch_set_info_guest( > v->is_initialised = 1; > > if ( ctxt->flags & VGCF_online ) > + { > clear_bit(_VPF_down, &v->pause_flags); > - else > + vgic_vcpu_inject_irq(v, v->domain->arch.evtchn_irq); I'd like a comment above each vgic_vcpu_inject(v, evtchn_irq) to explain why we need them. So in the future we won't need to spend hours to search in log because someone has moved the line. > + } else Coding style: else { Regards,
On Wed, 2014-06-25 at 16:03 +0100, Julien Grall wrote: > Hi Stefano, > > On 06/24/2014 07:11 PM, Stefano Stabellini wrote: > > evtchn_upcall_pending is already set by common code at vcpu creation, > > therefore on ARM we also need to call vgic_vcpu_inject_irq for it. > > Currently we do that from vgic_enable_irqs as a workaround. > > > > Do this properly by calling vgic_vcpu_inject_irq in the appropriate > > places at vcpu creation time, making sure to call it after the vcpu is > > up (_VPF_down has been cleared). > > While it's works perfectly on common case, as the toolstack is always > setting VGCF_online. > > It would be possible to call the hypercall DOMCTL_vcpusetcontext without > this flags enable. If so, the new VCPU will never receive event channel > interrupt. I think the only other way to clear _VPF_down is the psci code, so I suppose it needs a kick too. The other option is VCPUOP_up whch we do not support (or wire up) on ARM.
On 07/02/2014 04:38 PM, Ian Campbell wrote: > On Wed, 2014-06-25 at 16:03 +0100, Julien Grall wrote: >> Hi Stefano, >> >> On 06/24/2014 07:11 PM, Stefano Stabellini wrote: >>> evtchn_upcall_pending is already set by common code at vcpu creation, >>> therefore on ARM we also need to call vgic_vcpu_inject_irq for it. >>> Currently we do that from vgic_enable_irqs as a workaround. >>> >>> Do this properly by calling vgic_vcpu_inject_irq in the appropriate >>> places at vcpu creation time, making sure to call it after the vcpu is >>> up (_VPF_down has been cleared). >> >> While it's works perfectly on common case, as the toolstack is always >> setting VGCF_online. >> >> It would be possible to call the hypercall DOMCTL_vcpusetcontext without >> this flags enable. If so, the new VCPU will never receive event channel >> interrupt. > > I think the only other way to clear _VPF_down is the psci code, so I > suppose it needs a kick too. The function PSCI on will call arch_set_info_guest directly with VGCF_online. > The other option is VCPUOP_up whch we do not support (or wire up) on > ARM. Hmmm... right I was looking directly to do_vcpu_op, but we have an extra layer on ARM (do_arm_vcpu_op). In this case, I would return an error if VGCF_online is not set. I will allow us to catch easily an error later if we decide to implement VCPUOP_up. Regards,
On Wed, 2 Jul 2014, Julien Grall wrote: > On 07/02/2014 04:38 PM, Ian Campbell wrote: > > On Wed, 2014-06-25 at 16:03 +0100, Julien Grall wrote: > >> Hi Stefano, > >> > >> On 06/24/2014 07:11 PM, Stefano Stabellini wrote: > >>> evtchn_upcall_pending is already set by common code at vcpu creation, > >>> therefore on ARM we also need to call vgic_vcpu_inject_irq for it. > >>> Currently we do that from vgic_enable_irqs as a workaround. > >>> > >>> Do this properly by calling vgic_vcpu_inject_irq in the appropriate > >>> places at vcpu creation time, making sure to call it after the vcpu is > >>> up (_VPF_down has been cleared). > >> > >> While it's works perfectly on common case, as the toolstack is always > >> setting VGCF_online. > >> > >> It would be possible to call the hypercall DOMCTL_vcpusetcontext without > >> this flags enable. If so, the new VCPU will never receive event channel > >> interrupt. > > > > I think the only other way to clear _VPF_down is the psci code, so I > > suppose it needs a kick too. > > The function PSCI on will call arch_set_info_guest directly with > VGCF_online. > > > The other option is VCPUOP_up whch we do not support (or wire up) on > > ARM. > > Hmmm... right I was looking directly to do_vcpu_op, but we have an extra > layer on ARM (do_arm_vcpu_op). > > In this case, I would return an error if VGCF_online is not set. I will > allow us to catch easily an error later if we decide to implement VCPUOP_up. I could print a message and return an error, seems reasonable.
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index e20ba0b..c29b063 100644 --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -651,8 +651,10 @@ int arch_set_info_guest( v->is_initialised = 1; if ( ctxt->flags & VGCF_online ) + { clear_bit(_VPF_down, &v->pause_flags); - else + vgic_vcpu_inject_irq(v, v->domain->arch.evtchn_irq); + } else set_bit(_VPF_down, &v->pause_flags); return 0; diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 9d9cba9..f7cf80d 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -1134,6 +1134,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 7f1e263..1806b72 100644 --- a/xen/arch/arm/vgic.c +++ b/xen/arch/arm/vgic.c @@ -504,20 +504,10 @@ static void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n) v_target = _vgic_get_target_vcpu(v, irq); p = irq_to_pending(v_target, irq); set_bit(GIC_IRQ_GUEST_ENABLED, &p->status); - /* We need to force the first injection of evtchn_irq because - * evtchn_upcall_pending is already set by common code on vcpu - * creation. */ - if ( irq == v_target->domain->arch.evtchn_irq && - vcpu_info(current, evtchn_upcall_pending) && - list_empty(&p->inflight) ) - vgic_vcpu_inject_irq(v_target, irq); - else { - unsigned long flags; - spin_lock_irqsave(&v_target->arch.vgic.lock, flags); - if ( !list_empty(&p->inflight) && !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) ) - gic_raise_guest_irq(v_target, irq, p->priority); - spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags); - } + spin_lock_irqsave(&v_target->arch.vgic.lock, flags); + if ( !list_empty(&p->inflight) && !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) ) + gic_raise_guest_irq(v_target, irq, p->priority); + spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags); if ( p->desc != NULL ) { irq_set_affinity(p->desc, cpumask_of(v_target->processor));
evtchn_upcall_pending is already set by common code at vcpu creation, therefore on ARM we also need to call vgic_vcpu_inject_irq for it. Currently we do that from vgic_enable_irqs as a workaround. Do this properly by calling vgic_vcpu_inject_irq in the appropriate places at vcpu creation time, making sure to call it after the vcpu is up (_VPF_down has been cleared). Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> --- xen/arch/arm/domain.c | 4 +++- xen/arch/arm/domain_build.c | 2 ++ xen/arch/arm/vgic.c | 18 ++++-------------- 3 files changed, 9 insertions(+), 15 deletions(-)