Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 8A804C433CB
Subject: Re: [PATCH v0] irqchip/gic-v3: Avoid check of lpi configuration for
 non existent cpu
To:     Marc Zyngier <maz@kernel.org>
Cc:     tglx@linutronix.de, linux-kernel@vger.kernel.org,
        linux-arm-msm@vger.kernel.org
References: <1575543357-31892-1-git-send-email-gkohli@codeaurora.org>
 <60f61282c1b1e512ca6ce638b6dfca09@www.loen.fr>
From:   Gaurav Kohli <gkohli@codeaurora.org>
Message-ID: <0101016ed626c22f-170ea43c-0db3-40aa-b360-21db453b391b-000000@us-west-2.amazonses.com>
Date:   Thu, 5 Dec 2019 13:01:34 +0000
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.9.1
MIME-Version: 1.0
In-Reply-To: <60f61282c1b1e512ca6ce638b6dfca09@www.loen.fr>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Feedback-ID: 1.us-west-2.CZuq2qbDmUIuT3qdvXlRHZZCpfZqZ4GtG9v3VKgRyF0=:AmazonSES
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


On 12/5/2019 6:17 PM, Marc Zyngier wrote:
> Hi Gaurav,
> 
> On 2019-12-05 10:55, Gaurav Kohli wrote:
>> As per GIC specification, we can configure gic for more no of cpus
>> then the available cpus in the soc, But this can cause mem abort
>> while iterating lpi region for non existent cpu as we don't map
> 
> Which LPI region? We're talking about RDs, right... Or does LPI mean
> something other than GIC LPIs for you?
> 

Yes RDs only.
>> redistrubutor region for non-existent cpu.
>>
>> To avoid this issue, put one more check of valid mpidr.
> 
> Sorry, but I'm not sure I grasp your problem. Let me try and rephrase it:
> 
> - Your GIC is configured for (let's say) 8 CPUs, and your SoC has only 4.
Yes, suppose gic is configured for 8 cpus but soc has only 4 cpus. Then 
in this case gic_iterate will iterate till it get TYPER_LAST.

But as gic is configured for 8, So last bit sets in eight redistributor 
regions only.
> 
> - As part of the probing, the driver iterates on the RD regions and 
> explodes
>    because something isn't mapped?
> 
> That'd be a grave bug, but I believe the issue is somewhere else.

There are 4 cpus present, that's why we have mapped 4 redistributor 
only, but during probe below function keeps iterating and give mem abort 
for 5th cpu.

static void gic_update_vlpi_properties(void)
{
         gic_iterate_rdists(__gic_update_vlpi_properties);

}

We can solve this problem by mapping all eight redistributor in dt, but 
ideally code should also able to handle this and we can avoid mappin?
>
>>
>> Signed-off-by: Gaurav Kohli <gkohli@codeaurora.org>
>>
>> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
>> index 1edc993..adc9186 100644
>> --- a/drivers/irqchip/irq-gic-v3.c
>> +++ b/drivers/irqchip/irq-gic-v3.c
>> @@ -766,6 +766,7 @@ static int gic_iterate_rdists(int (*fn)(struct
>> redist_region *, void __iomem *))
>>  {
>>      int ret = -ENODEV;
>>      int i;
>> +    int cpu = 0;
>>
>>      for (i = 0; i < gic_data.nr_redist_regions; i++) {
>>          void __iomem *ptr = gic_data.redist_regions[i].redist_base;
>> @@ -780,6 +781,7 @@ static int gic_iterate_rdists(int (*fn)(struct
>> redist_region *, void __iomem *))
>>          }
>>
>>          do {
>> +            cpu++;
>>              typer = gic_read_typer(ptr + GICR_TYPER);
>>              ret = fn(gic_data.redist_regions + i, ptr);
>>              if (!ret)
>> @@ -795,7 +797,8 @@ static int gic_iterate_rdists(int (*fn)(struct
>> redist_region *, void __iomem *))
>>                  if (typer & GICR_TYPER_VLPIS)
>>                      ptr += SZ_64K * 2; /* Skip VLPI_base + reserved 
>> page */
>>              }
>> -        } while (!(typer & GICR_TYPER_LAST));
>> +        } while (!(typer & GICR_TYPER_LAST) &&
>> +                    cpu_logical_map(cpu) != INVALID_HWID);
>>      }
>>
>>      return ret ? -ENODEV : 0;
> 
> This makes little sense. A redistributor region contains a bunch of RDs,
> each of which maps onto a given CPU. We iterate on the RDs, and not on the
> CPUs, as it is the RD that tells us which CPU it is affine with, not the
> other way around.
> 
> If a RD is for some reason unavailable, then it shouldn't be described in
> the firmware the first place. If you end-up exposing RD regions that do
> not have the last RD having GICR_TYPER.Last set, then your SoC is broken,
> and this needs yet another quirk.
> 
>          M.

-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.