| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <e0c378fc-15f8-0f7f-1fa7-9b498294b9bb@arm.com>
Date: Wed, 2 Aug 2023 08:14:36 +0530
From: Anshuman Khandual <anshuman.khandual@....com>
To: Will Deacon <will@...nel.org>
Cc: linux-arm-kernel@...ts.infradead.org, suzuki.poulose@....com,
Catalin Marinas <catalin.marinas@....com>,
Mark Rutland <mark.rutland@....com>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH V2 1/4] arm_pmu: acpi: Refactor
arm_spe_acpi_register_device()
On 8/1/23 20:19, Will Deacon wrote:
> On Tue, Aug 01, 2023 at 03:10:49PM +0530, Anshuman Khandual wrote:
>> Sanity checking all the GICC tables for same interrupt number, and ensuring
>> a homogeneous ACPI based machine, could be used for other platform devices
>> as well. Hence this refactors arm_spe_acpi_register_device() into a common
>> helper arm_acpi_register_pmu_device().
>>
>> Cc: Catalin Marinas <catalin.marinas@....com>
>> Cc: Will Deacon <will@...nel.org>
>> Cc: Mark Rutland <mark.rutland@....com>
>> Cc: linux-arm-kernel@...ts.infradead.org
>> Cc: linux-kernel@...r.kernel.org
>> Co-developed-by: Will Deacon <will@...nel.org>
>> Signed-off-by: Will Deacon <will@...nel.org>
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@....com>
>> ---
>> drivers/perf/arm_pmu_acpi.c | 110 +++++++++++++++++++++++-------------
>> 1 file changed, 70 insertions(+), 40 deletions(-)
>>
>> diff --git a/drivers/perf/arm_pmu_acpi.c b/drivers/perf/arm_pmu_acpi.c
>> index 90815ad762eb..d9d5a7bbb92f 100644
>> --- a/drivers/perf/arm_pmu_acpi.c
>> +++ b/drivers/perf/arm_pmu_acpi.c
>> @@ -70,6 +70,68 @@ static void arm_pmu_acpi_unregister_irq(int cpu)
>> }
>>
>> #if IS_ENABLED(CONFIG_ARM_SPE_PMU)
>> +static int
>> +arm_acpi_register_pmu_device(struct platform_device *pdev, u8 len,
>> + u16 (*parse_gsi)(struct acpi_madt_generic_interrupt *))
>> +{
>> + int cpu, hetid, irq, ret;
>> + bool matched = false;
>> + u16 gsi = 0;
>> +
>> + /*
>> + * Ensure that platform device must have IORESOURCE_IRQ
>> + * resource to hold gsi interrupt.
>> + */
>> + if (pdev->num_resources != 1)
>> + return -ENXIO;
>> +
>> + if (pdev->resource[0].flags != IORESOURCE_IRQ)
>> + return -ENXIO;
>> +
>> + /*
>> + * Sanity check all the GICC tables for the same interrupt
>> + * number. For now, only support homogeneous ACPI machines.
>> + */
>> + for_each_possible_cpu(cpu) {
>> + struct acpi_madt_generic_interrupt *gicc;
>> + u16 this_gsi;
>> +
>> + gicc = acpi_cpu_get_madt_gicc(cpu);
>> + if (gicc->header.length < len)
>> + return matched ? -ENXIO : 0;
>> +
>> + this_gsi = parse_gsi(gicc);
>> + if (!this_gsi)
>> + return matched ? -ENXIO : 0;
>
> I think you can push this check into the conditional below...
>
>> +
>> + if (!matched) {
>> + hetid = find_acpi_cpu_topology_hetero_id(cpu);
>
> ... i.e. add a:
>
> if (!this_gsi)
> return -ENXIO;
>
> here.
>
>> + gsi = this_gsi;
>> + matched = true;
>
> And then, come to think of it, can we get rid of 'matched' altogether?
> Since a gsi of 0 is treated as invalid, we could just check that instead,
> no? So this becomes:
>
>
> gicc = acpi_cpu_get_madt_gicc(cpu);
> if (gicc->header.length < len)
> return gsi ? -ENXIO : 0;
>
> this_hetid = find_acpi_cpu_topology_hetero_id(cpu);
> this_gsi = parse_gsi(gicc);
> if (!gsi) {
> if (!this_gsi)
> return -ENXIO;
Unlike before, this returns -ENXIO on the very first instance itself without
a match. Previously this returned 0 in such cases. But I guess that is okay.
> gsi = this_gsi;
> hetid = this_hetid;
> } else if (hetid != this_hetid || gsi != this_gsi) {
> pr_warn("ACPI: %s: must be homogeneous\n", pdev->name);
> return -ENXIO;
> }
>
>
> What do you reckon?
This makes sense, and the modified function will look something like
arm_acpi_register_pmu_device(struct platform_device *pdev, u8 len,
u16 (*parse_gsi)(struct acpi_madt_generic_interrupt *))
{
int cpu, irq, ret;
u16 gsi = 0;
/*
* Ensure that platform device must have IORESOURCE_IRQ
* resource to hold gsi interrupt.
*/
if (pdev->num_resources != 1)
return -ENXIO;
if (pdev->resource[0].flags != IORESOURCE_IRQ)
return -ENXIO;
/*
* Sanity check all the GICC tables for the same interrupt
* number. For now, only support homogeneous ACPI machines.
*/
for_each_possible_cpu(cpu) {
struct acpi_madt_generic_interrupt *gicc;
int this_hetid, hetid;
u16 this_gsi;
gicc = acpi_cpu_get_madt_gicc(cpu);
if (gicc->header.length < len)
return gsi ? -ENXIO : 0;
this_hetid = find_acpi_cpu_topology_hetero_id(cpu);
this_gsi = parse_gsi(gicc);
if (!gsi) {
if (!this_gsi)
return -ENXIO;
hetid = this_hetid;
gsi = this_gsi;
} else if (hetid != this_hetid || gsi != this_gsi) {
pr_warn("ACPI: %s: must be homogeneous\n", pdev->name);
return -ENXIO;
}
}
irq = acpi_register_gsi(NULL, gsi, ACPI_LEVEL_SENSITIVE, ACPI_ACTIVE_HIGH);
if (irq < 0) {
pr_warn("ACPI: %s Unable to register interrupt: %d\n", pdev->name, gsi);
return -ENXIO;
}
pdev->resource[0].start = irq;
ret = platform_device_register(pdev);
if (ret < 0) {
pr_warn("ACPI: %s: Unable to register device\n", pdev->name);
acpi_unregister_gsi(gsi);
}
return ret;
}
Powered by blists - more mailing lists