hw/intc/arm_gicv3: Fix GICD_TYPER ITLinesNumber advertisement

[PATCH] hw/intc/arm_gicv3: Fix GICD_TYPER ITLinesNumber advertisement

Posted by Luke Starrett 1 year, 4 months ago

The ARM GICv3 TRM describes that the ITLinesNumber field of GICD_TYPER
register:

"indicates the maximum SPI INTID that the GIC implementation supports"

As SPI #0 is absolute IRQ #32, the max SPI INTID should have accounted
for the internal 16x SGI's and 16x PPI's.  However, the original GICv3
model subtracted off the SGI/PPI.  Cosmetically this can be seen at OS
boot (Linux) showing 32 shy of what should be there, i.e.:

    [    0.000000] GICv3: 224 SPIs implemented

Though in hw/arm/virt.c, the machine is configured for 256 SPI's.  ARM
virt machine likely doesn't have a problem with this because the upper
32 IRQ's don't actually have anything meaningful wired. But, this does
become a functional issue on a custom use case which wants to make use
of these IRQ's.  Additionally, boot code (i.e. TF-A) will only init up
to the number (blocks of 32) that it believes to actually be there.

Signed-off-by: Luke Starrett <lukes@xsightlabs.com>
---
 hw/intc/arm_gicv3_dist.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/intc/arm_gicv3_dist.c b/hw/intc/arm_gicv3_dist.c
index eea0368118..d599fefcbc 100644
--- a/hw/intc/arm_gicv3_dist.c
+++ b/hw/intc/arm_gicv3_dist.c
@@ -390,9 +390,9 @@ static bool gicd_readl(GICv3State *s, hwaddr offset,
          * MBIS == 0 (message-based SPIs not supported)
          * SecurityExtn == 1 if security extns supported
          * CPUNumber == 0 since for us ARE is always 1
-         * ITLinesNumber == (num external irqs / 32) - 1
+         * ITLinesNumber == (((max SPI IntID + 1) / 32) - 1)
          */
-        int itlinesnumber = ((s->num_irq - GIC_INTERNAL) / 32) - 1;
+        int itlinesnumber = (s->num_irq / 32) - 1;
         /*
          * SecurityExtn must be RAZ if GICD_CTLR.DS == 1, and
          * "security extensions not supported" always implies DS == 1,
-- 
2.27.0

Re: [PATCH] hw/intc/arm_gicv3: Fix GICD_TYPER ITLinesNumber advertisement

Posted by Peter Maydell 1 year, 4 months ago

On Tue, 22 Nov 2022 at 18:31, Luke Starrett <lukes@xsightlabs.com> wrote:
>
> The ARM GICv3 TRM describes that the ITLinesNumber field of GICD_TYPER
> register:
>
> "indicates the maximum SPI INTID that the GIC implementation supports"
>
> As SPI #0 is absolute IRQ #32, the max SPI INTID should have accounted
> for the internal 16x SGI's and 16x PPI's.  However, the original GICv3
> model subtracted off the SGI/PPI.  Cosmetically this can be seen at OS
> boot (Linux) showing 32 shy of what should be there, i.e.:
>
>     [    0.000000] GICv3: 224 SPIs implemented
>
> Though in hw/arm/virt.c, the machine is configured for 256 SPI's.  ARM
> virt machine likely doesn't have a problem with this because the upper
> 32 IRQ's don't actually have anything meaningful wired. But, this does
> become a functional issue on a custom use case which wants to make use
> of these IRQ's.  Additionally, boot code (i.e. TF-A) will only init up
> to the number (blocks of 32) that it believes to actually be there.

Nice bricktext commit message :-)

> Signed-off-by: Luke Starrett <lukes@xsightlabs.com>
> ---
>  hw/intc/arm_gicv3_dist.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/intc/arm_gicv3_dist.c b/hw/intc/arm_gicv3_dist.c
> index eea0368118..d599fefcbc 100644
> --- a/hw/intc/arm_gicv3_dist.c
> +++ b/hw/intc/arm_gicv3_dist.c
> @@ -390,9 +390,9 @@ static bool gicd_readl(GICv3State *s, hwaddr offset,
>           * MBIS == 0 (message-based SPIs not supported)
>           * SecurityExtn == 1 if security extns supported
>           * CPUNumber == 0 since for us ARE is always 1
> -         * ITLinesNumber == (num external irqs / 32) - 1
> +         * ITLinesNumber == (((max SPI IntID + 1) / 32) - 1)
>           */
> -        int itlinesnumber = ((s->num_irq - GIC_INTERNAL) / 32) - 1;
> +        int itlinesnumber = (s->num_irq / 32) - 1;
>          /*
>           * SecurityExtn must be RAZ if GICD_CTLR.DS == 1, and
>           * "security extensions not supported" always implies DS == 1,

I always find interrupt number counting confusing because
there are multiple ways to do it...

In QEMU's GICv3 model, s->num_irq is the total number of interrupts,
including both PPIs and SPIs. So if s->num_irq is 256 that means
"the maximum SPI INTID is 255". The virt board code agrees with this:
it defines NUM_IRQS as 256 and sets the GIC num-irq property to
NUM_IRQS + 32.

The GICv3 spec says that if this GICR_TYPER.ITLinesNumber field
is N, then the maximum SPI INTID is 32(N+1)-1. Rearranging:
   max SPI intid == num_irq - 1 = 32 * (N+1) - 1
   num_irq = 32 * (N+1)
   num_irq / 32 = N + 1
   N = num_irq / 32 - 1

(We enforce that num_irq is a multiple of 32 in
arm_gicv3_common_realize(), so the division is fine.)

So yes, the setting of this field is wrong and the patch is correct.
I've applied the patch to my target-arm-for-8.0 branch and it
will go in once 7.2 is out in a few weeks' time. (This doesn't
seem to me like a release-critical bug because we've behaved this
way forever.)

Interestingly, we got this right in GICv2:

        if (offset == 4) {
            /* GICD_TYPER byte 0 */
            return ((s->num_irq / 32) - 1) | ((s->num_cpu - 1) << 5);
        }

