[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



On Tue, 2014-04-15 at 16:47 +0530, vijay.kilari@xxxxxxxxx wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxxxxxxxxxx>
> 
> 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@xxxxxxxxxxxxxxxxxx>
> ---
>  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
>  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@xxxxxxxxxxxxxxxxxx>
> + * 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)
> +
> +/*
> + * 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 */

These two comments should be merged.

What is a "byte register diff"? Do you mean offset in bytes?

The shift still confuses me, even with the comment. Isn't it shifting
the wrong way to convert something to bytes?

> +#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;

I'm confused again by this comment, if the input n is at word
granularity why divide and not multiple to get bytes? Or maybe the
comment is backwards and you have bytes and want a word sized offset?

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

Please can you do as you have done for the physical gic and name these
vgicv3 (and likewise in an earlier patch for v2).

> +static int __vgic_rdistr_mmio_read(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);
> +    u64 mpidr;
> +    u64 aff;

Please use uint64_t throughout.

> +
> +    switch ( gicr_reg )
> +    {
> +    case GICR_CTLR:
> +        /* We have implemented LPI's, read zero */
> +        goto read_as_zero;
> +    case GICR_IIDR:

Size check? Many of the cases which aren't read_as_zero seem to to miss
these.

> +        *r = GICR_IIDR_VAL;
> +        return 1;
> +    case GICR_TYPER:
> +        mpidr = cpu_logical_map(smp_processor_id());
> +        aff  = mpidr & ((1 << 24) - 1);
> +        aff |= (mpidr >> 8) & (0xffUL << 24);
> +        aff = aff << 32;
> +        aff = (aff | ((u64)smp_processor_id() << 8));
> +        aff = (aff | (0x9UL << 0));

Didn't you define a macro in an earlier patch to help with all this bit
manipulation?

Please also comment on the magic numbers, like 0x9UL, or use #defines.

Please either align = signs or don't, not a mixture.

> +        *r = aff;
> +        return 1;
> +    case GICR_STATUSR:
> +        /* Not implemented */

For optional registers which are not implemented can you please comment
as /* Optional register, unimplemented, RAZ/WI */ or something so we can
see at a glance why they are not implemented.

Since this is optional I suppose there must be a register somewhere
which announces the presence/absence? If so please can you add a comment
in the read of that register describing what the value you are returning
implies.

> +        goto read_as_zero;
> +    case GICR_WAKER:
> +        /* Not implemented */

Is this a secure only register? If yes then please say so in the
comment.

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

What is the defined/expected hardware behaviour if you read this
register? Read garbage or trap?

> +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 */

This is the write handler.

> +    case GICR_NSACR:
> +        if ( dabt.size != 2 ) goto bad_width;

Read ignored? Should this read as zero or??

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

v2 prints for both of these. Maybe that isn't wise? In any case a
comment here to explain what's happening would be nice.


> +    case GICR_ICFGR0:
> +        /* RO */

Missing goto.

> +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;

Please initialiase rdist_stride to a default which would cause this same
behaviour when it is not explicit and avoid the conditional here. 

> +
> +    if ( offset < SZ_64K )
> +       return __vgic_rdistr_mmio_read(v, info, offset);
> +    else if ( (offset - SZ_64K) < SZ_64K )

Can you write this as if ( offset > SZ_64K && offset < 2*SZ_64K )
please. (with <=/>= as appropriate)

> +       return vgic_rdistr_sgi_mmio_read(v, info, (offset - SZ_64K));
> +    else
> +       dprintk(XENLOG_WARNING, "vGICR: wrong gpa read address %"PRIpaddr"\n",

s/wrong/unknown/

I guess we don't know the rdist number here? Maybe it isn't worth
logging in any case.

> +               info->gpa);
> +    return 0;
> +}
> +
> +static int vgic_rdistr_mmio_write(struct vcpu *v, mmio_info_t *info)

All the same comments as the read case.

> +{
> +    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);

Please name this vgic_rdistr_FOO_mmio_write where FOO is some name to
distinguish from the sgi region. (likewise the read variant)

> +    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);
> +
> +    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
> +         */

I think this comment wuld be better at the definition of GICD_IIDR_VAL.

> +    case GICD_IROUTER ... GICD_IROUTERN:
> +        rank = vgic_irq_rank(v, 64, gicd_reg - GICD_IROUTERN);
> +        if ( rank == NULL) goto read_as_zero;
> +
> +        vgic_lock_rank(v, rank);
> +        /* IROUTER is 64 bit so, to make it byte size right shift by 3.
> +           Here once. macro REG_RANK_INDEX will do it twice */

Then REG_RANK_INDEX is buggy I think, since you pass it 64 indicating
that you want the index of a 64 bit register based on the given offset
and that is what it should return without needing to massage the input
as well.

> +    case GICD_PIDR7... GICD_PIDR0:

Width check (I expect elsewhere in this file too)

> +        /* GICv3 identification value */
> +        *r = GICD_PIDR0_GICV3;
> +        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):

What distinguishes these registers from those captured by the default
clause? Are these unimplemented implementation defined registers? Is
there a reason to expect a guest to legitimately prod them?

Please comment (or remove in favour of the default case).
+
> +    /* R/O -- write ignored */
> +    case GICD_TYPER:

Either fall through for all or none please.

> +    case GICD_ISENABLER ... GICD_ISENABLERN:
> +        if ( dabt.size != 2 ) goto bad_width;
> +        rank = vgic_irq_rank(v, 1, gicd_reg - GICD_ISENABLER);
> +        if ( rank == NULL) goto write_ignore;
> +        vgic_lock_rank(v, rank);
> +        tr = rank->ienable;
> +        rank->ienable |= *r;
> +        vgic_unlock_rank(v, rank);
> +        vgic_enable_irqs(v, (*r) & (~tr), (gicd_reg - GICD_ISENABLER) >> 2);


I'd prefer this shift to be part of the implementation if
vgic_enable_irqs if possible, or to use one of the macros which does
these sorts of conversions (or a new one if nothing existing suites)


> +        return 1;
> +    case GICD_ICENABLER ... GICD_ICENABLERN:
> +        if ( dabt.size != 2 ) goto bad_width;
> +        rank = vgic_irq_rank(v, 1, gicd_reg - GICD_ICENABLER);
> +        if ( rank == NULL) goto write_ignore;
> +        vgic_lock_rank(v, rank);
> +        tr = rank->ienable;
> +        rank->ienable &= ~*r;
> +        vgic_unlock_rank(v, rank);
> +        vgic_disable_irqs(v, (*r) & tr, gicd_reg - GICD_ICENABLER);

No shift for the disable case?

> +    case GICD_IROUTER ... GICD_IROUTER + 8*7:

What is 8*7? Please comment, or better yet use a #define.

> +        /* SGI/PPI target is read only */
> +        goto write_ignore;
> +    case GICD_IROUTER + 8*8 ... GICD_IROUTERN:
> +        rank = vgic_irq_rank(v, 64, gicd_reg - GICD_IROUTER);
> +        if ( rank == NULL) goto write_ignore;
> +        vgic_lock_rank(v, rank);
> +        rank->irouter[REG_RANK_INDEX(64, (gicd_reg - GICD_IROUTER)>>1)] = *r;
> +        vgic_unlock_rank(v, rank);
> +        return 1;
> +    case GICD_IPRIORITYR ... GICD_IPRIORITYRN:

> +    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: write unknown 0x00c 0xfcc  r%d offset %#08x\n", 
> dabt.reg, offset);

0x00c 0xfcc hardcoded in the message? Needs a comment or to be merged
with the default case.

> +static int vgic_domain_init(struct domain *d)
> +{
> +    int i;
> +
> +    vgic_distr_mmio_handler.addr = d->arch.vgic.dbase;
> +    vgic_distr_mmio_handler.size = d->arch.vgic.dbase_size;
> +    register_mmio_handler(d, &vgic_distr_mmio_handler);

Using a global struct like this is invalid since you store a pointer to
it and not a copy.

> +    for ( i = 0; i < d->arch.vgic.rdist_count; i++ )
> +    {
> +        vgic_rdistr_mmio_handler.addr = d->arch.vgic.rbase[i];
> +        vgic_rdistr_mmio_handler.size = d->arch.vgic.rbase_size[i];
> +        register_mmio_handler(d, &vgic_rdistr_mmio_handler);

Can we processes the rdistr stride here (or when we setup
d->arch.vgic.rbase*) and register the sgi region separately?

> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index f487784..0ad5e51 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -62,7 +62,9 @@ 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 )
> +    if ( gic_hw_version() == GIC_V3 )
> +        vgic_v3_init(d);
> +    else if ( gic_hw_version() == GIC_V2 )
>          vgic_v2_init(d);

When dealing with enums please use a switch.

>      else
>          panic("No VGIC found\n");
> 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

That comment about JDEC etc should go here somewhere I think.

> +#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];

itargets is v2 specific and irouter v3, right? In which case please do
    union {
        struct {
            uint32_t itargets[8];
        } v2;
        struct {
            uint64_t irouter[32];
        } v3;
    }

Ian.


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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.