[PATCH] cxl/hdm: Limit emulation to the number of range registers

[PATCH] cxl/hdm: Limit emulation to the number of range registers

[Dan Williams]

Recall that range register emulation seeks to treat the 2 potential
range registers as Linux CXL "decoder" objects. The number of range
registers can be 1 or 2, while HDM decoder ranges can include more than
2.

Be careful not to confuse DVSEC range count with HDM capability decoder
count. Commit to range register earlier in devm_cxl_setup_hdm().
Otherwise, a device with more HDM decoders than range registers can set
@cxlhdm->decoder_count to an invalid value.

Avoid introducing a forward declaration by just moving the definition of
should_emulate_decoders() earlier in the file. should_emulate_decoders()
is unchanged.

Tested-by: Dave Jiang <dave.jiang@intel.com>
Fixes: d7a2153762c7 ("cxl/hdm: Add emulation when HDM decoders are not committed")
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/hdm.c |   82 +++++++++++++++++++++++++++---------------------
 1 file changed, 46 insertions(+), 36 deletions(-)

diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index cc123996b1a4..9884b6d4d930 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -101,6 +101,42 @@ static int map_hdm_decoder_regs(struct cxl_port *port, void __iomem *crb,
 				      BIT(CXL_CM_CAP_CAP_ID_HDM));
 }
 
+static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info)
+{
+	struct cxl_hdm *cxlhdm;
+	void __iomem *hdm;
+	u32 ctrl;
+	int i;
+
+	if (!info)
+		return false;
+
+	cxlhdm = dev_get_drvdata(&info->port->dev);
+	hdm = cxlhdm->regs.hdm_decoder;
+
+	if (!hdm)
+		return true;
+
+	/*
+	 * If HDM decoders are present and the driver is in control of
+	 * Mem_Enable skip DVSEC based emulation
+	 */
+	if (!info->mem_enabled)
+		return false;
+
+	/*
+	 * If any decoders are committed already, there should not be any
+	 * emulated DVSEC decoders.
+	 */
+	for (i = 0; i < cxlhdm->decoder_count; i++) {
+		ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(i));
+		if (FIELD_GET(CXL_HDM_DECODER0_CTRL_COMMITTED, ctrl))
+			return false;
+	}
+
+	return true;
+}
+
 /**
  * devm_cxl_setup_hdm - map HDM decoder component registers
  * @port: cxl_port to map
@@ -140,6 +176,16 @@ struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port,
 		return ERR_PTR(-ENXIO);
 	}
 
+	/*
+	 * Now that the hdm capability is parsed, decide if range
+	 * register emulation is needed and fixup cxlhdm accordingly.
+	 */
+	if (should_emulate_decoders(info)) {
+		dev_dbg(dev, "Fallback map %d range register%s\n", info->ranges,
+			info->ranges > 1 ? "s" : "");
+		cxlhdm->decoder_count = info->ranges;
+	}
+
 	return cxlhdm;
 }
 EXPORT_SYMBOL_NS_GPL(devm_cxl_setup_hdm, CXL);
@@ -717,42 +763,6 @@ static int cxl_setup_hdm_decoder_from_dvsec(struct cxl_port *port,
 	return 0;
 }
 
-static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info)
-{
-	struct cxl_hdm *cxlhdm;
-	void __iomem *hdm;
-	u32 ctrl;
-	int i;
-
-	if (!info)
-		return false;
-
-	cxlhdm = dev_get_drvdata(&info->port->dev);
-	hdm = cxlhdm->regs.hdm_decoder;
-
-	if (!hdm)
-		return true;
-
-	/*
-	 * If HDM decoders are present and the driver is in control of
-	 * Mem_Enable skip DVSEC based emulation
-	 */
-	if (!info->mem_enabled)
-		return false;
-
-	/*
-	 * If any decoders are committed already, there should not be any
-	 * emulated DVSEC decoders.
-	 */
-	for (i = 0; i < cxlhdm->decoder_count; i++) {
-		ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(i));
-		if (FIELD_GET(CXL_HDM_DECODER0_CTRL_COMMITTED, ctrl))
-			return false;
-	}
-
-	return true;
-}
-
 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)

Re: [PATCH] cxl/hdm: Limit emulation to the number of range registers

[Dave Jiang]

