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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 31/05/15 19:05, Julien Grall wrote:
> Hi Chen,
>
> On 31/05/2015 16:31, Chen Baozi wrote:
>>> On May 31, 2015, at 21:35, Julien Grall <julien.grall@citrix.com>
>>> wrote:
>>>> +
>>>>   #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)
>
> Well if the only issue if the name, please rename the structure. IHMO,
> the name is fine for me, I will let Ian & Stefano decides about it.
>
> As the vGIC is always supporting N cpus and will never change, it's
> pointless to use a function (thinking about the "cost" vs access an
> field).

We have plenty of other structures with mixed function pointers and data.

A "const unsigned int max_vcpus;" should absolutely be used in a case
like this.

~Andrew