Re: [Xen-devel] [RFC PATCH 08/49] ARM: VGIC: move max_vcpus VGIC limit to struct arch_domain

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 02/28/2018 12:32 PM, Andre Przywara wrote:
> Hi,
>
> On 09/02/18 19:27, Julien Grall wrote:
> > Hi,
> >
> > On 02/09/2018 02:38 PM, Andre Przywara wrote:
> > > The VGIC model used for a domain (GICv2 or GICv3) determines the maximum
> > > number of VCPUs for that guest, as GICv2 can only handle 8 processors.
> > > In the moment we carry this per-VGIC-model limit in the vgic_ops,
> > > alongside the model specific functions. That makes some sense, but
> > > exposes some current VGIC implementation details to generic Xen code.
> > > Add a new arch specific field in our domain structure to hold this
> > > vcpu limit,
> > > and initialize it when we set the ops. This allows us to plug in the new
> > > VGIC later without also needing to carry some ops structure.
> > >
> > > Signed-off-by: Andre Przywara <andre.przywara@xxxxxxxxxx>
> > > ---
> > >    xen/arch/arm/domain.c        | 5 ++---
> > >    xen/arch/arm/vgic.c          | 3 ++-
> > >    xen/include/asm-arm/domain.h | 1 +
> > >    3 files changed, 5 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> > > index a010443bfd..9ad4cd0a6e 100644
> > > --- a/xen/arch/arm/domain.c
> > > +++ b/xen/arch/arm/domain.c
> > > @@ -975,11 +975,10 @@ unsigned int domain_max_vcpus(const struct
> > > domain *d)
> > >         * allocation when the vgic_ops haven't been initialised yet,
> > >         * we return MAX_VIRT_CPUS if d->arch.vgic.handler is null.
> > >         */
> > > -    if ( !d->arch.vgic.handler )
> > > +    if ( !d->arch.max_vcpus )
> > >            return MAX_VIRT_CPUS;
> > >        else
> > > -        return min_t(unsigned int, MAX_VIRT_CPUS,
> > > -                     d->arch.vgic.handler->max_vcpus);
> > > +        return d->arch.max_vcpus;
> > >    }
> > >      /*
> > > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> > > index 9921769b15..5f47aa84a9 100644
> > > --- a/xen/arch/arm/vgic.c
> > > +++ b/xen/arch/arm/vgic.c
> > > @@ -166,7 +166,8 @@ int domain_vgic_init(struct domain *d, unsigned
> > > int nr_spis)
> > >      void register_vgic_ops(struct domain *d, const struct vgic_ops *ops)
> > >    {
> > > -   d->arch.vgic.handler = ops;
> > > +    d->arch.vgic.handler = ops;
> > > +    d->arch.max_vcpus = ops->max_vcpus;
> > >    }
> > >      void domain_vgic_free(struct domain *d)
> > > diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> > > index 1dd9683d25..2fef32eaee 100644
> > > --- a/xen/include/asm-arm/domain.h
> > > +++ b/xen/include/asm-arm/domain.h
> > > @@ -149,6 +149,7 @@ struct arch_domain
> > >    #ifdef CONFIG_SBSA_VUART_CONSOLE
> > >        struct vpl011 vpl011;
> > >    #endif
> > > +    unsigned int max_vcpus;
> > Now, you have max_vcpus defined in both arch_domain and domain. Which
> > makes the code very confusing to read. This is becoming apparent in the
> > check if (d->arch.max_vcpus > d->max_vcpus).
> True, though I was just copying the name from the vgic_ops ;-)
> I could suggest arch.vgic_vcpu_limit instead, but...
>
> > If you plan to ditch the ops, then I would prefer a check on the vGIC
> > version. Even if it means carrying vGIC specific implementation in
> > generic Xen code.
> So what about just providing per-VGIC implementations of
> domain_max_vcpus()? That seems to be the only user of this information,
> AFAICS? So I move this from xen/arch/arm/domain.c to
> xen/arch/arm{/vgic,}/vgic.c, where we can do all kind of internal VGIC
> tricks to learn this bit of information.
Sounds good to me.

Cheers,

--
Julien Grall

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