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

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

Hi Vijay,

On 06/26/2014 06:34 AM, vijay.kilari@gmail.com wrote:

[..]

> +const struct mmio_handler_ops vgic_v2_distr_mmio_handler = {

There was a static on the previous location of this code. You should
keep it.

[..]

> +const static struct vgic_ops vgic_v2_ops = {

static const

[..]

>  int domain_vgic_init(struct domain *d)
>  {
>      int i;
> @@ -75,6 +67,15 @@ int domain_vgic_init(struct domain *d)
>      else
>          d->arch.vgic.nr_lines = 0; /* We don't need SPIs for the guest */
>  
> +    switch ( gic_hw_version() )
> +    {
> +    case GIC_V2:
> +        vgic_v2_init(d);

vgic_v2_init is returning an int to indicate if initialization has
succeeded or not. Please check the return to see if there is an error.

[..]

> -static int vgic_to_sgi(struct vcpu *v, register_t sgir)
> +/* TODO: unsigned long is used to fit vcpu_mask. Change to cpu_mask */
> +int vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, int virq,
> +                unsigned long vcpu_mask)
>  {

[..]

> +    case SGI_TARGET_OTHERS:

[..]

> +    case SGI_TARGET_SELF:
> +        set_bit(current->vcpu_id, &vcpu_mask);

I already said it on V4 and V5 and don't see any change here...

For SGI_TARGET_{OTHERS,SELF}, you can't assume that vcpu_mask will be
equal to 0...

it comes directly from GICD_SIGR. Please make sure to handle it or the
guest could send SGI to the wrong VCPUs.

[..]

> +struct vgic_ops {
> +    /* Initialize vGIC */
> +    int (*vcpu_init)(struct vcpu *v);
> +    /* Domain specific initialization of vGIC */
> +    int (*domain_init)(struct domain *d);
> +    /* SGI handler of vGIC */
> +    int (*send_sgi)(struct vcpu *v, register_t sgir);

You've introduce vgic_send_sgi here, I'm not sure to understand why...
Can you give a bit more input?

> +};
> +
> +extern enum gic_sgi_mode irqmode;
> +

Hu? What is used for?

Regards

-- 
Julien Grall