[PATCH] acpi: Support CONFIG_ACPI without CONFIG_PCI

[PATCH] acpi: Support CONFIG_ACPI without CONFIG_PCI

[Suraj Jitindar Singh]

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(+)

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

Re: [PATCH] acpi: Support CONFIG_ACPI without CONFIG_PCI

[Rafael J. Wysocki]

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
>
>

Re: [PATCH] acpi: Support CONFIG_ACPI without CONFIG_PCI

[Jitindar Singh, Suraj]

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
> > 
> > 

Re: [PATCH] acpi: Support CONFIG_ACPI without CONFIG_PCI

[Rafael J. Wysocki]

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?