Re: [PATCH] PCI: Align MPS to upstream bridge for SAFE and PERFORMANCE mode

[Tyler Hicks <code@tyhicks.com>]

On 2022-11-09 17:42:10, Tyler Hicks (Microsoft) wrote:
> On 2022-11-03 17:14:29, Bjorn Helgaas wrote:
> > On Thu, Oct 27, 2022 at 05:51:49PM -0500, Bjorn Helgaas wrote:
> > > On Tue, Oct 25, 2022 at 12:07:47AM -0500, Tyler Hicks wrote:
> > > > On 2022-10-20 15:24:37, Bjorn Helgaas wrote:
> > 
> > > > I've been talking to the firmware folks on why SAFE mode was selected,
> > > > based on Keith's question from Wednesday. From what I've been told,
> > > > U-Boot doesn't seed the RP MPS with a value so the kernel sees a value
> > > > of 128 for p_mps in pci_configure_mps() when using the DEFAULT mode.
> > > > Apparently UEFI does seed the RP MPS but we don't get that with U-Boot.
> > > > Therefore, SAFE mode was selected to get an initial, tuned RP MPS value
> > > > set to 256.
> > > 
> > > Are there any devices below the RP at the time we set MPS=256?
> > > 
> > > > > A subsequent hot-add will do nothing in pci_configure_mps(), and
> > > > > pcie_bus_configure_settings() looks like it would set the RP and EP
> > > > > MPS to the minimum of the RP and EP MPS_Supported.
> > > > > 
> > > > > Is that what you see?  What would you like to see instead?
> > > > 
> > > > No, not quite. After hot-add, we see the EP MPS set to 128 with SAFE
> > > > mode even though the RP MPS is 256.
> > > 
> > > Can you add the relevant topology here so we can work through the
> > > concrete details?
> 
> # lspci -t
> -[0000:00]---00.0-[01-ff]--+-00.0
>                            +-00.1
>                            +-00.2
>                            +-00.3
>                            \-00.4
> 
> 
> > > Is the endpoint hot-added directly below a Root Port?  Or is there a
> > > switch involved?
> 
> There's not a switch involved. The multifunction endpoint is hot-added directly
> below the root port.
> 
> > > What are the MPS_Supported values for all the devices?  If there's a switch
> > > in the picture, it looks like we currently restrict to 128, I think because
> > > it's possible an endpoint that can only do 128 may be added.
> 
> 0000:00's MPS_Supported value is 256.
> 
> The multifunction endpoint's MPS_Supported is 512.
> 
> > Ping?  I'd like to talk about a concrete scenario.  It's too hard for
> > me to imagine all the possible things that could go wrong.
> 
> Sorry for the slow reply here. Thanks for your interest in getting more
> details.

Hey Bjorn - I wanted to re-ping you on this discussion since we're on
the other side of the merge window now. Let me know if you need anymore
details. Thanks!

Tyler

> 
> > I guess part of what's interesting here is that things work better
> > when firmware has already configured MPS?  It doesn't seem like we
> > should *depend* on firmware having done that.
> 
> Our firmware folks felt the same way.
> 
> Tyler
> 
> > 
> > Bjorn