| Message ID | 168193556660.1178687.15477509915255912089.stgit@djiang5-mobl3 |
|---|---|
| Headers | show |
| Series | cxl: Add support for QTG ID retrieval for CXL subsystem | expand |
On Wed, 19 Apr 2023 13:21:07 -0700 Dave Jiang <dave.jiang@intel.com> wrote: > Export the QoS Throttling Group ID from the CXL Fixed Memory Window > Structure (CFMWS) under the root decoder sysfs attributes. > CXL rev3.0 9.17.1.3 CXL Fixed Memory Window Structure (CFMWS) > > cxl cli will use this QTG ID to match with the _DSM retrieved QTG ID for a > hot-plugged CXL memory device DPA memory range to make sure that the DPA range > is under the right CFMWS window. > > Reviewed-by: Ira Weiny <ira.weiny@intel.com> > Signed-off-by: Dave Jiang <dave.jiang@intel.com> > Bikeshedding alert: Why not just call it qtg? What does the _id add? I don't really care either way... LGTM Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> One (more) completely trivial comment inline. Jonathan > --- > v4: > - Change kernel version for documentation to v6.5 > v2: > - Add explanation commit header (Jonathan) > --- > Documentation/ABI/testing/sysfs-bus-cxl | 9 +++++++++ > drivers/cxl/acpi.c | 3 +++ > drivers/cxl/core/port.c | 14 ++++++++++++++ > drivers/cxl/cxl.h | 3 +++ > 4 files changed, 29 insertions(+) > > diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl > index 3acf2f17a73f..bd2b59784979 100644 > --- a/Documentation/ABI/testing/sysfs-bus-cxl > +++ b/Documentation/ABI/testing/sysfs-bus-cxl > @@ -309,6 +309,15 @@ Description: > (WO) Write a string in the form 'regionZ' to delete that region, > provided it is currently idle / not bound to a driver. > > +What: /sys/bus/cxl/devices/decoderX.Y/qtg_id > +Date: Jan, 2023 > +KernelVersion: v6.5 > +Contact: linux-cxl@vger.kernel.org > +Description: > + (RO) Shows the QoS Throttling Group ID. The QTG ID for a root > + decoder comes from the CFMWS structure of the CEDT. A value of > + -1 indicates that no QTG ID was retrieved. The QTG ID is used as > + guidance to match against the QTG ID of a hot-plugged device. > > What: /sys/bus/cxl/devices/regionZ/uuid > Date: May, 2022 > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c > index 7e1765b09e04..abc24137c291 100644 > --- a/drivers/cxl/acpi.c > +++ b/drivers/cxl/acpi.c > @@ -289,6 +289,9 @@ static int cxl_parse_cfmws(union acpi_subtable_headers *header, void *arg, > } > } > } > + > + cxld->qtg_id = cfmws->qtg_id; > + > rc = cxl_decoder_add(cxld, target_map); > err_xormap: > if (rc) > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c > index 4d1f9c5b5029..024d4178f557 100644 > --- a/drivers/cxl/core/port.c > +++ b/drivers/cxl/core/port.c > @@ -276,6 +276,16 @@ static ssize_t interleave_ways_show(struct device *dev, > > static DEVICE_ATTR_RO(interleave_ways); > > +static ssize_t qtg_id_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct cxl_decoder *cxld = to_cxl_decoder(dev); > + > + return sysfs_emit(buf, "%d\n", cxld->qtg_id); > +} > + No blank line here would be more consistent with local style (based on a really quick look). > +static DEVICE_ATTR_RO(qtg_id); > + > static struct attribute *cxl_decoder_base_attrs[] = { > &dev_attr_start.attr, > &dev_attr_size.attr, > @@ -295,6 +305,7 @@ static struct attribute *cxl_decoder_root_attrs[] = { > &dev_attr_cap_type2.attr, > &dev_attr_cap_type3.attr, > &dev_attr_target_list.attr, > + &dev_attr_qtg_id.attr, > SET_CXL_REGION_ATTR(create_pmem_region) > SET_CXL_REGION_ATTR(create_ram_region) > SET_CXL_REGION_ATTR(delete_region) > @@ -1625,6 +1636,7 @@ struct cxl_root_decoder *cxl_root_decoder_alloc(struct cxl_port *port, > } > > atomic_set(&cxlrd->region_id, rc); > + cxld->qtg_id = CXL_QTG_ID_INVALID; > return cxlrd; > } > EXPORT_SYMBOL_NS_GPL(cxl_root_decoder_alloc, CXL); > @@ -1662,6 +1674,7 @@ struct cxl_switch_decoder *cxl_switch_decoder_alloc(struct cxl_port *port, > > cxld = &cxlsd->cxld; > cxld->dev.type = &cxl_decoder_switch_type; > + cxld->qtg_id = CXL_QTG_ID_INVALID; > return cxlsd; > } > EXPORT_SYMBOL_NS_GPL(cxl_switch_decoder_alloc, CXL); > @@ -1694,6 +1707,7 @@ struct cxl_endpoint_decoder *cxl_endpoint_decoder_alloc(struct cxl_port *port) > } > > cxld->dev.type = &cxl_decoder_endpoint_type; > + cxld->qtg_id = CXL_QTG_ID_INVALID; > return cxled; > } > EXPORT_SYMBOL_NS_GPL(cxl_endpoint_decoder_alloc, CXL); > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > index 044a92d9813e..278ab6952332 100644 > --- a/drivers/cxl/cxl.h > +++ b/drivers/cxl/cxl.h > @@ -300,6 +300,7 @@ enum cxl_decoder_type { > */ > #define CXL_DECODER_MAX_INTERLEAVE 16 > > +#define CXL_QTG_ID_INVALID -1 > > /** > * struct cxl_decoder - Common CXL HDM Decoder Attributes > @@ -311,6 +312,7 @@ enum cxl_decoder_type { > * @target_type: accelerator vs expander (type2 vs type3) selector > * @region: currently assigned region for this decoder > * @flags: memory type capabilities and locking > + * @qtg_id: QoS Throttling Group ID > * @commit: device/decoder-type specific callback to commit settings to hw > * @reset: device/decoder-type specific callback to reset hw settings > */ > @@ -323,6 +325,7 @@ struct cxl_decoder { > enum cxl_decoder_type target_type; > struct cxl_region *region; > unsigned long flags; > + int qtg_id; > int (*commit)(struct cxl_decoder *cxld); > int (*reset)(struct cxl_decoder *cxld); > }; > >
On Wed, 19 Apr 2023 13:21:25 -0700 Dave Jiang <dave.jiang@intel.com> wrote: > Add helper functions to parse the CDAT table and provide a callback to > parse the sub-table. Helpers are provided for DSMAS and DSLBIS sub-table > parsing. The code is patterned after the ACPI table parsing helpers. > > Signed-off-by: Dave Jiang <dave.jiang@intel.com> > A few minor things inline. More than possible you addressed them in earlier versions though. Jonathan > --- > v2: > - Use local headers to handle LE instead of ACPI header > - Reduce complexity of parser function. (Jonathan) > - Directly access header type. (Jonathan) > - Simplify header ptr math. (Jonathan) > - Move parsed counter to the correct location. (Jonathan) > - Add LE to host conversion for entry length > --- > drivers/cxl/core/Makefile | 1 > drivers/cxl/core/cdat.c | 100 +++++++++++++++++++++++++++++++++++++++++++++ > drivers/cxl/cxlpci.h | 29 +++++++++++++ > 3 files changed, 130 insertions(+) > create mode 100644 drivers/cxl/core/cdat.c > > diff --git a/drivers/cxl/core/Makefile b/drivers/cxl/core/Makefile > index ca4ae31d8f57..867a8014b462 100644 > --- a/drivers/cxl/core/Makefile > +++ b/drivers/cxl/core/Makefile > @@ -12,5 +12,6 @@ cxl_core-y += memdev.o > cxl_core-y += mbox.o > cxl_core-y += pci.o > cxl_core-y += hdm.o > +cxl_core-y += cdat.o > cxl_core-$(CONFIG_TRACING) += trace.o > cxl_core-$(CONFIG_CXL_REGION) += region.o > diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c > new file mode 100644 > index 000000000000..210f4499bddb > --- /dev/null > +++ b/drivers/cxl/core/cdat.c > @@ -0,0 +1,100 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* Copyright(c) 2023 Intel Corporation. All rights reserved. */ > +#include "cxlpci.h" > +#include "cxl.h" > + > +static bool has_handler(struct cdat_subtable_proc *proc) Even though they are static, I'd add a cxl_ or cdat_ prefix to these to make it clear they are local. > +{ > + return proc->handler; > +} > + > +static int call_handler(struct cdat_subtable_proc *proc, > + struct cdat_subtable_entry *ent) > +{ > + if (has_handler(proc)) Do we need to check this again? It's checked in the parse_entries code well before this point. Also, if moving to checking it once, then is it worth the little wrapper functions? > + return proc->handler(ent->hdr, proc->arg); > + return -EINVAL; > +} > + > +static bool cdat_is_subtable_match(struct cdat_subtable_entry *ent) > +{ > + return ent->hdr->type == ent->type; > +} > + > +static int cdat_table_parse_entries(enum cdat_type type, > + struct cdat_header *table_header, > + struct cdat_subtable_proc *proc) > +{ > + unsigned long table_end, entry_len; > + struct cdat_subtable_entry entry; > + int count = 0; > + int rc; > + > + if (!has_handler(proc)) > + return -EINVAL; > + > + table_end = (unsigned long)table_header + table_header->length; > + > + if (type >= CDAT_TYPE_RESERVED) > + return -EINVAL; > + > + entry.type = type; > + entry.hdr = (struct cdat_entry_header *)(table_header + 1); > + > + while ((unsigned long)entry.hdr < table_end) { > + entry_len = le16_to_cpu(entry.hdr->length); > + > + if ((unsigned long)entry.hdr + entry_len > table_end) > + return -EINVAL; > + > + if (entry_len == 0) > + return -EINVAL; > + > + if (cdat_is_subtable_match(&entry)) { > + rc = call_handler(proc, &entry); > + if (rc) > + return rc; > + count++; > + } > + > + entry.hdr = (struct cdat_entry_header *)((unsigned long)entry.hdr + entry_len); > + } > + > + return count; > +} ... > +int cdat_table_parse_sslbis(struct cdat_header *table, > + cdat_tbl_entry_handler handler, void *arg) Feels like these ones should take a typed arg. Sure you'll loose that again to use the generic handling code, but at this level we can do it I think. > +{ > + struct cdat_subtable_proc proc = { > + .handler = handler, > + .arg = arg, > + }; > + > + return cdat_table_parse_entries(CDAT_TYPE_SSLBIS, table, &proc); > +}
On Wed, 19 Apr 2023 13:21:31 -0700 Dave Jiang <dave.jiang@intel.com> wrote: > Provide a callback function to the CDAT parser in order to parse the Device > Scoped Memory Affinity Structure (DSMAS). Each DSMAS structure contains the > DPA range and its associated attributes in each entry. See the CDAT > specification for details. > > Coherent Device Attribute Table 1.03 2.1 Device Scoped memory Affinity > Structure (DSMAS) I'm not sure what purpose of this is. If it's just detecting problems with the entry because we aren't interested in the content yet, then fine but good to make that clear in patch intro. Maybe I'm missing something! Thanks, Jonathan > > Signed-off-by: Dave Jiang <dave.jiang@intel.com> > > --- > v3: > - Add spec section number. (Alison) > - Remove cast from void *. (Alison) > - Refactor cxl_port_probe() block. (Alison) > - Move CDAT parse to cxl_endpoint_port_probe() > > v2: > - Add DSMAS table size check. (Lukas) > - Use local DSMAS header for LE handling. > - Remove dsmas lock. (Jonathan) > - Fix handle size (Jonathan) > - Add LE to host conversion for DSMAS address and length. > - Make dsmas_list local > diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c > index 615e0ef6b440..3022bdd52439 100644 > --- a/drivers/cxl/port.c > +++ b/drivers/cxl/port.c > @@ -57,6 +57,16 @@ static int discover_region(struct device *dev, void *root) > return 0; > } > static int cxl_switch_port_probe(struct cxl_port *port) > { > struct cxl_hdm *cxlhdm; > @@ -125,6 +135,18 @@ static int cxl_endpoint_port_probe(struct cxl_port *port) > device_for_each_child(&port->dev, root, discover_region); > put_device(&root->dev); > > + if (port->cdat.table) { > + LIST_HEAD(dsmas_list); > + > + rc = cdat_table_parse_dsmas(port->cdat.table, > + cxl_dsmas_parse_entry, > + (void *)&dsmas_list); > + if (rc < 0) > + dev_warn(&port->dev, "Failed to parse DSMAS: %d\n", rc); > + > + dsmas_list_destroy(&dsmas_list); I'm a little confused here. What's the point? Parse them then throw the info away? Maybe a comment if all we are trying to do is warn about CDAT problems. > + } > + > return 0; > } > > >
On Thu, 20 Apr 2023 12:33:50 +0100 Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote: > On Wed, 19 Apr 2023 13:21:31 -0700 > Dave Jiang <dave.jiang@intel.com> wrote: > > > Provide a callback function to the CDAT parser in order to parse the Device > > Scoped Memory Affinity Structure (DSMAS). Each DSMAS structure contains the > > DPA range and its associated attributes in each entry. See the CDAT > > specification for details. > > > > Coherent Device Attribute Table 1.03 2.1 Device Scoped memory Affinity > > Structure (DSMAS) > > I'm not sure what purpose of this is. If it's just detecting problems > with the entry because we aren't interested in the content yet, then fine > but good to make that clear in patch intro. > > Maybe I'm missing something! > Ah. Got to next patch. Perhaps a forwards reference to that will avoid anyone else wondering what is going on here! > Thanks, > > Jonathan > > > > > Signed-off-by: Dave Jiang <dave.jiang@intel.com> > > > > --- > > v3: > > - Add spec section number. (Alison) > > - Remove cast from void *. (Alison) > > - Refactor cxl_port_probe() block. (Alison) > > - Move CDAT parse to cxl_endpoint_port_probe() > > > > v2: > > - Add DSMAS table size check. (Lukas) > > - Use local DSMAS header for LE handling. > > - Remove dsmas lock. (Jonathan) > > - Fix handle size (Jonathan) > > - Add LE to host conversion for DSMAS address and length. > > - Make dsmas_list local > > > > diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c > > index 615e0ef6b440..3022bdd52439 100644 > > --- a/drivers/cxl/port.c > > +++ b/drivers/cxl/port.c > > @@ -57,6 +57,16 @@ static int discover_region(struct device *dev, void *root) > > return 0; > > } > > > static int cxl_switch_port_probe(struct cxl_port *port) > > { > > struct cxl_hdm *cxlhdm; > > @@ -125,6 +135,18 @@ static int cxl_endpoint_port_probe(struct cxl_port *port) > > device_for_each_child(&port->dev, root, discover_region); > > put_device(&root->dev); > > > > + if (port->cdat.table) { > > + LIST_HEAD(dsmas_list); > > + > > + rc = cdat_table_parse_dsmas(port->cdat.table, > > + cxl_dsmas_parse_entry, > > + (void *)&dsmas_list); > > + if (rc < 0) > > + dev_warn(&port->dev, "Failed to parse DSMAS: %d\n", rc); > > + > > + dsmas_list_destroy(&dsmas_list); > > I'm a little confused here. What's the point? Parse them then throw the info away? > Maybe a comment if all we are trying to do is warn about CDAT problems. > > > > + } > > + > > return 0; > > } > > > > > > >
On Wed, 19 Apr 2023 13:21:43 -0700 Dave Jiang <dave.jiang@intel.com> wrote: > Provide a callback to parse the Switched Scoped Latency and Bandwidth > Information Structure (DSLBIS) in the CDAT structures. The SSLBIS > contains the bandwidth and latency information that's tied to the > CLX switch that the data table has been read from. The extracted CLX? :) > values are indexed by the downstream port id. It is possible > the downstream port id is 0xffff which is a wildcard value for any > port id. > > Coherent Device Attribute Table 1.03 2.1 Switched Scoped Latency > and Bandwidth Information Structure (DSLBIS) > > Signed-off-by: Dave Jiang <dave.jiang@intel.com> > LGTM subject to same comment on typing arg. Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
On Wed, 19 Apr 2023 13:21:55 -0700 Dave Jiang <dave.jiang@intel.com> wrote: > Provide a helper to find the ACPI0017 device in order to issue the _DSM. > The helper will take the 'struct device' from a cxl_port and iterate until > the root device is reached. The ACPI handle will be returned from the root > device. > > Signed-off-by: Dave Jiang <dave.jiang@intel.com> Question inline. If the answer is no then this looks fine to me. Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > +/** > + * cxl_acpi_get_rootdev_handle - get the ACPI handle of the CXL root device > + * @dev: 'struct device' to start searching from. Should be from cxl_port->dev. > + * > + * Return: acpi_handle on success, errptr of errno on error. > + * > + * Looks for the ACPI0017 device and return the ACPI handle > + **/ Could we implement this in terms of find_cxl_root()? I think that will end up giving you the same device though I haven't tested it. > +acpi_handle cxl_acpi_get_rootdev_handle(struct device *dev) > +{ > + struct device *itr = dev; > + struct device *root_dev; > + acpi_handle handle; > + > + if (!dev) > + return ERR_PTR(-EINVAL); > + > + while (itr->parent) { > + root_dev = itr; > + itr = itr->parent; > + } > + > + if (!dev_is_platform(root_dev)) > + return ERR_PTR(-ENODEV); > + > + handle = ACPI_HANDLE(root_dev); > + if (!handle) > + return ERR_PTR(-ENODEV); > + > + return handle; > +} > +EXPORT_SYMBOL_NS_GPL(cxl_acpi_get_rootdev_handle, CXL);
On 4/20/23 1:51 AM, Jonathan Cameron wrote: > On Wed, 19 Apr 2023 13:21:07 -0700 > Dave Jiang <dave.jiang@intel.com> wrote: > >> Export the QoS Throttling Group ID from the CXL Fixed Memory Window >> Structure (CFMWS) under the root decoder sysfs attributes. >> CXL rev3.0 9.17.1.3 CXL Fixed Memory Window Structure (CFMWS) >> >> cxl cli will use this QTG ID to match with the _DSM retrieved QTG ID for a >> hot-plugged CXL memory device DPA memory range to make sure that the DPA range >> is under the right CFMWS window. >> >> Reviewed-by: Ira Weiny <ira.weiny@intel.com> >> Signed-off-by: Dave Jiang <dave.jiang@intel.com> >> > > Bikeshedding alert: > > Why not just call it qtg? What does the _id add? > > I don't really care either way... > > LGTM > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > One (more) completely trivial comment inline. > > Jonathan > >> --- >> v4: >> - Change kernel version for documentation to v6.5 >> v2: >> - Add explanation commit header (Jonathan) >> --- >> Documentation/ABI/testing/sysfs-bus-cxl | 9 +++++++++ >> drivers/cxl/acpi.c | 3 +++ >> drivers/cxl/core/port.c | 14 ++++++++++++++ >> drivers/cxl/cxl.h | 3 +++ >> 4 files changed, 29 insertions(+) >> >> diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl >> index 3acf2f17a73f..bd2b59784979 100644 >> --- a/Documentation/ABI/testing/sysfs-bus-cxl >> +++ b/Documentation/ABI/testing/sysfs-bus-cxl >> @@ -309,6 +309,15 @@ Description: >> (WO) Write a string in the form 'regionZ' to delete that region, >> provided it is currently idle / not bound to a driver. >> >> +What: /sys/bus/cxl/devices/decoderX.Y/qtg_id >> +Date: Jan, 2023 >> +KernelVersion: v6.5 >> +Contact: linux-cxl@vger.kernel.org >> +Description: >> + (RO) Shows the QoS Throttling Group ID. The QTG ID for a root >> + decoder comes from the CFMWS structure of the CEDT. A value of >> + -1 indicates that no QTG ID was retrieved. The QTG ID is used as >> + guidance to match against the QTG ID of a hot-plugged device. >> >> What: /sys/bus/cxl/devices/regionZ/uuid >> Date: May, 2022 >> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c >> index 7e1765b09e04..abc24137c291 100644 >> --- a/drivers/cxl/acpi.c >> +++ b/drivers/cxl/acpi.c >> @@ -289,6 +289,9 @@ static int cxl_parse_cfmws(union acpi_subtable_headers *header, void *arg, >> } >> } >> } >> + >> + cxld->qtg_id = cfmws->qtg_id; >> + >> rc = cxl_decoder_add(cxld, target_map); >> err_xormap: >> if (rc) >> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c >> index 4d1f9c5b5029..024d4178f557 100644 >> --- a/drivers/cxl/core/port.c >> +++ b/drivers/cxl/core/port.c >> @@ -276,6 +276,16 @@ static ssize_t interleave_ways_show(struct device *dev, >> >> static DEVICE_ATTR_RO(interleave_ways); >> >> +static ssize_t qtg_id_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct cxl_decoder *cxld = to_cxl_decoder(dev); >> + >> + return sysfs_emit(buf, "%d\n", cxld->qtg_id); >> +} >> + > > No blank line here would be more consistent with local style (based on > a really quick look). I see it going both ways in that file. But I'll delete the line. > >> +static DEVICE_ATTR_RO(qtg_id); >> + >> static struct attribute *cxl_decoder_base_attrs[] = { >> &dev_attr_start.attr, >> &dev_attr_size.attr, >> @@ -295,6 +305,7 @@ static struct attribute *cxl_decoder_root_attrs[] = { >> &dev_attr_cap_type2.attr, >> &dev_attr_cap_type3.attr, >> &dev_attr_target_list.attr, >> + &dev_attr_qtg_id.attr, >> SET_CXL_REGION_ATTR(create_pmem_region) >> SET_CXL_REGION_ATTR(create_ram_region) >> SET_CXL_REGION_ATTR(delete_region) >> @@ -1625,6 +1636,7 @@ struct cxl_root_decoder *cxl_root_decoder_alloc(struct cxl_port *port, >> } >> >> atomic_set(&cxlrd->region_id, rc); >> + cxld->qtg_id = CXL_QTG_ID_INVALID; >> return cxlrd; >> } >> EXPORT_SYMBOL_NS_GPL(cxl_root_decoder_alloc, CXL); >> @@ -1662,6 +1674,7 @@ struct cxl_switch_decoder *cxl_switch_decoder_alloc(struct cxl_port *port, >> >> cxld = &cxlsd->cxld; >> cxld->dev.type = &cxl_decoder_switch_type; >> + cxld->qtg_id = CXL_QTG_ID_INVALID; >> return cxlsd; >> } >> EXPORT_SYMBOL_NS_GPL(cxl_switch_decoder_alloc, CXL); >> @@ -1694,6 +1707,7 @@ struct cxl_endpoint_decoder *cxl_endpoint_decoder_alloc(struct cxl_port *port) >> } >> >> cxld->dev.type = &cxl_decoder_endpoint_type; >> + cxld->qtg_id = CXL_QTG_ID_INVALID; >> return cxled; >> } >> EXPORT_SYMBOL_NS_GPL(cxl_endpoint_decoder_alloc, CXL); >> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h >> index 044a92d9813e..278ab6952332 100644 >> --- a/drivers/cxl/cxl.h >> +++ b/drivers/cxl/cxl.h >> @@ -300,6 +300,7 @@ enum cxl_decoder_type { >> */ >> #define CXL_DECODER_MAX_INTERLEAVE 16 >> >> +#define CXL_QTG_ID_INVALID -1 >> >> /** >> * struct cxl_decoder - Common CXL HDM Decoder Attributes >> @@ -311,6 +312,7 @@ enum cxl_decoder_type { >> * @target_type: accelerator vs expander (type2 vs type3) selector >> * @region: currently assigned region for this decoder >> * @flags: memory type capabilities and locking >> + * @qtg_id: QoS Throttling Group ID >> * @commit: device/decoder-type specific callback to commit settings to hw >> * @reset: device/decoder-type specific callback to reset hw settings >> */ >> @@ -323,6 +325,7 @@ struct cxl_decoder { >> enum cxl_decoder_type target_type; >> struct cxl_region *region; >> unsigned long flags; >> + int qtg_id; >> int (*commit)(struct cxl_decoder *cxld); >> int (*reset)(struct cxl_decoder *cxld); >> }; >> >> >
On 4/20/23 2:41 AM, Jonathan Cameron wrote: > On Wed, 19 Apr 2023 13:21:25 -0700 > Dave Jiang <dave.jiang@intel.com> wrote: > >> Add helper functions to parse the CDAT table and provide a callback to >> parse the sub-table. Helpers are provided for DSMAS and DSLBIS sub-table >> parsing. The code is patterned after the ACPI table parsing helpers. >> >> Signed-off-by: Dave Jiang <dave.jiang@intel.com> >> > A few minor things inline. More than possible you addressed them > in earlier versions though. > > Jonathan > >> --- >> v2: >> - Use local headers to handle LE instead of ACPI header >> - Reduce complexity of parser function. (Jonathan) >> - Directly access header type. (Jonathan) >> - Simplify header ptr math. (Jonathan) >> - Move parsed counter to the correct location. (Jonathan) >> - Add LE to host conversion for entry length >> --- >> drivers/cxl/core/Makefile | 1 >> drivers/cxl/core/cdat.c | 100 +++++++++++++++++++++++++++++++++++++++++++++ >> drivers/cxl/cxlpci.h | 29 +++++++++++++ >> 3 files changed, 130 insertions(+) >> create mode 100644 drivers/cxl/core/cdat.c >> >> diff --git a/drivers/cxl/core/Makefile b/drivers/cxl/core/Makefile >> index ca4ae31d8f57..867a8014b462 100644 >> --- a/drivers/cxl/core/Makefile >> +++ b/drivers/cxl/core/Makefile >> @@ -12,5 +12,6 @@ cxl_core-y += memdev.o >> cxl_core-y += mbox.o >> cxl_core-y += pci.o >> cxl_core-y += hdm.o >> +cxl_core-y += cdat.o >> cxl_core-$(CONFIG_TRACING) += trace.o >> cxl_core-$(CONFIG_CXL_REGION) += region.o >> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c >> new file mode 100644 >> index 000000000000..210f4499bddb >> --- /dev/null >> +++ b/drivers/cxl/core/cdat.c >> @@ -0,0 +1,100 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* Copyright(c) 2023 Intel Corporation. All rights reserved. */ >> +#include "cxlpci.h" >> +#include "cxl.h" >> + >> +static bool has_handler(struct cdat_subtable_proc *proc) > > Even though they are static, I'd add a cxl_ or cdat_ prefix > to these to make it clear they are local. Ok I'll change to cdat_* > >> +{ >> + return proc->handler; >> +} >> + >> +static int call_handler(struct cdat_subtable_proc *proc, >> + struct cdat_subtable_entry *ent) >> +{ >> + if (has_handler(proc)) > > Do we need to check this again? It's checked in the parse_entries code > well before this point. > > Also, if moving to checking it once, then is it worth the > little wrapper functions? Ok I'll call it directly and remove the wrapper. > > >> + return proc->handler(ent->hdr, proc->arg); >> + return -EINVAL; >> +} >> + >> +static bool cdat_is_subtable_match(struct cdat_subtable_entry *ent) >> +{ >> + return ent->hdr->type == ent->type; >> +} >> + >> +static int cdat_table_parse_entries(enum cdat_type type, >> + struct cdat_header *table_header, >> + struct cdat_subtable_proc *proc) >> +{ >> + unsigned long table_end, entry_len; >> + struct cdat_subtable_entry entry; >> + int count = 0; >> + int rc; >> + >> + if (!has_handler(proc)) >> + return -EINVAL; >> + >> + table_end = (unsigned long)table_header + table_header->length; >> + >> + if (type >= CDAT_TYPE_RESERVED) >> + return -EINVAL; >> + >> + entry.type = type; >> + entry.hdr = (struct cdat_entry_header *)(table_header + 1); >> + >> + while ((unsigned long)entry.hdr < table_end) { >> + entry_len = le16_to_cpu(entry.hdr->length); >> + >> + if ((unsigned long)entry.hdr + entry_len > table_end) >> + return -EINVAL; >> + >> + if (entry_len == 0) >> + return -EINVAL; >> + >> + if (cdat_is_subtable_match(&entry)) { >> + rc = call_handler(proc, &entry); >> + if (rc) >> + return rc; >> + count++; >> + } >> + >> + entry.hdr = (struct cdat_entry_header *)((unsigned long)entry.hdr + entry_len); >> + } >> + >> + return count; >> +} > > ... > >> +int cdat_table_parse_sslbis(struct cdat_header *table, >> + cdat_tbl_entry_handler handler, void *arg) > > Feels like these ones should take a typed arg. Sure you'll loose > that again to use the generic handling code, but at this level we can > do it I think. while DSMAS and DSLBIS takes a list_head, SSLBIS takes an xarray. I can create a union. > >> +{ >> + struct cdat_subtable_proc proc = { >> + .handler = handler, >> + .arg = arg, >> + }; >> + >> + return cdat_table_parse_entries(CDAT_TYPE_SSLBIS, table, &proc); >> +} >
On 4/20/23 4:35 AM, Jonathan Cameron wrote: > On Thu, 20 Apr 2023 12:33:50 +0100 > Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote: > >> On Wed, 19 Apr 2023 13:21:31 -0700 >> Dave Jiang <dave.jiang@intel.com> wrote: >> >>> Provide a callback function to the CDAT parser in order to parse the Device >>> Scoped Memory Affinity Structure (DSMAS). Each DSMAS structure contains the >>> DPA range and its associated attributes in each entry. See the CDAT >>> specification for details. >>> >>> Coherent Device Attribute Table 1.03 2.1 Device Scoped memory Affinity >>> Structure (DSMAS) >> >> I'm not sure what purpose of this is. If it's just detecting problems >> with the entry because we aren't interested in the content yet, then fine >> but good to make that clear in patch intro. >> >> Maybe I'm missing something! >> > Ah. Got to next patch. Perhaps a forwards reference to that will avoid > anyone else wondering what is going on here! Ok I'll clarify. > >> Thanks, >> >> Jonathan >> >>> >>> Signed-off-by: Dave Jiang <dave.jiang@intel.com> >>> >>> --- >>> v3: >>> - Add spec section number. (Alison) >>> - Remove cast from void *. (Alison) >>> - Refactor cxl_port_probe() block. (Alison) >>> - Move CDAT parse to cxl_endpoint_port_probe() >>> >>> v2: >>> - Add DSMAS table size check. (Lukas) >>> - Use local DSMAS header for LE handling. >>> - Remove dsmas lock. (Jonathan) >>> - Fix handle size (Jonathan) >>> - Add LE to host conversion for DSMAS address and length. >>> - Make dsmas_list local >> >> >>> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c >>> index 615e0ef6b440..3022bdd52439 100644 >>> --- a/drivers/cxl/port.c >>> +++ b/drivers/cxl/port.c >>> @@ -57,6 +57,16 @@ static int discover_region(struct device *dev, void *root) >>> return 0; >>> } >> >>> static int cxl_switch_port_probe(struct cxl_port *port) >>> { >>> struct cxl_hdm *cxlhdm; >>> @@ -125,6 +135,18 @@ static int cxl_endpoint_port_probe(struct cxl_port *port) >>> device_for_each_child(&port->dev, root, discover_region); >>> put_device(&root->dev); >>> >>> + if (port->cdat.table) { >>> + LIST_HEAD(dsmas_list); >>> + >>> + rc = cdat_table_parse_dsmas(port->cdat.table, >>> + cxl_dsmas_parse_entry, >>> + (void *)&dsmas_list); >>> + if (rc < 0) >>> + dev_warn(&port->dev, "Failed to parse DSMAS: %d\n", rc); >>> + >>> + dsmas_list_destroy(&dsmas_list); >> >> I'm a little confused here. What's the point? Parse them then throw the info away? >> Maybe a comment if all we are trying to do is warn about CDAT problems. >> >> >>> + } >>> + >>> return 0; >>> } >>> >>> >>> >> >
> >> +int cdat_table_parse_sslbis(struct cdat_header *table, > >> + cdat_tbl_entry_handler handler, void *arg) > > > > Feels like these ones should take a typed arg. Sure you'll loose > > that again to use the generic handling code, but at this level we can > > do it I think. > > while DSMAS and DSLBIS takes a list_head, SSLBIS takes an xarray. I can > create a union. I don't understand why, If you drop the macro usage introduced in a later patch you can just have each one take the right thing. That macro isn't a huge saving anyway. Jonathan > > > > >> +{ > >> + struct cdat_subtable_proc proc = { > >> + .handler = handler, > >> + .arg = arg, > >> + }; > >> + > >> + return cdat_table_parse_entries(CDAT_TYPE_SSLBIS, table, &proc); > >> +} > > >
On 4/21/23 9:06 AM, Jonathan Cameron wrote: > >>>> +int cdat_table_parse_sslbis(struct cdat_header *table, >>>> + cdat_tbl_entry_handler handler, void *arg) >>> >>> Feels like these ones should take a typed arg. Sure you'll loose >>> that again to use the generic handling code, but at this level we can >>> do it I think. >> >> while DSMAS and DSLBIS takes a list_head, SSLBIS takes an xarray. I can >> create a union. > > I don't understand why, If you drop the macro usage introduced in > a later patch you can just have each one take the right thing. > That macro isn't a huge saving anyway. Oh I think I understand where you are trying to get at. Ok I'll update. > > Jonathan > >> >>> >>>> +{ >>>> + struct cdat_subtable_proc proc = { >>>> + .handler = handler, >>>> + .arg = arg, >>>> + }; >>>> + >>>> + return cdat_table_parse_entries(CDAT_TYPE_SSLBIS, table, &proc); >>>> +} >>> >> >
On 4/20/23 5:06 AM, Jonathan Cameron wrote: > On Wed, 19 Apr 2023 13:21:55 -0700 > Dave Jiang <dave.jiang@intel.com> wrote: > >> Provide a helper to find the ACPI0017 device in order to issue the _DSM. >> The helper will take the 'struct device' from a cxl_port and iterate until >> the root device is reached. The ACPI handle will be returned from the root >> device. >> >> Signed-off-by: Dave Jiang <dave.jiang@intel.com> > > Question inline. If the answer is no then this looks fine to me. > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > >> +/** >> + * cxl_acpi_get_rootdev_handle - get the ACPI handle of the CXL root device >> + * @dev: 'struct device' to start searching from. Should be from cxl_port->dev. >> + * >> + * Return: acpi_handle on success, errptr of errno on error. >> + * >> + * Looks for the ACPI0017 device and return the ACPI handle >> + **/ > > Could we implement this in terms of find_cxl_root()? I think that will > end up giving you the same device though I haven't tested it. Yes I can simplify this. Thanks. > >> +acpi_handle cxl_acpi_get_rootdev_handle(struct device *dev) >> +{ >> + struct device *itr = dev; >> + struct device *root_dev; >> + acpi_handle handle; >> + >> + if (!dev) >> + return ERR_PTR(-EINVAL); >> + >> + while (itr->parent) { >> + root_dev = itr; >> + itr = itr->parent; >> + } >> + >> + if (!dev_is_platform(root_dev)) >> + return ERR_PTR(-ENODEV); >> + >> + handle = ACPI_HANDLE(root_dev); >> + if (!handle) >> + return ERR_PTR(-ENODEV); >> + >> + return handle; >> +} >> +EXPORT_SYMBOL_NS_GPL(cxl_acpi_get_rootdev_handle, CXL); > >
v4: - Reworked PCIe link path latency calculation - 0-day fixes - Removed unused qos_list from cxl_memdev and its stray usages v3: - Please see specific patches for log entries addressing comments from v2. - Refactor cxl_port_probe() additions. (Alison) - Convert to use 'struct node_hmem_attrs' - Refactor to use common code for genport target allocation. - Add third array entry for target hmem_attrs to store genport locality data. - Go back to per partition QTG ID. (Dan) v2: - Please see specific patches for log entries addressing comments from v1. - Removed ACPICA code usages. - Removed PCI subsystem helpers for latency and bandwidth. - Add CXL switch CDAT parsing support (SSLBIS) - Add generic port SRAT+HMAT support (ACPI) - Export a single QTG ID via sysfs per memory device (Dan) - Provide rest of DSMAS range info in debugfs (Dan) Hi Rafael, please review the relevant patches to ACPI: 13/23-16/23. Thank you! If they are ok, Dan can take them through the CXL tree for upstream merging. 13 - Adds enum for memory_target hmem_attrs in order to enumerate the array index. 14 - Add generic port target allocation for SRAT parsing during HMAT init in order to extract and store the device handle. 15 - Add a new index for hmem_attrs and save the locality data to the new hmem_attrs array element with generic port data. The old array elements are preserved for later when we want to store the calculated CXL memory target locality data. 16 - Add ACPI helper function to retrieve the locality data for generic port. Used by CXL driver to calculate the full locality data for the CXL memory device. This series adds the retrieval of QoS Throttling Group (QTG) IDs for the CXL Fixed Memory Window Structure (CFMWS) and the CXL memory device. It provides the QTG IDs to user space to provide some guidance with putting the proper DPA range under the appropriate CFMWS window for a hot-plugged CXL memory device. The CFMWS structure contains a QTG ID that is associated with the memory window that the structure exports. On Linux, the CFMWS is represented as a CXL root decoder. The QTG ID will be attached to the CXL root decoder and exported as a sysfs attribute (qtg_id). The QTG ID for a device is retrieved via sending a _DSM method to the ACPI0017 device. The _DSM expects an input package of 4 DWORDS that contains the read latency, write latency, read bandwidth, and write banwidth. These are the caluclated numbers for the path between the CXL device and the CXL host bridge (HB). The QTG ID is also exported as a sysfs attribute under the mem device memory partition type: /sys/bus/cxl/devices/memX/ram/qtg_id /sys/bus/cxl/devices/memX/pmem/qtg_id Only the first QTG ID is exported. The rest of the information can be found under /sys/kernel/debug/cxl/memX/qtgmap where all the DPA ranges with the correlated QTG ID are displayed. Each DSMAS from the device CDAT will provide a DPA range. The latency numbers are the aggregated latencies for the path between the CXL device and the CPU. If a CXL device is directly attached to the CXL HB, the latency would be the aggregated latencies from the device Coherent Device Attribute Table (CDAT), the caluclated PCIe link latency between the device and the HB, and the generic port data from ACPI SRAT+HMAT. The bandwidth in this configuration would be the minimum between the CDAT bandwidth number, link bandwidth between the device and the HB, and the bandwidth data from the generic port data via ACPI SRAT+HMAT. If a configuration has a switch in between then the latency would be the aggregated latencies from the device CDAT, the link latency between device and switch, the latency from the switch CDAT, the link latency between switch and the HB, and the generic port latency between the CPU and the CXL HB. The bandwidth calculation would be the min of device CDAT bandwidth, link bandwith between device and switch, switch CDAT bandwidth, the link bandwidth between switch and HB, and the generic port bandwidth There can be 0 or more switches between the CXL device and the CXL HB. There are detailed examples on calculating bandwidth and latency in the CXL Memory Device Software Guide [4]. The CDAT provides Device Scoped Memory Affinity Structures (DSMAS) that contains the Device Physical Address (DPA) range and the related Device Scoped Latency and Bandwidth Informat Stuctures (DSLBIS). Each DSLBIS provides a latency or bandwidth entry that is tied to a DSMAS entry via a per DSMAS unique DSMAD handle. This series is based on Lukas's latest DOE changes [5]. Kernel branch with all the code can be retrieved here [6] for convenience. Test setup is done with runqemu genport support branch [6]. The setup provides 2 CXL HBs with one HB having a CXL switch underneath. It also provides generic port support detailed below. A hacked up qemu branch is used to support generic port SRAT and HMAT [7]. To create the appropriate HMAT entries for generic port, the following qemu paramters must be added: -object genport,id=$X -numa node,genport=genport$X,nodeid=$Y,initiator=$Z -numa hmat-lb,initiator=$Z,target=$X,hierarchy=memory,data-type=access-latency,latency=$latency -numa hmat-lb,initiator=$Z,target=$X,hierarchy=memory,data-type=access-bandwidth,bandwidth=$bandwidthM for ((i = 0; i < total_nodes; i++)); do for ((j = 0; j < cxl_hbs; j++ )); do # 2 CXL HBs -numa dist,src=$i,dst=$X,val=$dist done done See the genport run_qemu branch for full details. [1]: https://www.computeexpresslink.org/download-the-specification [2]: https://uefi.org/sites/default/files/resources/Coherent%20Device%20Attribute%20Table_1.01.pdf [3]: https://uefi.org/sites/default/files/resources/ACPI_Spec_6_5_Aug29.pdf [4]: https://cdrdv2-public.intel.com/643805/643805_CXL%20Memory%20Device%20SW%20Guide_Rev1p0.pdf [5]: https://lore.kernel.org/linux-cxl/20230313195530.GA1532686@bhelgaas/T/#t [6]: https://git.kernel.org/pub/scm/linux/kernel/git/djiang/linux.git/log/?h=cxl-qtg [7]: https://github.com/pmem/run_qemu/tree/djiang/genport [8]: https://github.com/davejiang/qemu/tree/genport --- Dave Jiang (23): cxl: Export QTG ids from CFMWS to sysfs cxl: Add checksum verification to CDAT from CXL cxl: Add support for reading CXL switch CDAT table cxl: Add common helpers for cdat parsing cxl: Add callback to parse the DSMAS subtables from CDAT cxl: Add callback to parse the DSLBIS subtable from CDAT cxl: Add callback to parse the SSLBIS subtable from CDAT cxl: Add support for _DSM Function for retrieving QTG ID cxl: Add helper function to retrieve ACPI handle of CXL root device cxl: Add helpers to calculate pci latency for the CXL device cxl: Add helper function that calculates QoS values for switches cxl: Add helper function that calculate QoS values for PCI path ACPI: NUMA: Create enum for memory_target hmem_attrs indexing ACPI: NUMA: Add genport target allocation to the HMAT parsing ACPI: NUMA: Add setting of generic port locality attributes ACPI: NUMA: Add helper function to retrieve the performance attributes cxl: Add helper function to retrieve generic port QoS cxl: Add latency and bandwidth calculations for the CXL path cxl: Wait Memory_Info_Valid before access memory related info cxl: Move identify and partition query from pci probe to port probe cxl: Store QTG IDs and related info to the CXL memory device context cxl: Export sysfs attributes for memory device QTG ID cxl/mem: Add debugfs output for QTG related data Documentation/ABI/testing/debugfs-cxl | 11 + Documentation/ABI/testing/sysfs-bus-cxl | 31 +++ drivers/acpi/numa/hmat.c | 138 ++++++++++-- drivers/cxl/acpi.c | 3 + drivers/cxl/core/Makefile | 2 + drivers/cxl/core/acpi.c | 180 ++++++++++++++++ drivers/cxl/core/cdat.c | 270 ++++++++++++++++++++++++ drivers/cxl/core/mbox.c | 3 + drivers/cxl/core/memdev.c | 26 +++ drivers/cxl/core/pci.c | 187 ++++++++++++++-- drivers/cxl/core/port.c | 183 ++++++++++++++++ drivers/cxl/cxl.h | 27 +++ drivers/cxl/cxlmem.h | 21 ++ drivers/cxl/cxlpci.h | 117 ++++++++++ drivers/cxl/mem.c | 17 ++ drivers/cxl/pci.c | 21 -- drivers/cxl/port.c | 155 +++++++++++++- include/acpi/actbl3.h | 2 + include/linux/acpi.h | 6 + 19 files changed, 1348 insertions(+), 52 deletions(-) create mode 100644 Documentation/ABI/testing/debugfs-cxl create mode 100644 drivers/cxl/core/acpi.c create mode 100644 drivers/cxl/core/cdat.c --