Re: [PATCH v6 16/22] arm/acpi: Configure SPI interrupt type and route to Dom0 dynamically

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

Hi Shannon,

On 17/03/16 09:41, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
>
> Interrupt information is described in DSDT and is not available at the
> time of booting. Check if the interrupt is permitted to access and set
> the interrupt type, route it to guest dynamically only for SPI
> and Dom0.
>
> Signed-off-by: Parth Dixit <parth.dixit@linaro.org>
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
> v6: coding style
> ---
>   xen/arch/arm/vgic.c | 32 ++++++++++++++++++++++++++++++++
>   1 file changed, 32 insertions(+)
>
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index ee35683..39d858c 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -25,6 +25,8 @@
>   #include <xen/irq.h>
>   #include <xen/sched.h>
>   #include <xen/perfc.h>
> +#include <xen/iocap.h>
> +#include <xen/acpi.h>
>
>   #include <asm/current.h>
>
> @@ -334,6 +336,19 @@ void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
>       }
>   }
>
> +#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_enable_irqs(struct vcpu *v, uint32_t r, int n)
>   {
>       const unsigned long mask = r;
> @@ -342,9 +357,26 @@ 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;
> +    int ret;
>
>       while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
>           irq = i + (32 * n);
> +        /* Set the irq type and route it to guest only for SPI and Dom0 */
> +        if( irq_access_permitted(d, irq) && is_hardware_domain(d) &&
> +            ( irq >= 32 ) && ( !acpi_disabled ) )
> +        {
> +            ret = irq_set_spi_type(irq, get_the_irq_type(v, n, i));
> +            if ( ret )
> +                printk(XENLOG_WARNING "The irq type is not correct\n");

XENLOG_WARNING (and XENLOG_ERR) are not rate-limited. Therefore a domain 
(even if it's dom0) could flood the console.

Please use XENLOG_G_* instead. However in this case, "v" is the current 
vCPU so you can use gprintk(XENLOG_WARNING, ...);

> +
> +            vgic_reserve_virq(d, irq);
> +
> +            ret = route_irq_to_guest(d, irq, irq, NULL);
> +            if ( ret )
> +                printk(XENLOG_ERR "Unable to route IRQ %u to domain %u\n",
> +                       irq, d->domain_id);

I'm not against this solution for short term. But in long term we would 
benefit to split the IRQ configuration from the routing.

The routing will be done before DOM0 is booting. The IRQ configuration 
will just end up to write in the ICFGR register.

This will also help for PCI-passthrough as the guest will have to 
configure the SPIs (we can't expect DOM0 doing it for it). But the 
routing will be done ahead.

> +        }
>           v_target = __vgic_get_target_vcpu(v, irq);
>           p = irq_to_pending(v_target, irq);
>           set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
>

Regards,

-- 
Julien Grall

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