[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-arm] [Qemu-devel] [PATCH v2 1/1] arm_gic: Update ID registers
From: |
Alistair Francis |
Subject: |
Re: [Qemu-arm] [Qemu-devel] [PATCH v2 1/1] arm_gic: Update ID registers based on revision |
Date: |
Wed, 20 Jan 2016 15:49:04 -0800 |
On Wed, Jan 20, 2016 at 3:37 PM, Peter Maydell <address@hidden> wrote:
> 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();
> }
>
Ok, I like that.
Thanks,
Alistair
> or something.
>
> thanks
> -- PMM
>