Re: [PATCH v7 7/8] xen/arm: make domain_max_vcpus return value from vgic_ops

[Julien Grall <julien.grall@citrix.com>]

Hi Chen,

On 11/06/2015 09:05, Chen Baozi wrote:
> From: Chen Baozi <baozich@gmail.com>
>
> When a guest uses vGICv2, the maximum number of vCPU it can support
> should not be as many as MAX_VIRT_CPUS, which will be more than 8
> when GICv3 is used on arm64.

This sentence is not clear to me. What about:

"Each vGIC driver support a different maximum numbers of vCPUs. They are 
relying on the domain creation code to never create domain with vCPUs 
than their limitation.

The vGICv2 is limited to 8 vCPUs because of the specifications. In 
another side, the vGICv3 has been modified to support 2 level of 
affinity (AFF1 and AFF0) which theoretically allow a guest to use more 
than 4096."

 > So the domain_max_vcpus should return
> the value according to the vGIC the domain uses.

Which is not entirely true with your the new changes in this version. 
Please update the commit message.

> We didn't keep it as the old static inline form because it will break
> compilation when access the member of struct domain:
>
> In file included from xen/include/xen/domain.h:6:0,
>                   from xen/include/xen/sched.h:10,
>                   from arm64/asm-offsets.c:10:
> xen/include/asm/domain.h: In function ‘domain_max_vcpus’:
> xen/include/asm/domain.h:266:10: error: dereferencing pointer to incomplete type
>       if (d->arch.vgic.version == GIC_V2)
>            ^
>
> Signed-off-by: Chen Baozi <baozich@gmail.com>
> ---
>   xen/arch/arm/domain.c        | 9 +++++++++
>   xen/arch/arm/vgic-v2.c       | 1 +
>   xen/arch/arm/vgic-v3.c       | 7 +++++++
>   xen/include/asm-arm/domain.h | 5 +----
>   xen/include/asm-arm/vgic.h   | 2 ++
>   5 files changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 0cf147c..df71a60 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -890,6 +890,15 @@ void vcpu_block_unless_event_pending(struct vcpu *v)
>           vcpu_unblock(current);
>   }
>
> +unsigned int domain_max_vcpus(const struct domain *d)
> +{
> +    if ( !d->arch.vgic.handler )
> +        return MAX_VIRT_CPUS;

You should mention somewhere (commit message and maybe code) why this is 
necessary. Otherwise someone may want to remove it later by mistake.

> +    else
> +        return min_t(unsigned int, MAX_VIRT_CPUS,
> +                     d->arch.vgic.handler->max_vcpus);
> +}
> +
>   /*
>    * Local variables:
>    * mode: C
> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> index 5949cf1..bbeb740 100644
> --- a/xen/arch/arm/vgic-v2.c
> +++ b/xen/arch/arm/vgic-v2.c
> @@ -585,6 +585,7 @@ const struct vgic_ops vgic_v2_ops = {
>       .domain_init = vgic_v2_domain_init,
>       .get_irq_priority = vgic_v2_get_irq_priority,
>       .get_target_vcpu = vgic_v2_get_target_vcpu,
> +    .max_vcpus = 8,
>   };
>
>   /*
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index 93610d0..417729b 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -32,6 +32,12 @@
>   #include <asm/gic.h>
>   #include <asm/vgic.h>
>
> +/*
> + * We will use both AFF1 and AFF0 in (v)MPIDR. Thus, the max number of CPU
> + * that can be supported is up to 4096(256*16) in theory.
> + */
> +#define GICV3_MAX_CPUS  4096
> +

As you did for GICv2, please remove the define and opencode the value 
below. You can move the comment on top of the field assignation.

Regards,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel