Re: [PATCH v3 1/1] PCI: Add Extended Tag + MRRS quirk for Xeon 6

[Dan Williams]

Ilpo Järvinen wrote:
> When bifurcated to x2, Xeon 6 Root Port performance is sensitive to the
> configuration of Extended Tags, Max Read Request Size (MRRS), and 10-Bit
> Tag Requester (note: there is currently no 10-Bit Tag support in the
> kernel). While those can be configured to the recommended values by FW,
> kernel may decide to overwrite the initial values.
> 
> Add a quirk that disallows enabling Extended Tags and setting MRRS
> larger than 128B for devices under Xeon 6 Root Ports if the Root Port
> is bifurcated to x2. Use the host bridge's enable_device hook to
> overwrite MRRS if it's set to >128B for the device to be enabled.
> 
> The earlier attempts to implement this quirk polluted PCI core code
> with the checks necessary to support this quirk. Using the
> enable_device hook keeps the quirk well-contained, away from the PCI
> core code.
> 
> Suggested-by: Lukas Wunner <lukas@xxxxxxxxx>
> Link: https://cdrdv2.intel.com/v1/dl/getContent/837176
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> ---
> 
> Ingo gave his Ack on v2 but since the used approach has once again
> changed, I didn't add his Ack.
> 
> Mani did express his concern on using enable_device hook but suggested
> I send the patch anyway.
> 
> We're also looking into using _HPX Type 3 (suggested by Bjorn) eventually
> for this class of problems where FW settings get overwritten by the
> kernel (but it will take time to make it the sanctioned solution). In
> the meantime, this is a real problem for Xeon 6 out there so it warrants
> adding the quirk (which is now pretty well-contained).
> 
> v3:
> - Use enable_device hook to overwrite MRRS to 128B if needed. (Lukas)
> - Typo fix to comment (Ingo)
> 
> v2:
> - Explain in changelog why FW cannot solve this on its own
> - Moved the quirk under arch/x86/pci/
> - Don't NULL check value from pci_find_host_bridge()
> - Added comment above the quirk about the performance degradation
> - Removed all setup chain 128B quirk overrides expect for MRRS write
>   itself (assumes a sane initial value is set by FW)
> 
> 
>  arch/x86/pci/fixup.c | 40 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)

Thanks for the work on this Ilpo, and thanks for the cleanup suggestions
Lukas. This version looks reasonable to carry the kernel until _HPX can
be relied upon to help the kernel apply quirks like this.

You can add:

Reviewed-by: Dan Williams <dan.j.williams@xxxxxxxxx>