On Thu, Dec 30, 2021 at 3:37 AM Michael Klinglesmith <michael.klinglesmith@...> wrote:

2) section 4.7.3.4: A discussion of No_snoop must also include Relaxed Order (RO).  When a root port forwards a mix of traffic with NS=0, RO=0 and NS=1 , RO=0.  The requirement to enforce ordering tends to lead to ignoring the NS hint and snooping the trarffic anyway.

Proposed text for 4.7.3.4:

A system is required to provide hardware managed coherence for PCIe traffic. A system may ignore the No_snoop hint bit and treat all PCIe traffic as HW coherent.  A system may choose to honor the No_snoop (NS) hint bit on incoming PCIe transactions.  In this case software must manage coherence for the memory used by the device issuing the transactions.  Such systems must also honor the relaxed order (RO) hint bit.  To take full advantage of SW coherence the device should set both NS and RO to 1.  Otherwise, when RO=0 the snooped and non-snooped traffic arriving through a host bridge must be kept ordered, eliminating the benefit of routing some traffic around the cache hierarchy to memory.

 

AFAIK, these are two separate attributes and a requester could set either or both of them.  I am not sure if we can link the two together. Please correct me if I am wrong but does it mean that ,for e.g, all the mem write requests which may have the NS attribute set must also have the RO attribute set?

Hi, thanks for the response.

My thinking on intX is that if the HW can provide all the required MSI support, then SW won’t need to fall back to MSIs.  The intent of the PCIe spec was to depricate INTx messages eventually.  For new risc-v platforms it seems reasonable to build a forward looking system that only supports MSI interrupts.  For the topology of a RCiEP and RCEC that have MSI or MSI-X support adding legacy INTx support as a requirement is just make work, since all modern software has the MSI support.   For Root Ports, it’s a bit more complicated because the platform designer has less control of what gets plugged in, still it seems reasonable at this time to make INTx support optional.

You are correct that RO and NS are separate bits and it’s legal for a card to set one and not the other.  My point is that in implementing the logic above the host bridge, if NS is set, but RO is not it doesn’t provide much value, because the transaction must be strongly ordered with previous writes.  In practice its easiest to send such transactions (NS=1, RO=0)  through the snooped path to maintain ordering.  

The key point is NS is a hint so the statement “Memory that is cacheable by harts is not kept coherent by hardware…” may or may not be true in an implementation.  So here’s a smaller change proposal that would make the text more accurate:

“ Memory that is cacheable by harts may not be kept coherent by hardware when PCIe transactions to that memory are marked with a No_Snoop bit of one.   On systems that honor the No Snoop hint, software must manage coherence on such memory; otherwise, software coherence management is not required.”

At the very least the typo on the No Snoop polarity should be corrected.

My final point on topology was that the the spec seems to make it illegal to implement a single host bridge that has both root ports and RCiEP attached.  Maybe that wasn’t the intent.  It would be nice to see a 3rd topology drawing with a single host bridge, a RCiEP, an RCEC and a Root Port all on Bus 0.  If we do add that topology the final rule of section 4.7.3.5 becomes not-needed:

• If both the topologies mentioned above are supported then RCiEP and RCEC must be implemented in a separate PCIe domain and must be addressable via a separate ECAM I/O region.

Intel south bridges, as an example, mix RCiEP and root ports in the same PCIe domain addressable by a single ECAM I/O region.  What is the motivation for excluding this topology?

Cheers,

Michael.

On Jan 4, 2022, at 11:16 AM, Mayuresh Chitale <mchitale@...> wrote:

On Thu, Dec 30, 2021 at 3:37 AM Michael Klinglesmith <michael.klinglesmith@...> wrote:

Hello,

I’m new to participating in the platform WG.  I’m working at SiFive now.  I spent the last 20 years doing PCIe compliant IO fabrics for Intel chipsets.

I have a few comments / questions about the PCIe:

1) section 4.7.3.3: Why are we requiring INTx?  We should allow a modern system to be built without the legacy INTx requirements.

The intention behind requiring intx was to ensure better compatibility. There are few cases where we fallback to INTx if MSI is not available. For e.g:

https://elixir.bootlin.com/linux/v5.15.12/source/drivers/pci/pcie/portdrv_core.c#L165. 

But if such cases are considered to be rare and not required to be supported then this requirement can be removed.

2) section 4.7.3.4: A discussion of No_snoop must also include Relaxed Order (RO).  When a root port forwards a mix of traffic with NS=0, RO=0 and NS=1 , RO=0.  The requirement to enforce ordering tends to lead to ignoring the NS hint and snooping the trarffic anyway.

Proposed text for 4.7.3.4:

A system is required to provide hardware managed coherence for PCIe traffic. A system may ignore the No_snoop hint bit and treat all PCIe traffic as HW coherent.  A system may choose to honor the No_snoop (NS) hint bit on incoming PCIe transactions.  In this case software must manage coherence for the memory used by the device issuing the transactions.  Such systems must also honor the relaxed order (RO) hint bit.  To take full advantage of SW coherence the device should set both NS and RO to 1.  Otherwise, when RO=0 the snooped and non-snooped traffic arriving through a host bridge must be kept ordered, eliminating the benefit of routing some traffic around the cache hierarchy to memory.

 

AFAIK, these are two separate attributes and a requester could set either or both of them.  I am not sure if we can link the two together. Please correct me if I am wrong but does it mean that ,for e.g, all the mem write requests which may have the NS attribute set must also have the RO attribute set?

 

2) section 4.7.3.5: Why do we exclude the possibility of a Host bridge that supports both RCiEP and Root Ports?

Actually the requirement is to support atleast one of them ie RCiEP (+RCEC) or Root port but an RC can certainly  support both.

Cheers,

Michael.