| Message ID | 20240613211011.413120-1-surajjs@amazon.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | acpi: Support CONFIG_ACPI without CONFIG_PCI | expand |
On Thu, Jun 13, 2024 at 11:10 PM Suraj Jitindar Singh <surajjs@amazon.com> wrote: > > Make is possible to use ACPI without having CONFIG_PCI set. > > When initialising ACPI the following call chain occurs: > > acpi_init() -> > acpi_bus_init() -> > acpi_load_tables() -> > acpi_ev_install_region_handlers() -> > > acpi_ev_install_region_handlers() calls acpi_ev_install_space_handler() on > each of the default address spaces defined as: > > u8 acpi_gbl_default_address_spaces[ACPI_NUM_DEFAULT_SPACES] = { > ACPI_ADR_SPACE_SYSTEM_MEMORY, > ACPI_ADR_SPACE_SYSTEM_IO, > ACPI_ADR_SPACE_PCI_CONFIG, > ACPI_ADR_SPACE_DATA_TABLE > }; > > However in acpi_ev_install_space_handler() the case statement for > ACPI_ADR_SPACE_PCI_CONFIG is ifdef'd as: > > #ifdef ACPI_PCI_CONFIGURED > case ACPI_ADR_SPACE_PCI_CONFIG: > > handler = acpi_ex_pci_config_space_handler; > setup = acpi_ev_pci_config_region_setup; > break; > #endif > > ACPI_PCI_CONFIGURED is not defined if CONFIG_PCI is not enabled, thus the > attempt to install the handler fails. > > Fix this by ifdef'ing ACPI_ADR_SPACE_PCI_CONFIG in the list of default > address spaces. What if there are PCI operation regions in the AML on the platform? How are they going to be handled? > Fixes: bd23fac3eaaa ("ACPICA: Remove PCI bits from ACPICA when CONFIG_PCI is unset") > CC: stable@vger.kernel.org # 5.0.x- > Signed-off-by: Suraj Jitindar Singh <surajjs@amazon.com> > --- > drivers/acpi/acpica/evhandler.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/acpi/acpica/evhandler.c b/drivers/acpi/acpica/evhandler.c > index 1c8cb6d924df..371093acb362 100644 > --- a/drivers/acpi/acpica/evhandler.c > +++ b/drivers/acpi/acpica/evhandler.c > @@ -26,7 +26,9 @@ acpi_ev_install_handler(acpi_handle obj_handle, > u8 acpi_gbl_default_address_spaces[ACPI_NUM_DEFAULT_SPACES] = { > ACPI_ADR_SPACE_SYSTEM_MEMORY, > ACPI_ADR_SPACE_SYSTEM_IO, > +#ifdef ACPI_PCI_CONFIGURED > ACPI_ADR_SPACE_PCI_CONFIG, > +#endif > ACPI_ADR_SPACE_DATA_TABLE > }; > > -- > 2.34.1 > >
On Fri, 2024-06-14 at 13:08 +0200, Rafael J. Wysocki wrote: > On Thu, Jun 13, 2024 at 11:10 PM Suraj Jitindar Singh > <surajjs@amazon.com> wrote: > > > > Make is possible to use ACPI without having CONFIG_PCI set. > > > > When initialising ACPI the following call chain occurs: > > > > acpi_init() -> > > acpi_bus_init() -> > > acpi_load_tables() -> > > acpi_ev_install_region_handlers() -> > > > > acpi_ev_install_region_handlers() calls > > acpi_ev_install_space_handler() on > > each of the default address spaces defined as: > > > > u8 acpi_gbl_default_address_spaces[ACPI_NUM_DEFAULT_SPACES] = { > > ACPI_ADR_SPACE_SYSTEM_MEMORY, > > ACPI_ADR_SPACE_SYSTEM_IO, > > ACPI_ADR_SPACE_PCI_CONFIG, > > ACPI_ADR_SPACE_DATA_TABLE > > }; > > > > However in acpi_ev_install_space_handler() the case statement for > > ACPI_ADR_SPACE_PCI_CONFIG is ifdef'd as: > > > > #ifdef ACPI_PCI_CONFIGURED > > case ACPI_ADR_SPACE_PCI_CONFIG: > > > > handler = > > acpi_ex_pci_config_space_handler; > > setup = acpi_ev_pci_config_region_setup; > > break; > > #endif > > > > ACPI_PCI_CONFIGURED is not defined if CONFIG_PCI is not enabled, > > thus the > > attempt to install the handler fails. > > > > Fix this by ifdef'ing ACPI_ADR_SPACE_PCI_CONFIG in the list of > > default > > address spaces. > > What if there are PCI operation regions in the AML on the platform? > How are they going to be handled? Hi, Appreciate the response. I think the short answer is that if there are PCI operation regions in the AML on the platform then the kernel will need to be built with PCI support (CONFIG_PCI) and if it isn't then there won't be a handler installed and the operation will error. Correct me if I'm wrong but it seems the intention of the patch series: 36ad7d2b9e9b ACPI: Move PCI reset to a separate function 86689776878f ACPI: Allow CONFIG_PCI to be unset for reboot bd23fac3eaaa ACPICA: Remove PCI bits from ACPICA when CONFIG_PCI is unset 5d32a66541c4 PCI/ACPI: Allow ACPI to be built without CONFIG_PCI set was to decouple the dependency between CONFIG_PCI and CONFIG_ACPI. bd23fac3eaaa ("ACPICA: Remove PCI bits from ACPICA when CONFIG_PCI is unset") added an ifdef around the code to install the handler for the PCI CONFIG region making it dependent on ACPI_PCI_CONFIGURED (and thus CONFIG_PCI). Thus it is not possible to install the default handler for the PCI CONFIG region unless CONFIG_PCI is set meaning it makes no sense to have it in the list of default address spaces. I can gather that this leads to 2 possibilities: 1. If there are PCI operation regions in the AML on the platform then these will error on a kernel not compiled with CONFIG_PCI. or, 2. The code to install the handler for the PCI CONFIG region should not be ifdef'ed and should be executed irrespective of if the kernel is compiled with CONFIG_PCI to allow for PCI CONFIG regions in the AML. Thanks > > > Fixes: bd23fac3eaaa ("ACPICA: Remove PCI bits from ACPICA when > > CONFIG_PCI is unset") > > CC: stable@vger.kernel.org # 5.0.x- > > Signed-off-by: Suraj Jitindar Singh <surajjs@amazon.com> > > --- > > drivers/acpi/acpica/evhandler.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/acpi/acpica/evhandler.c > > b/drivers/acpi/acpica/evhandler.c > > index 1c8cb6d924df..371093acb362 100644 > > --- a/drivers/acpi/acpica/evhandler.c > > +++ b/drivers/acpi/acpica/evhandler.c > > @@ -26,7 +26,9 @@ acpi_ev_install_handler(acpi_handle obj_handle, > > u8 acpi_gbl_default_address_spaces[ACPI_NUM_DEFAULT_SPACES] = { > > ACPI_ADR_SPACE_SYSTEM_MEMORY, > > ACPI_ADR_SPACE_SYSTEM_IO, > > +#ifdef ACPI_PCI_CONFIGURED > > ACPI_ADR_SPACE_PCI_CONFIG, > > +#endif > > ACPI_ADR_SPACE_DATA_TABLE > > }; > > > > -- > > 2.34.1 > > > >
On Wed, Jul 3, 2024 at 1:01 AM Jitindar Singh, Suraj <surajjs@amazon.com> wrote: > > On Fri, 2024-06-14 at 13:08 +0200, Rafael J. Wysocki wrote: > > On Thu, Jun 13, 2024 at 11:10 PM Suraj Jitindar Singh > > <surajjs@amazon.com> wrote: > > > > > > Make is possible to use ACPI without having CONFIG_PCI set. > > > > > > When initialising ACPI the following call chain occurs: > > > > > > acpi_init() -> > > > acpi_bus_init() -> > > > acpi_load_tables() -> > > > acpi_ev_install_region_handlers() -> > > > > > > acpi_ev_install_region_handlers() calls > > > acpi_ev_install_space_handler() on > > > each of the default address spaces defined as: > > > > > > u8 acpi_gbl_default_address_spaces[ACPI_NUM_DEFAULT_SPACES] = { > > > ACPI_ADR_SPACE_SYSTEM_MEMORY, > > > ACPI_ADR_SPACE_SYSTEM_IO, > > > ACPI_ADR_SPACE_PCI_CONFIG, > > > ACPI_ADR_SPACE_DATA_TABLE > > > }; > > > > > > However in acpi_ev_install_space_handler() the case statement for > > > ACPI_ADR_SPACE_PCI_CONFIG is ifdef'd as: > > > > > > #ifdef ACPI_PCI_CONFIGURED > > > case ACPI_ADR_SPACE_PCI_CONFIG: > > > > > > handler = > > > acpi_ex_pci_config_space_handler; > > > setup = acpi_ev_pci_config_region_setup; > > > break; > > > #endif > > > > > > ACPI_PCI_CONFIGURED is not defined if CONFIG_PCI is not enabled, > > > thus the > > > attempt to install the handler fails. > > > > > > Fix this by ifdef'ing ACPI_ADR_SPACE_PCI_CONFIG in the list of > > > default > > > address spaces. > > > > What if there are PCI operation regions in the AML on the platform? > > How are they going to be handled? > > Hi, > > Appreciate the response. > > I think the short answer is that if there are PCI operation regions in > the AML on the platform then the kernel will need to be built with PCI > support (CONFIG_PCI) and if it isn't then there won't be a handler > installed and the operation will error. A problem with this approach is that AML has no good way of handling such errors. It accesses a location in an address space of some sort and expects the access to be successful. The interpreter can catch them, but then the only thing it can do is to abort the AML which then may lead to all sorts of unexpected behavior of the platform. > Correct me if I'm wrong but it seems the intention of the patch series: > > 36ad7d2b9e9b ACPI: Move PCI reset to a separate function > 86689776878f ACPI: Allow CONFIG_PCI to be unset for reboot > bd23fac3eaaa ACPICA: Remove PCI bits from ACPICA when CONFIG_PCI is > unset > 5d32a66541c4 PCI/ACPI: Allow ACPI to be built without CONFIG_PCI set > > was to decouple the dependency between CONFIG_PCI and CONFIG_ACPI. Yes, and as per the above, it was a mistake. > bd23fac3eaaa ("ACPICA: Remove PCI bits from ACPICA when CONFIG_PCI is > unset") added an ifdef around the code to install the handler for the > PCI CONFIG region making it dependent on ACPI_PCI_CONFIGURED (and thus > CONFIG_PCI). Thus it is not possible to install the default handler for > the PCI CONFIG region unless CONFIG_PCI is set meaning it makes no > sense to have it in the list of default address spaces. > > I can gather that this leads to 2 possibilities: > > 1. If there are PCI operation regions in the AML on the platform then > these will error on a kernel not compiled with CONFIG_PCI. But as I said, there is no good way of handling such errors. Basically, the kernel should panic() in those cases. > or, > > 2. The code to install the handler for the PCI CONFIG region should not > be ifdef'ed and should be executed irrespective of if the kernel is > compiled with CONFIG_PCI to allow for PCI CONFIG regions in the AML. That doesn't work either because the PCI config address space may not be really accessible without CONFIG_PCI. IOW, I don't see how this can be made work. Can you please remind me what the use case for ACPI without PCI is?
diff --git a/drivers/acpi/acpica/evhandler.c b/drivers/acpi/acpica/evhandler.c index 1c8cb6d924df..371093acb362 100644 --- a/drivers/acpi/acpica/evhandler.c +++ b/drivers/acpi/acpica/evhandler.c @@ -26,7 +26,9 @@ acpi_ev_install_handler(acpi_handle obj_handle, u8 acpi_gbl_default_address_spaces[ACPI_NUM_DEFAULT_SPACES] = { ACPI_ADR_SPACE_SYSTEM_MEMORY, ACPI_ADR_SPACE_SYSTEM_IO, +#ifdef ACPI_PCI_CONFIGURED ACPI_ADR_SPACE_PCI_CONFIG, +#endif ACPI_ADR_SPACE_DATA_TABLE };
Make is possible to use ACPI without having CONFIG_PCI set. When initialising ACPI the following call chain occurs: acpi_init() -> acpi_bus_init() -> acpi_load_tables() -> acpi_ev_install_region_handlers() -> acpi_ev_install_region_handlers() calls acpi_ev_install_space_handler() on each of the default address spaces defined as: u8 acpi_gbl_default_address_spaces[ACPI_NUM_DEFAULT_SPACES] = { ACPI_ADR_SPACE_SYSTEM_MEMORY, ACPI_ADR_SPACE_SYSTEM_IO, ACPI_ADR_SPACE_PCI_CONFIG, ACPI_ADR_SPACE_DATA_TABLE }; However in acpi_ev_install_space_handler() the case statement for ACPI_ADR_SPACE_PCI_CONFIG is ifdef'd as: #ifdef ACPI_PCI_CONFIGURED case ACPI_ADR_SPACE_PCI_CONFIG: handler = acpi_ex_pci_config_space_handler; setup = acpi_ev_pci_config_region_setup; break; #endif ACPI_PCI_CONFIGURED is not defined if CONFIG_PCI is not enabled, thus the attempt to install the handler fails. Fix this by ifdef'ing ACPI_ADR_SPACE_PCI_CONFIG in the list of default address spaces. Fixes: bd23fac3eaaa ("ACPICA: Remove PCI bits from ACPICA when CONFIG_PCI is unset") CC: stable@vger.kernel.org # 5.0.x- Signed-off-by: Suraj Jitindar Singh <surajjs@amazon.com> --- drivers/acpi/acpica/evhandler.c | 2 ++ 1 file changed, 2 insertions(+)