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

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

Hi Vijay,

On 05/26/2014 11:26 AM, 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)

You are also modify the vgic-v2 driver. Please update the commit message.

> +static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info,
> +                                        uint32_t gicr_reg)
> +{
> +    struct hsr_dabt dabt = info->dabt;
> +    struct cpu_user_regs *regs = guest_cpu_user_regs();
> +    register_t *r = select_user_reg(regs, dabt.reg);
> +    uint64_t mpidr;
> +    uint64_t aff;
> +
> +    switch ( gicr_reg )
> +    {
> +    case GICR_CTLR:
> +        /* We have not implemented LPI's, read zero */
> +        goto read_as_zero;
> +    case GICR_IIDR:
> +        if ( dabt.size != DABT_WORD ) goto bad_width;
> +        *r = GICV3_GICR_IIDR_VAL;
> +        return 1;
> +    case GICR_TYPER:
> +        if ( dabt.size != DABT_DOUBLE_WORD ) goto bad_width;
> +        /* TBD: Update processor id in [23:8] when ITS support is added */
> +        mpidr = cpu_logical_map(v->vcpu_id);

hu? cpu_logical_map contains the MPIDR for the physical CPUs not virtual
CPUs.

You should look at v->arch.vmpidr.

[..]

> +    case GICD_ICFGR: /* Restricted to configure SGIs */
> +        goto write_ignore;
> +    case GICD_ICFGR + 4 ... GICD_ICFGRN: /* PPI + SPIs */
> +        /* ICFGR1 for PPI's, which is implementation defined
> +           if ICFGR1 is programmable or not. We chose to program */
> +        if ( dabt.size != DABT_WORD ) goto bad_width;
> +        rank = vgic_irq_rank(v, 2, reg - GICD_ICFGR, DABT_WORD);
> +        vgic_lock_rank(v, rank);
> +        if ( rank == NULL) goto write_ignore;

You've blindly copied the code from GICv2 and keep the security issue.
This should be:

if ( rank == NULL ) ...
vgic_lock_rank(v, rank);

[..]

> +static int vgic_v3_distr_mmio_read(struct vcpu *v, mmio_info_t *info)

[..]

> +    case GICD_TYPER:
> +        if ( dabt.size != DABT_WORD ) goto bad_width;
> +        /* No secure world support for guests. */
> +        vgic_lock(v);

This is a copy from GICv2. I'm not sure if we need to take the vgic lock
here.

[..]

> +const static struct mmio_handler_ops vgic_rdistr_mmio_handler = {

static const

> +    .read_handler  = vgic_v3_rdistr_mmio_read,
> +    .write_handler = vgic_v3_rdistr_mmio_write,
> +};
> +
> +const static struct mmio_handler_ops vgic_distr_mmio_handler = {

static const

> +    .read_handler  = vgic_v3_distr_mmio_read,
> +    .write_handler = vgic_v3_distr_mmio_write,
> +};
> +
> +static int vgicv3_vcpu_init(struct vcpu *v)
> +{
> +    int i;
> +    uint64_t affinity;
> +
> +    /* For SGI and PPI the target is always this CPU */
> +    affinity = cpu_logical_map(smp_processor_id());

The SGI and PPI should be redirect to the virtual VCPU. In this case you
have to use the virtual CPU ID *not* the physical CPU ID.

> +    for ( i = 0 ; i < 32 ; i++ )
> +        v->arch.vgic.private_irqs->v3.irouter[i] = affinity;
> +
> +    return 0;
> +}
> +
> +static int vgicv3_domain_init(struct domain *d)
> +{
> +    int i;
> +
> +    register_mmio_handler(d, &vgic_distr_mmio_handler, d->arch.vgic.dbase,
> +                          d->arch.vgic.dbase_size);
> +
> +    /*
> +     * Register mmio handler per redistributor region but not for
> +     * every sgi rdist region which is per core.
> +     * The redistributor region encompasses per core sgi region.
> +     */
> +    for ( i = 0; i < d->arch.vgic.rdist_count; i++ )
> +        register_mmio_handler(d, &vgic_rdistr_mmio_handler,
> +            d->arch.vgic.rbase[i], d->arch.vgic.rbase_size[i]);
> +
> +    return 0;
> +}
> +
> +const static struct vgic_ops v3_ops = {

static const

[..]

> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 3fa0857..787c547 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -264,10 +264,19 @@ 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 )
> +    switch ( gic_hw_version() )
> +    {
> +#ifdef CONFIG_ARM_64
> +    case GIC_V3:
> +        vgic_v3_init(d);
> +        break;
> +#endif
> +    case GIC_V2:
>          vgic_v2_init(d);
> -    else
> +        break;
> +    default:
>          panic("No VGIC found\n");

I think I've already said in an earlier patch. Please avoid to use panic
when this function is called by a VM.

Regards,

-- 
Julien Grall