Re: [PATCH 12/18] cxl: Add helpers to calculate pci latency for the CXL device

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Wed, 8 Feb 2023 16:56:30 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> On 2/8/23 3:15 PM, Bjorn Helgaas wrote:
> > On Tue, Feb 07, 2023 at 01:51:17PM -0700, Dave Jiang wrote:  
> >>
> >>
> >> On 2/6/23 3:39 PM, Bjorn Helgaas wrote:  
> >>> On Mon, Feb 06, 2023 at 01:51:10PM -0700, Dave Jiang wrote:  
> >>>> The latency is calculated by dividing the FLIT size over the
> >>>> bandwidth. Add support to retrieve the FLIT size for the CXL
> >>>> device and calculate the latency of the downstream link.  
> >   
> >>> I guess you only care about the latency of a single link, not the
> >>> entire path?  
> >>
> >> I am adding each of the link individually together in the next
> >> patch. Are you suggesting a similar function like
> >> pcie_bandwidth_available() but for latency for the entire path?  
> > 
> > Only a clarifying question.
> >   
> >>>> +static int cxl_get_flit_size(struct pci_dev *pdev)
> >>>> +{
> >>>> +	if (cxl_pci_flit_256(pdev))
> >>>> +		return 256;
> >>>> +
> >>>> +	return 66;  
> >>>
> >>> I don't know about the 66-byte flit format, maybe this part is
> >>> CXL-specific?  
> >>
> >> 68-byte flit format. Looks like this is a typo from me.  
> > 
> > This part must be CXL-specific, since I don't think PCIe mentions
> > 68-byte flits.
> >   
> >>>> + * The table indicates that if PCIe Flit Mode is set, then CXL is in 256B flits
> >>>> + * mode, otherwise it's 68B flits mode.
> >>>> + */
> >>>> +static inline bool cxl_pci_flit_256(struct pci_dev *pdev)
> >>>> +{
> >>>> +	u32 lnksta2;
> >>>> +
> >>>> +	pcie_capability_read_dword(pdev, PCI_EXP_LNKSTA2, &lnksta2);
> >>>> +	return lnksta2 & BIT(10);  
> >>>
> >>> Add a #define for the bit.  
> >>
> >> ok will add.
> >>  
> >>>
> >>> AFAICT, the PCIe spec defines this bit, and it only indicates the link
> >>> is or will be operating in Flit Mode; it doesn't actually say anything
> >>> about how large the flits are.  I suppose that's because PCIe only
> >>> talks about 256B flits, not 66B ones?  
> >>
> >> Looking at CXL v1.0 rev3.0 6.2.3 "256B Flit Mode", table 6-4, it shows that
> >> when PCIe Flit Mode is set, then CXL is in 256B flits mode, otherwise, it is
> >> 68B flits. So an assumption is made here regarding the flit side based on
> >> the table.  
> > 
> > So reading PCI_EXP_LNKSTA2 and extracting the Flit Mode bit is
> > PCIe-generic, but the interpretation of "PCIe Flit Mode not enabled
> > means 68-byte flits" is CXL-specific?
> > 
> > This sounds wrong, but I don't know quite how.  How would the PCI core
> > manage links where Flit Mode being cleared really means Flit Mode is
> > *enabled* but with a different size?  Seems like something could go
> > wrong there.  
> 
> Looking at the PCIe base spec and the CXL spec, that seemed to be the 
> only way that implies the flit size for a CXL device as far as I can 
> tell. I've yet to find a good way to make that determination. Dan?

So a given CXL port has either trained up in:
* normal PCI (in which case all the normal PCI stuff applies) and we'll
  fail some of the other checks in the CXL driver never get hear here
  - I 'think' the driver will load for the PCI device to enable things
  like firmware upgrade, but we won't register the CXL Port devices
  that ultimately call this stuff.
  It's perfectly possible to have a driver that will cope with this
  but it's pretty meaningless for a lot of cxl type 3 driver.
* 68 byte flit (which was CXL precursor to PCI going flit based)
  Can be queried via CXL DVSEC Flex Bus Port Status CXL r3.0 8.2.1.3.3
* 256 byte flits (may or may not be compatible with PCIe ones as there
  are some optional latency optimizations)

So if the 68 byte flit is enabled the 256 byte one should never be and
CXL description is overriding the old PCIe

Hence I think we should have the additional check on the flex bus
dvsec even though it should be consistent with your assumption above.

Hmm. That does raise a question of how we take the latency optimized
flits into account or indeed some of the other latency impacting things
that may or may not be running - IDE in it's various modes for example.

For latency optimized we can query relevant bit in the flex bus port status.
IDE info will be somewhere I guess though no idea if there is a way to
know the latency impacts.  

Jonathan

> 
> 
> > 
> > Bjorn