Re: Kernel 6.7 regression doesn't boot if using AMD eGPU

[Robin Murphy <robin.murphy@arm.com>]

On 2024-04-16 1:39 am, Jason Gunthorpe wrote:
> On Mon, Apr 15, 2024 at 10:44:34PM +0100, Robin Murphy wrote:
>> On 2024-04-15 7:57 pm, Eric Wagner wrote:
>>> Apologies if I made a mistake in the first bisect, I'm new to kernel
>>> debugging.
>>>
>>> I tested cedc811c76778bdef91d405717acee0de54d8db5 (x86/amd) and
>>> 3613047280ec42a4e1350fdc1a6dd161ff4008cc (core) directly and both were good.
>>> Then I ran git bisect again with e8cca466a84a75f8ff2a7a31173c99ee6d1c59d2
>>> as the bad and 6e6c6d6bc6c96c2477ddfea24a121eb5ee12b7a3 as the good and the
>>> bisect log is attached. It ended up at the same commit as before.
>>>
>>> I've also attached a picture of the boot screen that occurs when it hangs.
>>> 0000:05:00.0 is the PCIe bus address of the RX 580 eGPU that's causing the
>>> problem.
>>
>> Looks like 59ddce4418da483 probably broke things most - prior to that, the
>> fact that it's behind a Thunderbolt port would have always taken precedence
>> and forced IOMMU_DOMAIN_DMA regardless of what the driver may have wanted to
>> saywhereas now we ask the driver first, then complain that it conflicts
>> with the untrusted status and ultimately don't configure the IOMMU
>> at all.
> 
> Yes, if the driver wants to force a domain type it should be
> forced. Driver knows best. Eg AMD forces IDENTITY when the HW/driver
> is incapable of supporting otherwise.

No, in the case of AMD it only forces identity if it thinks the device 
might want to use PASIDs (because of the architectural limitation that 
the RID always operates in GPA space so can't have its own independent 
translation).

Either way, though, there's really little sense to that argument - if 
enforcing strict translation *might* compromise the device's 
functionality, we should instead go out of our way to ensure it's 
definitely as broken as possible? I fail to see how that can be 
justified as useful or desirable behaviour.

>> Meanwhile the GPU driver presumably goes on to believe it's using dma-direct
>> with no IOMMU present, resulting in fireworks when its traffic reaches the
>> IOMMU. Great :(
> 
> I wonder where is the missing error handling.. iommu probe failure
> should not go on to allow driver attach, there is no guarentee any DMA
> works now that many iommus are booting up in BLOCKED.

What do you mean error handling? After you spent a year rewriting the 
probing code to your own grand design, don't suggest you don't even know 
how it fundamentally works...

"Failing" iommu_probe_device is merely how we tell ourselves that we're 
not interested in a device, and consequently tell the rest of the kernel 
it doesn't have an IOMMU (via device_iommu_mapped() returning false). 
This is normal and expected for devices which legitimately have no IOMMU 
in the first place; conversely we don't do a great deal for unexpected 
failures since those typically represent system-fatal conditions 
whatever we might try to do. We've never had much of a notion of 
expected failures when an IOMMU *is* present, but even then, denying any 
trace of the IOMMU and removing ourselves from the picture is clearly 
not the ideal way to approach that. We're running off a bus notifier (or 
even later), so ultimately our return value is meaningless; at that 
point the device already exists and has been added to its bus, we can't 
undo that.

However it looks to be even more fun if failure occurs in *deferred* 
default domain creation via bus_iommu_probe(), since then we give up and 
dismiss the entire IOMMU. Except the x86 drivers ignore the return from 
iommu_device_register(), so further hilarity ensues...

I think I've now satisfied myself that a simple fix for the core code is 
appropriate and will write that up now; one other thing I couldn't quite 
figure out is whether the AMD driver somehow prevents PASIDs being used 
while the group is attached to a non-identity (and non-nested) domain - 
that's probably one for Vasant to confirm.

Thanks,
Robin.