Re: [PATCH v16 11/18] PCI/CXL: Cache endpoint HDM state during PCI enumeration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] PCI/CXL: out-of-bounds MMIO read in cxl_pci_hdm_map_regs() before bounds check
- [High] PCI/CXL: pci_cxl_hdm_cache_init() runs before BAR assignment and bridge enable
- [Medium] cxl/pci: unchecked PCI_POSSIBLE_ERROR() in cxl_set_mem_enable() corrupts cached dvsec_ctrl
- [Medium] cxl/pci: disable_hdm() leaves stale CXL_HDM_DECODER_ENABLE in cached global_ctrl
--

commit a7ebc7643586c3c5e8b6adb597a5c7e1384802d3
Author: Srirangan Madhavan <smadhavan@nvidia.com>

PCI/CXL: Cache endpoint HDM state during PCI enumeration

This commit caches endpoint HDM decoder and Device DVSEC state in
pci_dev->hdm during PCI enumeration so that CXL reset paths can restore
hardware state without a bound CXL driver. It also hooks DVSEC control
updates in the CXL core to keep the cached DVSEC control value synchronized.

> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index e2034a2179b48..44fa9d5d1a052 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -209,22 +209,24 @@ static int cxl_set_mem_enable(struct cxl_dev_state *cxlds, u16 val)
>  	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
>  	int d = cxlds->cxl_dvsec;
>  	u16 ctrl;
> -	int rc;
> +	int rc, ret = 1;
>  
>  	rc = pci_read_config_word(pdev, d + PCI_DVSEC_CXL_CTRL, &ctrl);
>  	if (rc)
>  		return pcibios_err_to_errno(rc);
>  
> -	if ((ctrl & PCI_DVSEC_CXL_MEM_ENABLE) == val)
> -		return 1;
> -	ctrl &= ~PCI_DVSEC_CXL_MEM_ENABLE;
> -	ctrl |= val;
> +	if ((ctrl & PCI_DVSEC_CXL_MEM_ENABLE) != val) {
> +		ctrl &= ~PCI_DVSEC_CXL_MEM_ENABLE;
> +		ctrl |= val;
>  
> -	rc = pci_write_config_word(pdev, d + PCI_DVSEC_CXL_CTRL, ctrl);
> -	if (rc)
> -		return pcibios_err_to_errno(rc);
> +		rc = pci_write_config_word(pdev, d + PCI_DVSEC_CXL_CTRL, ctrl);
> +		if (rc)
> +			return pcibios_err_to_errno(rc);
> +		ret = 0;
> +	}
>  
> -	return 0;
> +	cxl_hdm_cache_set_dvsec_ctrl(pdev, ctrl);

[Severity: Medium]
What happens in cxl_set_mem_enable() if the device is undergoing a reset or
the link is down when pci_read_config_word() reads PCI_DVSEC_CXL_CTRL?

On root complexes where config reads to an unresponsive device return
PCIBIOS_SUCCESSFUL with ctrl == 0xffff, cxl_set_mem_enable() does not check
PCI_POSSIBLE_ERROR(ctrl). When called from clear_mem_enable() during endpoint
port teardown (which is not serialized by pci_dev_lock(pdev)) or from
devm_cxl_enable_mem(), ctrl (0xffff) is masked to 0xfffb (or kept as 0xffff),
written back to PCI_DVSEC_CXL_CTRL (including RW1C Viral_Status,
Viral_Enable, Cache_Enable, and reserved bits), and then stored into
pdev->hdm->dvsec_ctrl via cxl_hdm_cache_set_dvsec_ctrl().

Unlike cxl_hdm_cache_refresh_decoder(), which checks global_ctrl != U32_MAX
to preserve the last good value when the device does not respond,
cxl_hdm_cache_set_dvsec_ctrl() unconditionally overwrites
pdev->hdm->dvsec_ctrl, permanently clobbering the cached DVSEC control state
for subsequent resets. Should cxl_set_mem_enable() check
PCI_POSSIBLE_ERROR(ctrl) before modifying and caching ctrl?

[Severity: Medium]
Also, while cxl_set_mem_enable() and clear_mem_enable() keep
pdev->hdm->dvsec_ctrl updated on both enable and teardown, does disable_hdm()
leave a stale CXL_HDM_DECODER_ENABLE bit in pdev->hdm->global_ctrl?

When the CXL driver binds and sets up endpoint decoders:

  devm_cxl_endpoint_decoders_setup()
    cxl_hdm_decode_init()
      devm_cxl_enable_hdm()
      devm_cxl_enable_mem()
        cxl_set_mem_enable()
          cxl_hdm_cache_set_dvsec_ctrl()
    devm_cxl_enumerate_decoders()
      cxl_hdm_refresh_decoder()
        cxl_hdm_cache_refresh_decoder()
          info->global_ctrl = readl(hdm + CXL_HDM_DECODER_CTRL_OFFSET);

cxl_hdm_cache_refresh_decoder() latches CXL_HDM_DECODER_ENABLE into
pdev->hdm->global_ctrl. When the endpoint port driver later unbinds (or fails
probe after refreshing a decoder), devres runs clear_mem_enable(), which
clears PCI_DVSEC_CXL_MEM_ENABLE in pdev->hdm->dvsec_ctrl, and then runs
disable_hdm():

drivers/cxl/core/pci.c:disable_hdm() {
	global_ctrl = readl(hdm + CXL_HDM_DECODER_CTRL_OFFSET);
	writel(global_ctrl & ~CXL_HDM_DECODER_ENABLE,
	       hdm + CXL_HDM_DECODER_CTRL_OFFSET);
}

