Re: [Xen-devel] [PATCH v2 7/9] xen/arm: Allow DOM0 to set the IRQ type

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 14 Jul 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.
> 
> 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 may never be set, which is fine
> because the field is only used to configure the type during the routing.
> 
> Based on 4.3.13 in ARM IHI 0048B.b, changing the value of Int_config is
> UNPREDICTABLE when the corresponding interrupt is not disabled.
> 
> Therefore, setting the IRQ type when the guest is writing into ICFGR
> would require more work to make sure the IRQ has been disabled before
> writing into the host ICFGR. As the behavior is UNPREDICTABLE, the type
> will be set before enabling the physical IRQ associated to the virtual IRQ.
> 
> Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> 
> ---
> 
> It might be possible to let any domain configure the IRQ
> type (could be useful when passthrough an IRQ with ACPI). However, we would
> need to consider any potential security impact beforehand.
> 
>     Changes in v2:
>         - Rename the patch
>         - Allow any DOM0 to set the IRQ type
>         - Re-use in part of vgic_get_virq_type from
>         "Configure SPI interrupt type and route to Dom0 dynamically".
>         - Add rationale why the IRQ type is set in enable
> ---
>  xen/arch/arm/gic.c        |  5 +++--
>  xen/arch/arm/irq.c        | 13 ++++++++++++-
>  xen/arch/arm/vgic.c       | 19 +++++++++++++++++++
>  xen/include/asm-arm/gic.h |  3 +++
>  xen/include/asm-arm/irq.h |  6 ++++++
>  5 files changed, 43 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 72bb885..63c744a 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
> @@ -160,7 +160,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 3fc22f2..06d4843 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -395,6 +395,17 @@ bool_t is_assignable_irq(unsigned int irq)
>  }
>  
>  /*
> + * Only the hardware domain is allowed to set the configure the
> + * interrupt type for now.
> + *
> + * 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);
> +}
> +
> +/*
>   * Route an IRQ to a specific guest.
>   * For now only SPIs are assignable to the guest.
>   */
> @@ -449,7 +460,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 ( !irq_type_set_by_domain(d) && desc->arch.type == IRQ_TYPE_INVALID )
>      {
>          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 5070452..a7ccfe7 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -344,6 +344,22 @@ void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
>      }
>  }
>  
> +#define VGIC_ICFG_MASK(intr) (1 << ((2 * ((intr) % 16)) + 1))
> +
> +/* The function should be called with the rank lock taken */
> +static inline unsigned int vgic_get_virq_type(struct vcpu *v, int n, int 
> index)
> +{
> +    struct vgic_irq_rank *r = vgic_get_rank(v, n);
> +    uint32_t tr = r->icfg[index >> 4];
> +
> +    ASSERT(spin_is_locked(&r->lock));
> +
> +    if ( tr & VGIC_ICFG_MASK(index) )
> +        return IRQ_TYPE_EDGE_RISING;
> +    else
> +        return IRQ_TYPE_LEVEL_HIGH;
> +}
> +
>  void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
>  {
>      const unsigned long mask = r;
> @@ -352,6 +368,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);
> @@ -366,6 +383,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, n, i));

The patch looks good, but we should probably set the type only for irq >= 32.


>              p->desc->handler->enable(p->desc);
>              spin_unlock_irqrestore(&p->desc->lock, flags);
>          }
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 2214e87..836a103 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, unsigned int 
> priority);
>  extern int gic_route_irq_to_guest(struct domain *, unsigned int virq,
> 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
https://lists.xen.org/xen-devel