Re: [PATCH RFC v2 02/11] iommu: Add iommu_group_singleton_lockdown()

From: Lu Baolu
Date: Wed Apr 06 2022 - 10:38:40 EST

On 2022/4/6 18:44, Tian, Kevin wrote:
From: Lu Baolu <>
Sent: Wednesday, April 6, 2022 6:02 PM

Hi Kevin,

On 2022/4/2 15:12, Tian, Kevin wrote:
Add a flag to the group that positively indicates the group can never
have more than one member, even after hot plug. eg because it is
impossible due to ACS, or lack of bridges, and so on.
OK, I see your point. It essentially refers to a singleton group which
is immutable to hotplug.
Yes, known at creation time, not retroactively enforced because
someone used SVA

We may check following conditions to set the immutable flag when
a new group is created for a device in pci_device_group():

1) ACS is enabled in the upstream path of the device;
2) the device is single function or ACS is enabled on a multi-function device;
3) the device type is PCI_EXP_TYPE_ENDPOINT (thus no hotplug);
4) no 'dma aliasing' on this device;

The last one is a bit conservative as it also precludes a device which aliasing
dma due to quirks from being treated as a singleton group. But doing so
saves the effort on trying to separate different aliasing scenarios as defined
in pci_for_each_dma_alias(). Probably we can go this way as the first step.

Once the flag is set on a group no other event can change it. If a new
identified device hits an existing singleton group in pci_device_group()
then it's a bug.

How about below implementation?

/* callback for pci_for_each_dma_alias() */
static int has_pci_alias(struct pci_dev *pdev, u16 alias, void *opaque)
return -EEXIST;

static bool pci_dev_is_immutably_isolated(struct pci_dev *pdev)
/* Skip bridges. */
if (pci_is_bridge(pdev))
return false;

/* Either connect to root bridge or the ACS-enabled bridge. */
if (!pci_is_root_bus(pdev->bus) &&
!pci_acs_enabled(pdev->bus->self, REQ_ACS_FLAGS))
return false;

it's not sufficient to just check the non-root bridge itself. This needs to
cover the entire path from the bridge to the root port, as pci_device_group()

Yes! You are right.

/* ACS is required for MFD. */
if (pdev->multifunction && !pci_acs_enabled(pdev, REQ_ACS_FLAGS))
return false;

Above two checks be replaced by a simple check as below:

if (!pci_acs_path_enabled(pdev, NULL, REQ_ACS_FLAGS))
return false;

If !pdev->multifunction, do we still need to start from the device
itself? ACS is only for MFDs and bridges, do I understand it right?
Do we need to consider the SRIOV case?

/* Make sure no PCI alias. */
if (pci_for_each_dma_alias(pdev, has_pci_alias, NULL))
return false;

return true;

I didn't get why do we need to check the PCI_EXP_TYPE_ENDPOINT device
type. Can you please elaborate a bit more?

I didn't know there is a pci_is_bridge() facility thus be conservative
to restrict it to only endpoint device. If checking pci_is_bridge() alone
excludes any hotplug possibility, then it's definitely better.

Okay! Thanks!


Best regards,