Re: [Qemu-arm] [Qemu-devel] [PATCH v2 1/1] arm_gic: Update ID registers based on revision

[Peter Maydell]

On 20 January 2016 at 23:08, Alistair Francis
<address@hidden> wrote:
> On Wed, Jan 20, 2016 at 2:48 PM, Peter Maydell <address@hidden> wrote:
>> On 20 January 2016 at 22:42, Alistair Francis
>> <address@hidden> wrote:
>>> Update the GIC ID registers (registers above 0xfe0) based on the GIC
>>> revision instead of using the sames values for all GIC implementations.
>>>
>>> Signed-off-by: Alistair Francis <address@hidden>
>>> Tested-by: Sören Brinkmann <address@hidden>
>>> ---
>>> V2:
>>>  - Update array indexing to match new values
>>>  - Check the maximum value of offset as well
>>>
>>>  hw/intc/arm_gic.c | 35 ++++++++++++++++++++++++++++++-----
>>>  1 file changed, 30 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
>>> index 13e297d..8eb3a2d 100644
>>> --- a/hw/intc/arm_gic.c
>>> +++ b/hw/intc/arm_gic.c
>>> @@ -31,8 +31,16 @@ do { fprintf(stderr, "arm_gic: " fmt , ## __VA_ARGS__); 
>>> } while (0)
>>>  #define DPRINTF(fmt, ...) do {} while(0)
>>>  #endif
>>>
>>> -static const uint8_t gic_id[] = {
>>> -    0x90, 0x13, 0x04, 0x00, 0x0d, 0xf0, 0x05, 0xb1
>>> +static const uint8_t gic_id_11mpcore[] = {
>>> +    0x00, 0x00, 0x00, 0x00, 0x90, 0x13, 0x04, 0x00, 0x0d, 0xf0, 0x05, 0xb1
>>> +};
>>> +
>>> +static const uint8_t gic_id_gicv1[] = {
>>> +    0x04, 0x00, 0x00, 0x00, 0x90, 0xb3, 0x1b, 0x00, 0x0d, 0xf0, 0x05, 0xb1
>>> +};
>>> +
>>> +static const uint8_t gic_id_gicv2[] = {
>>> +    0x04, 0x00, 0x00, 0x00, 0x90, 0xb4, 0x2b, 0x00, 0x0d, 0xf0, 0x05, 0xb1
>>>  };
>>>
>>>  static inline int gic_get_current_cpu(GICState *s)
>>> @@ -683,13 +691,30 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr 
>>> offset, MemTxAttrs attrs)
>>>          }
>>>
>>>          res = s->sgi_pending[irq][cpu];
>>> -    } else if (offset < 0xfe0) {
>>> +    } else if (offset < 0xfd0) {
>>>          goto bad_reg;
>>> -    } else /* offset >= 0xfe0 */ {
>>> +    } else /* offset >= 0xfd0 */ {
>>>          if (offset & 3) {
>>>              res = 0;
>>> +        } else if (offset > 0xfdf) {
>>> +            goto bad_reg;
>>
>> Where did this condition come from? It will mean all
>> the ID regs fe0 and above end up treated as bad registers
>> rather than taking the code below to return the ID value.
>
> Woops, I copied the wrong value. I mean that to be 0xffc
>
> Do you still want a maximum check (otherwise it will walk off the array)?

FFD..FFF should read as zeroes (like the other non-multiple-of-4
offset addresses in this region), and you can't get more than 0xfff
because the size of the memory region is 0x1000. That's not
entirely obvious though, so if you want a max check you can
put one in. I think that having it match the style of the rest
of the if..else if ladder would be better though:

  ...
 else if (offset < 0x1000) {
     array code here;
 else {
     g_assert_not_reached();
 }

or something.

thanks
-- PMM