Re: [PATCH v3 12/16] xen/arm: split vgic driver into generic and vgic-v2 driver

[Julien Grall <julien.grall@linaro.org>]

Hello Vijaya,

Thank you for the patch.

On 04/15/2014 12:17 PM, vijay.kilari@gmail.com wrote:

[..]

> +/*
> + * Offset of GICD_<FOO><n> with its rank, for GICD_<FOO> with
> + * <b>-bits-per-interrupt.
> + */
> +#define REG_RANK_INDEX(b, n) ((n) & ((b)-1))
> +
> +/*
> + * Returns rank corresponding to a GICD_<FOO><n> register for
> + * GICD_<FOO> with <b>-bits-per-interrupt.
> + */
> +static struct vgic_irq_rank *vgic_irq_rank(struct vcpu *v, int b, int n)

I think all the functions have to be named vgic_v2_...

It will improve readability for backtrace.

> +{
> +    int rank = REG_RANK_NR(b, n);
> +    return vgic_get_irqrank(v, rank);
> +}
> +
> +static int vgic_read_priority(struct vcpu *v, int irq)
> +{
> +   int idx = irq >> 2;
> +   struct vgic_irq_rank *rank = vgic_irq_rank(v, 8, idx);

New line here for clarity.

> +   return byte_read(rank->ipriority[REG_RANK_INDEX(8, idx)], 0, irq & 0x3);
> +}

[..]

