Re: [RFC 7/8] xen/arm: Allow DOM0 to set the irq type when ACPI is inuse

[Julien Grall <julien.grall@arm.com>]

Hi Stefano,

On 22/06/16 12:23, Stefano Stabellini wrote:
> On Tue, 7 Jun 2016, Julien Grall wrote:
>> The function route_irq_to_guest mandates the IRQ type, stored in
>> desc->arch.type, to be valid. However, in case of ACPI, these
>> information is not part of the static tables. Therefore Xen needs to
>> rely on DOM0 to provide a valid type based on the firmware tables.
>>
>> A new helper, irq_type_set_by_domain is provided to check whether a
>> domain is allowed to set the IRQ type. For now, only DOM0 is allowed to
>> configure it when ACPI is inuse.
>>
>> When the helper returns 1, the routing function will not check whether
>> the IRQ type is correctly set and configure the GIC. Instead, this will
>> be done when the domain will enable the interrupt.
>>
>> Note that irq_set_spi_type is not called because it validates the type
>> and does not allow it the domain to change the type after the first
>> write. It means that desc->arch.type will never be set, which is fine
>> because the field is only used to configure the type during the routing.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>
>> ---
>>
>> I am thinking to at least extend the behavior to DOM0 using DT. This
>> would make us resilient to a DT not providing the correct type without
>> having to workaround them in Xen.
>
> I agree with you. It is also better to have only one way of doing things
> rather than two.

I will update this patch to allow the hardware domain setting the IRQ type.

[...]

>> +#define NR_CFG_PER_ICFGR 16
>> +#define NR_BITS_PER_CFG (32U / NR_CFG_PER_ICFGR)
>> +
>> +/* The function should be called witht the rank lock taken. */
>                                        ^ with
>
>> +static int __vgic_get_virq_type(struct vcpu *v, unsigned int virq)
>> +{
>> +    struct vgic_irq_rank *rank = vgic_rank_irq(v, virq);
>> +    uint8_t index = virq & INTERRUPT_RANK_MASK;
>> +    uint32_t reg = rank->icfg[index / NR_CFG_PER_ICFGR];
>> +    uint8_t val;
>> +
>> +    ASSERT(spin_is_locked(&rank->lock));
>> +
>> +    val = reg >> ((index % NR_CFG_PER_ICFGR) * NR_BITS_PER_CFG);
>> +
>> +    return (val & 0x2) ? IRQ_TYPE_EDGE_RISING : IRQ_TYPE_LEVEL_HIGH;
>> +}
>
> I would stick to the function and definition we already had from "from
> Configure SPI interrupt type and route to Dom0 dynamically" by Shannon:

I found Shannon's function more difficult to understand. However the 
code is simpler, so I will use his function for the next version.

>    #define VGIC_ICFG_MASK(intr) (1 << ((2 * ((intr) % 16)) + 1))
>
>    static inline unsigned int get_the_irq_type(struct vcpu *v, int n, int index)
>    {
>        struct vgic_irq_rank *vr = vgic_get_rank(v, n);
>        uint32_t tr = vr->icfg[index >> 4];
>
>        if ( tr & VGIC_ICFG_MASK(index) )
>            return IRQ_TYPE_EDGE_BOTH;
>        else
>            return IRQ_TYPE_LEVEL_MASK;
>    }
>
>
>
>>   void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq)
>>   {
>>       unsigned long flags;
>> @@ -342,6 +360,7 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
>>       unsigned long flags;
>>       int i = 0;
>>       struct vcpu *v_target;
>> +    struct domain *d = v->domain;
>>
>>       while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
>>           irq = i + (32 * n);
>> @@ -356,6 +375,8 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
>>           {
>>               irq_set_affinity(p->desc, cpumask_of(v_target->processor));
>>               spin_lock_irqsave(&p->desc->lock, flags);
>> +            if ( irq_type_set_by_domain(d) )
>> +                gic_set_irq_type(p->desc, __vgic_get_virq_type(v, irq));
>>               p->desc->handler->enable(p->desc);
>>               spin_unlock_irqrestore(&p->desc->lock, flags);
>>           }
>
> This is OK. But I am wondering if it wouldn't make sense to call
> gic_set_irq_type when the guest actually writes to the virtual icfg?

Hmmm it looks like that I forgot to explain in the commit message why I 
chose to call gic_set_irq_type here.

Changing the value of Int_config is UNPREDICTABLE when the corresponding
interrupt is not disabled (see 4.3.13 in ARM IHI 0048B.b).

So calling gic_set_irq_type when the guest actualy writes to the virtual 
cfg would require to disable the IRQ before hand.

Given that the behavior is UNPREDICTABLE, I chose to set the type before 
enabling the IRQ because the resulting code is simpler.

Regards,

-- 
Julien Grall

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