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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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.


> Furthermore, it might be possible to let any domain configuring the IRQ
> type (could be useful when passthrough an IRQ with ACPI). However, we would
> need to consider any potential security impact beforehand.
> ---
>  xen/arch/arm/gic.c        |  5 +++--
>  xen/arch/arm/irq.c        | 14 +++++++++++++-
>  xen/arch/arm/vgic.c       | 21 +++++++++++++++++++++
>  xen/include/asm-arm/gic.h |  3 +++
>  xen/include/asm-arm/irq.h |  6 ++++++
>  5 files changed, 46 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 27cd177..80d93a8 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -97,7 +97,7 @@ void gic_restore_state(struct vcpu *v)
>  }
>  
>  /* desc->irq needs to be disabled before calling this function */
> -static void gic_set_irq_type(struct irq_desc *desc, unsigned int type)
> +void gic_set_irq_type(struct irq_desc *desc, unsigned int type)
>  {
>      /*
>       * IRQ must be disabled before configuring it (see 4.3.13 in ARM IHI
> @@ -162,7 +162,8 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int 
> virq,
>      desc->handler = gic_hw_ops->gic_guest_irq_type;
>      set_bit(_IRQ_GUEST, &desc->status);
>  
> -    gic_set_irq_type(desc, desc->arch.type);
> +    if ( !irq_type_set_by_domain(d) )
> +        gic_set_irq_type(desc, desc->arch.type);
>      gic_set_irq_priority(desc, priority);
>  
>      p->desc = desc;
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index 2f8af72..b9d7749 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -27,6 +27,7 @@
>  
>  #include <asm/gic.h>
>  #include <asm/vgic.h>
> +#include <asm/acpi.h>
>  
>  static unsigned int local_irqs_type[NR_LOCAL_IRQS];
>  static DEFINE_SPINLOCK(local_irqs_type_lock);
> @@ -395,6 +396,17 @@ bool_t is_assignable_irq(unsigned int irq)
>  }
>  
>  /*
> + * When ACPI is inuse, only the hardware domain knows the interrupt
> + * type.
> + *
> + * XXX: See whether it is possible to let any domain configure the type.
> + */
> +bool_t irq_type_set_by_domain(const struct domain *d)
> +{
> +    return ((d == hardware_domain) && !acpi_disabled);
> +}
> +
> +/*
>   * Route an IRQ to a specific guest.
>   * For now only SPIs are assignable to the guest.
>   */
> @@ -449,7 +461,7 @@ int route_irq_to_guest(struct domain *d, unsigned int 
> virq,
>  
>      spin_lock_irqsave(&desc->lock, flags);
>  
> -    if ( desc->arch.type == IRQ_TYPE_INVALID )
> +    if ( desc->arch.type == IRQ_TYPE_INVALID && !irq_type_set_by_domain(d) )
>      {
>          printk(XENLOG_G_ERR "IRQ %u has not been configured\n", irq);
>          retval = -EIO;
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index ee35683..3e1c572 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -249,6 +249,24 @@ static int vgic_get_virq_priority(struct vcpu *v, 
> unsigned int virq)
>      return priority;
>  }
>  
> +#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:

  #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?


> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index ddc45a8..44b9ef6 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -222,6 +222,9 @@ enum gic_version {
>  
>  extern enum gic_version gic_hw_version(void);
>  
> +/* Program the IRQ type into the GIC */
> +void gic_set_irq_type(struct irq_desc *desc, unsigned int type);
> +
>  /* Program the GIC to route an interrupt */
>  extern void gic_route_irq_to_xen(struct irq_desc *desc, const cpumask_t 
> *cpu_mask,
>                                   unsigned int priority);
> diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
> index 493773c..8f7a167 100644
> --- a/xen/include/asm-arm/irq.h
> +++ b/xen/include/asm-arm/irq.h
> @@ -58,6 +58,12 @@ int platform_get_irq(const struct dt_device_node *device, 
> int index);
>  
>  void irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask);
>  
> +/*
> + * Use this helper in places that need to know whether the IRQ type is
> + * set by the domain.
> + */
> +bool_t irq_type_set_by_domain(const struct domain *d);
> +
>  #endif /* _ASM_HW_IRQ_H */
>  /*
>   * Local variables:
> -- 
> 1.9.1
> 

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