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

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

Hi Vijay,

On 05/26/2014 11:26 AM, vijay.kilari@gmail.com wrote:
> +static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
> +{

I didn't review closer this function. I guess it's a simple copy from
vgic_distr_mmio_read?

[..]

> +    /* Reserved -- read as zero */
> +    case 0x00c ... 0x01c:
> +    case 0x040 ... 0x07c:
> +    case 0x7fc:
> +    case 0xbfc:
> +    case 0xf04 ... 0xf0c:
> +    case 0xf30 ... 0xfcc:
> +        goto read_as_zero;

Hrrmm ... actually you dropped REG( ). Please specify every big changes
you made in the commit message.

[..]

> +static int vgic_v2_to_sgi(struct vcpu *v, register_t sgir)
> +{
> +
> +    int virq;
> +    int irqmode;
> +    unsigned long vcpu_mask = 0;
> +
> +    irqmode = (sgir  >> GICD_SGI_TARGET_LIST_SHIFT) & GICD_SGI_TARGET_LIST_MASK;

irqmode is an enum gic_sgi_mode, right? If so the type should be irqmode.

Futhermore in vgic_to_sgi you are using SGI_TARGET_* which as no
assigned value (see your enum). It doesn't sound right to blindly store
the architectural value in this enum...

> +    virq = (sgir & GICD_SGI_INTID_MASK);
> +    vcpu_mask = (sgir & GICD_SGI_TARGET_MASK) >> GICD_SGI_TARGET_SHIFT;
> +
> +    return vgic_to_sgi(v, sgir, irqmode, virq, vcpu_mask);
> +}
> +
> +static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
> +{

Same remark vgic_v2_distr_mmio_write. I didn't review closer this function.

[..]

> +    case GICD_ICFGR: /* SGIs */
> +        goto write_ignore;
> +    case GICD_ICFGR + 1: /* PPIs */
> +        /* It is implementation defined if these are writeable. We chose not */
> +        goto write_ignore;
> +    case GICD_ICFGR + 2 ... GICD_ICFGRN: /* SPIs */
> +        if ( dabt.size != DABT_WORD ) goto bad_width;
> +        rank = vgic_irq_rank(v, 2, gicd_reg - GICD_ICFGR, DABT_WORD);
> +        vgic_lock_rank(v, rank);
> +        if ( rank == NULL) goto write_ignore;

You've reintroduce the XSA-94 here (see bf70db7 vgic: Check rank in
GICD_ICFGR* emulation before locking). When you send a new version of a
serie, please check there is no update on this code which may fix error.

I saw you shared a part of the emulation between the distributor and the
redistributor in GICv3. I think you can also share with GICv2, this
could avoid fix in 2 places the same bug (or worst only fixing in 1 place).

[..]

> -static int vgic_to_sgi(struct vcpu *v, register_t sgir)
> +int vgic_to_sgi(struct vcpu *v, register_t sgir, int irqmode, int virq,

irqmode should be enum gic_sgi_mode.

> +                unsigned long vcpu_mask)

You can't assume that all the VCPU bits will fit in an unsigned long. We
will have to use cpumask_t at some point.

I'm fine if you don't handle it for now, but you need to write down
somewhere the limitation of this function.

[..]

> +    case SGI_TARGET_OTHERS:

> +    case SGI_TARGET_SELF:

For this 2 case, you can't assume that vcpu_mask will be equal to 0...
It comes from the GICD_SGIR...

> +    default:
> +        gdprintk(XENLOG_WARNING, "vGICD: unhandled GICD_SGIR write %"PRIregister" with wrong mode\n",
> +                 sgir);
> +       return 0;
>      }
>  
>      for_each_set_bit( vcpuid, &vcpu_mask, d->max_vcpus )
>      {
>          if ( !is_vcpu_running(d, vcpuid) )
>          {
> -            gdprintk(XENLOG_WARNING, "vGICD: GICD_SGIR write r=%"PRIregister" vcpu_mask=%lx, wrong CPUTargetList\n",
> +            gdprintk(XENLOG_WARNING, " write r=%"PRIregister" vcpu_mask=%lx, wrong CPUTargetList\n",

For clarity, I would keep at least vGIC in the warning message.

[..]

> @@ -654,7 +254,6 @@ out:
>  int domain_vgic_init(struct domain *d)
>  {
>      int i;
> -

Spurious change? And also invalid following the coding style.

>      d->arch.vgic.ctlr = 0;
>  
>      /* Currently nr_lines in vgic and gic doesn't have the same meanings
> @@ -665,20 +264,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");

As said on V3, panic is not the right solution in a domain creation code
path. You should return here.

Futhermore, gic_hw_version is returning an enum. I would use switch/case
there.

[..]

> +    if ( d->arch.vgic.pending_irqs == NULL )
> +    {
> +        xfree(d->arch.vgic.shared_irqs);
> +        return -ENOMEM;
> +    }
> +

Hrrrmmm... this change has been pused more than one month ago on master.

Regards,

-- 
Julien Grall