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

[Chen Baozi <baozich@xxxxxxxxx>]

> On May 31, 2015, at 21:35, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> 
> Hi Chen,
> 
> On 30/05/2015 12:07, Chen Baozi wrote:
>> From: Chen Baozi <baozich@xxxxxxxxx>
>> 
>> 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@xxxxxxxxx>
>> ---
>>  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â

But is it appropriate that we define a field value in a struct called vgic_ops? 
I
thought âXXX_opsâ refers to a struct that is made up by function points. (FIXME)

Cheers,

Baozi.

Attachment: signature.asc
Description: Message signed with OpenPGP using GPGMail

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