| Message ID | 173753635601.3849855.5582594127330525596.stgit@dwillia2-xfh.jf.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | cxl: DPA partition metadata is a mess... | expand |
Dan Williams wrote: > CXL_DECODER_MIXED is a safety mechanism introduced for the case where > platform firmware has programmed an endpoint decoder that straddles a > DPA partition boundary. While the kernel is careful to only allocate DPA > capacity within a single partition there is no guarantee that platform > firmware, or anything that touched the device before the current kernel, > gets that right. > > However, __cxl_dpa_reserve() will never get to the CXL_DECODER_MIXED > designation because of the way it tracks partition boundaries. A > request_resource() that spans ->ram_res and ->pmem_res fails with the > following signature: > > __cxl_dpa_reserve: cxl_port endpoint15: decoder15.0: failed to reserve allocation > > CXL_DECODER_MIXED is dead defensive programming after the driver has > already given up on the device. It has never offered any protection in > practice, just delete it. > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> Reviewed-by: Ira Weiny <ira.weiny@intel.com>
On Wed, 22 Jan 2025 00:59:16 -0800 Dan Williams <dan.j.williams@intel.com> wrote: > CXL_DECODER_MIXED is a safety mechanism introduced for the case where > platform firmware has programmed an endpoint decoder that straddles a > DPA partition boundary. While the kernel is careful to only allocate DPA > capacity within a single partition there is no guarantee that platform > firmware, or anything that touched the device before the current kernel, > gets that right. > > However, __cxl_dpa_reserve() will never get to the CXL_DECODER_MIXED > designation because of the way it tracks partition boundaries. A > request_resource() that spans ->ram_res and ->pmem_res fails with the > following signature: > > __cxl_dpa_reserve: cxl_port endpoint15: decoder15.0: failed to reserve allocation > > CXL_DECODER_MIXED is dead defensive programming after the driver has > already given up on the device. It has never offered any protection in > practice, just delete it. > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
On 1/22/25 08:59, Dan Williams wrote: > CXL_DECODER_MIXED is a safety mechanism introduced for the case where > platform firmware has programmed an endpoint decoder that straddles a > DPA partition boundary. While the kernel is careful to only allocate DPA > capacity within a single partition there is no guarantee that platform > firmware, or anything that touched the device before the current kernel, > gets that right. > > However, __cxl_dpa_reserve() will never get to the CXL_DECODER_MIXED > designation because of the way it tracks partition boundaries. A > request_resource() that spans ->ram_res and ->pmem_res fails with the > following signature: > > __cxl_dpa_reserve: cxl_port endpoint15: decoder15.0: failed to reserve allocation > > CXL_DECODER_MIXED is dead defensive programming after the driver has > already given up on the device. It has never offered any protection in > practice, just delete it. > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > --- > drivers/cxl/core/hdm.c | 6 +++--- > drivers/cxl/core/region.c | 12 ------------ > drivers/cxl/cxl.h | 4 +--- > 3 files changed, 4 insertions(+), 18 deletions(-) > > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c > index 28edd5822486..2848d6991d45 100644 > --- a/drivers/cxl/core/hdm.c > +++ b/drivers/cxl/core/hdm.c > @@ -332,9 +332,9 @@ static int __cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled, > else if (resource_contains(&cxlds->ram_res, res)) > cxled->mode = CXL_DECODER_RAM; > else { > - dev_warn(dev, "decoder%d.%d: %pr mixed mode not supported\n", > - port->id, cxled->cxld.id, cxled->dpa_res); > - cxled->mode = CXL_DECODER_MIXED; > + dev_warn(dev, "decoder%d.%d: %pr does not map any partition\n", > + port->id, cxled->cxld.id, res); > + cxled->mode = CXL_DECODER_NONE; > } > > port->hdm_end++; > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index d77899650798..e4885acac853 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -2725,18 +2725,6 @@ static int poison_by_decoder(struct device *dev, void *arg) > if (!cxled->dpa_res || !resource_size(cxled->dpa_res)) > return rc; > > - /* > - * Regions are only created with single mode decoders: pmem or ram. > - * Linux does not support mixed mode decoders. This means that > - * reading poison per endpoint decoder adheres to the requirement > - * that poison reads of pmem and ram must be separated. > - * CXL 3.0 Spec 8.2.9.8.4.1 > - */ > - if (cxled->mode == CXL_DECODER_MIXED) { > - dev_dbg(dev, "poison list read unsupported in mixed mode\n"); > - return rc; > - } > - > cxlmd = cxled_to_memdev(cxled); > if (cxled->skip) { > offset = cxled->dpa_res->start - cxled->skip; > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > index f6015f24ad38..4d0550367042 100644 > --- a/drivers/cxl/cxl.h > +++ b/drivers/cxl/cxl.h > @@ -379,7 +379,6 @@ enum cxl_decoder_mode { > CXL_DECODER_NONE, > CXL_DECODER_RAM, > CXL_DECODER_PMEM, > - CXL_DECODER_MIXED, > CXL_DECODER_DEAD, > }; > > @@ -389,10 +388,9 @@ static inline const char *cxl_decoder_mode_name(enum cxl_decoder_mode mode) > [CXL_DECODER_NONE] = "none", > [CXL_DECODER_RAM] = "ram", > [CXL_DECODER_PMEM] = "pmem", > - [CXL_DECODER_MIXED] = "mixed", > }; > > - if (mode >= CXL_DECODER_NONE && mode <= CXL_DECODER_MIXED) > + if (mode >= CXL_DECODER_NONE && mode < CXL_DECODER_DEAD) > return names[mode]; > return "mixed"; > } > Reviewed-by: Alejandro Lucero <alucerop@amd.com>
On 1/22/25 1:59 AM, Dan Williams wrote: > CXL_DECODER_MIXED is a safety mechanism introduced for the case where > platform firmware has programmed an endpoint decoder that straddles a > DPA partition boundary. While the kernel is careful to only allocate DPA > capacity within a single partition there is no guarantee that platform > firmware, or anything that touched the device before the current kernel, > gets that right. > > However, __cxl_dpa_reserve() will never get to the CXL_DECODER_MIXED > designation because of the way it tracks partition boundaries. A > request_resource() that spans ->ram_res and ->pmem_res fails with the > following signature: > > __cxl_dpa_reserve: cxl_port endpoint15: decoder15.0: failed to reserve allocation > > CXL_DECODER_MIXED is dead defensive programming after the driver has > already given up on the device. It has never offered any protection in > practice, just delete it. > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> Reviewed-by: Dave Jiang <dave.jiang@intel.com> > --- > drivers/cxl/core/hdm.c | 6 +++--- > drivers/cxl/core/region.c | 12 ------------ > drivers/cxl/cxl.h | 4 +--- > 3 files changed, 4 insertions(+), 18 deletions(-) > > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c > index 28edd5822486..2848d6991d45 100644 > --- a/drivers/cxl/core/hdm.c > +++ b/drivers/cxl/core/hdm.c > @@ -332,9 +332,9 @@ static int __cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled, > else if (resource_contains(&cxlds->ram_res, res)) > cxled->mode = CXL_DECODER_RAM; > else { > - dev_warn(dev, "decoder%d.%d: %pr mixed mode not supported\n", > - port->id, cxled->cxld.id, cxled->dpa_res); > - cxled->mode = CXL_DECODER_MIXED; > + dev_warn(dev, "decoder%d.%d: %pr does not map any partition\n", > + port->id, cxled->cxld.id, res); > + cxled->mode = CXL_DECODER_NONE; > } > > port->hdm_end++; > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index d77899650798..e4885acac853 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -2725,18 +2725,6 @@ static int poison_by_decoder(struct device *dev, void *arg) > if (!cxled->dpa_res || !resource_size(cxled->dpa_res)) > return rc; > > - /* > - * Regions are only created with single mode decoders: pmem or ram. > - * Linux does not support mixed mode decoders. This means that > - * reading poison per endpoint decoder adheres to the requirement > - * that poison reads of pmem and ram must be separated. > - * CXL 3.0 Spec 8.2.9.8.4.1 > - */ > - if (cxled->mode == CXL_DECODER_MIXED) { > - dev_dbg(dev, "poison list read unsupported in mixed mode\n"); > - return rc; > - } > - > cxlmd = cxled_to_memdev(cxled); > if (cxled->skip) { > offset = cxled->dpa_res->start - cxled->skip; > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > index f6015f24ad38..4d0550367042 100644 > --- a/drivers/cxl/cxl.h > +++ b/drivers/cxl/cxl.h > @@ -379,7 +379,6 @@ enum cxl_decoder_mode { > CXL_DECODER_NONE, > CXL_DECODER_RAM, > CXL_DECODER_PMEM, > - CXL_DECODER_MIXED, > CXL_DECODER_DEAD, > }; > > @@ -389,10 +388,9 @@ static inline const char *cxl_decoder_mode_name(enum cxl_decoder_mode mode) > [CXL_DECODER_NONE] = "none", > [CXL_DECODER_RAM] = "ram", > [CXL_DECODER_PMEM] = "pmem", > - [CXL_DECODER_MIXED] = "mixed", > }; > > - if (mode >= CXL_DECODER_NONE && mode <= CXL_DECODER_MIXED) > + if (mode >= CXL_DECODER_NONE && mode < CXL_DECODER_DEAD) > return names[mode]; > return "mixed"; > } >
diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c index 28edd5822486..2848d6991d45 100644 --- a/drivers/cxl/core/hdm.c +++ b/drivers/cxl/core/hdm.c @@ -332,9 +332,9 @@ static int __cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled, else if (resource_contains(&cxlds->ram_res, res)) cxled->mode = CXL_DECODER_RAM; else { - dev_warn(dev, "decoder%d.%d: %pr mixed mode not supported\n", - port->id, cxled->cxld.id, cxled->dpa_res); - cxled->mode = CXL_DECODER_MIXED; + dev_warn(dev, "decoder%d.%d: %pr does not map any partition\n", + port->id, cxled->cxld.id, res); + cxled->mode = CXL_DECODER_NONE; } port->hdm_end++; diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c index d77899650798..e4885acac853 100644 --- a/drivers/cxl/core/region.c +++ b/drivers/cxl/core/region.c @@ -2725,18 +2725,6 @@ static int poison_by_decoder(struct device *dev, void *arg) if (!cxled->dpa_res || !resource_size(cxled->dpa_res)) return rc; - /* - * Regions are only created with single mode decoders: pmem or ram. - * Linux does not support mixed mode decoders. This means that - * reading poison per endpoint decoder adheres to the requirement - * that poison reads of pmem and ram must be separated. - * CXL 3.0 Spec 8.2.9.8.4.1 - */ - if (cxled->mode == CXL_DECODER_MIXED) { - dev_dbg(dev, "poison list read unsupported in mixed mode\n"); - return rc; - } - cxlmd = cxled_to_memdev(cxled); if (cxled->skip) { offset = cxled->dpa_res->start - cxled->skip; diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h index f6015f24ad38..4d0550367042 100644 --- a/drivers/cxl/cxl.h +++ b/drivers/cxl/cxl.h @@ -379,7 +379,6 @@ enum cxl_decoder_mode { CXL_DECODER_NONE, CXL_DECODER_RAM, CXL_DECODER_PMEM, - CXL_DECODER_MIXED, CXL_DECODER_DEAD, }; @@ -389,10 +388,9 @@ static inline const char *cxl_decoder_mode_name(enum cxl_decoder_mode mode) [CXL_DECODER_NONE] = "none", [CXL_DECODER_RAM] = "ram", [CXL_DECODER_PMEM] = "pmem", - [CXL_DECODER_MIXED] = "mixed", }; - if (mode >= CXL_DECODER_NONE && mode <= CXL_DECODER_MIXED) + if (mode >= CXL_DECODER_NONE && mode < CXL_DECODER_DEAD) return names[mode]; return "mixed"; }
CXL_DECODER_MIXED is a safety mechanism introduced for the case where platform firmware has programmed an endpoint decoder that straddles a DPA partition boundary. While the kernel is careful to only allocate DPA capacity within a single partition there is no guarantee that platform firmware, or anything that touched the device before the current kernel, gets that right. However, __cxl_dpa_reserve() will never get to the CXL_DECODER_MIXED designation because of the way it tracks partition boundaries. A request_resource() that spans ->ram_res and ->pmem_res fails with the following signature: __cxl_dpa_reserve: cxl_port endpoint15: decoder15.0: failed to reserve allocation CXL_DECODER_MIXED is dead defensive programming after the driver has already given up on the device. It has never offered any protection in practice, just delete it. Signed-off-by: Dan Williams <dan.j.williams@intel.com> --- drivers/cxl/core/hdm.c | 6 +++--- drivers/cxl/core/region.c | 12 ------------ drivers/cxl/cxl.h | 4 +--- 3 files changed, 4 insertions(+), 18 deletions(-)