lkml.org 
[lkml]   [2022]   [Apr]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH RFC v2 02/11] iommu: Add iommu_group_singleton_lockdown()
From
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?

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

\
 
 \ /
  Last update: 2022-04-06 16:41    [W:0.325 / U:0.092 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site