Re: [PATCH] cxl/port: Enable the HDM decoder capability for switch ports

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

On Wed, 17 May 2023 20:19:43 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> Derick noticed, when testing hot plug, that hot-add behaves nominally
> after a removal. However, if the hot-add is done without a prior
> removal, CXL.mem accesses fail. It turns out that the original
> implementation of the port driver and region programming wrongly assumed
> that platform-firmware always enables the host-bridge HDM decoder
> capability. Add support turning on switch-level HDM decoders in the case
> where platform-firmware has not.
> 
> The implementation is careful to only arrange for the enable to be
> undone if the current instance of the driver was the one that did the
> enable. This is to interoperate with platform-firmware that may expect
> CXL.mem to remain active after the driver is shutdown. This comes at the
> cost of potentially not shutting down the enable on kexec flows, but it
> is mitigated by the fact that the related HDM decoders still need to be
> enabled on an individual basis.
> 
> Cc: <stable@vger.kernel.org>
> Reported-by: Derick Marks <derick.w.marks@intel.com>
> Fixes: 54cdbf845cf7 ("cxl/port: Add a driver for 'struct cxl_port' objects")
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

I doubt anyone will be surprised to hear that the QEMU emulation wasn't
checking this bit when doing the topology walk.
Oops - I'll put together a fix.

I'm guessing this is already queued or upstream but if not
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>


Jonathan

> ---
>  drivers/cxl/core/pci.c        |   27 +++++++++++++++++++++++----
>  drivers/cxl/cxl.h             |    1 +
>  drivers/cxl/port.c            |   14 +++++++++-----
>  tools/testing/cxl/Kbuild      |    1 +
>  tools/testing/cxl/test/mock.c |   15 +++++++++++++++
>  5 files changed, 49 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index f332fe7af92b..c35c002b65dd 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -241,17 +241,36 @@ static void disable_hdm(void *_cxlhdm)
>  	       hdm + CXL_HDM_DECODER_CTRL_OFFSET);
>  }
>  
> -static int devm_cxl_enable_hdm(struct device *host, struct cxl_hdm *cxlhdm)
> +int devm_cxl_enable_hdm(struct cxl_port *port, struct cxl_hdm *cxlhdm)
>  {
> -	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
> +	void __iomem *hdm;
>  	u32 global_ctrl;
>  
> +	/*
> +	 * If the hdm capability was not mapped there is nothing to enable and
> +	 * the caller is responsible for what happens next.  For example,
> +	 * emulate a passthrough decoder.
> +	 */
> +	if (IS_ERR(cxlhdm))
> +		return 0;
> +
> +	hdm = cxlhdm->regs.hdm_decoder;
>  	global_ctrl = readl(hdm + CXL_HDM_DECODER_CTRL_OFFSET);
> +
> +	/*
> +	 * If the HDM decoder capability was enabled on entry, skip
> +	 * registering disable_hdm() since this decode capability may be
> +	 * owned by platform firmware.
> +	 */
> +	if (global_ctrl & CXL_HDM_DECODER_ENABLE)
> +		return 0;
> +
>  	writel(global_ctrl | CXL_HDM_DECODER_ENABLE,
>  	       hdm + CXL_HDM_DECODER_CTRL_OFFSET);
>  
> -	return devm_add_action_or_reset(host, disable_hdm, cxlhdm);
> +	return devm_add_action_or_reset(&port->dev, disable_hdm, cxlhdm);
>  }
> +EXPORT_SYMBOL_NS_GPL(devm_cxl_enable_hdm, CXL);
>  
>  int cxl_dvsec_rr_decode(struct device *dev, int d,
>  			struct cxl_endpoint_dvsec_info *info)
> @@ -425,7 +444,7 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
>  	if (info->mem_enabled)
>  		return 0;
>  
> -	rc = devm_cxl_enable_hdm(&port->dev, cxlhdm);
> +	rc = devm_cxl_enable_hdm(port, cxlhdm);
>  	if (rc)
>  		return rc;
>  
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 044a92d9813e..f93a28538962 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -710,6 +710,7 @@ struct cxl_endpoint_dvsec_info {
>  struct cxl_hdm;
>  struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port,
>  				   struct cxl_endpoint_dvsec_info *info);
> +int devm_cxl_enable_hdm(struct cxl_port *port, 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);
> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
> index eb57324c4ad4..17a95f469c26 100644
> --- a/drivers/cxl/port.c
> +++ b/drivers/cxl/port.c
> @@ -60,13 +60,17 @@ static int discover_region(struct device *dev, void *root)
>  static int cxl_switch_port_probe(struct cxl_port *port)
>  {
>  	struct cxl_hdm *cxlhdm;
> -	int rc;
> +	int rc, nr_dports;
>  
> -	rc = devm_cxl_port_enumerate_dports(port);
> -	if (rc < 0)
> -		return rc;
> +	nr_dports = devm_cxl_port_enumerate_dports(port);
> +	if (nr_dports < 0)
> +		return nr_dports;
>  
>  	cxlhdm = devm_cxl_setup_hdm(port, NULL);
> +	rc = devm_cxl_enable_hdm(port, cxlhdm);
> +	if (rc)
> +		return rc;
> +
>  	if (!IS_ERR(cxlhdm))
>  		return devm_cxl_enumerate_decoders(cxlhdm, NULL);
>  
> @@ -75,7 +79,7 @@ static int cxl_switch_port_probe(struct cxl_port *port)
>  		return PTR_ERR(cxlhdm);
>  	}
>  
> -	if (rc == 1) {
> +	if (nr_dports == 1) {
>  		dev_dbg(&port->dev, "Fallback to passthrough decoder\n");
>  		return devm_cxl_add_passthrough_decoder(port);
>  	}