[RFC PATCH 0/8] cxl: Introduce HDM decoder emulation from DVSEC range registers

[RFC PATCH 0/8] cxl: Introduce HDM decoder emulation from DVSEC range registers

[Dave Jiang]

This series provides the emulation of HDM decoders from the programmed range
registers. From CXL 3.0 spec 8.1.3.8, there can be up to 2 ranges programmed.
Some device may not implement HDM decoder registers and some may not be
programmed by the BIOS. Under such scenarios, if one of more range registers
are programmed, then we can create an emulated HDM decoder per active range
indicated by the range registers. The emulated HDM decoders will show up as
locked and cannot be reprogrammed.

Below is a table that indicates different scenarios the driver may encounter:

rr: Range registers not programmed
hdm: HDM decoders not programmed
RR: Range registers programmed by BIOS
HDM: HDM decoders programmed by BIOS

emulate HDM: Create HDM decoder software structs and use values from range registers.
keep HDM: Populate HDM decoder software structs with values in HDM decoder registers.

rr             RR             rr hdm	 rr HDM	     RR hdm        RR HDM
unsupported    emulate HDM    keep HDM	 keep HDM    emulate HDM   keep HDM

This series is based on the current RCD work [1] that's going through upstream review.
For convenience, the kernel branch can be retrieved here [2].

[1]: https://lore.kernel.org/linux-cxl/Y4ePZD776yXv2rG3@rric.localdomain/T/#t
[2]: https://git.kernel.org/pub/scm/linux/kernel/git/djiang/linux.git/log/?h=cxl-rch

---

Dave Jiang (8):
      cxl: break out range register decoding from cxl_hdm_decode_init()
      cxl: export cxl_dvsec_rr_decode() to cxl_port
      cxl: refactor cxl_hdm_decode_init()
      cxl: emulate HDM decoder from DVSEC range registers
      cxl: create emulated cxl_hdm for devices that do not have HDM decoders
      cxl: create emulated decoders for devices without HDM decoders
      cxl: suppress component register discovery failure warning for RCD
      cxl: remove locked check for dvsec_range_allowed()


 drivers/cxl/core/hdm.c | 115 +++++++++++++++++++++++---
 drivers/cxl/core/pci.c | 179 +++++++++++++++++------------------------
 drivers/cxl/cxl.h      |  20 ++++-
 drivers/cxl/cxlmem.h   |  13 +--
 drivers/cxl/cxlpci.h   |   3 +-
 drivers/cxl/pci.c      |   2 +-
 drivers/cxl/port.c     |  19 +++--
 7 files changed, 215 insertions(+), 136 deletions(-)

--

[RFC PATCH 1/8] cxl: break out range register decoding from cxl_hdm_decode_init()

[Dave Jiang]

There are 2 scenarios that requires additional handling. 1. A device that
has active ranges in DVSEC range registers (RR) but no HDM decoder register
block. 2. A device that has both RR active and HDM, but the HDM decoders are
not programmed. The goal is to create emulated decoder software structs
based on the RR.

Move the CXL DVSEC range register decoding code block from
cxl_hdm_decode_init() to its own function. Refactor code in preparation for
the HDM decoder emulation.  There is no functionality change to the code.
Name the new function to cxl_dvsec_rr_decode().

The only change is to set range->start to CXL_RESOURCE_NONE if the range is
not programmed correctly or active.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/cxl/core/pci.c |   55 ++++++++++++++++++++++++++++++------------------
 1 file changed, 34 insertions(+), 21 deletions(-)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 0dbbe8d39b07..d674ddfe141c 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -142,10 +142,9 @@ int cxl_await_media_ready(struct cxl_dev_state *cxlds)
 }
 EXPORT_SYMBOL_NS_GPL(cxl_await_media_ready, CXL);
 
