Re: [RFC PATCH 06/10] ARM: vGIC: move config from irq_rank to struct pending_irq

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

Hi Andre,

On 04/05/17 16:31, Andre Przywara wrote:
> Currently we store the interrupt type configuration (level or edge
> triggered) in the rank structure. Move this value into struct pending_irq.
> We just need one bit (edge or level), so use one of the status bits.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  xen/arch/arm/vgic-v2.c     | 29 ++++++++++-------------------
>  xen/arch/arm/vgic-v3.c     | 31 ++++++++++++-------------------
>  xen/arch/arm/vgic.c        | 39 ++++++++++++++++++++-------------------
>  xen/include/asm-arm/vgic.h |  7 ++++++-
>  4 files changed, 48 insertions(+), 58 deletions(-)
>
> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> index 5c59fb4..795173c 100644
> --- a/xen/arch/arm/vgic-v2.c
> +++ b/xen/arch/arm/vgic-v2.c
> @@ -278,20 +278,10 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info,
>          goto read_reserved;
>
>      case VRANGE32(GICD_ICFGR, GICD_ICFGRN):
> -    {
> -        uint32_t icfgr;
> -
>          if ( dabt.size != DABT_WORD ) goto bad_width;
> -        rank = vgic_rank_offset(v, 2, gicd_reg - GICD_ICFGR, DABT_WORD);
> -        if ( rank == NULL) goto read_as_zero;
> -        vgic_lock_rank(v, rank, flags);
> -        icfgr = rank->icfg[REG_RANK_INDEX(2, gicd_reg - GICD_ICFGR, DABT_WORD)];
> -        vgic_unlock_rank(v, rank, flags);
> -
> -        *r = vgic_reg32_extract(icfgr, info);
> -
> +        irq = (gicd_reg - GICD_ICFGR) * 4;

This would be nicer to have that handle directly in 
gather_irq_info_config rather than open-coding everywhere. Also it 
avoids this confusion of what irq actually meaning in this context.

I was also expecting some check on whether the interrupts is valid for 
the vGIC.


> +        *r = vgic_reg32_extract(gather_irq_info_config(v, irq), info);
>          return 1;
> -    }
>
>      case VRANGE32(0xD00, 0xDFC):
>          goto read_impl_defined;
> @@ -534,15 +524,16 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info,
>          goto write_ignore_32;
>
>      case VRANGE32(GICD_ICFGR2, GICD_ICFGRN): /* SPIs */
> +    {
> +        uint32_t icfgr;
> +
>          if ( dabt.size != DABT_WORD ) goto bad_width;
> -        rank = vgic_rank_offset(v, 2, gicd_reg - GICD_ICFGR, DABT_WORD);
> -        if ( rank == NULL) goto write_ignore;
> -        vgic_lock_rank(v, rank, flags);
> -        vgic_reg32_update(&rank->icfg[REG_RANK_INDEX(2, gicd_reg - GICD_ICFGR,
> -                                                     DABT_WORD)],
> -                          r, info);
> -        vgic_unlock_rank(v, rank, flags);
> +        irq = (gicd_reg - GICD_ICFGR) * 4;

Ditto for the irq and the check.

> +        icfgr = gather_irq_info_config(v, irq);
> +        vgic_reg32_update(&icfgr, r, info);
> +        scatter_irq_info_config(v, irq, icfgr);
>          return 1;
> +    }
>
>      case VRANGE32(0xD00, 0xDFC):
>          goto write_impl_defined;
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index 10db939..7989989 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -521,20 +521,11 @@ static int __vgic_v3_distr_common_mmio_read(const char *name, struct vcpu *v,
>          return 1;
>
>      case VRANGE32(GICD_ICFGR, GICD_ICFGRN):
> -    {
> -        uint32_t icfgr;
> -
>          if ( dabt.size != DABT_WORD ) goto bad_width;
> -        rank = vgic_rank_offset(v, 2, reg - GICD_ICFGR, DABT_WORD);
> -        if ( rank == NULL ) goto read_as_zero;
> -        vgic_lock_rank(v, rank, flags);
> -        icfgr = rank->icfg[REG_RANK_INDEX(2, reg - GICD_ICFGR, DABT_WORD)];
> -        vgic_unlock_rank(v, rank, flags);
> -
> -        *r = vgic_reg32_extract(icfgr, info);
> -
> +        irq = (reg - GICD_IPRIORITYR) * 4;
> +        if ( irq >= v->domain->arch.vgic.nr_spis + 32 ) goto read_as_zero;

This should really be an helper.

> +        *r = vgic_reg32_extract(gather_irq_info_config(v, irq), info);
>          return 1;
> -    }
>
>      default:
>          printk(XENLOG_G_ERR
> @@ -636,17 +627,19 @@ static int __vgic_v3_distr_common_mmio_write(const char *name, struct vcpu *v,
>          goto write_ignore_32;
>
>      case VRANGE32(GICD_ICFGR + 4, GICD_ICFGRN): /* PPI + SPIs */
> +    {
> +        uint32_t icfgr;
> +
>          /* ICFGR1 for PPI's, which is implementation defined
>             if ICFGR1 is programmable or not. We chose to program */
>          if ( dabt.size != DABT_WORD ) goto bad_width;
> -        rank = vgic_rank_offset(v, 2, reg - GICD_ICFGR, DABT_WORD);
> -        if ( rank == NULL ) goto write_ignore;
> -        vgic_lock_rank(v, rank, flags);
> -        vgic_reg32_update(&rank->icfg[REG_RANK_INDEX(2, reg - GICD_ICFGR,
> -                                                     DABT_WORD)],
> -                          r, info);
> -        vgic_unlock_rank(v, rank, flags);
> +        irq = (reg - GICD_ICFGR) * 4;
> +        if ( irq >= v->domain->arch.vgic.nr_spis + 32 ) goto write_ignore;
> +        icfgr = gather_irq_info_config(v, irq);
> +        vgic_reg32_update(&icfgr, r, info);
> +        scatter_irq_info_config(v, irq, icfgr);
>          return 1;
> +    }
>
>      default:
>          printk(XENLOG_G_ERR
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 68eef47..02c1d12 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -235,6 +235,19 @@ static void set_priority(struct pending_irq *p, uint8_t prio)
>      p->new_priority = prio;
>  }
>
> +unsigned int extract_config(struct pending_irq *p)

Why this is exported? Also naming.

> +{
> +    return test_bit(GIC_IRQ_GUEST_EDGE, &p->status) ? 2 : 0;

Please document 0, 2. Likely with a some define.

> +}
> +
> +static void set_config(struct pending_irq *p, unsigned int config)

Naming.

> +{
> +    if ( config < 2 )

Ditto.

> +        clear_bit(GIC_IRQ_GUEST_EDGE, &p->status);
> +    else
> +        set_bit(GIC_IRQ_GUEST_EDGE, &p->status);
> +}
> +
>
>  #define DEFINE_GATHER_IRQ_INFO(name, get_val, shift)                         \
>  uint32_t gather_irq_info_##name(struct vcpu *v, unsigned int irq)            \
> @@ -267,6 +280,8 @@ void scatter_irq_info_##name(struct vcpu *v, unsigned int irq,               \
>  /* grep fodder: gather_irq_info_priority, scatter_irq_info_priority below */
>  DEFINE_GATHER_IRQ_INFO(priority, extract_priority, 8)
>  DEFINE_SCATTER_IRQ_INFO(priority, set_priority, 8)
> +DEFINE_GATHER_IRQ_INFO(config, extract_config, 2)
> +DEFINE_SCATTER_IRQ_INFO(config, set_config, 2)
>
>  bool vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq)
>  {
> @@ -357,27 +372,11 @@ 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;
>      struct pending_irq *p;
> -    unsigned int irq;
> +    unsigned int irq, int_type;
>      unsigned long flags;
>      int i = 0;
>      struct vcpu *v_target;
> @@ -392,6 +391,8 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
>          spin_lock(&p->lock);
>
>          set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
> +        int_type = test_bit(GIC_IRQ_GUEST_EDGE, &p->status) ?
> +                            IRQ_TYPE_EDGE_RISING : IRQ_TYPE_LEVEL_HIGH;
>
>          if ( !list_empty(&p->inflight) && !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
>              gic_raise_guest_irq(v_target, p);
> @@ -399,15 +400,15 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
>          spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags);
>          if ( p->desc != NULL )
>          {
> -            irq_set_affinity(p->desc, cpumask_of(v_target->processor));
>              spin_lock_irqsave(&p->desc->lock, flags);
> +            irq_set_affinity(p->desc, cpumask_of(v_target->processor));
>              /*
>               * The irq cannot be a PPI, we only support delivery of SPIs
>               * to guests.
>               */
>              ASSERT(irq >= 32);
>              if ( irq_type_set_by_domain(d) )
> -                gic_set_irq_type(p->desc, vgic_get_virq_type(v, n, i));
> +                gic_set_irq_type(p->desc, int_type);
>              p->desc->handler->enable(p->desc);
>              spin_unlock_irqrestore(&p->desc->lock, flags);
>          }
> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index 38a5e76..931a672 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -60,12 +60,15 @@ struct pending_irq
>       * vcpu while it is still inflight and on an GICH_LR register on the
>       * old vcpu.
>       *
> +     * GIC_IRQ_GUEST_EDGE: the IRQ is an edge triggered interrupt.

Also, explain that by default the interrupt will be level.

> +     *
>       */
>  #define GIC_IRQ_GUEST_QUEUED   0
>  #define GIC_IRQ_GUEST_ACTIVE   1
>  #define GIC_IRQ_GUEST_VISIBLE  2
>  #define GIC_IRQ_GUEST_ENABLED  3
>  #define GIC_IRQ_GUEST_MIGRATING   4
> +#define GIC_IRQ_GUEST_EDGE     5
>      unsigned long status;
>      struct irq_desc *desc; /* only set it the irq corresponds to a physical irq */
>      unsigned int irq;
> @@ -102,7 +105,6 @@ struct vgic_irq_rank {
>      uint8_t index;
>
>      uint32_t ienable;
> -    uint32_t icfg[2];
>
>      /*
>       * It's more convenient to store a target VCPU per vIRQ
> @@ -173,6 +175,9 @@ static inline int REG_RANK_NR(int b, uint32_t n)
>  uint32_t gather_irq_info_priority(struct vcpu *v, unsigned int irq);
>  void scatter_irq_info_priority(struct vcpu *v, unsigned int irq,
>                                 unsigned int value);
> +uint32_t gather_irq_info_config(struct vcpu *v, unsigned int irq);
> +void scatter_irq_info_config(struct vcpu *v, unsigned int irq,
> +                             unsigned int value);
>
>  #define VGIC_REG_MASK(size) ((~0UL) >> (BITS_PER_LONG - ((1 << (size)) * 8)))
>
>

Cheers,

-- 
Julien Grall

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