| Message ID | 20231010082123.4167603-1-guohanjun@huawei.com (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
| Series | ACPI: bus: Move acpi_arm_init() to the place of after acpi_ghes_init() | expand |
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
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.
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
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
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
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!
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();
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(-)