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

[Vasant Hegde]

Hi,


On 4/16/2024 5:19 PM, Jason Gunthorpe wrote:
> On Tue, Apr 16, 2024 at 12:25:52PM +0100, Robin Murphy wrote:
>> 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).
> 
> AMD forces this because it doesn't yet have a way to automatically
> choose it's v1/v2 page table format during alloc domain. It is just a
> SW bug.

Yes. This will be fixed.

> 
> The CC/SNP limitation is also a SW bug but is more fatal as it can't
> even attach a v1 page table in this mode.

Memory Encryption needs Encryption bit support. So we enforce Paging mode (Both
AMD v1 and v2 page table format works fine).

SNP is hardware limitation. When SNP is enabled then IOMMU must be configured
with v1 page table. It won't support Identity mapping.

> 
>> 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.
> 
> For SNP cases the attach of a DMA domain will fail, so yes, moving the
> failure earlier and giving a clear message is desirable.

Its handled during initialization itself (iommu_snp_enable()). If IOMMU is not
configured with V1 page table we don't enable SNP.

-Vasant


> 
>> "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). 
> 
> Probing failing with ENODEV means the device has no iommu and the rest
> of the code should assume DMA direct will work.
> 
> Probing failing with any other error code means the device has an
> iommu and it couldn't be setup. DMA direct probably won't work today.
> 
> If you want all failure codes to mean the device is safe for DMA
> direct then we need to try and attach the IDENTITY domain on various
> probe failure paths too.
> 
>> 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
> 
> It really doesn't match the design here.
> 
> Jason