Re: [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.z...@linaro.org wrote:
> >> From: Parth Dixit 
> >>
> >> 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 
> >> Signed-off-by: Shannon Zhao 
> >> ---
> >>  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 
> >>  #include 
> >>  #include 
> >> +#include 
> >>  
> >>  #include 
> >>  
> >> @@ -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(, 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, >status);
> >>
> > 
> > Regards,
> > 
> > [1] http://lists.xen.org/archives/html/xen-devel/2015-06/msg01128.html
> > 
> 
> -- 
> Shannon
> 

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

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

[Shannon Zhao]

Hi,

On 2015/11/30 23:42, Julien Grall wrote:
> Hi Shannon,
> 
> On 17/11/15 09:40, shannon.z...@linaro.org wrote:
>> From: Parth Dixit 
>>
>> 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 
>> Signed-off-by: Shannon Zhao 
>> ---
>>  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 
>>  #include 
>>  #include 
>> +#include 
>>  
>>  #include 
>>  
>> @@ -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(, 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.

> 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, >status);
>>
> 
> Regards,
> 
> [1] http://lists.xen.org/archives/html/xen-devel/2015-06/msg01128.html
> 

-- 
Shannon


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

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

[Julien Grall]

Hi Shannon,

On 17/11/15 09:40, shannon.z...@linaro.org wrote:
> From: Parth Dixit 
> 
> 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 
> Signed-off-by: Shannon Zhao 
> ---
>  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 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  
> @@ -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(, 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.

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, >status);
> 

Regards,

[1] http://lists.xen.org/archives/html/xen-devel/2015-06/msg01128.html

-- 
Julien Grall

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

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

[Stefano Stabellini]

On Tue, 17 Nov 2015, shannon.z...@linaro.org wrote:
> From: Parth Dixit 
> 
> Interrupt information is described in DSDT and is not available at
> the time of booting. Configure the interrupts dynamically when requested
> by Dom0
> 
> Signed-off-by: Parth Dixit 
> Signed-off-by: Shannon Zhao 
> ---
>  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 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  
> @@ -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(, 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, >status);
> -- 
> 2.1.0
> 

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

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

[Stefano Stabellini]

On Fri, 27 Nov 2015, Stefano Stabellini wrote:
> On Tue, 17 Nov 2015, shannon.z...@linaro.org wrote:
> > From: Parth Dixit 
> > 
> > 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 
> > Signed-off-by: Shannon Zhao 
> > ---
> >  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 
> >  #include 
> >  #include 
> > +#include 
> >  
> >  #include 
> >  
> > @@ -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(, 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, >status);
> > -- 
> > 2.1.0
> > 
> 

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