-static int wait_for_valid(struct cxl_dev_state *cxlds)
+static int wait_for_valid(struct pci_dev *pdev, int d)
 {
-	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
-	int d = cxlds->cxl_dvsec, rc;
+	int rc;
 	u32 val;
 
 	/*
@@ -335,20 +334,12 @@ static bool __cxl_hdm_decode_init(struct cxl_dev_state *cxlds,
 	return true;
 }
 
-/**
- * cxl_hdm_decode_init() - Setup HDM decoding for the endpoint
- * @cxlds: Device state
- * @cxlhdm: Mapped HDM decoder Capability
- *
- * Try to enable the endpoint's HDM Decoder Capability
- */
-int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm)
+
+static int cxl_dvsec_rr_decode(struct pci_dev *pdev, int d,
+			       struct cxl_endpoint_dvsec_info *info)
 {
-	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
-	struct cxl_endpoint_dvsec_info info = { 0 };
 	int hdm_count, rc, i, ranges = 0;
 	struct device *dev = &pdev->dev;
-	int d = cxlds->cxl_dvsec;
 	u16 cap, ctrl;
 
 	if (!d) {
@@ -379,7 +370,7 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm)
 	if (!hdm_count || hdm_count > 2)
 		return -EINVAL;
 
-	rc = wait_for_valid(cxlds);
+	rc = wait_for_valid(pdev, d);
 	if (rc) {
 		dev_dbg(dev, "Failure awaiting MEM_INFO_VALID (%d)\n", rc);
 		return rc;
@@ -390,9 +381,9 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm)
 	 * disabled, and they will remain moot after the HDM Decoder
 	 * capability is enabled.
 	 */
-	info.mem_enabled = FIELD_GET(CXL_DVSEC_MEM_ENABLE, ctrl);
-	if (!info.mem_enabled)
-		goto hdm_init;
+	info->mem_enabled = FIELD_GET(CXL_DVSEC_MEM_ENABLE, ctrl);
+	if (!info->mem_enabled)
+		return 0;
 
 	for (i = 0; i < hdm_count; i++) {
 		u64 base, size;
@@ -426,22 +417,44 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm)
 
 		base |= temp & CXL_DVSEC_MEM_BASE_LOW_MASK;
 
-		info.dvsec_range[i] = (struct range) {
+		info->dvsec_range[i] = (struct range) {
 			.start = base,
 			.end = base + size - 1
 		};
 
 		if (size)
 			ranges++;
+		else
+			info->dvsec_range[i].start = CXL_RESOURCE_NONE;
 	}
 
-	info.ranges = ranges;
+	info->ranges = ranges;
+	return 0;
+}
+
+/**
+ * cxl_hdm_decode_init() - Setup HDM decoding for the endpoint
+ * @cxlds: Device state
+ * @cxlhdm: Mapped HDM decoder Capability
+ *
+ * Try to enable the endpoint's HDM Decoder Capability
+ */
+int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm)
+{
+	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
+	struct cxl_endpoint_dvsec_info info = { 0 };
+	int rc;
+	struct device *dev = &pdev->dev;
+	int d = cxlds->cxl_dvsec;
+
+	rc = cxl_dvsec_rr_decode(pdev, d, &info);
+	if (rc < 0)
+		return rc;
 
 	/*
 	 * If DVSEC ranges are being used instead of HDM decoder registers there
 	 * is no use in trying to manage those.
 	 */
-hdm_init:
 	if (!__cxl_hdm_decode_init(cxlds, cxlhdm, &info)) {
 		dev_err(dev,
 			"Legacy range registers configuration prevents HDM operation.\n");

Re: [RFC PATCH 1/8] cxl: break out range register decoding from cxl_hdm_decode_init()

[Jonathan Cameron]

On Wed, 30 Nov 2022 16:12:20 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> There are 2 scenarios that requires additional handling. 1. A device that
> has active ranges in DVSEC range registers (RR) but no HDM decoder register
> block. 2. A device that has both RR active and HDM, but the HDM decoders are
> not programmed. The goal is to create emulated decoder software structs
> based on the RR.
> 
> Move the CXL DVSEC range register decoding code block from
> cxl_hdm_decode_init() to its own function. Refactor code in preparation for
> the HDM decoder emulation.  There is no functionality change to the code.
> Name the new function to cxl_dvsec_rr_decode().
> 
> The only change is to set range->start to CXL_RESOURCE_NONE if the range is
> not programmed correctly or active.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

Hi Dave,

I think this refactor, whilst fairly minimal reveals some places
where with a slightly more invasive set of changes we can improve the resulting
code.

Jonathan

> ---


> -int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm)
> +
> +static int cxl_dvsec_rr_decode(struct pci_dev *pdev, int d,
> +			       struct cxl_endpoint_dvsec_info *info)
>  {
...

>  	for (i = 0; i < hdm_count; i++) {
>  		u64 base, size;
> @@ -426,22 +417,44 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm)
>  
>  		base |= temp & CXL_DVSEC_MEM_BASE_LOW_MASK;
>  
> -		info.dvsec_range[i] = (struct range) {
> +		info->dvsec_range[i] = (struct range) {
>  			.start = base,
>  			.end = base + size - 1
>  		};
>  
>  		if (size)
>  			ranges++;
> +		else
> +			info->dvsec_range[i].start = CXL_RESOURCE_NONE;

The following might become irrelevant after later patches in series...

Seems a little odd to do it this way for the !size case and set it
directly above for the case where size is non zero.  Also, is there any
purpose to setting end?
Perhaps just sent whole thing down here and set end to the magic flag?

		if (size) {
			info->dvsec_range[i] = (struct range) {
				.start = base,
				.end = base + size - 1,
			};
			ranges++;
		} else {
			info->dvsec_range[i] = (struct range) {
				.start = CXL_RESOURCE_NONE,
				.end = CXL_RESOURCE_NONE,
			};
		}

or for a more major refactor short cut the !size case and don't bother
reading the pci registers values that are going to be thrown away
anyway.

		if (!size) {
			info->dvsec_range[i] = (struct range) {
				.start = CXL_RESOURCE_NONE,
				.end = CXL_RESOURCE_NONE,
			};
			continue;
		}

		rc = pci_read_config_dword(
			pdev, d + CXL_DVSEC_RANGE_BASE_HIGH(i), &temp);
		if (rc)
			return rc;

		...

		info->dvsec_range[i] = (struct range) {
			.start = base,
			.end = base + size - 1,
		};
		ranges++;
	}

>  	}
>  
> -	info.ranges = ranges;
> +	info->ranges = ranges;

Trivial but I would like a blank line here.

> +	return 0;
> +}
> +
> +/**
> + * cxl_hdm_decode_init() - Setup HDM decoding for the endpoint
> + * @cxlds: Device state
> + * @cxlhdm: Mapped HDM decoder Capability
> + *
> + * Try to enable the endpoint's HDM Decoder Capability
> + */
> +int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm)
> +{
> +	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
> +	struct cxl_endpoint_dvsec_info info = { 0 };
> +	int rc;

Trivial but probably want to reorder these to maintain the reverse xmas tree.

> +	struct device *dev = &pdev->dev;
> +	int d = cxlds->cxl_dvsec;
> +
> +	rc = cxl_dvsec_rr_decode(pdev, d, &info);
> +	if (rc < 0)
> +		return rc;
>  
>  	/*
>  	 * If DVSEC ranges are being used instead of HDM decoder registers there
>  	 * is no use in trying to manage those.
>  	 */
> -hdm_init:
>  	if (!__cxl_hdm_decode_init(cxlds, cxlhdm, &info)) {
>  		dev_err(dev,
>  			"Legacy range registers configuration prevents HDM operation.\n");
> 
> 

Re: [RFC PATCH 1/8] cxl: break out range register decoding from cxl_hdm_decode_init()

[Dave Jiang]

On 12/19/22 8:59 AM, Jonathan Cameron wrote:
> On Wed, 30 Nov 2022 16:12:20 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> There are 2 scenarios that requires additional handling. 1. A device that
>> has active ranges in DVSEC range registers (RR) but no HDM decoder register
>> block. 2. A device that has both RR active and HDM, but the HDM decoders are
>> not programmed. The goal is to create emulated decoder software structs
>> based on the RR.
>>
>> Move the CXL DVSEC range register decoding code block from
>> cxl_hdm_decode_init() to its own function. Refactor code in preparation for
>> the HDM decoder emulation.  There is no functionality change to the code.
>> Name the new function to cxl_dvsec_rr_decode().
>>
>> The only change is to set range->start to CXL_RESOURCE_NONE if the range is
>> not programmed correctly or active.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> 
> Hi Dave,
> 
> I think this refactor, whilst fairly minimal reveals some places
> where with a slightly more invasive set of changes we can improve the resulting
> code.
> 
> Jonathan
> 
>> ---
> 
> 
>> -int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm)
>> +
>> +static int cxl_dvsec_rr_decode(struct pci_dev *pdev, int d,
>> +			       struct cxl_endpoint_dvsec_info *info)
>>   {
> ...
> 
>>   	for (i = 0; i < hdm_count; i++) {
>>   		u64 base, size;
>> @@ -426,22 +417,44 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm)
>>   
>>   		base |= temp & CXL_DVSEC_MEM_BASE_LOW_MASK;
>>   
>> -		info.dvsec_range[i] = (struct range) {
>> +		info->dvsec_range[i] = (struct range) {
>>   			.start = base,
>>   			.end = base + size - 1
>>   		};
>>   
>>   		if (size)
>>   			ranges++;
>> +		else
>> +			info->dvsec_range[i].start = CXL_RESOURCE_NONE;
> 
> The following might become irrelevant after later patches in series...
> 
> Seems a little odd to do it this way for the !size case and set it
> directly above for the case where size is non zero.  Also, is there any
> purpose to setting end?
> Perhaps just sent whole thing down here and set end to the magic flag?
> 
> 		if (size) {
> 			info->dvsec_range[i] = (struct range) {
> 				.start = base,
> 				.end = base + size - 1,
> 			};
> 			ranges++;
> 		} else {
> 			info->dvsec_range[i] = (struct range) {
> 				.start = CXL_RESOURCE_NONE,
> 				.end = CXL_RESOURCE_NONE,
> 			};
> 		}
> 
> or for a more major refactor short cut the !size case and don't bother
> reading the pci registers values that are going to be thrown away
> anyway.
> 
> 		if (!size) {
> 			info->dvsec_range[i] = (struct range) {
> 				.start = CXL_RESOURCE_NONE,
> 				.end = CXL_RESOURCE_NONE,
> 			};
> 			continue;
> 		}
> 
> 		rc = pci_read_config_dword(
> 			pdev, d + CXL_DVSEC_RANGE_BASE_HIGH(i), &temp);
> 		if (rc)
> 			return rc;
> 
> 		...
> 
> 		info->dvsec_range[i] = (struct range) {
> 			.start = base,
> 			.end = base + size - 1,
> 		};
> 		ranges++;
> 	}

Ok I like this optimization. It makes sense not reading the other 
registers if the range is not enabled.

> 
>>   	}
>>   
>> -	info.ranges = ranges;
>> +	info->ranges = ranges;
> 
> Trivial but I would like a blank line here.

ok

> 
>> +	return 0;
>> +}
>> +
>> +/**
>> + * cxl_hdm_decode_init() - Setup HDM decoding for the endpoint
>> + * @cxlds: Device state
>> + * @cxlhdm: Mapped HDM decoder Capability
>> + *
>> + * Try to enable the endpoint's HDM Decoder Capability
>> + */
>> +int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm)
>> +{
>> +	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
>> +	struct cxl_endpoint_dvsec_info info = { 0 };
>> +	int rc;
> 
> Trivial but probably want to reorder these to maintain the reverse xmas tree.

ok

> 
>> +	struct device *dev = &pdev->dev;
>> +	int d = cxlds->cxl_dvsec;
>> +
>> +	rc = cxl_dvsec_rr_decode(pdev, d, &info);
>> +	if (rc < 0)
>> +		return rc;
>>   
>>   	/*
>>   	 * If DVSEC ranges are being used instead of HDM decoder registers there
>>   	 * is no use in trying to manage those.
>>   	 */
>> -hdm_init:
>>   	if (!__cxl_hdm_decode_init(cxlds, cxlhdm, &info)) {
>>   		dev_err(dev,
>>   			"Legacy range registers configuration prevents HDM operation.\n");
>>
>>
> 

[RFC PATCH 2/8] cxl: export cxl_dvsec_rr_decode() to cxl_port

[Dave Jiang]

Call cxl_dvsec_rr_decode() in the beginning of cxl_port_probe() and
preserve the decoded information in the 'struct cxl_endpoint_dvsec_info'.
This info can be passed to various functions later on in order to support
the HDM decoder emulation. The invocation of cxl_dvsec_rr_decode() in
cxl_hdm_decode_init() is removed and a pointer to the 'struct
cxl_endpoint_dvsec_info' is passed in.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/cxl/core/pci.c |   17 ++++++-----------
 drivers/cxl/cxl.h      |   14 ++++++++++++++
 drivers/cxl/cxlmem.h   |   12 ------------
 drivers/cxl/cxlpci.h   |    3 ++-
 drivers/cxl/port.c     |   15 +++++++++++----
 5 files changed, 33 insertions(+), 28 deletions(-)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index d674ddfe141c..7196b1fcdcfc 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -335,8 +335,8 @@ static bool __cxl_hdm_decode_init(struct cxl_dev_state *cxlds,
 }
 
 
-static int cxl_dvsec_rr_decode(struct pci_dev *pdev, int d,
-			       struct cxl_endpoint_dvsec_info *info)
+int cxl_dvsec_rr_decode(struct pci_dev *pdev, int d,
+			struct cxl_endpoint_dvsec_info *info)
 {
 	int hdm_count, rc, i, ranges = 0;
 	struct device *dev = &pdev->dev;
@@ -431,6 +431,7 @@ static int cxl_dvsec_rr_decode(struct pci_dev *pdev, int d,
 	info->ranges = ranges;
 	return 0;
 }
+EXPORT_SYMBOL_NS_GPL(cxl_dvsec_rr_decode, CXL);
 
 /**
  * cxl_hdm_decode_init() - Setup HDM decoding for the endpoint
@@ -439,23 +440,17 @@ static int cxl_dvsec_rr_decode(struct pci_dev *pdev, int d,
  *
  * Try to enable the endpoint's HDM Decoder Capability
  */
-int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm)
+int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
+			struct cxl_endpoint_dvsec_info *info)
 {
 	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
-	struct cxl_endpoint_dvsec_info info = { 0 };
-	int rc;
 	struct device *dev = &pdev->dev;
-	int d = cxlds->cxl_dvsec;
-
-	rc = cxl_dvsec_rr_decode(pdev, d, &info);
-	if (rc < 0)
-		return rc;
 
 	/*
 	 * If DVSEC ranges are being used instead of HDM decoder registers there
 	 * is no use in trying to manage those.
 	 */
-	if (!__cxl_hdm_decode_init(cxlds, cxlhdm, &info)) {
+	if (!__cxl_hdm_decode_init(cxlds, cxlhdm, info)) {
 		dev_err(dev,
 			"Legacy range registers configuration prevents HDM operation.\n");
 		return -EBUSY;
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index d94635e43a50..b6caf1cfb3e9 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -591,10 +591,24 @@ int cxl_decoder_add_locked(struct cxl_decoder *cxld, int *target_map);
 int cxl_decoder_autoremove(struct device *host, struct cxl_decoder *cxld);
 int cxl_endpoint_autoremove(struct cxl_memdev *cxlmd, struct cxl_port *endpoint);
 
+/**
+  * struct cxl_endpoint_dvsec_info - Cached DVSEC info
+  * @mem_enabled: cached value of mem_enabled in the DVSEC, PCIE_DEVICE
+  * @ranges: Number of active HDM ranges this device uses.
+  * @dvsec_range: cached attributes of the ranges in the DVSEC, PCIE_DEVICE
+  */
+struct cxl_endpoint_dvsec_info {
+	bool mem_enabled;
+	int ranges;
+	struct range dvsec_range[2];
+};
+
 struct cxl_hdm;
 struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port);
 int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm);
 int devm_cxl_add_passthrough_decoder(struct cxl_port *port);
+int cxl_dvsec_rr_decode(struct pci_dev *pdev, int dvsec,
+			struct cxl_endpoint_dvsec_info *info);
 
 bool is_cxl_region(struct device *dev);
 
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 35d485d041f0..fd8ed573fbf9 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -179,18 +179,6 @@ static inline int cxl_mbox_cmd_rc2errno(struct cxl_mbox_cmd *mbox_cmd)
  */
 #define CXL_CAPACITY_MULTIPLIER SZ_256M
 
-/**
- * struct cxl_endpoint_dvsec_info - Cached DVSEC info
- * @mem_enabled: cached value of mem_enabled in the DVSEC, PCIE_DEVICE
- * @ranges: Number of active HDM ranges this device uses.
- * @dvsec_range: cached attributes of the ranges in the DVSEC, PCIE_DEVICE
- */
-struct cxl_endpoint_dvsec_info {
-	bool mem_enabled;
-	int ranges;
-	struct range dvsec_range[2];
-};
-
 /**
  * struct cxl_dev_state - The driver device state
  *
diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h
index eec597dbe763..02a39a2609b7 100644
--- a/drivers/cxl/cxlpci.h
+++ b/drivers/cxl/cxlpci.h
@@ -73,6 +73,7 @@ static inline resource_size_t cxl_regmap_to_base(struct pci_dev *pdev,
 
 int devm_cxl_port_enumerate_dports(struct cxl_port *port);
 struct cxl_dev_state;
-int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm);
+int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
+			struct cxl_endpoint_dvsec_info *info);
 void read_cdat_data(struct cxl_port *port);
 #endif /* __CXL_PCI_H__ */
diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
index 5453771bf330..f899d62a91af 100644
--- a/drivers/cxl/port.c
+++ b/drivers/cxl/port.c
@@ -32,7 +32,10 @@ static void schedule_detach(void *cxlmd)
 
 static int cxl_port_probe(struct device *dev)
 {
+	struct cxl_endpoint_dvsec_info info = { 0 };
 	struct cxl_port *port = to_cxl_port(dev);
+	struct cxl_dev_state *cxlds;
+	struct cxl_memdev *cxlmd;
 	struct cxl_hdm *cxlhdm;
 	int rc;
 
@@ -43,6 +46,13 @@ static int cxl_port_probe(struct device *dev)
 			return rc;
 		if (rc == 1)
 			return devm_cxl_add_passthrough_decoder(port);
+	} else {
+		cxlmd = to_cxl_memdev(port->uport);
+		cxlds = cxlmd->cxlds;
+		rc = cxl_dvsec_rr_decode(to_pci_dev(cxlds->dev),
+					 cxlds->cxl_dvsec, &info);
+		if (rc < 0)
+			return rc;
 	}
 
 	cxlhdm = devm_cxl_setup_hdm(port);
@@ -50,9 +60,6 @@ static int cxl_port_probe(struct device *dev)
 		return PTR_ERR(cxlhdm);
 
 	if (is_cxl_endpoint(port)) {
-		struct cxl_memdev *cxlmd = to_cxl_memdev(port->uport);
-		struct cxl_dev_state *cxlds = cxlmd->cxlds;
-
 		/* Cache the data early to ensure is_visible() works */
 		read_cdat_data(port);
 
@@ -61,7 +68,7 @@ static int cxl_port_probe(struct device *dev)
 		if (rc)
 			return rc;
 
-		rc = cxl_hdm_decode_init(cxlds, cxlhdm);
+		rc = cxl_hdm_decode_init(cxlds, cxlhdm, &info);
 		if (rc)
 			return rc;
 

Re: [RFC PATCH 2/8] cxl: export cxl_dvsec_rr_decode() to cxl_port

[Jonathan Cameron]

On Wed, 30 Nov 2022 16:12:26 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> Call cxl_dvsec_rr_decode() in the beginning of cxl_port_probe() and
> preserve the decoded information in the 'struct cxl_endpoint_dvsec_info'.
This confused me a little as it already stores info in such a structure.
Perhaps reword as

preserve the decoded information in a local 'struct cxl_endpoint_dvsec_info'.

> This info can be passed to various functions later on in order to support
> the HDM decoder emulation. The invocation of cxl_dvsec_rr_decode() in
> cxl_hdm_decode_init() is removed and a pointer to the 'struct
> cxl_endpoint_dvsec_info' is passed in.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

A few comments inline.  Whilst this is an RFC, given I'm reading it
I'll give it a fullish review.

Jonathan

> ---
>  drivers/cxl/core/pci.c |   17 ++++++-----------
>  drivers/cxl/cxl.h      |   14 ++++++++++++++
>  drivers/cxl/cxlmem.h   |   12 ------------
>  drivers/cxl/cxlpci.h   |    3 ++-
>  drivers/cxl/port.c     |   15 +++++++++++----
>  5 files changed, 33 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index d674ddfe141c..7196b1fcdcfc 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -335,8 +335,8 @@ static bool __cxl_hdm_decode_init(struct cxl_dev_state *cxlds,
>  }
>  
>  
> -static int cxl_dvsec_rr_decode(struct pci_dev *pdev, int d,
> -			       struct cxl_endpoint_dvsec_info *info)
> +int cxl_dvsec_rr_decode(struct pci_dev *pdev, int d,
> +			struct cxl_endpoint_dvsec_info *info)
>  {
>  	int hdm_count, rc, i, ranges = 0;
>  	struct device *dev = &pdev->dev;
> @@ -431,6 +431,7 @@ static int cxl_dvsec_rr_decode(struct pci_dev *pdev, int d,
>  	info->ranges = ranges;
>  	return 0;
>  }
> +EXPORT_SYMBOL_NS_GPL(cxl_dvsec_rr_decode, CXL);
>  
>  /**
>   * cxl_hdm_decode_init() - Setup HDM decoding for the endpoint
> @@ -439,23 +440,17 @@ static int cxl_dvsec_rr_decode(struct pci_dev *pdev, int d,
>   *
>   * Try to enable the endpoint's HDM Decoder Capability
>   */
> -int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm)
> +int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
> +			struct cxl_endpoint_dvsec_info *info)

Update the docs for the new parameter.

>  {
>  	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
> -	struct cxl_endpoint_dvsec_info info = { 0 };
> -	int rc;
>  	struct device *dev = &pdev->dev;
> -	int d = cxlds->cxl_dvsec;
> -
> -	rc = cxl_dvsec_rr_decode(pdev, d, &info);
> -	if (rc < 0)
> -		return rc;
>  
>  	/*
>  	 * If DVSEC ranges are being used instead of HDM decoder registers there
>  	 * is no use in trying to manage those.
>  	 */
> -	if (!__cxl_hdm_decode_init(cxlds, cxlhdm, &info)) {
> +	if (!__cxl_hdm_decode_init(cxlds, cxlhdm, info)) {
>  		dev_err(dev,
>  			"Legacy range registers configuration prevents HDM operation.\n");
>  		return -EBUSY;

...


>  void read_cdat_data(struct cxl_port *port);
>  #endif /* __CXL_PCI_H__ */
> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
> index 5453771bf330..f899d62a91af 100644
> --- a/drivers/cxl/port.c
> +++ b/drivers/cxl/port.c
> @@ -32,7 +32,10 @@ static void schedule_detach(void *cxlmd)
>  
>  static int cxl_port_probe(struct device *dev)
>  {
> +	struct cxl_endpoint_dvsec_info info = { 0 };
>  	struct cxl_port *port = to_cxl_port(dev);
> +	struct cxl_dev_state *cxlds;
> +	struct cxl_memdev *cxlmd;
>  	struct cxl_hdm *cxlhdm;
>  	int rc;
>  
> @@ -43,6 +46,13 @@ static int cxl_port_probe(struct device *dev)
>  			return rc;
>  		if (rc == 1)
>  			return devm_cxl_add_passthrough_decoder(port);
> +	} else {

I'd suggest flipping logic of the if so we have
two clearly matched
	if (is_cxl_endpoint(port)
blocks. That should make it more obvious that we are safe
in setting cxlmd and cxlds here and using it the later one.

I don't think a compiler can tell that is_cxl_endpoint(port)
will return the same in both cases so you might want to use
a local bool so that is only called once.  Otherwise static
analysis might throw up a false positive. 



> +		cxlmd = to_cxl_memdev(port->uport);
> +		cxlds = cxlmd->cxlds;
> +		rc = cxl_dvsec_rr_decode(to_pci_dev(cxlds->dev),
> +					 cxlds->cxl_dvsec, &info);
> +		if (rc < 0)
> +			return rc;
>  	}
>  
>  	cxlhdm = devm_cxl_setup_hdm(port);
> @@ -50,9 +60,6 @@ static int cxl_port_probe(struct device *dev)
>  		return PTR_ERR(cxlhdm);
>  
>  	if (is_cxl_endpoint(port)) {
> -		struct cxl_memdev *cxlmd = to_cxl_memdev(port->uport);
> -		struct cxl_dev_state *cxlds = cxlmd->cxlds;
> -
>  		/* Cache the data early to ensure is_visible() works */
>  		read_cdat_data(port);
>  
> @@ -61,7 +68,7 @@ static int cxl_port_probe(struct device *dev)
>  		if (rc)
>  			return rc;
>  
> -		rc = cxl_hdm_decode_init(cxlds, cxlhdm);
> +		rc = cxl_hdm_decode_init(cxlds, cxlhdm, &info);
>  		if (rc)
>  			return rc;
>  
> 
> 

[RFC PATCH 3/8] cxl: refactor cxl_hdm_decode_init()

[Dave Jiang]

With the previous refactoring of DVSEC range registers out of
cxl_hdm_decode_init(), it basically becomes a skeleton function. Squash
__cxl_hdm_decode_init() with cxl_hdm_decode_init() to simplify the code.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/cxl/core/pci.c |  138 ++++++++++++++++++++----------------------------
 1 file changed, 57 insertions(+), 81 deletions(-)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 7196b1fcdcfc..385dbe9bd5f2 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -260,81 +260,6 @@ static int devm_cxl_enable_hdm(struct device *host, struct cxl_hdm *cxlhdm)
 	return devm_add_action_or_reset(host, disable_hdm, cxlhdm);
 }
 
-static bool __cxl_hdm_decode_init(struct cxl_dev_state *cxlds,
-				  struct cxl_hdm *cxlhdm,
-				  struct cxl_endpoint_dvsec_info *info)
-{
-	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
-	struct cxl_port *port = cxlhdm->port;
-	struct device *dev = cxlds->dev;
-	struct cxl_port *root;
-	int i, rc, allowed;
-	u32 global_ctrl;
-
-	global_ctrl = readl(hdm + CXL_HDM_DECODER_CTRL_OFFSET);
-
-	/*
-	 * If the HDM Decoder Capability is already enabled then assume
-	 * that some other agent like platform firmware set it up.
-	 */
-	if (global_ctrl & CXL_HDM_DECODER_ENABLE) {
-		rc = devm_cxl_enable_mem(&port->dev, cxlds);
-		if (rc)
-			return false;
-		return true;
-	}
-
-	root = to_cxl_port(port->dev.parent);
-	while (!is_cxl_root(root) && is_cxl_port(root->dev.parent))
-		root = to_cxl_port(root->dev.parent);
-	if (!is_cxl_root(root)) {
-		dev_err(dev, "Failed to acquire root port for HDM enable\n");
-		return false;
-	}
-
-	for (i = 0, allowed = 0; info->mem_enabled && i < info->ranges; i++) {
-		struct device *cxld_dev;
-
-		cxld_dev = device_find_child(&root->dev, &info->dvsec_range[i],
-					     dvsec_range_allowed);
-		if (!cxld_dev) {
-			dev_dbg(dev, "DVSEC Range%d denied by platform\n", i);
-			continue;
-		}
-		dev_dbg(dev, "DVSEC Range%d allowed by platform\n", i);
-		put_device(cxld_dev);
-		allowed++;
-	}
-
-	if (!allowed) {
-		cxl_set_mem_enable(cxlds, 0);
-		info->mem_enabled = 0;
-	}
-
-	/*
-	 * Per CXL 2.0 Section 8.1.3.8.3 and 8.1.3.8.4 DVSEC CXL Range 1 Base
-	 * [High,Low] when HDM operation is enabled the range register values
-	 * are ignored by the device, but the spec also recommends matching the
-	 * DVSEC Range 1,2 to HDM Decoder Range 0,1. So, non-zero info->ranges
-	 * are expected even though Linux does not require or maintain that
-	 * match. If at least one DVSEC range is enabled and allowed, skip HDM
-	 * Decoder Capability Enable.
-	 */
-	if (info->mem_enabled)
-		return false;
-
-	rc = devm_cxl_enable_hdm(&port->dev, cxlhdm);
-	if (rc)
-		return false;
-
-	rc = devm_cxl_enable_mem(&port->dev, cxlds);
-	if (rc)
-		return false;
-
-	return true;
-}
-
-
 int cxl_dvsec_rr_decode(struct pci_dev *pdev, int d,
 			struct cxl_endpoint_dvsec_info *info)
 {
@@ -445,17 +370,68 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
 {
 	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
 	struct device *dev = &pdev->dev;
+	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
+	struct cxl_port *port = cxlhdm->port;
+	struct cxl_port *root;
+	int i, rc, allowed;
+	u32 global_ctrl;
+
+	global_ctrl = readl(hdm + CXL_HDM_DECODER_CTRL_OFFSET);
 
 	/*
-	 * If DVSEC ranges are being used instead of HDM decoder registers there
-	 * is no use in trying to manage those.
+	 * If the HDM Decoder Capability is already enabled then assume
+	 * that some other agent like platform firmware set it up.
 	 */
-	if (!__cxl_hdm_decode_init(cxlds, cxlhdm, info)) {
-		dev_err(dev,
-			"Legacy range registers configuration prevents HDM operation.\n");
-		return -EBUSY;
+	if (global_ctrl & CXL_HDM_DECODER_ENABLE)
+		return devm_cxl_enable_mem(&port->dev, cxlds);
+
+	root = to_cxl_port(port->dev.parent);
+	while (!is_cxl_root(root) && is_cxl_port(root->dev.parent))
+		root = to_cxl_port(root->dev.parent);
+	if (!is_cxl_root(root)) {
+		dev_err(dev, "Failed to acquire root port for HDM enable\n");
+		return -ENODEV;
 	}
 
+	for (i = 0, allowed = 0; info->mem_enabled && i < info->ranges; i++) {
+		struct device *cxld_dev;
+
+		cxld_dev = device_find_child(&root->dev, &info->dvsec_range[i],
+					     dvsec_range_allowed);
+		if (!cxld_dev) {
+			dev_dbg(dev, "DVSEC Range%d denied by platform\n", i);
+			continue;
+		}
+		dev_dbg(dev, "DVSEC Range%d allowed by platform\n", i);
+		put_device(cxld_dev);
+		allowed++;
+	}
+
+	if (!allowed) {
+		cxl_set_mem_enable(cxlds, 0);
+		info->mem_enabled = 0;
+	}
+
+	/*
+	 * Per CXL 2.0 Section 8.1.3.8.3 and 8.1.3.8.4 DVSEC CXL Range 1 Base
+	 * [High,Low] when HDM operation is enabled the range register values
+	 * are ignored by the device, but the spec also recommends matching the
+	 * DVSEC Range 1,2 to HDM Decoder Range 0,1. So, non-zero info->ranges
+	 * are expected even though Linux does not require or maintain that
+	 * match. If at least one DVSEC range is enabled and allowed, skip HDM
+	 * Decoder Capability Enable.
+	 */
+	if (info->mem_enabled)
+		return -EBUSY;
+
+	rc = devm_cxl_enable_hdm(&port->dev, cxlhdm);
+	if (rc)
+		return rc;
+
+	rc = devm_cxl_enable_mem(&port->dev, cxlds);
+	if (rc)
+		return rc;
+
 	return 0;
 }
 EXPORT_SYMBOL_NS_GPL(cxl_hdm_decode_init, CXL);

Re: [RFC PATCH 3/8] cxl: refactor cxl_hdm_decode_init()

[Jonathan Cameron]

On Wed, 30 Nov 2022 16:12:32 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> With the previous refactoring of DVSEC range registers out of
> cxl_hdm_decode_init(), it basically becomes a skeleton function. Squash
> __cxl_hdm_decode_init() with cxl_hdm_decode_init() to simplify the code.

Should mention briefly that this results in richer (i.e. not all -EBUSY)
error return codes.  One other trivial suggestion below that might well
become irrelevant in later patches.


> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

> @@ -445,17 +370,68 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
>  {

...

> +	rc = devm_cxl_enable_mem(&port->dev, cxlds);
> +	if (rc)
> +		return rc;
> +
>  	return 0;
	return dev_cxl_enable_mem()
assuming this doesn't then need putting back as it is in later patches becasue
you add more stuff before the return 0.

>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_hdm_decode_init, CXL);
> 
> 

[RFC PATCH 4/8] cxl: emulate HDM decoder from DVSEC range registers

[Dave Jiang]

In the case where HDM decoder register block exists but is not programmed
and at the same time the DVSEC range register range is active, populate the
CXL decoder object 'cxl_decoder' with info from DVSEC range registers.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/cxl/core/hdm.c |   33 ++++++++++++++++++++++++++++++---
 drivers/cxl/core/pci.c |   12 ------------
 drivers/cxl/cxl.h      |    3 ++-
 drivers/cxl/port.c     |    2 +-
 4 files changed, 33 insertions(+), 17 deletions(-)

diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index d1d2caea5c62..9773a5efaefd 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -674,9 +674,31 @@ static int cxl_decoder_reset(struct cxl_decoder *cxld)
 	return 0;
 }
 
+static int cxl_setup_hdm_decoder_from_dvsec(struct cxl_port *port,
+					    struct cxl_decoder *cxld, int which,
+					    struct cxl_endpoint_dvsec_info *info)
+{
+	if (!is_cxl_endpoint(port))
+		return -EOPNOTSUPP;
+
+	if (info->dvsec_range[which].start == CXL_RESOURCE_NONE)
+		return -ENXIO;
+
+	cxld->target_type = CXL_DECODER_ACCELERATOR;
+
+	cxld->hpa_range = (struct range) {
+		.start = info->dvsec_range[which].start,
+		.end = info->dvsec_range[which].end,
+	};
+
+	cxld->flags |= CXL_DECODER_F_ENABLE | CXL_DECODER_F_LOCK;
+	port->commit_end = cxld->id;
+	return 0;
+}
+
 static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
 			    int *target_map, void __iomem *hdm, int which,
-			    u64 *dpa_base)
+			    u64 *dpa_base, struct cxl_endpoint_dvsec_info *info)
 {
 	struct cxl_endpoint_decoder *cxled = NULL;
 	u64 size, base, skip, dpa_size;
@@ -712,6 +734,10 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
 		.end = base + size - 1,
 	};
 
+	if (cxled && !committed &&
+	    info->dvsec_range[which].start != CXL_RESOURCE_NONE)
+		return cxl_setup_hdm_decoder_from_dvsec(port, cxld, which, info);
+
 	/* decoders are enabled if committed */
 	if (committed) {
 		cxld->flags |= CXL_DECODER_F_ENABLE;
@@ -785,7 +811,8 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
  * devm_cxl_enumerate_decoders - add decoder objects per HDM register set
  * @cxlhdm: Structure to populate with HDM capabilities
  */
-int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm)
+int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
+				struct cxl_endpoint_dvsec_info *info)
 {
 	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
 	struct cxl_port *port = cxlhdm->port;
@@ -837,7 +864,7 @@ int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm)
 			cxld = &cxlsd->cxld;
 		}
 
-		rc = init_hdm_decoder(port, cxld, target_map, hdm, i, &dpa_base);
+		rc = init_hdm_decoder(port, cxld, target_map, hdm, i, &dpa_base, info);
 		if (rc) {
 			put_device(&cxld->dev);
 			return rc;
diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 385dbe9bd5f2..5e44fe23fa76 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -412,18 +412,6 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
 		info->mem_enabled = 0;
 	}
 
-	/*
-	 * Per CXL 2.0 Section 8.1.3.8.3 and 8.1.3.8.4 DVSEC CXL Range 1 Base
-	 * [High,Low] when HDM operation is enabled the range register values
-	 * are ignored by the device, but the spec also recommends matching the
-	 * DVSEC Range 1,2 to HDM Decoder Range 0,1. So, non-zero info->ranges
-	 * are expected even though Linux does not require or maintain that
-	 * match. If at least one DVSEC range is enabled and allowed, skip HDM
-	 * Decoder Capability Enable.
-	 */
-	if (info->mem_enabled)
-		return -EBUSY;
-
 	rc = devm_cxl_enable_hdm(&port->dev, cxlhdm);
 	if (rc)
 		return rc;
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index b6caf1cfb3e9..3fe1043a7796 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -605,7 +605,8 @@ struct cxl_endpoint_dvsec_info {
 
 struct cxl_hdm;
 struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port);
-int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm);
+int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
+				struct cxl_endpoint_dvsec_info *info);
 int devm_cxl_add_passthrough_decoder(struct cxl_port *port);
 int cxl_dvsec_rr_decode(struct pci_dev *pdev, int dvsec,
 			struct cxl_endpoint_dvsec_info *info);
diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
index f899d62a91af..3dedf3dd534a 100644
--- a/drivers/cxl/port.c
+++ b/drivers/cxl/port.c
@@ -79,7 +79,7 @@ static int cxl_port_probe(struct device *dev)
 		}
 	}
 
