Re: [Xen-devel] [PATCH v6 3/5] xen/arm: support irq delivery to vcpu > 0

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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
> 

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