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

[Julien Grall <julien.grall@xxxxxxx>]

On 03/20/2018 05:11 PM, Andre Przywara wrote:
> 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.
Well, the compiler will usually tell you if you miss one case. In that 
circumstance you put a value that may not make sense for that new item 
in the enum and you will have some trouble to catch it.
So the default should really be a BUG() or ASSERT_UNREACHABLE().

> Plus we have this already in arch_domain_create() (in switch
> get_hw_version()).
See my point above.

> > 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?
Well depends which series is going first. If it is yours, then you still 
need it and Andrew has to fix it.
> > 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?
I think so. But see above.

Cheers,

--
Julien Grall

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