| Message ID | 20220413183720.2444089-7-ben.widawsky@intel.com |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Region driver | expand |
On Wed, Apr 13, 2022 at 11:38 AM Ben Widawsky <ben.widawsky@intel.com> wrote: > > Use a gen_pool to manage the physical address space that is routed by > the platform decoder (root decoder). As described in 'cxl/acpi: Resereve > CXL resources from request_free_mem_region' the address space does not > coexist well if part of all of it is conveyed in the memory map to the > kernel. > > Since the existing resource APIs of interest all rely on the root > decoder's address space being in iomem_resource, I do not understand what this is trying to convey. Nothing requires that a given 'struct resource' be managed under iomem_resource. > the choices are to roll > a new allocator because on struct resource, or use gen_pool. gen_pool is > a good choice because it already has all the capabilities needed to > satisfy CXL programming. Not sure what comparison to 'struct resource' is being made here, what is the tradeoff as you see it? In other words, why mention 'struct resource' as a consideration? > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> > --- > drivers/cxl/acpi.c | 36 ++++++++++++++++++++++++++++++++++++ > drivers/cxl/cxl.h | 2 ++ > 2 files changed, 38 insertions(+) > > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c > index 0870904fe4b5..a6b0c3181d0e 100644 > --- a/drivers/cxl/acpi.c > +++ b/drivers/cxl/acpi.c > @@ -1,6 +1,7 @@ > // SPDX-License-Identifier: GPL-2.0-only > /* Copyright(c) 2021 Intel Corporation. All rights reserved. */ > #include <linux/platform_device.h> > +#include <linux/genalloc.h> > #include <linux/module.h> > #include <linux/device.h> > #include <linux/kernel.h> > @@ -79,6 +80,25 @@ struct cxl_cfmws_context { > struct acpi_cedt_cfmws *high_cfmws; > }; > > +static int cfmws_cookie; > + > +static int fill_busy_mem(struct resource *res, void *_window) > +{ > + struct gen_pool *window = _window; > + struct genpool_data_fixed gpdf; > + unsigned long addr; > + void *type; > + > + gpdf.offset = res->start; > + addr = gen_pool_alloc_algo_owner(window, resource_size(res), > + gen_pool_fixed_alloc, &gpdf, &type); The "_owner" variant of gen_pool was only added for p2pdma as a way to coordinate reference counts across p2pdma space allocation and a 'strcuct dev_pagemap' instance. The use here seems completely vestigial and can just move to gen_pool_alloc_algo. > + if (addr != res->start || (res->start == 0 && type != &cfmws_cookie)) > + return -ENXIO; How can the second condition ever be true? > + > + pr_devel("%pR removed from CFMWS\n", res); > + return 0; > +} > + > static int cxl_parse_cfmws(union acpi_subtable_headers *header, void *arg, > const unsigned long end) > { > @@ -88,6 +108,8 @@ static int cxl_parse_cfmws(union acpi_subtable_headers *header, void *arg, > struct device *dev = ctx->dev; > struct acpi_cedt_cfmws *cfmws; > struct cxl_decoder *cxld; > + struct gen_pool *window; > + char name[64]; > int rc, i; > > cfmws = (struct acpi_cedt_cfmws *) header; > @@ -116,6 +138,20 @@ static int cxl_parse_cfmws(union acpi_subtable_headers *header, void *arg, > cxld->interleave_ways = CFMWS_INTERLEAVE_WAYS(cfmws); > cxld->interleave_granularity = CFMWS_INTERLEAVE_GRANULARITY(cfmws); > > + sprintf(name, "cfmws@%#llx", cfmws->base_hpa); > + window = devm_gen_pool_create(dev, ilog2(SZ_256M), NUMA_NO_NODE, name); > + if (IS_ERR(window)) > + return 0; > + > + gen_pool_add_owner(window, cfmws->base_hpa, -1, cfmws->window_size, > + NUMA_NO_NODE, &cfmws_cookie); Similar comment about the "_owner" variant serving no visible purpose. These seems to pre-suppose that only the allocator will ever want to interrogate the state of free space, it might be worth registering objects for each intersection that are not cxl_regions so that userspace explicitly sees what the cxl_acpi driver sees in terms of available resources. > + > + /* Area claimed by other resources, remove those from the gen_pool. */ > + walk_iomem_res_desc(IORES_DESC_NONE, 0, cfmws->base_hpa, > + cfmws->base_hpa + cfmws->window_size - 1, window, > + fill_busy_mem); > + to_cxl_root_decoder(cxld)->window = window; > + > rc = cxl_decoder_add(cxld, target_map); > if (rc) > put_device(&cxld->dev); > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > index 85fd5e84f978..0e1c65761ead 100644 > --- a/drivers/cxl/cxl.h > +++ b/drivers/cxl/cxl.h > @@ -246,10 +246,12 @@ struct cxl_switch_decoder { > /** > * struct cxl_root_decoder - A toplevel/platform decoder > * @base: Base class decoder > + * @window: host address space allocator > * @targets: Downstream targets (ie. hostbridges). > */ > struct cxl_root_decoder { > struct cxl_decoder base; > + struct gen_pool *window; > struct cxl_decoder_targets *targets; > }; > > -- > 2.35.1 >
On 22-04-18 15:15:47, Dan Williams wrote: > On Wed, Apr 13, 2022 at 11:38 AM Ben Widawsky <ben.widawsky@intel.com> wrote: > > > > Use a gen_pool to manage the physical address space that is routed by > > the platform decoder (root decoder). As described in 'cxl/acpi: Resereve > > CXL resources from request_free_mem_region' the address space does not > > coexist well if part of all of it is conveyed in the memory map to the > > kernel. > > > > Since the existing resource APIs of interest all rely on the root > > decoder's address space being in iomem_resource, > > I do not understand what this is trying to convey. Nothing requires > that a given 'struct resource' be managed under iomem_resource. > > > the choices are to roll > > a new allocator because on struct resource, or use gen_pool. gen_pool is > > a good choice because it already has all the capabilities needed to > > satisfy CXL programming. > > Not sure what comparison to 'struct resource' is being made here, what > is the tradeoff as you see it? In other words, why mention 'struct > resource' as a consideration? > > > > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> > > --- > > drivers/cxl/acpi.c | 36 ++++++++++++++++++++++++++++++++++++ > > drivers/cxl/cxl.h | 2 ++ > > 2 files changed, 38 insertions(+) > > > > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c > > index 0870904fe4b5..a6b0c3181d0e 100644 > > --- a/drivers/cxl/acpi.c > > +++ b/drivers/cxl/acpi.c > > @@ -1,6 +1,7 @@ > > // SPDX-License-Identifier: GPL-2.0-only > > /* Copyright(c) 2021 Intel Corporation. All rights reserved. */ > > #include <linux/platform_device.h> > > +#include <linux/genalloc.h> > > #include <linux/module.h> > > #include <linux/device.h> > > #include <linux/kernel.h> > > @@ -79,6 +80,25 @@ struct cxl_cfmws_context { > > struct acpi_cedt_cfmws *high_cfmws; > > }; > > > > +static int cfmws_cookie; > > + > > +static int fill_busy_mem(struct resource *res, void *_window) > > +{ > > + struct gen_pool *window = _window; > > + struct genpool_data_fixed gpdf; > > + unsigned long addr; > > + void *type; > > + > > + gpdf.offset = res->start; > > + addr = gen_pool_alloc_algo_owner(window, resource_size(res), > > + gen_pool_fixed_alloc, &gpdf, &type); > > The "_owner" variant of gen_pool was only added for p2pdma as a way to > coordinate reference counts across p2pdma space allocation and a > 'strcuct dev_pagemap' instance. The use here seems completely > vestigial and can just move to gen_pool_alloc_algo. > The problem that it's trying to solve is for the case when gpdf.offset is 0. I think that's a highly unlikely case with the current plan, however, if reparenting comes into play, you could very likely have offset 0, and then you have no way to distinguish error from success without the cookie. Thoughts? > > + if (addr != res->start || (res->start == 0 && type != &cfmws_cookie)) > > + return -ENXIO; > > How can the second condition ever be true? > 0 offset but failure. > > + > > + pr_devel("%pR removed from CFMWS\n", res); > > + return 0; > > +} > > + > > static int cxl_parse_cfmws(union acpi_subtable_headers *header, void *arg, > > const unsigned long end) > > { > > @@ -88,6 +108,8 @@ static int cxl_parse_cfmws(union acpi_subtable_headers *header, void *arg, > > struct device *dev = ctx->dev; > > struct acpi_cedt_cfmws *cfmws; > > struct cxl_decoder *cxld; > > + struct gen_pool *window; > > + char name[64]; > > int rc, i; > > > > cfmws = (struct acpi_cedt_cfmws *) header; > > @@ -116,6 +138,20 @@ static int cxl_parse_cfmws(union acpi_subtable_headers *header, void *arg, > > cxld->interleave_ways = CFMWS_INTERLEAVE_WAYS(cfmws); > > cxld->interleave_granularity = CFMWS_INTERLEAVE_GRANULARITY(cfmws); > > > > + sprintf(name, "cfmws@%#llx", cfmws->base_hpa); > > + window = devm_gen_pool_create(dev, ilog2(SZ_256M), NUMA_NO_NODE, name); > > + if (IS_ERR(window)) > > + return 0; > > + > > + gen_pool_add_owner(window, cfmws->base_hpa, -1, cfmws->window_size, > > + NUMA_NO_NODE, &cfmws_cookie); > > Similar comment about the "_owner" variant serving no visible purpose. > > These seems to pre-suppose that only the allocator will ever want to > interrogate the state of free space, it might be worth registering > objects for each intersection that are not cxl_regions so that > userspace explicitly sees what the cxl_acpi driver sees in terms of > available resources. > > > + > > + /* Area claimed by other resources, remove those from the gen_pool. */ > > + walk_iomem_res_desc(IORES_DESC_NONE, 0, cfmws->base_hpa, > > + cfmws->base_hpa + cfmws->window_size - 1, window, > > + fill_busy_mem); > > + to_cxl_root_decoder(cxld)->window = window; > > + > > rc = cxl_decoder_add(cxld, target_map); > > if (rc) > > put_device(&cxld->dev); > > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > > index 85fd5e84f978..0e1c65761ead 100644 > > --- a/drivers/cxl/cxl.h > > +++ b/drivers/cxl/cxl.h > > @@ -246,10 +246,12 @@ struct cxl_switch_decoder { > > /** > > * struct cxl_root_decoder - A toplevel/platform decoder > > * @base: Base class decoder > > + * @window: host address space allocator > > * @targets: Downstream targets (ie. hostbridges). > > */ > > struct cxl_root_decoder { > > struct cxl_decoder base; > > + struct gen_pool *window; > > struct cxl_decoder_targets *targets; > > }; > > > > -- > > 2.35.1 > >
diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c index 0870904fe4b5..a6b0c3181d0e 100644 --- a/drivers/cxl/acpi.c +++ b/drivers/cxl/acpi.c @@ -1,6 +1,7 @@ // SPDX-License-Identifier: GPL-2.0-only /* Copyright(c) 2021 Intel Corporation. All rights reserved. */ #include <linux/platform_device.h> +#include <linux/genalloc.h> #include <linux/module.h> #include <linux/device.h> #include <linux/kernel.h> @@ -79,6 +80,25 @@ struct cxl_cfmws_context { struct acpi_cedt_cfmws *high_cfmws; }; +static int cfmws_cookie; + +static int fill_busy_mem(struct resource *res, void *_window) +{ + struct gen_pool *window = _window; + struct genpool_data_fixed gpdf; + unsigned long addr; + void *type; + + gpdf.offset = res->start; + addr = gen_pool_alloc_algo_owner(window, resource_size(res), + gen_pool_fixed_alloc, &gpdf, &type); + if (addr != res->start || (res->start == 0 && type != &cfmws_cookie)) + return -ENXIO; + + pr_devel("%pR removed from CFMWS\n", res); + return 0; +} + static int cxl_parse_cfmws(union acpi_subtable_headers *header, void *arg, const unsigned long end) { @@ -88,6 +108,8 @@ static int cxl_parse_cfmws(union acpi_subtable_headers *header, void *arg, struct device *dev = ctx->dev; struct acpi_cedt_cfmws *cfmws; struct cxl_decoder *cxld; + struct gen_pool *window; + char name[64]; int rc, i; cfmws = (struct acpi_cedt_cfmws *) header; @@ -116,6 +138,20 @@ static int cxl_parse_cfmws(union acpi_subtable_headers *header, void *arg, cxld->interleave_ways = CFMWS_INTERLEAVE_WAYS(cfmws); cxld->interleave_granularity = CFMWS_INTERLEAVE_GRANULARITY(cfmws); + sprintf(name, "cfmws@%#llx", cfmws->base_hpa); + window = devm_gen_pool_create(dev, ilog2(SZ_256M), NUMA_NO_NODE, name); + if (IS_ERR(window)) + return 0; + + gen_pool_add_owner(window, cfmws->base_hpa, -1, cfmws->window_size, + NUMA_NO_NODE, &cfmws_cookie); + + /* Area claimed by other resources, remove those from the gen_pool. */ + walk_iomem_res_desc(IORES_DESC_NONE, 0, cfmws->base_hpa, + cfmws->base_hpa + cfmws->window_size - 1, window, + fill_busy_mem); + to_cxl_root_decoder(cxld)->window = window; + rc = cxl_decoder_add(cxld, target_map); if (rc) put_device(&cxld->dev); diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h index 85fd5e84f978..0e1c65761ead 100644 --- a/drivers/cxl/cxl.h +++ b/drivers/cxl/cxl.h @@ -246,10 +246,12 @@ struct cxl_switch_decoder { /** * struct cxl_root_decoder - A toplevel/platform decoder * @base: Base class decoder + * @window: host address space allocator * @targets: Downstream targets (ie. hostbridges). */ struct cxl_root_decoder { struct cxl_decoder base; + struct gen_pool *window; struct cxl_decoder_targets *targets; };
Use a gen_pool to manage the physical address space that is routed by the platform decoder (root decoder). As described in 'cxl/acpi: Resereve CXL resources from request_free_mem_region' the address space does not coexist well if part of all of it is conveyed in the memory map to the kernel. Since the existing resource APIs of interest all rely on the root decoder's address space being in iomem_resource, the choices are to roll a new allocator because on struct resource, or use gen_pool. gen_pool is a good choice because it already has all the capabilities needed to satisfy CXL programming. Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> --- drivers/cxl/acpi.c | 36 ++++++++++++++++++++++++++++++++++++ drivers/cxl/cxl.h | 2 ++ 2 files changed, 38 insertions(+)