Re: [PATCH v4 1/3] PCI: Allow ATS to be always on for CXL.cache capable devices

["Dan Williams (nvidia)" <djbw@kernel.org>]

Nicolin Chen wrote:
> On Thu, Apr 30, 2026 at 02:41:22PM -0700, Dan Williams (nvidia) wrote:
> > > +static bool pci_cxl_ats_always_on(struct pci_dev *pdev)
> > > +{
> > > +	int offset;
> > > +	u16 cap;
> > > +
> > > +	offset = pci_find_dvsec_capability(pdev, PCI_VENDOR_ID_CXL,
> > > +					   PCI_DVSEC_CXL_DEVICE);
> > > +	if (!offset)
> > > +		return false;
> > > +
> > > +	if (pci_read_config_word(pdev, offset + PCI_DVSEC_CXL_CAP, &cap))
> > > +		return false;
> > > +
> > > +	return cap & PCI_DVSEC_CXL_CACHE_CAPABLE;
> [...]
> > Apologies for coming to this late and forgive me if the following has
> > already been asked and answered. Why not check for actual CXL.cache
> > protocol on the wire being present?
> 
> Actually it would make the patch smaller. The thing is that this
> is_cxl property wasn't added when I started the series. So, it's
> not using it. :)
> 
> > @@ -1733,9 +1733,8 @@ static void set_pcie_cxl(struct pci_dev *dev)
> >  	pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_FLEXBUS_PORT_STATUS,
> >  			     &cap);
> >  
> > -	dev->is_cxl = FIELD_GET(PCI_DVSEC_CXL_FLEXBUS_PORT_STATUS_CACHE, cap) ||
> > -		FIELD_GET(PCI_DVSEC_CXL_FLEXBUS_PORT_STATUS_MEM, cap);
> > -
> > +	dev->is_cxl_cache = FIELD_GET(PCI_DVSEC_CXL_FLEXBUS_PORT_STATUS_CACHE, cap);
> > +	dev->is_cxl_mem = FIELD_GET(PCI_DVSEC_CXL_FLEXBUS_PORT_STATUS_MEM, cap);
> 
> One caveat is that:
> 
> Here it checks the cap from:
> 	PCI_DVSEC_CXL_FLEXBUS_PORT_STATUS (0xE) via
> 		PCI_DVSEC_CXL_FLEXBUS_PORT_STATUS (0x7)
> 
> On the other hand, mine checks from:
> 	PCI_DVSEC_CXL_CAP (0xA) via
> 		PCI_DVSEC_CXL_DEVICE (0x0)
> 
> The spec mentions in 8.2.1.3.1 DVSEC Flex Bus Port Capability: "
>   Note: The Mem_Capable, IO_Capable, and Cache_Capable fields are
>   also present in the DVSEC Flex Bus for the device [which is the
>   legacy name for DVSEC 0x0]. This allows for future scalability
>   where multiple devices, each with potentially different
>   capabilities, may be populated behind a single Port.
> "
> 
> Not arguing that set_pcie_cxl() is wrong, but I am not sure if there
> would be any side effect to rely on the "legacy name" over DVSEC 0x0.
> 
> Is there any CXL expert who can help confirm?

You appear to be confusing Cache_Capable and Cache_Enabled.

"8.2.1.3.1 DVSEC Flex Bus Port Capability" != "8.2.1.3.3 DVSEC Flex Bus Port Status"

Cache_Capable is only a capability. To check that the device has
actually trained the CXL.cache alternate protocol you need to look at
the status register.