Re: [Xen-devel] [PATCH v2 40/45] ARM: new VGIC: vgic-init: register VGIC

[Julien Grall <julien.grall@xxxxxxx>]

Hi Andre,

On 03/15/2018 08:30 PM, Andre Przywara wrote:
> diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c
> index 002fec57e6..4b9664f313 100644
> --- a/xen/arch/arm/vgic/vgic.c
> +++ b/xen/arch/arm/vgic/vgic.c
> @@ -946,6 +946,28 @@ void vgic_sync_hardware_irq(struct domain *d,
>       spin_unlock_irqrestore(&desc->lock, flags);
>   }
>   
> +unsigned int vgic_max_vcpus(const struct domain *d)
> +{
> +    unsigned int vgic_vcpu_limit;
> +
> +    switch ( d->arch.vgic.version )
> +    {
> +#ifdef CONFIG_HAS_GICV3
> +    case GIC_V3:
> +        vgic_vcpu_limit = VGIC_V3_MAX_CPUS;
> +        break;
> +#endif
It is a bit strange that you handle GICV3 here but don't in 
domain_vgic_register.
> +    case GIC_V2:
> +        vgic_vcpu_limit = VGIC_V2_MAX_CPUS;
> +        break;
> +    default:
> +        vgic_vcpu_limit = MAX_VIRT_CPUS;
I feel this is a bit odd. We only support GICv2 and GICv3 and the enum 
has two values. Likely GCC will complain if CONFIG_HAS_GICV3 is set 
because default label is not used.
Lastly, I can't see how you handle the corner case mentioned in the 
current vGIC:
    /*
     * Since evtchn_init would call domain_max_vcpus for poll_mask
     * allocation when the vgic_ops haven't been initialised yet,
     * we return MAX_VIRT_CPUS if d->arch.vgic.handler is null.
     */

The comment in the code would also be very useful as the reason to call 
vgic_max_vcpus before the full initialization is not that straightforward.
Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel