Re: [regression, bisected, pci/iommu] Bug 216865 - Black screen when amdgpu started during 6.2-rc1 boot with AMD IOMMU enabled

[Baolu Lu]

On 2023/1/9 21:43, Jason Gunthorpe wrote:
> On Sat, Jan 07, 2023 at 10:44:46AM +0800, Baolu Lu wrote:
> > On 1/6/2023 10:14 PM, Jason Gunthorpe wrote:
> > > On Thu, Jan 05, 2023 at 03:57:28PM +0530, Vasant Hegde wrote:
> > > > Matt,
> > > >
> > > > On 1/5/2023 6:39 AM, Matt Fagnani wrote:
> > > > > I built 6.2-rc2 with the patch applied. The same black screen problem happened
> > > > > with 6.2-rc2 with the patch. I tried to use early kdump with 6.2-rc2 with the
> > > > > patch twice by panicking the kernel with sysrq+alt+c after the black screen
> > > > > happened. The system rebooted after about 10-20 seconds both times, but no kdump
> > > > > and dmesg files were saved in /var/crash. I'm attaching the lspci -vvv output as
> > > > > requested.
> > > >
> > > > Thanks for testing. As mentioned earlier I was not expecting this patch to fix
> > > > the black screen issue. It should fix kernel warnings and IOMMU page fault
> > > > related call traces. By any chance do you have the kernel boot logs?
> > > >
> > > >
> > > > @Baolu,
> > > >     Looking into lspci output, it doesn't list ACS feature for Graphics card. So
> > > > with your fix it didn't enable PASID and hence it failed to boot.
> > >
> > > The ACS checks being done are feature of the path not the end point or
> > > root port.
> > >
> > > If we are expecting ACS on the end port then it is just a bug in how
> > > the test was written.. The test should be a NOP because there are no
> > > switches in this topology.
> > >
> > > Looking at it, this seems to just be because pci_enable_pasid is
> > > calling pci_acs_path_enabled wrong, the only other user is here:
> > >
> > > 	for (bus = pdev->bus; !pci_is_root_bus(bus); bus = bus->parent) {
> > > 		if (!bus->self)
> > > 			continue;
> > >
> > > 		if (pci_acs_path_enabled(bus->self, NULL, REQ_ACS_FLAGS))
> > > 			break;
> > >
> > > 		pdev = bus->self;
> > >
> > > 		group = iommu_group_get(&pdev->dev);
> > > 		if (group)
> > > 			return group;
> > > 	}
> > >
> > > And notice it is calling it on pdev->bus not on pdev itself which
> > > naturally excludes the end point from the ACS validation.
> > >
> > > So try something like:
> > >
> > > 	if (!pci_acs_path_enabled(pdev->bus->self, NULL, PCI_ACS_RR | PCI_ACS_UF))
> > >
> > > (and probably need to check for null ?)
> >
> > Yeah! This really is a misuse of pci_acs_path_enabled().
> >
> > But if @pdev is an endpoint of a multiple function device, perhaps we
> > still need to check acs on it?
>
> Ah, I don't know anything about what this means from a spec
> perspective.
>
> Certainly if a function can internalize MMIO and loop it back to
> another function then it surely is not OK for PASID either, nor should
> those functions be in different iommu groups.
>
> So, either this never happens for some spec reason, or the test in the
> iommu code forming groups is incorrect.

The pci_device_group() path handles this like below:

/*
 * For multifunction devices which are not isolated from each other, find
 * all the other non-isolated functions and look for existing groups.  For
 * each function, we also need to look for aliases to or from other devices
 * that may already have a group.
 */
static struct iommu_group *get_pci_function_alias_group(struct pci_dev 
*pdev,
                                                        unsigned long 
*devfns)
{
        struct pci_dev *tmp = NULL;
        struct iommu_group *group;

        if (!pdev->multifunction || pci_acs_enabled(pdev, REQ_ACS_FLAGS))
                return NULL;

It seems that all devices of an MFD shares a single iommu group if
there lacks ACS control.

--
Best regards,
baolu