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

[engguopeng]

> On 8/17/26 12:58 AM, penn wrote:
> > From: Penn <engguopeng@xxxxxxxxxxx>
> > 
> > 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 <johnny.li@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Penn <engguopeng@xxxxxxxxxxx>
> 
> After addressing Lukas's comment,
> Reviewed-by: Dave Jiang <dave.jiang@xxxxxxxxx>
> 
> 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

Thanks for the review. Lukas's comment has already been addressed in v2,
but I missed his Reviewed-by tag. When applying, please also include:

Reviewed-by: Lukas Wunner <lukas@xxxxxxxxx>

I will add it if another revision is needed.

In addition, I will look into moving the CXL mailbox block parsing code to a common
PCI library so that it can be shared with the PCIe MMIO mailbox support.

Since that would be a broader refactoring, I would prefer to keep this
patch focused on fixing the Vendor ID validation issue and handle the
common parsing code in a separate follow-up patch or series. I will also
test the follow-up changes on real CXL hardware with MMPT support.

> > ---
> > 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));