-	rc = devm_cxl_enumerate_decoders(cxlhdm);
+	rc = devm_cxl_enumerate_decoders(cxlhdm, &info);
 	if (rc) {
 		dev_err(dev, "Couldn't enumerate decoders (%d)\n", rc);
 		return rc;

Re: [RFC PATCH 4/8] cxl: emulate HDM decoder from DVSEC range registers

[Jonathan Cameron]

On Wed, 30 Nov 2022 16:12:38 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> In the case where HDM decoder register block exists but is not programmed
> and at the same time the DVSEC range register range is active, populate the
> CXL decoder object 'cxl_decoder' with info from DVSEC range registers.

I may be overthinking this...

So I think this results in us enabling hdm decoder registers on
a device that the BIOS already set the range registers for?

I'm not sure the spec guarantees that is a safe operation if accesses are
in flight.
You can imagine a device which goes through an unsafe intermediate state
when switching over from range registers to HDM decoders.  That wouldn't
normally be a problem as we'd not expect traffic in flight, but if the
BIOS already set up a mapping the OS might see that as normal memory which
it is using when this transition occurs.

If just feels like a transition no one will test that might bite us in
future in really hard to detect ways.

> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/cxl/core/hdm.c |   33 ++++++++++++++++++++++++++++++---
>  drivers/cxl/core/pci.c |   12 ------------
>  drivers/cxl/cxl.h      |    3 ++-
>  drivers/cxl/port.c     |    2 +-
>  4 files changed, 33 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index d1d2caea5c62..9773a5efaefd 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -674,9 +674,31 @@ static int cxl_decoder_reset(struct cxl_decoder *cxld)
>  	return 0;
>  }
>  
> +static int cxl_setup_hdm_decoder_from_dvsec(struct cxl_port *port,
> +					    struct cxl_decoder *cxld, int which,
> +					    struct cxl_endpoint_dvsec_info *info)
> +{
> +	if (!is_cxl_endpoint(port))
> +		return -EOPNOTSUPP;
> +
> +	if (info->dvsec_range[which].start == CXL_RESOURCE_NONE)
> +		return -ENXIO;
> +
> +	cxld->target_type = CXL_DECODER_ACCELERATOR;

Why chose type 2 target type?  Definitely needs a comment.
Also would be good to have a precursor patch that moves these
over to the CXL 3.0 naming to incorportate the fun difference
between HDM-DB and HDM-H type 3 devices

CXL_DEVICE_COHERENT_ADDRESS_RANGE
CXL_HOST_ONLY_COHERENT_ADDRESS_RANGE



> +
> +	cxld->hpa_range = (struct range) {
> +		.start = info->dvsec_range[which].start,
> +		.end = info->dvsec_range[which].end,
> +	};
> +
> +	cxld->flags |= CXL_DECODER_F_ENABLE | CXL_DECODER_F_LOCK;
> +	port->commit_end = cxld->id;

blank line before all simple returns like this makes the code
slightly more readable.

> +	return 0;
> +}
> +

