Re: [RFC PATCH] ACPI: Add support for CXL_OSC

["Sean V Kelley" <sean.v.kelley@linux.intel.com>]

Hi,

Thanks for for your feedback, my comments below:

On 10 Jul 2020, at 14:53, Bjorn Helgaas wrote:

> On Wed, Jul 08, 2020 at 04:57:42PM -0700, Sean V Kelley wrote:
>> Compute eXpress Link host bridges enumerate to the OS with
>> ACPI-described support and control capabilities. An _OSC interface
>> applies to host bridge ACPI devices that originate CXL hierarchies.
>>
>> These ACPI devices have a _HID of (_CID including) 
>> EISAID(“ACPI0016”).
>> For CXL 2.0 and later hierarchies, CXL _OSC is required. CXL _OSC is
>> optional for CXL 1.1 hierarchies. A CXL host bridge also originates a
>> PCIe hierarchy and will have a _CID of EISAID(“PNP0A08”). As 
>> such, a CXL
>> Host Bridge device may expose either CXL _OSC or PCIe _OSC.
>>
>> CXL end point devices connected behind a CXL bridge are standard PCI 
>> or
>> CXL devices. It is the same device with two labels - ACPI0016 or 
>> PNP0A08
>> label. For example, with a given bridge device ‘PC01’ definition, 
>> the OS
>> will be able to find PCI devices connected behind this bridge.
>>
>>                       | CPU Root Bus 0x14 |
>>                       | Downstream Port   |
>>                        -------------------
>>                                |
>>                                | Flex Bus
>>                                |
>>                       |                   |  ACPI Device PC01
>>                       |  Root Bus 0x15    |
>>                        -------------------
>>                           |          |
>>                           |          |
>>                       | RCiEP |   | RCiEP |
>>                       |       |   | or EP |
>>                       | D0 F0 |   | D0 F1 |
>>                        -------     -------
>>
>> As a result, it instantiates another PNP0A08 or ACPI0016 device. 
>> PN0A08
>> is therefore legacy support and the ACPI0016 is for a CXL aware OS. A 
>> CXL
>> aware OS would look for CXL first. If the OS evaluates CXL _OSC, it 
>> shall
>> not evaluate PCIe _OSC.
>
> s/PN0A08/PNP0A08/
>
> But Linux really doesn't use PNP0A08 at all except to avoid a warning
> for conventional PCI host bridges for which we don't have an _OSC.
>
> PNP0A03 is really all Linux uses today.  And presumably your CXL
> bridges would also include PNP0A03 in their _CID.

Yes it’s used as a part of the _CID as well.  According to the spec - 
"ACPI-6_3-ECR-CXL OSC", the Host Bridge created for CXL can have a _CID 
of "PNP0A08”. However, this _CID value of “PNP0A08” by itself will 
result in an OSC method not being evaluated by an OS without any CXL 
awareness. As a result, a legacy OS cannot fully support the PCIe 
features used by CXL devices.  While the BIOS vendor can use the _CID of 
“PNP0A03” it’s best to fix the spec.  So that is a request 
underway (modification of Spec "ACPI-6_3-ECR-CXL OSC.docx" (and 
consequently BIOS) to ensure primacy of _CID "PNP0A03" with regards to 
"PNP0A08”)


>
>> This patch implements support for CXL _OSC for CXL 1.1 hiearchies.  
>> This
>> is intended as an RFC to open discussion on the feature and its
>> implementation.
>>
>> Some caveats and questions to consider:
>>
>> 1) pci_root.c goes through many levels of calls that may need some
>> changes. These patches attempt to live-with the current state. Should
>> the first step perhaps be more refactoring to make room so to speak
>> for the addition of CXL?
>
> negotiate_os_control() is incredibly ugly.  I'd love it if it could be
> simplified somehow, but I don't have any good suggestions.  I've tried
> and failed several times.

I’ll have a further look to perhaps suggest some changes.

>
>> 2) I avoided rename where possible, and so where you see say 
>> OSC_SUPPORT
>> that implies PCI.
>>
>> 3) I attempted to avoid increasing the complexity of an existing 
>> function
>> by creating a new CXL specific one.  In some cases CXL and PCI 
>> handling
>> coexist in the same function.
>>
>> As with my past CXL Enumeration RFC there are no fw images or device
>> drivers yet and so this is not ready for consideration for merge.
>> However, I would like to get discussion started on the ACPI changes
>> needed especially if it points to some prior refactoring needs.
>>
>> DocLink: https://www.computeexpresslink.org/
>> DocLink: https://uefi.org/
>
> More specific references would be useful.  Specific document,
> revision, and section number.

