Re: [RFC PATCH 05/10] ARM: vGIC: move priority 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 priority for newly triggered IRQs in the rank
> structure. To get eventually rid of this structure, move this value
> into the struct pending_irq. This one already contains a priority value,
> but we have to keep the two apart, as the priority for guest visible
> IRQs must not change while they are in a VCPU.
> This patch introduces a framework to get some per-IRQ information for a
> number of interrupts and collate them into one register. Similarily

s/Similarily/Similarly/

> there is the opposite function to spread values from one register into
> multiple pending_irq's.

Also, the last paragraph is a call to split the patch in two:
	1) Introducing the framework
	2) Move priority from irq_rank to struct pending_irq

>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  xen/arch/arm/vgic-v2.c     | 33 +++++++++--------------------
>  xen/arch/arm/vgic-v3.c     | 33 ++++++++++-------------------
>  xen/arch/arm/vgic.c        | 53 ++++++++++++++++++++++++++++++++++------------
>  xen/include/asm-arm/vgic.h | 17 ++++++---------
>  4 files changed, 67 insertions(+), 69 deletions(-)
>
> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> index dc9f95b..5c59fb4 100644
> --- a/xen/arch/arm/vgic-v2.c
> +++ b/xen/arch/arm/vgic-v2.c
> @@ -171,6 +171,7 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info,
>      struct vgic_irq_rank *rank;
>      int gicd_reg = (int)(info->gpa - v->domain->arch.vgic.dbase);
>      unsigned long flags;
> +    unsigned int irq;

s/irq/virq/

>
>      perfc_incr(vgicd_reads);
>
> @@ -250,22 +251,10 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info,
>          goto read_as_zero;
>
>      case VRANGE32(GICD_IPRIORITYR, GICD_IPRIORITYRN):
> -    {
> -        uint32_t ipriorityr;
> -
>          if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;
> -        rank = vgic_rank_offset(v, 8, gicd_reg - GICD_IPRIORITYR, DABT_WORD);
> -        if ( rank == NULL ) goto read_as_zero;
> -
> -        vgic_lock_rank(v, rank, flags);
> -        ipriorityr = rank->ipriorityr[REG_RANK_INDEX(8,
> -                                                     gicd_reg - GICD_IPRIORITYR,
> -                                                     DABT_WORD)];
> -        vgic_unlock_rank(v, rank, flags);
> -        *r = vgic_reg32_extract(ipriorityr, info);
> -
> +        irq = gicd_reg - GICD_IPRIORITYR;

This variable name does not make sense. This is not rely an irq but an 
offset.

> +        *r = vgic_reg32_extract(gather_irq_info_priority(v, irq), info);
>          return 1;
> -    }
>
>      case VREG32(0x7FC):
>          goto read_reserved;
> @@ -415,6 +404,7 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info,
>      int gicd_reg = (int)(info->gpa - v->domain->arch.vgic.dbase);
>      uint32_t tr;
>      unsigned long flags;
> +    unsigned int irq;

s/irq/virq/

