Re: [PATCH 6/8] arm: vgic: Split vgic_domain_init() functionality into two functions

[Julien Grall <julien.grall@arm.com>]

Hello Shanker,

On 19/06/16 00:45, Shanker Donthineni wrote:
> Split code that installs mmio handlers for GICD and Re-distributor
> regions to a new function. The intension of this separation is to defer
> steps that registers vgic_v2/v3 mmio handlers.

Looking at this patch and the follow-up ones, I don't think this is the 
right way to go. You differ the registration of the IO handlers just 
because you are unable to find the size of the handlers array.

I am wondering if the array for the handlers is the best solution here. 
On another side, it would be possible to find the maximum of handlers 
before hand.

> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 5df5f01..5b39e0d 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -151,9 +151,12 @@ int domain_vgic_init(struct domain *d, unsigned int nr_spis)
>       for ( i = 0; i < NR_GIC_SGI; i++ )
>           set_bit(i, d->arch.vgic.allocated_irqs);
>
> +    d->arch.vgic.handler->domain_register_mmio(d);
> +
>       return 0;
>   }
>
> +

Spurious change.

>   void register_vgic_ops(struct domain *d, const struct vgic_ops *ops)
>   {
>      d->arch.vgic.handler = ops;
> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index fbb763a..8fe65b4 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -132,6 +132,8 @@ struct vgic_ops {
>       void (*domain_free)(struct domain *d);
>       /* vGIC sysreg emulation */
>       int (*emulate_sysreg)(struct cpu_user_regs *regs, union hsr hsr);
> +    /* Register mmio handlers */
> +    void (*domain_register_mmio)(struct domain *d);
>       /* Maximum number of vCPU supported */
>       const unsigned int max_vcpus;
>   };
>

Regards,

-- 
Julien Grall

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