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

[Lu Baolu]

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;

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

	/* 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?

Best regards,
baolu