Date: Tue, 16 Apr 2024 22:16:55 -0400
From: Eric Wagner <ewagner12@...il.com>
To: Vasant Hegde <vashegde@....com>
Cc: Jason Gunthorpe <jgg@...pe.ca>, Robin Murphy <robin.murphy@....com>, 
	Vasant Hegde <vasant.hegde@....com>, Joerg Roedel <joro@...tes.org>, Will Deacon <will@...nel.org>, 
	Suravee Suthikulpanit <suravee.suthikulpanit@....com>, iommu@...ts.linux.dev, 
	linux-kernel@...r.kernel.org
Subject: Re: Kernel 6.7 regression doesn't boot if using AMD eGPU

On Tue, Apr 16, 2024 at 6:53 AM Vasant Hegde <vashegde@....com> wrote:
>
> Hi Eric,
>
>
> On 4/16/2024 7:42 AM, Eric Wagner wrote:
> > On Mon, Apr 15, 2024 at 5:44 PM Robin Murphy <robin.murphy@....com
> > <mailto:robin.murphy@....com>> wrote:
> >
> >     As a first step I'd test the quick hack below, but be prepared for things
> >     to still break slightly differently.
> >
> >     Cheers,
> >     Robin.
> >
> >     ----->8-----
> >     diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> >     index 996e79dc582d..063e1eb32fbd 100644
> >     --- a/drivers/iommu/iommu.c
> >     +++ b/drivers/iommu/iommu.c
> >     @@ -1774,7 +1774,7 @@ static int iommu_get_default_domain_type(struct
> >     iommu_group *group,
> >                                      untrusted,
> >                                      "Device is not trusted, but driver is
> >     overriding group %u to %s,
> >     refusing to probe.\n",
> >                                      group->id, iommu_domain_type_str(driver_type));
> >     -                       return -1;
> >     +                       //return -1;
> >                      }
> >                      driver_type = IOMMU_DOMAIN_DMA;
> >              }
> >
> > This worked and got the system booting for me.
>
> Thanks for testing.
>
> IIUC eGPU is behind Thunderbolt  and hence IOMMU treated it as 'untrusted device'.
>
> AMD driver tries to allocate "IDENTITY" domain for GPU devices.
> iommu_get_default_domain_type() return -1 as it expects IOMMU_DOMAIN_DMA for
> untrusted device.
>
> Can you please attach lspci -vvv output? Also /proc/cmdline output.
>
>
> >
> > On Mon, Apr 15, 2024 at 8:39 PM Jason Gunthorpe <jgg@...pe.ca
> > <mailto:jgg@...pe.ca>> 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.
>
>
> @Jason,
>
> Looks like before commit 59ddce4418da483, core IOMMU layer was enforcing
> 'IOMMU_DOMAIN_DMA' for untrusted device. If its trusted device then it was
> letting HW IOMMU driver to pick best domain type.
>
> Did we change that flow? Are we expecting HW IOMMU driver to handler untrusted
> devices as well?
>
> >
> >      > 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.
>
>
> Looking into code path, in failure path we cleanup device, not group. May be
> that's causing issue? Not sure where it failed. If I manage to find some system
> I will try to debug further.
>
>
>
> >
> >      > However the other notable thing that also happened between 6.6 and 6.7 was
> >      > the removal of the AMD iommu_v2 code, so there's some possibility that the
> >      > GPU driver still may have only been working before due to that also
> >
> >     Most likely it is the above change interacting with this patch when
> >     they are both combined in the merge:
> >
> >     commit 92e2bd56a5f9fc44313fda802a43a63cc2a9c8f6
> >     Author: Vasant Hegde <vasant.hegde@....com <mailto:vasant.hegde@...com>>
> >     Date:   Thu Sep 21 09:21:45 2023 +0000
> >
> >          iommu/amd: Introduce iommu_dev_data.flags to track device capabilities
> >
> >     @@ -2471,7 +2481,7 @@ static int amd_iommu_def_domain_type(struct device *dev)
> >               *    and require remapping.
> >               *  - SNP is enabled, because it prohibits DTE[Mode]=0.
> >               */
> >     -       if (dev_data->iommu_v2 &&
> >     +       if (pdev_pasid_supported(dev_data) &&
> >                  !cc_platform_has(CC_ATTR_MEM_ENCRYPT) &&
> >                  !amd_iommu_snp_en) {
> >                      return IOMMU_DOMAIN_IDENTITY;
> >
> >     Which, IIRC, was intended to be temporary to work around limitations
> >     in the DTE programming logic within the driver. Previously iommu_v2 as
> >     a module option that Eric probably doesn't set, I guess.
> >
> >     The below will probably make it boot, but Vasant should check what
> >     happens if PASID is eventually attached too.
> >
> >     diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> >     index d35c1b8c8e65ce..f3da6a5b6cb1cb 100644
> >     --- a/drivers/iommu/amd/iommu.c
> >     +++ b/drivers/iommu/amd/iommu.c
> >     @@ -2758,11 +2758,16 @@ static int amd_iommu_def_domain_type(struct device *dev)
> >               *    and require remapping.
> >               *  - SNP is enabled, because it prohibits DTE[Mode]=0.
> >               */
> >     -       if (pdev_pasid_supported(dev_data) &&
> >     -           !cc_platform_has(CC_ATTR_MEM_ENCRYPT) &&
> >     -           !amd_iommu_snp_en) {
> >     +       if (!cc_platform_has(CC_ATTR_MEM_ENCRYPT) && !amd_iommu_snp_en)
> >     +               return IOMMU_DOMAIN_IDENTITY;
> >     +
> >     +       /*
> >     +        * For now driver limitations prevent enabling PASID as a paging domain
> >     +        * on the RID together.
> >     +        */
> >     +       if (dev_is_pci(dev) && !to_pci_dev(dev)->untrusted &&
> >     +           pdev_pasid_supported(dev_data))
> >                      return IOMMU_DOMAIN_IDENTITY;
> >     -       }
> >
> >              return 0;
> >       }
> >
> >     Jason
> >
> >
> > As it booted ok with Robin's patch above, these changes to
> > drivers/iommu/amd/iommu.c didn't seem to make a difference for me. I was testing
> > with amd iommu v2 off in the kernel config and I also have TSME enabled in the
> > BIOS if that matters.
>
> TMSE is transparent to OS. So its fine.
>
> -Vasant
>
Output of lspci -vvv and /proc/cmdline attached.

-Eric

View attachment "cmdline.log" of type "text/x-log" (83 bytes)

View attachment "lspci.log" of type "text/x-log" (84184 bytes)

Powered by blists - more mailing lists