Re: [PATCH v2 2/2] cxl/core: Skip non-CXL capabilities in MCAP

On 8/17/26 12:58 AM, penn wrote:
> From: Penn <[email protected]>
> 
> When MMPT is enabled, a CXL device may expose CXL-defined capabilities
> and PCIe Management Message Passthrough (MMPT) capabilities in the same
> MMIO Capabilities Register Block (MCAP).
> 
> The CXL capability parser currently identifies entries using only the
> capability ID. Since capability IDs are scoped by Vendor ID, a
> PCI-SIG-defined capability may have the same ID as a CXL-defined
> capability. This causes non-CXL entries, including MMPT and MMIO Mailbox,
> to be interpreted as CXL register blocks.
> 
> The MMPT register block may be interpreted as a CXL mailbox. This
> causes mailbox initialization to fail with:
> 
>     cxl_pci 0000:3b:00.0: Mailbox is too small (64b)
> 
> Check the MCAP Vendor ID before interpreting an entry as a CXL-defined
> capability. Skip capabilities with a non-zero Vendor ID other than
> PCI_VENDOR_ID_CXL.
> 
> In legacy CXL capability headers, the field now used for the MCAP
> Vendor ID was reserved and reads as zero. Continue to accept zero to
> preserve compatibility with those devices.
> 
> The fix has been tested on CXL 1.1 and CXL 3.0 devices. On the
> MMPT-enabled device, it prevents both Mailbox and Status capabilities
> from being misidentified.
> 
> Fixes: 8adaf747c9f0 ("cxl/mem: Find device capabilities")
> Suggested-by: Johnny <[email protected]>
> Signed-off-by: Penn <[email protected]>

After addressing Lukas's comment,
Reviewed-by: Dave Jiang <[email protected]>

Given that you are utilizing the new PCIe MMIO MBOX feature, have you considered migrate the CXL MBOX block parsing code to a PCI lib and shared between PCI and CXL? I did attempted something [1] like that a while back but never upstreamed the code due to no hardware to test on.
 
[1]: https://git.kernel.org/pub/scm/linux/kernel/git/djiang/linux.git/log/?h=pci-mbox

DJ

> ---
> Changes in v2:
> - Accept a zero Vendor ID for compatibility with legacy CXL capability
>   headers.
> - Move the u16 declaration above the u32 declaration to follow the
>   reverse Christmas tree convention.
> - Document testing on CXL 1.1 and CXL 3.0 devices.
> 
>  drivers/cxl/core/regs.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
> index 93710cf4f0a6..c7c14089f2c6 100644
> --- a/drivers/cxl/core/regs.c
> +++ b/drivers/cxl/core/regs.c
> @@ -133,8 +133,18 @@ void cxl_probe_device_regs(struct device *dev, void __iomem *base,
>  
>  	for (cap = 1; cap <= cap_count; cap++) {
>  		struct cxl_reg_map *rmap;
> +		u16 cap_id, vendor_id;
>  		u32 offset, length;
> -		u16 cap_id;
> +
> +		vendor_id = FIELD_GET(PCI_MCAP_HDR_VENDOR_ID,
> +				      readl(base + PCI_MCAP_HDR_BASE(cap) +
> +					    PCI_MCAP_HDR_REG_4));
> +		/*
> +		 * The Vendor ID field is reserved and reads as zero in legacy
> +		 * CXL capability headers. See CXL r3.2, Table 8-44.
> +		 */
> +		if (vendor_id && vendor_id != PCI_VENDOR_ID_CXL)
> +			continue;
>  
>  		cap_id = FIELD_GET(CXLDEV_CAP_HDR_CAP_ID_MASK,
>  				   readl(base + cap * 0x10));