| Message ID | 167564537131.847146.9020072654741860107.stgit@dwillia2-xfh.jf.intel.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | CXL RAM and the 'Soft Reserved' => 'System RAM' default | expand |
On Sun, 05 Feb 2023 17:02:51 -0800 Dan Williams <dan.j.williams@intel.com> wrote: > In preparation for a new region mode, do not, for example, allow > 'ram' decoders to be assigned to 'pmem' regions and vice versa. > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> I guess we aren't support mixed endpoint decoders for now (and maybe never..) If we did I'd expect this to have to allow mixed with either RAM or PMEM. Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/cxl/core/region.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index c9e7f05caa0f..53d6dbe4de6d 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -1217,6 +1217,12 @@ static int cxl_region_attach(struct cxl_region *cxlr, > struct cxl_dport *dport; > int i, rc = -ENXIO; > > + if (cxled->mode != cxlr->mode) { > + dev_dbg(&cxlr->dev, "%s region mode: %d mismatch: %d\n", > + dev_name(&cxled->cxld.dev), cxlr->mode, cxled->mode); > + return -EINVAL; > + } > + > if (cxled->mode == CXL_DECODER_DEAD) { > dev_dbg(&cxlr->dev, "%s dead\n", dev_name(&cxled->cxld.dev)); > return -ENODEV; >
On Sun, Feb 05, 2023 at 05:02:51PM -0800, Dan Williams wrote: > In preparation for a new region mode, do not, for example, allow > 'ram' decoders to be assigned to 'pmem' regions and vice versa. > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > --- > drivers/cxl/core/region.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index c9e7f05caa0f..53d6dbe4de6d 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -1217,6 +1217,12 @@ static int cxl_region_attach(struct cxl_region *cxlr, > struct cxl_dport *dport; > int i, rc = -ENXIO; > > + if (cxled->mode != cxlr->mode) { > + dev_dbg(&cxlr->dev, "%s region mode: %d mismatch: %d\n", > + dev_name(&cxled->cxld.dev), cxlr->mode, cxled->mode); > + return -EINVAL; > + } > + > if (cxled->mode == CXL_DECODER_DEAD) { > dev_dbg(&cxlr->dev, "%s dead\n", dev_name(&cxled->cxld.dev)); > return -ENODEV; > Maybe a stupid question. It will be entirely possible to "convert" pmem to "ram" (i.e. just online it as system memory). Are type-3 devices with pmem expected to carry 2 decoders (one ram, 1 pmem), such that you can create a ram region from pmem, or is it expected that the pmem decoder will be used for pmem, even if it gets onlined as volatile? Just asking because depending on how that flushes out, you might expected a "ram region" to get assigned to a "pmem decoder", unless you're expecting all pmem-carrying-devices to create 2 decoders per pmem region depending on how the end user intends to use it. ~Gregory
Jonathan Cameron wrote: > On Sun, 05 Feb 2023 17:02:51 -0800 > Dan Williams <dan.j.williams@intel.com> wrote: > > > In preparation for a new region mode, do not, for example, allow > > 'ram' decoders to be assigned to 'pmem' regions and vice versa. > > > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > > I guess we aren't support mixed endpoint decoders for now (and > maybe never..) If we did I'd expect this to have to allow > mixed with either RAM or PMEM. Mixed is a bug, or I otherwise can not think of a scenario where it is valid. The kernel prevents it, but platform firmware can make that mistake. I think if the kernel ever encounters that and works around it that will come with a loud TAINT_FIRMWARE_WORKAROUND warning. > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > --- > > drivers/cxl/core/region.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > > index c9e7f05caa0f..53d6dbe4de6d 100644 > > --- a/drivers/cxl/core/region.c > > +++ b/drivers/cxl/core/region.c > > @@ -1217,6 +1217,12 @@ static int cxl_region_attach(struct cxl_region *cxlr, > > struct cxl_dport *dport; > > int i, rc = -ENXIO; > > > > + if (cxled->mode != cxlr->mode) { > > + dev_dbg(&cxlr->dev, "%s region mode: %d mismatch: %d\n", > > + dev_name(&cxled->cxld.dev), cxlr->mode, cxled->mode); > > + return -EINVAL; > > + } > > + > > if (cxled->mode == CXL_DECODER_DEAD) { > > dev_dbg(&cxlr->dev, "%s dead\n", dev_name(&cxled->cxld.dev)); > > return -ENODEV; > > >
Dan Williams wrote: > In preparation for a new region mode, do not, for example, allow > 'ram' decoders to be assigned to 'pmem' regions and vice versa. > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> Reviewed-by: Ira Weiny <ira.weiny@intel.com>
On Mon, Feb 06, 2023 at 01:51:18PM -0800, Dan Williams wrote: > Gregory Price wrote: > > On Sun, Feb 05, 2023 at 05:02:51PM -0800, Dan Williams wrote: > > > In preparation for a new region mode, do not, for example, allow > > > 'ram' decoders to be assigned to 'pmem' regions and vice versa. > > > > > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > > > --- > > > drivers/cxl/core/region.c | 6 ++++++ > > > 1 file changed, 6 insertions(+) > > > [...] sounds good, thanks for the explanation Reviewed-by: Gregory Price <gregory.price@memverge.com>
Gregory Price wrote: > On Sun, Feb 05, 2023 at 05:02:51PM -0800, Dan Williams wrote: > > In preparation for a new region mode, do not, for example, allow > > 'ram' decoders to be assigned to 'pmem' regions and vice versa. > > > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > > --- > > drivers/cxl/core/region.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > > index c9e7f05caa0f..53d6dbe4de6d 100644 > > --- a/drivers/cxl/core/region.c > > +++ b/drivers/cxl/core/region.c > > @@ -1217,6 +1217,12 @@ static int cxl_region_attach(struct cxl_region *cxlr, > > struct cxl_dport *dport; > > int i, rc = -ENXIO; > > > > + if (cxled->mode != cxlr->mode) { > > + dev_dbg(&cxlr->dev, "%s region mode: %d mismatch: %d\n", > > + dev_name(&cxled->cxld.dev), cxlr->mode, cxled->mode); > > + return -EINVAL; > > + } > > + > > if (cxled->mode == CXL_DECODER_DEAD) { > > dev_dbg(&cxlr->dev, "%s dead\n", dev_name(&cxled->cxld.dev)); > > return -ENODEV; > > > > > Maybe a stupid question. It will be entirely possible to "convert" pmem > to "ram" (i.e. just online it as system memory). That's a good question, there are two mechanisms for this: 1/ Use ndctl to assign the resulting pmem namespace to devdax mode and then use dax_kmem to online that persistent memory. 2/ If the CXL device supports partitioning ram vs pmem capacity: tear down the pmem region ('cxl destroy-region ...'), repartition the pmem ('cxl set-partition -t ram ...'), create a new ram region ('cxl create-region ...'). > Are type-3 devices with pmem expected to carry 2 decoders (one ram, 1 > pmem), such that you can create a ram region from pmem, or is it expected > that the pmem decoder will be used for pmem, even if it gets onlined > as volatile? Not sure there are any formal expectations when it comes to number of decoders. > Just asking because depending on how that flushes out, you might > expected a "ram region" to get assigned to a "pmem decoder", unless > you're expecting all pmem-carrying-devices to create 2 decoders per pmem > region depending on how the end user intends to use it. Per 2/ above even with a 1-decoder-device there is a spec-defined mechanism to assign all of the device's capacity to ram.
On 2/5/23 6:02 PM, Dan Williams wrote: > In preparation for a new region mode, do not, for example, allow > 'ram' decoders to be assigned to 'pmem' regions and vice versa. > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> Reviewed-by: Dave Jiang <dave.jiang@intel.com> > --- > drivers/cxl/core/region.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index c9e7f05caa0f..53d6dbe4de6d 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -1217,6 +1217,12 @@ static int cxl_region_attach(struct cxl_region *cxlr, > struct cxl_dport *dport; > int i, rc = -ENXIO; > > + if (cxled->mode != cxlr->mode) { > + dev_dbg(&cxlr->dev, "%s region mode: %d mismatch: %d\n", > + dev_name(&cxled->cxld.dev), cxlr->mode, cxled->mode); > + return -EINVAL; > + } > + > if (cxled->mode == CXL_DECODER_DEAD) { > dev_dbg(&cxlr->dev, "%s dead\n", dev_name(&cxled->cxld.dev)); > return -ENODEV; >
On Sun, 2023-02-05 at 17:02 -0800, Dan Williams wrote: > In preparation for a new region mode, do not, for example, allow > 'ram' decoders to be assigned to 'pmem' regions and vice versa. > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > --- > drivers/cxl/core/region.c | 6 ++++++ > 1 file changed, 6 insertions(+) Looks good, Reviewed-by: Vishal Verma <vishal.l.verma@intel.com> > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index c9e7f05caa0f..53d6dbe4de6d 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -1217,6 +1217,12 @@ static int cxl_region_attach(struct cxl_region *cxlr, > struct cxl_dport *dport; > int i, rc = -ENXIO; > > + if (cxled->mode != cxlr->mode) { > + dev_dbg(&cxlr->dev, "%s region mode: %d mismatch: %d\n", > + dev_name(&cxled->cxld.dev), cxlr->mode, cxled->mode); > + return -EINVAL; > + } > + > if (cxled->mode == CXL_DECODER_DEAD) { > dev_dbg(&cxlr->dev, "%s dead\n", dev_name(&cxled->cxld.dev)); > return -ENODEV; >
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c index c9e7f05caa0f..53d6dbe4de6d 100644 --- a/drivers/cxl/core/region.c +++ b/drivers/cxl/core/region.c @@ -1217,6 +1217,12 @@ static int cxl_region_attach(struct cxl_region *cxlr, struct cxl_dport *dport; int i, rc = -ENXIO; + if (cxled->mode != cxlr->mode) { + dev_dbg(&cxlr->dev, "%s region mode: %d mismatch: %d\n", + dev_name(&cxled->cxld.dev), cxlr->mode, cxled->mode); + return -EINVAL; + } + if (cxled->mode == CXL_DECODER_DEAD) { dev_dbg(&cxlr->dev, "%s dead\n", dev_name(&cxled->cxld.dev)); return -ENODEV;
In preparation for a new region mode, do not, for example, allow 'ram' decoders to be assigned to 'pmem' regions and vice versa. Signed-off-by: Dan Williams <dan.j.williams@intel.com> --- drivers/cxl/core/region.c | 6 ++++++ 1 file changed, 6 insertions(+)