Re: [PATCH v16 12/22] sfc: obtain root decoder with enough HPA free space

[Dan Williams <dan.j.williams@intel.com>]

alejandro.lucero-palau@ wrote:
> From: Alejandro Lucero <alucerop@amd.com>
> 
> Asking for available HPA space is the previous step to try to obtain
> an HPA range suitable to accel driver purposes.
> 
> Add this call to efx cxl initialization.
> 
> Make sfc cxl build dependent on CXL region.
> 
> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> Reviewed-by: Martin Habets <habetsm.xilinx@gmail.com>
> Acked-by: Edward Cree <ecree.xilinx@gmail.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  drivers/net/ethernet/sfc/Kconfig   |  1 +
>  drivers/net/ethernet/sfc/efx_cxl.c | 19 +++++++++++++++++++
>  2 files changed, 20 insertions(+)
> 
> diff --git a/drivers/net/ethernet/sfc/Kconfig b/drivers/net/ethernet/sfc/Kconfig
> index 979f2801e2a8..e959d9b4f4ce 100644
> --- a/drivers/net/ethernet/sfc/Kconfig
> +++ b/drivers/net/ethernet/sfc/Kconfig
> @@ -69,6 +69,7 @@ config SFC_MCDI_LOGGING
>  config SFC_CXL
>  	bool "Solarflare SFC9100-family CXL support"
>  	depends on SFC && CXL_BUS >= SFC
> +	depends on CXL_REGION
>  	default SFC
>  	help
>  	  This enables SFC CXL support if the kernel is configuring CXL for
> diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c
> index 53ff97ad07f5..5635672b3fc3 100644
> --- a/drivers/net/ethernet/sfc/efx_cxl.c
> +++ b/drivers/net/ethernet/sfc/efx_cxl.c
> @@ -26,6 +26,7 @@ int efx_cxl_init(struct efx_probe_data *probe_data)
>  	struct cxl_dpa_info sfc_dpa_info = {
>  		.size = EFX_CTPIO_BUFFER_SIZE
>  	};
> +	resource_size_t max_size;
>  	struct efx_cxl *cxl;
>  	u16 dvsec;
>  	int rc;
> @@ -84,6 +85,22 @@ int efx_cxl_init(struct efx_probe_data *probe_data)
>  		return PTR_ERR(cxl->cxlmd);
>  	}
>  
> +	cxl->cxlrd = cxl_get_hpa_freespace(cxl->cxlmd, 1,
> +					   CXL_DECODER_F_RAM | CXL_DECODER_F_TYPE2,
> +					   &max_size);
> +
> +	if (IS_ERR(cxl->cxlrd)) {
> +		pci_err(pci_dev, "cxl_get_hpa_freespace failed\n");
> +		return PTR_ERR(cxl->cxlrd);
> +	}

This is a simple enough model, but it does mean that if async-driver
loading causes this driver to load before cxl_acpi or cxl_mem have
completed their init work, then it will die here.

It is also worth noting that nothing stops cxl_mem or cxl_acpi from
detaching immediately after passing the above check. So more work is
needed here (likely post-merge) to revoke and invalidate usage of that
freespace when that happens.

Otherwise you can do something like:

Driver1			Driver2			Notes
cxl_get_hpa_freespace()				"Driver1 gets rangeX"
	--- cxl_acpi unloaded ---		"forgets rangeX was assigned"	
	--- cxl_acpi reloaded ---			
			cxl_get_hpa_freespace() "Driver2 gets rangeX"
use_cxl(rangeX)		use_cxl(rangeX)		"...uh oh"

So longer term there needs to be notification back to the creator of the
memdev to require it to handle cleaning up when the CXL topology is torn
down either physically or logically.

To date the CXL subsystem has not reset decoders on unload because it
needs to handle coordinating with HDM decode established by platform
firmware. Type-2 driver however should be prepared to have their CXL
range revoked at any moment.

The Type-3 case handles this because cxl_mem is the driver itself, for
Type-2 that driver wants to coordinate with cxl_mem on these events. To
me that looks like cxl_mem error handler operation callbacks.