| Message ID | 20211022183709.1199701-6-ben.widawsky@intel.com |
|---|---|
| State | New, archived |
| Headers | show |
| Series | CXL Region Creation / HDM decoder programming | expand |
On Fri, Oct 22, 2021 at 11:37 AM Ben Widawsky <ben.widawsky@intel.com> wrote: > > Regions will use the resource API in order to help manage allocated > space. As regions are children of the decoder, it makes sense that the > parent host the main resource to be suballocated by the region. > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> > --- > drivers/cxl/acpi.c | 12 ++++-------- > drivers/cxl/core/bus.c | 4 ++-- > drivers/cxl/cxl.h | 4 ++-- > 3 files changed, 8 insertions(+), 12 deletions(-) > > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c > index 7d13e7f0aefc..b972abc9f6ef 100644 > --- a/drivers/cxl/acpi.c > +++ b/drivers/cxl/acpi.c > @@ -126,10 +126,9 @@ static void cxl_add_cfmws_decoders(struct device *dev, > > cxld->flags = cfmws_to_decoder_flags(cfmws->restrictions); > cxld->target_type = CXL_DECODER_EXPANDER; > - cxld->range = (struct range) { > - .start = cfmws->base_hpa, > - .end = cfmws->base_hpa + cfmws->window_size - 1, > - }; > + cxld->res = (struct resource)DEFINE_RES_MEM_NAMED(cfmws->base_hpa, > + cfmws->window_size, > + "cfmws"); I think this should just be DEFINE_RES_MEM(), and then set the name of it inside cxl_decoder_add() to the dev_name() of the decoder device. That way a dump of the resource tree hierarchy makes sense compared to the device hierarchy with actual device names not a series of repeating "cfmws" entries. > cxld->interleave_ways = CFMWS_INTERLEAVE_WAYS(cfmws); > cxld->interleave_granularity = > CFMWS_INTERLEAVE_GRANULARITY(cfmws); > @@ -339,10 +338,7 @@ static int add_host_bridge_uport(struct device *match, void *arg) > cxld->interleave_ways = 1; > cxld->interleave_granularity = PAGE_SIZE; > cxld->target_type = CXL_DECODER_EXPANDER; > - cxld->range = (struct range) { > - .start = 0, > - .end = -1, > - }; > + cxld->res = (struct resource)DEFINE_RES_MEM(0, 0); > > device_lock(&port->dev); > dport = list_first_entry(&port->dports, typeof(*dport), list); > diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c > index ebd061d03950..454d4d846eb2 100644 > --- a/drivers/cxl/core/bus.c > +++ b/drivers/cxl/core/bus.c > @@ -47,7 +47,7 @@ static ssize_t start_show(struct device *dev, struct device_attribute *attr, > { > struct cxl_decoder *cxld = to_cxl_decoder(dev); > > - return sysfs_emit(buf, "%#llx\n", cxld->range.start); > + return sysfs_emit(buf, "%#llx\n", cxld->res.start); > } > static DEVICE_ATTR_RO(start); > > @@ -56,7 +56,7 @@ static ssize_t size_show(struct device *dev, struct device_attribute *attr, > { > struct cxl_decoder *cxld = to_cxl_decoder(dev); > > - return sysfs_emit(buf, "%#llx\n", range_len(&cxld->range)); > + return sysfs_emit(buf, "%#llx\n", resource_size(&cxld->res)); It's not clear to me that anything other than root decoders will host a resource tree, every decoder downstream of the root would reference a subset of the root decoder range. Something like: diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h index 5e2e93451928..00bf742396e0 100644 --- a/drivers/cxl/cxl.h +++ b/drivers/cxl/cxl.h @@ -200,7 +200,8 @@ enum cxl_decoder_type { * struct cxl_decoder - CXL address range decode configuration * @dev: this decoder's device * @id: kernel device name id - * @range: address range considered by this decoder + * @res: platform address range hosted by a root decoder + * @range: address range programmed in the (non-root) decoder * @interleave_ways: number of cxl_dports in this decode * @interleave_granularity: data stride per dport * @target_type: accelerator vs expander (type2 vs type3) selector @@ -211,7 +212,10 @@ enum cxl_decoder_type { struct cxl_decoder { struct device dev; int id; - struct range range; + union { + struct resource res; + struct range range; + }; int interleave_ways; int interleave_granularity; enum cxl_decoder_type target_type; ...because regions will __request_region(&cxld->res, ...) from the root decoder, and all the intervening decoders in that stack will be updated to make that mapping happen. Note, this is just how I had it roughly mapped out in my head, I defer making it a hard recommendation until I get deeper into this set to see if we diverge. > } > static DEVICE_ATTR_RO(size); > > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > index 4483e1a39fc3..7f2e2bdc7883 100644 > --- a/drivers/cxl/cxl.h > +++ b/drivers/cxl/cxl.h > @@ -195,7 +195,7 @@ enum cxl_decoder_type { > * struct cxl_decoder - CXL address range decode configuration > * @dev: this decoder's device > * @id: kernel device name id > - * @range: address range considered by this decoder > + * @res: address space resources considered by this decoder > * @interleave_ways: number of cxl_dports in this decode > * @interleave_granularity: data stride per dport > * @target_type: accelerator vs expander (type2 vs type3) selector > @@ -206,7 +206,7 @@ enum cxl_decoder_type { > struct cxl_decoder { > struct device dev; > int id; > - struct range range; > + struct resource res; > int interleave_ways; > int interleave_granularity; > enum cxl_decoder_type target_type; > -- > 2.33.1 >
On 21-10-29 13:50:29, Dan Williams wrote: > On Fri, Oct 22, 2021 at 11:37 AM Ben Widawsky <ben.widawsky@intel.com> wrote: > > > > Regions will use the resource API in order to help manage allocated > > space. As regions are children of the decoder, it makes sense that the > > parent host the main resource to be suballocated by the region. > > > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> > > --- > > drivers/cxl/acpi.c | 12 ++++-------- > > drivers/cxl/core/bus.c | 4 ++-- > > drivers/cxl/cxl.h | 4 ++-- > > 3 files changed, 8 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c > > index 7d13e7f0aefc..b972abc9f6ef 100644 > > --- a/drivers/cxl/acpi.c > > +++ b/drivers/cxl/acpi.c > > @@ -126,10 +126,9 @@ static void cxl_add_cfmws_decoders(struct device *dev, > > > > cxld->flags = cfmws_to_decoder_flags(cfmws->restrictions); > > cxld->target_type = CXL_DECODER_EXPANDER; > > - cxld->range = (struct range) { > > - .start = cfmws->base_hpa, > > - .end = cfmws->base_hpa + cfmws->window_size - 1, > > - }; > > + cxld->res = (struct resource)DEFINE_RES_MEM_NAMED(cfmws->base_hpa, > > + cfmws->window_size, > > + "cfmws"); > > I think this should just be DEFINE_RES_MEM(), and then set the name of > it inside cxl_decoder_add() to the dev_name() of the decoder device. > That way a dump of the resource tree hierarchy makes sense compared to > the device hierarchy with actual device names not a series of > repeating "cfmws" entries. > > > cxld->interleave_ways = CFMWS_INTERLEAVE_WAYS(cfmws); > > cxld->interleave_granularity = > > CFMWS_INTERLEAVE_GRANULARITY(cfmws); > > @@ -339,10 +338,7 @@ static int add_host_bridge_uport(struct device *match, void *arg) > > cxld->interleave_ways = 1; > > cxld->interleave_granularity = PAGE_SIZE; > > cxld->target_type = CXL_DECODER_EXPANDER; > > - cxld->range = (struct range) { > > - .start = 0, > > - .end = -1, > > - }; > > + cxld->res = (struct resource)DEFINE_RES_MEM(0, 0); > > > > device_lock(&port->dev); > > dport = list_first_entry(&port->dports, typeof(*dport), list); > > diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c > > index ebd061d03950..454d4d846eb2 100644 > > --- a/drivers/cxl/core/bus.c > > +++ b/drivers/cxl/core/bus.c > > @@ -47,7 +47,7 @@ static ssize_t start_show(struct device *dev, struct device_attribute *attr, > > { > > struct cxl_decoder *cxld = to_cxl_decoder(dev); > > > > - return sysfs_emit(buf, "%#llx\n", cxld->range.start); > > + return sysfs_emit(buf, "%#llx\n", cxld->res.start); > > } > > static DEVICE_ATTR_RO(start); > > > > @@ -56,7 +56,7 @@ static ssize_t size_show(struct device *dev, struct device_attribute *attr, > > { > > struct cxl_decoder *cxld = to_cxl_decoder(dev); > > > > - return sysfs_emit(buf, "%#llx\n", range_len(&cxld->range)); > > + return sysfs_emit(buf, "%#llx\n", resource_size(&cxld->res)); > > It's not clear to me that anything other than root decoders will host > a resource tree, every decoder downstream of the root would reference > a subset of the root decoder range. Something like: > > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > index 5e2e93451928..00bf742396e0 100644 > --- a/drivers/cxl/cxl.h > +++ b/drivers/cxl/cxl.h > @@ -200,7 +200,8 @@ enum cxl_decoder_type { > * struct cxl_decoder - CXL address range decode configuration > * @dev: this decoder's device > * @id: kernel device name id > - * @range: address range considered by this decoder > + * @res: platform address range hosted by a root decoder > + * @range: address range programmed in the (non-root) decoder > * @interleave_ways: number of cxl_dports in this decode > * @interleave_granularity: data stride per dport > * @target_type: accelerator vs expander (type2 vs type3) selector > @@ -211,7 +212,10 @@ enum cxl_decoder_type { > struct cxl_decoder { > struct device dev; > int id; > - struct range range; > + union { > + struct resource res; > + struct range range; > + }; > int interleave_ways; > int interleave_granularity; > enum cxl_decoder_type target_type; > > ...because regions will __request_region(&cxld->res, ...) from the > root decoder, and all the intervening decoders in that stack will be > updated to make that mapping happen. > > Note, this is just how I had it roughly mapped out in my head, I defer > making it a hard recommendation until I get deeper into this set to > see if we diverge. > You're correct. I had changed this already for my v3. I ended up with this: + union { + struct resource cfmws_res; + struct resource *decoder_res; + }; Not sure if you prefer one way or another. > > } > > static DEVICE_ATTR_RO(size); > > > > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > > index 4483e1a39fc3..7f2e2bdc7883 100644 > > --- a/drivers/cxl/cxl.h > > +++ b/drivers/cxl/cxl.h > > @@ -195,7 +195,7 @@ enum cxl_decoder_type { > > * struct cxl_decoder - CXL address range decode configuration > > * @dev: this decoder's device > > * @id: kernel device name id > > - * @range: address range considered by this decoder > > + * @res: address space resources considered by this decoder > > * @interleave_ways: number of cxl_dports in this decode > > * @interleave_granularity: data stride per dport > > * @target_type: accelerator vs expander (type2 vs type3) selector > > @@ -206,7 +206,7 @@ enum cxl_decoder_type { > > struct cxl_decoder { > > struct device dev; > > int id; > > - struct range range; > > + struct resource res; > > int interleave_ways; > > int interleave_granularity; > > enum cxl_decoder_type target_type; > > -- > > 2.33.1 > >
On Fri, Oct 29, 2021 at 2:27 PM Ben Widawsky <ben.widawsky@intel.com> wrote: [..] > > Note, this is just how I had it roughly mapped out in my head, I defer > > making it a hard recommendation until I get deeper into this set to > > see if we diverge. > > > > You're correct. > > I had changed this already for my v3. I ended up with this: > + union { > + struct resource cfmws_res; "cfmws" is something that will only appear on ACPI based CXL systems, so if it needs a prefix it should be something that does not preclude CXL on other platform firmware architectures. So, "platform_" or "root_"? > + struct resource *decoder_res; So the region device is going to be doing the __request_region() from @cfmws_res, not decoders, so why is this a resource and not a range?
On 21-10-29 15:22:19, Dan Williams wrote: > On Fri, Oct 29, 2021 at 2:27 PM Ben Widawsky <ben.widawsky@intel.com> wrote: > [..] > > > Note, this is just how I had it roughly mapped out in my head, I defer > > > making it a hard recommendation until I get deeper into this set to > > > see if we diverge. > > > > > > > You're correct. > > > > I had changed this already for my v3. I ended up with this: > > + union { > > + struct resource cfmws_res; > > "cfmws" is something that will only appear on ACPI based CXL systems, > so if it needs a prefix it should be something that does not preclude > CXL on other platform firmware architectures. So, "platform_" or > "root_"? Sure. Platform_ is better. > > > + struct resource *decoder_res; > > So the region device is going to be doing the __request_region() from > @cfmws_res, not decoders, so why is this a resource and not a range? Well, sort of. Most of v3 was to ultimately go with the decoders doing the __request_region, although thinking about this further that will not work since multiple decoders might want the same ranges of the parent resource. So I guess it has to be region. I can go back to renaming it res in that case. You're correct that the region device should own doing the __request_region.
On 21-10-29 15:37:51, Ben Widawsky wrote: > On 21-10-29 15:22:19, Dan Williams wrote: > > On Fri, Oct 29, 2021 at 2:27 PM Ben Widawsky <ben.widawsky@intel.com> wrote: > > [..] > > > > Note, this is just how I had it roughly mapped out in my head, I defer > > > > making it a hard recommendation until I get deeper into this set to > > > > see if we diverge. > > > > > > > > > > You're correct. > > > > > > I had changed this already for my v3. I ended up with this: > > > + union { > > > + struct resource cfmws_res; > > > > "cfmws" is something that will only appear on ACPI based CXL systems, > > so if it needs a prefix it should be something that does not preclude > > CXL on other platform firmware architectures. So, "platform_" or > > "root_"? > > Sure. Platform_ is better. > Coming back to this. While "CFMWS" is indeed defined as part of ACPI, I do think it could also be a generic name for what it represents. In other words, I don't see any issue with using CFMWS, but, I'll start using "platform" since it seems it's your preference (and I prefer that to "root"). > > > > > + struct resource *decoder_res; > > > > So the region device is going to be doing the __request_region() from > > @cfmws_res, not decoders, so why is this a resource and not a range? > > Well, sort of. Most of v3 was to ultimately go with the decoders doing the > __request_region, although thinking about this further that will not work since > multiple decoders might want the same ranges of the parent resource. > > So I guess it has to be region. I can go back to renaming it res in that case. > You're correct that the region device should own doing the __request_region.
diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c index 7d13e7f0aefc..b972abc9f6ef 100644 --- a/drivers/cxl/acpi.c +++ b/drivers/cxl/acpi.c @@ -126,10 +126,9 @@ static void cxl_add_cfmws_decoders(struct device *dev, cxld->flags = cfmws_to_decoder_flags(cfmws->restrictions); cxld->target_type = CXL_DECODER_EXPANDER; - cxld->range = (struct range) { - .start = cfmws->base_hpa, - .end = cfmws->base_hpa + cfmws->window_size - 1, - }; + cxld->res = (struct resource)DEFINE_RES_MEM_NAMED(cfmws->base_hpa, + cfmws->window_size, + "cfmws"); cxld->interleave_ways = CFMWS_INTERLEAVE_WAYS(cfmws); cxld->interleave_granularity = CFMWS_INTERLEAVE_GRANULARITY(cfmws); @@ -339,10 +338,7 @@ static int add_host_bridge_uport(struct device *match, void *arg) cxld->interleave_ways = 1; cxld->interleave_granularity = PAGE_SIZE; cxld->target_type = CXL_DECODER_EXPANDER; - cxld->range = (struct range) { - .start = 0, - .end = -1, - }; + cxld->res = (struct resource)DEFINE_RES_MEM(0, 0); device_lock(&port->dev); dport = list_first_entry(&port->dports, typeof(*dport), list); diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c index ebd061d03950..454d4d846eb2 100644 --- a/drivers/cxl/core/bus.c +++ b/drivers/cxl/core/bus.c @@ -47,7 +47,7 @@ static ssize_t start_show(struct device *dev, struct device_attribute *attr, { struct cxl_decoder *cxld = to_cxl_decoder(dev); - return sysfs_emit(buf, "%#llx\n", cxld->range.start); + return sysfs_emit(buf, "%#llx\n", cxld->res.start); } static DEVICE_ATTR_RO(start); @@ -56,7 +56,7 @@ static ssize_t size_show(struct device *dev, struct device_attribute *attr, { struct cxl_decoder *cxld = to_cxl_decoder(dev); - return sysfs_emit(buf, "%#llx\n", range_len(&cxld->range)); + return sysfs_emit(buf, "%#llx\n", resource_size(&cxld->res)); } static DEVICE_ATTR_RO(size); diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h index 4483e1a39fc3..7f2e2bdc7883 100644 --- a/drivers/cxl/cxl.h +++ b/drivers/cxl/cxl.h @@ -195,7 +195,7 @@ enum cxl_decoder_type { * struct cxl_decoder - CXL address range decode configuration * @dev: this decoder's device * @id: kernel device name id - * @range: address range considered by this decoder + * @res: address space resources considered by this decoder * @interleave_ways: number of cxl_dports in this decode * @interleave_granularity: data stride per dport * @target_type: accelerator vs expander (type2 vs type3) selector @@ -206,7 +206,7 @@ enum cxl_decoder_type { struct cxl_decoder { struct device dev; int id; - struct range range; + struct resource res; int interleave_ways; int interleave_granularity; enum cxl_decoder_type target_type;
Regions will use the resource API in order to help manage allocated space. As regions are children of the decoder, it makes sense that the parent host the main resource to be suballocated by the region. Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> --- drivers/cxl/acpi.c | 12 ++++-------- drivers/cxl/core/bus.c | 4 ++-- drivers/cxl/cxl.h | 4 ++-- 3 files changed, 8 insertions(+), 12 deletions(-)