On 3/29/23 2:35 PM, Dan Williams wrote:
> Recall that range register emulation seeks to treat the 2 potential
> range registers as Linux CXL "decoder" objects. The number of range
> registers can be 1 or 2, while HDM decoder ranges can include more than
> 2.
> 
> Be careful not to confuse DVSEC range count with HDM capability decoder
> count. Commit to range register earlier in devm_cxl_setup_hdm().
> Otherwise, a device with more HDM decoders than range registers can set
> @cxlhdm->decoder_count to an invalid value.
> 
> Avoid introducing a forward declaration by just moving the definition of
> should_emulate_decoders() earlier in the file. should_emulate_decoders()
> is unchanged.
> 
> Tested-by: Dave Jiang <dave.jiang@intel.com>
> Fixes: d7a2153762c7 ("cxl/hdm: Add emulation when HDM decoders are not committed")
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

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

> ---
>   drivers/cxl/core/hdm.c |   82 +++++++++++++++++++++++++++---------------------
>   1 file changed, 46 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index cc123996b1a4..9884b6d4d930 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -101,6 +101,42 @@ static int map_hdm_decoder_regs(struct cxl_port *port, void __iomem *crb,
>   				      BIT(CXL_CM_CAP_CAP_ID_HDM));
>   }
>   
> +static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info)
> +{
> +	struct cxl_hdm *cxlhdm;
> +	void __iomem *hdm;
> +	u32 ctrl;
> +	int i;
> +
> +	if (!info)
> +		return false;
> +
> +	cxlhdm = dev_get_drvdata(&info->port->dev);
> +	hdm = cxlhdm->regs.hdm_decoder;
> +
> +	if (!hdm)
> +		return true;
> +
> +	/*
> +	 * If HDM decoders are present and the driver is in control of
> +	 * Mem_Enable skip DVSEC based emulation
> +	 */
> +	if (!info->mem_enabled)
> +		return false;
> +
> +	/*
> +	 * If any decoders are committed already, there should not be any
> +	 * emulated DVSEC decoders.
> +	 */
> +	for (i = 0; i < cxlhdm->decoder_count; i++) {
> +		ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(i));
> +		if (FIELD_GET(CXL_HDM_DECODER0_CTRL_COMMITTED, ctrl))
> +			return false;
> +	}
> +
> +	return true;
> +}
> +
>   /**
>    * devm_cxl_setup_hdm - map HDM decoder component registers
>    * @port: cxl_port to map
> @@ -140,6 +176,16 @@ struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port,
>   		return ERR_PTR(-ENXIO);
>   	}
>   
> +	/*
> +	 * Now that the hdm capability is parsed, decide if range
> +	 * register emulation is needed and fixup cxlhdm accordingly.
> +	 */
> +	if (should_emulate_decoders(info)) {
> +		dev_dbg(dev, "Fallback map %d range register%s\n", info->ranges,
> +			info->ranges > 1 ? "s" : "");
> +		cxlhdm->decoder_count = info->ranges;
> +	}
> +
>   	return cxlhdm;
>   }
>   EXPORT_SYMBOL_NS_GPL(devm_cxl_setup_hdm, CXL);
> @@ -717,42 +763,6 @@ static int cxl_setup_hdm_decoder_from_dvsec(struct cxl_port *port,
>   	return 0;
>   }
>   
> -static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info)
> -{
> -	struct cxl_hdm *cxlhdm;
> -	void __iomem *hdm;
> -	u32 ctrl;
> -	int i;
> -
> -	if (!info)
> -		return false;
> -
> -	cxlhdm = dev_get_drvdata(&info->port->dev);
> -	hdm = cxlhdm->regs.hdm_decoder;
> -
> -	if (!hdm)
> -		return true;
> -
> -	/*
> -	 * If HDM decoders are present and the driver is in control of
> -	 * Mem_Enable skip DVSEC based emulation
> -	 */
> -	if (!info->mem_enabled)
> -		return false;
> -
> -	/*
> -	 * If any decoders are committed already, there should not be any
> -	 * emulated DVSEC decoders.
> -	 */
> -	for (i = 0; i < cxlhdm->decoder_count; i++) {
> -		ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(i));
> -		if (FIELD_GET(CXL_HDM_DECODER0_CTRL_COMMITTED, ctrl))
> -			return false;
> -	}
> -
> -	return true;
> -}
> -
>   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)
> 

Re: [PATCH] cxl/hdm: Limit emulation to the number of range registers

[Jonathan Cameron]