...

> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 385dbe9bd5f2..5e44fe23fa76 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -412,18 +412,6 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
>  		info->mem_enabled = 0;
>  	}
>  
> -	/*
> -	 * Per CXL 2.0 Section 8.1.3.8.3 and 8.1.3.8.4 DVSEC CXL Range 1 Base
> -	 * [High,Low] when HDM operation is enabled the range register values
> -	 * are ignored by the device, but the spec also recommends matching the
> -	 * DVSEC Range 1,2 to HDM Decoder Range 0,1. So, non-zero info->ranges
> -	 * are expected even though Linux does not require or maintain that
> -	 * match. If at least one DVSEC range is enabled and allowed, skip HDM
> -	 * Decoder Capability Enable.
> -	 */
> -	if (info->mem_enabled)
> -		return -EBUSY;
> -
Dropping this condition is the bit I'm referring to at the top.  I think
if (info->mem_enabled)
	return 0;

would avoid that transition.

>  	rc = devm_cxl_enable_hdm(&port->dev, cxlhdm);
>  	if (rc)
>  		return rc;

Re: [RFC PATCH 4/8] cxl: emulate HDM decoder from DVSEC range registers

[Dave Jiang]

On 12/19/22 9:42 AM, Jonathan Cameron wrote:
> On Wed, 30 Nov 2022 16:12:38 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> In the case where HDM decoder register block exists but is not programmed
>> and at the same time the DVSEC range register range is active, populate the
>> CXL decoder object 'cxl_decoder' with info from DVSEC range registers.
> 
> I may be overthinking this...
> 
> So I think this results in us enabling hdm decoder registers on
> a device that the BIOS already set the range registers for?