>
>      perfc_incr(vgicd_writes);
>
> @@ -499,17 +489,14 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info,
>
>      case VRANGE32(GICD_IPRIORITYR, GICD_IPRIORITYRN):
>      {
> -        uint32_t *ipriorityr;
> +        uint32_t ipriorityr;
>
>          if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;
> -        rank = vgic_rank_offset(v, 8, gicd_reg - GICD_IPRIORITYR, DABT_WORD);
> -        if ( rank == NULL) goto write_ignore;
> -        vgic_lock_rank(v, rank, flags);
> -        ipriorityr = &rank->ipriorityr[REG_RANK_INDEX(8,
> -                                                      gicd_reg - GICD_IPRIORITYR,
> -                                                      DABT_WORD)];
> -        vgic_reg32_update(ipriorityr, r, info);
> -        vgic_unlock_rank(v, rank, flags);
> +        irq = gicd_reg - GICD_IPRIORITYR;
> +
> +        ipriorityr = gather_irq_info_priority(v, irq);
> +        vgic_reg32_update(&ipriorityr, r, info);
> +        scatter_irq_info_priority(v, irq, ipriorityr);
>          return 1;
>      }
>
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index d10757a..10db939 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -476,6 +476,7 @@ static int __vgic_v3_distr_common_mmio_read(const char *name, struct vcpu *v,
>      struct hsr_dabt dabt = info->dabt;
>      struct vgic_irq_rank *rank;
>      unsigned long flags;
> +    unsigned int irq;

s/irq/virq/

>
>      switch ( reg )
>      {
> @@ -513,22 +514,11 @@ static int __vgic_v3_distr_common_mmio_read(const char *name, struct vcpu *v,
>          goto read_as_zero;
>
>      case VRANGE32(GICD_IPRIORITYR, GICD_IPRIORITYRN):
> -    {
> -        uint32_t ipriorityr;
> -
>          if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;
> -        rank = vgic_rank_offset(v, 8, reg - GICD_IPRIORITYR, DABT_WORD);
> -        if ( rank == NULL ) goto read_as_zero;
> -
> -        vgic_lock_rank(v, rank, flags);
> -        ipriorityr = rank->ipriorityr[REG_RANK_INDEX(8, reg - GICD_IPRIORITYR,
> -                                                     DABT_WORD)];
> -        vgic_unlock_rank(v, rank, flags);
> -
> -        *r = vgic_reg32_extract(ipriorityr, info);
> -
> +        irq = reg - GICD_IPRIORITYR;
> +        if ( irq >= v->domain->arch.vgic.nr_spis + 32 ) goto read_as_zero;

This check will likely belong to an helper.

> +        *r = vgic_reg32_extract(gather_irq_info_priority(v, irq), info);
>          return 1;
> -    }
>
>      case VRANGE32(GICD_ICFGR, GICD_ICFGRN):
>      {
> @@ -572,6 +562,7 @@ static int __vgic_v3_distr_common_mmio_write(const char *name, struct vcpu *v,
>      struct vgic_irq_rank *rank;
>      uint32_t tr;
>      unsigned long flags;
> +    unsigned int irq;

s/irq/virq/

>
>      switch ( reg )
>      {
> @@ -630,16 +621,14 @@ static int __vgic_v3_distr_common_mmio_write(const char *name, struct vcpu *v,
>
>      case VRANGE32(GICD_IPRIORITYR, GICD_IPRIORITYRN):
>      {
> -        uint32_t *ipriorityr;
> +        uint32_t ipriorityr;
>
>          if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;
> -        rank = vgic_rank_offset(v, 8, reg - GICD_IPRIORITYR, DABT_WORD);
> -        if ( rank == NULL ) goto write_ignore;
> -        vgic_lock_rank(v, rank, flags);
> -        ipriorityr = &rank->ipriorityr[REG_RANK_INDEX(8, reg - GICD_IPRIORITYR,
> -                                                      DABT_WORD)];
> -        vgic_reg32_update(ipriorityr, r, info);
> -        vgic_unlock_rank(v, rank, flags);
> +        irq = reg - GICD_IPRIORITYR;
> +        if ( irq >= v->domain->arch.vgic.nr_spis + 32 ) goto write_ignore;

Ditto.

> +        ipriorityr = gather_irq_info_priority(v, irq);
> +        vgic_reg32_update(&ipriorityr, r, info);
> +        scatter_irq_info_priority(v, irq, ipriorityr);
>          return 1;
>      }
>
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 44363bb..68eef47 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -225,19 +225,49 @@ struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int virq)
>      return v->domain->vcpu[target];
>  }
>
> -static int vgic_get_virq_priority(struct vcpu *v, unsigned int virq)
> +static uint8_t extract_priority(struct pending_irq *p)

Please append vgic_

>  {
> -    struct vgic_irq_rank *rank = vgic_rank_irq(v, virq);
> -    unsigned long flags;
> -    int priority;
> +    return p->new_priority;
> +}
> +
> +static void set_priority(struct pending_irq *p, uint8_t prio)

Ditto.

> +{
> +    p->new_priority = prio;
> +}
> +
>
> -    vgic_lock_rank(v, rank, flags);
> -    priority = rank->priority[virq & INTERRUPT_RANK_MASK];
> -    vgic_unlock_rank(v, rank, flags);
> +#define DEFINE_GATHER_IRQ_INFO(name, get_val, shift)                         \
> +uint32_t gather_irq_info_##name(struct vcpu *v, unsigned int irq)            \

The name of this function are too generic. This should at least contain 
vgic.

Also I would like to keep the naming consistent with what we did with 
irouter and itargetr. I.e fetch/store.

Lastly, irq is confusing? How many irqs will it gather/scatter?

> +{                                                                            \
> +    uint32_t ret = 0, i;                                                     \

newline here.

> +    for ( i = 0; i < (32 / shift); i++ )                                     \

Why 32?

> +    {                                                                        \
> +        struct pending_irq *p = irq_to_pending(v, irq + i);                  \
> +        spin_lock(&p->lock);                                                 \

I am fairly surprised that you don't need to disable the interrupts 
here. the pending_irq lock will be used in vgic_inject_irq which will be 
called in the interrupt context.

> +        ret |= get_val(p) << (shift * i);                                    \
> +        spin_unlock(&p->lock);                                               \
> +    }                                                                        \
> +    return ret;                                                              \
> +}
>
> -    return priority;
> +#define DEFINE_SCATTER_IRQ_INFO(name, set_val, shift)                        \

Why do you need to define two separate macros? Both could be done at the 
same time.

> +void scatter_irq_info_##name(struct vcpu *v, unsigned int irq,               \

Same remarks as above.

> +                             unsigned int value)                             \
> +{                                                                            \
> +    unsigned int i;                                                          \

newline here.

> +    for ( i = 0; i < (32 / shift); i++ )                                     \
> +    {                                                                        \
> +        struct pending_irq *p = irq_to_pending(v, irq + i);                  \
> +        spin_lock(&p->lock);                                                 \
> +        set_val(p, (value >> (shift * i)) & ((1 << shift) - 1));             \
> +        spin_unlock(&p->lock);                                               \
> +    }                                                                        \
>  }

I do think those functions have to be introduced in a separate patch.

>
> +/* 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)
> +
>  bool vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq)
>  {
>      unsigned long flags;
> @@ -471,13 +501,10 @@ void vgic_clear_pending_irqs(struct vcpu *v)
>
>  void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq)
>  {
> -    uint8_t priority;
>      struct pending_irq *iter, *n = irq_to_pending(v, virq);
>      unsigned long flags;
>      bool running;
>
> -    priority = vgic_get_virq_priority(v, virq);
> -
>      spin_lock_irqsave(&v->arch.vgic.lock, flags);
>
>      /* vcpu offline */
> @@ -497,7 +524,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq)
>          goto out;
>      }
>
> -    n->priority = priority;
> +    n->priority = n->new_priority;
>
>      /* the irq is enabled */
>      if ( test_bit(GIC_IRQ_GUEST_ENABLED, &n->status) )
> @@ -505,7 +532,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq)
>
>      list_for_each_entry ( iter, &v->arch.vgic.inflight_irqs, inflight )
>      {
> -        if ( iter->priority > priority )
> +        if ( iter->priority > n->priority )
>          {
>              list_add_tail(&n->inflight, &iter->inflight);
>              spin_unlock(&n->lock);
> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index e7322fc..38a5e76 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -71,7 +71,8 @@ struct pending_irq
>      unsigned int irq;
>  #define GIC_INVALID_LR         (uint8_t)~0
>      uint8_t lr;
> -    uint8_t priority;
> +    uint8_t priority;           /* the priority of the currently inflight IRQ */
> +    uint8_t new_priority;       /* the priority of newly triggered IRQs */
>      /* inflight is used to append instances of pending_irq to
>       * vgic.inflight_irqs */
>      struct list_head inflight;
> @@ -104,16 +105,6 @@ struct vgic_irq_rank {
>      uint32_t icfg[2];
>
>      /*
> -     * Provide efficient access to the priority of an vIRQ while keeping
> -     * the emulation simple.
> -     * Note, this is working fine as long as Xen is using little endian.
> -     */
> -    union {
> -        uint8_t priority[32];
> -        uint32_t ipriorityr[8];
> -    };
> -
> -    /*
>       * It's more convenient to store a target VCPU per vIRQ
>       * than the register ITARGETSR/IROUTER itself.
>       * Use atomic operations to read/write the vcpu fields to avoid
> @@ -179,6 +170,10 @@ 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);
> +
>  #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