Re: [Xen-devel] [PATCH v5 16/21] xen/arm: split vgic driver into generic and vgic-v2 driver

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi Vijay,

You didn't address the comments I made on V4 for this patch.
See a copy of them inline...

On 12/06/14 14:36, vijay.kilari@xxxxxxxxx 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?

[..]

> +static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
Same question here?

[..]

> +    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;
> +        rank->icfg[REG_RANK_INDEX(2, gicd_reg - GICD_ICFGR, DABT_WORD)] = *r;
> +        vgic_unlock_rank(v, rank);
> +        return 1;
You've reintroduced 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).

[..]

> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index a6b1894..61c225f 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
[..]

>   int domain_vgic_init(struct domain *d)
>   {
[..]

> +    for ( i = 0; i < DOMAIN_NR_RANKS(d); i++ )
> +        spin_lock_init(&d->arch.vgic.shared_irqs[i].lock);
> +
Why did you move this 2 lines earlier? There were fine below...

>       d->arch.vgic.pending_irqs =
>           xzalloc_array(struct pending_irq, d->arch.vgic.nr_lines);
>       if ( d->arch.vgic.pending_irqs == NULL )
> @@ -88,18 +91,17 @@ int domain_vgic_init(struct domain *d)
>           INIT_LIST_HEAD(&d->arch.vgic.pending_irqs[i].inflight);
>           INIT_LIST_HEAD(&d->arch.vgic.pending_irqs[i].lr_queue);
>       }
> -    for (i=0; i<DOMAIN_NR_RANKS(d); i++)
> -        spin_lock_init(&d->arch.vgic.shared_irqs[i].lock);
Just here.

> -static int vgic_to_sgi(struct vcpu *v, register_t sgir)
> +int vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, 
> int virq,
> +                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.

Regards,

--
Julien Grall

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