Re: [PATCH v2 12/15] xen/arm: gic: Store the necessary HW information per vGIC ...

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

Hi Ian,

On 30/06/15 13:56, Ian Campbell wrote:
> On Fri, 2015-06-26 at 10:34 +0100, Julien Grall wrote:
>> ... in order to decouple the vGIC driver from the GIC driver.
>>
>> Each vGIC HW structure contains a boolean to indicate if the current GIC is
>> able to support this specific version of virtual GIC.
>>
>> Helpers have been introduced in order to help the GIC to setup correctly
>> the vGIC. The GIC will have to call them to announce the support of this
>> specific version.
>>
> 
> "...to help the GIC correctly setup the vGIC"
> 
> "...to announce support for this specific version"
> 
> 
>> Also drop fields that become unecessary in each global state.
> 
> "unnecessary"

I will fix it in the next version.

>> @@ -228,6 +232,55 @@ static inline int vgic_allocate_spi(struct domain *d)
>>  
>>  extern void vgic_free_virq(struct domain *d, unsigned int virq);
>>  
>> +struct vgic_v2_hw_config
>> +{
>> +    bool_t enabled;
>> +    /* Distributor interface address */
>> +    paddr_t dbase;
>> +    /* CPU interface address */
>> +    paddr_t cbase;
>> +    /* Virtual CPU interface address */
>> +    paddr_t vbase;
>> +};
>> +
>> +extern struct vgic_v2_hw_config vgic_v2_hw;
> 
> My inclination is to call this either vgic_v2_hwdom(_config) (since it
> is vgic config for the hw dom) or to call it gic_v2_hw_config (since it
> contains config info of the physical gic which we happen to be going to
> use for vgic).
> 
> I think given the expected usage the former makes more sense.

I think that the 2 names don't work for the usage of the structure:
	- vgic_v2_hwdom: vbase is used to map the guest CPU interface into the
virtual CPU interface
	- gic_v2_hw_config: it gives the impression to be physical GIC specific
rather the virtual GIC.

I would prefer to stick in vgic_v2_hw_config which show that we use it
for the virtual GIC.

>> +
>> +static inline void vgic_v2_setup_hw(paddr_t dbase, paddr_t cbase,
>> +                                    paddr_t vbase)
>> +{
>> +    vgic_v2_hw.enabled = 1;
>> +    vgic_v2_hw.dbase = dbase;
>> +    vgic_v2_hw.cbase = cbase;
>> +    vgic_v2_hw.vbase = vbase;
>> +}
> 
> If you were to move this out of line into vgic-v2.c would that mean that
> vgic_v2_hw_config etc could be static to that file?

No, we have to access the field enabled in domain_vgic_init to verify
the GIC is supporting the version of the vGIC.

Regards,

-- 
Julien Grall