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

[Julien Grall <julien.grall@xxxxxxx>]

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@xxxxxxx>
> >
> > ---
> >
> > 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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel