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

[Baolu Lu <baolu.lu@linux.intel.com>]

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