Messages in this thread | | | From | "Tian, Kevin" <> | Subject | RE: [PATCH RFC v2 02/11] iommu: Add iommu_group_singleton_lockdown() | Date | Wed, 6 Apr 2022 23:56:24 +0000 |
| |
> From: Lu Baolu <baolu.lu@linux.intel.com> > Sent: Wednesday, April 6, 2022 7:03 PM > > On 2022/4/6 18:44, Tian, Kevin wrote: > >> From: Lu Baolu <baolu.lu@linux.intel.com> > >> 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
| |