but obviously got confused when doing GICv3...

thanks
-- PMM

Re: [PATCH] hw/intc/arm_gicv3: Fix GICD_TYPER ITLinesNumber advertisement

Posted by Luke Starrett 1 year, 4 months ago

On 11/25/2022 8:34 AM, Peter Maydell wrote:
> On Tue, 22 Nov 2022 at 18:31, Luke Starrett <lukes@xsightlabs.com> wrote:
>> The ARM GICv3 TRM describes that the ITLinesNumber field of GICD_TYPER
>> register:
>>
>> "indicates the maximum SPI INTID that the GIC implementation supports"
>>
>> As SPI #0 is absolute IRQ #32, the max SPI INTID should have accounted
>> for the internal 16x SGI's and 16x PPI's.  However, the original GICv3
>> model subtracted off the SGI/PPI.  Cosmetically this can be seen at OS
>> boot (Linux) showing 32 shy of what should be there, i.e.:
>>
>>      [    0.000000] GICv3: 224 SPIs implemented
>>
>> Though in hw/arm/virt.c, the machine is configured for 256 SPI's.  ARM
>> virt machine likely doesn't have a problem with this because the upper
>> 32 IRQ's don't actually have anything meaningful wired. But, this does
>> become a functional issue on a custom use case which wants to make use
>> of these IRQ's.  Additionally, boot code (i.e. TF-A) will only init up
>> to the number (blocks of 32) that it believes to actually be there.
> Nice bricktext commit message :-)
>
>> Signed-off-by: Luke Starrett <lukes@xsightlabs.com>
>> ---
>>   hw/intc/arm_gicv3_dist.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/intc/arm_gicv3_dist.c b/hw/intc/arm_gicv3_dist.c
>> index eea0368118..d599fefcbc 100644
>> --- a/hw/intc/arm_gicv3_dist.c
>> +++ b/hw/intc/arm_gicv3_dist.c
>> @@ -390,9 +390,9 @@ static bool gicd_readl(GICv3State *s, hwaddr offset,
>>            * MBIS == 0 (message-based SPIs not supported)
>>            * SecurityExtn == 1 if security extns supported
>>            * CPUNumber == 0 since for us ARE is always 1
>> -         * ITLinesNumber == (num external irqs / 32) - 1
>> +         * ITLinesNumber == (((max SPI IntID + 1) / 32) - 1)
>>            */
>> -        int itlinesnumber = ((s->num_irq - GIC_INTERNAL) / 32) - 1;
>> +        int itlinesnumber = (s->num_irq / 32) - 1;
>>           /*
>>            * SecurityExtn must be RAZ if GICD_CTLR.DS == 1, and
>>            * "security extensions not supported" always implies DS == 1,
> I always find interrupt number counting confusing because
> there are multiple ways to do it...
>
> In QEMU's GICv3 model, s->num_irq is the total number of interrupts,
> including both PPIs and SPIs. So if s->num_irq is 256 that means
> "the maximum SPI INTID is 255". The virt board code agrees with this:
> it defines NUM_IRQS as 256 and sets the GIC num-irq property to
> NUM_IRQS + 32.
>
> The GICv3 spec says that if this GICR_TYPER.ITLinesNumber field
> is N, then the maximum SPI INTID is 32(N+1)-1. Rearranging:
>     max SPI intid == num_irq - 1 = 32 * (N+1) - 1
>     num_irq = 32 * (N+1)
>     num_irq / 32 = N + 1
>     N = num_irq / 32 - 1
>
> (We enforce that num_irq is a multiple of 32 in
> arm_gicv3_common_realize(), so the division is fine.)
>
> So yes, the setting of this field is wrong and the patch is correct.
> I've applied the patch to my target-arm-for-8.0 branch and it
> will go in once 7.2 is out in a few weeks' time. (This doesn't
> seem to me like a release-critical bug because we've behaved this
> way forever.)

Thanks.  And yes, you're correct.  This is not urgent at all. Most 
likely the only way someone would expose this is by creating some custom 
platform/machine, that needed to use the upper (last) 32 SPI/interrupts 
(which we did).

> Interestingly, we got this right in GICv2:
>
>          if (offset == 4) {
>              /* GICD_TYPER byte 0 */
>              return ((s->num_irq / 32) - 1) | ((s->num_cpu - 1) << 5);
>          }
>
> but obviously got confused when doing GICv3...

Right. I noticed this as well and it kind of surprised me.  But the 
numbering convention is really what created this confusion in the first 
place.

Luke

>
> thanks
> -- PMM