Re: [Qemu-devel] [PATCH v1 1/1] arm_gic: Include the GIC ArchRev in the ICPIDR2 register

[Alistair Francis]

On Mon, Jan 11, 2016 at 5:58 AM, Peter Maydell <address@hidden> wrote:
> On 8 January 2016 at 18:57, Alistair Francis
> <address@hidden> wrote:
>> The ARM GIC documentation (page 4-119) describes that bits
>> 7 to 4 of the ICPIDR2 register should include the GIC architecture
>> version. This patche ORs the version into the existing return value.
>>
>> Signed-off-by: Alistair Francis <address@hidden>
>> Tested-by: Sören Brinkmann <address@hidden>
>> ---
>>
>>  hw/intc/arm_gic.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
>> index 13e297d..f47d606 100644
>> --- a/hw/intc/arm_gic.c
>> +++ b/hw/intc/arm_gic.c
>> @@ -688,6 +688,10 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr 
>> offset, MemTxAttrs attrs)
>>      } else /* offset >= 0xfe0 */ {
>>          if (offset & 3) {
>>              res = 0;
>> +        } else if (offset == 0xfe8 && s->revision != REV_11MPCORE &&
>> +                                      s->revision != REV_NVIC) {
>> +            /* ICPIDR2 includes the GICv1 or GICv2 version information */
>> +            res = gic_id[(offset - 0xfe0) >> 2] | (s->revision << 4);
>>          } else {
>>              res = gic_id[(offset - 0xfe0) >> 2];
>>          }
>
> The current implementation of the ID registers seems to be
> simply "like the 11MPCore interrupt controller". I think we
> should get them right more generally if we're going to fix them:
>
>                        fd0 .. fdc     fe0 .. fec      ff0 ... ffc
> 11MPCore               reserved       90 13 04 00     0d f0 05 b1
> ARM GICv1 (eg A9)      04 00 00 00    90 b3 1b 00     0d f0 05 b1
> ARM GICv2 (eg A15)     04 00 00 00    90 b4 2b 00     0d f0 05 b1
>
> Your patch doesn't account for ICPIDR1 also having a field that
> changes between GICv1 and GICv2 (for ARM implementations), and
> we're missing the fd0..fdc bytes.
>
> I think this is probably simplest modelled with several
> gic_id arrays and using the appropriate one for 11MPCore/GICv1/GICv2,
> rather than trying to OR extra values into the 11MPCore ID values.

Ok, just to make sure I am reading this right. You think I should just
create three arrays and then if() the revision to determine which one
to use?

Thanks,

Alistair

>
> For the NVIC these registers don't exist at all, and the
> construction of the memory regions in armv7m_nvic.c ensures
> we can't reach this bit of the function, so we don't need
> to specially handle it. (Also there's a rewrite of the NVIC
> to disentangle it from the GIC which hopefully will land
> some time before 2.6.)
>
> thanks
> -- PMM
>