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

[Tian, Kevin]

> From: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> Sent: Wednesday, April 6, 2022 7:03 PM
> 
> On 2022/4/6 18:44, Tian, Kevin wrote:
> >> From: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> >> 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()
> > does.
> 
> 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?

SRIOV is same as MFD. and all those tricks are already considered
properly in pci_acs_enabled().

> 
> >
> >>
> >> 	/* 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!
> 
> > Thanks
> > Kevin
> 
> Best regards,
> baolu