* [PATCH 0/2] acpi: add support for CXL _OSC @ 2022-03-18 21:30 Vishal Verma 2022-03-18 21:30 ` [PATCH 1/2] PCI/ACPI: Use CXL _OSC instead of PCIe _OSC Vishal Verma ` (2 more replies) 0 siblings, 3 replies; 14+ messages in thread From: Vishal Verma @ 2022-03-18 21:30 UTC (permalink / raw) To: linux-cxl Cc: linux-acpi, Jonathan Cameron, Dan Williams, Rafael J. Wysocki, Robert Moore, Bjorn Helgaas, Vishal Verma Changes since v1[1]: - Update changelogs for both patches (Dan) - Fix support/control calculation to be based off CONFIG_MEMORY_FAILURE (Dan) - Use defines instead of magic numbers in a few places in patch 2 (Jonathan) - Fix 'capbuf' array to be the correct 5 elements. ACPI previously had '6' where only 3 were needed. With CXL capabilities, now, 5 are needed. (Jonathan). - Fix a couple of uninitialized variable warnings reported by 0day/lkp. - Drop 'RFC' annotation for the set Add support for using the CXL definition of _OSC where applicable, and negotiating CXL specific support and control bits. Patch 1 adds the new CXL _OSC UUID, and uses it instead of the PCI UUID when a root port is CXL enabled. It provides a fallback method for CXL-1.1 platforms that may not implement the CXL-2.0 _OSC. Patch 2 performs negotiation for the CXL specific _OSC support and control bits. I've tested these against a custom qemu[2], which adds the CXL _OSC (in addition to other CXL support). Specifically, _OSC support is added here[3]. [1]: https://lore.kernel.org/linux-cxl/146514b2e5237a3c027239a75ace69e72671d4c8.camel@intel.com/T/#t [2]: https://gitlab.com/jic23/qemu/-/tree/cxl-v7-draft-2-for-test [3]: https://gitlab.com/jic23/qemu/-/commit/31c85054b84645dfbd9e9bb14aa35286141c14cf Dan Williams (1): PCI/ACPI: Use CXL _OSC instead of PCIe _OSC Vishal Verma (1): acpi/pci_root: negotiate CXL _OSC include/linux/acpi.h | 14 +++ include/acpi/acpi_bus.h | 7 +- drivers/acpi/pci_root.c | 204 ++++++++++++++++++++++++++++++++++------ 3 files changed, 193 insertions(+), 32 deletions(-) base-commit: 74be98774dfbc5b8b795db726bd772e735d2edd4 -- 2.35.1 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2] PCI/ACPI: Use CXL _OSC instead of PCIe _OSC 2022-03-18 21:30 [PATCH 0/2] acpi: add support for CXL _OSC Vishal Verma @ 2022-03-18 21:30 ` Vishal Verma 2022-03-22 18:01 ` Rafael J. Wysocki ` (2 more replies) 2022-03-18 21:30 ` [PATCH 2/2] acpi/pci_root: negotiate CXL _OSC Vishal Verma 2022-03-18 21:34 ` [PATCH 0/2] acpi: add support for " Verma, Vishal L 2 siblings, 3 replies; 14+ messages in thread From: Vishal Verma @ 2022-03-18 21:30 UTC (permalink / raw) To: linux-cxl Cc: linux-acpi, Jonathan Cameron, Dan Williams, Rafael J. Wysocki, Robert Moore, Bjorn Helgaas, Rafael J. Wysocki, Jonathan Cameron, Vishal Verma From: Dan Williams <dan.j.williams@intel.com> Rather than pass a boolean flag alongside @root to all the helper functions that need to consider PCIe specifics, add is_pcie() and is_cxl() helper functions to check the flavor of @root. This also allows for dynamic fallback to PCIe _OSC in cases where an attempt to use CXL _OXC fails. This can happen on CXL 1.1 platforms that publish ACPI0016 devices to indicate CXL host bridges, but do not publish the optional CXL _OSC method. CXL _OSC is mandatory for CXL 2.0 hosts. Cc: Bjorn Helgaas <bhelgaas@google.com> Cc: "Rafael J. Wysocki" <rafael@kernel.org> Cc: Robert Moore <robert.moore@intel.com> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> Signed-off-by: Dan Williams <dan.j.williams@intel.com> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> --- include/acpi/acpi_bus.h | 1 + drivers/acpi/pci_root.c | 62 +++++++++++++++++++++++++++++++---------- 2 files changed, 48 insertions(+), 15 deletions(-) diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h index ca88c4706f2b..768ef1584055 100644 --- a/include/acpi/acpi_bus.h +++ b/include/acpi/acpi_bus.h @@ -585,6 +585,7 @@ struct acpi_pci_root { struct acpi_device * device; struct pci_bus *bus; u16 segment; + bool cxl_osc_disable; struct resource secondary; /* downstream bus range */ u32 osc_support_set; /* _OSC state of support bits */ diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c index b76db99cced3..2d834504096b 100644 --- a/drivers/acpi/pci_root.c +++ b/drivers/acpi/pci_root.c @@ -170,20 +170,47 @@ static void decode_osc_control(struct acpi_pci_root *root, char *msg, u32 word) ARRAY_SIZE(pci_osc_control_bit)); } -static u8 pci_osc_uuid_str[] = "33DB4D5B-1FF7-401C-9657-7441C03DD766"; +static bool is_pcie(struct acpi_pci_root *root) +{ + return strcmp(acpi_device_hid(root->device), "PNP0A08") == 0; +} -static acpi_status acpi_pci_run_osc(acpi_handle handle, +static bool is_cxl(struct acpi_pci_root *root) +{ + if (root->cxl_osc_disable) + return false; + return strcmp(acpi_device_hid(root->device), "ACPI0016") == 0; +} + +static u8 pci_osc_uuid_str[] = "33DB4D5B-1FF7-401C-9657-7441C03DD766"; +static u8 cxl_osc_uuid_str[] = "68F2D50B-C469-4d8A-BD3D-941A103FD3FC"; + +static char *to_uuid(struct acpi_pci_root *root) +{ + if (is_cxl(root)) + return cxl_osc_uuid_str; + return pci_osc_uuid_str; +} + +static int cap_length(struct acpi_pci_root *root) +{ + if (is_cxl(root)) + return sizeof(u32) * 6; + return sizeof(u32) * 3; +} + +static acpi_status acpi_pci_run_osc(struct acpi_pci_root *root, const u32 *capbuf, u32 *retval) { struct acpi_osc_context context = { - .uuid_str = pci_osc_uuid_str, + .uuid_str = to_uuid(root), .rev = 1, - .cap.length = 12, + .cap.length = cap_length(root), .cap.pointer = (void *)capbuf, }; acpi_status status; - status = acpi_run_osc(handle, &context); + status = acpi_run_osc(root->device->handle, &context); if (ACPI_SUCCESS(status)) { *retval = *((u32 *)(context.ret.pointer + 8)); kfree(context.ret.pointer); @@ -196,7 +223,7 @@ static acpi_status acpi_pci_query_osc(struct acpi_pci_root *root, u32 *control) { acpi_status status; - u32 result, capbuf[3]; + u32 result, capbuf[6]; support |= root->osc_support_set; @@ -204,10 +231,18 @@ static acpi_status acpi_pci_query_osc(struct acpi_pci_root *root, capbuf[OSC_SUPPORT_DWORD] = support; capbuf[OSC_CONTROL_DWORD] = *control | root->osc_control_set; - status = acpi_pci_run_osc(root->device->handle, capbuf, &result); +retry: + status = acpi_pci_run_osc(root, capbuf, &result); if (ACPI_SUCCESS(status)) { root->osc_support_set = support; *control = result; + } else if (is_cxl(root)) { + /* + * CXL _OSC is optional on CXL 1.1 hosts. Fall back to PCIe _OSC + * upon any failure using CXL _OSC. + */ + root->cxl_osc_disable = true; + goto retry; } return status; } @@ -338,7 +373,7 @@ static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 s u32 req = OSC_PCI_EXPRESS_CAPABILITY_CONTROL; struct acpi_pci_root *root; acpi_status status; - u32 ctrl, capbuf[3]; + u32 ctrl, capbuf[6]; if (!mask) return AE_BAD_PARAMETER; @@ -375,7 +410,7 @@ static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 s 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_pci_run_osc(root, capbuf, mask); if (ACPI_FAILURE(status)) return status; @@ -454,8 +489,7 @@ static bool os_control_query_checks(struct acpi_pci_root *root, u32 support) return true; } -static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm, - bool is_pcie) +static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm) { u32 support, control = 0, requested = 0; acpi_status status; @@ -506,7 +540,7 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm, *no_aspm = 1; /* _OSC is optional for PCI host bridges */ - if ((status == AE_NOT_FOUND) && !is_pcie) + if ((status == AE_NOT_FOUND) && !is_pcie(root)) return; if (control) { @@ -529,7 +563,6 @@ 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; root = kzalloc(sizeof(struct acpi_pci_root), GFP_KERNEL); if (!root) @@ -587,8 +620,7 @@ 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); + negotiate_os_control(root, &no_aspm); /* * TBD: Need PCI interface for enumeration/configuration of roots. -- 2.35.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] PCI/ACPI: Use CXL _OSC instead of PCIe _OSC 2022-03-18 21:30 ` [PATCH 1/2] PCI/ACPI: Use CXL _OSC instead of PCIe _OSC Vishal Verma @ 2022-03-22 18:01 ` Rafael J. Wysocki 2022-03-22 18:08 ` Dan Williams 2022-03-23 19:23 ` Rafael J. Wysocki 2 siblings, 0 replies; 14+ messages in thread From: Rafael J. Wysocki @ 2022-03-22 18:01 UTC (permalink / raw) To: Vishal Verma Cc: linux-cxl, ACPI Devel Maling List, Jonathan Cameron, Dan Williams, Rafael J. Wysocki, Robert Moore, Bjorn Helgaas, Rafael J. Wysocki On Fri, Mar 18, 2022 at 10:30 PM Vishal Verma <vishal.l.verma@intel.com> wrote: > > From: Dan Williams <dan.j.williams@intel.com> > > Rather than pass a boolean flag alongside @root to all the helper > functions that need to consider PCIe specifics, add is_pcie() and > is_cxl() helper functions to check the flavor of @root. This also > allows for dynamic fallback to PCIe _OSC in cases where an attempt to > use CXL _OXC fails. This can happen on CXL 1.1 platforms that publish > ACPI0016 devices to indicate CXL host bridges, but do not publish the > optional CXL _OSC method. CXL _OSC is mandatory for CXL 2.0 hosts. > > Cc: Bjorn Helgaas <bhelgaas@google.com> > Cc: "Rafael J. Wysocki" <rafael@kernel.org> > Cc: Robert Moore <robert.moore@intel.com> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> > --- > include/acpi/acpi_bus.h | 1 + > drivers/acpi/pci_root.c | 62 +++++++++++++++++++++++++++++++---------- > 2 files changed, 48 insertions(+), 15 deletions(-) > > diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h > index ca88c4706f2b..768ef1584055 100644 > --- a/include/acpi/acpi_bus.h > +++ b/include/acpi/acpi_bus.h > @@ -585,6 +585,7 @@ struct acpi_pci_root { > struct acpi_device * device; > struct pci_bus *bus; > u16 segment; > + bool cxl_osc_disable; > struct resource secondary; /* downstream bus range */ > > u32 osc_support_set; /* _OSC state of support bits */ > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c > index b76db99cced3..2d834504096b 100644 > --- a/drivers/acpi/pci_root.c > +++ b/drivers/acpi/pci_root.c > @@ -170,20 +170,47 @@ static void decode_osc_control(struct acpi_pci_root *root, char *msg, u32 word) > ARRAY_SIZE(pci_osc_control_bit)); > } > > -static u8 pci_osc_uuid_str[] = "33DB4D5B-1FF7-401C-9657-7441C03DD766"; > +static bool is_pcie(struct acpi_pci_root *root) > +{ > + return strcmp(acpi_device_hid(root->device), "PNP0A08") == 0; > +} > > -static acpi_status acpi_pci_run_osc(acpi_handle handle, > +static bool is_cxl(struct acpi_pci_root *root) > +{ > + if (root->cxl_osc_disable) > + return false; > + return strcmp(acpi_device_hid(root->device), "ACPI0016") == 0; > +} > + > +static u8 pci_osc_uuid_str[] = "33DB4D5B-1FF7-401C-9657-7441C03DD766"; > +static u8 cxl_osc_uuid_str[] = "68F2D50B-C469-4d8A-BD3D-941A103FD3FC"; > + > +static char *to_uuid(struct acpi_pci_root *root) > +{ > + if (is_cxl(root)) > + return cxl_osc_uuid_str; > + return pci_osc_uuid_str; > +} > + > +static int cap_length(struct acpi_pci_root *root) > +{ > + if (is_cxl(root)) > + return sizeof(u32) * 6; > + return sizeof(u32) * 3; > +} > + > +static acpi_status acpi_pci_run_osc(struct acpi_pci_root *root, > const u32 *capbuf, u32 *retval) > { > struct acpi_osc_context context = { > - .uuid_str = pci_osc_uuid_str, > + .uuid_str = to_uuid(root), > .rev = 1, > - .cap.length = 12, > + .cap.length = cap_length(root), > .cap.pointer = (void *)capbuf, > }; > acpi_status status; > > - status = acpi_run_osc(handle, &context); > + status = acpi_run_osc(root->device->handle, &context); > if (ACPI_SUCCESS(status)) { > *retval = *((u32 *)(context.ret.pointer + 8)); > kfree(context.ret.pointer); > @@ -196,7 +223,7 @@ static acpi_status acpi_pci_query_osc(struct acpi_pci_root *root, > u32 *control) > { > acpi_status status; > - u32 result, capbuf[3]; > + u32 result, capbuf[6]; > > support |= root->osc_support_set; > > @@ -204,10 +231,18 @@ static acpi_status acpi_pci_query_osc(struct acpi_pci_root *root, > capbuf[OSC_SUPPORT_DWORD] = support; > capbuf[OSC_CONTROL_DWORD] = *control | root->osc_control_set; > > - status = acpi_pci_run_osc(root->device->handle, capbuf, &result); > +retry: > + status = acpi_pci_run_osc(root, capbuf, &result); > if (ACPI_SUCCESS(status)) { > root->osc_support_set = support; > *control = result; > + } else if (is_cxl(root)) { > + /* > + * CXL _OSC is optional on CXL 1.1 hosts. Fall back to PCIe _OSC > + * upon any failure using CXL _OSC. > + */ > + root->cxl_osc_disable = true; > + goto retry; > } > return status; > } > @@ -338,7 +373,7 @@ static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 s > u32 req = OSC_PCI_EXPRESS_CAPABILITY_CONTROL; > struct acpi_pci_root *root; > acpi_status status; > - u32 ctrl, capbuf[3]; > + u32 ctrl, capbuf[6]; > > if (!mask) > return AE_BAD_PARAMETER; > @@ -375,7 +410,7 @@ static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 s > 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_pci_run_osc(root, capbuf, mask); > if (ACPI_FAILURE(status)) > return status; > > @@ -454,8 +489,7 @@ static bool os_control_query_checks(struct acpi_pci_root *root, u32 support) > return true; > } > > -static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm, > - bool is_pcie) > +static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm) > { > u32 support, control = 0, requested = 0; > acpi_status status; > @@ -506,7 +540,7 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm, > *no_aspm = 1; > > /* _OSC is optional for PCI host bridges */ > - if ((status == AE_NOT_FOUND) && !is_pcie) > + if ((status == AE_NOT_FOUND) && !is_pcie(root)) The parens around the status check are not needed. Although they are present in the original code, you may as well drop them as you are changing this line anyway. The patch looks good to me otherwise. > return; > > if (control) { > @@ -529,7 +563,6 @@ 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; > > root = kzalloc(sizeof(struct acpi_pci_root), GFP_KERNEL); > if (!root) > @@ -587,8 +620,7 @@ 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); > + negotiate_os_control(root, &no_aspm); > > /* > * TBD: Need PCI interface for enumeration/configuration of roots. > -- > 2.35.1 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] PCI/ACPI: Use CXL _OSC instead of PCIe _OSC 2022-03-18 21:30 ` [PATCH 1/2] PCI/ACPI: Use CXL _OSC instead of PCIe _OSC Vishal Verma 2022-03-22 18:01 ` Rafael J. Wysocki @ 2022-03-22 18:08 ` Dan Williams 2022-03-23 19:23 ` Rafael J. Wysocki 2 siblings, 0 replies; 14+ messages in thread From: Dan Williams @ 2022-03-22 18:08 UTC (permalink / raw) To: Vishal Verma Cc: linux-cxl, Linux ACPI, Jonathan Cameron, Rafael J. Wysocki, Robert Moore, Bjorn Helgaas, Rafael J. Wysocki On Fri, Mar 18, 2022 at 2:30 PM Vishal Verma <vishal.l.verma@intel.com> wrote: > > From: Dan Williams <dan.j.williams@intel.com> Oh, you dropped the first paragraph. When I said "I would replace" in my comment [1] I was only referring to the light-on-details second paragraph, so prepend this for the final version of the patch: In preparation for negotiating OS control of CXL _OSC features, do the minimal enabling to use CXL _OSC to handle the base PCIe feature negotiation. Recall that CXL _OSC is a super-set of PCIe _OSC and the CXL 2.0 specification mandates: "If a CXL Host Bridge device exposes CXL _OSC, CXL aware OSPM shall evaluate CXL _OSC and not evaluate PCIe _OSC." [1]: https://lore.kernel.org/r/CAPcyv4h581oXv59Praskpyk6ywPBm1ksxT4YPvZiv80F6ugZnw@mail.gmail.com > > Rather than pass a boolean flag alongside @root to all the helper > functions that need to consider PCIe specifics, add is_pcie() and > is_cxl() helper functions to check the flavor of @root. This also > allows for dynamic fallback to PCIe _OSC in cases where an attempt to > use CXL _OXC fails. This can happen on CXL 1.1 platforms that publish > ACPI0016 devices to indicate CXL host bridges, but do not publish the > optional CXL _OSC method. CXL _OSC is mandatory for CXL 2.0 hosts. > > Cc: Bjorn Helgaas <bhelgaas@google.com> > Cc: "Rafael J. Wysocki" <rafael@kernel.org> > Cc: Robert Moore <robert.moore@intel.com> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> > --- > include/acpi/acpi_bus.h | 1 + > drivers/acpi/pci_root.c | 62 +++++++++++++++++++++++++++++++---------- > 2 files changed, 48 insertions(+), 15 deletions(-) > > diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h > index ca88c4706f2b..768ef1584055 100644 > --- a/include/acpi/acpi_bus.h > +++ b/include/acpi/acpi_bus.h > @@ -585,6 +585,7 @@ struct acpi_pci_root { > struct acpi_device * device; > struct pci_bus *bus; > u16 segment; > + bool cxl_osc_disable; > struct resource secondary; /* downstream bus range */ > > u32 osc_support_set; /* _OSC state of support bits */ > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c > index b76db99cced3..2d834504096b 100644 > --- a/drivers/acpi/pci_root.c > +++ b/drivers/acpi/pci_root.c > @@ -170,20 +170,47 @@ static void decode_osc_control(struct acpi_pci_root *root, char *msg, u32 word) > ARRAY_SIZE(pci_osc_control_bit)); > } > > -static u8 pci_osc_uuid_str[] = "33DB4D5B-1FF7-401C-9657-7441C03DD766"; > +static bool is_pcie(struct acpi_pci_root *root) > +{ > + return strcmp(acpi_device_hid(root->device), "PNP0A08") == 0; > +} > > -static acpi_status acpi_pci_run_osc(acpi_handle handle, > +static bool is_cxl(struct acpi_pci_root *root) > +{ > + if (root->cxl_osc_disable) > + return false; > + return strcmp(acpi_device_hid(root->device), "ACPI0016") == 0; > +} > + > +static u8 pci_osc_uuid_str[] = "33DB4D5B-1FF7-401C-9657-7441C03DD766"; > +static u8 cxl_osc_uuid_str[] = "68F2D50B-C469-4d8A-BD3D-941A103FD3FC"; > + > +static char *to_uuid(struct acpi_pci_root *root) > +{ > + if (is_cxl(root)) > + return cxl_osc_uuid_str; > + return pci_osc_uuid_str; > +} > + > +static int cap_length(struct acpi_pci_root *root) > +{ > + if (is_cxl(root)) > + return sizeof(u32) * 6; > + return sizeof(u32) * 3; > +} > + > +static acpi_status acpi_pci_run_osc(struct acpi_pci_root *root, > const u32 *capbuf, u32 *retval) > { > struct acpi_osc_context context = { > - .uuid_str = pci_osc_uuid_str, > + .uuid_str = to_uuid(root), > .rev = 1, > - .cap.length = 12, > + .cap.length = cap_length(root), > .cap.pointer = (void *)capbuf, > }; > acpi_status status; > > - status = acpi_run_osc(handle, &context); > + status = acpi_run_osc(root->device->handle, &context); > if (ACPI_SUCCESS(status)) { > *retval = *((u32 *)(context.ret.pointer + 8)); > kfree(context.ret.pointer); > @@ -196,7 +223,7 @@ static acpi_status acpi_pci_query_osc(struct acpi_pci_root *root, > u32 *control) > { > acpi_status status; > - u32 result, capbuf[3]; > + u32 result, capbuf[6]; > > support |= root->osc_support_set; > > @@ -204,10 +231,18 @@ static acpi_status acpi_pci_query_osc(struct acpi_pci_root *root, > capbuf[OSC_SUPPORT_DWORD] = support; > capbuf[OSC_CONTROL_DWORD] = *control | root->osc_control_set; > > - status = acpi_pci_run_osc(root->device->handle, capbuf, &result); > +retry: > + status = acpi_pci_run_osc(root, capbuf, &result); > if (ACPI_SUCCESS(status)) { > root->osc_support_set = support; > *control = result; > + } else if (is_cxl(root)) { > + /* > + * CXL _OSC is optional on CXL 1.1 hosts. Fall back to PCIe _OSC > + * upon any failure using CXL _OSC. > + */ > + root->cxl_osc_disable = true; > + goto retry; > } > return status; > } > @@ -338,7 +373,7 @@ static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 s > u32 req = OSC_PCI_EXPRESS_CAPABILITY_CONTROL; > struct acpi_pci_root *root; > acpi_status status; > - u32 ctrl, capbuf[3]; > + u32 ctrl, capbuf[6]; > > if (!mask) > return AE_BAD_PARAMETER; > @@ -375,7 +410,7 @@ static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 s > 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_pci_run_osc(root, capbuf, mask); > if (ACPI_FAILURE(status)) > return status; > > @@ -454,8 +489,7 @@ static bool os_control_query_checks(struct acpi_pci_root *root, u32 support) > return true; > } > > -static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm, > - bool is_pcie) > +static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm) > { > u32 support, control = 0, requested = 0; > acpi_status status; > @@ -506,7 +540,7 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm, > *no_aspm = 1; > > /* _OSC is optional for PCI host bridges */ > - if ((status == AE_NOT_FOUND) && !is_pcie) > + if ((status == AE_NOT_FOUND) && !is_pcie(root)) > return; > > if (control) { > @@ -529,7 +563,6 @@ 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; > > root = kzalloc(sizeof(struct acpi_pci_root), GFP_KERNEL); > if (!root) > @@ -587,8 +620,7 @@ 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); > + negotiate_os_control(root, &no_aspm); > > /* > * TBD: Need PCI interface for enumeration/configuration of roots. > -- > 2.35.1 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] PCI/ACPI: Use CXL _OSC instead of PCIe _OSC 2022-03-18 21:30 ` [PATCH 1/2] PCI/ACPI: Use CXL _OSC instead of PCIe _OSC Vishal Verma 2022-03-22 18:01 ` Rafael J. Wysocki 2022-03-22 18:08 ` Dan Williams @ 2022-03-23 19:23 ` Rafael J. Wysocki 2022-03-23 20:39 ` Dan Williams 2 siblings, 1 reply; 14+ messages in thread From: Rafael J. Wysocki @ 2022-03-23 19:23 UTC (permalink / raw) To: Vishal Verma Cc: linux-cxl, ACPI Devel Maling List, Jonathan Cameron, Dan Williams, Rafael J. Wysocki, Robert Moore, Bjorn Helgaas, Rafael J. Wysocki On Fri, Mar 18, 2022 at 10:30 PM Vishal Verma <vishal.l.verma@intel.com> wrote: > > From: Dan Williams <dan.j.williams@intel.com> > > Rather than pass a boolean flag alongside @root to all the helper > functions that need to consider PCIe specifics, add is_pcie() and > is_cxl() helper functions to check the flavor of @root. This also > allows for dynamic fallback to PCIe _OSC in cases where an attempt to > use CXL _OXC fails. This can happen on CXL 1.1 platforms that publish > ACPI0016 devices to indicate CXL host bridges, but do not publish the > optional CXL _OSC method. CXL _OSC is mandatory for CXL 2.0 hosts. > > Cc: Bjorn Helgaas <bhelgaas@google.com> > Cc: "Rafael J. Wysocki" <rafael@kernel.org> > Cc: Robert Moore <robert.moore@intel.com> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> > --- > include/acpi/acpi_bus.h | 1 + > drivers/acpi/pci_root.c | 62 +++++++++++++++++++++++++++++++---------- > 2 files changed, 48 insertions(+), 15 deletions(-) > > diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h > index ca88c4706f2b..768ef1584055 100644 > --- a/include/acpi/acpi_bus.h > +++ b/include/acpi/acpi_bus.h > @@ -585,6 +585,7 @@ struct acpi_pci_root { > struct acpi_device * device; > struct pci_bus *bus; > u16 segment; > + bool cxl_osc_disable; > struct resource secondary; /* downstream bus range */ > > u32 osc_support_set; /* _OSC state of support bits */ > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c > index b76db99cced3..2d834504096b 100644 > --- a/drivers/acpi/pci_root.c > +++ b/drivers/acpi/pci_root.c > @@ -170,20 +170,47 @@ static void decode_osc_control(struct acpi_pci_root *root, char *msg, u32 word) > ARRAY_SIZE(pci_osc_control_bit)); > } > > -static u8 pci_osc_uuid_str[] = "33DB4D5B-1FF7-401C-9657-7441C03DD766"; > +static bool is_pcie(struct acpi_pci_root *root) > +{ > + return strcmp(acpi_device_hid(root->device), "PNP0A08") == 0; > +} > > -static acpi_status acpi_pci_run_osc(acpi_handle handle, > +static bool is_cxl(struct acpi_pci_root *root) > +{ > + if (root->cxl_osc_disable) > + return false; > + return strcmp(acpi_device_hid(root->device), "ACPI0016") == 0; One more thing. This will cause the device ID matching to take place every time is_cxl() is called which seems a bit excessive to me (and the same applies to the PCIe counterpart). IMV it would make sense to have a "bridge_type" field instead of cxl_osc_disable in acpi_pci_root and use that for all of the checks. Moreover, IIUC every CXL bridge is also a PCIe one, so the "bridge_type" could be say 1 for PCIe and 2 for CXL and checks like "bridge_type >= PCIe" would cover both these types then. > +} > + > +static u8 pci_osc_uuid_str[] = "33DB4D5B-1FF7-401C-9657-7441C03DD766"; > +static u8 cxl_osc_uuid_str[] = "68F2D50B-C469-4d8A-BD3D-941A103FD3FC"; > + > +static char *to_uuid(struct acpi_pci_root *root) > +{ > + if (is_cxl(root)) > + return cxl_osc_uuid_str; > + return pci_osc_uuid_str; > +} > + > +static int cap_length(struct acpi_pci_root *root) > +{ > + if (is_cxl(root)) > + return sizeof(u32) * 6; > + return sizeof(u32) * 3; > +} > + > +static acpi_status acpi_pci_run_osc(struct acpi_pci_root *root, > const u32 *capbuf, u32 *retval) > { > struct acpi_osc_context context = { > - .uuid_str = pci_osc_uuid_str, > + .uuid_str = to_uuid(root), > .rev = 1, > - .cap.length = 12, > + .cap.length = cap_length(root), > .cap.pointer = (void *)capbuf, > }; > acpi_status status; > > - status = acpi_run_osc(handle, &context); > + status = acpi_run_osc(root->device->handle, &context); > if (ACPI_SUCCESS(status)) { > *retval = *((u32 *)(context.ret.pointer + 8)); > kfree(context.ret.pointer); > @@ -196,7 +223,7 @@ static acpi_status acpi_pci_query_osc(struct acpi_pci_root *root, > u32 *control) > { > acpi_status status; > - u32 result, capbuf[3]; > + u32 result, capbuf[6]; > > support |= root->osc_support_set; > > @@ -204,10 +231,18 @@ static acpi_status acpi_pci_query_osc(struct acpi_pci_root *root, > capbuf[OSC_SUPPORT_DWORD] = support; > capbuf[OSC_CONTROL_DWORD] = *control | root->osc_control_set; > > - status = acpi_pci_run_osc(root->device->handle, capbuf, &result); > +retry: > + status = acpi_pci_run_osc(root, capbuf, &result); > if (ACPI_SUCCESS(status)) { > root->osc_support_set = support; > *control = result; > + } else if (is_cxl(root)) { > + /* > + * CXL _OSC is optional on CXL 1.1 hosts. Fall back to PCIe _OSC > + * upon any failure using CXL _OSC. > + */ > + root->cxl_osc_disable = true; > + goto retry; > } > return status; > } > @@ -338,7 +373,7 @@ static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 s > u32 req = OSC_PCI_EXPRESS_CAPABILITY_CONTROL; > struct acpi_pci_root *root; > acpi_status status; > - u32 ctrl, capbuf[3]; > + u32 ctrl, capbuf[6]; > > if (!mask) > return AE_BAD_PARAMETER; > @@ -375,7 +410,7 @@ static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 s > 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_pci_run_osc(root, capbuf, mask); > if (ACPI_FAILURE(status)) > return status; > > @@ -454,8 +489,7 @@ static bool os_control_query_checks(struct acpi_pci_root *root, u32 support) > return true; > } > > -static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm, > - bool is_pcie) > +static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm) > { > u32 support, control = 0, requested = 0; > acpi_status status; > @@ -506,7 +540,7 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm, > *no_aspm = 1; > > /* _OSC is optional for PCI host bridges */ > - if ((status == AE_NOT_FOUND) && !is_pcie) > + if ((status == AE_NOT_FOUND) && !is_pcie(root)) > return; > > if (control) { > @@ -529,7 +563,6 @@ 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; > > root = kzalloc(sizeof(struct acpi_pci_root), GFP_KERNEL); > if (!root) > @@ -587,8 +620,7 @@ 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); > + negotiate_os_control(root, &no_aspm); > > /* > * TBD: Need PCI interface for enumeration/configuration of roots. > -- > 2.35.1 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] PCI/ACPI: Use CXL _OSC instead of PCIe _OSC 2022-03-23 19:23 ` Rafael J. Wysocki @ 2022-03-23 20:39 ` Dan Williams 0 siblings, 0 replies; 14+ messages in thread From: Dan Williams @ 2022-03-23 20:39 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Vishal Verma, linux-cxl, ACPI Devel Maling List, Jonathan Cameron, Rafael J. Wysocki, Robert Moore, Bjorn Helgaas On Wed, Mar 23, 2022 at 12:24 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Fri, Mar 18, 2022 at 10:30 PM Vishal Verma <vishal.l.verma@intel.com> wrote: > > > > From: Dan Williams <dan.j.williams@intel.com> > > > > Rather than pass a boolean flag alongside @root to all the helper > > functions that need to consider PCIe specifics, add is_pcie() and > > is_cxl() helper functions to check the flavor of @root. This also > > allows for dynamic fallback to PCIe _OSC in cases where an attempt to > > use CXL _OXC fails. This can happen on CXL 1.1 platforms that publish > > ACPI0016 devices to indicate CXL host bridges, but do not publish the > > optional CXL _OSC method. CXL _OSC is mandatory for CXL 2.0 hosts. > > > > Cc: Bjorn Helgaas <bhelgaas@google.com> > > Cc: "Rafael J. Wysocki" <rafael@kernel.org> > > Cc: Robert Moore <robert.moore@intel.com> > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> > > --- > > include/acpi/acpi_bus.h | 1 + > > drivers/acpi/pci_root.c | 62 +++++++++++++++++++++++++++++++---------- > > 2 files changed, 48 insertions(+), 15 deletions(-) > > > > diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h > > index ca88c4706f2b..768ef1584055 100644 > > --- a/include/acpi/acpi_bus.h > > +++ b/include/acpi/acpi_bus.h > > @@ -585,6 +585,7 @@ struct acpi_pci_root { > > struct acpi_device * device; > > struct pci_bus *bus; > > u16 segment; > > + bool cxl_osc_disable; > > struct resource secondary; /* downstream bus range */ > > > > u32 osc_support_set; /* _OSC state of support bits */ > > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c > > index b76db99cced3..2d834504096b 100644 > > --- a/drivers/acpi/pci_root.c > > +++ b/drivers/acpi/pci_root.c > > @@ -170,20 +170,47 @@ static void decode_osc_control(struct acpi_pci_root *root, char *msg, u32 word) > > ARRAY_SIZE(pci_osc_control_bit)); > > } > > > > -static u8 pci_osc_uuid_str[] = "33DB4D5B-1FF7-401C-9657-7441C03DD766"; > > +static bool is_pcie(struct acpi_pci_root *root) > > +{ > > + return strcmp(acpi_device_hid(root->device), "PNP0A08") == 0; > > +} > > > > -static acpi_status acpi_pci_run_osc(acpi_handle handle, > > +static bool is_cxl(struct acpi_pci_root *root) > > +{ > > + if (root->cxl_osc_disable) > > + return false; > > + return strcmp(acpi_device_hid(root->device), "ACPI0016") == 0; > > One more thing. > > This will cause the device ID matching to take place every time > is_cxl() is called which seems a bit excessive to me (and the same > applies to the PCIe counterpart). > > IMV it would make sense to have a "bridge_type" field instead of > cxl_osc_disable in acpi_pci_root and use that for all of the checks. > > Moreover, IIUC every CXL bridge is also a PCIe one, so the > "bridge_type" could be say 1 for PCIe and 2 for CXL and checks like > "bridge_type >= PCIe" would cover both these types then. Sounds good, I like it. ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/2] acpi/pci_root: negotiate CXL _OSC 2022-03-18 21:30 [PATCH 0/2] acpi: add support for CXL _OSC Vishal Verma 2022-03-18 21:30 ` [PATCH 1/2] PCI/ACPI: Use CXL _OSC instead of PCIe _OSC Vishal Verma @ 2022-03-18 21:30 ` Vishal Verma 2022-03-22 18:22 ` Rafael J. Wysocki ` (2 more replies) 2022-03-18 21:34 ` [PATCH 0/2] acpi: add support for " Verma, Vishal L 2 siblings, 3 replies; 14+ messages in thread From: Vishal Verma @ 2022-03-18 21:30 UTC (permalink / raw) To: linux-cxl Cc: linux-acpi, Jonathan Cameron, Dan Williams, Rafael J. Wysocki, Robert Moore, Bjorn Helgaas, Vishal Verma, Rafael J. Wysocki, kernel test robot Add full support for negotiating _OSC as defined in the CXL 2.0 spec, as applicable to CXL-enabled platforms. Advertise support for the CXL features we support - 'CXL 2.0 port/device register access', 'Protocol Error Reporting', and 'CL Native Hot Plug'. Request control for 'CXL Memory Error Reporting'. The requests are dependent on CONFIG_* based pre-requisites, and prior PCI enabling, similar to how the standard PCI _OSC bits are determined. The CXL specification does not define any additional constraints on the hotplug flow beyond PCIe native hotplug, so a kernel that supports native PCIe hotplug, supports CXL hotplug. For error handling protocol and link errors just use PCIe AER. There is nascent support for amending AER events with CXL specific status [1], but there's otherwise no additional OS responsibility for CXL errors beyond PCIe AER. CXL Memory Errors behave the same as typical memory errors so CONFIG_MEMORY_FAILURE is sufficient to indicate support to platform firmware. [1]: https://lore.kernel.org/linux-cxl/164740402242.3912056.8303625392871313860.stgit@dwillia2-desk3.amr.corp.intel.com/ Cc: Bjorn Helgaas <bhelgaas@google.com> Cc: "Rafael J. Wysocki" <rafael@kernel.org> Cc: Robert Moore <robert.moore@intel.com> Cc: Dan Williams <dan.j.williams@intel.com> Reported-by: kernel test robot <lkp@intel.com> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> --- include/linux/acpi.h | 14 ++++ include/acpi/acpi_bus.h | 6 +- drivers/acpi/pci_root.c | 150 +++++++++++++++++++++++++++++++++++----- 3 files changed, 149 insertions(+), 21 deletions(-) diff --git a/include/linux/acpi.h b/include/linux/acpi.h index 6274758648e3..97c2e39f10de 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -550,10 +550,15 @@ struct acpi_osc_context { acpi_status acpi_run_osc(acpi_handle handle, struct acpi_osc_context *context); +/* Max possible _OSC capability DWORDS */ +#define OSC_CAPABILITY_DWORDS_MAX 5 + /* Indexes into _OSC Capabilities Buffer (DWORDs 2 & 3 are device-specific) */ #define OSC_QUERY_DWORD 0 /* DWORD 1 */ #define OSC_SUPPORT_DWORD 1 /* DWORD 2 */ #define OSC_CONTROL_DWORD 2 /* DWORD 3 */ +#define OSC_CXL_SUPPORT_DWORD 3 /* DWORD 4 */ +#define OSC_CXL_CONTROL_DWORD 4 /* DWORD 5 */ /* _OSC Capabilities DWORD 1: Query/Control and Error Returns (generic) */ #define OSC_QUERY_ENABLE 0x00000001 /* input */ @@ -607,6 +612,15 @@ extern u32 osc_sb_native_usb4_control; #define OSC_PCI_EXPRESS_LTR_CONTROL 0x00000020 #define OSC_PCI_EXPRESS_DPC_CONTROL 0x00000080 +/* CXL _OSC: Capabilities DWORD 4: Support Field */ +#define OSC_CXL_1_1_PORT_REG_ACCESS_SUPPORT 0x00000001 +#define OSC_CXL_2_0_PORT_DEV_REG_ACCESS_SUPPORT 0x00000002 +#define OSC_CXL_PROTOCOL_ERR_REPORTING_SUPPORT 0x00000004 +#define OSC_CXL_NATIVE_HP_SUPPORT 0x00000008 + +/* CXL _OSC: Capabilities DWORD 5: Control Field */ +#define OSC_CXL_ERROR_REPORTING_CONTROL 0x00000001 + #define ACPI_GSB_ACCESS_ATTRIB_QUICK 0x00000002 #define ACPI_GSB_ACCESS_ATTRIB_SEND_RCV 0x00000004 #define ACPI_GSB_ACCESS_ATTRIB_BYTE 0x00000006 diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h index 768ef1584055..5776d4c1509a 100644 --- a/include/acpi/acpi_bus.h +++ b/include/acpi/acpi_bus.h @@ -588,8 +588,10 @@ struct acpi_pci_root { bool cxl_osc_disable; 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 */ phys_addr_t mcfg_addr; }; diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c index 2d834504096b..c8c83fc37b81 100644 --- a/drivers/acpi/pci_root.c +++ b/drivers/acpi/pci_root.c @@ -142,6 +142,17 @@ static struct pci_osc_bit_struct pci_osc_control_bit[] = { { OSC_PCI_EXPRESS_DPC_CONTROL, "DPC" }, }; +static struct pci_osc_bit_struct cxl_osc_support_bit[] = { + { OSC_CXL_1_1_PORT_REG_ACCESS_SUPPORT, "CXL11PortRegAccess" }, + { OSC_CXL_2_0_PORT_DEV_REG_ACCESS_SUPPORT, "CXL20PortDevRegAccess" }, + { OSC_CXL_PROTOCOL_ERR_REPORTING_SUPPORT, "CXLProtocolErrorReporting" }, + { OSC_CXL_NATIVE_HP_SUPPORT, "CXLNativeHotPlug" }, +}; + +static struct pci_osc_bit_struct cxl_osc_control_bit[] = { + { OSC_CXL_ERROR_REPORTING_CONTROL, "CXLMemErrorReporting" }, +}; + static void decode_osc_bits(struct acpi_pci_root *root, char *msg, u32 word, struct pci_osc_bit_struct *table, int size) { @@ -170,6 +181,18 @@ static void decode_osc_control(struct acpi_pci_root *root, char *msg, u32 word) ARRAY_SIZE(pci_osc_control_bit)); } +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 bool is_pcie(struct acpi_pci_root *root) { return strcmp(acpi_device_hid(root->device), "PNP0A08") == 0; @@ -199,8 +222,21 @@ static int cap_length(struct acpi_pci_root *root) return sizeof(u32) * 3; } +static u32 acpi_osc_ctx_get_pci_control(struct acpi_osc_context *context) +{ + return *((u32 *)(context->ret.pointer + + (sizeof(u32) * OSC_CONTROL_DWORD))); +} + +static u32 acpi_osc_ctx_get_cxl_control(struct acpi_osc_context *context) +{ + return *((u32 *)(context->ret.pointer + + (sizeof(u32) * OSC_CXL_CONTROL_DWORD))); +} + static acpi_status acpi_pci_run_osc(struct acpi_pci_root *root, - const u32 *capbuf, u32 *retval) + const u32 *capbuf, u32 *pci_control, + u32 *cxl_control) { struct acpi_osc_context context = { .uuid_str = to_uuid(root), @@ -212,18 +248,20 @@ static acpi_status acpi_pci_run_osc(struct acpi_pci_root *root, status = acpi_run_osc(root->device->handle, &context); if (ACPI_SUCCESS(status)) { - *retval = *((u32 *)(context.ret.pointer + 8)); + *pci_control = acpi_osc_ctx_get_pci_control(&context); + if (is_cxl(root)) + *cxl_control = acpi_osc_ctx_get_cxl_control(&context); kfree(context.ret.pointer); } return status; } -static acpi_status acpi_pci_query_osc(struct acpi_pci_root *root, - u32 support, - u32 *control) +static acpi_status acpi_pci_query_osc(struct acpi_pci_root *root, u32 support, + u32 *control, u32 cxl_support, + u32 *cxl_control) { acpi_status status; - u32 result, capbuf[6]; + u32 pci_result, cxl_result, capbuf[OSC_CAPABILITY_DWORDS_MAX]; support |= root->osc_support_set; @@ -231,11 +269,21 @@ static acpi_status acpi_pci_query_osc(struct acpi_pci_root *root, capbuf[OSC_SUPPORT_DWORD] = support; capbuf[OSC_CONTROL_DWORD] = *control | root->osc_control_set; + if (is_cxl(root)) { + cxl_support |= root->cxl_osc_support_set; + capbuf[OSC_CXL_SUPPORT_DWORD] = cxl_support; + capbuf[OSC_CXL_CONTROL_DWORD] = *cxl_control | root->cxl_osc_control_set; + } + retry: - status = acpi_pci_run_osc(root, capbuf, &result); + status = acpi_pci_run_osc(root, capbuf, &pci_result, &cxl_result); if (ACPI_SUCCESS(status)) { root->osc_support_set = support; - *control = result; + *control = pci_result; + if (is_cxl(root)) { + root->cxl_osc_support_set = cxl_support; + *cxl_control = cxl_result; + } } else if (is_cxl(root)) { /* * CXL _OSC is optional on CXL 1.1 hosts. Fall back to PCIe _OSC @@ -358,6 +406,8 @@ EXPORT_SYMBOL_GPL(acpi_get_pci_dev); * @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. * @support: _OSC supported capability. + * @cxl_mask: Mask of CXL _OSC control bits, place to store control mask. + * @cxl_support: CXL _OSC supported capability. * * Run _OSC query for @mask and if that is successful, compare the returned * mask of control bits with @req. If all of the @req bits are set in the @@ -368,12 +418,14 @@ EXPORT_SYMBOL_GPL(acpi_get_pci_dev); * _OSC bits the BIOS has granted control of, but its contents are meaningless * on failure. **/ -static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 support) +static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, + u32 support, u32 *cxl_mask, + u32 cxl_support) { u32 req = OSC_PCI_EXPRESS_CAPABILITY_CONTROL; struct acpi_pci_root *root; acpi_status status; - u32 ctrl, capbuf[6]; + u32 ctrl, cxl_ctrl = 0, capbuf[OSC_CAPABILITY_DWORDS_MAX]; if (!mask) return AE_BAD_PARAMETER; @@ -385,20 +437,35 @@ static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 s ctrl = *mask; *mask |= root->osc_control_set; + if (is_cxl(root)) { + cxl_ctrl = *cxl_mask; + *mask |= root->osc_control_set; + } + /* Need to check the available controls bits before requesting them. */ do { - status = acpi_pci_query_osc(root, support, mask); + status = acpi_pci_query_osc(root, support, mask, cxl_support, + cxl_mask); if (ACPI_FAILURE(status)) return status; - if (ctrl == *mask) - break; - decode_osc_control(root, "platform does not support", - ctrl & ~(*mask)); + if (is_cxl(root)) { + if ((ctrl == *mask) && (cxl_ctrl == *cxl_mask)) + break; + decode_cxl_osc_control(root, "platform does not support", + cxl_ctrl & ~(*cxl_mask)); + } else { + if (ctrl == *mask) + break; + decode_osc_control(root, "platform does not support", + ctrl & ~(*mask)); + } ctrl = *mask; - } while (*mask); + cxl_ctrl = *cxl_mask; + } while (*mask || *cxl_mask); /* No need to request _OSC if the control was already granted. */ - if ((root->osc_control_set & ctrl) == ctrl) + if (((root->osc_control_set & ctrl) == ctrl) && + ((root->cxl_osc_control_set & cxl_ctrl) == cxl_ctrl)) return AE_OK; if ((ctrl & req) != req) { @@ -410,11 +477,17 @@ static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 s capbuf[OSC_QUERY_DWORD] = 0; capbuf[OSC_SUPPORT_DWORD] = root->osc_support_set; capbuf[OSC_CONTROL_DWORD] = ctrl; - status = acpi_pci_run_osc(root, capbuf, mask); + if (is_cxl(root)) { + capbuf[OSC_CXL_SUPPORT_DWORD] = root->cxl_osc_support_set; + capbuf[OSC_CXL_CONTROL_DWORD] = cxl_ctrl; + } + + status = acpi_pci_run_osc(root, capbuf, mask, cxl_mask); if (ACPI_FAILURE(status)) return status; root->osc_control_set = *mask; + root->cxl_osc_control_set = *cxl_mask; return AE_OK; } @@ -440,6 +513,19 @@ static u32 calculate_support(void) return support; } +static u32 calculate_cxl_support(void) +{ + u32 support; + + support = OSC_CXL_2_0_PORT_DEV_REG_ACCESS_SUPPORT; + if (pci_aer_available()) + support |= OSC_CXL_PROTOCOL_ERR_REPORTING_SUPPORT; + if (IS_ENABLED(CONFIG_HOTPLUG_PCI_PCIE)) + support |= OSC_CXL_NATIVE_HP_SUPPORT; + + return support; +} + static u32 calculate_control(void) { u32 control; @@ -471,6 +557,16 @@ static u32 calculate_control(void) return control; } +static u32 calculate_cxl_control(void) +{ + u32 control = 0; + + if (IS_ENABLED(CONFIG_MEMORY_FAILURE)) + control |= OSC_CXL_ERROR_REPORTING_CONTROL; + + return control; +} + static bool os_control_query_checks(struct acpi_pci_root *root, u32 support) { struct acpi_device *device = root->device; @@ -492,6 +588,7 @@ static bool os_control_query_checks(struct acpi_pci_root *root, u32 support) static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm) { u32 support, control = 0, requested = 0; + u32 cxl_support = 0, cxl_control = 0, cxl_requested = 0; acpi_status status; struct acpi_device *device = root->device; acpi_handle handle = device->handle; @@ -515,10 +612,20 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm) if (os_control_query_checks(root, support)) requested = control = calculate_control(); - status = acpi_pci_osc_control_set(handle, &control, support); + if (is_cxl(root)) { + cxl_support = calculate_cxl_support(); + decode_cxl_osc_support(root, "OS supports", cxl_support); + cxl_requested = cxl_control = calculate_cxl_control(); + } + + status = acpi_pci_osc_control_set(handle, &control, support, + &cxl_control, cxl_support); if (ACPI_SUCCESS(status)) { if (control) decode_osc_control(root, "OS now controls", control); + if (cxl_control) + decode_cxl_osc_control(root, "OS now controls", + cxl_control); if (acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_ASPM) { /* @@ -547,6 +654,11 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm) decode_osc_control(root, "OS requested", requested); decode_osc_control(root, "platform willing to grant", control); } + if (cxl_control) { + decode_cxl_osc_control(root, "OS requested", cxl_requested); + decode_cxl_osc_control(root, "platform willing to grant", + cxl_control); + } dev_info(&device->dev, "_OSC: platform retains control of PCIe features (%s)\n", acpi_format_exception(status)); -- 2.35.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] acpi/pci_root: negotiate CXL _OSC 2022-03-18 21:30 ` [PATCH 2/2] acpi/pci_root: negotiate CXL _OSC Vishal Verma @ 2022-03-22 18:22 ` Rafael J. Wysocki 2022-03-23 0:47 ` Davidlohr Bueso 2022-03-23 19:25 ` Rafael J. Wysocki 2 siblings, 0 replies; 14+ messages in thread From: Rafael J. Wysocki @ 2022-03-22 18:22 UTC (permalink / raw) To: Vishal Verma Cc: linux-cxl, ACPI Devel Maling List, Jonathan Cameron, Dan Williams, Rafael J. Wysocki, Robert Moore, Bjorn Helgaas, Rafael J. Wysocki, kernel test robot On Fri, Mar 18, 2022 at 10:30 PM Vishal Verma <vishal.l.verma@intel.com> wrote: > > Add full support for negotiating _OSC as defined in the CXL 2.0 spec, as > applicable to CXL-enabled platforms. Advertise support for the CXL > features we support - 'CXL 2.0 port/device register access', 'Protocol > Error Reporting', and 'CL Native Hot Plug'. Request control for 'CXL > Memory Error Reporting'. The requests are dependent on CONFIG_* based > pre-requisites, and prior PCI enabling, similar to how the standard PCI > _OSC bits are determined. > > The CXL specification does not define any additional constraints on > the hotplug flow beyond PCIe native hotplug, so a kernel that supports > native PCIe hotplug, supports CXL hotplug. For error handling protocol > and link errors just use PCIe AER. There is nascent support for > amending AER events with CXL specific status [1], but there's > otherwise no additional OS responsibility for CXL errors beyond PCIe > AER. CXL Memory Errors behave the same as typical memory errors so > CONFIG_MEMORY_FAILURE is sufficient to indicate support to platform > firmware. > > [1]: https://lore.kernel.org/linux-cxl/164740402242.3912056.8303625392871313860.stgit@dwillia2-desk3.amr.corp.intel.com/ > > Cc: Bjorn Helgaas <bhelgaas@google.com> > Cc: "Rafael J. Wysocki" <rafael@kernel.org> > Cc: Robert Moore <robert.moore@intel.com> > Cc: Dan Williams <dan.j.williams@intel.com> > Reported-by: kernel test robot <lkp@intel.com> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> > --- > include/linux/acpi.h | 14 ++++ > include/acpi/acpi_bus.h | 6 +- > drivers/acpi/pci_root.c | 150 +++++++++++++++++++++++++++++++++++----- > 3 files changed, 149 insertions(+), 21 deletions(-) > > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > index 6274758648e3..97c2e39f10de 100644 > --- a/include/linux/acpi.h > +++ b/include/linux/acpi.h > @@ -550,10 +550,15 @@ struct acpi_osc_context { > > acpi_status acpi_run_osc(acpi_handle handle, struct acpi_osc_context *context); > > +/* Max possible _OSC capability DWORDS */ > +#define OSC_CAPABILITY_DWORDS_MAX 5 > + > /* Indexes into _OSC Capabilities Buffer (DWORDs 2 & 3 are device-specific) */ The comment needs to be updated too. > #define OSC_QUERY_DWORD 0 /* DWORD 1 */ > #define OSC_SUPPORT_DWORD 1 /* DWORD 2 */ > #define OSC_CONTROL_DWORD 2 /* DWORD 3 */ > +#define OSC_CXL_SUPPORT_DWORD 3 /* DWORD 4 */ > +#define OSC_CXL_CONTROL_DWORD 4 /* DWORD 5 */ > > /* _OSC Capabilities DWORD 1: Query/Control and Error Returns (generic) */ > #define OSC_QUERY_ENABLE 0x00000001 /* input */ > @@ -607,6 +612,15 @@ extern u32 osc_sb_native_usb4_control; > #define OSC_PCI_EXPRESS_LTR_CONTROL 0x00000020 > #define OSC_PCI_EXPRESS_DPC_CONTROL 0x00000080 > > +/* CXL _OSC: Capabilities DWORD 4: Support Field */ > +#define OSC_CXL_1_1_PORT_REG_ACCESS_SUPPORT 0x00000001 > +#define OSC_CXL_2_0_PORT_DEV_REG_ACCESS_SUPPORT 0x00000002 > +#define OSC_CXL_PROTOCOL_ERR_REPORTING_SUPPORT 0x00000004 > +#define OSC_CXL_NATIVE_HP_SUPPORT 0x00000008 > + > +/* CXL _OSC: Capabilities DWORD 5: Control Field */ > +#define OSC_CXL_ERROR_REPORTING_CONTROL 0x00000001 > + > #define ACPI_GSB_ACCESS_ATTRIB_QUICK 0x00000002 > #define ACPI_GSB_ACCESS_ATTRIB_SEND_RCV 0x00000004 > #define ACPI_GSB_ACCESS_ATTRIB_BYTE 0x00000006 > diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h > index 768ef1584055..5776d4c1509a 100644 > --- a/include/acpi/acpi_bus.h > +++ b/include/acpi/acpi_bus.h > @@ -588,8 +588,10 @@ struct acpi_pci_root { > bool cxl_osc_disable; > 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 */ > phys_addr_t mcfg_addr; > }; > > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c > index 2d834504096b..c8c83fc37b81 100644 > --- a/drivers/acpi/pci_root.c > +++ b/drivers/acpi/pci_root.c > @@ -142,6 +142,17 @@ static struct pci_osc_bit_struct pci_osc_control_bit[] = { > { OSC_PCI_EXPRESS_DPC_CONTROL, "DPC" }, > }; > > +static struct pci_osc_bit_struct cxl_osc_support_bit[] = { > + { OSC_CXL_1_1_PORT_REG_ACCESS_SUPPORT, "CXL11PortRegAccess" }, > + { OSC_CXL_2_0_PORT_DEV_REG_ACCESS_SUPPORT, "CXL20PortDevRegAccess" }, > + { OSC_CXL_PROTOCOL_ERR_REPORTING_SUPPORT, "CXLProtocolErrorReporting" }, > + { OSC_CXL_NATIVE_HP_SUPPORT, "CXLNativeHotPlug" }, > +}; > + > +static struct pci_osc_bit_struct cxl_osc_control_bit[] = { > + { OSC_CXL_ERROR_REPORTING_CONTROL, "CXLMemErrorReporting" }, > +}; > + > static void decode_osc_bits(struct acpi_pci_root *root, char *msg, u32 word, > struct pci_osc_bit_struct *table, int size) > { > @@ -170,6 +181,18 @@ static void decode_osc_control(struct acpi_pci_root *root, char *msg, u32 word) > ARRAY_SIZE(pci_osc_control_bit)); > } > > +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 bool is_pcie(struct acpi_pci_root *root) > { > return strcmp(acpi_device_hid(root->device), "PNP0A08") == 0; > @@ -199,8 +222,21 @@ static int cap_length(struct acpi_pci_root *root) > return sizeof(u32) * 3; > } > > +static u32 acpi_osc_ctx_get_pci_control(struct acpi_osc_context *context) > +{ > + return *((u32 *)(context->ret.pointer + > + (sizeof(u32) * OSC_CONTROL_DWORD))); I would do u32 *ret = context->ret.pointer; return ret[OSC_CONTROL_DWORD]; Wouldn't this work? > +} > + > +static u32 acpi_osc_ctx_get_cxl_control(struct acpi_osc_context *context) > +{ > + return *((u32 *)(context->ret.pointer + > + (sizeof(u32) * OSC_CXL_CONTROL_DWORD))); > +} And same here. > + > static acpi_status acpi_pci_run_osc(struct acpi_pci_root *root, > - const u32 *capbuf, u32 *retval) > + const u32 *capbuf, u32 *pci_control, > + u32 *cxl_control) > { > struct acpi_osc_context context = { > .uuid_str = to_uuid(root), > @@ -212,18 +248,20 @@ static acpi_status acpi_pci_run_osc(struct acpi_pci_root *root, > > status = acpi_run_osc(root->device->handle, &context); > if (ACPI_SUCCESS(status)) { > - *retval = *((u32 *)(context.ret.pointer + 8)); > + *pci_control = acpi_osc_ctx_get_pci_control(&context); > + if (is_cxl(root)) > + *cxl_control = acpi_osc_ctx_get_cxl_control(&context); > kfree(context.ret.pointer); > } > return status; > } > > -static acpi_status acpi_pci_query_osc(struct acpi_pci_root *root, > - u32 support, > - u32 *control) > +static acpi_status acpi_pci_query_osc(struct acpi_pci_root *root, u32 support, > + u32 *control, u32 cxl_support, > + u32 *cxl_control) > { > acpi_status status; > - u32 result, capbuf[6]; > + u32 pci_result, cxl_result, capbuf[OSC_CAPABILITY_DWORDS_MAX]; > > support |= root->osc_support_set; > > @@ -231,11 +269,21 @@ static acpi_status acpi_pci_query_osc(struct acpi_pci_root *root, > capbuf[OSC_SUPPORT_DWORD] = support; > capbuf[OSC_CONTROL_DWORD] = *control | root->osc_control_set; > > + if (is_cxl(root)) { > + cxl_support |= root->cxl_osc_support_set; > + capbuf[OSC_CXL_SUPPORT_DWORD] = cxl_support; > + capbuf[OSC_CXL_CONTROL_DWORD] = *cxl_control | root->cxl_osc_control_set; > + } > + > retry: > - status = acpi_pci_run_osc(root, capbuf, &result); > + status = acpi_pci_run_osc(root, capbuf, &pci_result, &cxl_result); > if (ACPI_SUCCESS(status)) { > root->osc_support_set = support; > - *control = result; > + *control = pci_result; > + if (is_cxl(root)) { > + root->cxl_osc_support_set = cxl_support; > + *cxl_control = cxl_result; > + } > } else if (is_cxl(root)) { > /* > * CXL _OSC is optional on CXL 1.1 hosts. Fall back to PCIe _OSC > @@ -358,6 +406,8 @@ EXPORT_SYMBOL_GPL(acpi_get_pci_dev); > * @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. > * @support: _OSC supported capability. > + * @cxl_mask: Mask of CXL _OSC control bits, place to store control mask. > + * @cxl_support: CXL _OSC supported capability. > * > * Run _OSC query for @mask and if that is successful, compare the returned > * mask of control bits with @req. If all of the @req bits are set in the > @@ -368,12 +418,14 @@ EXPORT_SYMBOL_GPL(acpi_get_pci_dev); > * _OSC bits the BIOS has granted control of, but its contents are meaningless > * on failure. > **/ > -static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 support) > +static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, > + u32 support, u32 *cxl_mask, > + u32 cxl_support) > { > u32 req = OSC_PCI_EXPRESS_CAPABILITY_CONTROL; > struct acpi_pci_root *root; > acpi_status status; > - u32 ctrl, capbuf[6]; > + u32 ctrl, cxl_ctrl = 0, capbuf[OSC_CAPABILITY_DWORDS_MAX]; > > if (!mask) > return AE_BAD_PARAMETER; > @@ -385,20 +437,35 @@ static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 s > ctrl = *mask; > *mask |= root->osc_control_set; > > + if (is_cxl(root)) { > + cxl_ctrl = *cxl_mask; > + *mask |= root->osc_control_set; > + } > + > /* Need to check the available controls bits before requesting them. */ > do { > - status = acpi_pci_query_osc(root, support, mask); > + status = acpi_pci_query_osc(root, support, mask, cxl_support, > + cxl_mask); > if (ACPI_FAILURE(status)) > return status; > - if (ctrl == *mask) > - break; > - decode_osc_control(root, "platform does not support", > - ctrl & ~(*mask)); > + if (is_cxl(root)) { > + if ((ctrl == *mask) && (cxl_ctrl == *cxl_mask)) Inner parens are not needed. > + break; > + decode_cxl_osc_control(root, "platform does not support", > + cxl_ctrl & ~(*cxl_mask)); > + } else { > + if (ctrl == *mask) > + break; > + decode_osc_control(root, "platform does not support", > + ctrl & ~(*mask)); > + } > ctrl = *mask; > - } while (*mask); > + cxl_ctrl = *cxl_mask; > + } while (*mask || *cxl_mask); > > /* No need to request _OSC if the control was already granted. */ > - if ((root->osc_control_set & ctrl) == ctrl) > + if (((root->osc_control_set & ctrl) == ctrl) && > + ((root->cxl_osc_control_set & cxl_ctrl) == cxl_ctrl)) Same here. > return AE_OK; > > if ((ctrl & req) != req) { > @@ -410,11 +477,17 @@ static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 s > capbuf[OSC_QUERY_DWORD] = 0; > capbuf[OSC_SUPPORT_DWORD] = root->osc_support_set; > capbuf[OSC_CONTROL_DWORD] = ctrl; > - status = acpi_pci_run_osc(root, capbuf, mask); > + if (is_cxl(root)) { > + capbuf[OSC_CXL_SUPPORT_DWORD] = root->cxl_osc_support_set; > + capbuf[OSC_CXL_CONTROL_DWORD] = cxl_ctrl; > + } > + > + status = acpi_pci_run_osc(root, capbuf, mask, cxl_mask); > if (ACPI_FAILURE(status)) > return status; > > root->osc_control_set = *mask; > + root->cxl_osc_control_set = *cxl_mask; > return AE_OK; > } > > @@ -440,6 +513,19 @@ static u32 calculate_support(void) > return support; > } > > +static u32 calculate_cxl_support(void) > +{ > + u32 support; > + > + support = OSC_CXL_2_0_PORT_DEV_REG_ACCESS_SUPPORT; > + if (pci_aer_available()) > + support |= OSC_CXL_PROTOCOL_ERR_REPORTING_SUPPORT; > + if (IS_ENABLED(CONFIG_HOTPLUG_PCI_PCIE)) > + support |= OSC_CXL_NATIVE_HP_SUPPORT; > + > + return support; > +} > + > static u32 calculate_control(void) > { > u32 control; > @@ -471,6 +557,16 @@ static u32 calculate_control(void) > return control; > } > > +static u32 calculate_cxl_control(void) > +{ > + u32 control = 0; > + > + if (IS_ENABLED(CONFIG_MEMORY_FAILURE)) > + control |= OSC_CXL_ERROR_REPORTING_CONTROL; > + > + return control; > +} > + > static bool os_control_query_checks(struct acpi_pci_root *root, u32 support) > { > struct acpi_device *device = root->device; > @@ -492,6 +588,7 @@ static bool os_control_query_checks(struct acpi_pci_root *root, u32 support) > static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm) > { > u32 support, control = 0, requested = 0; > + u32 cxl_support = 0, cxl_control = 0, cxl_requested = 0; > acpi_status status; > struct acpi_device *device = root->device; > acpi_handle handle = device->handle; > @@ -515,10 +612,20 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm) > if (os_control_query_checks(root, support)) > requested = control = calculate_control(); > > - status = acpi_pci_osc_control_set(handle, &control, support); > + if (is_cxl(root)) { > + cxl_support = calculate_cxl_support(); > + decode_cxl_osc_support(root, "OS supports", cxl_support); > + cxl_requested = cxl_control = calculate_cxl_control(); > + } > + > + status = acpi_pci_osc_control_set(handle, &control, support, > + &cxl_control, cxl_support); > if (ACPI_SUCCESS(status)) { > if (control) > decode_osc_control(root, "OS now controls", control); > + if (cxl_control) > + decode_cxl_osc_control(root, "OS now controls", > + cxl_control); > > if (acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_ASPM) { > /* > @@ -547,6 +654,11 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm) > decode_osc_control(root, "OS requested", requested); > decode_osc_control(root, "platform willing to grant", control); > } > + if (cxl_control) { > + decode_cxl_osc_control(root, "OS requested", cxl_requested); > + decode_cxl_osc_control(root, "platform willing to grant", > + cxl_control); > + } > > dev_info(&device->dev, "_OSC: platform retains control of PCIe features (%s)\n", > acpi_format_exception(status)); > -- Apart from the minor things mentioned above, it looks good to me. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] acpi/pci_root: negotiate CXL _OSC 2022-03-18 21:30 ` [PATCH 2/2] acpi/pci_root: negotiate CXL _OSC Vishal Verma 2022-03-22 18:22 ` Rafael J. Wysocki @ 2022-03-23 0:47 ` Davidlohr Bueso 2022-03-23 18:29 ` Verma, Vishal L 2022-03-23 19:25 ` Rafael J. Wysocki 2 siblings, 1 reply; 14+ messages in thread From: Davidlohr Bueso @ 2022-03-23 0:47 UTC (permalink / raw) To: Vishal Verma Cc: linux-cxl, linux-acpi, Jonathan Cameron, Dan Williams, Rafael J. Wysocki, Robert Moore, Bjorn Helgaas, Rafael J. Wysocki, kernel test robot On Fri, 18 Mar 2022, Vishal Verma wrote: >+/* Max possible _OSC capability DWORDS */ >+#define OSC_CAPABILITY_DWORDS_MAX 5 >+ > /* Indexes into _OSC Capabilities Buffer (DWORDs 2 & 3 are device-specific) */ > #define OSC_QUERY_DWORD 0 /* DWORD 1 */ > #define OSC_SUPPORT_DWORD 1 /* DWORD 2 */ > #define OSC_CONTROL_DWORD 2 /* DWORD 3 */ >+#define OSC_CXL_SUPPORT_DWORD 3 /* DWORD 4 */ >+#define OSC_CXL_CONTROL_DWORD 4 /* DWORD 5 */ Shouldn't all this be in patch 1/2 (and also as enum maybe)? Or at least the define OSC_CAPABILITY_DWORDS_MAX instead of having to do: >-static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 support) >+static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, >+ u32 support, u32 *cxl_mask, >+ u32 cxl_support) > { > u32 req = OSC_PCI_EXPRESS_CAPABILITY_CONTROL; > struct acpi_pci_root *root; > acpi_status status; >- u32 ctrl, capbuf[6]; >+ u32 ctrl, cxl_ctrl = 0, capbuf[OSC_CAPABILITY_DWORDS_MAX]; ... which btw why is capbuf 6 in the previous patch and now 5 in this one? Sorry if I missed anything obvious here, just seems odd. And also it's ugly to just add extra params to acpi_pci_osc_control_set() and have callers do do: >+ status = acpi_pci_osc_control_set(handle, &control, support, >+ &cxl_control, cxl_support); And this sort of thing happens all over the patch with struct acpi_pci_root. So the whole handling of the _OSC state of support/control bits imo really wants to be consolidated between CXL and non-CXL. Thanks, Davidlohr ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] acpi/pci_root: negotiate CXL _OSC 2022-03-23 0:47 ` Davidlohr Bueso @ 2022-03-23 18:29 ` Verma, Vishal L 2022-03-23 19:02 ` Dan Williams 0 siblings, 1 reply; 14+ messages in thread From: Verma, Vishal L @ 2022-03-23 18:29 UTC (permalink / raw) To: dave Cc: lkp, rafael, linux-cxl, Jonathan.Cameron, Williams, Dan J, linux-acpi, Wysocki, Rafael J, Moore, Robert, bhelgaas On Tue, 2022-03-22 at 17:47 -0700, Davidlohr Bueso wrote: > On Fri, 18 Mar 2022, Vishal Verma wrote: > > > +/* Max possible _OSC capability DWORDS */ > > +#define OSC_CAPABILITY_DWORDS_MAX 5 > > + > > /* Indexes into _OSC Capabilities Buffer (DWORDs 2 & 3 are device-specific) */ > > #define OSC_QUERY_DWORD 0 /* DWORD 1 */ > > #define OSC_SUPPORT_DWORD 1 /* DWORD 2 */ > > #define OSC_CONTROL_DWORD 2 /* DWORD 3 */ > > +#define OSC_CXL_SUPPORT_DWORD 3 /* DWORD 4 */ > > +#define OSC_CXL_CONTROL_DWORD 4 /* DWORD 5 */ > > Shouldn't all this be in patch 1/2 (and also as enum maybe)? Or at least > the define OSC_CAPABILITY_DWORDS_MAX instead of having to do: Yeah moving DWORDS_MAX and the associated changes to patch 1 makes sense. > > > -static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 support) > > +static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, > > + u32 support, u32 *cxl_mask, > > + u32 cxl_support) > > { > > u32 req = OSC_PCI_EXPRESS_CAPABILITY_CONTROL; > > struct acpi_pci_root *root; > > acpi_status status; > > - u32 ctrl, capbuf[6]; > > + u32 ctrl, cxl_ctrl = 0, capbuf[OSC_CAPABILITY_DWORDS_MAX]; > > ... which btw why is capbuf 6 in the previous patch and now 5 in this one? > Sorry if I missed anything obvious here, just seems odd. Oh, I just noticed patch 1 changes it to 6 in the first place (I thought it was just 6 even before this set), that is wrong. Previously, the PCI only _OSC had 3, and with CXL it becomes 5. I'll fix this up. > > And also it's ugly to just add extra params to acpi_pci_osc_control_set() > and have callers do do: > > > + status = acpi_pci_osc_control_set(handle, &control, support, > > + &cxl_control, cxl_support); > > And this sort of thing happens all over the patch with struct acpi_pci_root. > So the whole handling of the _OSC state of support/control bits imo really > wants to be consolidated between CXL and non-CXL. I don't disagree :) - Any thoughts on what can be done to consolidate things further? This seemed like the lowest touch approach that kept existing PCI-only paths as-is. > > Thanks, > Davidlohr Thanks for the review! ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] acpi/pci_root: negotiate CXL _OSC 2022-03-23 18:29 ` Verma, Vishal L @ 2022-03-23 19:02 ` Dan Williams 0 siblings, 0 replies; 14+ messages in thread From: Dan Williams @ 2022-03-23 19:02 UTC (permalink / raw) To: Verma, Vishal L Cc: dave, lkp, rafael, linux-cxl, Jonathan.Cameron@Huawei.com, linux-acpi, Wysocki, Rafael J, Moore, Robert, bhelgaas On Wed, Mar 23, 2022 at 11:29 AM Verma, Vishal L <vishal.l.verma@intel.com> wrote: [..] > > > -static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 support) > > > +static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, > > > + u32 support, u32 *cxl_mask, > > > + u32 cxl_support) > > > { > > > u32 req = OSC_PCI_EXPRESS_CAPABILITY_CONTROL; > > > struct acpi_pci_root *root; > > > acpi_status status; > > > - u32 ctrl, capbuf[6]; > > > + u32 ctrl, cxl_ctrl = 0, capbuf[OSC_CAPABILITY_DWORDS_MAX]; > > > > ... which btw why is capbuf 6 in the previous patch and now 5 in this one? > > Sorry if I missed anything obvious here, just seems odd. > > Oh, I just noticed patch 1 changes it to 6 in the first place (I > thought it was just 6 even before this set), that is wrong. Previously, > the PCI only _OSC had 3, and with CXL it becomes 5. I'll fix this up. Yeah, my fault for making it 6 think I was confusing the DWORD numbers as 0-based. > > And also it's ugly to just add extra params to acpi_pci_osc_control_set() > > and have callers do do: > > > > > + status = acpi_pci_osc_control_set(handle, &control, support, > > > + &cxl_control, cxl_support); > > > > And this sort of thing happens all over the patch with struct acpi_pci_root. > > So the whole handling of the _OSC state of support/control bits imo really > > wants to be consolidated between CXL and non-CXL. > > I don't disagree :) - Any thoughts on what can be done to consolidate > things further? This seemed like the lowest touch approach that kept > existing PCI-only paths as-is. That sounds like a follow-on cleanup opportunity to me, but we have time since this series seems like v5.19 material given the merge window is already open. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] acpi/pci_root: negotiate CXL _OSC 2022-03-18 21:30 ` [PATCH 2/2] acpi/pci_root: negotiate CXL _OSC Vishal Verma 2022-03-22 18:22 ` Rafael J. Wysocki 2022-03-23 0:47 ` Davidlohr Bueso @ 2022-03-23 19:25 ` Rafael J. Wysocki 2022-03-25 23:11 ` Verma, Vishal L 2 siblings, 1 reply; 14+ messages in thread From: Rafael J. Wysocki @ 2022-03-23 19:25 UTC (permalink / raw) To: Vishal Verma Cc: linux-cxl, ACPI Devel Maling List, Jonathan Cameron, Dan Williams, Rafael J. Wysocki, Robert Moore, Bjorn Helgaas, Rafael J. Wysocki, kernel test robot On Fri, Mar 18, 2022 at 10:30 PM Vishal Verma <vishal.l.verma@intel.com> wrote: > > Add full support for negotiating _OSC as defined in the CXL 2.0 spec, as > applicable to CXL-enabled platforms. Advertise support for the CXL > features we support - 'CXL 2.0 port/device register access', 'Protocol > Error Reporting', and 'CL Native Hot Plug'. Request control for 'CXL > Memory Error Reporting'. The requests are dependent on CONFIG_* based > pre-requisites, and prior PCI enabling, similar to how the standard PCI > _OSC bits are determined. > > The CXL specification does not define any additional constraints on > the hotplug flow beyond PCIe native hotplug, so a kernel that supports > native PCIe hotplug, supports CXL hotplug. For error handling protocol > and link errors just use PCIe AER. There is nascent support for > amending AER events with CXL specific status [1], but there's > otherwise no additional OS responsibility for CXL errors beyond PCIe > AER. CXL Memory Errors behave the same as typical memory errors so > CONFIG_MEMORY_FAILURE is sufficient to indicate support to platform > firmware. > > [1]: https://lore.kernel.org/linux-cxl/164740402242.3912056.8303625392871313860.stgit@dwillia2-desk3.amr.corp.intel.com/ > > Cc: Bjorn Helgaas <bhelgaas@google.com> > Cc: "Rafael J. Wysocki" <rafael@kernel.org> > Cc: Robert Moore <robert.moore@intel.com> > Cc: Dan Williams <dan.j.williams@intel.com> > Reported-by: kernel test robot <lkp@intel.com> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> > --- > include/linux/acpi.h | 14 ++++ > include/acpi/acpi_bus.h | 6 +- > drivers/acpi/pci_root.c | 150 +++++++++++++++++++++++++++++++++++----- > 3 files changed, 149 insertions(+), 21 deletions(-) > > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > index 6274758648e3..97c2e39f10de 100644 > --- a/include/linux/acpi.h > +++ b/include/linux/acpi.h > @@ -550,10 +550,15 @@ struct acpi_osc_context { > > acpi_status acpi_run_osc(acpi_handle handle, struct acpi_osc_context *context); > > +/* Max possible _OSC capability DWORDS */ > +#define OSC_CAPABILITY_DWORDS_MAX 5 > + > /* Indexes into _OSC Capabilities Buffer (DWORDs 2 & 3 are device-specific) */ > #define OSC_QUERY_DWORD 0 /* DWORD 1 */ > #define OSC_SUPPORT_DWORD 1 /* DWORD 2 */ > #define OSC_CONTROL_DWORD 2 /* DWORD 3 */ > +#define OSC_CXL_SUPPORT_DWORD 3 /* DWORD 4 */ > +#define OSC_CXL_CONTROL_DWORD 4 /* DWORD 5 */ > > /* _OSC Capabilities DWORD 1: Query/Control and Error Returns (generic) */ > #define OSC_QUERY_ENABLE 0x00000001 /* input */ > @@ -607,6 +612,15 @@ extern u32 osc_sb_native_usb4_control; > #define OSC_PCI_EXPRESS_LTR_CONTROL 0x00000020 > #define OSC_PCI_EXPRESS_DPC_CONTROL 0x00000080 > > +/* CXL _OSC: Capabilities DWORD 4: Support Field */ > +#define OSC_CXL_1_1_PORT_REG_ACCESS_SUPPORT 0x00000001 > +#define OSC_CXL_2_0_PORT_DEV_REG_ACCESS_SUPPORT 0x00000002 > +#define OSC_CXL_PROTOCOL_ERR_REPORTING_SUPPORT 0x00000004 > +#define OSC_CXL_NATIVE_HP_SUPPORT 0x00000008 > + > +/* CXL _OSC: Capabilities DWORD 5: Control Field */ > +#define OSC_CXL_ERROR_REPORTING_CONTROL 0x00000001 > + > #define ACPI_GSB_ACCESS_ATTRIB_QUICK 0x00000002 > #define ACPI_GSB_ACCESS_ATTRIB_SEND_RCV 0x00000004 > #define ACPI_GSB_ACCESS_ATTRIB_BYTE 0x00000006 > diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h > index 768ef1584055..5776d4c1509a 100644 > --- a/include/acpi/acpi_bus.h > +++ b/include/acpi/acpi_bus.h > @@ -588,8 +588,10 @@ struct acpi_pci_root { > bool cxl_osc_disable; > 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 */ > phys_addr_t mcfg_addr; > }; > > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c > index 2d834504096b..c8c83fc37b81 100644 > --- a/drivers/acpi/pci_root.c > +++ b/drivers/acpi/pci_root.c > @@ -142,6 +142,17 @@ static struct pci_osc_bit_struct pci_osc_control_bit[] = { > { OSC_PCI_EXPRESS_DPC_CONTROL, "DPC" }, > }; > > +static struct pci_osc_bit_struct cxl_osc_support_bit[] = { > + { OSC_CXL_1_1_PORT_REG_ACCESS_SUPPORT, "CXL11PortRegAccess" }, > + { OSC_CXL_2_0_PORT_DEV_REG_ACCESS_SUPPORT, "CXL20PortDevRegAccess" }, > + { OSC_CXL_PROTOCOL_ERR_REPORTING_SUPPORT, "CXLProtocolErrorReporting" }, > + { OSC_CXL_NATIVE_HP_SUPPORT, "CXLNativeHotPlug" }, > +}; > + > +static struct pci_osc_bit_struct cxl_osc_control_bit[] = { > + { OSC_CXL_ERROR_REPORTING_CONTROL, "CXLMemErrorReporting" }, > +}; > + > static void decode_osc_bits(struct acpi_pci_root *root, char *msg, u32 word, > struct pci_osc_bit_struct *table, int size) > { > @@ -170,6 +181,18 @@ static void decode_osc_control(struct acpi_pci_root *root, char *msg, u32 word) > ARRAY_SIZE(pci_osc_control_bit)); > } > > +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 bool is_pcie(struct acpi_pci_root *root) > { > return strcmp(acpi_device_hid(root->device), "PNP0A08") == 0; > @@ -199,8 +222,21 @@ static int cap_length(struct acpi_pci_root *root) > return sizeof(u32) * 3; > } > > +static u32 acpi_osc_ctx_get_pci_control(struct acpi_osc_context *context) > +{ > + return *((u32 *)(context->ret.pointer + > + (sizeof(u32) * OSC_CONTROL_DWORD))); > +} > + > +static u32 acpi_osc_ctx_get_cxl_control(struct acpi_osc_context *context) > +{ > + return *((u32 *)(context->ret.pointer + > + (sizeof(u32) * OSC_CXL_CONTROL_DWORD))); > +} > + > static acpi_status acpi_pci_run_osc(struct acpi_pci_root *root, > - const u32 *capbuf, u32 *retval) > + const u32 *capbuf, u32 *pci_control, > + u32 *cxl_control) > { > struct acpi_osc_context context = { > .uuid_str = to_uuid(root), > @@ -212,18 +248,20 @@ static acpi_status acpi_pci_run_osc(struct acpi_pci_root *root, > > status = acpi_run_osc(root->device->handle, &context); > if (ACPI_SUCCESS(status)) { > - *retval = *((u32 *)(context.ret.pointer + 8)); > + *pci_control = acpi_osc_ctx_get_pci_control(&context); > + if (is_cxl(root)) > + *cxl_control = acpi_osc_ctx_get_cxl_control(&context); > kfree(context.ret.pointer); > } > return status; > } > > -static acpi_status acpi_pci_query_osc(struct acpi_pci_root *root, > - u32 support, > - u32 *control) > +static acpi_status acpi_pci_query_osc(struct acpi_pci_root *root, u32 support, > + u32 *control, u32 cxl_support, > + u32 *cxl_control) > { > acpi_status status; > - u32 result, capbuf[6]; > + u32 pci_result, cxl_result, capbuf[OSC_CAPABILITY_DWORDS_MAX]; > > support |= root->osc_support_set; > > @@ -231,11 +269,21 @@ static acpi_status acpi_pci_query_osc(struct acpi_pci_root *root, > capbuf[OSC_SUPPORT_DWORD] = support; > capbuf[OSC_CONTROL_DWORD] = *control | root->osc_control_set; > > + if (is_cxl(root)) { > + cxl_support |= root->cxl_osc_support_set; > + capbuf[OSC_CXL_SUPPORT_DWORD] = cxl_support; > + capbuf[OSC_CXL_CONTROL_DWORD] = *cxl_control | root->cxl_osc_control_set; > + } > + > retry: > - status = acpi_pci_run_osc(root, capbuf, &result); > + status = acpi_pci_run_osc(root, capbuf, &pci_result, &cxl_result); > if (ACPI_SUCCESS(status)) { > root->osc_support_set = support; > - *control = result; > + *control = pci_result; > + if (is_cxl(root)) { > + root->cxl_osc_support_set = cxl_support; > + *cxl_control = cxl_result; > + } > } else if (is_cxl(root)) { > /* > * CXL _OSC is optional on CXL 1.1 hosts. Fall back to PCIe _OSC > @@ -358,6 +406,8 @@ EXPORT_SYMBOL_GPL(acpi_get_pci_dev); > * @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. > * @support: _OSC supported capability. > + * @cxl_mask: Mask of CXL _OSC control bits, place to store control mask. > + * @cxl_support: CXL _OSC supported capability. > * > * Run _OSC query for @mask and if that is successful, compare the returned > * mask of control bits with @req. If all of the @req bits are set in the > @@ -368,12 +418,14 @@ EXPORT_SYMBOL_GPL(acpi_get_pci_dev); > * _OSC bits the BIOS has granted control of, but its contents are meaningless > * on failure. > **/ > -static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 support) > +static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, > + u32 support, u32 *cxl_mask, > + u32 cxl_support) > { > u32 req = OSC_PCI_EXPRESS_CAPABILITY_CONTROL; > struct acpi_pci_root *root; > acpi_status status; > - u32 ctrl, capbuf[6]; > + u32 ctrl, cxl_ctrl = 0, capbuf[OSC_CAPABILITY_DWORDS_MAX]; > > if (!mask) > return AE_BAD_PARAMETER; > @@ -385,20 +437,35 @@ static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 s > ctrl = *mask; > *mask |= root->osc_control_set; > > + if (is_cxl(root)) { > + cxl_ctrl = *cxl_mask; > + *mask |= root->osc_control_set; Is this correct? It appears redundant at least, because the same update has already happened above. > + } > + > /* Need to check the available controls bits before requesting them. */ > do { > - status = acpi_pci_query_osc(root, support, mask); > + status = acpi_pci_query_osc(root, support, mask, cxl_support, > + cxl_mask); > if (ACPI_FAILURE(status)) > return status; > - if (ctrl == *mask) > - break; > - decode_osc_control(root, "platform does not support", > - ctrl & ~(*mask)); > + if (is_cxl(root)) { > + if ((ctrl == *mask) && (cxl_ctrl == *cxl_mask)) > + break; > + decode_cxl_osc_control(root, "platform does not support", > + cxl_ctrl & ~(*cxl_mask)); > + } else { > + if (ctrl == *mask) > + break; > + decode_osc_control(root, "platform does not support", > + ctrl & ~(*mask)); > + } > ctrl = *mask; > - } while (*mask); > + cxl_ctrl = *cxl_mask; > + } while (*mask || *cxl_mask); > > /* No need to request _OSC if the control was already granted. */ > - if ((root->osc_control_set & ctrl) == ctrl) > + if (((root->osc_control_set & ctrl) == ctrl) && > + ((root->cxl_osc_control_set & cxl_ctrl) == cxl_ctrl)) > return AE_OK; > > if ((ctrl & req) != req) { > @@ -410,11 +477,17 @@ static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 s > capbuf[OSC_QUERY_DWORD] = 0; > capbuf[OSC_SUPPORT_DWORD] = root->osc_support_set; > capbuf[OSC_CONTROL_DWORD] = ctrl; > - status = acpi_pci_run_osc(root, capbuf, mask); > + if (is_cxl(root)) { > + capbuf[OSC_CXL_SUPPORT_DWORD] = root->cxl_osc_support_set; > + capbuf[OSC_CXL_CONTROL_DWORD] = cxl_ctrl; > + } > + > + status = acpi_pci_run_osc(root, capbuf, mask, cxl_mask); > if (ACPI_FAILURE(status)) > return status; > > root->osc_control_set = *mask; > + root->cxl_osc_control_set = *cxl_mask; > return AE_OK; > } > > @@ -440,6 +513,19 @@ static u32 calculate_support(void) > return support; > } > > +static u32 calculate_cxl_support(void) > +{ > + u32 support; > + > + support = OSC_CXL_2_0_PORT_DEV_REG_ACCESS_SUPPORT; > + if (pci_aer_available()) > + support |= OSC_CXL_PROTOCOL_ERR_REPORTING_SUPPORT; > + if (IS_ENABLED(CONFIG_HOTPLUG_PCI_PCIE)) > + support |= OSC_CXL_NATIVE_HP_SUPPORT; > + > + return support; > +} > + > static u32 calculate_control(void) > { > u32 control; > @@ -471,6 +557,16 @@ static u32 calculate_control(void) > return control; > } > > +static u32 calculate_cxl_control(void) > +{ > + u32 control = 0; > + > + if (IS_ENABLED(CONFIG_MEMORY_FAILURE)) > + control |= OSC_CXL_ERROR_REPORTING_CONTROL; > + > + return control; > +} > + > static bool os_control_query_checks(struct acpi_pci_root *root, u32 support) > { > struct acpi_device *device = root->device; > @@ -492,6 +588,7 @@ static bool os_control_query_checks(struct acpi_pci_root *root, u32 support) > static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm) > { > u32 support, control = 0, requested = 0; > + u32 cxl_support = 0, cxl_control = 0, cxl_requested = 0; > acpi_status status; > struct acpi_device *device = root->device; > acpi_handle handle = device->handle; > @@ -515,10 +612,20 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm) > if (os_control_query_checks(root, support)) > requested = control = calculate_control(); > > - status = acpi_pci_osc_control_set(handle, &control, support); > + if (is_cxl(root)) { > + cxl_support = calculate_cxl_support(); > + decode_cxl_osc_support(root, "OS supports", cxl_support); > + cxl_requested = cxl_control = calculate_cxl_control(); > + } > + > + status = acpi_pci_osc_control_set(handle, &control, support, > + &cxl_control, cxl_support); > if (ACPI_SUCCESS(status)) { > if (control) > decode_osc_control(root, "OS now controls", control); > + if (cxl_control) > + decode_cxl_osc_control(root, "OS now controls", > + cxl_control); > > if (acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_ASPM) { > /* > @@ -547,6 +654,11 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm) > decode_osc_control(root, "OS requested", requested); > decode_osc_control(root, "platform willing to grant", control); > } > + if (cxl_control) { > + decode_cxl_osc_control(root, "OS requested", cxl_requested); > + decode_cxl_osc_control(root, "platform willing to grant", > + cxl_control); > + } > > dev_info(&device->dev, "_OSC: platform retains control of PCIe features (%s)\n", > acpi_format_exception(status)); > -- > 2.35.1 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] acpi/pci_root: negotiate CXL _OSC 2022-03-23 19:25 ` Rafael J. Wysocki @ 2022-03-25 23:11 ` Verma, Vishal L 0 siblings, 0 replies; 14+ messages in thread From: Verma, Vishal L @ 2022-03-25 23:11 UTC (permalink / raw) To: rafael Cc: lkp, linux-cxl, Jonathan.Cameron, Williams, Dan J, linux-acpi, Wysocki, Rafael J, Moore, Robert, bhelgaas On Wed, 2022-03-23 at 20:25 +0100, Rafael J. Wysocki wrote: > On Fri, Mar 18, 2022 at 10:30 PM Vishal Verma <vishal.l.verma@intel.com> wrote: > > > > @@ -385,20 +437,35 @@ static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 s > > ctrl = *mask; > > *mask |= root->osc_control_set; > > > > + if (is_cxl(root)) { > > + cxl_ctrl = *cxl_mask; > > + *mask |= root->osc_control_set; > > Is this correct? > > It appears redundant at least, because the same update has already > happened above. Oh good spot - this was a copy-paste error, it needs to be: *cxl_mask |= root->cxl_osc_control_set; I've fixed this up and all other comments too for v2. Thanks for the review! > > > + } > > + > > /* Need to check the available controls bits before requesting them. */ > > do { > > - status = acpi_pci_query_osc(root, support, mask); > > + status = acpi_pci_query_osc(root, support, mask, cxl_support, > > + cxl_mask); > > if (ACPI_FAILURE(status)) > > return status; > > - if (ctrl == *mask) > > - break; > > - decode_osc_control(root, "platform does not support", > > - ctrl & ~(*mask)); > > + if (is_cxl(root)) { > > + if ((ctrl == *mask) && (cxl_ctrl == *cxl_mask)) > > + break; > > + decode_cxl_osc_control(root, "platform does not support", > > + cxl_ctrl & ~(*cxl_mask)); > > + } else { > > + if (ctrl == *mask) > > + break; > > + decode_osc_control(root, "platform does not support", > > + ctrl & ~(*mask)); > > + } > > ctrl = *mask; > > - } while (*mask); > > + cxl_ctrl = *cxl_mask; > > + } while (*mask || *cxl_mask); > > > > /* No need to request _OSC if the control was already granted. */ > > - if ((root->osc_control_set & ctrl) == ctrl) > > + if (((root->osc_control_set & ctrl) == ctrl) && > > + ((root->cxl_osc_control_set & cxl_ctrl) == cxl_ctrl)) > > return AE_OK; > > > > if ((ctrl & req) != req) { > > @@ -410,11 +477,17 @@ static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 s > > capbuf[OSC_QUERY_DWORD] = 0; > > capbuf[OSC_SUPPORT_DWORD] = root->osc_support_set; > > capbuf[OSC_CONTROL_DWORD] = ctrl; > > - status = acpi_pci_run_osc(root, capbuf, mask); > > + if (is_cxl(root)) { > > + capbuf[OSC_CXL_SUPPORT_DWORD] = root->cxl_osc_support_set; > > + capbuf[OSC_CXL_CONTROL_DWORD] = cxl_ctrl; > > + } > > + > > + status = acpi_pci_run_osc(root, capbuf, mask, cxl_mask); > > if (ACPI_FAILURE(status)) > > return status; > > > > root->osc_control_set = *mask; > > + root->cxl_osc_control_set = *cxl_mask; > > return AE_OK; > > } > > > > @@ -440,6 +513,19 @@ static u32 calculate_support(void) > > return support; > > } > > > > +static u32 calculate_cxl_support(void) > > +{ > > + u32 support; > > + > > + support = OSC_CXL_2_0_PORT_DEV_REG_ACCESS_SUPPORT; > > + if (pci_aer_available()) > > + support |= OSC_CXL_PROTOCOL_ERR_REPORTING_SUPPORT; > > + if (IS_ENABLED(CONFIG_HOTPLUG_PCI_PCIE)) > > + support |= OSC_CXL_NATIVE_HP_SUPPORT; > > + > > + return support; > > +} > > + > > static u32 calculate_control(void) > > { > > u32 control; > > @@ -471,6 +557,16 @@ static u32 calculate_control(void) > > return control; > > } > > > > +static u32 calculate_cxl_control(void) > > +{ > > + u32 control = 0; > > + > > + if (IS_ENABLED(CONFIG_MEMORY_FAILURE)) > > + control |= OSC_CXL_ERROR_REPORTING_CONTROL; > > + > > + return control; > > +} > > + > > static bool os_control_query_checks(struct acpi_pci_root *root, u32 support) > > { > > struct acpi_device *device = root->device; > > @@ -492,6 +588,7 @@ static bool os_control_query_checks(struct acpi_pci_root *root, u32 support) > > static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm) > > { > > u32 support, control = 0, requested = 0; > > + u32 cxl_support = 0, cxl_control = 0, cxl_requested = 0; > > acpi_status status; > > struct acpi_device *device = root->device; > > acpi_handle handle = device->handle; > > @@ -515,10 +612,20 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm) > > if (os_control_query_checks(root, support)) > > requested = control = calculate_control(); > > > > - status = acpi_pci_osc_control_set(handle, &control, support); > > + if (is_cxl(root)) { > > + cxl_support = calculate_cxl_support(); > > + decode_cxl_osc_support(root, "OS supports", cxl_support); > > + cxl_requested = cxl_control = calculate_cxl_control(); > > + } > > + > > + status = acpi_pci_osc_control_set(handle, &control, support, > > + &cxl_control, cxl_support); > > if (ACPI_SUCCESS(status)) { > > if (control) > > decode_osc_control(root, "OS now controls", control); > > + if (cxl_control) > > + decode_cxl_osc_control(root, "OS now controls", > > + cxl_control); > > > > if (acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_ASPM) { > > /* > > @@ -547,6 +654,11 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm) > > decode_osc_control(root, "OS requested", requested); > > decode_osc_control(root, "platform willing to grant", control); > > } > > + if (cxl_control) { > > + decode_cxl_osc_control(root, "OS requested", cxl_requested); > > + decode_cxl_osc_control(root, "platform willing to grant", > > + cxl_control); > > + } > > > > dev_info(&device->dev, "_OSC: platform retains control of PCIe features (%s)\n", > > acpi_format_exception(status)); > > -- > > 2.35.1 > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/2] acpi: add support for CXL _OSC 2022-03-18 21:30 [PATCH 0/2] acpi: add support for CXL _OSC Vishal Verma 2022-03-18 21:30 ` [PATCH 1/2] PCI/ACPI: Use CXL _OSC instead of PCIe _OSC Vishal Verma 2022-03-18 21:30 ` [PATCH 2/2] acpi/pci_root: negotiate CXL _OSC Vishal Verma @ 2022-03-18 21:34 ` Verma, Vishal L 2 siblings, 0 replies; 14+ messages in thread From: Verma, Vishal L @ 2022-03-18 21:34 UTC (permalink / raw) To: Wysocki, Rafael J Cc: Williams, Dan J, Moore, Robert, Jonathan.Cameron, linux-cxl, bhelgaas, linux-acpi On Fri, 2022-03-18 at 15:30 -0600, Vishal Verma wrote: > Changes since v1[1]: > - Update changelogs for both patches (Dan) > - Fix support/control calculation to be based off CONFIG_MEMORY_FAILURE > (Dan) > - Use defines instead of magic numbers in a few places in patch 2 > (Jonathan) > - Fix 'capbuf' array to be the correct 5 elements. ACPI previously had > '6' where only 3 were needed. With CXL capabilities, now, 5 are > needed. (Jonathan). > - Fix a couple of uninitialized variable warnings reported by 0day/lkp. > - Drop 'RFC' annotation for the set > > Add support for using the CXL definition of _OSC where applicable, and > negotiating CXL specific support and control bits. > > Patch 1 adds the new CXL _OSC UUID, and uses it instead of the PCI UUID > when a root port is CXL enabled. It provides a fallback method for > CXL-1.1 platforms that may not implement the CXL-2.0 _OSC. > > Patch 2 performs negotiation for the CXL specific _OSC support and > control bits. > > I've tested these against a custom qemu[2], which adds the CXL _OSC (in > addition to other CXL support). Specifically, _OSC support is added > here[3]. > > [1]: https://lore.kernel.org/linux-cxl/146514b2e5237a3c027239a75ace69e72671d4c8.camel@intel.com/T/#t > [2]: https://gitlab.com/jic23/qemu/-/tree/cxl-v7-draft-2-for-test > [3]: https://gitlab.com/jic23/qemu/-/commit/31c85054b84645dfbd9e9bb14aa35286141c14cf Logistical question - Rafael, do you expect this to go through ACPI, or should Dan pick it up to go through the CXL tree? > > > Dan Williams (1): > PCI/ACPI: Use CXL _OSC instead of PCIe _OSC > > Vishal Verma (1): > acpi/pci_root: negotiate CXL _OSC > > include/linux/acpi.h | 14 +++ > include/acpi/acpi_bus.h | 7 +- > drivers/acpi/pci_root.c | 204 ++++++++++++++++++++++++++++++++++------ > 3 files changed, 193 insertions(+), 32 deletions(-) > > > base-commit: 74be98774dfbc5b8b795db726bd772e735d2edd4 ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2022-03-25 23:11 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-03-18 21:30 [PATCH 0/2] acpi: add support for CXL _OSC Vishal Verma 2022-03-18 21:30 ` [PATCH 1/2] PCI/ACPI: Use CXL _OSC instead of PCIe _OSC Vishal Verma 2022-03-22 18:01 ` Rafael J. Wysocki 2022-03-22 18:08 ` Dan Williams 2022-03-23 19:23 ` Rafael J. Wysocki 2022-03-23 20:39 ` Dan Williams 2022-03-18 21:30 ` [PATCH 2/2] acpi/pci_root: negotiate CXL _OSC Vishal Verma 2022-03-22 18:22 ` Rafael J. Wysocki 2022-03-23 0:47 ` Davidlohr Bueso 2022-03-23 18:29 ` Verma, Vishal L 2022-03-23 19:02 ` Dan Williams 2022-03-23 19:25 ` Rafael J. Wysocki 2022-03-25 23:11 ` Verma, Vishal L 2022-03-18 21:34 ` [PATCH 0/2] acpi: add support for " Verma, Vishal L
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).