* [RFC] cxl: Update Soft Reserved resources upon region creation
@ 2024-10-04 18:17 Nathan Fontenot
2024-10-07 14:53 ` kernel test robot
` (3 more replies)
0 siblings, 4 replies; 8+ messages in thread
From: Nathan Fontenot @ 2024-10-04 18:17 UTC (permalink / raw)
To: alison.schofield, dan.j.williams; +Cc: linux-cxl
Update handling of SOFT RESERVE iomem resources that intersect with
CXL region resources to remove the intersections from the SOFT RESERVE
resources. The current approach of leaving the SOFT RESERVE
resource as is can cause failures during hotplug replace of CXL
devices because the resource is not available for reuse after
teardown.
The approach sought is to trim out any pieces of SOFT RESERVE
resources that intersect with CXL regions. To do this, first set
aside any SOFT RESERVE resources that intersect with a CFMWS
into a separate resource tree during e820__reserve_resources_late()
that would have been otherwise added to the iomem resource tree.
As CXL regions are created the cxl resource created for the new
region is used to trim intersections from the SOFT RESERVE
resources that were previously set aside.
The next steps are to add any SOFT RESERVE resources remaining to the
iomem resource tree after CXL device probe completes and to notify
the dax driver so it may consume the added SOFT RESERVE resources.
This patch includes the use of a delayed work queue to wait
for CXL device probe completion and then have a worker thread add
any remaining SOFT RESERVE resources to the iomem resource tree.
Not in this patch is notification of the dax driver so it may consume
the SOFT RESERVE resources.
The goal of presenting this RFC is to drive discussion of the
current approach for trimming SOFT RESERVE resources, the use of
a delayed work queue to add remaining SOFT RESERVE resources to
the iomem resource tree, and methods for notifying the dax driver
of any newly added SOFT RESERVE resources.
NOTE: As this is a RFC the temporary pr_err("CXL DEBUG...") messages
have been left in to aid in testing and validation.
Signed-off-by: Nathan Fontenot <nathan.fontenot@amd.com>
Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
arch/x86/include/asm/e820/api.h | 3 +
arch/x86/kernel/e820.c | 156 +++++++++++++++++++++++++++++++-
drivers/cxl/core/region.c | 14 ++-
drivers/cxl/port.c | 34 +++++++
4 files changed, 203 insertions(+), 4 deletions(-)
diff --git a/arch/x86/include/asm/e820/api.h b/arch/x86/include/asm/e820/api.h
index 2e74a7f0e935..542a52871beb 100644
--- a/arch/x86/include/asm/e820/api.h
+++ b/arch/x86/include/asm/e820/api.h
@@ -44,6 +44,9 @@ extern void e820__register_nosave_regions(unsigned long limit_pfn);
extern int e820__get_entry_type(u64 start, u64 end);
+extern void e820__trim_soft_reserves(const struct resource *cxl_res);
+extern void e820__insert_soft_reserves(void);
+
/*
* Returns true iff the specified range [start,end) is completely contained inside
* the ISA region.
diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index 4893d30ce438..855c26460bb9 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -1208,15 +1208,165 @@ static unsigned long __init ram_alignment(resource_size_t pos)
#define MAX_RESOURCE_SIZE ((resource_size_t)-1)
+static struct resource e820_sr_res = {
+ .name = "e820 Soft Reserves",
+ .start = 0,
+ .end = MAX_RESOURCE_SIZE,
+};
+
+void e820__insert_soft_reserves(void)
+{
+ struct resource *res, *next;
+ int rc;
+
+ pr_err("CXL DEBUG Inserting Soft Reserves\n");
+ for (res = e820_sr_res.child; res; res = next) {
+ next = res->sibling;
+
+ pr_err("CXL DEBUG Inserting Soft Reserve %pr\n", res);
+ remove_resource(res);
+ rc = insert_resource(&iomem_resource, res);
+ if (rc)
+ pr_debug("CXL DEBUG Cannot insert %pr\n", res);
+ }
+}
+EXPORT_SYMBOL_GPL(e820__insert_soft_reserves);
+
+static void e820__add_soft_reserve(resource_size_t start, resource_size_t len,
+ unsigned long flags)
+{
+ struct resource *res;
+
+ res = kzalloc(sizeof(*res), GFP_KERNEL);
+ if (!res) {
+ pr_err("CXL DEBUG Couldn't add Soft Reserved %llx (%llx)\n",
+ start, len);
+ return;
+ }
+
+ *res = DEFINE_RES_NAMED(start, len, "Soft Reserved", flags);
+ res->desc = IORES_DESC_SOFT_RESERVED;
+ pr_err("CXL DEBUG Inserting new Soft Reserved %pr\n", res);
+ insert_resource(&e820_sr_res, res);
+}
+
+static void e820__trim_soft_reserve(struct resource *res,
+ const struct resource *cxl_res)
+{
+ resource_size_t new_start, new_end;
+ int rc;
+
+ pr_err("CXL DEBUG Trimming Soft Reserves for %pr\n", cxl_res);
+
+ if (res->start == cxl_res->start && res->end == cxl_res->end) {
+ pr_err("CXL DEBUG Releasing resource %pr\n", res);
+ release_resource(res);
+ kfree(res);
+ } else if (res->start == cxl_res->start || res->end == cxl_res->end) {
+ if (res->start == cxl_res->start) {
+ new_start = cxl_res->end + 1;
+ new_end = res->end;
+ } else {
+ new_start = res->start;
+ new_end = cxl_res->start - 1;
+ }
+
+ pr_err("CXL DEBUG Adjusting resource %pr (%llx - %llx)\n",
+ res, new_start, new_end);
+ rc = adjust_resource(res, new_start, new_end - new_start + 1);
+ if (rc)
+ pr_debug("Cannot adjust soft reserved resource %pr\n",
+ res);
+ } else {
+ new_start = res->start;
+ new_end = res->end;
+
+ /*Adjust existing to beginning resource */
+ pr_err("CXL DEBUG Adjusting resource %pr (%llx - %llx)\n", res,
+ new_start, cxl_res->start);
+ adjust_resource(res, new_start, cxl_res->start - new_start + 1);
+
+ /* Add new resource for end piece */
+ e820__add_soft_reserve(cxl_res->end + 1, new_end - cxl_res->end,
+ res->flags);
+ }
+}
+
+void e820__trim_soft_reserves(const struct resource *cxl_res)
+{
+ struct resource *res, *next;
+
+ pr_err("CXL DEBUG Trimming Soft Reserves\n");
+ for (res = e820_sr_res.child; res; res = next) {
+ next = res->sibling;
+
+ if (resource_contains(res, cxl_res)) {
+ e820__trim_soft_reserve(res, cxl_res);
+ break;
+ }
+ }
+}
+EXPORT_SYMBOL_GPL(e820__trim_soft_reserves);
+
+static int __init e820_parse_cfmws(union acpi_subtable_headers *hdr, void *arg,
+ const unsigned long unused)
+{
+ struct acpi_cedt_cfmws *cfmws;
+ struct resource *res = arg;
+ struct resource cfmws_res;
+
+ /* Validation check, remove when finished debugging */
+ if (!res->parent && res->end)
+ pr_err("CXL DEBUG Should insert %pr\n", res);
+
+ if (res->parent || !res->end)
+ return 0;
+
+ cfmws = (struct acpi_cedt_cfmws *)hdr;
+ cfmws_res = DEFINE_RES_MEM(cfmws->base_hpa,
+ cfmws->base_hpa + cfmws->window_size);
+ pr_err("CXL DEBUG Found CFMWS: %pr\n", &cfmws_res);
+
+ if (resource_overlaps(&cfmws_res, res)) {
+ pr_err("CXL DEBUG Found SOFT RESERVE intersection %llx - %llx : %llx - %llx\n",
+ res->start, res->end, cfmws_res.start, cfmws_res.end);
+ e820__add_soft_reserve(res->start, resource_size(res),
+ res->flags);
+ return 1;
+ }
+
+ return 0;
+}
+
void __init e820__reserve_resources_late(void)
{
- int i;
+ int i, rc;
struct resource *res;
+ /*
+ * Prior to inserting SOFT_RESERVED resources we want to check for an
+ * intersection with potential CXL resources. Any SOFT_RESERVED resources
+ * that do intersect a potential CXL resource are set aside so they
+ * can be trimmed to accommodate CXL resource intersections and added to
+ * the iomem resource tree after the CXL drivers have completed their
+ * device probe.
+ */
+ pr_err("CXL DEBUG Checking e820 iomem resources\n");
+
res = e820_res;
for (i = 0; i < e820_table->nr_entries; i++) {
- if (!res->parent && res->end)
- insert_resource_expand_to_fit(&iomem_resource, res);
+ pr_err("CXL DEBUG Checking e820 iomem resource %llx - %llx\n",
+ res->start, res->end);
+ if (res->desc == IORES_DESC_SOFT_RESERVED) {
+ rc = acpi_table_parse_cedt(ACPI_CEDT_TYPE_CFMWS,
+ e820_parse_cfmws, res);
+ if (rc) {
+ res++;
+ continue;
+ }
+ }
+ pr_err("CXL DEBUG Inserting %llx - %llx\n", res->start, res->end);
+ insert_resource_expand_to_fit(&iomem_resource, res);
res++;
}
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 21ad5f242875..539cccfffda0 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -14,6 +14,10 @@
#include <cxl.h>
#include "core.h"
+#if CONFIG_X86
+#include <asm/e820/api.h>
+#endif
+
/**
* DOC: cxl core region
*
@@ -3226,6 +3230,14 @@ static int match_region_by_range(struct device *dev, void *data)
return rc;
}
+static int insert_region_resource(struct resource *parent, struct resource *res)
+{
+#if CONFIG_X86
+ e820__trim_soft_reserves(res);
+#endif
+ return insert_resource(parent, res);
+}
+
/* Establish an empty region covering the given HPA range */
static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
struct cxl_endpoint_decoder *cxled)
@@ -3272,7 +3284,7 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
*res = DEFINE_RES_MEM_NAMED(hpa->start, range_len(hpa),
dev_name(&cxlr->dev));
- rc = insert_resource(cxlrd->res, res);
+ rc = insert_region_resource(cxlrd->res, res);
if (rc) {
/*
* Platform-firmware may not have split resources like "System
diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
index d7d5d982ce69..9f94730c488f 100644
--- a/drivers/cxl/port.c
+++ b/drivers/cxl/port.c
@@ -7,6 +7,10 @@
#include "cxlmem.h"
#include "cxlpci.h"
+#if CONFIG_X86
+#include <asm/e820/api.h>
+#endif
+
/**
* DOC: cxl port
*
@@ -89,6 +93,35 @@ static int cxl_switch_port_probe(struct cxl_port *port)
return -ENXIO;
}
+DECLARE_RWSEM(cxl_sr_rwsem);
+
+static void cxl_sr_update(struct work_struct *w)
+{
+ down_write(&cxl_sr_rwsem);
+ pr_err("CXL DEBUG Updating soft reserves\n");
+ e820__insert_soft_reserves();
+ up_write(&cxl_sr_rwsem);
+}
+
+DECLARE_DELAYED_WORK(cxl_sr_work, cxl_sr_update);
+
+static void schedule_soft_reserve_update(void)
+{
+ static bool update_scheduled;
+ int timeout = 5 * HZ;
+
+ down_write(&cxl_sr_rwsem);
+ if (update_scheduled) {
+ pr_err("CXL DEBUG modifying delayed work timeout\n");
+ mod_delayed_work(system_wq, &cxl_sr_work, timeout);
+ } else {
+ pr_err("CXL DEBUG Adding delayed work\n");
+ schedule_delayed_work(&cxl_sr_work, timeout);
+ update_scheduled = true;
+ }
+ up_write(&cxl_sr_rwsem);
+}
+
static int cxl_endpoint_port_probe(struct cxl_port *port)
{
struct cxl_endpoint_dvsec_info info = { .port = port };
@@ -140,6 +173,7 @@ static int cxl_endpoint_port_probe(struct cxl_port *port)
*/
device_for_each_child(&port->dev, root, discover_region);
+ schedule_soft_reserve_update();
return 0;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [RFC] cxl: Update Soft Reserved resources upon region creation
2024-10-04 18:17 [RFC] cxl: Update Soft Reserved resources upon region creation Nathan Fontenot
@ 2024-10-07 14:53 ` kernel test robot
2024-10-07 15:04 ` kernel test robot
` (2 subsequent siblings)
3 siblings, 0 replies; 8+ messages in thread
From: kernel test robot @ 2024-10-07 14:53 UTC (permalink / raw)
To: Nathan Fontenot; +Cc: llvm, oe-kbuild-all
Hi Nathan,
[This is a private test report for your RFC patch.]
kernel test robot noticed the following build warnings:
[auto build test WARNING on tip/x86/core]
[also build test WARNING on linus/master v6.12-rc2 next-20241004]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Nathan-Fontenot/cxl-Update-Soft-Reserved-resources-upon-region-creation/20241005-021920
base: tip/x86/core
patch link: https://lore.kernel.org/r/20241004181754.8960-1-nathan.fontenot%40amd.com
patch subject: [RFC] cxl: Update Soft Reserved resources upon region creation
config: x86_64-allnoconfig (https://download.01.org/0day-ci/archive/20241007/202410072203.uX5lXYkD-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241007/202410072203.uX5lXYkD-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410072203.uX5lXYkD-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> arch/x86/kernel/e820.c:1311:42: warning: declaration of 'union acpi_subtable_headers' will not be visible outside of this function [-Wvisibility]
1311 | static int __init e820_parse_cfmws(union acpi_subtable_headers *hdr, void *arg,
| ^
arch/x86/kernel/e820.c:1361:9: error: call to undeclared function 'acpi_table_parse_cedt'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
1361 | rc = acpi_table_parse_cedt(ACPI_CEDT_TYPE_CFMWS,
| ^
arch/x86/kernel/e820.c:1361:9: note: did you mean 'acpi_table_parse'?
include/linux/acpi.h:913:19: note: 'acpi_table_parse' declared here
913 | static inline int acpi_table_parse(char *id,
| ^
1 warning and 1 error generated.
vim +1311 arch/x86/kernel/e820.c
1310
> 1311 static int __init e820_parse_cfmws(union acpi_subtable_headers *hdr, void *arg,
1312 const unsigned long unused)
1313 {
1314 struct acpi_cedt_cfmws *cfmws;
1315 struct resource *res = arg;
1316 struct resource cfmws_res;
1317
1318 /* Validation check, remove when finished debugging */
1319 if (!res->parent && res->end)
1320 pr_err("CXL DEBUG Should insert %pr\n", res);
1321
1322 if (res->parent || !res->end)
1323 return 0;
1324
1325 cfmws = (struct acpi_cedt_cfmws *)hdr;
1326 cfmws_res = DEFINE_RES_MEM(cfmws->base_hpa,
1327 cfmws->base_hpa + cfmws->window_size);
1328 pr_err("CXL DEBUG Found CFMWS: %pr\n", &cfmws_res);
1329
1330 if (resource_overlaps(&cfmws_res, res)) {
1331 pr_err("CXL DEBUG Found SOFT RESERVE intersection %llx - %llx : %llx - %llx\n",
1332 res->start, res->end, cfmws_res.start, cfmws_res.end);
1333 e820__add_soft_reserve(res->start, resource_size(res),
1334 res->flags);
1335 return 1;
1336 }
1337
1338 return 0;
1339 }
1340
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC] cxl: Update Soft Reserved resources upon region creation
2024-10-04 18:17 [RFC] cxl: Update Soft Reserved resources upon region creation Nathan Fontenot
2024-10-07 14:53 ` kernel test robot
@ 2024-10-07 15:04 ` kernel test robot
2024-10-07 19:30 ` kernel test robot
2024-10-16 15:43 ` Jonathan Cameron
3 siblings, 0 replies; 8+ messages in thread
From: kernel test robot @ 2024-10-07 15:04 UTC (permalink / raw)
To: Nathan Fontenot; +Cc: oe-kbuild-all
Hi Nathan,
[This is a private test report for your RFC patch.]
kernel test robot noticed the following build errors:
[auto build test ERROR on tip/x86/core]
[also build test ERROR on linus/master v6.12-rc2 next-20241004]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Nathan-Fontenot/cxl-Update-Soft-Reserved-resources-upon-region-creation/20241005-021920
base: tip/x86/core
patch link: https://lore.kernel.org/r/20241004181754.8960-1-nathan.fontenot%40amd.com
patch subject: [RFC] cxl: Update Soft Reserved resources upon region creation
config: i386-allnoconfig (https://download.01.org/0day-ci/archive/20241007/202410072217.TJBadKhH-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241007/202410072217.TJBadKhH-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410072217.TJBadKhH-lkp@intel.com/
All error/warnings (new ones prefixed by >>):
11 | #define KERN_ERR KERN_SOH "3" /* error conditions */
| ^~~~~~~~
include/linux/printk.h:508:16: note: in expansion of macro 'KERN_ERR'
508 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~~
arch/x86/kernel/e820.c:1242:17: note: in expansion of macro 'pr_err'
1242 | pr_err("CXL DEBUG Couldn't add Soft Reserved %llx (%llx)\n",
| ^~~~~~
include/linux/kern_levels.h:5:25: warning: format '%llx' expects argument of type 'long long unsigned int', but argument 3 has type 'resource_size_t' {aka 'unsigned int'} [-Wformat=]
5 | #define KERN_SOH "\001" /* ASCII Start Of Header */
| ^~~~~~
include/linux/printk.h:437:25: note: in definition of macro 'printk_index_wrap'
437 | _p_func(_fmt, ##__VA_ARGS__); \
| ^~~~
include/linux/printk.h:508:9: note: in expansion of macro 'printk'
508 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~
include/linux/kern_levels.h:11:25: note: in expansion of macro 'KERN_SOH'
11 | #define KERN_ERR KERN_SOH "3" /* error conditions */
| ^~~~~~~~
include/linux/printk.h:508:16: note: in expansion of macro 'KERN_ERR'
508 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~~
arch/x86/kernel/e820.c:1242:17: note: in expansion of macro 'pr_err'
1242 | pr_err("CXL DEBUG Couldn't add Soft Reserved %llx (%llx)\n",
| ^~~~~~
arch/x86/kernel/e820.c: In function 'e820__trim_soft_reserve':
include/linux/kern_levels.h:5:25: warning: format '%llx' expects argument of type 'long long unsigned int', but argument 3 has type 'resource_size_t' {aka 'unsigned int'} [-Wformat=]
5 | #define KERN_SOH "\001" /* ASCII Start Of Header */
| ^~~~~~
include/linux/printk.h:437:25: note: in definition of macro 'printk_index_wrap'
437 | _p_func(_fmt, ##__VA_ARGS__); \
| ^~~~
include/linux/printk.h:508:9: note: in expansion of macro 'printk'
508 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~
include/linux/kern_levels.h:11:25: note: in expansion of macro 'KERN_SOH'
11 | #define KERN_ERR KERN_SOH "3" /* error conditions */
| ^~~~~~~~
include/linux/printk.h:508:16: note: in expansion of macro 'KERN_ERR'
508 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~~
arch/x86/kernel/e820.c:1274:17: note: in expansion of macro 'pr_err'
1274 | pr_err("CXL DEBUG Adjusting resource %pr (%llx - %llx)\n",
| ^~~~~~
include/linux/kern_levels.h:5:25: warning: format '%llx' expects argument of type 'long long unsigned int', but argument 4 has type 'resource_size_t' {aka 'unsigned int'} [-Wformat=]
5 | #define KERN_SOH "\001" /* ASCII Start Of Header */
| ^~~~~~
include/linux/printk.h:437:25: note: in definition of macro 'printk_index_wrap'
437 | _p_func(_fmt, ##__VA_ARGS__); \
| ^~~~
include/linux/printk.h:508:9: note: in expansion of macro 'printk'
508 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~
include/linux/kern_levels.h:11:25: note: in expansion of macro 'KERN_SOH'
11 | #define KERN_ERR KERN_SOH "3" /* error conditions */
| ^~~~~~~~
include/linux/printk.h:508:16: note: in expansion of macro 'KERN_ERR'
508 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~~
arch/x86/kernel/e820.c:1274:17: note: in expansion of macro 'pr_err'
1274 | pr_err("CXL DEBUG Adjusting resource %pr (%llx - %llx)\n",
| ^~~~~~
include/linux/kern_levels.h:5:25: warning: format '%llx' expects argument of type 'long long unsigned int', but argument 3 has type 'resource_size_t' {aka 'unsigned int'} [-Wformat=]
5 | #define KERN_SOH "\001" /* ASCII Start Of Header */
| ^~~~~~
include/linux/printk.h:437:25: note: in definition of macro 'printk_index_wrap'
437 | _p_func(_fmt, ##__VA_ARGS__); \
| ^~~~
include/linux/printk.h:508:9: note: in expansion of macro 'printk'
508 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~
include/linux/kern_levels.h:11:25: note: in expansion of macro 'KERN_SOH'
11 | #define KERN_ERR KERN_SOH "3" /* error conditions */
| ^~~~~~~~
include/linux/printk.h:508:16: note: in expansion of macro 'KERN_ERR'
508 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~~
arch/x86/kernel/e820.c:1285:17: note: in expansion of macro 'pr_err'
1285 | pr_err("CXL DEBUG Adjusting resource %pr (%llx - %llx)\n", res,
| ^~~~~~
include/linux/kern_levels.h:5:25: warning: format '%llx' expects argument of type 'long long unsigned int', but argument 4 has type 'resource_size_t' {aka 'unsigned int'} [-Wformat=]
5 | #define KERN_SOH "\001" /* ASCII Start Of Header */
| ^~~~~~
include/linux/printk.h:437:25: note: in definition of macro 'printk_index_wrap'
437 | _p_func(_fmt, ##__VA_ARGS__); \
| ^~~~
include/linux/printk.h:508:9: note: in expansion of macro 'printk'
508 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~
include/linux/kern_levels.h:11:25: note: in expansion of macro 'KERN_SOH'
11 | #define KERN_ERR KERN_SOH "3" /* error conditions */
| ^~~~~~~~
include/linux/printk.h:508:16: note: in expansion of macro 'KERN_ERR'
508 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~~
arch/x86/kernel/e820.c:1285:17: note: in expansion of macro 'pr_err'
1285 | pr_err("CXL DEBUG Adjusting resource %pr (%llx - %llx)\n", res,
| ^~~~~~
arch/x86/kernel/e820.c: At top level:
>> arch/x86/kernel/e820.c:1311:42: warning: 'union acpi_subtable_headers' declared inside parameter list will not be visible outside of this definition or declaration
1311 | static int __init e820_parse_cfmws(union acpi_subtable_headers *hdr, void *arg,
| ^~~~~~~~~~~~~~~~~~~~~
arch/x86/kernel/e820.c: In function 'e820_parse_cfmws':
include/linux/kern_levels.h:5:25: warning: format '%llx' expects argument of type 'long long unsigned int', but argument 2 has type 'resource_size_t' {aka 'unsigned int'} [-Wformat=]
5 | #define KERN_SOH "\001" /* ASCII Start Of Header */
| ^~~~~~
include/linux/printk.h:437:25: note: in definition of macro 'printk_index_wrap'
437 | _p_func(_fmt, ##__VA_ARGS__); \
| ^~~~
include/linux/printk.h:508:9: note: in expansion of macro 'printk'
508 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~
include/linux/kern_levels.h:11:25: note: in expansion of macro 'KERN_SOH'
11 | #define KERN_ERR KERN_SOH "3" /* error conditions */
| ^~~~~~~~
include/linux/printk.h:508:16: note: in expansion of macro 'KERN_ERR'
508 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~~
arch/x86/kernel/e820.c:1331:17: note: in expansion of macro 'pr_err'
1331 | pr_err("CXL DEBUG Found SOFT RESERVE intersection %llx - %llx : %llx - %llx\n",
| ^~~~~~
include/linux/kern_levels.h:5:25: warning: format '%llx' expects argument of type 'long long unsigned int', but argument 3 has type 'resource_size_t' {aka 'unsigned int'} [-Wformat=]
5 | #define KERN_SOH "\001" /* ASCII Start Of Header */
| ^~~~~~
include/linux/printk.h:437:25: note: in definition of macro 'printk_index_wrap'
437 | _p_func(_fmt, ##__VA_ARGS__); \
| ^~~~
include/linux/printk.h:508:9: note: in expansion of macro 'printk'
508 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~
include/linux/kern_levels.h:11:25: note: in expansion of macro 'KERN_SOH'
11 | #define KERN_ERR KERN_SOH "3" /* error conditions */
| ^~~~~~~~
include/linux/printk.h:508:16: note: in expansion of macro 'KERN_ERR'
508 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~~
arch/x86/kernel/e820.c:1331:17: note: in expansion of macro 'pr_err'
1331 | pr_err("CXL DEBUG Found SOFT RESERVE intersection %llx - %llx : %llx - %llx\n",
| ^~~~~~
include/linux/kern_levels.h:5:25: warning: format '%llx' expects argument of type 'long long unsigned int', but argument 4 has type 'resource_size_t' {aka 'unsigned int'} [-Wformat=]
5 | #define KERN_SOH "\001" /* ASCII Start Of Header */
| ^~~~~~
include/linux/printk.h:437:25: note: in definition of macro 'printk_index_wrap'
437 | _p_func(_fmt, ##__VA_ARGS__); \
| ^~~~
include/linux/printk.h:508:9: note: in expansion of macro 'printk'
508 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~
include/linux/kern_levels.h:11:25: note: in expansion of macro 'KERN_SOH'
11 | #define KERN_ERR KERN_SOH "3" /* error conditions */
| ^~~~~~~~
include/linux/printk.h:508:16: note: in expansion of macro 'KERN_ERR'
508 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~~
arch/x86/kernel/e820.c:1331:17: note: in expansion of macro 'pr_err'
1331 | pr_err("CXL DEBUG Found SOFT RESERVE intersection %llx - %llx : %llx - %llx\n",
| ^~~~~~
include/linux/kern_levels.h:5:25: warning: format '%llx' expects argument of type 'long long unsigned int', but argument 5 has type 'resource_size_t' {aka 'unsigned int'} [-Wformat=]
5 | #define KERN_SOH "\001" /* ASCII Start Of Header */
| ^~~~~~
include/linux/printk.h:437:25: note: in definition of macro 'printk_index_wrap'
437 | _p_func(_fmt, ##__VA_ARGS__); \
| ^~~~
include/linux/printk.h:508:9: note: in expansion of macro 'printk'
508 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~
include/linux/kern_levels.h:11:25: note: in expansion of macro 'KERN_SOH'
11 | #define KERN_ERR KERN_SOH "3" /* error conditions */
| ^~~~~~~~
include/linux/printk.h:508:16: note: in expansion of macro 'KERN_ERR'
508 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~~
arch/x86/kernel/e820.c:1331:17: note: in expansion of macro 'pr_err'
1331 | pr_err("CXL DEBUG Found SOFT RESERVE intersection %llx - %llx : %llx - %llx\n",
| ^~~~~~
arch/x86/kernel/e820.c: In function 'e820__reserve_resources_late':
include/linux/kern_levels.h:5:25: warning: format '%llx' expects argument of type 'long long unsigned int', but argument 2 has type 'resource_size_t' {aka 'unsigned int'} [-Wformat=]
5 | #define KERN_SOH "\001" /* ASCII Start Of Header */
| ^~~~~~
include/linux/printk.h:437:25: note: in definition of macro 'printk_index_wrap'
437 | _p_func(_fmt, ##__VA_ARGS__); \
| ^~~~
include/linux/printk.h:508:9: note: in expansion of macro 'printk'
508 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~
include/linux/kern_levels.h:11:25: note: in expansion of macro 'KERN_SOH'
11 | #define KERN_ERR KERN_SOH "3" /* error conditions */
| ^~~~~~~~
include/linux/printk.h:508:16: note: in expansion of macro 'KERN_ERR'
508 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~~
arch/x86/kernel/e820.c:1358:17: note: in expansion of macro 'pr_err'
1358 | pr_err("CXL DEBUG Checking e820 iomem resource %llx - %llx\n",
| ^~~~~~
include/linux/kern_levels.h:5:25: warning: format '%llx' expects argument of type 'long long unsigned int', but argument 3 has type 'resource_size_t' {aka 'unsigned int'} [-Wformat=]
5 | #define KERN_SOH "\001" /* ASCII Start Of Header */
| ^~~~~~
include/linux/printk.h:437:25: note: in definition of macro 'printk_index_wrap'
437 | _p_func(_fmt, ##__VA_ARGS__); \
| ^~~~
include/linux/printk.h:508:9: note: in expansion of macro 'printk'
508 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~
include/linux/kern_levels.h:11:25: note: in expansion of macro 'KERN_SOH'
11 | #define KERN_ERR KERN_SOH "3" /* error conditions */
| ^~~~~~~~
include/linux/printk.h:508:16: note: in expansion of macro 'KERN_ERR'
508 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~~
arch/x86/kernel/e820.c:1358:17: note: in expansion of macro 'pr_err'
1358 | pr_err("CXL DEBUG Checking e820 iomem resource %llx - %llx\n",
| ^~~~~~
>> arch/x86/kernel/e820.c:1361:30: error: implicit declaration of function 'acpi_table_parse_cedt'; did you mean 'acpi_table_parse'? [-Werror=implicit-function-declaration]
1361 | rc = acpi_table_parse_cedt(ACPI_CEDT_TYPE_CFMWS,
| ^~~~~~~~~~~~~~~~~~~~~
| acpi_table_parse
include/linux/kern_levels.h:5:25: warning: format '%llx' expects argument of type 'long long unsigned int', but argument 2 has type 'resource_size_t' {aka 'unsigned int'} [-Wformat=]
5 | #define KERN_SOH "\001" /* ASCII Start Of Header */
| ^~~~~~
include/linux/printk.h:437:25: note: in definition of macro 'printk_index_wrap'
437 | _p_func(_fmt, ##__VA_ARGS__); \
| ^~~~
include/linux/printk.h:508:9: note: in expansion of macro 'printk'
508 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~
include/linux/kern_levels.h:11:25: note: in expansion of macro 'KERN_SOH'
11 | #define KERN_ERR KERN_SOH "3" /* error conditions */
| ^~~~~~~~
include/linux/printk.h:508:16: note: in expansion of macro 'KERN_ERR'
508 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~~
arch/x86/kernel/e820.c:1368:17: note: in expansion of macro 'pr_err'
1368 | pr_err("CXL DEBUG Inserting %llx - %llx\n", res->start, res->end);
| ^~~~~~
include/linux/kern_levels.h:5:25: warning: format '%llx' expects argument of type 'long long unsigned int', but argument 3 has type 'resource_size_t' {aka 'unsigned int'} [-Wformat=]
5 | #define KERN_SOH "\001" /* ASCII Start Of Header */
| ^~~~~~
include/linux/printk.h:437:25: note: in definition of macro 'printk_index_wrap'
437 | _p_func(_fmt, ##__VA_ARGS__); \
| ^~~~
include/linux/printk.h:508:9: note: in expansion of macro 'printk'
508 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~
include/linux/kern_levels.h:11:25: note: in expansion of macro 'KERN_SOH'
11 | #define KERN_ERR KERN_SOH "3" /* error conditions */
| ^~~~~~~~
include/linux/printk.h:508:16: note: in expansion of macro 'KERN_ERR'
508 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~~
arch/x86/kernel/e820.c:1368:17: note: in expansion of macro 'pr_err'
1368 | pr_err("CXL DEBUG Inserting %llx - %llx\n", res->start, res->end);
| ^~~~~~
cc1: some warnings being treated as errors
vim +1361 arch/x86/kernel/e820.c
1252
1253 static void e820__trim_soft_reserve(struct resource *res,
1254 const struct resource *cxl_res)
1255 {
1256 resource_size_t new_start, new_end;
1257 int rc;
1258
1259 pr_err("CXL DEBUG Trimming Soft Reserves for %pr\n", cxl_res);
1260
1261 if (res->start == cxl_res->start && res->end == cxl_res->end) {
1262 pr_err("CXL DEBUG Releasing resource %pr\n", res);
1263 release_resource(res);
1264 kfree(res);
1265 } else if (res->start == cxl_res->start || res->end == cxl_res->end) {
1266 if (res->start == cxl_res->start) {
1267 new_start = cxl_res->end + 1;
1268 new_end = res->end;
1269 } else {
1270 new_start = res->start;
1271 new_end = cxl_res->start - 1;
1272 }
1273
1274 pr_err("CXL DEBUG Adjusting resource %pr (%llx - %llx)\n",
1275 res, new_start, new_end);
1276 rc = adjust_resource(res, new_start, new_end - new_start + 1);
1277 if (rc)
1278 pr_debug("Cannot adjust soft reserved resource %pr\n",
1279 res);
1280 } else {
1281 new_start = res->start;
1282 new_end = res->end;
1283
1284 /*Adjust existing to beginning resource */
> 1285 pr_err("CXL DEBUG Adjusting resource %pr (%llx - %llx)\n", res,
1286 new_start, cxl_res->start);
1287 adjust_resource(res, new_start, cxl_res->start - new_start + 1);
1288
1289 /* Add new resource for end piece */
1290 e820__add_soft_reserve(cxl_res->end + 1, new_end - cxl_res->end,
1291 res->flags);
1292 }
1293 }
1294
1295 void e820__trim_soft_reserves(const struct resource *cxl_res)
1296 {
1297 struct resource *res, *next;
1298
1299 pr_err("CXL DEBUG Trimming Soft Reserves\n");
1300 for (res = e820_sr_res.child; res; res = next) {
1301 next = res->sibling;
1302
1303 if (resource_contains(res, cxl_res)) {
1304 e820__trim_soft_reserve(res, cxl_res);
1305 break;
1306 }
1307 }
1308 }
1309 EXPORT_SYMBOL_GPL(e820__trim_soft_reserves);
1310
> 1311 static int __init e820_parse_cfmws(union acpi_subtable_headers *hdr, void *arg,
1312 const unsigned long unused)
1313 {
1314 struct acpi_cedt_cfmws *cfmws;
1315 struct resource *res = arg;
1316 struct resource cfmws_res;
1317
1318 /* Validation check, remove when finished debugging */
1319 if (!res->parent && res->end)
1320 pr_err("CXL DEBUG Should insert %pr\n", res);
1321
1322 if (res->parent || !res->end)
1323 return 0;
1324
1325 cfmws = (struct acpi_cedt_cfmws *)hdr;
1326 cfmws_res = DEFINE_RES_MEM(cfmws->base_hpa,
1327 cfmws->base_hpa + cfmws->window_size);
1328 pr_err("CXL DEBUG Found CFMWS: %pr\n", &cfmws_res);
1329
1330 if (resource_overlaps(&cfmws_res, res)) {
1331 pr_err("CXL DEBUG Found SOFT RESERVE intersection %llx - %llx : %llx - %llx\n",
1332 res->start, res->end, cfmws_res.start, cfmws_res.end);
1333 e820__add_soft_reserve(res->start, resource_size(res),
1334 res->flags);
1335 return 1;
1336 }
1337
1338 return 0;
1339 }
1340
1341 void __init e820__reserve_resources_late(void)
1342 {
1343 int i, rc;
1344 struct resource *res;
1345
1346 /*
1347 * Prior to inserting SOFT_RESERVED resources we want to check for an
1348 * intersection with potential CXL resources. Any SOFT_RESERVED resources
1349 * that do intersect a potential CXL resource are set aside so they
1350 * can be trimmed to accommodate CXL resource intersections and added to
1351 * the iomem resource tree after the CXL drivers have completed their
1352 * device probe.
1353 */
1354 pr_err("CXL DEBUG Checking e820 iomem resources\n");
1355
1356 res = e820_res;
1357 for (i = 0; i < e820_table->nr_entries; i++) {
> 1358 pr_err("CXL DEBUG Checking e820 iomem resource %llx - %llx\n",
1359 res->start, res->end);
1360 if (res->desc == IORES_DESC_SOFT_RESERVED) {
> 1361 rc = acpi_table_parse_cedt(ACPI_CEDT_TYPE_CFMWS,
1362 e820_parse_cfmws, res);
1363 if (rc) {
1364 res++;
1365 continue;
1366 }
1367 }
1368 pr_err("CXL DEBUG Inserting %llx - %llx\n", res->start, res->end);
1369 insert_resource_expand_to_fit(&iomem_resource, res);
1370 res++;
1371 }
1372
1373 /*
1374 * Try to bump up RAM regions to reasonable boundaries, to
1375 * avoid stolen RAM:
1376 */
1377 for (i = 0; i < e820_table->nr_entries; i++) {
1378 struct e820_entry *entry = &e820_table->entries[i];
1379 u64 start, end;
1380
1381 if (entry->type != E820_TYPE_RAM)
1382 continue;
1383
1384 start = entry->addr + entry->size;
1385 end = round_up(start, ram_alignment(start)) - 1;
1386 if (end > MAX_RESOURCE_SIZE)
1387 end = MAX_RESOURCE_SIZE;
1388 if (start >= end)
1389 continue;
1390
1391 printk(KERN_DEBUG "e820: reserve RAM buffer [mem %#010llx-%#010llx]\n", start, end);
1392 reserve_region_with_split(&iomem_resource, start, end, "RAM buffer");
1393 }
1394 }
1395
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC] cxl: Update Soft Reserved resources upon region creation
2024-10-04 18:17 [RFC] cxl: Update Soft Reserved resources upon region creation Nathan Fontenot
2024-10-07 14:53 ` kernel test robot
2024-10-07 15:04 ` kernel test robot
@ 2024-10-07 19:30 ` kernel test robot
2024-10-16 15:43 ` Jonathan Cameron
3 siblings, 0 replies; 8+ messages in thread
From: kernel test robot @ 2024-10-07 19:30 UTC (permalink / raw)
To: Nathan Fontenot; +Cc: llvm, oe-kbuild-all
Hi Nathan,
[This is a private test report for your RFC patch.]
kernel test robot noticed the following build warnings:
[auto build test WARNING on tip/x86/core]
[also build test WARNING on linus/master v6.12-rc2 next-20241004]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Nathan-Fontenot/cxl-Update-Soft-Reserved-resources-upon-region-creation/20241005-021920
base: tip/x86/core
patch link: https://lore.kernel.org/r/20241004181754.8960-1-nathan.fontenot%40amd.com
patch subject: [RFC] cxl: Update Soft Reserved resources upon region creation
config: i386-defconfig (https://download.01.org/0day-ci/archive/20241008/202410080207.DfKmNFfh-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241008/202410080207.DfKmNFfh-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410080207.DfKmNFfh-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> arch/x86/kernel/e820.c:1243:10: warning: format specifies type 'unsigned long long' but the argument has type 'resource_size_t' (aka 'unsigned int') [-Wformat]
1242 | pr_err("CXL DEBUG Couldn't add Soft Reserved %llx (%llx)\n",
| ~~~~
| %x
1243 | start, len);
| ^~~~~
include/linux/printk.h:508:33: note: expanded from macro 'pr_err'
508 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
| ~~~ ^~~~~~~~~~~
include/linux/printk.h:465:60: note: expanded from macro 'printk'
465 | #define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__)
| ~~~ ^~~~~~~~~~~
include/linux/printk.h:437:19: note: expanded from macro 'printk_index_wrap'
437 | _p_func(_fmt, ##__VA_ARGS__); \
| ~~~~ ^~~~~~~~~~~
arch/x86/kernel/e820.c:1243:17: warning: format specifies type 'unsigned long long' but the argument has type 'resource_size_t' (aka 'unsigned int') [-Wformat]
1242 | pr_err("CXL DEBUG Couldn't add Soft Reserved %llx (%llx)\n",
| ~~~~
| %x
1243 | start, len);
| ^~~
include/linux/printk.h:508:33: note: expanded from macro 'pr_err'
508 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
| ~~~ ^~~~~~~~~~~
include/linux/printk.h:465:60: note: expanded from macro 'printk'
465 | #define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__)
| ~~~ ^~~~~~~~~~~
include/linux/printk.h:437:19: note: expanded from macro 'printk_index_wrap'
437 | _p_func(_fmt, ##__VA_ARGS__); \
| ~~~~ ^~~~~~~~~~~
arch/x86/kernel/e820.c:1275:15: warning: format specifies type 'unsigned long long' but the argument has type 'resource_size_t' (aka 'unsigned int') [-Wformat]
1274 | pr_err("CXL DEBUG Adjusting resource %pr (%llx - %llx)\n",
| ~~~~
| %x
1275 | res, new_start, new_end);
| ^~~~~~~~~
include/linux/printk.h:508:33: note: expanded from macro 'pr_err'
508 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
| ~~~ ^~~~~~~~~~~
include/linux/printk.h:465:60: note: expanded from macro 'printk'
465 | #define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__)
| ~~~ ^~~~~~~~~~~
include/linux/printk.h:437:19: note: expanded from macro 'printk_index_wrap'
437 | _p_func(_fmt, ##__VA_ARGS__); \
| ~~~~ ^~~~~~~~~~~
arch/x86/kernel/e820.c:1275:26: warning: format specifies type 'unsigned long long' but the argument has type 'resource_size_t' (aka 'unsigned int') [-Wformat]
1274 | pr_err("CXL DEBUG Adjusting resource %pr (%llx - %llx)\n",
| ~~~~
| %x
1275 | res, new_start, new_end);
| ^~~~~~~
include/linux/printk.h:508:33: note: expanded from macro 'pr_err'
508 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
| ~~~ ^~~~~~~~~~~
include/linux/printk.h:465:60: note: expanded from macro 'printk'
465 | #define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__)
| ~~~ ^~~~~~~~~~~
include/linux/printk.h:437:19: note: expanded from macro 'printk_index_wrap'
437 | _p_func(_fmt, ##__VA_ARGS__); \
| ~~~~ ^~~~~~~~~~~
arch/x86/kernel/e820.c:1286:10: warning: format specifies type 'unsigned long long' but the argument has type 'resource_size_t' (aka 'unsigned int') [-Wformat]
1285 | pr_err("CXL DEBUG Adjusting resource %pr (%llx - %llx)\n", res,
| ~~~~
| %x
1286 | new_start, cxl_res->start);
| ^~~~~~~~~
include/linux/printk.h:508:33: note: expanded from macro 'pr_err'
508 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
| ~~~ ^~~~~~~~~~~
include/linux/printk.h:465:60: note: expanded from macro 'printk'
465 | #define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__)
| ~~~ ^~~~~~~~~~~
include/linux/printk.h:437:19: note: expanded from macro 'printk_index_wrap'
437 | _p_func(_fmt, ##__VA_ARGS__); \
| ~~~~ ^~~~~~~~~~~
arch/x86/kernel/e820.c:1286:21: warning: format specifies type 'unsigned long long' but the argument has type 'resource_size_t' (aka 'unsigned int') [-Wformat]
1285 | pr_err("CXL DEBUG Adjusting resource %pr (%llx - %llx)\n", res,
| ~~~~
| %x
1286 | new_start, cxl_res->start);
| ^~~~~~~~~~~~~~
include/linux/printk.h:508:33: note: expanded from macro 'pr_err'
508 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
| ~~~ ^~~~~~~~~~~
include/linux/printk.h:465:60: note: expanded from macro 'printk'
465 | #define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__)
| ~~~ ^~~~~~~~~~~
include/linux/printk.h:437:19: note: expanded from macro 'printk_index_wrap'
437 | _p_func(_fmt, ##__VA_ARGS__); \
| ~~~~ ^~~~~~~~~~~
arch/x86/kernel/e820.c:1332:10: warning: format specifies type 'unsigned long long' but the argument has type 'resource_size_t' (aka 'unsigned int') [-Wformat]
1331 | pr_err("CXL DEBUG Found SOFT RESERVE intersection %llx - %llx : %llx - %llx\n",
| ~~~~
| %x
1332 | res->start, res->end, cfmws_res.start, cfmws_res.end);
| ^~~~~~~~~~
include/linux/printk.h:508:33: note: expanded from macro 'pr_err'
508 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
| ~~~ ^~~~~~~~~~~
include/linux/printk.h:465:60: note: expanded from macro 'printk'
465 | #define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__)
vim +1243 arch/x86/kernel/e820.c
1234
1235 static void e820__add_soft_reserve(resource_size_t start, resource_size_t len,
1236 unsigned long flags)
1237 {
1238 struct resource *res;
1239
1240 res = kzalloc(sizeof(*res), GFP_KERNEL);
1241 if (!res) {
1242 pr_err("CXL DEBUG Couldn't add Soft Reserved %llx (%llx)\n",
> 1243 start, len);
1244 return;
1245 }
1246
1247 *res = DEFINE_RES_NAMED(start, len, "Soft Reserved", flags);
1248 res->desc = IORES_DESC_SOFT_RESERVED;
1249 pr_err("CXL DEBUG Inserting new Soft Reserved %pr\n", res);
1250 insert_resource(&e820_sr_res, res);
1251 }
1252
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC] cxl: Update Soft Reserved resources upon region creation
2024-10-04 18:17 [RFC] cxl: Update Soft Reserved resources upon region creation Nathan Fontenot
` (2 preceding siblings ...)
2024-10-07 19:30 ` kernel test robot
@ 2024-10-16 15:43 ` Jonathan Cameron
2024-10-17 14:49 ` Fontenot, Nathan
3 siblings, 1 reply; 8+ messages in thread
From: Jonathan Cameron @ 2024-10-16 15:43 UTC (permalink / raw)
To: Nathan Fontenot; +Cc: alison.schofield, dan.j.williams, linux-cxl
On Fri, 4 Oct 2024 13:17:54 -0500
Nathan Fontenot <nathan.fontenot@amd.com> wrote:
> Update handling of SOFT RESERVE iomem resources that intersect with
> CXL region resources to remove the intersections from the SOFT RESERVE
> resources. The current approach of leaving the SOFT RESERVE
> resource as is can cause failures during hotplug replace of CXL
> devices because the resource is not available for reuse after
> teardown.
Good to give an example here + any debug error messages etc.
>
> The approach sought is to trim out any pieces of SOFT RESERVE
> resources that intersect with CXL regions. To do this, first set
> aside any SOFT RESERVE resources that intersect with a CFMWS
> into a separate resource tree during e820__reserve_resources_late()
> that would have been otherwise added to the iomem resource tree.
>
> As CXL regions are created the cxl resource created for the new
> region is used to trim intersections from the SOFT RESERVE
> resources that were previously set aside.
>
> The next steps are to add any SOFT RESERVE resources remaining to the
> iomem resource tree after CXL device probe completes and to notify
> the dax driver so it may consume the added SOFT RESERVE resources.
>
> This patch includes the use of a delayed work queue to wait
> for CXL device probe completion and then have a worker thread add
> any remaining SOFT RESERVE resources to the iomem resource tree.
>
> Not in this patch is notification of the dax driver so it may consume
> the SOFT RESERVE resources.
>
> The goal of presenting this RFC is to drive discussion of the
> current approach for trimming SOFT RESERVE resources, the use of
> a delayed work queue to add remaining SOFT RESERVE resources to
> the iomem resource tree, and methods for notifying the dax driver
> of any newly added SOFT RESERVE resources.
>
> NOTE: As this is a RFC the temporary pr_err("CXL DEBUG...") messages
> have been left in to aid in testing and validation.
>
> Signed-off-by: Nathan Fontenot <nathan.fontenot@amd.com>
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
Trivial, but why Alison's SoB? She didn't send the patch which would
be normal reason for last SoB in a list like this (handling of the patch).
Maybe a Co-developed-by is missing?
Whilst I appreciate this is an RFC, the arch specific bleed
into CXL code needs to go. + we need some info here on whether this
is an x86 specific issue. I'm fairly sure it's not and that should
be called out as future work.
Also, much of this is actually generic, with only a few bits
being e820 related. Can we pull the soft reserve list etc out
of them into generic code.
To me the general approach seems reasonable but I'd suggest +CC linux-mm
as the issue maybe more about the impacts on DAX than this working for CXL.
Comments inline are mostly general suggestions of simplifications
and refactoring to aid readability.
> ---
> arch/x86/include/asm/e820/api.h | 3 +
> arch/x86/kernel/e820.c | 156 +++++++++++++++++++++++++++++++-
> drivers/cxl/core/region.c | 14 ++-
> drivers/cxl/port.c | 34 +++++++
> 4 files changed, 203 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/include/asm/e820/api.h b/arch/x86/include/asm/e820/api.h
> index 2e74a7f0e935..542a52871beb 100644
> --- a/arch/x86/include/asm/e820/api.h
> +++ b/arch/x86/include/asm/e820/api.h
> @@ -44,6 +44,9 @@ extern void e820__register_nosave_regions(unsigned long limit_pfn);
>
> extern int e820__get_entry_type(u64 start, u64 end);
>
> +extern void e820__trim_soft_reserves(const struct resource *cxl_res);
> +extern void e820__insert_soft_reserves(void);
> +
> /*
> * Returns true iff the specified range [start,end) is completely contained inside
> * the ISA region.
> diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
> index 4893d30ce438..855c26460bb9 100644
> --- a/arch/x86/kernel/e820.c
> +++ b/arch/x86/kernel/e820.c
> +static void e820__trim_soft_reserve(struct resource *res,
> + const struct resource *cxl_res)
> +{
> + resource_size_t new_start, new_end;
> + int rc;
> +
> + pr_err("CXL DEBUG Trimming Soft Reserves for %pr\n", cxl_res);
> +
> + if (res->start == cxl_res->start && res->end == cxl_res->end) {
> + pr_err("CXL DEBUG Releasing resource %pr\n", res);
> + release_resource(res);
> + kfree(res);
> + } else if (res->start == cxl_res->start || res->end == cxl_res->end) {
> + if (res->start == cxl_res->start) {
Not a huge amount of sharing in here.
} else if (res->start = cxl_res->start) {
/* Left over bit at top end */
rc = adjust_resource(res, cxl_res->end + 1, res->end - cxl_res->end);
if (rc)...
} else if (res->end == cxl_res->end) {
/* Left over bit at bottom end */
rc = adjust_resource(res, res->start, cxl_res->start - res->start);
if (rc) ...
} else {
/* Left over bits on either side. */
}
> + new_start = cxl_res->end + 1;
> + new_end = res->end;
> + } else {
> + new_start = res->start;
> + new_end = cxl_res->start - 1;
> + }
> +
> + pr_err("CXL DEBUG Adjusting resource %pr (%llx - %llx)\n",
> + res, new_start, new_end);
> + rc = adjust_resource(res, new_start, new_end - new_start + 1);
> + if (rc)
> + pr_debug("Cannot adjust soft reserved resource %pr\n",
> + res);
> + } else {
> + new_start = res->start;
> + new_end = res->end;
Confusing use of parameters here as they don't match use in early legs.
Maybe just putting the values inline would be clearer.
> +
> + /*Adjust existing to beginning resource */
> + pr_err("CXL DEBUG Adjusting resource %pr (%llx - %llx)\n", res,
> + new_start, cxl_res->start);
> + adjust_resource(res, new_start, cxl_res->start - new_start + 1);
Off by 1? It's different form above leftover bottom part (should be the same).
> +
> + /* Add new resource for end piece */
> + e820__add_soft_reserve(cxl_res->end + 1, new_end - cxl_res->end,
> + res->flags);
> + }
> +}
> +
> +void e820__trim_soft_reserves(const struct resource *cxl_res)
> +{
> + struct resource *res, *next;
> +
> + pr_err("CXL DEBUG Trimming Soft Reserves\n");
> + for (res = e820_sr_res.child; res; res = next) {
for (res = e820_sr_res.child; res; res = res->sibling)
is the same I think and avoids need for next.
> + next = res->sibling;
> +
> + if (resource_contains(res, cxl_res)) {
> + e820__trim_soft_reserve(res, cxl_res);
> + break;
return;
> + }
> + }
> +}
> +EXPORT_SYMBOL_GPL(e820__trim_soft_reserves);
> +
> +static int __init e820_parse_cfmws(union acpi_subtable_headers *hdr, void *arg,
> + const unsigned long unused)
> +{
> + struct acpi_cedt_cfmws *cfmws;
> + struct resource *res = arg;
> + struct resource cfmws_res;
> +
> + /* Validation check, remove when finished debugging */
> + if (!res->parent && res->end)
> + pr_err("CXL DEBUG Should insert %pr\n", res);
> +
> + if (res->parent || !res->end)
> + return 0;
> +
> + cfmws = (struct acpi_cedt_cfmws *)hdr;
> + cfmws_res = DEFINE_RES_MEM(cfmws->base_hpa,
> + cfmws->base_hpa + cfmws->window_size);
> + pr_err("CXL DEBUG Found CFMWS: %pr\n", &cfmws_res);
> +
> + if (resource_overlaps(&cfmws_res, res)) {
> + pr_err("CXL DEBUG Found SOFT RESERVE intersection %llx - %llx : %llx - %llx\n",
> + res->start, res->end, cfmws_res.start, cfmws_res.end);
> + e820__add_soft_reserve(res->start, resource_size(res),
> + res->flags);
> + return 1;
> + }
> +
> + return 0;
> +}
> +
> void __init e820__reserve_resources_late(void)
> {
> - int i;
> + int i, rc;
> struct resource *res;
>
> + /*
> + * Prior to inserting SOFT_RESERVED resources we want to check for an
> + * intersection with potential CXL resources. Any SOFT_RESERVED resources
> + * that do intersect a potential CXL resource are set aside so they
> + * can be trimmed to accommodate CXL resource intersections and added to
> + * the iomem resource tree after the CXL drivers have completed their
> + * device probe.
> + */
> + pr_err("CXL DEBUG Checking e820 iomem resources\n");
> +
> res = e820_res;
> for (i = 0; i < e820_table->nr_entries; i++) {
> - if (!res->parent && res->end)
> - insert_resource_expand_to_fit(&iomem_resource, res);
> + pr_err("CXL DEBUG Checking e820 iomem resource %llx - %llx\n",
> + res->start, res->end);
> + if (res->desc == IORES_DESC_SOFT_RESERVED) {
> + rc = acpi_table_parse_cedt(ACPI_CEDT_TYPE_CFMWS,
> + e820_parse_cfmws, res);
Can we make this somewhat generic by having a parse to find a match
then call
e820__add_soft_reserve(res->start, resource_size(res), res->flags);
if the match occurs. That way the check can be in arch independent code.
cxl_res_overlaps_cfmws() perhaps.
> + if (rc) {
> + res++;
Move the res++ into the for loop ... i++, res++)
> + continue;
> + }
> + }
> + pr_err("CXL DEBUG Inserting %llx - %llx\n", res->start, res->end);
> + insert_resource_expand_to_fit(&iomem_resource, res);
> res++;
> }
>
> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
> index d7d5d982ce69..9f94730c488f 100644
> --- a/drivers/cxl/port.c
> +++ b/drivers/cxl/port.c
> @@ -7,6 +7,10 @@
> #include "cxlmem.h"
> #include "cxlpci.h"
>
> +#if CONFIG_X86
> +#include <asm/e820/api.h>
> +#endif
> +
> /**
> * DOC: cxl port
> *
> @@ -89,6 +93,35 @@ static int cxl_switch_port_probe(struct cxl_port *port)
> return -ENXIO;
> }
>
> +DECLARE_RWSEM(cxl_sr_rwsem);
> +
> +static void cxl_sr_update(struct work_struct *w)
> +{
> + down_write(&cxl_sr_rwsem);
> + pr_err("CXL DEBUG Updating soft reserves\n");
> + e820__insert_soft_reserves();
That's arch specific code bleeding over to the cxl code.
So needs at minimum to be hidden behind an arch call.
> + up_write(&cxl_sr_rwsem);
> +}
> +
> +DECLARE_DELAYED_WORK(cxl_sr_work, cxl_sr_update);
> +
> +static void schedule_soft_reserve_update(void)
> +{
> + static bool update_scheduled;
> + int timeout = 5 * HZ;
> +
> + down_write(&cxl_sr_rwsem);
For the locks use guard()
However, what are they for?
> + if (update_scheduled) {
> + pr_err("CXL DEBUG modifying delayed work timeout\n");
> + mod_delayed_work(system_wq, &cxl_sr_work, timeout);
See docs for mod_delayed_work_on()
"If @dwork is idle, equivalent to queue_delayed_work_on(); "
which is where schedule_delayed_work() ends up.
So other than maybe for the debug prints you don't need this dance
just always call mod_delayed_work()
> + } else {
> + pr_err("CXL DEBUG Adding delayed work\n");
> + schedule_delayed_work(&cxl_sr_work, timeout);
> + update_scheduled = true;
> + }
> + up_write(&cxl_sr_rwsem);
> +}
> +
> static int cxl_endpoint_port_probe(struct cxl_port *port)
> {
> struct cxl_endpoint_dvsec_info info = { .port = port };
> @@ -140,6 +173,7 @@ static int cxl_endpoint_port_probe(struct cxl_port *port)
> */
> device_for_each_child(&port->dev, root, discover_region);
>
> + schedule_soft_reserve_update();
> return 0;
> }
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC] cxl: Update Soft Reserved resources upon region creation
2024-10-16 15:43 ` Jonathan Cameron
@ 2024-10-17 14:49 ` Fontenot, Nathan
2024-10-17 16:53 ` Jonathan Cameron
0 siblings, 1 reply; 8+ messages in thread
From: Fontenot, Nathan @ 2024-10-17 14:49 UTC (permalink / raw)
To: Jonathan Cameron, Nathan Fontenot
Cc: alison.schofield, dan.j.williams, linux-cxl
Hi Jonathan,
On 10/16/2024 10:43 AM, Jonathan Cameron wrote:
> On Fri, 4 Oct 2024 13:17:54 -0500
> Nathan Fontenot <nathan.fontenot@amd.com> wrote:
>
>> Update handling of SOFT RESERVE iomem resources that intersect with
>> CXL region resources to remove the intersections from the SOFT RESERVE
>> resources. The current approach of leaving the SOFT RESERVE
>> resource as is can cause failures during hotplug replace of CXL
>> devices because the resource is not available for reuse after
>> teardown.
> Good to give an example here + any debug error messages etc.
Good point. I'll include a better explanation of the failure with examples
in the next version.
>
>>
>> The approach sought is to trim out any pieces of SOFT RESERVE
>> resources that intersect with CXL regions. To do this, first set
>> aside any SOFT RESERVE resources that intersect with a CFMWS
>> into a separate resource tree during e820__reserve_resources_late()
>> that would have been otherwise added to the iomem resource tree.
>>
>> As CXL regions are created the cxl resource created for the new
>> region is used to trim intersections from the SOFT RESERVE
>> resources that were previously set aside.
>>
>> The next steps are to add any SOFT RESERVE resources remaining to the
>> iomem resource tree after CXL device probe completes and to notify
>> the dax driver so it may consume the added SOFT RESERVE resources.
>>
>> This patch includes the use of a delayed work queue to wait
>> for CXL device probe completion and then have a worker thread add
>> any remaining SOFT RESERVE resources to the iomem resource tree.
>>
>> Not in this patch is notification of the dax driver so it may consume
>> the SOFT RESERVE resources.
>>
>> The goal of presenting this RFC is to drive discussion of the
>> current approach for trimming SOFT RESERVE resources, the use of
>> a delayed work queue to add remaining SOFT RESERVE resources to
>> the iomem resource tree, and methods for notifying the dax driver
>> of any newly added SOFT RESERVE resources.
>>
>> NOTE: As this is a RFC the temporary pr_err("CXL DEBUG...") messages
>> have been left in to aid in testing and validation.
>>
>> Signed-off-by: Nathan Fontenot <nathan.fontenot@amd.com>
>> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
>
> Trivial, but why Alison's SoB? She didn't send the patch which would
> be normal reason for last SoB in a list like this (handling of the patch).
>
> Maybe a Co-developed-by is missing?
Alison helped develop the code so a Co-developed-by should be here.
>
> Whilst I appreciate this is an RFC, the arch specific bleed
> into CXL code needs to go. + we need some info here on whether this
> is an x86 specific issue. I'm fairly sure it's not and that should
> be called out as future work.
As far as I know this is an x86 issue for handling the SOFT RESERVE
resources, if other arches are seeing this than we need to update the approach.
>
> Also, much of this is actually generic, with only a few bits
> being e820 related. Can we pull the soft reserve list etc out
> of them into generic code.
Yes, we could do that. The handling of the soft reserve list doesn't
need to live in the e820 code.
>
> To me the general approach seems reasonable but I'd suggest +CC linux-mm
> as the issue maybe more about the impacts on DAX than this working for CXL.
ok, I'll add linux-mm. One of the pieces I wanted to get feedback on was the
dax component and how we could invoke the dax driver to consume any soft
reserves that remain after the cxl driver removes intersecting cxl region
resources.
>
> Comments inline are mostly general suggestions of simplifications
> and refactoring to aid readability.
>
>> ---
>> arch/x86/include/asm/e820/api.h | 3 +
>> arch/x86/kernel/e820.c | 156 +++++++++++++++++++++++++++++++-
>> drivers/cxl/core/region.c | 14 ++-
>> drivers/cxl/port.c | 34 +++++++
>> 4 files changed, 203 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/e820/api.h b/arch/x86/include/asm/e820/api.h
>> index 2e74a7f0e935..542a52871beb 100644
>> --- a/arch/x86/include/asm/e820/api.h
>> +++ b/arch/x86/include/asm/e820/api.h
>> @@ -44,6 +44,9 @@ extern void e820__register_nosave_regions(unsigned long limit_pfn);
>>
>> extern int e820__get_entry_type(u64 start, u64 end);
>>
>> +extern void e820__trim_soft_reserves(const struct resource *cxl_res);
>> +extern void e820__insert_soft_reserves(void);
>> +
>> /*
>> * Returns true iff the specified range [start,end) is completely contained inside
>> * the ISA region.
>> diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
>> index 4893d30ce438..855c26460bb9 100644
>> --- a/arch/x86/kernel/e820.c
>> +++ b/arch/x86/kernel/e820.c
>
>
>
>> +static void e820__trim_soft_reserve(struct resource *res,
>> + const struct resource *cxl_res)
>> +{
>> + resource_size_t new_start, new_end;
>> + int rc;
>> +
>> + pr_err("CXL DEBUG Trimming Soft Reserves for %pr\n", cxl_res);
>> +
>> + if (res->start == cxl_res->start && res->end == cxl_res->end) {
>> + pr_err("CXL DEBUG Releasing resource %pr\n", res);
>> + release_resource(res);
>> + kfree(res);
>> + } else if (res->start == cxl_res->start || res->end == cxl_res->end) {
>> + if (res->start == cxl_res->start) {
> Not a huge amount of sharing in here.
> } else if (res->start = cxl_res->start) {
> /* Left over bit at top end */
> rc = adjust_resource(res, cxl_res->end + 1, res->end - cxl_res->end);
> if (rc)...
>
> } else if (res->end == cxl_res->end) {
> /* Left over bit at bottom end */
> rc = adjust_resource(res, res->start, cxl_res->start - res->start);
> if (rc) ...
> } else {
> /* Left over bits on either side. */
> }
Yep, I'll update that.
>> + new_start = cxl_res->end + 1;
>> + new_end = res->end;
>> + } else {
>> + new_start = res->start;
>> + new_end = cxl_res->start - 1;
>> + }
>> +
>> + pr_err("CXL DEBUG Adjusting resource %pr (%llx - %llx)\n",
>> + res, new_start, new_end);
>> + rc = adjust_resource(res, new_start, new_end - new_start + 1);
>> + if (rc)
>> + pr_debug("Cannot adjust soft reserved resource %pr\n",
>> + res);
>> + } else {
>> + new_start = res->start;
>> + new_end = res->end;
> Confusing use of parameters here as they don't match use in early legs.
> Maybe just putting the values inline would be clearer.
will do.
>> +
>> + /*Adjust existing to beginning resource */
>> + pr_err("CXL DEBUG Adjusting resource %pr (%llx - %llx)\n", res,
>> + new_start, cxl_res->start);
>> + adjust_resource(res, new_start, cxl_res->start - new_start + 1);
>
> Off by 1? It's different form above leftover bottom part (should be the same).
I thought I got the math right, maybe I didn't. I'll re-run tests to and check it.
>
>
>> +
>> + /* Add new resource for end piece */
>> + e820__add_soft_reserve(cxl_res->end + 1, new_end - cxl_res->end,
>> + res->flags);
>> + }
>> +}
>> +
>> +void e820__trim_soft_reserves(const struct resource *cxl_res)
>> +{
>> + struct resource *res, *next;
>> +
>> + pr_err("CXL DEBUG Trimming Soft Reserves\n");
>> + for (res = e820_sr_res.child; res; res = next) {
> for (res = e820_sr_res.child; res; res = res->sibling)
> is the same I think and avoids need for next.
Yes, it should be. I'll update it.
>> + next = res->sibling;
>> +
>> + if (resource_contains(res, cxl_res)) {
>> + e820__trim_soft_reserve(res, cxl_res);
>> + break;
> return;
>
>> + }
>> + }
>> +}
>> +EXPORT_SYMBOL_GPL(e820__trim_soft_reserves);
>> +
>> +static int __init e820_parse_cfmws(union acpi_subtable_headers *hdr, void *arg,
>> + const unsigned long unused)
>> +{
>> + struct acpi_cedt_cfmws *cfmws;
>> + struct resource *res = arg;
>> + struct resource cfmws_res;
>> +
>> + /* Validation check, remove when finished debugging */
>> + if (!res->parent && res->end)
>> + pr_err("CXL DEBUG Should insert %pr\n", res);
>> +
>> + if (res->parent || !res->end)
>> + return 0;
>> +
>> + cfmws = (struct acpi_cedt_cfmws *)hdr;
>> + cfmws_res = DEFINE_RES_MEM(cfmws->base_hpa,
>> + cfmws->base_hpa + cfmws->window_size);
>> + pr_err("CXL DEBUG Found CFMWS: %pr\n", &cfmws_res);
>> +
>> + if (resource_overlaps(&cfmws_res, res)) {
>> + pr_err("CXL DEBUG Found SOFT RESERVE intersection %llx - %llx : %llx - %llx\n",
>> + res->start, res->end, cfmws_res.start, cfmws_res.end);
>> + e820__add_soft_reserve(res->start, resource_size(res),
>> + res->flags);
>> + return 1;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> void __init e820__reserve_resources_late(void)
>> {
>> - int i;
>> + int i, rc;
>> struct resource *res;
>>
>> + /*
>> + * Prior to inserting SOFT_RESERVED resources we want to check for an
>> + * intersection with potential CXL resources. Any SOFT_RESERVED resources
>> + * that do intersect a potential CXL resource are set aside so they
>> + * can be trimmed to accommodate CXL resource intersections and added to
>> + * the iomem resource tree after the CXL drivers have completed their
>> + * device probe.
>> + */
>> + pr_err("CXL DEBUG Checking e820 iomem resources\n");
>> +
>> res = e820_res;
>> for (i = 0; i < e820_table->nr_entries; i++) {
>> - if (!res->parent && res->end)
>> - insert_resource_expand_to_fit(&iomem_resource, res);
>> + pr_err("CXL DEBUG Checking e820 iomem resource %llx - %llx\n",
>> + res->start, res->end);
>> + if (res->desc == IORES_DESC_SOFT_RESERVED) {
>> + rc = acpi_table_parse_cedt(ACPI_CEDT_TYPE_CFMWS,
>> + e820_parse_cfmws, res);
> Can we make this somewhat generic by having a parse to find a match
> then call
> e820__add_soft_reserve(res->start, resource_size(res), res->flags);
> if the match occurs. That way the check can be in arch independent code.
>
> cxl_res_overlaps_cfmws() perhaps.
>
I like this better than what I have. I was never real crazy about having the
e820 code parsing the cfmws.
>
>> + if (rc) {
>> + res++;
>
> Move the res++ into the for loop ... i++, res++)
will do.
>
>> + continue;
>> + }
>> + }
>> + pr_err("CXL DEBUG Inserting %llx - %llx\n", res->start, res->end);
>> + insert_resource_expand_to_fit(&iomem_resource, res);
>> res++;
>> }
>>
>
>> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
>> index d7d5d982ce69..9f94730c488f 100644
>> --- a/drivers/cxl/port.c
>> +++ b/drivers/cxl/port.c
>> @@ -7,6 +7,10 @@
>> #include "cxlmem.h"
>> #include "cxlpci.h"
>>
>> +#if CONFIG_X86
>> +#include <asm/e820/api.h>
>> +#endif
>> +
>> /**
>> * DOC: cxl port
>> *
>> @@ -89,6 +93,35 @@ static int cxl_switch_port_probe(struct cxl_port *port)
>> return -ENXIO;
>> }
>>
>> +DECLARE_RWSEM(cxl_sr_rwsem);
>> +
>> +static void cxl_sr_update(struct work_struct *w)
>> +{
>> + down_write(&cxl_sr_rwsem);
>> + pr_err("CXL DEBUG Updating soft reserves\n");
>> + e820__insert_soft_reserves();
>
> That's arch specific code bleeding over to the cxl code.
> So needs at minimum to be hidden behind an arch call.
Agreed, keeping #ifdef <ARCH> out of the driver would be good. I'll look
at adding an arch call.
>
>> + up_write(&cxl_sr_rwsem);
>> +}
>> +
>> +DECLARE_DELAYED_WORK(cxl_sr_work, cxl_sr_update);
>> +
>> +static void schedule_soft_reserve_update(void)
>> +{
>> + static bool update_scheduled;
>> + int timeout = 5 * HZ;
>> +
>> + down_write(&cxl_sr_rwsem);
> For the locks use guard()
Yes can switch to guard().
>
> However, what are they for?
The concern is that we could have races on scheduling the soft reserve updates
and I wanted to try to keep only one work item on the queue. They may not be
needed, I was erring on the side of safety
>
>> + if (update_scheduled) {
>> + pr_err("CXL DEBUG modifying delayed work timeout\n");
>> + mod_delayed_work(system_wq, &cxl_sr_work, timeout);
>
> See docs for mod_delayed_work_on()
>
> "If @dwork is idle, equivalent to queue_delayed_work_on(); "
> which is where schedule_delayed_work() ends up.
> So other than maybe for the debug prints you don't need this dance
> just always call mod_delayed_work()
>
Thanks, I'll look into it.
...and thanks for all the review comments.
-Nathan
>
>
>> + } else {
>> + pr_err("CXL DEBUG Adding delayed work\n");
>> + schedule_delayed_work(&cxl_sr_work, timeout);
>> + update_scheduled = true;
>> + }
>> + up_write(&cxl_sr_rwsem);
>> +}
>> +
>> static int cxl_endpoint_port_probe(struct cxl_port *port)
>> {
>> struct cxl_endpoint_dvsec_info info = { .port = port };
>> @@ -140,6 +173,7 @@ static int cxl_endpoint_port_probe(struct cxl_port *port)
>> */
>> device_for_each_child(&port->dev, root, discover_region);
>>
>> + schedule_soft_reserve_update();
>> return 0;
>> }
>>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC] cxl: Update Soft Reserved resources upon region creation
2024-10-17 14:49 ` Fontenot, Nathan
@ 2024-10-17 16:53 ` Jonathan Cameron
2024-10-18 10:17 ` James Morse
0 siblings, 1 reply; 8+ messages in thread
From: Jonathan Cameron @ 2024-10-17 16:53 UTC (permalink / raw)
To: Fontenot, Nathan
Cc: Nathan Fontenot, alison.schofield, dan.j.williams, linux-cxl
On Thu, 17 Oct 2024 09:49:01 -0500
"Fontenot, Nathan" <nafonten@amd.com> wrote:
> Hi Jonathan,
Hi Nathan,
>
> On 10/16/2024 10:43 AM, Jonathan Cameron wrote:
> > On Fri, 4 Oct 2024 13:17:54 -0500
> > Nathan Fontenot <nathan.fontenot@amd.com> wrote:
> >
> >> Update handling of SOFT RESERVE iomem resources that intersect with
> >> CXL region resources to remove the intersections from the SOFT RESERVE
> >> resources. The current approach of leaving the SOFT RESERVE
> >> resource as is can cause failures during hotplug replace of CXL
> >> devices because the resource is not available for reuse after
> >> teardown.
> > Good to give an example here + any debug error messages etc.
>
> Good point. I'll include a better explanation of the failure with examples
> in the next version.
Thanks
>
> >
> >>
> >> The approach sought is to trim out any pieces of SOFT RESERVE
> >> resources that intersect with CXL regions. To do this, first set
> >> aside any SOFT RESERVE resources that intersect with a CFMWS
> >> into a separate resource tree during e820__reserve_resources_late()
> >> that would have been otherwise added to the iomem resource tree.
> >>
> >> As CXL regions are created the cxl resource created for the new
> >> region is used to trim intersections from the SOFT RESERVE
> >> resources that were previously set aside.
> >>
> >> The next steps are to add any SOFT RESERVE resources remaining to the
> >> iomem resource tree after CXL device probe completes and to notify
> >> the dax driver so it may consume the added SOFT RESERVE resources.
> >>
> >> This patch includes the use of a delayed work queue to wait
> >> for CXL device probe completion and then have a worker thread add
> >> any remaining SOFT RESERVE resources to the iomem resource tree.
> >>
> >> Not in this patch is notification of the dax driver so it may consume
> >> the SOFT RESERVE resources.
> >>
> >> The goal of presenting this RFC is to drive discussion of the
> >> current approach for trimming SOFT RESERVE resources, the use of
> >> a delayed work queue to add remaining SOFT RESERVE resources to
> >> the iomem resource tree, and methods for notifying the dax driver
> >> of any newly added SOFT RESERVE resources.
> >>
> >> NOTE: As this is a RFC the temporary pr_err("CXL DEBUG...") messages
> >> have been left in to aid in testing and validation.
> >>
> >> Signed-off-by: Nathan Fontenot <nathan.fontenot@amd.com>
> >> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> >
> > Trivial, but why Alison's SoB? She didn't send the patch which would
> > be normal reason for last SoB in a list like this (handling of the patch).
> >
> > Maybe a Co-developed-by is missing?
>
> Alison helped develop the code so a Co-developed-by should be here.
>
> >
> > Whilst I appreciate this is an RFC, the arch specific bleed
> > into CXL code needs to go. + we need some info here on whether this
> > is an x86 specific issue. I'm fairly sure it's not and that should
> > be called out as future work.
>
> As far as I know this is an x86 issue for handling the SOFT RESERVE
> resources, if other arches are seeing this than we need to update the approach.
Hmm. I've not fully understood where it is tripping up in the x86 code.
Any EFI table using arch (arm64 etc) has soft reserved handling so
a bios doing CXL enumeration might have chosen to mark the CXL memory
with that.
The rest of us just don't bounce through the ancient history of e820,
It's more than possible that other architectures don't care that
it was previously soft reserved on hotplug paths. I'm not sure.
I guess I could hack a test but it will be a while...
>
> >
> > Also, much of this is actually generic, with only a few bits
> > being e820 related. Can we pull the soft reserve list etc out
> > of them into generic code.
>
> Yes, we could do that. The handling of the soft reserve list doesn't
> need to live in the e820 code.
>
> >
> > To me the general approach seems reasonable but I'd suggest +CC linux-mm
> > as the issue maybe more about the impacts on DAX than this working for CXL.
>
> ok, I'll add linux-mm. One of the pieces I wanted to get feedback on was the
> dax component and how we could invoke the dax driver to consume any soft
> reserves that remain after the cxl driver removes intersecting cxl region
> resources.
Understood. That bit I can't help with and we all know everyone else
will back away faster than Dan Williams :)
> >
> >> + up_write(&cxl_sr_rwsem);
> >> +}
> >> +
> >> +DECLARE_DELAYED_WORK(cxl_sr_work, cxl_sr_update);
> >> +
> >> +static void schedule_soft_reserve_update(void)
> >> +{
> >> + static bool update_scheduled;
> >> + int timeout = 5 * HZ;
> >> +
> >> + down_write(&cxl_sr_rwsem);
> > For the locks use guard()
>
> Yes can switch to guard().
>
> >
> > However, what are they for?
>
> The concern is that we could have races on scheduling the soft reserve updates
> and I wanted to try to keep only one work item on the queue. They may not be
> needed, I was erring on the side of safety
You only have one item, so it can't be there twice. If the aim is to protec
that one item then maybe this makes sense.
Jonathan
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC] cxl: Update Soft Reserved resources upon region creation
2024-10-17 16:53 ` Jonathan Cameron
@ 2024-10-18 10:17 ` James Morse
0 siblings, 0 replies; 8+ messages in thread
From: James Morse @ 2024-10-18 10:17 UTC (permalink / raw)
To: Jonathan Cameron, Fontenot, Nathan
Cc: Nathan Fontenot, alison.schofield, dan.j.williams, linux-cxl
Hi guys,
On 17/10/2024 17:53, Jonathan Cameron wrote:
> On Thu, 17 Oct 2024 09:49:01 -0500
> "Fontenot, Nathan" <nafonten@amd.com> wrote:
>> On 10/16/2024 10:43 AM, Jonathan Cameron wrote:
>>> On Fri, 4 Oct 2024 13:17:54 -0500
>>> Nathan Fontenot <nathan.fontenot@amd.com> wrote:
>>>
>>>> Update handling of SOFT RESERVE iomem resources that intersect with
>>>> CXL region resources to remove the intersections from the SOFT RESERVE
>>>> resources. The current approach of leaving the SOFT RESERVE
>>>> resource as is can cause failures during hotplug replace of CXL
>>>> devices because the resource is not available for reuse after
>>>> teardown.
>>>> The approach sought is to trim out any pieces of SOFT RESERVE
>>>> resources that intersect with CXL regions. To do this, first set
>>>> aside any SOFT RESERVE resources that intersect with a CFMWS
>>>> into a separate resource tree during e820__reserve_resources_late()
>>>> that would have been otherwise added to the iomem resource tree.
>>>>
>>>> As CXL regions are created the cxl resource created for the new
>>>> region is used to trim intersections from the SOFT RESERVE
>>>> resources that were previously set aside.
>>>>
>>>> The next steps are to add any SOFT RESERVE resources remaining to the
>>>> iomem resource tree after CXL device probe completes and to notify
>>>> the dax driver so it may consume the added SOFT RESERVE resources.
>>>>
>>>> This patch includes the use of a delayed work queue to wait
>>>> for CXL device probe completion and then have a worker thread add
>>>> any remaining SOFT RESERVE resources to the iomem resource tree.
>>>>
>>>> Not in this patch is notification of the dax driver so it may consume
>>>> the SOFT RESERVE resources.
>>>>
>>>> The goal of presenting this RFC is to drive discussion of the
>>>> current approach for trimming SOFT RESERVE resources, the use of
>>>> a delayed work queue to add remaining SOFT RESERVE resources to
>>>> the iomem resource tree, and methods for notifying the dax driver
>>>> of any newly added SOFT RESERVE resources.
>>> Whilst I appreciate this is an RFC, the arch specific bleed
>>> into CXL code needs to go. + we need some info here on whether this
>>> is an x86 specific issue. I'm fairly sure it's not and that should
>>> be called out as future work.
>>
>> As far as I know this is an x86 issue for handling the SOFT RESERVE
>> resources, if other arches are seeing this than we need to update the approach.
>
> Hmm. I've not fully understood where it is tripping up in the x86 code.
> Any EFI table using arch (arm64 etc) has soft reserved handling so
> a bios doing CXL enumeration might have chosen to mark the CXL memory
> with that.
>
> The rest of us just don't bounce through the ancient history of e820,
>
> It's more than possible that other architectures don't care that
> it was previously soft reserved on hotplug paths. I'm not sure.
> I guess I could hack a test but it will be a while...
arm64 will try to block removal of memory that was described in the UEFI memory map.
The reason is we can't modify the UEFI tables when they have bits we don't understand.
(Should we preserve those bits if the memory comes back?)
On a UEFI platform, the memory map is the only information about where memory is that we
have, we can't fix it up with other things we know like x86 does, (or stir in another
memory map we found near the ISA hole)
These tables are treated as the static properties of the system that cannot be changed and
is just handed over during kexec. (This also means we don't have to work around the
lovecraftian matrix of bugs that could be introduced by kexecing via older kernels)
To remove memory on arm64, it should have been detected via one of the hotplug mechanisms.
I think this removal of early boot memory is going to be specific to x86.
Thanks,
James
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2024-10-18 10:17 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-10-04 18:17 [RFC] cxl: Update Soft Reserved resources upon region creation Nathan Fontenot
2024-10-07 14:53 ` kernel test robot
2024-10-07 15:04 ` kernel test robot
2024-10-07 19:30 ` kernel test robot
2024-10-16 15:43 ` Jonathan Cameron
2024-10-17 14:49 ` Fontenot, Nathan
2024-10-17 16:53 ` Jonathan Cameron
2024-10-18 10:17 ` James Morse
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.