Because disable_hdm() does not update pdev->hdm->global_ctrl, a subsequent
reset while unbound will restore CXL_HDM_DECODER_ENABLE in hardware while
PCI_DVSEC_CXL_MEM_ENABLE is disabled, and a later CXL driver re-bind in
cxl_hdm_decode_init() will see CXL_HDM_DECODER_ENABLE already set and skip
devm_cxl_enable_hdm(). Should disable_hdm() (and devm_cxl_enable_hdm()) also
update pdev->hdm->global_ctrl?

[ ... ]

> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> index 655ed53436d3e..aad567fb7c560 100644
> --- a/drivers/pci/bus.c
> +++ b/drivers/pci/bus.c
> @@ -359,6 +359,7 @@ void pci_bus_add_device(struct pci_dev *dev)
>  
>  	/* Save config space for error recoverability */
>  	pci_save_state(dev);
> +	pci_cxl_hdm_cache_init(dev);

[Severity: High]
Can pci_cxl_hdm_cache_init() run here before BAR resources are validated and
assigned, or while upstream bridges still have memory decoding disabled?

On x86 ACPI boot, acpi_init() (subsys_initcall) enumerates the root bus and
calls pci_bus_add_devices() -> pci_bus_add_device() before pci_subsys_init()
runs pcibios_resource_survey() and before pcibios_assign_resources()
(fs_initcall) assigns unassigned resources:

  acpi_init()
    acpi_pci_root_add()
      pci_bus_add_devices()
        pci_bus_add_device()
          pci_cxl_hdm_cache_init()

In drivers/pci/cxl.c, cxl_pci_bar_assigned() checks res->start and res->end
without checking res->parent:

drivers/pci/cxl.c:cxl_pci_bar_assigned() {
	struct resource *res = &pdev->resource[bar];

	return pci_resource_len(pdev, bar) &&
	       !(res->flags & (IORESOURCE_UNSET | IORESOURCE_DISABLED)) &&
	       resource_type(res) == IORESOURCE_MEM && res->start && res->end;
}

And cxl_pci_hdm_read_info() temporarily sets PCI_COMMAND_MEMORY directly on
pdev without enabling upstream bridges:

drivers/pci/cxl.c:cxl_pci_hdm_read_info() {
	if (!(command & PCI_COMMAND_MEMORY)) {
		rc = pci_write_config_word(pdev, PCI_COMMAND,
					   command | PCI_COMMAND_MEMORY);
		...
	}
}

This appears to have two consequences:
1. If a disabled device has an unclaimed BIOS BAR address that conflicts with
   another resource, enabling PCI_COMMAND_MEMORY before
   pcibios_resource_survey() claims and validates resources can activate the
   conflicting BAR decoding.
2. If the endpoint BAR or an upstream bridge window is assigned later during
   pcibios_assign_resources(), or if an upstream PCI-to-PCI bridge does not
   have PCI_COMMAND_MEMORY enabled yet, pci_cxl_hdm_cache_init() fails and
   leaves pdev->hdm permanently NULL.

> diff --git a/drivers/pci/cxl.c b/drivers/pci/cxl.c
> new file mode 100644
> index 0000000000000..b992f71d964a8
> --- /dev/null
> +++ b/drivers/pci/cxl.c
[ ... ]
> +static void __iomem *cxl_pci_hdm_map_regs(struct pci_dev *pdev,
> +					  struct cxl_register_map *map,
> +					  resource_size_t *hdm_offset,
> +					  int *decoder_count)
> +{
> +	struct cxl_reg_map *hdm_map = &map->component_map.hdm_decoder;
> +	void __iomem *hdm;
> +	int rc;
> +
> +	/* cxl_setup_regs() populates hdm_map, including its valid bit. */
> +	rc = cxl_setup_regs(map);
> +	if (rc)
> +		return ERR_PTR(rc);
> +	if (!hdm_map->valid)
> +		return ERR_PTR(-ENODEV);
> +	if (hdm_map->offset > map->max_size ||
> +	    hdm_map->size > map->max_size - hdm_map->offset)
> +		return ERR_PTR(-ENXIO);

[Severity: High]
Does cxl_setup_regs(map) perform MMIO reads past map->max_size before the
bounds check in cxl_pci_hdm_map_regs() is reached?

In __pci_cxl_hdm_cache_init(), cxl_find_regblock() populates map via
cxl_decode_regblock(), which only checks offset > pci_resource_len(pdev, bar)
and sets map->max_size = pci_resource_len(pdev, bar) - offset.

When cxl_pci_hdm_map_regs() calls cxl_setup_regs(map):

  cxl_pci_hdm_map_regs()
    cxl_setup_regs()
      cxl_map_regblock()
        map->base = ioremap(map->resource, map->max_size);
      cxl_probe_regs()
        cxl_probe_component_regs()

cxl_probe_component_regs() unconditionally adds CXL_CM_OFFSET (0x1000) to
map->base, reads CXL_CM_CAP_HDR_OFFSET, and then reads each capability header
at base + offset (which is map->base + hdm_map->offset) without checking
map->max_size.

If a device's remaining BAR length (map->max_size) is smaller than
CXL_CM_OFFSET + sizeof(u32) or smaller than hdm_map->offset + sizeof(u32),
won't cxl_probe_component_regs() fault on the unmapped ioremap guard page
before cxl_pci_hdm_map_regs() checks hdm_map->offset against map->max_size?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029145703.2185231-1-smadhavan@nvidia.com?part=11