The code don't actually program and enable an unprogrammed HDM decoder. 
It creates a locked HDM decoder structure that's filled with info from 
DVSEC. The interested info will show up in sysfs but there should not be 
any changes WRT existing HDM decoder. The DVSEC range register 
exclusively controls this memory range.

> 
> I'm not sure the spec guarantees that is a safe operation if accesses are
> in flight.
> You can imagine a device which goes through an unsafe intermediate state
> when switching over from range registers to HDM decoders.  That wouldn't
> normally be a problem as we'd not expect traffic in flight, but if the
> BIOS already set up a mapping the OS might see that as normal memory which
> it is using when this transition occurs.
> 
> If just feels like a transition no one will test that might bite us in
> future in really hard to detect ways.
> 
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>   drivers/cxl/core/hdm.c |   33 ++++++++++++++++++++++++++++++---
>>   drivers/cxl/core/pci.c |   12 ------------
>>   drivers/cxl/cxl.h      |    3 ++-
>>   drivers/cxl/port.c     |    2 +-
>>   4 files changed, 33 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
>> index d1d2caea5c62..9773a5efaefd 100644
>> --- a/drivers/cxl/core/hdm.c
>> +++ b/drivers/cxl/core/hdm.c
>> @@ -674,9 +674,31 @@ static int cxl_decoder_reset(struct cxl_decoder *cxld)
>>   	return 0;
>>   }
>>   
>> +static int cxl_setup_hdm_decoder_from_dvsec(struct cxl_port *port,
>> +					    struct cxl_decoder *cxld, int which,
>> +					    struct cxl_endpoint_dvsec_info *info)
>> +{
>> +	if (!is_cxl_endpoint(port))
>> +		return -EOPNOTSUPP;
>> +
>> +	if (info->dvsec_range[which].start == CXL_RESOURCE_NONE)
>> +		return -ENXIO;
>> +
>> +	cxld->target_type = CXL_DECODER_ACCELERATOR;
> 
> Why chose type 2 target type?  Definitely needs a comment.

There are only 2 defines for decoder type. I picked the one that is not 
EXPANDER.

> Also would be good to have a precursor patch that moves these
> over to the CXL 3.0 naming to incorportate the fun difference
> between HDM-DB and HDM-H type 3 devices
> 
> CXL_DEVICE_COHERENT_ADDRESS_RANGE
> CXL_HOST_ONLY_COHERENT_ADDRESS_RANGE

I'll take a look at the spec.
> 
> 
> 
>> +
>> +	cxld->hpa_range = (struct range) {
>> +		.start = info->dvsec_range[which].start,
>> +		.end = info->dvsec_range[which].end,
>> +	};
>> +
>> +	cxld->flags |= CXL_DECODER_F_ENABLE | CXL_DECODER_F_LOCK;
>> +	port->commit_end = cxld->id;
> 
> blank line before all simple returns like this makes the code
> slightly more readable.

ok
> 
>> +	return 0;
>> +}
>> +
> 
> ...
> 
>> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
>> index 385dbe9bd5f2..5e44fe23fa76 100644
>> --- a/drivers/cxl/core/pci.c
>> +++ b/drivers/cxl/core/pci.c
>> @@ -412,18 +412,6 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
>>   		info->mem_enabled = 0;
>>   	}
>>   
>> -	/*
>> -	 * Per CXL 2.0 Section 8.1.3.8.3 and 8.1.3.8.4 DVSEC CXL Range 1 Base
>> -	 * [High,Low] when HDM operation is enabled the range register values
>> -	 * are ignored by the device, but the spec also recommends matching the
>> -	 * DVSEC Range 1,2 to HDM Decoder Range 0,1. So, non-zero info->ranges
>> -	 * are expected even though Linux does not require or maintain that
>> -	 * match. If at least one DVSEC range is enabled and allowed, skip HDM
>> -	 * Decoder Capability Enable.
>> -	 */
>> -	if (info->mem_enabled)
>> -		return -EBUSY;
>> -
> Dropping this condition is the bit I'm referring to at the top.  I think
> if (info->mem_enabled)
> 	return 0;
> 
> would avoid that transition.

ok
> 
>>   	rc = devm_cxl_enable_hdm(&port->dev, cxlhdm);
>>   	if (rc)
>>   		return rc;

Re: [RFC PATCH 4/8] cxl: emulate HDM decoder from DVSEC range registers

[Dave Jiang]