Understood, will do.

>
> Of course, this will all have to be packaged up with users before it
> makes sense to merge it.  I know that's obvious :)

Agree!

>
>> Signed-off-by: Sean V Kelley <sean.v.kelley@linux.intel.com>
>> ---
>>  drivers/acpi/pci_root.c | 374 
>> ++++++++++++++++++++++++++++------------
>>  include/acpi/acpi_bus.h |   6 +-
>>  include/linux/acpi.h    |  18 ++
>>  3 files changed, 288 insertions(+), 110 deletions(-)
>>
>> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
>> index f90e841c59f5..e71fb682072b 100644
>> --- a/drivers/acpi/pci_root.c
>> +++ b/drivers/acpi/pci_root.c
>> @@ -45,6 +45,7 @@ static int acpi_pci_root_scan_dependent(struct 
>> acpi_device *adev)
>>
>>  static const struct acpi_device_id root_device_ids[] = {
>>  	{"PNP0A03", 0},
>> +	{"ACPI0016", 0},
>>  	{"", 0},
>>  };
>>
>> @@ -135,6 +136,13 @@ static struct pci_osc_bit_struct 
>> pci_osc_support_bit[] = {
>>  	{ OSC_PCI_HPX_TYPE_3_SUPPORT, "HPX-Type3" },
>>  };
>>
>> +static struct pci_osc_bit_struct cxl_osc_support_bit[] = {
>> +	{ OSC_CXL_PORT_REG_ACCESS_SUPPORT, "CXLPortRegAccess" },
>> +	{ OSC_CXL_PORT_DEV_REG_ACCESS_SUPPORT, "CXLPortDevRegAccess" },
>> +	{ OSC_CXL_PER_SUPPORT, "CXLProtocolErrorReporting" },
>> +	{ OSC_CXL_NATIVE_HP_SUPPORT, "CXLNativeHotPlug" },
>> +};
>> +
>>  static struct pci_osc_bit_struct pci_osc_control_bit[] = {
>>  	{ OSC_PCI_EXPRESS_NATIVE_HP_CONTROL, "PCIeHotplug" },
>>  	{ OSC_PCI_SHPC_NATIVE_HP_CONTROL, "SHPCHotplug" },
>> @@ -145,6 +153,10 @@ static struct pci_osc_bit_struct 
>> pci_osc_control_bit[] = {
>>  	{ OSC_PCI_EXPRESS_DPC_CONTROL, "DPC" },
>>  };
>>
>> +static struct pci_osc_bit_struct cxl_osc_control_bit[] = {
>> +	{ OSC_CXL_MEM_PER_CONTROL, "CXLMemProtocolErrorReport" },
>> +};
>> +
>>  static void decode_osc_bits(struct acpi_pci_root *root, char *msg, 
>> u32 word,
>>  			    struct pci_osc_bit_struct *table, int size)
>>  {
>> @@ -161,27 +173,40 @@ static void decode_osc_bits(struct 
>> acpi_pci_root *root, char *msg, u32 word,
>>  	dev_info(&root->device->dev, "_OSC: %s [%s]\n", msg, buf);
>>  }
>>
>> -static void decode_osc_support(struct acpi_pci_root *root, char 
>> *msg, u32 word)
>> +static void decode_cxl_osc_support(struct acpi_pci_root *root, char 
>> *msg, u32 word)
>> +{
>> +	decode_osc_bits(root, msg, word, cxl_osc_support_bit,
>> +			ARRAY_SIZE(cxl_osc_support_bit));
>> +}
>> +
>> +static void decode_cxl_osc_control(struct acpi_pci_root *root, char 
>> *msg, u32 word)
>> +{
>> +	decode_osc_bits(root, msg, word, cxl_osc_control_bit,
>> +			ARRAY_SIZE(cxl_osc_control_bit));
>> +}
>> +
>> +static void decode_pci_osc_support(struct acpi_pci_root *root, char 
>> *msg, u32 word)
>>  {
>>  	decode_osc_bits(root, msg, word, pci_osc_support_bit,
>>  			ARRAY_SIZE(pci_osc_support_bit));
>>  }
>>
>> -static void decode_osc_control(struct acpi_pci_root *root, char 
>> *msg, u32 word)
>> +static void decode_pci_osc_control(struct acpi_pci_root *root, char 
>> *msg, u32 word)
>>  {
>>  	decode_osc_bits(root, msg, word, pci_osc_control_bit,
>>  			ARRAY_SIZE(pci_osc_control_bit));
>>  }
>>
>>  static u8 pci_osc_uuid_str[] = 
>> "33DB4D5B-1FF7-401C-9657-7441C03DD766";
>> +static u8 cxl_osc_uuid_str[] = 
>> "68F2D50B-C469-4d8A-BD3D-941A103FD3FC";
>>
>> -static acpi_status acpi_pci_run_osc(acpi_handle handle,
>> -				    const u32 *capbuf, u32 *retval)
>> +static acpi_status acpi_cap_run_osc(acpi_handle handle,
>> +				    const u32 *capbuf, u8 *uuid_str, u32 *retval)
>>  {
>>  	struct acpi_osc_context context = {
>> -		.uuid_str = pci_osc_uuid_str,
>> +		.uuid_str = uuid_str,
>>  		.rev = 1,
>> -		.cap.length = 12,
>> +		.cap.length = 20,
>>  		.cap.pointer = (void *)capbuf,
>>  	};
>>  	acpi_status status;
>> @@ -194,12 +219,41 @@ static acpi_status acpi_pci_run_osc(acpi_handle 
>> handle,
>>  	return status;
>>  }
>>
>> +static acpi_status acpi_cxl_query_osc(struct acpi_pci_root *root,
>> +				      u32 support,
>> +				      u32 *control)
>> +{
>> +	acpi_status status;
>> +	u32 result, capbuf[5];
>> +
>> +	support &= OSC_CXL_SUPPORT_MASKS;
>> +	support |= root->cxl_osc_support_set;
>> +
>> +	capbuf[OSC_QUERY_DWORD] = OSC_QUERY_ENABLE;
>> +	capbuf[CXL_OSC_SUPPORT_DWORD] = support;
>> +	if (control) {
>> +		*control &= OSC_CXL_CONTROL_MASKS;
>> +		capbuf[CXL_OSC_CONTROL_DWORD] = *control | 
>> root->cxl_osc_control_set;
>> +	} else {
>> +		/* Run _OSC query only with existing controls. */
>> +		capbuf[CXL_OSC_CONTROL_DWORD] = root->cxl_osc_control_set;
>> +	}
>> +
>> +	status = acpi_cap_run_osc(root->device->handle, capbuf, 
>> cxl_osc_uuid_str, &result);
>> +	if (ACPI_SUCCESS(status)) {
>> +		root->cxl_osc_support_set = support;
>> +		if (control)
>> +			*control = result;
>> +	}
>> +	return status;
>> +}
>> +
>>  static acpi_status acpi_pci_query_osc(struct acpi_pci_root *root,
>> -					u32 support,
>> -					u32 *control)
>> +				      u32 support,
>> +				      u32 *control)
>>  {
>>  	acpi_status status;
>> -	u32 result, capbuf[3];
>> +	u32 result, capbuf[5];
>>
>>  	support &= OSC_PCI_SUPPORT_MASKS;
>>  	support |= root->osc_support_set;
>> @@ -214,7 +268,7 @@ static acpi_status acpi_pci_query_osc(struct 
>> acpi_pci_root *root,
>>  		capbuf[OSC_CONTROL_DWORD] = root->osc_control_set;
>>  	}
>>
>> -	status = acpi_pci_run_osc(root->device->handle, capbuf, &result);
>> +	status = acpi_cap_run_osc(root->device->handle, capbuf, 
>> pci_osc_uuid_str, &result);
>>  	if (ACPI_SUCCESS(status)) {
>>  		root->osc_support_set = support;
>>  		if (control)
>> @@ -223,6 +277,16 @@ static acpi_status acpi_pci_query_osc(struct 
>> acpi_pci_root *root,
>>  	return status;
>>  }
>>
>> +static acpi_status acpi_cxl_osc_support(struct acpi_pci_root *root, 
>> u32 flags)
>> +{
>> +	acpi_status status;
>> +
>> +	mutex_lock(&osc_lock);
>> +	status = acpi_cxl_query_osc(root, flags, NULL);
>> +	mutex_unlock(&osc_lock);
>> +	return status;
>> +}
>> +
>>  static acpi_status acpi_pci_osc_support(struct acpi_pci_root *root, 
>> u32 flags)
>>  {
>>  	acpi_status status;
>> @@ -359,7 +423,7 @@ acpi_status acpi_pci_osc_control_set(acpi_handle 
>> handle, u32 *mask, u32 req)
>>  {
>>  	struct acpi_pci_root *root;
>>  	acpi_status status = AE_OK;
>> -	u32 ctrl, capbuf[3];
>> +	u32 ctrl, capbuf[5];
>>
>>  	if (!mask)
>>  		return AE_BAD_PARAMETER;
>> @@ -386,14 +450,14 @@ acpi_status 
>> acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 req)
>>  			goto out;
>>  		if (ctrl == *mask)
>>  			break;
>> -		decode_osc_control(root, "platform does not support",
>> -				   ctrl & ~(*mask));
>> +		decode_pci_osc_control(root, "platform does not support",
>> +				       ctrl & ~(*mask));
>>  		ctrl = *mask;
>>  	}
>>
>>  	if ((ctrl & req) != req) {
>> -		decode_osc_control(root, "not requesting control; platform does 
>> not support",
>> -				   req & ~(ctrl));
>> +		decode_pci_osc_control(root, "not requesting control; platform 
>> does not support",
>> +				       req & ~(ctrl));
>>  		status = AE_SUPPORT;
>>  		goto out;
>>  	}
>> @@ -401,7 +465,7 @@ acpi_status acpi_pci_osc_control_set(acpi_handle 
>> handle, u32 *mask, u32 req)
>>  	capbuf[OSC_QUERY_DWORD] = 0;
>>  	capbuf[OSC_SUPPORT_DWORD] = root->osc_support_set;
>>  	capbuf[OSC_CONTROL_DWORD] = ctrl;
>> -	status = acpi_pci_run_osc(handle, capbuf, mask);
>> +	status = acpi_cap_run_osc(handle, capbuf, pci_osc_uuid_str, mask);
>>  	if (ACPI_SUCCESS(status))
>>  		root->osc_control_set = *mask;
>>  out:
>> @@ -410,119 +474,212 @@ acpi_status 
>> acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 req)
>>  }
>>  EXPORT_SYMBOL(acpi_pci_osc_control_set);
>>
>> +/**
>> + * acpi_cxl_osc_control_set - Request control of CXL root _OSC 
>> features.
>> + * @handle: ACPI handle of a PCI root bridge (or PCIe Root Complex).
>> + * @mask: Mask of _OSC bits to request control of, place to store 
>> control mask.
>> + * @req: Mask of _OSC bits the control of is essential to the 
>> caller.
>> + **/
>> +acpi_status acpi_cxl_osc_control_set(acpi_handle handle, u32 *mask, 
>> u32 req)
>> +{
>> +	struct acpi_pci_root *root;
>> +	acpi_status status = AE_OK;
>> +	u32 ctrl, capbuf[5];
>> +
>> +	if (!mask)
>> +		return AE_BAD_PARAMETER;
>> +
>> +	ctrl = *mask & OSC_CXL_CONTROL_MASKS;
>> +	if ((ctrl & req) != req)
>> +		return AE_TYPE;
>> +
>> +	root = acpi_pci_find_root(handle);
>> +	if (!root)
>> +		return AE_NOT_EXIST;
>> +
>> +	mutex_lock(&osc_lock);
>> +
>> +	*mask = ctrl | root->cxl_osc_control_set;
>> +	/* No need to evaluate _OSC if the control was already granted. */
>> +	if ((root->cxl_osc_control_set & ctrl) == ctrl)
>> +		goto out;
>> +
>> +	/* Need to check the available controls bits before requesting 
>> them. */
>> +	while (*mask) {
>> +		status = acpi_cxl_query_osc(root, root->cxl_osc_support_set, 
>> mask);
>> +		if (ACPI_FAILURE(status))
>> +			goto out;
>> +		if (ctrl == *mask)
>> +			break;
>> +		decode_cxl_osc_control(root, "platform does not support",
>> +				       ctrl & ~(*mask));
>> +		ctrl = *mask;
>> +	}
>> +
>> +	if ((ctrl & req) != req) {
>> +		decode_cxl_osc_control(root, "not requesting control; platform 
>> does not support",
>> +				       req & ~(ctrl));
>> +		status = AE_SUPPORT;
>> +		goto out;
>> +	}
>> +
>> +	capbuf[OSC_QUERY_DWORD] = 0;
>> +	capbuf[CXL_OSC_SUPPORT_DWORD] = root->cxl_osc_support_set;
>> +	capbuf[CXL_OSC_CONTROL_DWORD] = ctrl;
>> +	status = acpi_cap_run_osc(handle, capbuf, cxl_osc_uuid_str, mask);
>> +	if (ACPI_SUCCESS(status))
>> +		root->cxl_osc_control_set = *mask;
>> +out:
>> +	mutex_unlock(&osc_lock);
>> +	return status;
>> +}
>> +EXPORT_SYMBOL(acpi_cxl_osc_control_set);
>> +
>>  static void negotiate_os_control(struct acpi_pci_root *root, int 
>> *no_aspm,
>> -				 bool is_pcie)
>> +				 bool is_pcie, bool is_cxl)
>>  {
>> -	u32 support, control, requested;
>> +	u32 cxl_support, cxl_control, pci_support, pci_control, requested;
>>  	acpi_status status;
>>  	struct acpi_device *device = root->device;
>>  	acpi_handle handle = device->handle;
>>
>> -	/*
>> -	 * Apple always return failure on _OSC calls when _OSI("Darwin") 
>> has
>> -	 * been called successfully. We know the feature set supported by 
>> the
>> -	 * platform, so avoid calling _OSC at all
>> -	 */
>> -	if (x86_apple_machine) {
>> -		root->osc_control_set = ~OSC_PCI_EXPRESS_PME_CONTROL;
>> -		decode_osc_control(root, "OS assumes control of",
>> -				   root->osc_control_set);
>> -		return;
>> -	}
>>
>> -	/*
>> -	 * All supported architectures that use ACPI have support for
>> -	 * PCI domains, so we indicate this in _OSC support capabilities.
>> -	 */
>> -	support = OSC_PCI_SEGMENT_GROUPS_SUPPORT;
>> -	support |= OSC_PCI_HPX_TYPE_3_SUPPORT;
>> -	if (pci_ext_cfg_avail())
>> -		support |= OSC_PCI_EXT_CONFIG_SUPPORT;
>> -	if (pcie_aspm_support_enabled())
>> -		support |= OSC_PCI_ASPM_SUPPORT | OSC_PCI_CLOCK_PM_SUPPORT;
>> -	if (pci_msi_enabled())
>> -		support |= OSC_PCI_MSI_SUPPORT;
>> -	if (IS_ENABLED(CONFIG_PCIE_EDR))
>> -		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;
>> +	if (is_cxl) {
>
> It would drastically shorten your patch if you used this style
> instead:
>
>   if (is_cxl) {
>     ...
>     return;
>   }
>
>   <existing code>
>
> Or, probably even better, just make a new negotiate_cxl_control() and
> make the caller choose.

Agree, and I like the latter.  Will experiment.

>
>> +		cxl_support = 0; /* No presumed default CXL_OSC support 
>> capabilities */
>> +		cxl_control = 0; /* No presumed default CXL_OSC control 
>> capabilities */
>>
>> -		/* _OSC is optional for PCI host bridges */
>> -		if ((status == AE_NOT_FOUND) && !is_pcie)
>> +		decode_cxl_osc_support(root, "OS supports", cxl_support);
>> +		status = acpi_cxl_osc_support(root, cxl_support);
>> +		if (ACPI_FAILURE(status)) {
>> +			dev_info(&device->dev, "CXL_OSC failed (%s)\n",
>> +				 acpi_format_exception(status));
>>  			return;
>> +		}
>>
>> -		dev_info(&device->dev, "_OSC failed (%s)%s\n",
>> -			 acpi_format_exception(status),
>> -			 pcie_aspm_support_enabled() ? "; disabling ASPM" : "");
>> -		return;
>> -	}
>> +		cxl_control = OSC_CXL_MEM_PER_CONTROL;
>> +		requested = cxl_control;
>> +		status = acpi_cxl_osc_control_set(handle, &cxl_control,
>> +						  OSC_CXL_MEM_PER_CONTROL);
>> +		if (ACPI_SUCCESS(status)) {
>> +			decode_cxl_osc_control(root, "OS now controls",
>> +					       cxl_control);
>> +		} else {
>> +			decode_cxl_osc_control(root, "OS requested", requested);
>> +			decode_cxl_osc_control(root, "platform willing to grant",
>> +					       cxl_control);
>> +			dev_info(&device->dev, "_OSC failed (%s)\n",
>> +				 acpi_format_exception(status));
>> +		}
>>
>> -	if (pcie_ports_disabled) {
>> -		dev_info(&device->dev, "PCIe port services disabled; not 
>> requesting _OSC control\n");
>> -		return;
>> -	}
>> +	} else { /* PCI/PCIe */
>>
>> -	if ((support & ACPI_PCIE_REQ_SUPPORT) != ACPI_PCIE_REQ_SUPPORT) {
>> -		decode_osc_support(root, "not requesting OS control; OS requires",
>> -				   ACPI_PCIE_REQ_SUPPORT);
>> -		return;
>> -	}
>> +		/*
>> +		 * Apple always return failure on _OSC calls when _OSI("Darwin") 
>> has
>> +		 * been called successfully. We know the feature set supported by 
>> the
>> +		 * platform, so avoid calling _OSC at all
>> +		 */
>> +		if (x86_apple_machine) {
>> +			root->osc_control_set = ~OSC_PCI_EXPRESS_PME_CONTROL;
>> +			decode_pci_osc_control(root, "OS assumes control of",
>> +					       root->osc_control_set);
>> +			return;
>> +		}
>> +
>> +		/*
>> +		 * All supported architectures that use ACPI have support for
>> +		 * PCI domains, so we indicate this in _OSC support capabilities.
>> +		 */
>> +		pci_support = OSC_PCI_SEGMENT_GROUPS_SUPPORT;
>> +		pci_support |= OSC_PCI_HPX_TYPE_3_SUPPORT;
>> +		if (pci_ext_cfg_avail())
>> +			pci_support |= OSC_PCI_EXT_CONFIG_SUPPORT;
>> +		if (pcie_aspm_support_enabled())
>> +			pci_support |= OSC_PCI_ASPM_SUPPORT | OSC_PCI_CLOCK_PM_SUPPORT;
>> +		if (pci_msi_enabled())
>> +			pci_support |= OSC_PCI_MSI_SUPPORT;
>> +		if (IS_ENABLED(CONFIG_PCIE_EDR))
>> +			pci_support |= OSC_PCI_EDR_SUPPORT;
>> +
>> +		decode_pci_osc_support(root, "OS supports", pci_support);
>> +		status = acpi_pci_osc_support(root, pci_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 failed (%s)%s\n",
>> +				 acpi_format_exception(status),
>> +				 pcie_aspm_support_enabled() ? "; disabling ASPM" : "");
>> +			return;
>> +		}
>> +
>> +		if (pcie_ports_disabled) {
>> +			dev_info(&device->dev, "PCIe port services disabled; not 
>> requesting _OSC control\n");
>> +			return;
>> +		}
>>
>> -	control = OSC_PCI_EXPRESS_CAPABILITY_CONTROL
>> -		| OSC_PCI_EXPRESS_PME_CONTROL;
>> +		if ((pci_support & ACPI_PCIE_REQ_SUPPORT) != 
>> ACPI_PCIE_REQ_SUPPORT) {
>> +			decode_pci_osc_support(root, "not requesting OS control; OS 
>> requires",
>> +					       ACPI_PCIE_REQ_SUPPORT);
>> +			return;
>> +		}
>>
>> -	if (IS_ENABLED(CONFIG_PCIEASPM))
>> -		control |= OSC_PCI_EXPRESS_LTR_CONTROL;
>> +		pci_control = OSC_PCI_EXPRESS_CAPABILITY_CONTROL
>> +			| OSC_PCI_EXPRESS_PME_CONTROL;
>>
>> -	if (IS_ENABLED(CONFIG_HOTPLUG_PCI_PCIE))
>> -		control |= OSC_PCI_EXPRESS_NATIVE_HP_CONTROL;
>> +		if (IS_ENABLED(CONFIG_PCIEASPM))
>> +			pci_control |= OSC_PCI_EXPRESS_LTR_CONTROL;
>>
>> -	if (IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC))
>> -		control |= OSC_PCI_SHPC_NATIVE_HP_CONTROL;
>> +		if (IS_ENABLED(CONFIG_HOTPLUG_PCI_PCIE))
>> +			pci_control |= OSC_PCI_EXPRESS_NATIVE_HP_CONTROL;
>>
>> -	if (pci_aer_available())
>> -		control |= OSC_PCI_EXPRESS_AER_CONTROL;
>> +		if (IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC))
>> +			pci_control |= OSC_PCI_SHPC_NATIVE_HP_CONTROL;
>>
>> -	/*
>> -	 * Per the Downstream Port Containment Related Enhancements ECN to
>> -	 * the PCI Firmware Spec, r3.2, sec 4.5.1, table 4-5,
>> -	 * OSC_PCI_EXPRESS_DPC_CONTROL indicates the OS supports both DPC
>> -	 * and EDR.
>> -	 */
>> -	if (IS_ENABLED(CONFIG_PCIE_DPC) && IS_ENABLED(CONFIG_PCIE_EDR))
>> -		control |= OSC_PCI_EXPRESS_DPC_CONTROL;
>> +		if (pci_aer_available())
>> +			pci_control |= OSC_PCI_EXPRESS_AER_CONTROL;
>> +
>> +		/*
>> +		 * Per the Downstream Port Containment Related Enhancements ECN to
>> +		 * the PCI Firmware Spec, r3.2, sec 4.5.1, table 4-5,
>> +		 * OSC_PCI_EXPRESS_DPC_CONTROL indicates the OS supports both DPC
>> +		 * and EDR.
>> +		 */
>> +		if (IS_ENABLED(CONFIG_PCIE_DPC) && IS_ENABLED(CONFIG_PCIE_EDR))
>> +			pci_control |= OSC_PCI_EXPRESS_DPC_CONTROL;
>> +
>> +		requested = pci_control;
>> +		status = acpi_pci_osc_control_set(handle, &pci_control,
>> +						  OSC_PCI_EXPRESS_CAPABILITY_CONTROL);
>> +		if (ACPI_SUCCESS(status)) {
>> +			decode_pci_osc_control(root, "OS now controls", pci_control);
>> +			if (acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_ASPM) {
>> +				/*
>> +				 * We have ASPM control, but the FADT indicates that
>> +				 * it's unsupported. Leave existing configuration
>> +				 * intact and prevent the OS from touching it.
>> +				 */
>> +				dev_info(&device->dev, "FADT indicates ASPM is unsupported, 
>> using BIOS configuration\n");
>> +				*no_aspm = 1;
>> +			}
>> +		} else {
>> +			decode_pci_osc_control(root, "OS requested", requested);
>> +			decode_pci_osc_control(root, "platform willing to grant",
>> +					       pci_control);
>> +			dev_info(&device->dev, "_OSC failed (%s); disabling ASPM\n",
>> +				 acpi_format_exception(status));
>>
>> -	requested = control;
>> -	status = acpi_pci_osc_control_set(handle, &control,
>> -					  OSC_PCI_EXPRESS_CAPABILITY_CONTROL);
>> -	if (ACPI_SUCCESS(status)) {
>> -		decode_osc_control(root, "OS now controls", control);
>> -		if (acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_ASPM) {
>>  			/*
>> -			 * We have ASPM control, but the FADT indicates that
>> -			 * it's unsupported. Leave existing configuration
>> -			 * intact and prevent the OS from touching it.
>> +			 * We want to disable ASPM here, but aspm_disabled
>> +			 * needs to remain in its state from boot so that we
>> +			 * properly handle PCIe 1.1 devices.  So we set this
>> +			 * flag here, to defer the action until after the ACPI
>> +			 * root scan.
>>  			 */
>> -			dev_info(&device->dev, "FADT indicates ASPM is unsupported, using 
>> BIOS configuration\n");
>>  			*no_aspm = 1;
>>  		}
>> -	} else {
>> -		decode_osc_control(root, "OS requested", requested);
>> -		decode_osc_control(root, "platform willing to grant", control);
>> -		dev_info(&device->dev, "_OSC failed (%s); disabling ASPM\n",
>> -			acpi_format_exception(status));
>> -
>> -		/*
>> -		 * We want to disable ASPM here, but aspm_disabled
>> -		 * needs to remain in its state from boot so that we
>> -		 * properly handle PCIe 1.1 devices.  So we set this
>> -		 * flag here, to defer the action until after the ACPI
>> -		 * root scan.
>> -		 */
>> -		*no_aspm = 1;
>>  	}
>>  }
>>
>> @@ -536,7 +693,7 @@ static int acpi_pci_root_add(struct acpi_device 
>> *device,
>>  	acpi_handle handle = device->handle;
>>  	int no_aspm = 0;
>>  	bool hotadd = system_state == SYSTEM_RUNNING;
>> -	bool is_pcie;
>> +	bool is_pcie, is_cxl;
>>
>>  	root = kzalloc(sizeof(struct acpi_pci_root), GFP_KERNEL);
>>  	if (!root)
>> @@ -595,7 +752,8 @@ static int acpi_pci_root_add(struct acpi_device 
>> *device,
>>  	root->mcfg_addr = acpi_pci_root_get_mcfg_addr(handle);
>>
>>  	is_pcie = strcmp(acpi_device_hid(device), "PNP0A08") == 0;
>> -	negotiate_os_control(root, &no_aspm, is_pcie);
>> +	is_cxl = strcmp(acpi_device_hid(device), "ACPI0016") == 0;
>> +	negotiate_os_control(root, &no_aspm, is_pcie, is_cxl);
>>
>>  	/*
>>  	 * TBD: Need PCI interface for enumeration/configuration of roots.
>> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
>> index 5afb6ceb284f..8f3068b6118b 100644
>> --- a/include/acpi/acpi_bus.h
>> +++ b/include/acpi/acpi_bus.h
>> @@ -577,8 +577,10 @@ struct acpi_pci_root {
>>  	u16 segment;
>>  	struct resource secondary;	/* downstream bus range */
>>
>> -	u32 osc_support_set;	/* _OSC state of support bits */
>> -	u32 osc_control_set;	/* _OSC state of control bits */
>> +	u32 osc_support_set;		/* _OSC state of support bits */
>> +	u32 osc_control_set;		/* _OSC state of control bits */
>> +	u32 cxl_osc_support_set;	/* _OSC state of CXL support bits */
>> +	u32 cxl_osc_control_set;	/* _OSC state of CXL control bits */
>
> Probably "osc_cxl_support_set".  Also for the #defines below.
>
> I assume there will be a CONFIG_PCI_CXL or similar that will wrap much
> of this code.
>
>>  	phys_addr_t mcfg_addr;
>>  };
>>
>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
>> index d661cd0ee64d..7c41535f4dc9 100644
>> --- a/include/linux/acpi.h
>> +++ b/include/linux/acpi.h
>> @@ -527,6 +527,8 @@ acpi_status acpi_run_osc(acpi_handle handle, 
>> struct acpi_osc_context *context);
>>  #define OSC_QUERY_DWORD				0	/* DWORD 1 */
>>  #define OSC_SUPPORT_DWORD			1	/* DWORD 2 */
>>  #define OSC_CONTROL_DWORD			2	/* DWORD 3 */
>> +#define CXL_OSC_SUPPORT_DWORD			3	/* DWORD 4 */
>> +#define CXL_OSC_CONTROL_DWORD			4	/* DWORD 5 */
>>
>>  /* _OSC Capabilities DWORD 1: Query/Control and Error Returns 
>> (generic) */
>>  #define OSC_QUERY_ENABLE			0x00000001  /* input */
>> @@ -570,6 +572,20 @@ extern bool osc_pc_lpi_support_confirmed;
>>  #define OSC_PCI_EXPRESS_DPC_CONTROL		0x00000080
>>  #define OSC_PCI_CONTROL_MASKS			0x000000bf
>>
>> +/* CXL Host Bridge _OSC: Capabilities DWORD 4: Support Field */
>> +#define OSC_CXL_PORT_REG_ACCESS_SUPPORT		0x00000001
>> +#define OSC_CXL_PORT_DEV_REG_ACCESS_SUPPORT	0x00000002
>> +#define OSC_CXL_PER_SUPPORT			0x00000004
>> +#define OSC_CXL_NATIVE_HP_SUPPORT		0x00000008
>> +#define OSC_CXL_SUPPORT_MASKS			0x0000000f
>> +
>> +/* CXL Host Bridge _OSC: Capabilities DWORD 5: Control Field */
>> +#define OSC_CXL_MEM_PER_CONTROL			0x00000001
>> +#define OSC_CXL_CONTROL_MASKS			0x00000001
>> +
>> +/* CXL Host Bridge _OSC: Capabilities DWORD 6: Query/Returns Field 
>> */
>> +#define OSC_CXL_MEM_EXPANDER_PER_CONTROL	0x00000001
>> +
>>  #define ACPI_GSB_ACCESS_ATTRIB_QUICK		0x00000002
>>  #define ACPI_GSB_ACCESS_ATTRIB_SEND_RCV         0x00000004
>>  #define ACPI_GSB_ACCESS_ATTRIB_BYTE		0x00000006
>> @@ -581,6 +597,8 @@ extern bool osc_pc_lpi_support_confirmed;
>>  #define ACPI_GSB_ACCESS_ATTRIB_RAW_BYTES	0x0000000E
>>  #define ACPI_GSB_ACCESS_ATTRIB_RAW_PROCESS	0x0000000F
>>
>> +extern acpi_status acpi_cxl_osc_control_set(acpi_handle handle,
>> +					     u32 *mask, u32 req);
>
> No need for this to be extern, right?  (I just sent a patch to make
> acpi_pci_osc_control_set() static also.)

Good, because I was wondering the same…

Thanks,

Sean

>
>>  extern acpi_status acpi_pci_osc_control_set(acpi_handle handle,
>>  					     u32 *mask, u32 req);
>>
>> -- 
>> 2.27.0
>>