| Message ID | 1403541463-23734-3-git-send-email-stefano.stabellini@eu.citrix.com |
|---|---|
| State | New |
| Headers | show |
Hi Stefano, On 06/23/2014 05:37 PM, Stefano Stabellini wrote: > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > index c7dda9b..54610ce 100644 > --- a/xen/arch/arm/gic.c > +++ b/xen/arch/arm/gic.c > @@ -292,7 +292,7 @@ void gic_route_irq_to_guest(struct domain *d, struct irq_desc *desc, > > gic_set_irq_properties(desc, cpumask_of(smp_processor_id()), GIC_PRI_IRQ); > > - /* TODO: do not assume delivery to vcpu0 */ > + /* Route to vcpu0 by default */ This comment is wrong here... we only set the desc for a virtual SPIs. The routing is done in domain_vgic_init. > +void vgic_vcpu_inject_spi(struct domain *d, unsigned int irq) > +{ > + struct vcpu *v; > + > + /* the IRQ needs to be an SPI */ > + ASSERT(irq >= 32 && irq <= 1019); I would use gic_number_lines() rather than 1019 here. If the IRQ is greater than the return value of the function then there is already an issue. IHMO, your ASSERT is implementing your comment in do_IRQ. So it would make sense to have this ASSERT just before calling vgic_vcpu_inject_spi. If the caller is calling this function with a PPIs or SGIs then he is stupid. Regards,
On Tue, 2014-06-24 at 14:33 +0100, Julien Grall wrote: > > + /* the IRQ needs to be an SPI */ > > + ASSERT(irq >= 32 && irq <= 1019); > > I would use gic_number_lines() rather than 1019 here. Or at least #define GIC_MAX_SPI 1019. Ian.
On Tue, 24 Jun 2014, Julien Grall wrote: > Hi Stefano, > > On 06/23/2014 05:37 PM, Stefano Stabellini wrote: > > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > > index c7dda9b..54610ce 100644 > > --- a/xen/arch/arm/gic.c > > +++ b/xen/arch/arm/gic.c > > @@ -292,7 +292,7 @@ void gic_route_irq_to_guest(struct domain *d, struct irq_desc *desc, > > > > gic_set_irq_properties(desc, cpumask_of(smp_processor_id()), GIC_PRI_IRQ); > > > > - /* TODO: do not assume delivery to vcpu0 */ > > + /* Route to vcpu0 by default */ > > This comment is wrong here... we only set the desc for a virtual SPIs. > > The routing is done in domain_vgic_init. OK > > +void vgic_vcpu_inject_spi(struct domain *d, unsigned int irq) > > +{ > > + struct vcpu *v; > > + > > + /* the IRQ needs to be an SPI */ > > + ASSERT(irq >= 32 && irq <= 1019); > > I would use gic_number_lines() rather than 1019 here. OK > If the IRQ is greater than the return value of the function then there > is already an issue. > > IHMO, your ASSERT is implementing your comment in do_IRQ. So it would > make sense to have this ASSERT just before calling vgic_vcpu_inject_spi. > If the caller is calling this function with a PPIs or SGIs then he is > stupid. I don't like the idea of moving ASSERTs to the call site: what if we end up having 3 or 4 places that independently call vgic_vcpu_inject_spi, Should we add the same ASSERT to all of them? This is why I prefer to keep the ASSERT in one place, vgic_vcpu_inject_spi. > Regards, > > -- > Julien Grall >
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index c7dda9b..54610ce 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -292,7 +292,7 @@ void gic_route_irq_to_guest(struct domain *d, struct irq_desc *desc, gic_set_irq_properties(desc, cpumask_of(smp_processor_id()), GIC_PRI_IRQ); - /* TODO: do not assume delivery to vcpu0 */ + /* Route to vcpu0 by default */ p = irq_to_pending(d->vcpu[0], desc->irq); p->desc = desc; } diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c index 9c141bc..15b8edc 100644 --- a/xen/arch/arm/irq.c +++ b/xen/arch/arm/irq.c @@ -197,8 +197,9 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq) desc->status |= IRQ_INPROGRESS; desc->arch.eoi_cpu = smp_processor_id(); - /* XXX: inject irq into all guest vcpus */ - vgic_vcpu_inject_irq(d->vcpu[0], irq); + /* the irq cannot be a PPI, we only support delivery of SPIs to + * guests */ + vgic_vcpu_inject_spi(d, irq); goto out_no_end; } diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c index 7924629..b2f922c 100644 --- a/xen/arch/arm/vgic.c +++ b/xen/arch/arm/vgic.c @@ -884,6 +884,17 @@ out: smp_send_event_check_mask(cpumask_of(v->processor)); } +void vgic_vcpu_inject_spi(struct domain *d, unsigned int irq) +{ + struct vcpu *v; + + /* the IRQ needs to be an SPI */ + ASSERT(irq >= 32 && irq <= 1019); + + v = vgic_get_target_vcpu(d->vcpu[0], irq); + vgic_vcpu_inject_irq(v, irq); +} + /* * Local variables: * mode: C diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h index 3950554..ba9ba9b 100644 --- a/xen/include/asm-arm/gic.h +++ b/xen/include/asm-arm/gic.h @@ -169,6 +169,7 @@ extern void domain_vgic_free(struct domain *d); extern int vcpu_vgic_init(struct vcpu *v); extern void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq); +extern void vgic_vcpu_inject_spi(struct domain *d, unsigned int irq); extern void vgic_clear_pending_irqs(struct vcpu *v); extern struct pending_irq *irq_to_pending(struct vcpu *v, unsigned int irq);
Use vgic_get_target_vcpu to retrieve the target vcpu from do_IRQ. Remove in-code comments about missing implementation of SGI delivery to vcpus other than 0. Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> --- Changes in v6: - add in-code comments; - assert that the guest irq is an SPI. Changes in v4: - the mask in gic_route_irq_to_guest is a physical cpu mask, treat it as such; - export vgic_get_target_vcpu in a previous patch. --- xen/arch/arm/gic.c | 2 +- xen/arch/arm/irq.c | 5 +++-- xen/arch/arm/vgic.c | 11 +++++++++++ xen/include/asm-arm/gic.h | 1 + 4 files changed, 16 insertions(+), 3 deletions(-)