Re: [PATCH V5 09/10] xen/arm: make domain_max_vcpus return value from vgic_ops

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

Hi Chen,

On 30/05/2015 12:07, 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 is 128 at the moment.
> So the domain_max_vcpus should return the value according to the vGIC
> the domain uses.
>
> 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        | 5 +++++
>   xen/arch/arm/domain_build.c  | 2 +-
>   xen/arch/arm/vgic-v2.c       | 6 ++++++
>   xen/arch/arm/vgic-v3.c       | 6 ++++++
>   xen/include/asm-arm/domain.h | 5 +----
>   xen/include/asm-arm/gic.h    | 3 +++
>   xen/include/asm-arm/vgic.h   | 2 ++
>   7 files changed, 24 insertions(+), 5 deletions(-)
>
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 0cf147c..c638ff5 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -890,6 +890,11 @@ void vcpu_block_unless_event_pending(struct vcpu *v)
>           vcpu_unblock(current);
>   }
>
> +unsigned int domain_max_vcpus(const struct domain *d)
> +{
> +    return d->arch.vgic.handler->get_max_vcpus();

As long as we keep MAX_VIRT_VCPUS in the common vGIC drivers, we need to 
make sure that we don't return a superior value because technically, 
with your changes, the vGICv3 could support more than 128 vCPUs...

I would do: return min(MAX_VIRT_CPUS, d->arch...)

> +}
> +
>   /*
>    * Local variables:
>    * mode: C
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 5591d82..88cfcee 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -769,7 +769,7 @@ static int make_cpus_node(const struct domain *d, void *fdt,
>            * is enough for the current max vcpu number.
>            */
>           mpidr_aff = vcpuid_to_vaffinity(cpu);
> -        DPRINT("Create cpu@%x node\n", mpidr_aff);
> +        DPRINT("Create cpu@%x (logical CPUID: %d) node\n", mpidr_aff, cpu);

This change belongs to patch #7...

>
>           snprintf(buf, sizeof(buf), "cpu@%x", mpidr_aff);
>           res = fdt_begin_node(fdt, buf);
> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> index 17a3c9f..206d289 100644
> --- a/xen/arch/arm/vgic-v2.c
> +++ b/xen/arch/arm/vgic-v2.c
> @@ -508,6 +508,11 @@ static struct vcpu *vgic_v2_get_target_vcpu(struct vcpu *v, unsigned int irq)
>       return v_target;
>   }
>
> +static int vgic_v2_get_max_vcpus(void)
> +{
> +    return GICV2_MAX_CPUS;
> +}
> +
>   static int vgic_v2_get_irq_priority(struct vcpu *v, unsigned int irq)
>   {
>       int priority;
> @@ -594,6 +599,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,
> +    .get_max_vcpus = vgic_v2_get_max_vcpus,
>   };
>
>   /*
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index 21d8d3f..e79b010 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -1048,6 +1048,11 @@ static int vgic_v3_emulate_sysreg(struct cpu_user_regs *regs, union hsr hsr)
>       }
>   }
>
> +static int vgic_v3_get_max_vcpus(void)
> +{
> +    return GICV3_MAX_CPUS;
> +};
> +
>   static const struct mmio_handler_ops vgic_rdistr_mmio_handler = {
>       .read_handler  = vgic_v3_rdistr_mmio_read,
>       .write_handler = vgic_v3_rdistr_mmio_write,
> @@ -1220,6 +1225,7 @@ const struct vgic_ops vgic_v3_ops = {
>       .get_irq_priority = vgic_v3_get_irq_priority,
>       .get_target_vcpu  = vgic_v3_get_target_vcpu,
>       .emulate_sysreg  = vgic_v3_emulate_sysreg,
> +    .get_max_vcpus = vgic_v3_get_max_vcpus,
>   };
>
>   /*
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index b7b5cd2..b525bec 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -261,10 +261,7 @@ struct arch_vcpu
>   void vcpu_show_execution_state(struct vcpu *);
>   void vcpu_show_registers(const struct vcpu *);
>
> -static inline unsigned int domain_max_vcpus(const struct domain *d)
> -{
> -    return MAX_VIRT_CPUS;
> -}
> +unsigned int domain_max_vcpus(const struct domain *);
>
>   /*
>    * Due to the restriction of GICv3, the number of vCPUs in AFF0 is
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index c6ef4bf..55f99cd 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -18,6 +18,9 @@
>   #ifndef __ASM_ARM_GIC_H__
>   #define __ASM_ARM_GIC_H__
>
> +#define GICV2_MAX_CPUS  8
> +#define GICV3_MAX_CPUS  128

This doesn't need to be exported and can go in each vGIC driver.

Furthermore, technically, the vGICv3 driver is able to support more than 
128 vCPUs...

> +
>   #define NR_GIC_LOCAL_IRQS  NR_LOCAL_IRQS
>   #define NR_GIC_SGI         16
>   #define MAX_RDIST_COUNT    4
> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index 2f413e1..1157b04 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -110,6 +110,8 @@ struct vgic_ops {
>       struct vcpu *(*get_target_vcpu)(struct vcpu *v, unsigned int irq);
>       /* vGIC sysreg emulation */
>       int (*emulate_sysreg)(struct cpu_user_regs *regs, union hsr hsr);
> +    /* Get the maximum number of vCPU supported */
> +    int (*get_max_vcpus)(void);

Why did you create a function? The value never change so a field value 
would have been better...

>   };
>
>   /* Number of ranks of interrupt registers for a domain */
>

-- 
Julien Grall

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