Re: [PATCH 2/2] xen/arm: gic-v3: Allow Xen to run on hardware reporting GICv4

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

On 15/09/15 10:49, Ian Campbell wrote:
> On Mon, 2015-09-14 at 16:32 +0100, Julien Grall wrote:
>> It seems that there is some hardware which report start to report GICv4
> 
> s/report start to reports/reports/ ?

Yes, reports.

> Also, this is an odd way to express it, what you mean is that some hardware
> is now shipping with GICv4. Unless you are trying to imply that they are
> claiming to be GICv4 without actually being so?

GICv4 is an extension of GICv3 and should support any GICv3 feature.

It's not like GICv2 vs GICv3 where the GICv2 part is an optional feature
in the GICv3.

> (If we agree on some wording I can modify this text on commit, subject to
> the discussion below).


What about:

"The GICv4 is an extension of GICv3 (see 1.1 in ARM IHI 0069A) which
means that the GICv3 driver can run normally on a GICv4 hardware.

Though, the GICv4-only feature won't be used."

>> in the GIC*_PIDR2 register.
>>
>> As GICv4 is a superset of GICv3, it should just work on Xen.
>>
>> Reported-by: Andre Przywara <andre.przywara@arm.com>
>> Signed-off-by: Julien Grall <julien.grall@citrix.com>
>> ---
>>  xen/arch/arm/gic-v3.c             | 4 ++--
>>  xen/include/asm-arm/gic_v3_defs.h | 1 +
>>  2 files changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>> index 4d623bf..1e3c19b 100644
>> --- a/xen/arch/arm/gic-v3.c
>> +++ b/xen/arch/arm/gic-v3.c
>> @@ -640,7 +640,7 @@ static int __init gicv3_populate_rdist(void)
>>          void __iomem *ptr = gicv3.rdist_regions[i].map_base;
>>  
>>          reg = readl_relaxed(ptr + GICR_PIDR2) & GIC_PIDR2_ARCH_MASK;
>> -        if ( reg != GIC_PIDR2_ARCH_GICv3 )
>> +        if ( reg != GIC_PIDR2_ARCH_GICv3 && reg != GIC_PIDR2_ARCH_GICv4 )
> 
> Once we have GICv5, 6, etc this is going to get unwieldy, shall we switch
> to a switch now?

We don't have a spec for GICv5 and v6 right now. I would wait to see if
the driver can be re-used before doing a such change.

>>          {
>>              dprintk(XENLOG_ERR,
>>                      "GICv3: No redistributor present @%"PRIpaddr"\n",
> 
> I wonder if GICv3 ought to become GICv%d, on the other hand this is really
> the GICv3 driver driving a v4 in v3 "mode", so maybe v3 is the best
> logging.

We are logging the driver name and not the version. I'd like to keep
like that because it's simple and easy to grep.

Regards,

-- 
Julien Grall