On 1/3/23 4:20 PM, Dave Jiang wrote:
> 
> 
> On 12/19/22 9:42 AM, Jonathan Cameron wrote:
>> On Wed, 30 Nov 2022 16:12:38 -0700
>> Dave Jiang <dave.jiang@intel.com> wrote:
>>
>>> In the case where HDM decoder register block exists but is not 
>>> programmed
>>> and at the same time the DVSEC range register range is active, 
>>> populate the
>>> CXL decoder object 'cxl_decoder' with info from DVSEC range registers.
>>
>> I may be overthinking this...
>>
>> So I think this results in us enabling hdm decoder registers on
>> a device that the BIOS already set the range registers for?
> 
> The code don't actually program and enable an unprogrammed HDM decoder. 
> It creates a locked HDM decoder structure that's filled with info from 
> DVSEC. The interested info will show up in sysfs but there should not be 
> any changes WRT existing HDM decoder. The DVSEC range register 
> exclusively controls this memory range.
> 
>>
>> I'm not sure the spec guarantees that is a safe operation if accesses are
>> in flight.
>> You can imagine a device which goes through an unsafe intermediate state
>> when switching over from range registers to HDM decoders.  That wouldn't
>> normally be a problem as we'd not expect traffic in flight, but if the
>> BIOS already set up a mapping the OS might see that as normal memory 
>> which
>> it is using when this transition occurs.
>>
>> If just feels like a transition no one will test that might bite us in
>> future in really hard to detect ways.
>>
>>>
>>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>>> ---
>>>   drivers/cxl/core/hdm.c |   33 ++++++++++++++++++++++++++++++---
>>>   drivers/cxl/core/pci.c |   12 ------------
>>>   drivers/cxl/cxl.h      |    3 ++-
>>>   drivers/cxl/port.c     |    2 +-
>>>   4 files changed, 33 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
>>> index d1d2caea5c62..9773a5efaefd 100644
>>> --- a/drivers/cxl/core/hdm.c
>>> +++ b/drivers/cxl/core/hdm.c
>>> @@ -674,9 +674,31 @@ static int cxl_decoder_reset(struct cxl_decoder 
>>> *cxld)
>>>       return 0;
>>>   }
>>> +static int cxl_setup_hdm_decoder_from_dvsec(struct cxl_port *port,
>>> +                        struct cxl_decoder *cxld, int which,
>>> +                        struct cxl_endpoint_dvsec_info *info)
>>> +{
>>> +    if (!is_cxl_endpoint(port))
>>> +        return -EOPNOTSUPP;
>>> +
>>> +    if (info->dvsec_range[which].start == CXL_RESOURCE_NONE)
>>> +        return -ENXIO;
>>> +
>>> +    cxld->target_type = CXL_DECODER_ACCELERATOR;
>>
>> Why chose type 2 target type?  Definitely needs a comment.
> 
> There are only 2 defines for decoder type. I picked the one that is not 
> EXPANDER.

Looks like there were some confusion on my part. For purpose of 
emulation here it's sufficient to just set it to generic 
CXL_DECODER_EXPANDER (type 3)?

DJ

> 
>> Also would be good to have a precursor patch that moves these
>> over to the CXL 3.0 naming to incorportate the fun difference
>> between HDM-DB and HDM-H type 3 devices
>>
>> CXL_DEVICE_COHERENT_ADDRESS_RANGE
>> CXL_HOST_ONLY_COHERENT_ADDRESS_RANGE
> 
> I'll take a look at the spec.
>>
>>
>>
>>> +
>>> +    cxld->hpa_range = (struct range) {
>>> +        .start = info->dvsec_range[which].start,
>>> +        .end = info->dvsec_range[which].end,
>>> +    };
>>> +
>>> +    cxld->flags |= CXL_DECODER_F_ENABLE | CXL_DECODER_F_LOCK;
>>> +    port->commit_end = cxld->id;
>>
>> blank line before all simple returns like this makes the code
>> slightly more readable.
> 
> ok
>>
>>> +    return 0;
>>> +}
>>> +
>>
>> ...
>>
>>> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
>>> index 385dbe9bd5f2..5e44fe23fa76 100644
>>> --- a/drivers/cxl/core/pci.c
>>> +++ b/drivers/cxl/core/pci.c
>>> @@ -412,18 +412,6 @@ int cxl_hdm_decode_init(struct cxl_dev_state 
>>> *cxlds, struct cxl_hdm *cxlhdm,
>>>           info->mem_enabled = 0;
>>>       }
>>> -    /*
>>> -     * Per CXL 2.0 Section 8.1.3.8.3 and 8.1.3.8.4 DVSEC CXL Range 1 
>>> Base
>>> -     * [High,Low] when HDM operation is enabled the range register 
>>> values
>>> -     * are ignored by the device, but the spec also recommends 
>>> matching the
>>> -     * DVSEC Range 1,2 to HDM Decoder Range 0,1. So, non-zero 
>>> info->ranges
>>> -     * are expected even though Linux does not require or maintain that
>>> -     * match. If at least one DVSEC range is enabled and allowed, 
>>> skip HDM
>>> -     * Decoder Capability Enable.
>>> -     */
>>> -    if (info->mem_enabled)
>>> -        return -EBUSY;
>>> -
>> Dropping this condition is the bit I'm referring to at the top.  I think
>> if (info->mem_enabled)
>>     return 0;
>>
>> would avoid that transition.
> 
> ok
>>
>>>       rc = devm_cxl_enable_hdm(&port->dev, cxlhdm);
>>>       if (rc)
>>>           return rc;

Re: [RFC PATCH 4/8] cxl: emulate HDM decoder from DVSEC range registers

[Jonathan Cameron]

On Wed, 4 Jan 2023 09:07:41 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> On 1/3/23 4:20 PM, Dave Jiang wrote:
> > 
> > 
> > On 12/19/22 9:42 AM, Jonathan Cameron wrote:  
> >> On Wed, 30 Nov 2022 16:12:38 -0700
> >> Dave Jiang <dave.jiang@intel.com> wrote:
> >>  
> >>> In the case where HDM decoder register block exists but is not 
> >>> programmed
> >>> and at the same time the DVSEC range register range is active, 
> >>> populate the
> >>> CXL decoder object 'cxl_decoder' with info from DVSEC range registers.  
> >>
> >> I may be overthinking this...
> >>
> >> So I think this results in us enabling hdm decoder registers on
> >> a device that the BIOS already set the range registers for?  
> > 
> > The code don't actually program and enable an unprogrammed HDM decoder. 
> > It creates a locked HDM decoder structure that's filled with info from 
> > DVSEC. The interested info will show up in sysfs but there should not be 
> > any changes WRT existing HDM decoder. The DVSEC range register 
> > exclusively controls this memory range.
> >   
> >>
> >> I'm not sure the spec guarantees that is a safe operation if accesses are
> >> in flight.
> >> You can imagine a device which goes through an unsafe intermediate state
> >> when switching over from range registers to HDM decoders.  That wouldn't
> >> normally be a problem as we'd not expect traffic in flight, but if the
> >> BIOS already set up a mapping the OS might see that as normal memory 
> >> which
> >> it is using when this transition occurs.
> >>
> >> If just feels like a transition no one will test that might bite us in
> >> future in really hard to detect ways.
> >>  
> >>>
> >>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> >>> ---
> >>>   drivers/cxl/core/hdm.c |   33 ++++++++++++++++++++++++++++++---
> >>>   drivers/cxl/core/pci.c |   12 ------------
> >>>   drivers/cxl/cxl.h      |    3 ++-
> >>>   drivers/cxl/port.c     |    2 +-
> >>>   4 files changed, 33 insertions(+), 17 deletions(-)
> >>>
> >>> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> >>> index d1d2caea5c62..9773a5efaefd 100644
> >>> --- a/drivers/cxl/core/hdm.c
> >>> +++ b/drivers/cxl/core/hdm.c
> >>> @@ -674,9 +674,31 @@ static int cxl_decoder_reset(struct cxl_decoder 
> >>> *cxld)
> >>>       return 0;
> >>>   }
> >>> +static int cxl_setup_hdm_decoder_from_dvsec(struct cxl_port *port,
> >>> +                        struct cxl_decoder *cxld, int which,
> >>> +                        struct cxl_endpoint_dvsec_info *info)
> >>> +{
> >>> +    if (!is_cxl_endpoint(port))
> >>> +        return -EOPNOTSUPP;
> >>> +
> >>> +    if (info->dvsec_range[which].start == CXL_RESOURCE_NONE)
> >>> +        return -ENXIO;
> >>> +
> >>> +    cxld->target_type = CXL_DECODER_ACCELERATOR;  
> >>
> >> Why chose type 2 target type?  Definitely needs a comment.  
> > 
> > There are only 2 defines for decoder type. I picked the one that is not 
> > EXPANDER.  
> 
> Looks like there were some confusion on my part. For purpose of 
> emulation here it's sufficient to just set it to generic 
> CXL_DECODER_EXPANDER (type 3)?

I think that's the right choice as well.

Jonathan

> 
> DJ
> 
> >   
> >> Also would be good to have a precursor patch that moves these
> >> over to the CXL 3.0 naming to incorportate the fun difference
> >> between HDM-DB and HDM-H type 3 devices
> >>
> >> CXL_DEVICE_COHERENT_ADDRESS_RANGE
> >> CXL_HOST_ONLY_COHERENT_ADDRESS_RANGE  
> > 
> > I'll take a look at the spec.  
> >>
> >>
> >>  
> >>> +
> >>> +    cxld->hpa_range = (struct range) {
> >>> +        .start = info->dvsec_range[which].start,
> >>> +        .end = info->dvsec_range[which].end,
> >>> +    };
> >>> +
> >>> +    cxld->flags |= CXL_DECODER_F_ENABLE | CXL_DECODER_F_LOCK;
> >>> +    port->commit_end = cxld->id;  
> >>
> >> blank line before all simple returns like this makes the code
> >> slightly more readable.  
> > 
> > ok  
> >>  
> >>> +    return 0;
> >>> +}
> >>> +  
> >>
> >> ...
> >>  
> >>> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> >>> index 385dbe9bd5f2..5e44fe23fa76 100644
> >>> --- a/drivers/cxl/core/pci.c
> >>> +++ b/drivers/cxl/core/pci.c
> >>> @@ -412,18 +412,6 @@ int cxl_hdm_decode_init(struct cxl_dev_state 
> >>> *cxlds, struct cxl_hdm *cxlhdm,
> >>>           info->mem_enabled = 0;
> >>>       }
> >>> -    /*
> >>> -     * Per CXL 2.0 Section 8.1.3.8.3 and 8.1.3.8.4 DVSEC CXL Range 1 
> >>> Base
> >>> -     * [High,Low] when HDM operation is enabled the range register 
> >>> values
> >>> -     * are ignored by the device, but the spec also recommends 
> >>> matching the
> >>> -     * DVSEC Range 1,2 to HDM Decoder Range 0,1. So, non-zero 
> >>> info->ranges
> >>> -     * are expected even though Linux does not require or maintain that
> >>> -     * match. If at least one DVSEC range is enabled and allowed, 
> >>> skip HDM
> >>> -     * Decoder Capability Enable.
> >>> -     */
> >>> -    if (info->mem_enabled)
> >>> -        return -EBUSY;
> >>> -  
> >> Dropping this condition is the bit I'm referring to at the top.  I think
> >> if (info->mem_enabled)
> >>     return 0;
> >>
> >> would avoid that transition.  
> > 
> > ok  
> >>  
> >>>       rc = devm_cxl_enable_hdm(&port->dev, cxlhdm);
> >>>       if (rc)
> >>>           return rc;  
> 

[RFC PATCH 5/8] cxl: create emulated cxl_hdm for devices that do not have HDM decoders

[Dave Jiang]

CXL rev3 spec 8.1.3
RCDs may not have HDM register blocks. Create a fake HDM with information
from the CXL PCIe DVSEC registers. The decoder count will be set to the
HDM count retrieved from the DVSEC cap register.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/cxl/core/hdm.c |   27 ++++++++++++++++++++++++++-
 drivers/cxl/core/pci.c |    9 ++++++---
 drivers/cxl/cxl.h      |    3 ++-
 drivers/cxl/cxlmem.h   |    1 +
 drivers/cxl/port.c     |    2 +-
 5 files changed, 36 insertions(+), 6 deletions(-)

diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index 9773a5efaefd..3a9e9b854587 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -96,11 +96,31 @@ static void __iomem *map_hdm_decoder_regs(struct cxl_port *port,
 	return crb + map.hdm_decoder.offset;
 }
 
