linux-kernel - Re: [PATCH v5 1/2] Add rev 2 check for PRESERVE_BOOT

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] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAJZ5v0gd5-QfbLhneYKJxER6XC1aDE8KxvX5j695=FdZWHc-kQ@mail.gmail.com>
Date: Thu, 27 Mar 2025 20:05:14 +0100
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Zhou Shengqing <zhoushengqing@...info.com>
Cc: Bjorn Helgaas <bhelgaas@...gle.com>, lenb@...nel.org, linux-acpi@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org
Subject: Re: [PATCH v5 1/2] Add rev 2 check for PRESERVE_BOOT_CONFIG function

On Thu, Mar 27, 2025 at 7:57 PM Rafael J. Wysocki <rafael@...nel.org> wrote:
>
> On Thu, Mar 13, 2025 at 11:07 AM Zhou Shengqing
> <zhoushengqing@...info.com> wrote:
> >
> > Per PCI Firmware Specification Revision 3.3 Table 4-7 _DSM Definitions
> > for PCI. Preserve PCI Boot Configuration Initial Revision ID changed to 2.
> >
> > And add acpi_check_dsm() for DSM_PCI_PRESERVE_BOOT_CONFIG.
> >
> > Signed-off-by: Zhou Shengqing <zhoushengqing@...info.com>
> > ---
> > v5:follow Bjorn advice, add acpi_check_dsm for PCI _DSM.
> > v4:Initialize *obj to NULL.
> > v3:try revision id 1 first, then try revision id 2.
> > v2:add Fixes tag.
> >
> > Fixes: 9d7d5db8e78e ("PCI: Move PRESERVE_BOOT_CONFIG _DSM evaluation to pci_re")
> > ---
> >  drivers/pci/pci-acpi.c | 43 ++++++++++++++++++++++++++++++------------
> >  1 file changed, 31 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> > index af370628e583..4f9e0548c96d 100644
> > --- a/drivers/pci/pci-acpi.c
> > +++ b/drivers/pci/pci-acpi.c
> > @@ -122,24 +122,43 @@ phys_addr_t acpi_pci_root_get_mcfg_addr(acpi_handle handle)
> >
> >  bool pci_acpi_preserve_config(struct pci_host_bridge *host_bridge)
> >  {
> > -       if (ACPI_HANDLE(&host_bridge->dev)) {
> > -               union acpi_object *obj;
> > +       bool rc = false;
> > +       union acpi_object *obj;
>
> + u64 rev;
>
> >
> > -               /*
> > -                * Evaluate the "PCI Boot Configuration" _DSM Function.  If it
> > -                * exists and returns 0, we must preserve any PCI resource
> > -                * assignments made by firmware for this host bridge.
> > -                */
> > +       if (!ACPI_HANDLE(&host_bridge->dev))
> > +               return false;
> > +
> > +       /*
> > +        * Evaluate the "PCI Boot Configuration" _DSM Function.  If it
> > +        * exists and returns 0, we must preserve any PCI resource
> > +        * assignments made by firmware for this host bridge.
> > +        *
> > +        * Per PCI Firmware r3.2, released Jan 26, 2015,
> > +        * DSM_PCI_PRESERVE_BOOT_CONFIG Revision ID is 1. But PCI Firmware r3.3,
> > +        * released Jan 20, 2021, changed sec 4.6.5 to say
> > +        * "lowest valid Revision ID value: 2". So check rev 1 first, then rev 2.
> > +        */
>
> +         for (rev = 1; rev <= 2; rev++) {
> +                 if (!acpi_check_dsm(ACPI_HANDLE(&host_bridge->dev),
> &pci_acpi_dsm_guid,
> +                                   rev, BIT(DSM_PCI_PRESERVE_BOOT_CONFIG)))
> +                         continue;
> +
> +                obj =
> acpi_evaluate_dsm_typed(ACPI_HANDLE(&host_bridge->dev), ,
> &pci_acpi_dsm_guid,
> +                                  rev, BIT(DSM_PCI_PRESERVE_BOOT_CONFIG));

Of course, the above acpi_evaluate_dsm_typed() call has been
messed-up, it should be

obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(&host_bridge->dev),
                                  &pci_acpi_dsm_guid, rev,
                                  DSM_PCI_PRESERVE_BOOT_CONFIG,
                                 NULL, ACPI_TYPE_INTEGER);

but the idea should be clear nevertheless.

> +                if (obj && obj->integer.value == 0)
> +                        rc = true;
> +
> +                ACPI_FREE(obj);
> +
> +                if (rc)
> +                       return true;
> +         }
> +
> +         return false;
>
> would achieve the same with fewer lines of code and less code
> duplication if I'm not mistaken.
>
> > +       if (acpi_check_dsm(ACPI_HANDLE(&host_bridge->dev),
> > +                               &pci_acpi_dsm_guid, 1, BIT(DSM_PCI_PRESERVE_BOOT_CONFIG))) {
> >                 obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(&host_bridge->dev),
> > -                                             &pci_acpi_dsm_guid,
> > -                                             1, DSM_PCI_PRESERVE_BOOT_CONFIG,
> > -                                             NULL, ACPI_TYPE_INTEGER);
> > +                                               &pci_acpi_dsm_guid,
> > +                                               1, DSM_PCI_PRESERVE_BOOT_CONFIG,
> > +                                               NULL, ACPI_TYPE_INTEGER);
> >                 if (obj && obj->integer.value == 0)
> > -                       return true;
> > +                       rc = true;
> > +               ACPI_FREE(obj);
> > +       } else if (acpi_check_dsm(ACPI_HANDLE(&host_bridge->dev),
> > +                                       &pci_acpi_dsm_guid, 2, BIT(DSM_PCI_PRESERVE_BOOT_CONFIG))) {
> > +               obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(&host_bridge->dev),
> > +                                               &pci_acpi_dsm_guid,
> > +                                               2, DSM_PCI_PRESERVE_BOOT_CONFIG,
> > +                                               NULL, ACPI_TYPE_INTEGER);
> > +               if (obj && obj->integer.value == 0)
> > +                       rc = true;
> >                 ACPI_FREE(obj);
> >         }
> >
> > -       return false;
> > +       return rc;
> >  }
> >
> >  /* _HPX PCI Setting Record (Type 0); same as _HPP */
> > --