[Xen-devel,RFC] xen/arm: Restrict when a physical IRQ can be routed/removed from/to a domain

From: Julien Grall <julien.grall@arm.com>

Xen is currently allowing to route/remove an interrupt from/to the
domain while it is running.

However, we never sync the virtual interrupt state to the physical
interrupt. This could lead to undesirable effect on the vGIC emulation
and potentially the hardware.

One solution would be to sync the interrupt state when routing, but I am
not sure it is worth the effort as you never really when it is safe to
route/remove the interrupt when a domain is running.

Signed-off-by: Julien Grall <julien.grall@arm.com>

---

RFC because I am not entirely sure what people are doing with physical
interrupt today.
---
 xen/arch/arm/gic.c | 38 ++++++++++++++++++++------------------
 1 file changed, 20 insertions(+), 18 deletions(-)

Gentle ping. The vGIC rework from Andre is based on that assumption.

Cheers,

On 08/03/18 15:24, julien.grall@arm.com wrote:
> From: Julien Grall <julien.grall@arm.com>
> 
> Xen is currently allowing to route/remove an interrupt from/to the
> domain while it is running.
> 
> However, we never sync the virtual interrupt state to the physical
> interrupt. This could lead to undesirable effect on the vGIC emulation
> and potentially the hardware.
> 
> One solution would be to sync the interrupt state when routing, but I am
> not sure it is worth the effort as you never really when it is safe to
> route/remove the interrupt when a domain is running.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> ---
> 
> RFC because I am not entirely sure what people are doing with physical
> interrupt today.
> ---
>   xen/arch/arm/gic.c | 38 ++++++++++++++++++++------------------
>   1 file changed, 20 insertions(+), 18 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 968e46fabb..653a815127 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -136,6 +136,14 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int virq,
>       ASSERT(virq < vgic_num_irqs(d));
>       ASSERT(!is_lpi(virq));
>   
> +    /*
> +     * When routing an IRQ to guest, the virtual state is not synced
> +     * back to the physical IRQ. To prevent get unsync, restrict the
> +     * routing to when the Domain is been created.
> +     */
> +    if ( d->creation_finished )
> +        return -EBUSY;
> +
>       ret = vgic_connect_hw_irq(d, NULL, virq, desc, true);
>       if ( ret )
>           return ret;
> @@ -160,25 +168,19 @@ int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
>       ASSERT(test_bit(_IRQ_GUEST, &desc->status));
>       ASSERT(!is_lpi(virq));
>   
> -    if ( d->is_dying )
> -    {
> -        desc->handler->shutdown(desc);
> +    /*
> +     * Removing an interrupt while the domain is running may have
> +     * undesirable effect on the vGIC emulation.
> +     */
> +    if ( !d->is_dying )
> +        return -EBUSY;
>   
> -        /* EOI the IRQ if it has not been done by the guest */
> -        if ( test_bit(_IRQ_INPROGRESS, &desc->status) )
> -            gic_hw_ops->deactivate_irq(desc);
> -        clear_bit(_IRQ_INPROGRESS, &desc->status);
> -    }
> -    else
> -    {
> -        /*
> -         * TODO: Handle eviction from LRs For now, deny
> -         * remove if the IRQ is inflight or not disabled.
> -         */
> -        if ( test_bit(_IRQ_INPROGRESS, &desc->status) ||
> -             !test_bit(_IRQ_DISABLED, &desc->status) )
> -            return -EBUSY;
> -    }
> +    desc->handler->shutdown(desc);
> +
> +    /* EOI the IRQ if it has not been done by the guest */
> +    if ( test_bit(_IRQ_INPROGRESS, &desc->status) )
> +        gic_hw_ops->deactivate_irq(desc);
> +    clear_bit(_IRQ_INPROGRESS, &desc->status);
>   
>       ret = vgic_connect_hw_irq(d, NULL, virq, desc, false);
>       if ( ret )
>

On Thu, 8 Mar 2018, julien.grall@arm.com wrote:
> From: Julien Grall <julien.grall@arm.com>
> 
> Xen is currently allowing to route/remove an interrupt from/to the
> domain while it is running.
> 
> However, we never sync the virtual interrupt state to the physical
> interrupt. This could lead to undesirable effect on the vGIC emulation
> and potentially the hardware.
> 
> One solution would be to sync the interrupt state when routing, but I am
> not sure it is worth the effort as you never really when it is safe to
> route/remove the interrupt when a domain is running.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> ---
> 
> RFC because I am not entirely sure what people are doing with physical
> interrupt today.

I think it is fine to disable dynamic routing. It is not really
something it is supposed to be used today.

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  xen/arch/arm/gic.c | 38 ++++++++++++++++++++------------------
>  1 file changed, 20 insertions(+), 18 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 968e46fabb..653a815127 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -136,6 +136,14 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int virq,
>      ASSERT(virq < vgic_num_irqs(d));
>      ASSERT(!is_lpi(virq));
>  
> +    /*
> +     * When routing an IRQ to guest, the virtual state is not synced
> +     * back to the physical IRQ. To prevent get unsync, restrict the
> +     * routing to when the Domain is been created.
> +     */
> +    if ( d->creation_finished )
> +        return -EBUSY;
> +
>      ret = vgic_connect_hw_irq(d, NULL, virq, desc, true);
>      if ( ret )
>          return ret;
> @@ -160,25 +168,19 @@ int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
>      ASSERT(test_bit(_IRQ_GUEST, &desc->status));
>      ASSERT(!is_lpi(virq));
>  
> -    if ( d->is_dying )
> -    {
> -        desc->handler->shutdown(desc);
> +    /*
> +     * Removing an interrupt while the domain is running may have
> +     * undesirable effect on the vGIC emulation.
> +     */
> +    if ( !d->is_dying )
> +        return -EBUSY;
>  
> -        /* EOI the IRQ if it has not been done by the guest */
> -        if ( test_bit(_IRQ_INPROGRESS, &desc->status) )
> -            gic_hw_ops->deactivate_irq(desc);
> -        clear_bit(_IRQ_INPROGRESS, &desc->status);
> -    }
> -    else
> -    {
> -        /*
> -         * TODO: Handle eviction from LRs For now, deny
> -         * remove if the IRQ is inflight or not disabled.
> -         */
> -        if ( test_bit(_IRQ_INPROGRESS, &desc->status) ||
> -             !test_bit(_IRQ_DISABLED, &desc->status) )
> -            return -EBUSY;
> -    }
> +    desc->handler->shutdown(desc);
> +
> +    /* EOI the IRQ if it has not been done by the guest */
> +    if ( test_bit(_IRQ_INPROGRESS, &desc->status) )
> +        gic_hw_ops->deactivate_irq(desc);
> +    clear_bit(_IRQ_INPROGRESS, &desc->status);
>  
>      ret = vgic_connect_hw_irq(d, NULL, virq, desc, false);
>      if ( ret )