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

[Andre Przywara <andre.przywara@xxxxxxxxxx>]

Hi,

On 20/03/18 01:17, Julien Grall wrote:
> 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.

Fair enough.

>> +    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.

AFAICT (and have tested) at least my GCC never complains about
"default:", even if every enum member has already been used. I think
it's good style to catch those cases should the enum get extended for
some reason.
Plus we have this already in arch_domain_create() (in switch
get_hw_version()).

> 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.
>      */

Do we need this still with Andrew's latest patch?

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

Otherwise this smells like we need to have enum gic_version extended, to
reserve the 0 case? enum gic_version {GIC_INVALID, GIC_V2, GIC_V3};?

That should cover the not-yet-initialised case, shouldn't it?

Cheers,
Andre.

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