Re: [PATCH v2 14/15] xen/arm: Add vgic support for GIC v3

[Ian Campbell <Ian.Campbell@citrix.com>]

On Fri, 2014-04-04 at 17:26 +0530, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> 
> Add vgic driver support for GIC v3 version.

There is an awful lot of code duplication with the v2 stuff here,
especially with the distributor, but even the redistrubotr stuff looks
like it could share common implementation with the v2 distro for at
least some functionality.

I think we should investigate a sinlge driver here, or at least
refactoring anything which is common into a library of helper functions
which both can use.

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

Would the real hardware fault here? Or would it read unknown or zero or
os "WO" short for "write ignore"? (in which case goto write_ignore)


> +    case GICR_PROPBASER:
> +        /* LPI's not implemented */
> +        goto read_as_zero;
> +    case GICR_PENDBASER:
> +        /* LPI's not implemented */
> +        goto read_as_zero;
> +    case GICR_INVLPIR:
> +    case GICR_INVALLR:
> +        /* WO */
> +        return 0;
> +    case GICR_SYNCR:
> +        if ( dabt.size != 2 ) goto bad_width;
> +        /* RO */
> +        /* Return as not busy */
> +        *r = 1;

#define for the 1 please.

> +        return 1;
> +    case GICR_MOVLPIR:
> +    case GICR_MOVALLR:
> +        /* WO */
> +        return 0;
> +    case GICR_PIDR7... GICR_PIDR0:
> +        *r = 0x93;

Magic number?

> +         return 1;
> +    default:
> +        printk("vGICR: read r%d offset %#08x\n not found", dabt.reg, offset);

missing return 0.
> +
> +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;
> +    case GICR_STATUSR:
> +        /* Not implemented */
> +        goto write_ignore;
> +    case GICR_WAKER:
> +        /* Not implemented */
> +        goto write_ignore;
> +    case GICR_SETLPIR:

Missing a comment? (Not implemented? Write ignore?) (several others
here)

> +        return 1;
> +    case GICR_CLRLPIR:
> +        return 1;
> +    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);

missing return.

> +    }
> +bad_width:
> +    printk("vGICD: 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;
> +    return 1;
> +}
> +
> +
> +static int vgic_rdistr_mmio_read(struct vcpu *v, mmio_info_t *info)
> +{
> +    uint32_t offset;
> +
> +    if ( !v->domain->arch.vgic.info->rdist_stride )
> +        offset = info->gpa & (v->domain->arch.vgic.info->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));

I think you should either register two mmio handlers or maybe just
handle this all at once in a single callback.

I didn't see any system register handling, I suppose that is all handled
by the hardware GICV stuff?