RE: [PATCH RFC v3 02/12] iommu: Add a flag to indicate immutable singleton group

[Tian, Kevin]

> From: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> Sent: Tuesday, April 12, 2022 1:09 PM
> On 2022/4/12 11:15, Tian, Kevin wrote:
> >> From: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> >> Sent: Sunday, April 10, 2022 6:25 PM
> >
> >>
> >> This adds a flag in the iommu_group struct to indicate an immutable
> >> singleton group, and uses standard PCI bus topology, isolation features,
> >> and DMA alias quirks to set the flag. If the device came from DT, assume
> >> it is static and then the singleton attribute can know from the device
> >> count in the group.
> >
> > where does the assumption come from?
> 
> Hotplug is the only factor that can dynamically affect the
> characteristics of IOMMU group singleton as far as I can see. If a
> device node was created from the DT, it could be treated as static,
> hence we can judge the singleton in iommu probe phase during boot.

I didn't get this. Let's look at your code in iommu_group_add_device():

 +	else if (is_of_node(dev_fwnode(dev)))
 +		group->immutable_singleton =
 +				(iommu_group_device_count(group) == 1);

Even if there is a multi-devices group above logic will set the flag when
the first device in the group is added since at that time there is only
one device in the group. We need other concrete information  to tell
it similar to how you walk PCI hierarchy to find out the fact... 

> >> +	/*
> >> +	 * The device could be considered to be fully isolated if
> >> +	 * all devices on the path from the parent to the host-PCI
> >> +	 * bridge are protected from peer-to-peer DMA by ACS.
> >> +	 */
> >> +	if (!pci_is_root_bus(pdev->bus) &&
> >> +	    !pci_acs_path_enabled(pdev->bus->self, NULL, REQ_ACS_FLAGS))
> >> +		return false;
> >> +
> >> +	/* Multi-function devices should have ACS enabled. */
> >> +	if (pdev->multifunction && !pci_acs_enabled(pdev, REQ_ACS_FLAGS))
> >> +		return false;
> >
> > Looks my earlier comment was lost, i.e. you can just use
> > pci_acs_path_enabled(pdev) to cover above two checks.
> 
> If a device is directly connected to the root bridge and it is not an
> MFD, do we still need ACS on it? The Intel idxd device seems to be such
> a device. I had a quick check with lspci, it has no ACS support.
> 
> I probably missed anything.
> 

single-function RCiEP doesn't need to implement ACS but this has
been covered by pci_acs_enabled() and pci_acs_path_enabled().