On Wed, 29 Mar 2023 14:35:49 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> Recall that range register emulation seeks to treat the 2 potential
> range registers as Linux CXL "decoder" objects. The number of range
> registers can be 1 or 2, while HDM decoder ranges can include more than
> 2.
> 
> Be careful not to confuse DVSEC range count with HDM capability decoder
> count. Commit to range register earlier in devm_cxl_setup_hdm().
> Otherwise, a device with more HDM decoders than range registers can set
> @cxlhdm->decoder_count to an invalid value.
> 
> Avoid introducing a forward declaration by just moving the definition of
> should_emulate_decoders() earlier in the file. should_emulate_decoders()
> is unchanged.
> 
> Tested-by: Dave Jiang <dave.jiang@intel.com>
> Fixes: d7a2153762c7 ("cxl/hdm: Add emulation when HDM decoders are not committed")
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Seems reasonable.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/cxl/core/hdm.c |   82 +++++++++++++++++++++++++++---------------------
>  1 file changed, 46 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index cc123996b1a4..9884b6d4d930 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -101,6 +101,42 @@ static int map_hdm_decoder_regs(struct cxl_port *port, void __iomem *crb,
>  				      BIT(CXL_CM_CAP_CAP_ID_HDM));
>  }
>  
> +static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info)
> +{
> +	struct cxl_hdm *cxlhdm;
> +	void __iomem *hdm;
> +	u32 ctrl;
> +	int i;
> +
> +	if (!info)
> +		return false;
> +
> +	cxlhdm = dev_get_drvdata(&info->port->dev);
> +	hdm = cxlhdm->regs.hdm_decoder;
> +
> +	if (!hdm)
> +		return true;
> +
> +	/*
> +	 * If HDM decoders are present and the driver is in control of
> +	 * Mem_Enable skip DVSEC based emulation
> +	 */
> +	if (!info->mem_enabled)
> +		return false;
> +
> +	/*
> +	 * If any decoders are committed already, there should not be any
> +	 * emulated DVSEC decoders.
> +	 */
> +	for (i = 0; i < cxlhdm->decoder_count; i++) {
> +		ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(i));
> +		if (FIELD_GET(CXL_HDM_DECODER0_CTRL_COMMITTED, ctrl))
> +			return false;
> +	}
> +
> +	return true;
> +}
> +
>  /**
>   * devm_cxl_setup_hdm - map HDM decoder component registers
>   * @port: cxl_port to map
> @@ -140,6 +176,16 @@ struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port,
>  		return ERR_PTR(-ENXIO);
>  	}
>  
> +	/*
> +	 * Now that the hdm capability is parsed, decide if range
> +	 * register emulation is needed and fixup cxlhdm accordingly.
> +	 */
> +	if (should_emulate_decoders(info)) {
> +		dev_dbg(dev, "Fallback map %d range register%s\n", info->ranges,
> +			info->ranges > 1 ? "s" : "");
> +		cxlhdm->decoder_count = info->ranges;
> +	}
> +
>  	return cxlhdm;
>  }
>  EXPORT_SYMBOL_NS_GPL(devm_cxl_setup_hdm, CXL);
> @@ -717,42 +763,6 @@ static int cxl_setup_hdm_decoder_from_dvsec(struct cxl_port *port,
>  	return 0;
>  }
>  
> -static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info)
> -{
> -	struct cxl_hdm *cxlhdm;
> -	void __iomem *hdm;
> -	u32 ctrl;
> -	int i;
> -
> -	if (!info)
> -		return false;
> -
> -	cxlhdm = dev_get_drvdata(&info->port->dev);
> -	hdm = cxlhdm->regs.hdm_decoder;
> -
> -	if (!hdm)
> -		return true;
> -
> -	/*
> -	 * If HDM decoders are present and the driver is in control of
> -	 * Mem_Enable skip DVSEC based emulation
> -	 */
> -	if (!info->mem_enabled)
> -		return false;
> -
> -	/*
> -	 * If any decoders are committed already, there should not be any
> -	 * emulated DVSEC decoders.
> -	 */
> -	for (i = 0; i < cxlhdm->decoder_count; i++) {
> -		ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(i));
> -		if (FIELD_GET(CXL_HDM_DECODER0_CTRL_COMMITTED, ctrl))
> -			return false;
> -	}
> -
> -	return true;
> -}
> -
>  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)
>