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

[Stefano Stabellini]

On Tue, 5 Jan 2016, Shannon Zhao wrote:
> Hi,
> 
> On 2015/11/30 23:42, Julien Grall wrote:
> > Hi Shannon,
> > 
> > On 17/11/15 09:40, shannon.zhao at linaro.org wrote:
> >> From: Parth Dixit <parth.dixit at linaro.org>
> >>
> >> Interrupt information is described in DSDT and is not available at
> >> the time of booting. Configure the interrupts dynamically when requested
> >> by Dom0
> > 
> > Missing ".".
> > 
> > As said on a previous version of this patch [1], I'd like to keep the
> > ACPI changes very contained to Xen boot. Your change is not ACPI
> > specific and could be used for DT.
> > 
> >>
> >> Signed-off-by: Parth Dixit <parth.dixit at linaro.org>
> >> Signed-off-by: Shannon Zhao <shannon.zhao at linaro.org>
> >> ---
> >>  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 ) )
> >> +
> >>  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);
> >> +        uint32_t tr;
> >> +
> >>          irq = i + (32 * n);
> >> +        if( ( !acpi_disabled ) && ( n != 0 ) && is_hardware_domain(d) )
> > 
> > You need to add a comment explaining the ( n != 0 ) i.e we don't SGIs
> > and PPIs are RO. It's implementation defined for PPI but it's preferable
> > to let Xen take care of it.
> > 
> > Also, we should set the type only for IRQ assigned to DOM0 (i.e p->desc
> > != NULL). With your current solution, DOM0 may change the configuration
> > of the serial IRQ by mistake and take down Xen because the physical IRQ
> > is enabled and the behavior will be unpredictable.
> > 
> >> +        {
> >> +            tr = vr->icfg[i >> 4] ;
> >> +
> >> +            if( ( tr & VGIC_ICFG_MASK(i) ) )
> >> +                irq_set_type(irq, ACPI_IRQ_TYPE_EDGE_BOTH);
> >> +            else
> >> +                irq_set_type(irq, ACPI_IRQ_TYPE_LEVEL_MASK);
> > 
> > Given that only SPI can be configured it would have been better to call
> > irq_set_type.
> > 
> > Although, those 2 functions don't do what you think. They are setting
> > the type internally in Xen but don't change the GIC interrupt
> > configuration register.
> > 
> Here it wants to set the type according to the value of GIC ICFG.
> 
> I don't know this well. Does it need to set the type here when Dom0
> configures the interrupts since I have set them as ACPI_IRQ_TYPE_NONE in
> patch 55?
> 
> How about following way?
> 1) In patch 55, it only permit the access of Dom0 to those irqs, not set
> the type.
> 2) Then in this function or in the handler of writing GIC ICFG, check if
> the irq is permitted and set the irq type.

Sounds good


> > Lastly, they will fail because the configuration has been set earlier
> > (as you did in patch #55)
> > 
> >> +        }
> >> +#else
> >> +        irq = i + (32 * n);
> >> +#endif
> >>          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);
> >>
> > 
> > Regards,
> > 
> > [1] http://lists.xen.org/archives/html/xen-devel/2015-06/msg01128.html
> > 
> 
> -- 
> Shannon
>