+static struct cxl_hdm *devm_cxl_setup_emulated_hdm(struct cxl_port *port,
+						   struct cxl_endpoint_dvsec_info *info)
+{
+	struct device *dev = &port->dev;
+	struct cxl_hdm *cxlhdm;
+
+	if (!info->mem_enabled)
+		return ERR_PTR(-ENODEV);
+
+	cxlhdm = devm_kzalloc(dev, sizeof(*cxlhdm), GFP_KERNEL);
+	if (!cxlhdm)
+		return ERR_PTR(-ENOMEM);
+
+	cxlhdm->port = port;
+	cxlhdm->decoder_count = info->ranges;
+	dev_set_drvdata(&port->dev, cxlhdm);
+	return cxlhdm;
+}
+
 /**
  * devm_cxl_setup_hdm - map HDM decoder component registers
  * @port: cxl_port to map
  */
-struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port)
+struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port,
+				   struct cxl_endpoint_dvsec_info *info)
 {
 	struct device *dev = &port->dev;
 	void __iomem *crb, *hdm;
@@ -111,9 +131,14 @@ struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port)
 		return ERR_PTR(-ENOMEM);
 
 	cxlhdm->port = port;
+	if (port->parent_dport->rch)
+		cxlhdm->rcd = true;
 	crb = devm_cxl_iomap_block(dev, port->component_reg_phys,
 				   CXL_COMPONENT_REG_BLOCK_SIZE);
 	if (!crb) {
+		if (info->mem_enabled)
+			return devm_cxl_setup_emulated_hdm(port, info);
+
 		dev_err(dev, "No component registers mapped\n");
 		return ERR_PTR(-ENXIO);
 	}
diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 5e44fe23fa76..009c11b43303 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -374,16 +374,19 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
 	struct cxl_port *port = cxlhdm->port;
 	struct cxl_port *root;
 	int i, rc, allowed;
-	u32 global_ctrl;
+	u32 global_ctrl = 0;
 
-	global_ctrl = readl(hdm + CXL_HDM_DECODER_CTRL_OFFSET);
+	if (hdm)
+		global_ctrl = readl(hdm + CXL_HDM_DECODER_CTRL_OFFSET);
 
 	/*
 	 * If the HDM Decoder Capability is already enabled then assume
 	 * that some other agent like platform firmware set it up.
 	 */
-	if (global_ctrl & CXL_HDM_DECODER_ENABLE)
+	if (global_ctrl & CXL_HDM_DECODER_ENABLE || (!hdm && info->mem_enabled))
 		return devm_cxl_enable_mem(&port->dev, cxlds);
+	else if (!hdm)
+		return -ENODEV;
 
 	root = to_cxl_port(port->dev.parent);
 	while (!is_cxl_root(root) && is_cxl_port(root->dev.parent))
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 3fe1043a7796..b2eea921bd5f 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -604,7 +604,8 @@ struct cxl_endpoint_dvsec_info {
 };
 
 struct cxl_hdm;
-struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port);
+struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port,
+				   struct cxl_endpoint_dvsec_info *info);
 int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
 				struct cxl_endpoint_dvsec_info *info);
 int devm_cxl_add_passthrough_decoder(struct cxl_port *port);
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index fd8ed573fbf9..6d477590559c 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -402,6 +402,7 @@ struct cxl_hdm {
 	unsigned int target_count;
 	unsigned int interleave_mask;
 	struct cxl_port *port;
+	bool rcd;
 };
 
 struct seq_file;
diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
index 3dedf3dd534a..910ebfc19a45 100644
--- a/drivers/cxl/port.c
+++ b/drivers/cxl/port.c
@@ -55,7 +55,7 @@ static int cxl_port_probe(struct device *dev)
 			return rc;
 	}
 
-	cxlhdm = devm_cxl_setup_hdm(port);
+	cxlhdm = devm_cxl_setup_hdm(port, &info);
 	if (IS_ERR(cxlhdm))
 		return PTR_ERR(cxlhdm);
 

Re: [RFC PATCH 5/8] cxl: create emulated cxl_hdm for devices that do not have HDM decoders

[Jonathan Cameron]

On Wed, 30 Nov 2022 16:12:43 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> CXL rev3 spec 8.1.3
> RCDs may not have HDM register blocks. Create a fake HDM with information
> from the CXL PCIe DVSEC registers. The decoder count will be set to the
> HDM count retrieved from the DVSEC cap register.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
One comment inline that probably doesn't matter about setting
the target_count = 1.
If you need it to be 0 (which isn't normally a valid option) then
perhaps a comment on why.

> ---
>  drivers/cxl/core/hdm.c |   27 ++++++++++++++++++++++++++-
>  drivers/cxl/core/pci.c |    9 ++++++---
>  drivers/cxl/cxl.h      |    3 ++-
>  drivers/cxl/cxlmem.h   |    1 +
>  drivers/cxl/port.c     |    2 +-
>  5 files changed, 36 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 9773a5efaefd..3a9e9b854587 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -96,11 +96,31 @@ static void __iomem *map_hdm_decoder_regs(struct cxl_port *port,
>  	return crb + map.hdm_decoder.offset;
>  }
>  
> +static struct cxl_hdm *devm_cxl_setup_emulated_hdm(struct cxl_port *port,
> +						   struct cxl_endpoint_dvsec_info *info)
> +{
> +	struct device *dev = &port->dev;
> +	struct cxl_hdm *cxlhdm;
> +
> +	if (!info->mem_enabled)
> +		return ERR_PTR(-ENODEV);
> +
> +	cxlhdm = devm_kzalloc(dev, sizeof(*cxlhdm), GFP_KERNEL);
> +	if (!cxlhdm)
> +		return ERR_PTR(-ENOMEM);
> +
> +	cxlhdm->port = port;
> +	cxlhdm->decoder_count = info->ranges;

Whilst I assume target_count isn't used, perhaps better to set it here
for consistency as 0 isn't a valid value for the HDM decoder related field.


> +	dev_set_drvdata(&port->dev, cxlhdm);

blank line here preferred.

> +	return cxlhdm;
> +}
> +

[RFC PATCH 6/8] cxl: create emulated decoders for devices without HDM decoders

[Dave Jiang]

CXL rev3.0 spec 8.1.3
RCDs may not have HDM register blocks. Create fake decoders based on CXL
PCIe DVSEC registers. The DVSEC Range Regiters provide the memory range for
these decoder structs. For the RCD, there can be up to 2 decoders depending
on the DVSEC Capability register HDM_count.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/cxl/core/hdm.c |   59 +++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 50 insertions(+), 9 deletions(-)

diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index 3a9e9b854587..60b6c141f514 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -721,6 +721,29 @@ static int cxl_setup_hdm_decoder_from_dvsec(struct cxl_port *port,
 	return 0;
 }
 
+static int init_emulated_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
+				     struct cxl_endpoint_dvsec_info *info, int which)
+{
+	if (info->dvsec_range[which].start == CXL_RESOURCE_NONE)
+		return -ENODEV;
+
+	cxld->hpa_range = (struct range) {
+		.start = info->dvsec_range[which].start,
+		.end = info->dvsec_range[which].end,
+	};
+
+	cxld->flags = CXL_DECODER_F_ENABLE | CXL_DECODER_F_LOCK;
+	cxld->target_type = CXL_DECODER_ACCELERATOR;
+	if (cxld->id != port->commit_end + 1) {
+		dev_warn(&port->dev,
+			 "decoder%d.%d: Committed out of order\n",
+			 port->id, cxld->id);
+		return -ENXIO;
+	}
+	port->commit_end = cxld->id;
+	return 0;
+}
+
 static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
 			    int *target_map, void __iomem *hdm, int which,
 			    u64 *dpa_base, struct cxl_endpoint_dvsec_info *info)
@@ -739,6 +762,13 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
 	if (is_endpoint_decoder(&cxld->dev))
 		cxled = to_cxl_endpoint_decoder(&cxld->dev);
 
+	if (!hdm) {
+		if (cxled)
+			return init_emulated_hdm_decoder(port, cxld, info, which);
+		else
+			return -EINVAL;
+	}
+
 	ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(which));
 	base = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_BASE_LOW_OFFSET(which));
 	size = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(which));
@@ -832,19 +862,15 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
 	return 0;
 }
 
-/**
- * devm_cxl_enumerate_decoders - add decoder objects per HDM register set
- * @cxlhdm: Structure to populate with HDM capabilities
- */
-int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
-				struct cxl_endpoint_dvsec_info *info)
+static void cxl_settle_decoders(struct cxl_hdm *cxlhdm)
 {
 	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
-	struct cxl_port *port = cxlhdm->port;
-	int i, committed;
-	u64 dpa_base = 0;
+	int committed, i;
 	u32 ctrl;
 
+	if (!hdm)
+		return;
+
 	/*
 	 * Since the register resource was recently claimed via request_region()
 	 * be careful about trusting the "not-committed" status until the commit
@@ -861,6 +887,21 @@ int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
 	/* ensure that future checks of committed can be trusted */
 	if (committed != cxlhdm->decoder_count)
 		msleep(20);
+}
+
+/**
+ * devm_cxl_enumerate_decoders - add decoder objects per HDM register set
+ * @cxlhdm: Structure to populate with HDM capabilities
+ */
+int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
+				struct cxl_endpoint_dvsec_info *info)
+{
+	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
+	struct cxl_port *port = cxlhdm->port;
+	int i;
+	u64 dpa_base = 0;
+
+	cxl_settle_decoders(cxlhdm);
 
 	for (i = 0; i < cxlhdm->decoder_count; i++) {
 		int target_map[CXL_DECODER_MAX_INTERLEAVE] = { 0 };

Re: [RFC PATCH 6/8] cxl: create emulated decoders for devices without HDM decoders

[Jonathan Cameron]

On Wed, 30 Nov 2022 16:12:49 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> CXL rev3.0 spec 8.1.3
> RCDs may not have HDM register blocks. Create fake decoders based on CXL
> PCIe DVSEC registers. The DVSEC Range Regiters provide the memory range for

Spell check (not that I can talk about spelling ;)

> these decoder structs. For the RCD, there can be up to 2 decoders depending
> on the DVSEC Capability register HDM_count.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

A few trivial things inline. LGTM otherwise.

> ---
>  drivers/cxl/core/hdm.c |   59 +++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 50 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 3a9e9b854587..60b6c141f514 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -721,6 +721,29 @@ static int cxl_setup_hdm_decoder_from_dvsec(struct cxl_port *port,
>  	return 0;
>  }
>  
> +static int init_emulated_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
> +				     struct cxl_endpoint_dvsec_info *info, int which)
> +{
> +	if (info->dvsec_range[which].start == CXL_RESOURCE_NONE)
> +		return -ENODEV;
> +
> +	cxld->hpa_range = (struct range) {
> +		.start = info->dvsec_range[which].start,
> +		.end = info->dvsec_range[which].end,
> +	};
> +
> +	cxld->flags = CXL_DECODER_F_ENABLE | CXL_DECODER_F_LOCK;
> +	cxld->target_type = CXL_DECODER_ACCELERATOR;

Why accelerator? Comment needed.
blank line

> +	if (cxld->id != port->commit_end + 1) {
> +		dev_warn(&port->dev,
> +			 "decoder%d.%d: Committed out of order\n",
> +			 port->id, cxld->id);
> +		return -ENXIO;
> +	}
blank line

> +	port->commit_end = cxld->id;
blank line.
> +	return 0;
> +}
> +
>  static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>  			    int *target_map, void __iomem *hdm, int which,
>  			    u64 *dpa_base, struct cxl_endpoint_dvsec_info *info)
> @@ -739,6 +762,13 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>  	if (is_endpoint_decoder(&cxld->dev))
>  		cxled = to_cxl_endpoint_decoder(&cxld->dev);
>  
> +	if (!hdm) {
> +		if (cxled)
> +			return init_emulated_hdm_decoder(port, cxld, info, which);
> +		else
> +			return -EINVAL;

		if (!cxled)
			return -EINVAL;

		return init_emulated_hdm_decoder(....);

I'd prefer puting the error case out of line and normal one in main flow
as much as possible.

> +	}
> +
>  	ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(which));
>  	base = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_BASE_LOW_OFFSET(which));
>  	size = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(which));
> @@ -832,19 +862,15 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>  	return 0;
>  }
>  

