Re: [PATCH v2 39/41] arm : acpi configure interrupts dynamically

[Parth Dixit <parth.dixit@linaro.org>]

+shannon

On 8 June 2015 at 23:09, Julien Grall <julien.grall@citrix.com> wrote:
> Hi Parth,
>
> On 17/05/2015 21:04, Parth Dixit wrote:
>>
>> Interrupt information is described in DSDT and is not available at
>> the time of booting. Configure the interrupts dynamically when requested
>> by Dom0
>
>
> Missing "."
>
> Also, I'm sure we talked about it multiple time. 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@linaro.org>
>> ---
>>   xen/arch/arm/vgic.c | 18 ++++++++++++++++++
>>   1 file changed, 18 insertions(+)
>>
>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
>> index 73a6f7e..f63deb4 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>
>>
>> @@ -285,6 +286,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;
>> @@ -296,7 +299,22 @@ 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;
>
>
> New line.
>
>> +        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.
>
> Furthermore, during passthrough, the IRQ may not have been configured by
> DOM0. So we have to let the guest configure the IRQ.
>
>> +        {
>> +            tr = vr->icfg[i >> 4] ;
>> +
>> +            if( ( tr & VGIC_ICFG_MASK(i) ) )
>> +                set_irq_type(irq, ACPI_IRQ_TYPE_EDGE_BOTH);
>> +            else
>> +                set_irq_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 may fail because the configuration as been set earlier (as you
> did in patch #41.
>
> Regards,
>
> --
> Julien Grall