Xen on ARM GIC setup

Xen on ARM GIC setup

[Sander Bogaert]

Hi,

I was going over this part of the gic setup code ( gic.c:gic_dist_init ):

    type = GICD[GICD_TYPER];
    gic.lines = 32 * (type & GICD_TYPE_LINES);
    ....
    /* Disable all global interrupts */
    for ( i = 32; i < gic.lines; i += 32 )
        GICD[GICD_ICENABLER + i / 32] = (uint32_t)~0ul;

The ARM GIC manual says about the ITLinesNumber bits of the TYPE register:

Indicates the maximum number of interrupts that the GIC supports. If
ITLinesNumber=N, the
maximum number of interrupts is 32(N+1). The interrupt ID range is
from 0 to (number of IDs – 1). For example:
0b00011
Up to 128 interrupt lines, interrupt IDs 0-127.

Shouldn't it be gic.lines = 32 * ((type & GICD_TYPE_LINES)+1); ? I
might be wrong but it seems like this way we are missing the last
register in those loops?

Regards,
Sander

Re: Xen on ARM GIC setup

[Julien Grall]

On 06/04/2013 08:42 PM, Sander Bogaert wrote:

> Hi,

Hi,

> I was going over this part of the gic setup code ( gic.c:gic_dist_init ):
> 
>     type = GICD[GICD_TYPER];
>     gic.lines = 32 * (type & GICD_TYPE_LINES);
>     ....
>     /* Disable all global interrupts */
>     for ( i = 32; i < gic.lines; i += 32 )
>         GICD[GICD_ICENABLER + i / 32] = (uint32_t)~0ul;

It seems you have an old tree :). A commit (2f2a3e31) was pushed at the
end of april to fix this issue.

Cheers,

-- 
Julien

Re: Xen on ARM GIC setup

[Julien Grall]

I forgot to cc Sander.

On 06/04/2013 10:21 PM, Julien Grall wrote:

> On 06/04/2013 08:42 PM, Sander Bogaert wrote:
> 
>> Hi,
> 
> Hi,
> 
>> I was going over this part of the gic setup code ( gic.c:gic_dist_init ):
>>
>>     type = GICD[GICD_TYPER];
>>     gic.lines = 32 * (type & GICD_TYPE_LINES);
>>     ....
>>     /* Disable all global interrupts */
>>     for ( i = 32; i < gic.lines; i += 32 )
>>         GICD[GICD_ICENABLER + i / 32] = (uint32_t)~0ul;
> 
> It seems you have an old tree :). A commit (2f2a3e31) was pushed at the
> end of april to fix this issue.
> 
> Cheers,
> 

Re: Xen on ARM GIC setup

[Sander Bogaert]

On 4 June 2013 23:23, Julien Grall <julien.grall@citrix.com> wrote:
> I forgot to cc Sander.
>
> On 06/04/2013 10:21 PM, Julien Grall wrote:
>
>> On 06/04/2013 08:42 PM, Sander Bogaert wrote:
>>
>>> Hi,
>>
>> Hi,
>>
>>> I was going over this part of the gic setup code ( gic.c:gic_dist_init ):
>>>
>>>     type = GICD[GICD_TYPER];
>>>     gic.lines = 32 * (type & GICD_TYPE_LINES);
>>>     ....
>>>     /* Disable all global interrupts */
>>>     for ( i = 32; i < gic.lines; i += 32 )
>>>         GICD[GICD_ICENABLER + i / 32] = (uint32_t)~0ul;
>>
>> It seems you have an old tree :). A commit (2f2a3e31) was pushed at the
>> end of april to fix this issue.

Ok I see :) Having different trees because of the other issue I mailed
about and I was looking in the wrong one. But now that I'm at it
anyway, same part of the code:

    /* Default all global IRQs to level, active low */
    for ( i = 32; i < gic.lines; i += 16 )
        GICD[GICD_ICFGR + i / 16] = 0x0;

    /* Route all global IRQs to this CPU */
    for ( i = 32; i < gic.lines; i += 4 )
        GICD[GICD_ICFGR + i / 4] = cpumask;

Shouldn't that second loop use the GICD_ITARGETSR register? Just
aksing :-) Seems strange to set the same field with different values
right after eachother also the += 4 loop doesn't make sense for it
since it's two bit each interrupt line. It would make sense for
ITARGETSR.

>>
>> Cheers,
>>
>
>

Re: Xen on ARM GIC setup

[Ian Campbell]

On Wed, 2013-06-05 at 10:29 +0200, Sander Bogaert wrote:
> On 4 June 2013 23:23, Julien Grall <julien.grall@citrix.com> wrote:
> > I forgot to cc Sander.
> >
> > On 06/04/2013 10:21 PM, Julien Grall wrote:
> >
> >> On 06/04/2013 08:42 PM, Sander Bogaert wrote:
> >>
> >>> Hi,
> >>
> >> Hi,
> >>
> >>> I was going over this part of the gic setup code ( gic.c:gic_dist_init ):
> >>>
> >>>     type = GICD[GICD_TYPER];
> >>>     gic.lines = 32 * (type & GICD_TYPE_LINES);
> >>>     ....
> >>>     /* Disable all global interrupts */
> >>>     for ( i = 32; i < gic.lines; i += 32 )
> >>>         GICD[GICD_ICENABLER + i / 32] = (uint32_t)~0ul;
> >>
> >> It seems you have an old tree :). A commit (2f2a3e31) was pushed at the
> >> end of april to fix this issue.
> 
> Ok I see :) Having different trees because of the other issue I mailed
> about and I was looking in the wrong one. But now that I'm at it
> anyway, same part of the code:
> 
>     /* Default all global IRQs to level, active low */
>     for ( i = 32; i < gic.lines; i += 16 )
>         GICD[GICD_ICFGR + i / 16] = 0x0;
> 
>     /* Route all global IRQs to this CPU */
>     for ( i = 32; i < gic.lines; i += 4 )
>         GICD[GICD_ICFGR + i / 4] = cpumask;
> 
> Shouldn't that second loop use the GICD_ITARGETSR register? Just
> aksing :-) Seems strange to set the same field with different values
> right after eachother also the += 4 loop doesn't make sense for it
> since it's two bit each interrupt line. It would make sense for
> ITARGETSR.

Yeah, that looks like a bug...