Re: [PATCH v4 02/27] cxl/regs: Skip sub-block region request for BAR-owning drivers

[Dave Jiang <dave.jiang@intel.com>]

On 8/13/26 2:36 AM, mhonap@nvidia.com wrote:
> From: Manish Honap <mhonap@nvidia.com>
> 
> cxl_map_component_regs() claims each mapped sub-block with
> devm_request_mem_region(). A driver that owns the whole component
> register BAR, such as vfio-cxl, has already claimed the full BAR, so
> the per-sub-block claim collides and the mapping fails.
> 
> Add a bar_owned parameter to cxl_pci_setup_regs() and record it in
> skip_sub_bar_request on the register map. When set, cxl-core maps the
> sub-block without requesting the region and leaves ownership with the
> upper driver. cxl_pci passes false and keeps its existing claim,
> preserving /dev/mem tooling access to the rest of the component space.

So currently it's either cxl_pci driver that binds to the PCI device or vfio_cxl driver right? Why does cxl_pci needs to pass in false? When vfio_cxl driver is being attached, cxl_pci is not involved and all the bar ranges should be free? Or is there a contention between vfio_cxl and cxl_core that it calls on? 

DJ
> 
> Signed-off-by: Manish Honap <mhonap@nvidia.com>
> ---
>  drivers/cxl/core/pci.c  |  9 ++++++++-
>  drivers/cxl/core/regs.c | 32 ++++++++++++++++++++++----------
>  drivers/cxl/pci.c       |  4 ++--
>  include/cxl/cxl.h       |  3 +++
>  include/cxl/pci.h       |  2 +-
>  5 files changed, 36 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 9d807c1a002c..08d4c955137d 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -720,7 +720,7 @@ static int cxl_rcrb_get_comp_regs(struct pci_dev *pdev,
>  }
>  
>  int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
> -		       struct cxl_register_map *map)
> +		       struct cxl_register_map *map, bool bar_owned)
>  {
>  	int rc;
>  
> @@ -750,6 +750,13 @@ int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
>  		return rc;
>  	}
>  
> +	/*
> +	 * A caller that owns the whole register BAR (for example vfio-cxl)
> +	 * maps the sub-blocks without claiming them, so the later
> +	 * cxl_map_component_regs() does not collide with the full-BAR request.
> +	 */
> +	map->skip_sub_bar_request = bar_owned;
> +
>  	return cxl_setup_regs(map);
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_pci_setup_regs, "CXL");
> diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
> index 989e79383b99..90dd85f322f5 100644
> --- a/drivers/cxl/core/regs.c
> +++ b/drivers/cxl/core/regs.c
> @@ -177,35 +177,47 @@ void cxl_probe_device_regs(struct device *dev, void __iomem *base,
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_probe_device_regs, "CXL");
>  
> -void __iomem *devm_cxl_iomap_block(struct device *dev, resource_size_t addr,
> -				   resource_size_t length)
> +static void __iomem *__cxl_iomap_block(struct device *dev, resource_size_t addr,
> +				       resource_size_t length, bool request)
>  {
> +	resource_size_t end = addr + length - 1;
>  	void __iomem *ret_val;
>  	struct resource *res;
>  
>  	if (WARN_ON_ONCE(addr == CXL_RESOURCE_NONE))
>  		return NULL;
>  
> -	res = devm_request_mem_region(dev, addr, length, dev_name(dev));
> -	if (!res) {
> -		resource_size_t end = addr + length - 1;
> -
> -		dev_err(dev, "Failed to request region %pa-%pa\n", &addr, &end);
> -		return NULL;
> +	/*
> +	 * An owning driver (e.g. vfio-cxl) may already hold the whole BAR; a
> +	 * sub-block request would then collide with that claim.
> +	 */
> +	if (request) {
> +		res = devm_request_mem_region(dev, addr, length, dev_name(dev));
> +		if (!res) {
> +			dev_err(dev, "Failed to request region %pa-%pa\n", &addr, &end);
> +			return NULL;
> +		}
>  	}
>  
>  	ret_val = devm_ioremap(dev, addr, length);
>  	if (!ret_val)
> -		dev_err(dev, "Failed to map region %pr\n", res);
> +		dev_err(dev, "Failed to map region %pa-%pa\n", &addr, &end);
>  
>  	return ret_val;
>  }
> +
> +void __iomem *devm_cxl_iomap_block(struct device *dev, resource_size_t addr,
> +				   resource_size_t length)
> +{
> +	return __cxl_iomap_block(dev, addr, length, true);
> +}
>  EXPORT_SYMBOL_NS_GPL(devm_cxl_iomap_block, "CXL");
>  
>  int cxl_map_component_regs(const struct cxl_register_map *map,
>  			   struct cxl_component_regs *regs,
>  			   unsigned long map_mask)
>  {
> +	bool request = !map->skip_sub_bar_request;
>  	struct device *host = map->host;
>  	struct mapinfo {
>  		const struct cxl_reg_map *rmap;
> @@ -227,7 +239,7 @@ int cxl_map_component_regs(const struct cxl_register_map *map,
>  			continue;
>  		addr = map->resource + mi->rmap->offset;
>  		length = mi->rmap->size;
> -		*(mi->addr) = devm_cxl_iomap_block(host, addr, length);
> +		*(mi->addr) = __cxl_iomap_block(host, addr, length, request);
>  		if (!*(mi->addr))
>  			return -ENOMEM;
>  	}
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index bb892dbfdd6d..ec1693c21f61 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -816,7 +816,7 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  
>  	cxlds->rcd = is_cxl_restricted(pdev);
>  
> -	rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_MEMDEV, &map);
> +	rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_MEMDEV, &map, false);
>  	if (rc)
>  		return rc;
>  
> @@ -829,7 +829,7 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	 * still be useful for management functions so don't return an error.
>  	 */
>  	rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_COMPONENT,
> -				&cxlds->reg_map);
> +				&cxlds->reg_map, false);
>  	if (rc)
>  		dev_warn(&pdev->dev, "No component registers (%d)\n", rc);
>  	else if (!cxlds->reg_map.component_map.ras.valid)
> diff --git a/include/cxl/cxl.h b/include/cxl/cxl.h
> index efb8ac97bd92..f8e8fddba152 100644
> --- a/include/cxl/cxl.h
> +++ b/include/cxl/cxl.h
> @@ -213,6 +213,8 @@ struct cxl_pmu_reg_map {
>   * @resource: physical resource base of the register block
>   * @max_size: maximum mapping size to perform register search
>   * @reg_type: see enum cxl_regloc_type
> + * @skip_sub_bar_request: set via cxl_pci_setup_regs(); the owning driver holds
> + *			  the BAR, so skip the sub-block claim
>   * @component_map: cxl_reg_map for component registers
>   * @device_map: cxl_reg_maps for device registers
>   * @pmu_map: cxl_reg_maps for CXL Performance Monitoring Units
> @@ -223,6 +225,7 @@ struct cxl_register_map {
>  	resource_size_t resource;
>  	resource_size_t max_size;
>  	u8 reg_type;
> +	bool skip_sub_bar_request;
>  	union {
>  		struct cxl_component_reg_map component_map;
>  		struct cxl_device_reg_map device_map;
> diff --git a/include/cxl/pci.h b/include/cxl/pci.h
> index 3e0000015871..81a32004a604 100644
> --- a/include/cxl/pci.h
> +++ b/include/cxl/pci.h
> @@ -18,5 +18,5 @@ struct cxl_register_map;
>  struct pci_dev;
>  
>  int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
> -		       struct cxl_register_map *map);
> +		       struct cxl_register_map *map, bool bar_owned);
>  #endif