linux-kernel - Re: [PATCH] PCI/APCI: Move acpi_pci_osc_support() check to negotiation phase

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: <20210604170938.GA2218177@bjorn-Precision-5520>
Date:   Fri, 4 Jun 2021 12:09:38 -0500
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Joerg Roedel <joro@...tes.org>
Cc:     Bjorn Helgaas <bhelgaas@...gle.com>, rjw@...ysocki.net,
        Len Brown <lenb@...nel.org>, linux-pci@...r.kernel.org,
        linux-acpi@...r.kernel.org, linux-kernel@...r.kernel.org,
        jroedel@...e.de
Subject: Re: [PATCH] PCI/APCI: Move acpi_pci_osc_support() check to
 negotiation phase

On Thu, Jun 03, 2021 at 03:50:47PM -0500, Bjorn Helgaas wrote:
> On Thu, Jun 03, 2021 at 02:48:14PM +0200, Joerg Roedel wrote:
> > From: Joerg Roedel <jroedel@...e.de>
> > ...

> > -static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 req)
> > +static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32
> > +					    *mask, u32 req, u32 support)
> >  {
> >  	struct acpi_pci_root *root;
> >  	acpi_status status;
> > @@ -370,7 +361,7 @@ static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 r
> >  
> >  	/* Need to check the available controls bits before requesting them. */
> >  	while (*mask) {
> > -		status = acpi_pci_query_osc(root, root->osc_support_set, mask);
> > +		status = acpi_pci_query_osc(root, support, mask);
> >  		if (ACPI_FAILURE(status))
> >  			return status;
> >  		if (ctrl == *mask)
> > @@ -433,18 +424,6 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
> >  		support |= OSC_PCI_EDR_SUPPORT;
> >  
> >  	decode_osc_support(root, "OS supports", support);
> > -	status = acpi_pci_osc_support(root, support);
> > -	if (ACPI_FAILURE(status)) {
> > -		*no_aspm = 1;
> > -
> > -		/* _OSC is optional for PCI host bridges */
> > -		if ((status == AE_NOT_FOUND) && !is_pcie)
> > -			return;
> > -
> > -		dev_info(&device->dev, "_OSC: platform retains control of PCIe features (%s)\n",
> > -			 acpi_format_exception(status));
> > -		return;
> > -	}
> >  
> >  	if (pcie_ports_disabled) {
> >  		dev_info(&device->dev, "PCIe port services disabled; not requesting _OSC control\n");
> 
> Also not related to this patch, but it seems pointless to compute and
> decode "support" above when we're not going to use _OSC at all.  I
> think the "pcie_ports_disabled" test should be the very first thing in
> this function (I'm assuming the "pcie_ports=compat" command line
> argument *should* apply even on x86_apple_machine, which it doesn't
> today).

I think I was wrong about this.  Even when "pcie_ports_disabled", the
current code *does* evaluate "_OSC(Query, SUPPORT=x, CONTROL=0)",
i.e., it tells the platform what Linux supports, but doesn't request
control of anything.

I think the platform may rely on this knowledge of what the OS
supports.  For example, if we tell the platform that Linux has
OSC_PCI_EXT_CONFIG_SUPPORT, that may change the behavior of ASL that
accesses config space.

So I don't think it's safe to move this test to the beginning of the
function as I proposed.

For the same reason, I'm not sure that it's safe to remove
acpi_pci_osc_support() as in this patch.  If either
"pcie_ports_disabled" or Linux doesn't support everything in
ACPI_PCIE_REQ_SUPPORT, we will never evaluate _OSC at all, so
the platform won't know that Linux has OSC_PCI_SEGMENT_GROUPS_SUPPORT,
OSC_PCI_HPX_TYPE_3_SUPPORT, OSC_PCI_EXT_CONFIG_SUPPORT, etc.

Bjorn