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

[Andre Przywara <andre.przywara@xxxxxxxxxx>]

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.

> This would also avoid to grow the struct domain just for a "constant
> field" used mostly at guest creation.

"Here’s a nickel, kid. Get yourself some memory." ;-)

Cheers,
Andre.

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