Re: [PATCH v7a 13/14] xen/arm: split vgic driver into generic and vgic-v2 driver

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

Hi Vijay,

You continue to ignore my comments on this patch... Please explain why
you don't want to make those changes. Sending 3 times the same patch
without any change is a waste of time for both of us.

On 07/01/2014 07:41 AM, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> 
> Existing vGIC driver has both generic code and hw specific
> code. Segregate vGIC low level driver into vgic-v2.c and
> keep generic code in existing vgic.c file

Missing "." at the end?

> some static generic functions in vgic.c is made as non-static

Uppercase for the first letter of the first word.

s/is/are/

[..]

> +const struct mmio_handler_ops vgic_v2_distr_mmio_handler = {

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

[..]

> +const static struct vgic_ops vgic_v2_ops = {
> +    .vcpu_init   = vgic_v2_vcpu_init,
> +    .domain_init = vgic_v2_domain_init,
> +    .send_sgi    = vgic_v2_to_sgi,
> +};

static const.

[..]

> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 78004b0..9f18347 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c

[..]

>  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 the initialization has
succeeded or not. Please check the return to see if an error occurred.

> -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:

[..]

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

I already said it on V4, V5 and now V6a and don't see any change or
comment from you side.

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

It comes directly guest write to GICD_SIGR. Please make sure to handle
it correctly 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 introduced vgic_send_sgi here, I'm not sure to understand why...
Can you give more input?

> +};
> +
>  /* Number of ranks of interrupt registers for a domain */
>  #define DOMAIN_NR_RANKS(d) (((d)->arch.vgic.nr_lines+31)/32)
>  
> @@ -133,6 +142,8 @@ static inline void vgic_byte_write(uint32_t *reg, uint32_t var, int offset)
>      *reg |= var;
>  }
>  
> +extern enum gic_sgi_mode irqmode;
> +

Hu? What is used for?

Regards,

-- 
Julien Grall