> +
> +/**
> + * devm_cxl_enumerate_decoders - add decoder objects per HDM register set
> + * @cxlhdm: Structure to populate with HDM capabilities

Docs update got missed somewhere.

> + */
> +int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
> +				struct cxl_endpoint_dvsec_info *info)
> +{
> +	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
> +	struct cxl_port *port = cxlhdm->port;
> +	int i;
> +	u64 dpa_base = 0;
> +
> +	cxl_settle_decoders(cxlhdm);
>  
>  	for (i = 0; i < cxlhdm->decoder_count; i++) {
>  		int target_map[CXL_DECODER_MAX_INTERLEAVE] = { 0 };
> 
> 

[RFC PATCH 7/8] cxl: suppress component register discovery failure warning for RCD

[Dave Jiang]

For an RCD, it is expected that component register won't be discovered by
cxl_pci. Suppress warning if RCD.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/cxl/pci.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 73ff6c33a0c0..d808da838909 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -486,7 +486,7 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	 */
 	cxlds->component_reg_phys = CXL_RESOURCE_NONE;
 	rc = cxl_setup_regs(pdev, CXL_REGLOC_RBI_COMPONENT, &map);
-	if (rc)
+	if (rc && !cxlds->rcd)
 		dev_warn(&pdev->dev, "No component registers (%d)\n", rc);
 
 	cxlds->component_reg_phys = cxl_regmap_to_base(pdev, &map);

Re: [RFC PATCH 7/8] cxl: suppress component register discovery failure warning for RCD

[Jonathan Cameron]

On Wed, 30 Nov 2022 16:12:55 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> For an RCD, it is expected that component register won't be discovered by
> cxl_pci. Suppress warning if RCD.

I've lost track a bit on where this ended up in the RCD series
but from a spec point of view 8.2.2 in CXL 3.0 makes it clear
that it's fine for component registers to be in a BAR and hence
the register block locator.  Hence does the "expected" want relaxing
to an "allowed" or even "likely".  Expected kind of implies the
opposite isn't expected.

> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/cxl/pci.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 73ff6c33a0c0..d808da838909 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -486,7 +486,7 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	 */
>  	cxlds->component_reg_phys = CXL_RESOURCE_NONE;
>  	rc = cxl_setup_regs(pdev, CXL_REGLOC_RBI_COMPONENT, &map);
> -	if (rc)
> +	if (rc && !cxlds->rcd)
>  		dev_warn(&pdev->dev, "No component registers (%d)\n", rc);
>  
>  	cxlds->component_reg_phys = cxl_regmap_to_base(pdev, &map);
> 
> 

[RFC PATCH 8/8] cxl: remove locked check for dvsec_range_allowed()

[Dave Jiang]

There is no reason that the CFMWS will always set the "Fixed Device
Configuration" bit in the "Window Restrictions" field. Remove the
CXL_DECODER_F_LOCK check.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/cxl/core/pci.c |    2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 009c11b43303..d92fb757e495 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -229,8 +229,6 @@ static int dvsec_range_allowed(struct device *dev, void *arg)
 
 	cxld = to_cxl_decoder(dev);
 
-	if (!(cxld->flags & CXL_DECODER_F_LOCK))
-		return 0;
 	if (!(cxld->flags & CXL_DECODER_F_RAM))
 		return 0;
 

Re: [RFC PATCH 0/8] cxl: Introduce HDM decoder emulation from DVSEC range registers

[Jonathan Cameron]

On Wed, 30 Nov 2022 16:12:15 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> This series provides the emulation of HDM decoders from the programmed range
> registers. From CXL 3.0 spec 8.1.3.8, there can be up to 2 ranges programmed.
> Some device may not implement HDM decoder registers and some may not be
> programmed by the BIOS. Under such scenarios, if one of more range registers
> are programmed, then we can create an emulated HDM decoder per active range
> indicated by the range registers. The emulated HDM decoders will show up as
> locked and cannot be reprogrammed.
> 
> Below is a table that indicates different scenarios the driver may encounter:
> 
> rr: Range registers not programmed
> hdm: HDM decoders not programmed
> RR: Range registers programmed by BIOS
> HDM: HDM decoders programmed by BIOS
> 
> emulate HDM: Create HDM decoder software structs and use values from range registers.
> keep HDM: Populate HDM decoder software structs with values in HDM decoder registers.
> 
> rr             RR             rr hdm	 rr HDM	     RR hdm        RR HDM
> unsupported    emulate HDM    keep HDM	 keep HDM    emulate HDM   keep HDM
> 
> This series is based on the current RCD work [1] that's going through upstream review.
> For convenience, the kernel branch can be retrieved here [2].

Just to check, why is this an RFC rather than a normal submission?
Because of the unstable dependency?

At least at top level, it looks like a sensible approach.

> 
> [1]: https://lore.kernel.org/linux-cxl/Y4ePZD776yXv2rG3@rric.localdomain/T/#t
> [2]: https://git.kernel.org/pub/scm/linux/kernel/git/djiang/linux.git/log/?h=cxl-rch
> 
> ---
> 
> Dave Jiang (8):
>       cxl: break out range register decoding from cxl_hdm_decode_init()
>       cxl: export cxl_dvsec_rr_decode() to cxl_port
>       cxl: refactor cxl_hdm_decode_init()
>       cxl: emulate HDM decoder from DVSEC range registers
>       cxl: create emulated cxl_hdm for devices that do not have HDM decoders
>       cxl: create emulated decoders for devices without HDM decoders
>       cxl: suppress component register discovery failure warning for RCD
>       cxl: remove locked check for dvsec_range_allowed()
> 
> 
>  drivers/cxl/core/hdm.c | 115 +++++++++++++++++++++++---
>  drivers/cxl/core/pci.c | 179 +++++++++++++++++------------------------
>  drivers/cxl/cxl.h      |  20 ++++-
>  drivers/cxl/cxlmem.h   |  13 +--
>  drivers/cxl/cxlpci.h   |   3 +-
>  drivers/cxl/pci.c      |   2 +-
>  drivers/cxl/port.c     |  19 +++--
>  7 files changed, 215 insertions(+), 136 deletions(-)
> 
> --
> 

Re: [RFC PATCH 0/8] cxl: Introduce HDM decoder emulation from DVSEC range registers

[Dave Jiang]

On 12/19/22 09:12, Jonathan Cameron wrote:
> On Wed, 30 Nov 2022 16:12:15 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> This series provides the emulation of HDM decoders from the programmed range
>> registers. From CXL 3.0 spec 8.1.3.8, there can be up to 2 ranges programmed.
>> Some device may not implement HDM decoder registers and some may not be
>> programmed by the BIOS. Under such scenarios, if one of more range registers
>> are programmed, then we can create an emulated HDM decoder per active range
>> indicated by the range registers. The emulated HDM decoders will show up as
>> locked and cannot be reprogrammed.
>>
>> Below is a table that indicates different scenarios the driver may encounter:
>>
>> rr: Range registers not programmed
>> hdm: HDM decoders not programmed
>> RR: Range registers programmed by BIOS
>> HDM: HDM decoders programmed by BIOS
>>
>> emulate HDM: Create HDM decoder software structs and use values from range registers.
>> keep HDM: Populate HDM decoder software structs with values in HDM decoder registers.
>>
>> rr             RR             rr hdm	 rr HDM	     RR hdm        RR HDM
>> unsupported    emulate HDM    keep HDM	 keep HDM    emulate HDM   keep HDM
>>
>> This series is based on the current RCD work [1] that's going through upstream review.
>> For convenience, the kernel branch can be retrieved here [2].
> 
> Just to check, why is this an RFC rather than a normal submission?
> Because of the unstable dependency?

That and also just making sure people are ok with the direction of the 
series. I can remove the RFC for the next rev.

> 
> At least at top level, it looks like a sensible approach.
> 
>>
>> [1]: https://lore.kernel.org/linux-cxl/Y4ePZD776yXv2rG3@rric.localdomain/T/#t
>> [2]: https://git.kernel.org/pub/scm/linux/kernel/git/djiang/linux.git/log/?h=cxl-rch
>>
>> ---
>>
>> Dave Jiang (8):
>>        cxl: break out range register decoding from cxl_hdm_decode_init()
>>        cxl: export cxl_dvsec_rr_decode() to cxl_port
>>        cxl: refactor cxl_hdm_decode_init()
>>        cxl: emulate HDM decoder from DVSEC range registers
>>        cxl: create emulated cxl_hdm for devices that do not have HDM decoders
>>        cxl: create emulated decoders for devices without HDM decoders
>>        cxl: suppress component register discovery failure warning for RCD
>>        cxl: remove locked check for dvsec_range_allowed()
>>
>>
>>   drivers/cxl/core/hdm.c | 115 +++++++++++++++++++++++---
>>   drivers/cxl/core/pci.c | 179 +++++++++++++++++------------------------
>>   drivers/cxl/cxl.h      |  20 ++++-
>>   drivers/cxl/cxlmem.h   |  13 +--
>>   drivers/cxl/cxlpci.h   |   3 +-
>>   drivers/cxl/pci.c      |   2 +-
>>   drivers/cxl/port.c     |  19 +++--
>>   7 files changed, 215 insertions(+), 136 deletions(-)
>>
>> --
>>
>