Re: [PATCH v3 14/16] xen/arm: Add virtual GICv3 support

[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:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> 
> Add virtual GICv3 driver support
> 
> This patch adds only basic v3 support.
> Does not support Interrupt Translation support (ITS)
> 
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> ---
>  xen/arch/arm/Makefile             |    2 +-
>  xen/arch/arm/vgic-v3.c            |  835 +++++++++++++++++++++++++++++++++++++
>  xen/arch/arm/vgic.c               |    4 +-
>  xen/include/asm-arm/gic_v3_defs.h |    2 +
>  xen/include/asm-arm/vgic.h        |    9 +-
>  5 files changed, 849 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 39166aa..d269191 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -26,7 +26,7 @@ obj-y += smpboot.o
>  obj-y += smp.o
>  obj-y += shutdown.o
>  obj-y += traps.o
> -obj-y += vgic.o vgic-v2.o
> +obj-y += vgic.o vgic-v3.o vgic-v2.o

Same as for patch #13, vgic-v3 should only be compiled on armv8 (i.e ARM64).

>  obj-y += vtimer.o
>  obj-y += vuart.o
>  obj-y += hvm.o
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> new file mode 100644
> index 0000000..e3d773a
> --- /dev/null
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -0,0 +1,835 @@
> +/*
> + * xen/arch/arm/vgic-v3.c
> + *
> + * ARM Virtual Generic Interrupt Controller v3 support
> + * based on xen/arch/arm/vgic.c
> + *
> + * Vijaya Kumar K <vijaya.kumar@caviumnetworks.com>
> + * Copyright (c) 2014 Cavium Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <xen/bitops.h>
> +#include <xen/config.h>
> +#include <xen/lib.h>
> +#include <xen/init.h>
> +#include <xen/softirq.h>
> +#include <xen/irq.h>
> +#include <xen/sched.h>
> +
> +#include <asm/current.h>
> +#include <asm/device.h>
> +
> +#include <asm/mmio.h>
> +#include <asm/gic_v3_defs.h>
> +#include <asm/gic.h>
> +#include <asm/vgic.h>
> +
> +#define REG(n) (n)

Why an id macro?

> +/*
> + * Offset of GICD_<FOO><n> with its rank, for GICD_<FOO> with
> + * <b>-bits-per-interrupt.
> + */
> +/* Shift n >> 2 to make it byte register diff */
> +#define REG_RANK_INDEX(b, n) (((n) >> 2) & ((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)
> +{
> +    int rank;
> +
> +    /* divide by 4 to make it byte size register difference
> +      as n is difference of 4 byte size register */
> +    n = n >> 2;
> +    rank  = REG_RANK_NR(b, n);
> +
> +    return vgic_get_irqrank(v, rank);
> +}
> +
> +static int vgic_read_priority(struct vcpu *v, int irq)
> +{
> +   struct vgic_irq_rank *rank = vgic_irq_rank(v, 8, irq);

Newline here for more clarity.

> +   return byte_read(rank->ipriority[REG_RANK_INDEX(8, irq)], 0, irq & 0x3);
> +}
> +
> +static int __vgic_rdistr_mmio_read(struct vcpu *v, mmio_info_t *info,
> +           uint32_t offset)
> +{

You have to check the size for all cases in this function...

> +    struct hsr_dabt dabt = info->dabt;
> +    struct cpu_user_regs *regs = guest_cpu_user_regs();
> +    register_t *r = select_user_reg(regs, dabt.reg);
> +    int gicr_reg = REG(offset);
> +    u64 mpidr;
> +    u64 aff;
> +
> +    switch ( gicr_reg )
> +    {
> +    case GICR_CTLR:
> +        /* We have implemented LPI's, read zero */

You are not constant in the comment. Did you implement LPI or not?

> +        goto read_as_zero;
> +    case GICR_IIDR:
> +        *r = GICR_IIDR_VAL;
> +        return 1;
> +    case GICR_TYPER:
> +        mpidr = cpu_logical_map(smp_processor_id());

Why do you use the physical CPU ID? Should not it be the vCPU ID?

FYI, VCPU are not necessary mapped 1:1 and can be moved.

> +        aff  = mpidr & ((1 << 24) - 1);
> +        aff |= (mpidr >> 8) & (0xffUL << 24);
> +        aff = aff << 32;
> +        aff = (aff | ((u64)smp_processor_id() << 8));
> +        aff = (aff | (0x9UL << 0));
> +        *r = aff;

You only use aff, mpidr in this case, right? If so, please define the
both variable in the case.

> +        return 1;
> +    case GICR_STATUSR:
> +        /* Not implemented */
> +        goto read_as_zero;

This register contains usefull bits for guest debugging. It would be
nice to implemented in the future.

> +    case GICR_WAKER:
> +        /* Not implemented */

Please explain why it's not implemented.

> +        goto read_as_zero;
> +    case GICR_SETLPIR:
> +    case GICR_CLRLPIR:
> +        /* WO return fail */
> +        return 0;

Returning 0 here will inject a data abort exception to the guest. I
don't think we want that.

>From the documentation (see 5.1.2, if you need more details I can
explain you privately) read on write-only register should be read 0.

There some other manner to handle it, but we don't care for now.

> +    case GICR_PROPBASER:
> +        /* LPI's not implemented */
> +        goto read_as_zero;

Hrmmm... you are read_as_zero is only able to handle 32-bits (see
dabt.size != 2). But PROPBASER is a 64 bit register.

> +    case GICR_PENDBASER:
> +        /* LPI's not implemented */
> +        goto read_as_zero;
> +    case GICR_INVLPIR:
> +    case GICR_INVALLR:
> +        /* WO return fail*/

Same remarks as GICR_SETLPIR...

> +        return 0;
> +    case GICR_SYNCR:
> +        if ( dabt.size != 2 ) goto bad_width;
> +        /* RO */
> +        /* Return as not busy */
> +        *r = GICR_SYNCR_NOT_BUSY;

Please explain why this value.

> +        return 1;
> +    case GICR_MOVLPIR:
> +    case GICR_MOVALLR:
> +        /* WO */

Same remarks as GICR_SETLPIR.

> +        return 0;
> +    case GICR_PIDR7... GICR_PIDR0:
> +        *r = GICR_PIDR0_GICV3;

Hrrmmm... no. PIDR1 should return 0xB (on ARM implementation)...

> +         return 1;
> +    default:
> +        printk("vGICR: read r%d offset %#08x\n not found", dabt.reg, offset);
> +        return 0;
> +    }
> +bad_width:
> +    printk("vGICD: bad read width %d r%d offset %#08x\n",
> +           dabt.size, dabt.reg, offset);
> +    domain_crash_synchronous();
> +    return 0;
> +
> +read_as_zero:
> +    if ( dabt.size != 2 ) goto bad_width;

As said previously this function only works for 32 bit access.

> +    *r = 0;
> +    return 1;
> +}
> +
> +static int __vgic_rdistr_mmio_write(struct vcpu *v, mmio_info_t *info,
> +          uint32_t offset)
> +{
> +    struct hsr_dabt dabt = info->dabt;
> +    struct cpu_user_regs *regs = guest_cpu_user_regs();
> +    register_t *r = select_user_reg(regs, dabt.reg);
> +    int gicr_reg = REG(offset);
> +
> +    switch ( gicr_reg )
> +    {
> +    case GICR_CTLR:
> +        /* We do not implement LPI's, read zero */
> +        goto write_ignore;
> +    case GICR_IIDR:
> +    case GICR_TYPER:
> +        /* RO */
> +        goto write_ignore;

Why do you sometime gather multiple case, and sometimes not? I would
prefer if you don't gather, as you do most of the times...

> +    case GICR_STATUSR:
> +        /* Not implemented */
> +        goto write_ignore;
> +    case GICR_WAKER:
> +        /* Not implemented */
> +        goto write_ignore;
> +    case GICR_SETLPIR:
> +        /* Not implemented */
> +        goto write_ignore;
> +    case GICR_CLRLPIR:
> +        /* Not implemented */
> +        goto write_ignore;

For all the case before, can you explain in a comment why you didn't
implement it?

> +    case GICR_PROPBASER:
> +        /* LPI is not implemented */
> +        goto write_ignore;
> +    case GICR_PENDBASER:
> +        /* LPI is not implemented */
> +        goto write_ignore;
> +    case GICR_INVLPIR:
> +        return 1;
> +    case GICR_INVALLR:
> +        return 1;
> +    case GICR_SYNCR:
> +        /* RO */
> +        goto write_ignore;
> +    case GICR_MOVLPIR:
> +        return 1;
> +    case GICR_MOVALLR:
> +        return 1;
> +    case GICR_PIDR7... GICR_PIDR0:
> +        /* RO */
> +        goto write_ignore;
> +    default:
> +        printk("vGICR: write r%d offset %#08x\n not found", dabt.reg, offset);
> +        return 0;
> +    }
> +bad_width:
> +    printk("vGICR: bad write width %d r%d=%"PRIregister" offset %#08x\n",
> +           dabt.size, dabt.reg, *r, offset);
> +    domain_crash_synchronous();
> +    return 0;
> +
> +write_ignore:
> +    if ( dabt.size != 2 ) goto bad_width;

This as the same issue as __vrdist_mmio_write.

[..]

> +static int vgic_rdistr_sgi_mmio_write(struct vcpu *v, mmio_info_t *info,
> +           uint32_t offset)
> +{

This function seems to be a copy of vgic_distr_mmio_write (with some
printk removed???? and s/GICD/GICR). Can you avoid duplicate code? It
will be easier to maintain.

[..]

> +    case GICR_ICENABLER0:
> +        if ( dabt.size != 2 ) goto bad_width;
> +        rank = vgic_irq_rank(v, 1, gicr_reg - GICR_ICENABLER0);
> +        if ( rank == NULL ) goto write_ignore;
> +        vgic_lock_rank(v, rank);
> +        rank->ienable &= ~*r;
> +        vgic_unlock_rank(v, rank);

You should call vgic_disable_irqs here.

> +        return 1;
> +    case GICR_ISPENDR0:
> +        if ( dabt.size != 0 && dabt.size != 2 ) goto bad_width;

printk....

> +        return 0;
> +    case GICR_ICPENDR0:
> +        if ( dabt.size != 0 && dabt.size != 2 ) goto bad_width;

printk...

> +        return 0;
> +    case GICR_ISACTIVER0:
> +        if ( dabt.size != 2 ) goto bad_width;
> +        rank = vgic_irq_rank(v, 1, gicr_reg - GICR_ISACTIVER0);
> +        if ( rank == NULL ) goto write_ignore;
> +        vgic_lock_rank(v, rank);
> +        rank->iactive &= ~*r;
> +        vgic_unlock_rank(v, rank);
> +        return 1;
> +    case GICR_ICACTIVER0:
> +        if ( dabt.size != 2 ) goto bad_width;
> +        rank = vgic_irq_rank(v, 1, gicr_reg - GICR_ICACTIVER0);
> +        if ( rank == NULL) goto write_ignore;
> +        vgic_lock_rank(v, rank);
> +        rank->iactive &= ~*r;
> +        vgic_unlock_rank(v, rank);
> +        return 1;
> +    case GICR_IPRIORITYR0...GICR_IPRIORITYR7:
> +        if ( dabt.size != 0 && dabt.size != 2 ) goto bad_width;
> +        rank = vgic_irq_rank(v, 8, gicr_reg - GICR_IPRIORITYR0);
> +        if ( rank == NULL) goto write_ignore;
> +        vgic_lock_rank(v, rank);
> +        if ( dabt.size == 2 )
> +            rank->ipriority[REG_RANK_INDEX(8, gicr_reg - GICR_IPRIORITYR0)] = *r;
> +        else
> +            byte_write(&rank->ipriority[REG_RANK_INDEX(8, gicr_reg - GICR_IPRIORITYR0)],
> +                       *r, offset);
> +        vgic_unlock_rank(v, rank);
> +        return 1;
> +    case GICR_ICFGR0:
> +        /* RO */
	   goto write_ignore;

And please specify why it's read-only.

> +    case GICR_ICFGR1:
> +        goto write_ignore;

Same remarks as GICR_ICFGR0.

> +static int vgic_rdistr_mmio_read(struct vcpu *v, mmio_info_t *info)
> +{
> +    uint32_t offset;
> +
> +    if ( !v->domain->arch.vgic.rdist_stride )
> +        offset = info->gpa & (v->domain->arch.vgic.rdist_stride - 1);
> +    else
> +        offset = info->gpa & 0x1FFFF;
> +
> +    if ( offset < SZ_64K )
> +       return __vgic_rdistr_mmio_read(v, info, offset);
> +    else if ( (offset - SZ_64K) < SZ_64K )
> +       return vgic_rdistr_sgi_mmio_read(v, info, (offset - SZ_64K));
> +    else
> +       dprintk(XENLOG_WARNING, "vGICR: wrong gpa read address %"PRIpaddr"\n",
> +               info->gpa);

gdprintk?

> +    return 0;
> +}
> +
> +static int vgic_rdistr_mmio_write(struct vcpu *v, mmio_info_t *info)
> +{
> +    uint32_t offset;
> +
> +    if ( !v->domain->arch.vgic.rdist_stride )
> +        offset = info->gpa & (v->domain->arch.vgic.rdist_stride - 1);
> +    else
> +        offset = info->gpa & 0x1FFFF;
> +
> +    if ( offset < SZ_64K )
> +       return __vgic_rdistr_mmio_write(v, info, offset);
> +    else if ( (offset - SZ_64K) < SZ_64K )
> +       return vgic_rdistr_sgi_mmio_write(v, info, (offset - SZ_64K));
> +    else
> +       dprintk(XENLOG_WARNING, "vGICR: wrong gpa write %"PRIpaddr"\n",
> +               info->gpa);
> +    return 0;
> +}
> +
> +static int vgic_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
> +{
> +    struct hsr_dabt dabt = info->dabt;
> +    struct cpu_user_regs *regs = guest_cpu_user_regs();
> +    register_t *r = select_user_reg(regs, dabt.reg);
> +    struct vgic_irq_rank *rank;
> +    int offset = (int)(info->gpa - v->domain->arch.vgic.dbase);
> +    int gicd_reg = REG(offset);
> +

Is it a copy of vgic v2 distr + few registers?

> +    switch ( gicd_reg )
> +    {
> +    case GICD_CTLR:
> +        if ( dabt.size != 2 ) goto bad_width;
> +        vgic_lock(v);
> +        *r = v->domain->arch.vgic.ctlr;
> +        vgic_unlock(v);
> +        return 1;
> +    case GICD_TYPER:
> +        if ( dabt.size != 2 ) goto bad_width;
> +        /* No secure world support for guests. */
> +        vgic_lock(v);
> +        *r = ( (v->domain->max_vcpus<<5) & GICD_TYPE_CPUS )
> +            |( ((v->domain->arch.vgic.nr_lines/32)) & GICD_TYPE_LINES );
> +        vgic_unlock(v);
> +        return 1;
> +    case GICD_STATUSR:
> +        /* Not implemented */
> +        goto read_as_zero;
> +    case GICD_IIDR:
> +        if ( dabt.size != 2 ) goto bad_width;
> +        /*
> +         * XXX Do we need a JEP106 manufacturer ID?
> +         * Just use the physical h/w value for now
> +         */
> +        *r = GICD_IIDR_VAL;
> +        return 1;
> +    /* Implementation defined -- read as zero */
> +    case REG(0x020) ... REG(0x03c):

Please move the comment after the case.

Also can you give a name to this register? For clarity.

[..]

> +    case GICD_PIDR7... GICD_PIDR0:
> +        /* GICv3 identification value */
> +        *r = GICD_PIDR0_GICV3;

Same as GICR_PIDR* in rdist read. This is wrong.

> +        return 1;
> +    case REG(0x00c):
> +    case REG(0x044):
> +    case REG(0x04c):
> +    case REG(0x05c) ... REG(0x07c):
> +    case REG(0xf30) ... REG(0x5fcc):
> +    case REG(0x8000) ... REG(0xbfcc):
> +    case REG(0xc000) ... REG(0xffcc):
> +        printk("vGICD: read unknown 0x00c .. 0xfcc r%d offset %#08x\n",
> +               dabt.reg, offset);

What are those registers?

> +        goto read_as_zero;
> +
> +    default:
> +        printk("vGICD: unhandled read r%d offset %#08x\n",
> +               dabt.reg, offset);
> +        return 0;
> +    }
> +
> +bad_width:
> +    dprintk(XENLOG_ERR, "vGICD: bad read width %d r%d offset %#08x\n",
> +           dabt.size, dabt.reg, offset);
> +    domain_crash_synchronous();
> +    return 0;
> +
> +read_as_zero:
> +    printk("vGICD: read as zero %d r%d offset %#08x\n",
> +           dabt.size, dabt.reg, offset);

You should remove this printk. It will call often.

> +    if ( dabt.size != 2 ) goto bad_width;
> +    *r = 0;
> +    return 1;
> +}
> +
> +static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
> +{
> +    struct hsr_dabt dabt = info->dabt;
> +    struct cpu_user_regs *regs = guest_cpu_user_regs();
> +    register_t *r = select_user_reg(regs, dabt.reg);
> +    struct vgic_irq_rank *rank;
> +    int offset = (int)(info->gpa - v->domain->arch.vgic.dbase);
> +    int gicd_reg = REG(offset);
> +    uint32_t tr;
> +
> +    switch ( gicd_reg )
> +    {
> +    case GICD_CTLR:
> +        if ( dabt.size != 2 ) goto bad_width;
> +        /* Ignore all but the enable bit */
> +        v->domain->arch.vgic.ctlr = (*r) & GICD_CTL_ENABLE;
> +        return 1;
> +
> +    /* R/O -- write ignored */
> +    case GICD_TYPER:


goto write_ignore.

> +    case GICD_IIDR:
> +        goto write_ignore;
> +    case GICD_STATUSR:
> +        goto write_ignore;
> +    case GICD_SETSPI_NSR:
> +        goto write_ignore;
> +    case GICD_CLRSPI_NSR:
> +        goto write_ignore;
> +    case GICD_SETSPI_SR:
> +        goto write_ignore;
> +    case GICD_CLRSPI_SR:
> +        goto write_ignore;
> +    /* Implementation defined -- write ignored */
> +    case REG(0x020) ... REG(0x03c):
> +        printk("vGICD: write unknown 0x020 - 0x03c r%d offset %#08x\n",
> +               dabt.reg, offset);

Please move the comment after the case.

[..]

> +static int vgic_vcpu_init(struct vcpu *v)
> +{
> +    int i;
> +    u64 affinity;
> +
> +    /* For SGI and PPI the target is always this CPU */
> +    affinity = cpu_logical_map(v->vcpu_id);

vcpu_id != physical ID.

> +    for ( i = 0 ; i < 32 ; i++ )
> +        v->arch.vgic.private_irqs->irouter[i] = affinity;
> +    return 0;
> +}

[..]

> diff --git a/xen/include/asm-arm/gic_v3_defs.h b/xen/include/asm-arm/gic_v3_defs.h
> index 578832b..f7f7932 100644
> --- a/xen/include/asm-arm/gic_v3_defs.h
> +++ b/xen/include/asm-arm/gic_v3_defs.h
> @@ -94,6 +94,8 @@
>  #define GICD_PIDR2_ARCH_MASK          (0xf0)
>  #define GICR_PIDR2_ARCH_MASK          GICD_PIDR2_ARCH_MASK
>  #define GICR_SYNCR_NOT_BUSY           1
> +#define GICD_IIDR_VAL                 0x34c

What does mean this value? How does Linux behave when this ID register
is read?

> +#define GICR_IIDR_VAL                 GICD_IIDR_VAL
>  
>  #define GICR_CTLR       (0x0000)
>  #define GICR_IIDR       (0x0004)
> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index 187846e..0de74eb 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -25,7 +25,10 @@ struct vgic_irq_rank {
>      uint32_t ienable, iactive, ipend, pendsgi;
>      uint32_t icfg[2];
>      uint32_t ipriority[8];
> -    uint32_t itargets[8];
> +    union {
> +        uint32_t itargets[8];
> +        uint64_t irouter[32];
> +   };

Can you add a comment why you use an union, and what VGIC version is
using what?

>  };
>  
>  struct vgic_ops {
> @@ -51,6 +54,9 @@ static inline int REG_RANK_NR(int b, uint32_t n)
>  {
>      switch ( b )
>      {
> +    case 64: return n >> 6;
> +    case 32: return n >> 5;
> +    case 16: return n >> 4;

It doesn't sounds right to update this common function. What happen if
VGIC v2 is trying to use this value?

>      case 8: return n >> 3;
>      case 4: return n >> 2;
>      case 2: return n >> 1;
> @@ -89,6 +95,7 @@ void byte_write(uint32_t *reg, uint32_t var, int offset);
>  struct vgic_irq_rank *vgic_get_irqrank(struct vcpu *v, int rank);
>  extern void register_vgic_ops(struct vgic_ops *ops);
>  int vgic_v2_init(struct domain *d);
> +int vgic_v3_init(struct domain *d);
>  
>  #endif
>  
> 

Regards,

-- 
Julien Grall