| Message ID | 20240612205905.1209448-1-nathan.fontenot@amd.com |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cxl/region: Remove soft reserve resource at region construction | expand |
On Wed, Jun 12, 2024 at 03:59:05PM -0500, Nathan Fontenot wrote: > Systems with a BIOS that creates soft reserve iomem resources > for memory ranges covered by CXL devices, the soft reserve resource > needs to be removed when hotplugging a CXL device. Without doing > so a new soft reserve for the memory range cannot be created > during hotplug add. Is a new soft reserved ever created? I think the reason is to free up the memory for the new region creation, period. If the SR exists, userspace cannot 'replace' the region. > > The previous approach from Alison [1] was to fix up the soft > reserve resource at region release so it longer covers the > memory range for the region being removed. > > Instead of waiting for region removal, remove the soft reserve > resource when constructing a region. This is done by walking the > children of the region's parent's resources, finding a soft > reserve resource (if it exists) containing the memory range > for the region and removing the soft reserve prior to inserting > the resource for the region being constructed. Is it OK to remove the entire Soft Reserved when that first region is constructed? ie. in this setup that removes the soft reserved before region1 is constructed. 100-600 : CXL Window 0 100-600 : Soft Reserved 100-200 : region0 200-300 : region1 one bit below... > > [1] https://lore.kernel.org/linux-cxl/cover.1692638817.git.alison.schofield@intel.com/ > > Signed-off-by: Nathan Fontenot <nathan.fontenot@amd.com> > --- > drivers/cxl/core/region.c | 30 +++++++++++++++++++++++++++++- > 1 file changed, 29 insertions(+), 1 deletion(-) > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index 00a9f0eef8dd..90e5c84230a5 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -3096,6 +3096,34 @@ static int match_region_by_range(struct device *dev, void *data) > return rc; > } > > +static int cxl_insert_resource(struct device *dev, struct resource *parent, > + struct resource *res) > +{ > + struct resource *soft; > + int rc; > + > + /* > + * Find and attempt to remove the SOFT_RESERVED resource > + * (if it exists) that includes the range of the new > + * resource we're inserting. > + */ > + for (soft = parent->child; soft; soft = soft->sibling) { > + if (soft->desc != IORES_DESC_SOFT_RESERVED) > + continue; > + > + if (soft->start <= res->start && soft->end >= res->end) { > + rc = remove_resource(soft); > + if (rc) > + dev_warn(dev, > + "Couldn't remove %s resource %llx - %llx\n", Use %pr format for struct resource. > + soft->name, soft->start, soft->end); > + break; > + } > + } > + > + 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) > @@ -3142,7 +3170,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 = cxl_insert_resource(cxlmd->dev.parent, cxlrd->res, res); > if (rc) { > /* > * Platform-firmware may not have split resources like "System > -- > 2.34.1 >
Nathan Fontenot wrote: > Systems with a BIOS that creates soft reserve iomem resources > for memory ranges covered by CXL devices, the soft reserve resource > needs to be removed when hotplugging a CXL device. Without doing > so a new soft reserve for the memory range cannot be created > during hotplug add. > > The previous approach from Alison [1] was to fix up the soft > reserve resource at region release so it longer covers the > memory range for the region being removed. > > Instead of waiting for region removal, remove the soft reserve > resource when constructing a region. This is done by walking the > children of the region's parent's resources, finding a soft > reserve resource (if it exists) containing the memory range > for the region and removing the soft reserve prior to inserting > the resource for the region being constructed. > > [1] https://lore.kernel.org/linux-cxl/cover.1692638817.git.alison.schofield@intel.com/ Somewhat disappointing that this changelog did not address anything from this write-up: http://lore.kernel.org/r/65e0fcae989d6_1138c7294c2@dwillia2-xfh.jf.intel.com.notmuch > > Signed-off-by: Nathan Fontenot <nathan.fontenot@amd.com> > --- > drivers/cxl/core/region.c | 30 +++++++++++++++++++++++++++++- > 1 file changed, 29 insertions(+), 1 deletion(-) > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index 00a9f0eef8dd..90e5c84230a5 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -3096,6 +3096,34 @@ static int match_region_by_range(struct device *dev, void *data) > return rc; > } > > +static int cxl_insert_resource(struct device *dev, struct resource *parent, > + struct resource *res) > +{ > + struct resource *soft; > + int rc; > + > + /* > + * Find and attempt to remove the SOFT_RESERVED resource > + * (if it exists) that includes the range of the new > + * resource we're inserting. > + */ I wish it were this simple: > + for (soft = parent->child; soft; soft = soft->sibling) { This list is protected by resource_lock, unlocked walks are racy. You might say, "what about walk_iomem_res_desc()", but that does not allow the callback routine to manipulate the resource tree. > + if (soft->desc != IORES_DESC_SOFT_RESERVED) > + continue; > + > + if (soft->start <= res->start && soft->end >= res->end) { > + rc = remove_resource(soft); The concern here is what happens if CXL region assembly eventually fails? There is no fallback possible. Instead the approach I outlined in 65e0fcae989d6_1138c7294c2@dwillia2-xfh.jf.intel.com.notmuch is where I would like to see this effort go.
On 6/12/24 19:30, Alison Schofield wrote: > On Wed, Jun 12, 2024 at 03:59:05PM -0500, Nathan Fontenot wrote: >> Systems with a BIOS that creates soft reserve iomem resources >> for memory ranges covered by CXL devices, the soft reserve resource >> needs to be removed when hotplugging a CXL device. Without doing >> so a new soft reserve for the memory range cannot be created >> during hotplug add. > > Is a new soft reserved ever created? I think the reason is to > free up the memory for the new region creation, period. If the > SR exists, userspace cannot 'replace' the region. You're correct. I had started from an bad assumption, there is no new soft reserve created. If I now understand correctly the issue is in alloc_hpa() when a user tries to set the size for the region. The call to alloc_free_mem_region() ensures the address space is not claimed, which in this case it is by the soft reserve. > >> >> The previous approach from Alison [1] was to fix up the soft >> reserve resource at region release so it longer covers the >> memory range for the region being removed. >> >> Instead of waiting for region removal, remove the soft reserve >> resource when constructing a region. This is done by walking the >> children of the region's parent's resources, finding a soft >> reserve resource (if it exists) containing the memory range >> for the region and removing the soft reserve prior to inserting >> the resource for the region being constructed. > > Is it OK to remove the entire Soft Reserved when that first region > is constructed? ie. in this setup that removes the soft reserved > before region1 is constructed. > > 100-600 : CXL Window 0 > 100-600 : Soft Reserved > 100-200 : region0 > 200-300 : region1 My approach was thinking that it is ok to go ahead and remove the entire soft reserve. I couldn't find a reason we needed to keep the soft reserve after this point. I could be wrong on this. One different approach I've been looking at is if it's possible to add the CFMWS resource as a child of the soft reserve. Instead of starting with 100 - 600 " CXL Window 0 100 - 600 : Soft Reserved We would have 100 - 600 : Soft Reserved 100 - 600 : CXL Window 0 My thinking is that with the soft reserve above the CXL Window we no longer need to manipulate the soft reserve range on hotplug remove. The alloc_hpa() call for user set up uses the CXL Window as the base for alloc_free_mem_region() and the soft reserve wouldn't come into play. I haven't looked at acpi:add_cxl_resources() enough to know if this is something that actually can be done safely. > > > one bit below... > > >> >> [1] https://lore.kernel.org/linux-cxl/cover.1692638817.git.alison.schofield@intel.com/ >> >> Signed-off-by: Nathan Fontenot <nathan.fontenot@amd.com> >> --- >> drivers/cxl/core/region.c | 30 +++++++++++++++++++++++++++++- >> 1 file changed, 29 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c >> index 00a9f0eef8dd..90e5c84230a5 100644 >> --- a/drivers/cxl/core/region.c >> +++ b/drivers/cxl/core/region.c >> @@ -3096,6 +3096,34 @@ static int match_region_by_range(struct device *dev, void *data) >> return rc; >> } >> >> +static int cxl_insert_resource(struct device *dev, struct resource *parent, >> + struct resource *res) >> +{ >> + struct resource *soft; >> + int rc; >> + >> + /* >> + * Find and attempt to remove the SOFT_RESERVED resource >> + * (if it exists) that includes the range of the new >> + * resource we're inserting. >> + */ >> + for (soft = parent->child; soft; soft = soft->sibling) { >> + if (soft->desc != IORES_DESC_SOFT_RESERVED) >> + continue; >> + >> + if (soft->start <= res->start && soft->end >= res->end) { >> + rc = remove_resource(soft); >> + if (rc) >> + dev_warn(dev, >> + "Couldn't remove %s resource %llx - %llx\n", > > Use %pr format for struct resource. good catch, I'll update this locally but Dan's response let me know the resource walk is not something I should be doing. -Nathan > >> + soft->name, soft->start, soft->end); >> + break; >> + } >> + } >> + >> + 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) >> @@ -3142,7 +3170,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 = cxl_insert_resource(cxlmd->dev.parent, cxlrd->res, res); >> if (rc) { >> /* >> * Platform-firmware may not have split resources like "System >> -- >> 2.34.1 >>
Nathan Fontenot wrote: [..] > > > > Is it OK to remove the entire Soft Reserved when that first region > > is constructed? ie. in this setup that removes the soft reserved > > before region1 is constructed. > > > > 100-600 : CXL Window 0 > > 100-600 : Soft Reserved > > 100-200 : region0 > > 200-300 : region1 > > My approach was thinking that it is ok to go ahead and remove the entire soft reserve. > I couldn't find a reason we needed to keep the soft reserve after this point. I could > be wrong on this. > > One different approach I've been looking at is if it's possible to add the CFMWS resource > as a child of the soft reserve. Instead of starting with > > 100 - 600 " CXL Window 0 > 100 - 600 : Soft Reserved > > We would have > > 100 - 600 : Soft Reserved > 100 - 600 : CXL Window 0 > > My thinking is that with the soft reserve above the CXL Window we no longer need to > manipulate the soft reserve range on hotplug remove. The alloc_hpa() call for user > set up uses the CXL Window as the base for alloc_free_mem_region() and the soft > reserve wouldn't come into play. > > I haven't looked at acpi:add_cxl_resources() enough to know if this is something that > actually can be done safely. Could you please stop trying to invent new approaches and respond to the one on the table?
On 6/13/24 14:31, Dan Williams wrote: > Nathan Fontenot wrote: > [..] >>> >>> Is it OK to remove the entire Soft Reserved when that first region >>> is constructed? ie. in this setup that removes the soft reserved >>> before region1 is constructed. >>> >>> 100-600 : CXL Window 0 >>> 100-600 : Soft Reserved >>> 100-200 : region0 >>> 200-300 : region1 >> >> My approach was thinking that it is ok to go ahead and remove the entire soft reserve. >> I couldn't find a reason we needed to keep the soft reserve after this point. I could >> be wrong on this. >> >> One different approach I've been looking at is if it's possible to add the CFMWS resource >> as a child of the soft reserve. Instead of starting with >> >> 100 - 600 " CXL Window 0 >> 100 - 600 : Soft Reserved >> >> We would have >> >> 100 - 600 : Soft Reserved >> 100 - 600 : CXL Window 0 >> >> My thinking is that with the soft reserve above the CXL Window we no longer need to >> manipulate the soft reserve range on hotplug remove. The alloc_hpa() call for user >> set up uses the CXL Window as the base for alloc_free_mem_region() and the soft >> reserve wouldn't come into play. >> >> I haven't looked at acpi:add_cxl_resources() enough to know if this is something that >> actually can be done safely. > > Could you please stop trying to invent new approaches and respond to the > one on the table? Going back through the previous conversation, I was just trying to explore different options. -Nathan
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c index 00a9f0eef8dd..90e5c84230a5 100644 --- a/drivers/cxl/core/region.c +++ b/drivers/cxl/core/region.c @@ -3096,6 +3096,34 @@ static int match_region_by_range(struct device *dev, void *data) return rc; } +static int cxl_insert_resource(struct device *dev, struct resource *parent, + struct resource *res) +{ + struct resource *soft; + int rc; + + /* + * Find and attempt to remove the SOFT_RESERVED resource + * (if it exists) that includes the range of the new + * resource we're inserting. + */ + for (soft = parent->child; soft; soft = soft->sibling) { + if (soft->desc != IORES_DESC_SOFT_RESERVED) + continue; + + if (soft->start <= res->start && soft->end >= res->end) { + rc = remove_resource(soft); + if (rc) + dev_warn(dev, + "Couldn't remove %s resource %llx - %llx\n", + soft->name, soft->start, soft->end); + break; + } + } + + 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) @@ -3142,7 +3170,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 = cxl_insert_resource(cxlmd->dev.parent, cxlrd->res, res); if (rc) { /* * Platform-firmware may not have split resources like "System
Systems with a BIOS that creates soft reserve iomem resources for memory ranges covered by CXL devices, the soft reserve resource needs to be removed when hotplugging a CXL device. Without doing so a new soft reserve for the memory range cannot be created during hotplug add. The previous approach from Alison [1] was to fix up the soft reserve resource at region release so it longer covers the memory range for the region being removed. Instead of waiting for region removal, remove the soft reserve resource when constructing a region. This is done by walking the children of the region's parent's resources, finding a soft reserve resource (if it exists) containing the memory range for the region and removing the soft reserve prior to inserting the resource for the region being constructed. [1] https://lore.kernel.org/linux-cxl/cover.1692638817.git.alison.schofield@intel.com/ Signed-off-by: Nathan Fontenot <nathan.fontenot@amd.com> --- drivers/cxl/core/region.c | 30 +++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-)