Re: [PATCH v5 6/7] cxl/hdm: Add emulation when HDM decoders are not committed

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Tue, 14 Feb 2023 11:41:35 -0800
Dan Williams <dan.j.williams@intel.com> wrote:

> From: Dave Jiang <dave.jiang@intel.com>
> 
> For the case where DVSEC range register(s) are active and HDM decoders are
> not committed, use RR to provide emulation. A first pass is done to note
> whether any decoders are committed. If there are no committed endpoint
> decoders, then DVSEC ranges will be used for emulation.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

I'm confused a bit here.  I 'think' the aim should always be to use
HDM decoders unless they either aren't present or we know the
DVSEC range registers are in use.

The conditions in here are too broad + trip up current QEMU.
There is a 'bug' in QEMU as it programs the base registers in DVSEC
but it doesn't affect this flow (and shouldn't).

I'm not sure how you are detecting 'active' for the DVSEC range
registers as described in this patch description. As far as I can
tell there is no way to work that out other than if the top level
CXL.mem_enabled == true;  The various active bits in the range registers
refer to whether the memory behind them is ready to use, not anything
to do whether the range registers are setup correctly and 'turned on'.

If CXL.mem is not enabled, the memory is definitely not
in use. Equation 8-2

Memory_active AND CXL Mem_Enable=1

Now the snag is that CXL mem enabled has been set already in
cxl_hdm_decode_init() called from cxl_endpoint_port_probe()

So the hack I'm carrying is to stash if 'it was not enabled when
we would otherwise turn it on'.  If that condition is true and
there are HDM decoders, we should use them.  Sometimes it feels
like there is no right order possible for enabling all of a CXL
device!

Note the QEMU device here is heaving like any unconfigured freshly
reset type 3 device - or a hotplugged one.


From 5c3e6c5c5c6c37dd0e614273113daf1ff2487e53 Mon Sep 17 00:00:00 2001
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Date: Mon, 20 Feb 2023 11:24:34 +0000
Subject: [PATCH] hack

---
 drivers/cxl/core/hdm.c | 11 ++++++++---
 drivers/cxl/core/pci.c |  1 +
 drivers/cxl/cxl.h      |  2 ++
 3 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index 45deda18ed32..a2b209cf9856 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -739,7 +739,8 @@ static int cxl_setup_hdm_decoder_from_dvsec(struct cxl_port *port,
 	return 0;
 }
 
-static bool should_emulate_decoders(struct cxl_port *port)
+static bool should_emulate_decoders(struct cxl_port *port,
+				    struct cxl_endpoint_dvsec_info *info)
 {
 	struct cxl_hdm *cxlhdm = dev_get_drvdata(&port->dev);
 	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
@@ -752,6 +753,9 @@ static bool should_emulate_decoders(struct cxl_port *port)
 	if (!hdm)
 		return true;
 
+	if (info->was_not_enabled_at_probe) {
+		return false;
+	}
 	/*
 	 * If any decoders are committed already, there should not be any
 	 * emulated DVSEC decoders.
@@ -780,7 +784,7 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
 		unsigned char target_id[8];
 	} target_list;
 
-	if (should_emulate_decoders(port))
+	if (should_emulate_decoders(port, info))
 		return cxl_setup_hdm_decoder_from_dvsec(port, cxld, which, info);
 
 	if (is_endpoint_decoder(&cxld->dev))
@@ -806,7 +810,8 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
 		.end = base + size - 1,
 	};
 
-	if (cxled && !committed && range_len(&info->dvsec_range[which]))
+	if ((!info || !info->was_not_enabled_at_probe) && cxled && !committed &&
+	    range_len(&info->dvsec_range[which]))
 		return cxl_setup_hdm_decoder_from_dvsec(port, cxld, which, info);
 
 	/* decoders are enabled if committed */
diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 7328a2552411..a08c35216ad8 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -377,6 +377,7 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
 	if (hdm)
 		global_ctrl = readl(hdm + CXL_HDM_DECODER_CTRL_OFFSET);
 
+	info->was_not_enabled_at_probe = !info->mem_enabled;
 	/*
 	 * If the HDM Decoder Capability is already enabled then assume
 	 * that some other agent like platform firmware set it up.
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index d853a0238ad7..c32a9f9438e8 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -696,11 +696,13 @@ 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
+ * @was_not_enabled_at_probe: hack
  * @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;
+	bool was_not_enabled_at_probe;
 	int ranges;
 	struct range dvsec_range[2];
 };
-- 
2.37.2




> ---
>  drivers/cxl/core/hdm.c |   29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index a49543f22dca..39e02f28b6a6 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -730,6 +730,32 @@ static int cxl_setup_hdm_decoder_from_dvsec(struct cxl_port *port,
>  	return 0;
>  }
>  
> +static bool should_emulate_decoders(struct cxl_port *port)
> +{
> +	struct cxl_hdm *cxlhdm = dev_get_drvdata(&port->dev);
> +	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
> +	u32 ctrl;
> +	int i;
> +
> +	if (!is_cxl_endpoint(cxlhdm->port))
> +		return false;
> +
> +	if (!hdm)
> +		return true;
> +
> +	/*
> +	 * 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)
> @@ -745,6 +771,9 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>  		unsigned char target_id[8];
>  	} target_list;
>  
> +	if (should_emulate_decoders(port))
> +		return cxl_setup_hdm_decoder_from_dvsec(port, cxld, which, info);
> +
>  	if (is_endpoint_decoder(&cxld->dev))
>  		cxled = to_cxl_endpoint_decoder(&cxld->dev);
>  
>