[PATCH] ACPI: bus: Move acpi_arm_init() to the place of after acpi_ghes_init()

[PATCH] ACPI: bus: Move acpi_arm_init() to the place of after acpi_ghes_init()

[Hanjun Guo]

acpi_agdi_init() in acpi_arm_init() will register a SDEI event, so
it needs the SDEI subsystem to be initialized (which is done in
acpi_ghes_init()) before the AGDI driver probing.

In commit fcea0ccf4fd7 ("ACPI: bus: Consolidate all arm specific
initialisation into acpi_arm_init()"), the acpi_agdi_init() was
called before acpi_ghes_init() and it causes following failure:

| [    0.515864] sdei: Failed to create event 1073741825: -5
| [    0.515866] agdi agdi.0: Failed to register for SDEI event 1073741825
| [    0.515867] agdi: probe of agdi.0 failed with error -5
| ...
| [    0.516022] sdei: SDEIv1.0 (0x0) detected in firmware.

Fix it by moving acpi_arm_init() to the place of after
acpi_ghes_init().

Fixes: fcea0ccf4fd7 ("ACPI: bus: Consolidate all arm specific initialisation into acpi_arm_init()")
Reported-by: D Scott Phillips <scott@os.amperecomputing.com>
Signed-off-by: Hanjun Guo <guohanjun@huawei.com>
---

I did a test on a ARM server and I didn't see regressions, but
I don't have a AGDI table firmware, so Scott please give a 
test to see if it fixes your issue.

 drivers/acpi/bus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index f41dda2d3493..a4aa53b7e2bb 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -1410,10 +1410,10 @@ static int __init acpi_init(void)
 	acpi_init_ffh();
 
 	pci_mmcfg_late_init();
-	acpi_arm_init();
 	acpi_viot_early_init();
 	acpi_hest_init();
 	acpi_ghes_init();
+	acpi_arm_init();
 	acpi_scan_init();
 	acpi_ec_init();
 	acpi_debugfs_init();
-- 
2.25.1

Re: [PATCH] ACPI: bus: Move acpi_arm_init() to the place of after acpi_ghes_init()

[Sudeep Holla]

On Tue, Oct 10, 2023 at 04:21:23PM +0800, Hanjun Guo wrote:
> acpi_agdi_init() in acpi_arm_init() will register a SDEI event, so
> it needs the SDEI subsystem to be initialized (which is done in
> acpi_ghes_init()) before the AGDI driver probing.
> 
> In commit fcea0ccf4fd7 ("ACPI: bus: Consolidate all arm specific
> initialisation into acpi_arm_init()"), the acpi_agdi_init() was
> called before acpi_ghes_init() and it causes following failure:
> 
> | [    0.515864] sdei: Failed to create event 1073741825: -5
> | [    0.515866] agdi agdi.0: Failed to register for SDEI event 1073741825
> | [    0.515867] agdi: probe of agdi.0 failed with error -5
> | ...
> | [    0.516022] sdei: SDEIv1.0 (0x0) detected in firmware.
> 
> Fix it by moving acpi_arm_init() to the place of after
> acpi_ghes_init().
> 
> Fixes: fcea0ccf4fd7 ("ACPI: bus: Consolidate all arm specific initialisation into acpi_arm_init()")
> Reported-by: D Scott Phillips <scott@os.amperecomputing.com>
> Signed-off-by: Hanjun Guo <guohanjun@huawei.com>
> ---
> 
> I did a test on a ARM server and I didn't see regressions, but
> I don't have a AGDI table firmware, so Scott please give a 
> test to see if it fixes your issue.
> 
>  drivers/acpi/bus.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index f41dda2d3493..a4aa53b7e2bb 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -1410,10 +1410,10 @@ static int __init acpi_init(void)
>  	acpi_init_ffh();
>
>  	pci_mmcfg_late_init();
> -	acpi_arm_init();
>  	acpi_viot_early_init();
>  	acpi_hest_init();
>  	acpi_ghes_init();
> +	acpi_arm_init();

I am fine with the change, but just wanted to check with Robin/Jean-Philippe
if there are any dependency on IORT initialisation for VIOT ? IIUC IORT was
always initialised before VIOT but that changes after this change.

If it turns how to have dependency, then does moving GHES and HEST
initialisation before acpi_arm_init() work ?

--
Regards,
Sudeep

Re: [PATCH] ACPI: bus: Move acpi_arm_init() to the place of after acpi_ghes_init()

[Robin Murphy]

On 2023-10-10 09:50, Sudeep Holla wrote:
> On Tue, Oct 10, 2023 at 04:21:23PM +0800, Hanjun Guo wrote:
>> acpi_agdi_init() in acpi_arm_init() will register a SDEI event, so
>> it needs the SDEI subsystem to be initialized (which is done in
>> acpi_ghes_init()) before the AGDI driver probing.
>>
>> In commit fcea0ccf4fd7 ("ACPI: bus: Consolidate all arm specific
>> initialisation into acpi_arm_init()"), the acpi_agdi_init() was
>> called before acpi_ghes_init() and it causes following failure:
>>
>> | [    0.515864] sdei: Failed to create event 1073741825: -5
>> | [    0.515866] agdi agdi.0: Failed to register for SDEI event 1073741825
>> | [    0.515867] agdi: probe of agdi.0 failed with error -5
>> | ...
>> | [    0.516022] sdei: SDEIv1.0 (0x0) detected in firmware.
>>
>> Fix it by moving acpi_arm_init() to the place of after
>> acpi_ghes_init().
>>
>> Fixes: fcea0ccf4fd7 ("ACPI: bus: Consolidate all arm specific initialisation into acpi_arm_init()")
>> Reported-by: D Scott Phillips <scott@os.amperecomputing.com>
>> Signed-off-by: Hanjun Guo <guohanjun@huawei.com>
>> ---
>>
>> I did a test on a ARM server and I didn't see regressions, but
>> I don't have a AGDI table firmware, so Scott please give a
>> test to see if it fixes your issue.
>>
>>   drivers/acpi/bus.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
>> index f41dda2d3493..a4aa53b7e2bb 100644
>> --- a/drivers/acpi/bus.c
>> +++ b/drivers/acpi/bus.c
>> @@ -1410,10 +1410,10 @@ static int __init acpi_init(void)
>>   	acpi_init_ffh();
>>
>>   	pci_mmcfg_late_init();
>> -	acpi_arm_init();
>>   	acpi_viot_early_init();
>>   	acpi_hest_init();
>>   	acpi_ghes_init();
>> +	acpi_arm_init();
> 
> I am fine with the change, but just wanted to check with Robin/Jean-Philippe
> if there are any dependency on IORT initialisation for VIOT ? IIUC IORT was
> always initialised before VIOT but that changes after this change.

They should be independent, and typically we'd only expect to see one or 
the other anyway (although strictly a VMM *could* provide virtio-iommu 
for some devices while also emulating an SMMU for others if it really 
really wanted to).

Cheers,
Robin.

Re: [PATCH] ACPI: bus: Move acpi_arm_init() to the place of after acpi_ghes_init()

[Sudeep Holla]

On Tue, Oct 10, 2023 at 10:30:56AM +0100, Robin Murphy wrote:
> On 2023-10-10 09:50, Sudeep Holla wrote:
> > On Tue, Oct 10, 2023 at 04:21:23PM +0800, Hanjun Guo wrote:
> > > acpi_agdi_init() in acpi_arm_init() will register a SDEI event, so
> > > it needs the SDEI subsystem to be initialized (which is done in
> > > acpi_ghes_init()) before the AGDI driver probing.
> > > 
> > > In commit fcea0ccf4fd7 ("ACPI: bus: Consolidate all arm specific
> > > initialisation into acpi_arm_init()"), the acpi_agdi_init() was
> > > called before acpi_ghes_init() and it causes following failure:
> > > 
> > > | [    0.515864] sdei: Failed to create event 1073741825: -5
> > > | [    0.515866] agdi agdi.0: Failed to register for SDEI event 1073741825
> > > | [    0.515867] agdi: probe of agdi.0 failed with error -5
> > > | ...
> > > | [    0.516022] sdei: SDEIv1.0 (0x0) detected in firmware.
> > > 
> > > Fix it by moving acpi_arm_init() to the place of after
> > > acpi_ghes_init().
> > > 
> > > Fixes: fcea0ccf4fd7 ("ACPI: bus: Consolidate all arm specific initialisation into acpi_arm_init()")
> > > Reported-by: D Scott Phillips <scott@os.amperecomputing.com>
> > > Signed-off-by: Hanjun Guo <guohanjun@huawei.com>
> > > ---
> > > 
> > > I did a test on a ARM server and I didn't see regressions, but
> > > I don't have a AGDI table firmware, so Scott please give a
> > > test to see if it fixes your issue.
> > > 
> > >   drivers/acpi/bus.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> > > index f41dda2d3493..a4aa53b7e2bb 100644
> > > --- a/drivers/acpi/bus.c
> > > +++ b/drivers/acpi/bus.c
> > > @@ -1410,10 +1410,10 @@ static int __init acpi_init(void)
> > >   	acpi_init_ffh();
> > > 
> > >   	pci_mmcfg_late_init();
> > > -	acpi_arm_init();
> > >   	acpi_viot_early_init();
> > >   	acpi_hest_init();
> > >   	acpi_ghes_init();
> > > +	acpi_arm_init();
> > 
> > I am fine with the change, but just wanted to check with Robin/Jean-Philippe
> > if there are any dependency on IORT initialisation for VIOT ? IIUC IORT was
> > always initialised before VIOT but that changes after this change.
> 
> They should be independent, and typically we'd only expect to see one or the
> other anyway (although strictly a VMM *could* provide virtio-iommu for some
> devices while also emulating an SMMU for others if it really really wanted
> to).
>

Thanks Robin for the explanation, that helped me to understand the usage
better.

Hanjun,

In that case,
Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>

Rafael,

Can you please pick this up as fix for v6.6 if possible ?

--
Regards,
Sudeep

Re: [PATCH] ACPI: bus: Move acpi_arm_init() to the place of after acpi_ghes_init()

[Jean-Philippe Brucker]

On Tue, Oct 10, 2023 at 10:30:56AM +0100, Robin Murphy wrote:
> On 2023-10-10 09:50, Sudeep Holla wrote:
> > On Tue, Oct 10, 2023 at 04:21:23PM +0800, Hanjun Guo wrote:
> > > acpi_agdi_init() in acpi_arm_init() will register a SDEI event, so
> > > it needs the SDEI subsystem to be initialized (which is done in
> > > acpi_ghes_init()) before the AGDI driver probing.
> > > 
> > > In commit fcea0ccf4fd7 ("ACPI: bus: Consolidate all arm specific
> > > initialisation into acpi_arm_init()"), the acpi_agdi_init() was
> > > called before acpi_ghes_init() and it causes following failure:
> > > 
> > > | [    0.515864] sdei: Failed to create event 1073741825: -5
> > > | [    0.515866] agdi agdi.0: Failed to register for SDEI event 1073741825
> > > | [    0.515867] agdi: probe of agdi.0 failed with error -5
> > > | ...
> > > | [    0.516022] sdei: SDEIv1.0 (0x0) detected in firmware.
> > > 
> > > Fix it by moving acpi_arm_init() to the place of after
> > > acpi_ghes_init().
> > > 
> > > Fixes: fcea0ccf4fd7 ("ACPI: bus: Consolidate all arm specific initialisation into acpi_arm_init()")
> > > Reported-by: D Scott Phillips <scott@os.amperecomputing.com>
> > > Signed-off-by: Hanjun Guo <guohanjun@huawei.com>
> > > ---
> > > 
> > > I did a test on a ARM server and I didn't see regressions, but
> > > I don't have a AGDI table firmware, so Scott please give a
> > > test to see if it fixes your issue.
> > > 
> > >   drivers/acpi/bus.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> > > index f41dda2d3493..a4aa53b7e2bb 100644
> > > --- a/drivers/acpi/bus.c
> > > +++ b/drivers/acpi/bus.c
> > > @@ -1410,10 +1410,10 @@ static int __init acpi_init(void)
> > >   	acpi_init_ffh();
> > > 
> > >   	pci_mmcfg_late_init();
> > > -	acpi_arm_init();
> > >   	acpi_viot_early_init();
> > >   	acpi_hest_init();
> > >   	acpi_ghes_init();
> > > +	acpi_arm_init();
> > 
> > I am fine with the change, but just wanted to check with Robin/Jean-Philippe
> > if there are any dependency on IORT initialisation for VIOT ? IIUC IORT was
> > always initialised before VIOT but that changes after this change.
> 
> They should be independent, and typically we'd only expect to see one or the
> other anyway (although strictly a VMM *could* provide virtio-iommu for some
> devices while also emulating an SMMU for others if it really really wanted
> to).

IORT also describes the GIC ITS topology, so QEMU (and likely
cloud-hypervisor) do present both tables to a guest. But I don't think the
order matters in any case, and QEMU still boots fine with this patch.

Thanks,
Jean

Re: [PATCH] ACPI: bus: Move acpi_arm_init() to the place of after acpi_ghes_init()

[D Scott Phillips]

Hanjun Guo <guohanjun@huawei.com> writes:

> acpi_agdi_init() in acpi_arm_init() will register a SDEI event, so
> it needs the SDEI subsystem to be initialized (which is done in
> acpi_ghes_init()) before the AGDI driver probing.
>
> In commit fcea0ccf4fd7 ("ACPI: bus: Consolidate all arm specific
> initialisation into acpi_arm_init()"), the acpi_agdi_init() was
> called before acpi_ghes_init() and it causes following failure:
>
> | [    0.515864] sdei: Failed to create event 1073741825: -5
> | [    0.515866] agdi agdi.0: Failed to register for SDEI event 1073741825
> | [    0.515867] agdi: probe of agdi.0 failed with error -5
> | ...
> | [    0.516022] sdei: SDEIv1.0 (0x0) detected in firmware.
>
> Fix it by moving acpi_arm_init() to the place of after
> acpi_ghes_init().
>
> Fixes: fcea0ccf4fd7 ("ACPI: bus: Consolidate all arm specific initialisation into acpi_arm_init()")
> Reported-by: D Scott Phillips <scott@os.amperecomputing.com>
> Signed-off-by: Hanjun Guo <guohanjun@huawei.com>

Tested-by: D Scott Phillips <scott@os.amperecomputing.com>

Thanks Hanjun and Sudeep

Re: [PATCH] ACPI: bus: Move acpi_arm_init() to the place of after acpi_ghes_init()

[Rafael J. Wysocki]

On Tue, Oct 10, 2023 at 8:11 PM D Scott Phillips
<scott@os.amperecomputing.com> wrote:
>
> Hanjun Guo <guohanjun@huawei.com> writes:
>
> > acpi_agdi_init() in acpi_arm_init() will register a SDEI event, so
> > it needs the SDEI subsystem to be initialized (which is done in
> > acpi_ghes_init()) before the AGDI driver probing.
> >
> > In commit fcea0ccf4fd7 ("ACPI: bus: Consolidate all arm specific
> > initialisation into acpi_arm_init()"), the acpi_agdi_init() was
> > called before acpi_ghes_init() and it causes following failure:
> >
> > | [    0.515864] sdei: Failed to create event 1073741825: -5
> > | [    0.515866] agdi agdi.0: Failed to register for SDEI event 1073741825
> > | [    0.515867] agdi: probe of agdi.0 failed with error -5
> > | ...
> > | [    0.516022] sdei: SDEIv1.0 (0x0) detected in firmware.
> >
> > Fix it by moving acpi_arm_init() to the place of after
> > acpi_ghes_init().
> >
> > Fixes: fcea0ccf4fd7 ("ACPI: bus: Consolidate all arm specific initialisation into acpi_arm_init()")
> > Reported-by: D Scott Phillips <scott@os.amperecomputing.com>
> > Signed-off-by: Hanjun Guo <guohanjun@huawei.com>
>
> Tested-by: D Scott Phillips <scott@os.amperecomputing.com>
>
> Thanks Hanjun and Sudeep

Applied as 6.6-rc material, thanks!