Re: [Xen-devel] [PATCH v3 60/62] arm/acpi: Configure interrupts dynamically

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Fri, 27 Nov 2015, Stefano Stabellini wrote:
> On Tue, 17 Nov 2015, shannon.zhao@xxxxxxxxxx wrote:
> > From: Parth Dixit <parth.dixit@xxxxxxxxxx>
> > 
> > Interrupt information is described in DSDT and is not available at
> > the time of booting. Configure the interrupts dynamically when requested
> > by Dom0

Actually wouldn't it make more sense to configure the physical
interrupts when dom0 writes to the virtual ICFG registers?


> > Signed-off-by: Parth Dixit <parth.dixit@xxxxxxxxxx>
> > Signed-off-by: Shannon Zhao <shannon.zhao@xxxxxxxxxx>
> > ---
> >  xen/arch/arm/vgic.c | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> > 
> > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> > index 7bb4570..d05abde 100644
> > --- a/xen/arch/arm/vgic.c
> > +++ b/xen/arch/arm/vgic.c
> > @@ -25,6 +25,7 @@
> >  #include <xen/irq.h>
> >  #include <xen/sched.h>
> >  #include <xen/perfc.h>
> > +#include <xen/acpi.h>
> >  
> >  #include <asm/current.h>
> >  
> > @@ -310,6 +311,8 @@ void vgic_disable_irqs(struct vcpu *v, uint32_t r, int 
> > n)
> >      }
> >  }
> >  
> > +#define VGIC_ICFG_MASK(intr) ( 1 << ( ( 2 * ( intr % 16 ) ) + 1 ) )
> 
> Coding style.
> 
> 
> >  void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
> >  {
> >      struct domain *d = v->domain;
> > @@ -321,7 +324,23 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r, int 
> > n)
> >      struct vcpu *v_target;
> >  
> >      while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
> > +#ifdef CONFIG_ACPI
> > +        struct vgic_irq_rank *vr = vgic_get_rank(v, n);
> 
> The rank is always the same, you can move this out of the while loop.
> 
> 
> > +        uint32_t tr;
> > +
> >          irq = i + (32 * n);
> > +        if( ( !acpi_disabled ) && ( n != 0 ) && is_hardware_domain(d) )
> 
> Instead of n != 0, I would prefer a more obvious irq >= 32 check
> 
> 
> > +        {
> > +            tr = vr->icfg[i >> 4] ;
> > +
> > +            if( ( tr & VGIC_ICFG_MASK(i) ) )
> 
> Coding style.
> 
> 
> > +                irq_set_type(irq, ACPI_IRQ_TYPE_EDGE_BOTH);
> > +            else
> > +                irq_set_type(irq, ACPI_IRQ_TYPE_LEVEL_MASK);
> 
> Instead of this ad-hoc ACPI code, I would prefer if we get the irq type
> and, if it is set to NONE or INVALID, we set it to what is in the
> r->icfg (only for the hardware_domain of course). If it is set to
> something other than NONE or INVALID, and it doesn't match what is on
> r->icfg, then we could print out a warning.
> 
> 
> > +        }
> > +#else
> > +        irq = i + (32 * n);
> > +#endif
> 
> Please leave the irq = i + (32 * n) out of the #ifdef.
> 
> 
> >          v_target = d->arch.vgic.handler->get_target_vcpu(v, irq);
> >          p = irq_to_pending(v_target, irq);
> >          set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
> > -- 
> > 2.1.0
> > 
> 

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