Re: Properly reflect IOMMU DMA Protection in 5.15.y+

[Mario Limonciello <mario.limonciello@amd.com>]

On 9/2/22 02:52, Greg KH wrote:
> On Fri, Sep 02, 2022 at 12:37:15AM -0500, Mario Limonciello wrote:
>> On 9/2/22 00:29, Greg KH wrote:
>>> On Fri, Sep 02, 2022 at 03:00:26AM +0000, Limonciello, Mario wrote:
>>>> [Public]
>>>>
>>>> Hi,
>>>>
>>>> A sysfs file /sys/bus/thunderbolt/devices/domainX/iommu_dma_protection is exported from the kernel and used by userspace to make judgments on whether to automatically authorize PCIe tunnels for USB4 devices.
>>>> Before kernel 5.19 this file was only populated on Intel USB4 and TBT3 controllers, but starting with 5.19 it also populates for non-Intel as well.
>>>
>>> So that's a new kernel feature?
>>
>> The sysfs file was there all along, but it always showed "0" for anything
>> but Intel systems.  This makes it work properly on everyone else.
>>
>>>
>>>> This is accomplished by an assertion from the IOMMU subsystem that it's safe to do so by a combination of firmware and hardware.
>>>>
>>>> Here is the patch series on top of 5.15.64:
>>>>
>>>> 3f6634d997dba8140b3a7cba01776b9638d70dff
>>>> ed36d04e8f8d7b00db451b0fa56a54e8e02ec43e
>>>> d0be55fbeb6ac694d15af5d1aad19cdec8cd64e5
>>>> f316ba0a8814f4c91e80a435da3421baf0ddd24c
>>>> f1ca70717bcb4525e29da422f3d280acbddb36fe
>>>> bfb3ba32061da1a9217ef6d02fbcffb528e4c8df
>>>> 418e0a3551bbef5b221705b0e5b8412cdc0afd39
>>>> acdb89b6c87a2d7b5c48a82756e6f5c6f599f60a
>>>> ea4692c75e1c63926e4fb0728f5775ef0d733888
>>>> 86eaf4a5b4312bea8676fb79399d9e08b53d8e71
>>>>
>>>> Can you please consider backporting them to 5.15.y+?
>>>
>>> I don't understand why all of the string helpers are needed just for the
>>> last commit, are you sure this is all necessary?
>>>
>> The last commit (thunderbolt commit) uses one of them.  That commit for the
>> one of them doesn't come back cleanly, but catching all of them up does.
>>
>> So I could potentially change the thunderbolt commit to not use the string
>> helper, but figured this was cleaner.
>>
>>> And again, this feels like new features are being added that are much
>>> more than just a "new device id added".  Why not just use 5.19 for this
>>> hardware?
>>
>> Stuff like this is targeted towards businesses that would want to be using
>> LTS kernels.  In fact I heard "But on Intel we just plug in the dock and it
>> just works" is what prompted the series in the first place.
>>
>> It improves usability quite a bit because without it you need to know to
>> manually change the sysfs file for your dock to work or you need to have
>> GNOME installed and go and find the panel to change it.
>>
>> With this sysfs file is showing the right value you get all that happening
>> automatically.
> 
> I understand the wish to have hardware work on older kernel versions,
> but in looking over the full patch series here, it's not just a trivial
> addition.  This is adding lots of things that were never in 5.15 to
> start with for AMD hardware (and touching other platform's code at the
> same time), which is fine for newer kernels, but not to backport to
> older ones.
> 
> Here's the overall diffstat of what you are asking for:
> 
>   drivers/iommu/amd/amd_iommu_types.h |  4 ++++
>   drivers/iommu/amd/init.c            |  3 +++
>   drivers/iommu/amd/iommu.c           |  2 ++
>   drivers/iommu/dma-iommu.c           |  5 +++++
>   drivers/iommu/intel/iommu.c         |  2 ++
>   drivers/iommu/iommu.c               | 73 ++++++++++++++++++++++++++++++++++++++++++++++++-------------------------
>   drivers/thunderbolt/domain.c        | 12 +++---------
>   drivers/thunderbolt/nhi.c           | 44 ++++++++++++++++++++++++++++++++++++++++++++
>   include/linux/iommu.h               | 19 +++++++++++++++++++
>   include/linux/string_helpers.h      | 25 +++++++++++++++++++++++++
>   include/linux/thunderbolt.h         |  2 ++
>   lib/string_helpers.c                | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   12 files changed, 221 insertions(+), 34 deletions(-)
> 
> The string helpers are trivial, but those iommu changes were not.
> 
> As proof of that, you missed some fixes in the kernel tree for the above
> requested commits that would have caused problems.  Heck, even the
> string helpers needed a fix that you missed, so I was wrong in saying
> they were trivial!
> 
> So if I would have taken these commits as asked for, they might not have
> even worked properly and caused problems for people.  So for that reason
> alone I would have had to reject this request.
> 
> Remember, stable kernels are not for "new hardware enablement", unlike
> how some distro kernels work.  If you wanted this hardware to be
> supported for last year's stable kernel release, you all would have done
> the work back then to get it accepted as obviously you all had the
> hardware back then and knew it was going to be an issue.
> 
> Just point any user of this hardware to the latest kernel release, and
> all will be fine.  And work on getting your new hardware supported
> upstream quicker please.  That will prevent this issue from happening
> again in the future.
> 
> thanks,
> 
> greg k-h

Got it, thanks for your review and guidance.