Message-ID: <612bf6f2-17a4-46fe-a5cd-ecb7023235ef@amd.com>
Date: Mon, 22 Jul 2024 09:44:32 -0500
From: Wei Huang <wei.huang2@....com>
To: Lukas Wunner <lukas@...ner.de>
Cc: linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-doc@...r.kernel.org, netdev@...r.kernel.org,
 Jonathan.Cameron@...wei.com, helgaas@...nel.org, corbet@....net,
 davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
 pabeni@...hat.com, alex.williamson@...hat.com, gospo@...adcom.com,
 michael.chan@...adcom.com, ajit.khaparde@...adcom.com,
 somnath.kotur@...adcom.com, andrew.gospodarek@...adcom.com,
 manoj.panicker2@....com, Eric.VanTassell@....com, vadim.fedorenko@...ux.dev,
 horms@...nel.org, bagasdotme@...il.com, bhelgaas@...gle.com,
 Paul Luse <paul.e.luse@...el.com>, Jing Liu <jing2.liu@...el.com>
Subject: Re: [PATCH V3 00/10] PCIe TPH and cache direct injection support



On 7/20/24 03:08, Lukas Wunner wrote:
> [cc += Paul Luse, Jing Liu]
> 
> On Wed, Jul 17, 2024 at 03:55:01PM -0500, Wei Huang wrote:
>> TPH (TLP Processing Hints) is a PCIe feature that allows endpoint devices to
>> provide optimization hints for requests that target memory space. These hints,
>> in a format called steering tag (ST), are provided in the requester's TLP
>> headers and allow the system hardware, including the Root Complex, to
>> optimize the utilization of platform resources for the requests.
> [...]
>> This series introduces generic TPH support in Linux, allowing STs to be
>> retrieved from ACPI _DSM (as defined by ACPI) and used by PCIe endpoint
>> drivers as needed. As a demonstration, it includes an example usage in the
>> Broadcom BNXT driver. When running on Broadcom NICs with the appropriate
>> firmware, Cache Injection shows substantial memory bandwidth savings and
>> better network bandwidth using real-world benchmarks. This solution is
>> vendor-neutral, as both TPH and ACPI _DSM are industry standards.
> 
> I think you need to add support for saving and restoring TPH registers,
> otherwise the changes you make to those registers may not survive
> reset recovery or system sleep.  Granted, system sleep may not be
> relevant for servers (which I assume you're targeting with your patches),
> but reset recovery very much is.
> 
> Paul Luse submitted a patch two years ago to save and restore
> TPH registers, perhaps you can include it in your patch set?

Thanks for pointing them out. I skimmed through Paul's patch and it is
straightforward to integrate.

Depending on Bjorn's preference, I can either integrate it into my
patchset with full credits to Paul and Jing, or Paul want to resubmit a
new version.

> 
> https://lore.kernel.org/all/20220712123641.2319-1-paul.e.luse@intel.com/
> 
> Bjorn left some comments on Paul's patch:
> 
> https://lore.kernel.org/all/20220912214516.GA538566@bhelgaas/
> 
> In particular, Bjorn asked for shared infrastructure to access
> TPH registers (which you're adding in your patch set) and spotted
> several nits (which should be easy to address).  So I think you may
> be able to integrate Paul's patch into your series without too much
> effort.

I read Bjorn's comments, lots of them have been addressed in my patchset
(e.g. move under /pci/pcie, support _DSM and dev->tph). So, as I said,
integration is doable.

> 
> However note that when writing to TPH registers through the API you're
> introducing, you also need to update the saved register state so that
> those changes aren't lost upon a subsequent reset recovery.
> 
> Thanks,
> 
> Lukas

Powered by blists - more mailing lists