linux-kernel - Re: Re: [PATCHv4] PCI/ACPI: _DSM PRESERVE_BOOT_CONFIG function rev id doesn't match with spec

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] [day] [month] [year] [list]

Message-Id: <20250301032822.17486-1-zhoushengqing@ttyinfo.com>
Date: Sat,  1 Mar 2025 03:28:22 +0000
From: Zhou Shengqing <zhoushengqing@...info.com>
To: helgaas@...nel.org
Cc: bhelgaas@...gle.com,
	lenb@...nel.org,
	linux-acpi@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	linux-pci@...r.kernel.org,
	rafael@...nel.org,
	zhoushengqing@...info.com
Subject: Re: Re: [PATCHv4] PCI/ACPI: _DSM PRESERVE_BOOT_CONFIG function rev id doesn't match with spec

On Thu, Feb 27, 2025 at 6:40 PM Bjorn Helgaas wrote:
> On Mon, Dec 16, 2024 at 05:27:51AM +0000, Zhou Shengqing 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.
> > But the code remains unchanged, still 1.
> > 
> > v4:Initialize *obj to NULL.
> > v3:try revision id 1 first, then try revision id 2.
> > v2:add Fixes tag.
> 
> Thanks for working on this issue.
> 
>   - Thanks for the revision history.  For future posts, put it below
>     the "---" line so it's in the email but not part of the commit log.
> 
>   - I think there's a leak in pci_acpi_preserve_config() because it
>     doesn't free "obj" before it returns true.  If you agree, add a
>     preparatory patch to fix this.
> 
>   - Add a preparatory patch to return false early in
>     pci_acpi_preserve_config() if !ACPI_HANDLE(&host_bridge->dev) so
>     the body of the function is unindented, similar to what
>     acpi_pci_add_bus() does.
> 
>   - Add another preparatory patch that adds acpi_check_dsm() of the
>     desired function/rev ID for DSM_PCI_PRESERVE_BOOT_CONFIG,
>     DSM_PCI_POWER_ON_RESET_DELAY, DSM_PCI_DEVICE_READINESS_DURATIONS.
>     Move the "Evaluate PCI Boot Configuration" comment above the
>     acpi_check_dsm() since it applies to the whole function, not just
>     the rev 1 code in this patch.
> 
>   - Rework this patch so it only adds acpi_check_dsm() and
>     acpi_evaluate_dsm_typed() for rev 2.

Could you please explain this in more detail? Do you mean we don't need to
consider rev 1 anymore?

> 
>   - Throughout, wrap code and comments to fit in 80 columns because
>     that's the convention for the rest of the file.
> 
>   - You can use "BIT(DSM_PCI_PRESERVE_BOOT_CONFIG)" and similar
>     instead of "1ULL << DSM_PCI_PRESERVE_BOOT_CONFIG" when calling
>     acpi_check_dsm().
> 
>   - Add a [0/n] cover letter when posting the series.  Each patch
>     should be a response to the cover letter.  "git format-patch" and
>     "git send-email" will do that for you automatically.
> 
> > Fixes: 9d7d5db8e78e ("PCI: Move PRESERVE_BOOT_CONFIG _DSM evaluation to pci_register_host_bridge()")
> > 
> > Signed-off-by: Zhou Shengqing <zhoushengqing@...info.com>
> > ---
> >  drivers/pci/pci-acpi.c | 42 ++++++++++++++++++++++++++++++++----------
> >  1 file changed, 32 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> > index af370628e583..f805cd134019 100644
> > --- a/drivers/pci/pci-acpi.c
> > +++ b/drivers/pci/pci-acpi.c
> > @@ -123,19 +123,41 @@ 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;
> > +		union acpi_object *obj = NULL;
> >  
> >  		/*
> > -		 * 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 try revision 1
> > +		 * first for old platform, then try revision 2.
> >  		 */
> > -		obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(&host_bridge->dev),
> > -					      &pci_acpi_dsm_guid,
> > -					      1, DSM_PCI_PRESERVE_BOOT_CONFIG,
> > -					      NULL, ACPI_TYPE_INTEGER);
> > -		if (obj && obj->integer.value == 0)
> > -			return true;
> > +		if (acpi_check_dsm(ACPI_HANDLE(&host_bridge->dev), &pci_acpi_dsm_guid, 1,
> > +				   1ULL << DSM_PCI_PRESERVE_BOOT_CONFIG)) {
> > +			/*
> > +			 * 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.
> > +			 */
> > +			obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(&host_bridge->dev),
> > +						      &pci_acpi_dsm_guid,
> > +						      1, DSM_PCI_PRESERVE_BOOT_CONFIG,
> > +						      NULL, ACPI_TYPE_INTEGER);
> > +			if (obj && obj->integer.value == 0)
> > +				return true;
> > +		}
> > +
> > +		if (acpi_check_dsm(ACPI_HANDLE(&host_bridge->dev), &pci_acpi_dsm_guid, 2,
> > +				   1ULL << 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)
> > +				return true;
> > +		}
> > +
> >  		ACPI_FREE(obj);
> >  	}
> >  
> > -- 
> > 2.39.2
> >