> +static int vgic_vcpu_init(struct vcpu *v)
> +{
> +    int i;
> +
> +    /* For SGI and PPI the target is always this CPU */
> +    for ( i = 0 ; i < 8 ; i++ )
> +        v->arch.vgic.private_irqs->itargets[i] =
> +              (1<<(v->vcpu_id+0))
> +            | (1<<(v->vcpu_id+8))
> +            | (1<<(v->vcpu_id+16))
> +            | (1<<(v->vcpu_id+24));

Newline here for clarity.

> +    return 0;
> +}
> +
> +static int vgic_domain_init(struct domain *d)
> +{
> +    vgic_distr_mmio_handler.addr = d->arch.vgic.dbase;
> +    vgic_distr_mmio_handler.size = PAGE_SIZE;
> +    register_mmio_handler(d, &vgic_distr_mmio_handler);
> +    return 0;
> +}
> +
> +static struct vgic_ops ops = {

This function as to be const.

> +    .vgic_vcpu_init   = vgic_vcpu_init,
> +    .vgic_domain_init = vgic_domain_init,
> +    .read_priority    = vgic_read_priority,
> +};
> +
> +int vgic_v2_init(struct domain *d)
> +{

What domain is used for if you unconditionally register the ops?

> +    register_vgic_ops(&ops);

I think you need to pass the domain here. And register the ops per domain.

Will be easier for a later change to support V2 on V3.

> +    return 0;
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 7e258ae..f487784 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -28,29 +28,20 @@
>  #include <asm/current.h>
>  
>  #include <asm/mmio.h>
> -#include <asm/gic_v2_defs.h>
>  #include <asm/gic.h>
>  #include <asm/vgic.h>
>  
> -#define REG(n) (n/4)
>  
> -static struct mmio_handler vgic_distr_mmio_handler;
> +static struct vgic_ops *vgic_ops;

static const ?

>  
> -/*
> - * Offset of GICD_<FOO><n> with its rank, for GICD_<FOO> with
> - * <b>-bits-per-interrupt.
> - */
> -#define REG_RANK_INDEX(b, n) ((n) & ((b)-1))
> -
> -/*
> - * Returns rank corresponding to a GICD_<FOO><n> register for
> - * GICD_<FOO> with <b>-bits-per-interrupt.
> - */
> -static struct vgic_irq_rank *vgic_irq_rank(struct vcpu *v, int b, int n)
> +void register_vgic_ops(struct vgic_ops *ops)

const struct vgic_ops *ops ?

>  {
> -    int rank = REG_RANK_NR(b, n);
> +   vgic_ops = ops;
> +}
>  
> -    if ( rank == 0 )
> +struct vgic_irq_rank *vgic_get_irqrank(struct vcpu *v, int rank)
> +{
> +    if ( rank == 0 ) /* Rank 0 is nothing but GICR registers in GICv3 */
>          return v->arch.vgic.private_irqs;
>      else if ( rank <= DOMAIN_NR_RANKS(v->domain) )
>          return &v->domain->arch.vgic.shared_irqs[rank - 1];
> @@ -61,7 +52,6 @@ static struct vgic_irq_rank *vgic_irq_rank(struct vcpu *v, int b, int n)
>  int domain_vgic_init(struct domain *d)
>  {
>      int i;
> -
>      d->arch.vgic.ctlr = 0;
>  
>      /* Currently nr_lines in vgic and gic doesn't have the same meanings
> @@ -72,21 +62,34 @@ int domain_vgic_init(struct domain *d)
>      else
>          d->arch.vgic.nr_lines = 0; /* We don't need SPIs for the guest */
>  
> +    if ( gic_hw_version() == GIC_V2 )
> +        vgic_v2_init(d);
> +    else
> +        panic("No VGIC found\n");

panic when a domain is creating is not the right thing to do... You
should return here.

> +
>      d->arch.vgic.shared_irqs =
>          xzalloc_array(struct vgic_irq_rank, DOMAIN_NR_RANKS(d));
> +    if ( d->arch.vgic.shared_irqs == NULL )
> +        return -ENOMEM;
> +

Which version of Xen are you using? A patch to check shared_irqs has
been upstreamed a while ago.


> +    for ( i = 0; i < DOMAIN_NR_RANKS(d); i++ )
> +        spin_lock_init(&d->arch.vgic.shared_irqs[i].lock);
> +

Why did you move this code earlier?

> +    vgic_ops->vgic_domain_init(d);
> +

I think vgic_domain_init should be called at the end. Not in middle of
the initialization...

>      d->arch.vgic.pending_irqs =
>          xzalloc_array(struct pending_irq, d->arch.vgic.nr_lines);
> -    for (i=0; i<d->arch.vgic.nr_lines; i++)
> +    if ( d->arch.vgic.pending_irqs == NULL )
> +    {
> +        xfree(d->arch.vgic.shared_irqs);
> +        return -ENOMEM;
> +    }
> +

Patch already sent to fix this.

> +    for ( i = 0; i < d->arch.vgic.nr_lines; i++)

for ( ... )

>      {
>          INIT_LIST_HEAD(&d->arch.vgic.pending_irqs[i].inflight);
>          INIT_LIST_HEAD(&d->arch.vgic.pending_irqs[i].lr_queue);
>      }
> -    for (i=0; i<DOMAIN_NR_RANKS(d); i++)
> -        spin_lock_init(&d->arch.vgic.shared_irqs[i].lock);
> -
> -    vgic_distr_mmio_handler.addr = d->arch.vgic.dbase;
> -    vgic_distr_mmio_handler.size = PAGE_SIZE;
> -    register_mmio_handler(d, &vgic_distr_mmio_handler);
>      return 0;
>  }
>  
> @@ -105,9 +108,13 @@ int vcpu_vgic_init(struct vcpu *v)
>        return -ENOMEM;
>  
>      memset(v->arch.vgic.private_irqs, 0, sizeof(v->arch.vgic.private_irqs));
> -

Spurious change?

>      spin_lock_init(&v->arch.vgic.private_irqs->lock);
>  
> +    if ( vgic_ops )
> +       vgic_ops->vgic_vcpu_init(v);
> +    else
> +       panic("No VGIC ops found\n");
> +

No panic here... vgic_ops has been set before. So why should you check here?


Same question as for vgic_vpcu_init. Why did you move this code early?

>      memset(&v->arch.vgic.pending_irqs, 0, sizeof(v->arch.vgic.pending_irqs));
>      for (i = 0; i < 32; i++)
>      {
> @@ -115,13 +122,6 @@ int vcpu_vgic_init(struct vcpu *v)
>          INIT_LIST_HEAD(&v->arch.vgic.pending_irqs[i].lr_queue);
>      }
>  
> -    /* For SGI and PPI the target is always this CPU */
> -    for ( i = 0 ; i < 8 ; i++ )
> -        v->arch.vgic.private_irqs->itargets[i] =
> -              (1<<(v->vcpu_id+0))
> -            | (1<<(v->vcpu_id+8))
> -            | (1<<(v->vcpu_id+16))
> -            | (1<<(v->vcpu_id+24));
>      INIT_LIST_HEAD(&v->arch.vgic.inflight_irqs);
>      INIT_LIST_HEAD(&v->arch.vgic.lr_pending);
>      spin_lock_init(&v->arch.vgic.lock);
> @@ -135,7 +135,7 @@ int vcpu_vgic_free(struct vcpu *v)
>      return 0;
>  }
>  
> -static uint32_t byte_read(uint32_t val, int sign, int offset)
> +uint32_t byte_read(uint32_t val, int sign, int offset)

If you export this function, please rename it.

Also, I think it can be moved static inline in vgic.h.

>  {
>      int byte = offset & 0x3;
>  
> @@ -147,7 +147,7 @@ static uint32_t byte_read(uint32_t val, int sign, int offset)
>      return val;
>  }
>  
> -static void byte_write(uint32_t *reg, uint32_t var, int offset)
> +void byte_write(uint32_t *reg, uint32_t var, int offset)

Same remarks here.

[..]

>  struct pending_irq *irq_to_pending(struct vcpu *v, unsigned int irq)
>  {
>      struct pending_irq *n;
> @@ -666,10 +229,9 @@ void vgic_clear_pending_irqs(struct vcpu *v)
>  
>  void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
>  {
> -    int idx = irq >> 2, byte = irq & 0x3;
>      uint8_t priority;
> -    struct vgic_irq_rank *rank = vgic_irq_rank(v, 8, idx);
> -    struct pending_irq *iter, *n = irq_to_pending(v, irq);
> +    struct pending_irq *iter;
> +    struct pending_irq  *n = irq_to_pending(v, irq);
>      unsigned long flags;
>      bool_t running;
>  
> @@ -682,7 +244,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
>          return;
>      }
>  
> -    priority = byte_read(rank->ipriority[REG_RANK_INDEX(8, idx)], 0, byte);
> +    priority = vgic_ops->read_priority(v, irq);
>  
>      n->irq = irq;
>      set_bit(GIC_IRQ_GUEST_PENDING, &n->status);
> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index f9d6549..187846e 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -28,6 +28,12 @@ struct vgic_irq_rank {
>      uint32_t itargets[8];
>  };
>  
> +struct vgic_ops {

Please describe every callbacks of this structure.

> +    int (*vgic_vcpu_init)(struct vcpu *v);
> +    int (*vgic_domain_init)(struct domain *d);
> +    int (*read_priority)(struct vcpu *v, int irq);

I saw this function is assuming that some locks are used. Please write a
comment about it.

Regards,

-- 
Julien Grall