linux-kernel - Re: [PATCH V12 01/10] APCI: irq: Add support for multiple GSI domains

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <81ec8b2b-b86f-a36a-966a-688161ce9b57@huawei.com>
Date:   Tue, 28 Jun 2022 17:12:24 +0800
From:   Hanjun Guo <guohanjun@...wei.com>
To:     Jianmin Lv <lvjianmin@...ngson.cn>, Marc Zyngier <maz@...nel.org>
CC:     Thomas Gleixner <tglx@...utronix.de>,
        <linux-kernel@...r.kernel.org>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        Jiaxun Yang <jiaxun.yang@...goat.com>,
        Huacai Chen <chenhuacai@...ngson.cn>
Subject: Re: [PATCH V12 01/10] APCI: irq: Add support for multiple GSI domains

On 2022/6/28 16:45, Jianmin Lv wrote:
> 
> 
> On 2022/6/28 下午3:42, Hanjun Guo wrote:
>> On 2022/6/18 18:36, Marc Zyngier wrote:
>> [...]
>>>>>> --- a/drivers/acpi/irq.c
>>>>>> +++ b/drivers/acpi/irq.c
>>>>>> @@ -12,7 +12,7 @@
>>>>>>      enum acpi_irq_model_id acpi_irq_model;
>>>>>>    -static struct fwnode_handle *acpi_gsi_domain_id;
>>>>>> +static struct fwnode_handle *(*acpi_get_gsi_domain_id)(u32 gsi);
>>>>>>      /**
>>>>>>     * acpi_gsi_to_irq() - Retrieve the linux irq number for a 
>>>>>> given GSI
>>>>>> @@ -26,10 +26,7 @@
>>>>>>     */
>>>>>>    int acpi_gsi_to_irq(u32 gsi, unsigned int *irq)
>>>>>>    {
>>>>>> -    struct irq_domain *d = 
>>>>>> irq_find_matching_fwnode(acpi_gsi_domain_id,
>>>>>> -                            DOMAIN_BUS_ANY);
>>>>>> -
>>>>>> -    *irq = irq_find_mapping(d, gsi);
>>>>>> +    *irq = acpi_register_gsi(NULL, gsi, -1, -1);
>>>>>
>>>>> What is this?
>>>>>
>>>>> - This wasn't part of my initial patch, and randomly changing patches
>>>>>     without mentioning it isn't acceptable
>>>>>
>>>>> - you *cannot* trigger a registration here, as this isn't what the API
>>>>>     advertises
>>>>>
>>>>> - what makes you think that passing random values (NULL, -1... )to
>>>>>     acpi_register_gsi() is an acceptable thing to do?
>>>>>
>>>>> The original patch had:
>>>>>
>>>>> @@ -26,8 +26,10 @@ static struct fwnode_handle *acpi_gsi_domain_id;
>>>>>      */
>>>>>     int acpi_gsi_to_irq(u32 gsi, unsigned int *irq)
>>>>>     {
>>>>> -    struct irq_domain *d = 
>>>>> irq_find_matching_fwnode(acpi_gsi_domain_id,
>>>>> -                            DOMAIN_BUS_ANY);
>>>>> +    struct irq_domain *d;
>>>>> +
>>>>> +    d = irq_find_matching_fwnode(acpi_get_gsi_domain_id(gsi),
>>>>> +                     DOMAIN_BUS_ANY);
>>>>>           *irq = irq_find_mapping(d, gsi);
>>>>>         /*
>>>>>
>>>>> and I don't think it needs anything else. If something breaks, let's
>>>>> discuss it, but don't abuse the API nor the fact that I usually don't
>>>>> review my own patches to sneak things in...
>>>>>
>>>>
>>>> Sorry, Marc, I don't know how to communicate with you for my change
>>>> here before submitting the patch, maybe I should mention it in the
>>>> patch commit or code.
>>>
>>> It should at least be discussed first, like you are doing it here.
>>>
>>>> When I use the patch, I found that acpi_gsi_to_irq in driver/acpi/irq.c
>>>> only handle existed mapping and will return -EINVAL if mapping not
>>>> found. When I test on my machine, a calling stack is as following:
>>>>
>>>>
>>>> acpi_bus_init
>>>> ->acpi_enable_subsystem
>>>>    ->acpi_ev_install_xrupt_handlers
>>>>      ->acpi_ev_install_sci_handler
>>>>        ->acpi_os_install_interrupt_handler
>>>>          ->acpi_gsi_to_irq
>>>>
>>>>
>>>> the acpi_gsi_to_irq returned -EINVAL because of no mapping found. I
>>>> looked into acpi_gsi_to_irq of x86, acpi_register_gsi is called in it
>>>> so that new mapping for gsi is created if no mapping is found.
>>>
>>> So it looks like we have a discrepancy between the x86 and ARM on that
>>> front.
>>>
>>> Lorenzo, Hanjun, can you please have a look at this and shed some
>>> light on what the expected behaviour is? It looks like we never
>>> encountered an issue with this on arm64, which tends to indicate that
>>> we don't usually use the above path.
>>
>> Sorry for the late reply, I just noticed this tomorrow.

What? Tomorrow? more coffee is needed... it's yesterday...

>>
>> As you said, we never encountered Jianmin's issue on ARM64 hardware,
>> for the call stack which Jianmin shows, acpi_ev_install_xrupt_handlers()
>> is only called for non-reduced ACPI hardware, but ARM64 is always
>> defined as reduced ACPI hardware in the ACPI spec, from the first
>> supported version of ACPI spec for ARM.
>>
>> Jianmin, is the LoongArch using the redunced hardware mode in ACPI?
>> if it's using SCI interrupt, I think not, correct me if I'm wrong.
>>
> 
> Thanks for your reply, Hanjun, LoongArch uses non-reduced ACPI hardware,
> so SCI interrupt is used, which is different from ARM using reduced 
> hardware.

OK, so for ARM64, it will not call acpi_gsi_to_irq() before the
irqdomain and mapping created.

Thanks
Hanjun