RE: [PATCH RFC v2 02/11] iommu: Add iommu_group_singleton_lockdown()
> From: Lu Baolu
> Sent: Wednesday, April 6, 2022 7:03 PM
>
> On 2022/4/6 18:44, Tian, Kevin wrote:
> >> From: Lu Baolu
> >> 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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC v2 02/11] iommu: Add iommu_group_singleton_lockdown()
On 2022/4/6 18:44, Tian, Kevin wrote:
From: Lu Baolu
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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH RFC v2 02/11] iommu: Add iommu_group_singleton_lockdown()
> From: Lu Baolu
> 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.
>
> /* 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;
>
> /* 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.
Thanks
Kevin
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC v2 02/11] iommu: Add iommu_group_singleton_lockdown()
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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC v2 02/11] iommu: Add iommu_group_singleton_lockdown()
On 2022/4/5 22:10, Jason Gunthorpe wrote: On Tue, Apr 05, 2022 at 02:12:42PM +0800, Lu Baolu wrote: On 2022/4/5 1:24, Jason Gunthorpe wrote: On Mon, Apr 04, 2022 at 01:43:49PM +0800, Lu Baolu wrote: On 2022/3/30 19:58, Jason Gunthorpe wrote: Testing the group size is inherently the wrong test to make. What is your suggestion then? 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. The check method seems to be bus specific. For platform devices, perhaps this kind of information should be retrieved from firmware interfaces like APCI or DT. From this point of view, would it be simpler and more reasonable for the device driver to do such check? After all, it is the device driver that decides whether to provide SVA services to the application via uacce. The check has to do with the interconnect, not the device - I don't see how a device driver would know any better. I'm worried about how to support this group flag for devices that are not connected to the system through PCI buses. If IOMMU can support sva_bind() only when this flag is set, the SVA on many devices cannot be supported. Or this flag is always set for non PCI devices by default? IHMO it is not so different from how we determine if ACS like functionality is supported on non-PCI. It is really just a more narrow application of the existing ACS idea. For instance it may be that if the iommu_group came from DT we can assume it is static and then singleton can know ACS is reliable. Okay, let me head this direction. Best regards, baolu ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC v2 02/11] iommu: Add iommu_group_singleton_lockdown()
On Tue, Apr 05, 2022 at 02:12:42PM +0800, Lu Baolu wrote: > On 2022/4/5 1:24, Jason Gunthorpe wrote: > > On Mon, Apr 04, 2022 at 01:43:49PM +0800, Lu Baolu wrote: > > > On 2022/3/30 19:58, Jason Gunthorpe wrote: > > > > > > Testing the group size is inherently the wrong test to make. > > > > > What is your suggestion then? > > > > 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. > > > > > > The check method seems to be bus specific. For platform devices, perhaps > > > this kind of information should be retrieved from firmware interfaces > > > like APCI or DT. > > > > > > From this point of view, would it be simpler and more reasonable for the > > > device driver to do such check? After all, it is the device driver that > > > decides whether to provide SVA services to the application via uacce. > > > > The check has to do with the interconnect, not the device - I don't > > see how a device driver would know any better. > > I'm worried about how to support this group flag for devices that are > not connected to the system through PCI buses. If IOMMU can support > sva_bind() only when this flag is set, the SVA on many devices cannot > be supported. Or this flag is always set for non PCI devices by > default? IHMO it is not so different from how we determine if ACS like functionality is supported on non-PCI. It is really just a more narrow application of the existing ACS idea. For instance it may be that if the iommu_group came from DT we can assume it is static and then singleton can know ACS is reliable. Jason ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC v2 02/11] iommu: Add iommu_group_singleton_lockdown()
On 2022/4/5 1:24, Jason Gunthorpe wrote: On Mon, Apr 04, 2022 at 01:43:49PM +0800, Lu Baolu wrote: On 2022/3/30 19:58, Jason Gunthorpe wrote: Testing the group size is inherently the wrong test to make. What is your suggestion then? 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. The check method seems to be bus specific. For platform devices, perhaps this kind of information should be retrieved from firmware interfaces like APCI or DT. From this point of view, would it be simpler and more reasonable for the device driver to do such check? After all, it is the device driver that decides whether to provide SVA services to the application via uacce. The check has to do with the interconnect, not the device - I don't see how a device driver would know any better. I'm worried about how to support this group flag for devices that are not connected to the system through PCI buses. If IOMMU can support sva_bind() only when this flag is set, the SVA on many devices cannot be supported. Or this flag is always set for non PCI devices by default? Why do you bring up uacce? Nothing should need uacce to access SVA. The uacce is irrelevant here. Best regards, baolu ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC v2 02/11] iommu: Add iommu_group_singleton_lockdown()
On Mon, Apr 04, 2022 at 01:43:49PM +0800, Lu Baolu wrote: > On 2022/3/30 19:58, Jason Gunthorpe wrote: > > > > Testing the group size is inherently the wrong test to make. > > > What is your suggestion then? > > 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. > > The check method seems to be bus specific. For platform devices, perhaps > this kind of information should be retrieved from firmware interfaces > like APCI or DT. > > From this point of view, would it be simpler and more reasonable for the > device driver to do such check? After all, it is the device driver that > decides whether to provide SVA services to the application via uacce. The check has to do with the interconnect, not the device - I don't see how a device driver would know any better. Why do you bring up uacce? Nothing should need uacce to access SVA. Jason ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC v2 02/11] iommu: Add iommu_group_singleton_lockdown()
On 2022/3/30 19:58, Jason Gunthorpe wrote: Testing the group size is inherently the wrong test to make. What is your suggestion then? 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. The check method seems to be bus specific. For platform devices, perhaps this kind of information should be retrieved from firmware interfaces like APCI or DT. From this point of view, would it be simpler and more reasonable for the device driver to do such check? After all, it is the device driver that decides whether to provide SVA services to the application via uacce. Best regards, baolu ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC v2 02/11] iommu: Add iommu_group_singleton_lockdown()
On Sat, Apr 02, 2022 at 07:12:12AM +, Tian, Kevin wrote: > That is one scenario of dma aliasing. Another is like Alex replied where > one device has an alias requestor ID due to PCI quirks. The alias RID > may or may not map to a real device but probably what we really care > here regarding to p2p are struct devices listed in the group. Those devices are just broken and not spec complaint. IMHO we can treat them as disabling ACS for their segment as well. > 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; It makes sense to me Jason ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH RFC v2 02/11] iommu: Add iommu_group_singleton_lockdown()
> From: Jason Gunthorpe > Sent: Wednesday, March 30, 2022 10:30 PM > > On Wed, Mar 30, 2022 at 02:12:57PM +, Tian, Kevin wrote: > > > From: Jason Gunthorpe > > > Sent: Wednesday, March 30, 2022 7:58 PM > > > > > > On Wed, Mar 30, 2022 at 06:50:11AM +, Tian, Kevin wrote: > > > > > > > One thing that I'm not very sure is about DMA alias. Even when > physically > > > > there is only a single device within the group the aliasing could lead > > > > to multiple RIDs in the group making it non-singleton. But probably we > > > > don't need support SVA on such device until a real demand comes? > > > > > > How can we have multiple RIDs in the same group and have only one > > > device in the group? > > > > Alex may help throw some insight here. Per what I read from the code > > looks like certain device can generate traffic with multiple RIDs. > > IIRC "dma alias" refers to things like legacy PCI to PCIe bridges that > do still have multiple PCI ID's behind the bridge used in > configuration cycles however the PCI to PCIe bridge will tag all PCIe > TLPs with its own RID because classic PCI has no way for the requestor > to convey a RID to the bridge. That is one scenario of dma aliasing. Another is like Alex replied where one device has an alias requestor ID due to PCI quirks. The alias RID may or may not map to a real device but probably what we really care here regarding to p2p are struct devices listed in the group. > > So, from a Linux perspective the group should have have multiple > struct devices behind the bridge, the bridge itself, and the RID the > IOMMU HW matches on is only the RID of the PCI bridge. > > But we know this because we know there is classic PCI stuff in the > heigharchy, so we can just mark that group as incompatible. Yes. > > > > 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. Thanks Kevin ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC v2 02/11] iommu: Add iommu_group_singleton_lockdown()
On Fri, Apr 01, 2022 at 02:20:23PM +0800, Yi Liu wrote: > > > On 2022/3/29 19:42, Jason Gunthorpe wrote: > > On Tue, Mar 29, 2022 at 08:42:13AM +, Tian, Kevin wrote: > > > > > btw I'm not sure whether this is what SVA requires. IIRC the problem with > > > SVA is because PASID TLP prefix is not counted in PCI packet routing thus > > > a DMA target address with PASID might be treated as P2P if the address > > > falls into the MMIO BAR of other devices in the group. This is why the > > > original code needs to strictly apply SVA in a group containing a single > > > device, instead of a group attached by a single driver, unless we want to > > > reserve those MMIO ranges in CPU VA space. > > > > I think it is not such a good idea to mix up group with this test > > > > Here you want to say that all TLPs from the RID route to the host > > bridge - ie ACS is on/etc. This is subtly different from a group with > > a single device. Specifically it is an immutable property of the > > fabric and doesn't change after hot plug events. > > so the group size can be immutable for specific topology. right? I think for > non-multi-function devices plugged behind an PCIE bridge which has enabled > ACS, such devices should have their own groups. Under such topology the > group size should be 1 constantly. May just enable SVA for such devices. Like I said, you should stop thinking about group size. You need to know that 100% of TLPs translate through the IOMMU to enable SVA, nothing less will do, and that property has nothing to do with group size. > > ie if we have a singleton group that doesn't have ACS and someone > > hotplugs in another device on a bridge, then our SVA is completely > > broken and we get data corruption. > > I think this may be a device plugged in a PCIE-to-PCI bridge, and then > hotplug a device to this bridge. The group size is variable. right? Per my > understanding, maybe such a bridge cannot support PASID Prefix at all, hence > no SVA support for such devices. Any PCIE-to-PCIE bridge will do, don't ned to involve legacy PCI here to have hotplug problems. Jason ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC v2 02/11] iommu: Add iommu_group_singleton_lockdown()
On 2022/3/29 19:42, Jason Gunthorpe wrote: On Tue, Mar 29, 2022 at 08:42:13AM +, Tian, Kevin wrote: btw I'm not sure whether this is what SVA requires. IIRC the problem with SVA is because PASID TLP prefix is not counted in PCI packet routing thus a DMA target address with PASID might be treated as P2P if the address falls into the MMIO BAR of other devices in the group. This is why the original code needs to strictly apply SVA in a group containing a single device, instead of a group attached by a single driver, unless we want to reserve those MMIO ranges in CPU VA space. I think it is not such a good idea to mix up group with this test Here you want to say that all TLPs from the RID route to the host bridge - ie ACS is on/etc. This is subtly different from a group with a single device. Specifically it is an immutable property of the fabric and doesn't change after hot plug events. so the group size can be immutable for specific topology. right? I think for non-multi-function devices plugged behind an PCIE bridge which has enabled ACS, such devices should have their own groups. Under such topology the group size should be 1 constantly. May just enable SVA for such devices. ie if we have a singleton group that doesn't have ACS and someone hotplugs in another device on a bridge, then our SVA is completely broken and we get data corruption. I think this may be a device plugged in a PCIE-to-PCI bridge, and then hotplug a device to this bridge. The group size is variable. right? Per my understanding, maybe such a bridge cannot support PASID Prefix at all, hence no SVA support for such devices. -- Regards, Yi Liu ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC v2 02/11] iommu: Add iommu_group_singleton_lockdown()
On 2022/3/29 16:42, Tian, Kevin wrote:
From: Lu Baolu
Sent: Tuesday, March 29, 2022 1:38 PM
Some of the interfaces in the IOMMU core require that only a single
kernel device driver controls the device in the IOMMU group. The
existing method is to check the device count in the IOMMU group in
the interfaces. This is unreliable because any device added to the
IOMMU group later breaks this assumption without notifying the driver
using the interface. This adds iommu_group_singleton_lockdown() that
checks the requirement and locks down the IOMMU group with only single
device driver bound.
Signed-off-by: Lu Baolu
---
drivers/iommu/iommu.c | 30 ++
1 file changed, 18 insertions(+), 12 deletions(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 0c42ece25854..9fb8a5b4491e 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -48,6 +48,7 @@ struct iommu_group {
struct list_head entry;
unsigned int owner_cnt;
void *owner;
+ bool singleton_lockdown;
};
struct group_device {
@@ -968,15 +969,16 @@ void iommu_group_remove_device(struct device
*dev)
}
EXPORT_SYMBOL_GPL(iommu_group_remove_device);
-static int iommu_group_device_count(struct iommu_group *group)
+/* Callers should hold the group->mutex. */
+static bool iommu_group_singleton_lockdown(struct iommu_group *group)
{
- struct group_device *entry;
- int ret = 0;
-
- list_for_each_entry(entry, >devices, list)
- ret++;
+ if (group->owner_cnt != 1 ||
+ group->domain != group->default_domain ||
+ group->owner)
+ return false;
Curious why there will be a case where group uses default_domain
while still having a owner? I have the impression that owner is used
for userspace DMA where a different domain is used.
+ group->singleton_lockdown = true;
- return ret;
+ return true;
}
btw I'm not sure whether this is what SVA requires. IIRC the problem with
SVA is because PASID TLP prefix is not counted in PCI packet routing thus
a DMA target address with PASID might be treated as P2P if the address
falls into the MMIO BAR of other devices in the group. This is why the
original code needs to strictly apply SVA in a group containing a single
device, instead of a group attached by a single driver, unless we want to
reserve those MMIO ranges in CPU VA space.
Jean can correct me if my memory is wrong.
I think so. I remember the major gap is PASID info is not used in the PCI's
address based TLP routing mechanism. So P2P may happen if the address
happens to be device MMIO. Per the commit message from Jean. Even for
singular groups, it's not an easy thing. So non-sigleton groups are not
considered so far.
"
IOMMU groups with more than one device aren't supported for SVA at the
moment. There may be P2P traffic between devices within a group, which
cannot be seen by an IOMMU (note that supporting PASID doesn't add any
form of isolation with regard to P2P). Supporting groups would require
calling bind() for all bound processes every time a device is added to a
group, to perform sanity checks (e.g. ensure that new devices support
PASIDs at least as big as those already allocated in the group). It also
means making sure that reserved ranges (IOMMU_RESV_*) of all devices are
carved out of processes. This is already tricky with single devices, but
becomes very difficult with groups. Since SVA-capable devices are expected
to be cleanly isolated, and since we don't have any way to test groups or
hot-plug, we only allow singular groups for now.
"
https://lore.kernel.org/kvm/20180511190641.23008-3-jean-philippe.bruc...@arm.com/
--
Regards,
Yi Liu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC v2 02/11] iommu: Add iommu_group_singleton_lockdown()
On Wed, 30 Mar 2022 14:18:47 + "Tian, Kevin" wrote: > +Alex > > > From: Tian, Kevin > > Sent: Wednesday, March 30, 2022 10:13 PM > > > > > From: Jason Gunthorpe > > > Sent: Wednesday, March 30, 2022 7:58 PM > > > > > > On Wed, Mar 30, 2022 at 06:50:11AM +, Tian, Kevin wrote: > > > > > > > One thing that I'm not very sure is about DMA alias. Even when > > > > physically > > > > there is only a single device within the group the aliasing could lead > > > > to multiple RIDs in the group making it non-singleton. But probably we > > > > don't need support SVA on such device until a real demand comes? > > > > > > How can we have multiple RIDs in the same group and have only one > > > device in the group? > > > > Alex may help throw some insight here. Per what I read from the code > > looks like certain device can generate traffic with multiple RIDs. You only need to look so far as the dma alias quirks to find devices that use the wrong RID for DMA. In general I don't think we have enough confidence to say that for all these devices the wrong RID is exclusively used versus some combination of both RIDs. Also, the aliased RID is not always a physical device. Thanks, Alex ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC v2 02/11] iommu: Add iommu_group_singleton_lockdown()
On Wed, Mar 30, 2022 at 02:12:57PM +, Tian, Kevin wrote: > > From: Jason Gunthorpe > > Sent: Wednesday, March 30, 2022 7:58 PM > > > > On Wed, Mar 30, 2022 at 06:50:11AM +, Tian, Kevin wrote: > > > > > One thing that I'm not very sure is about DMA alias. Even when physically > > > there is only a single device within the group the aliasing could lead > > > to multiple RIDs in the group making it non-singleton. But probably we > > > don't need support SVA on such device until a real demand comes? > > > > How can we have multiple RIDs in the same group and have only one > > device in the group? > > Alex may help throw some insight here. Per what I read from the code > looks like certain device can generate traffic with multiple RIDs. IIRC "dma alias" refers to things like legacy PCI to PCIe bridges that do still have multiple PCI ID's behind the bridge used in configuration cycles however the PCI to PCIe bridge will tag all PCIe TLPs with its own RID because classic PCI has no way for the requestor to convey a RID to the bridge. So, from a Linux perspective the group should have have multiple struct devices behind the bridge, the bridge itself, and the RID the IOMMU HW matches on is only the RID of the PCI bridge. But we know this because we know there is classic PCI stuff in the heigharchy, so we can just mark that group as incompatible. > > 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 Jason ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH RFC v2 02/11] iommu: Add iommu_group_singleton_lockdown()
+Alex > From: Tian, Kevin > Sent: Wednesday, March 30, 2022 10:13 PM > > > From: Jason Gunthorpe > > Sent: Wednesday, March 30, 2022 7:58 PM > > > > On Wed, Mar 30, 2022 at 06:50:11AM +, Tian, Kevin wrote: > > > > > One thing that I'm not very sure is about DMA alias. Even when physically > > > there is only a single device within the group the aliasing could lead > > > to multiple RIDs in the group making it non-singleton. But probably we > > > don't need support SVA on such device until a real demand comes? > > > > How can we have multiple RIDs in the same group and have only one > > device in the group? > > Alex may help throw some insight here. Per what I read from the code > looks like certain device can generate traffic with multiple RIDs. > > > > > > > ie if we have a singleton group that doesn't have ACS and someone > > > > hotplugs in another device on a bridge, then our SVA is completely > > > > broken and we get data corruption. > > > > > > Can we capture that in iommu_probe_device() when identifying > > > the group which the probed device will be added to has already been > > > locked down for SVA? i.e. make iommu_group_singleton_lockdown() > > > in this patch to lock down the fact of singleton group instead of > > > the fact of singleton driver... > > > > No, that is backwards > > > > > > Testing the group size is inherently the wrong test to make. > > > > > > What is your suggestion then? > > > > 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. > > Thanks > Kevin ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH RFC v2 02/11] iommu: Add iommu_group_singleton_lockdown()
> From: Jason Gunthorpe > Sent: Wednesday, March 30, 2022 7:58 PM > > On Wed, Mar 30, 2022 at 06:50:11AM +, Tian, Kevin wrote: > > > One thing that I'm not very sure is about DMA alias. Even when physically > > there is only a single device within the group the aliasing could lead > > to multiple RIDs in the group making it non-singleton. But probably we > > don't need support SVA on such device until a real demand comes? > > How can we have multiple RIDs in the same group and have only one > device in the group? Alex may help throw some insight here. Per what I read from the code looks like certain device can generate traffic with multiple RIDs. > > > > ie if we have a singleton group that doesn't have ACS and someone > > > hotplugs in another device on a bridge, then our SVA is completely > > > broken and we get data corruption. > > > > Can we capture that in iommu_probe_device() when identifying > > the group which the probed device will be added to has already been > > locked down for SVA? i.e. make iommu_group_singleton_lockdown() > > in this patch to lock down the fact of singleton group instead of > > the fact of singleton driver... > > No, that is backwards > > > > Testing the group size is inherently the wrong test to make. > > > > What is your suggestion then? > > 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. Thanks Kevin ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC v2 02/11] iommu: Add iommu_group_singleton_lockdown()
On Wed, Mar 30, 2022 at 06:50:11AM +, Tian, Kevin wrote: > One thing that I'm not very sure is about DMA alias. Even when physically > there is only a single device within the group the aliasing could lead > to multiple RIDs in the group making it non-singleton. But probably we > don't need support SVA on such device until a real demand comes? How can we have multiple RIDs in the same group and have only one device in the group? > > ie if we have a singleton group that doesn't have ACS and someone > > hotplugs in another device on a bridge, then our SVA is completely > > broken and we get data corruption. > > Can we capture that in iommu_probe_device() when identifying > the group which the probed device will be added to has already been > locked down for SVA? i.e. make iommu_group_singleton_lockdown() > in this patch to lock down the fact of singleton group instead of > the fact of singleton driver... No, that is backwards > > Testing the group size is inherently the wrong test to make. > > What is your suggestion then? 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. Jason ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC v2 02/11] iommu: Add iommu_group_singleton_lockdown()
Hi Kevin, On 2022/3/30 14:50, Tian, Kevin wrote: ie if we have a singleton group that doesn't have ACS and someone hotplugs in another device on a bridge, then our SVA is completely broken and we get data corruption. Can we capture that in iommu_probe_device() when identifying the group which the probed device will be added to has already been locked down for SVA? i.e. make iommu_group_singleton_lockdown() in this patch to lock down the fact of singleton group instead of the fact of singleton driver... The iommu_probe_device() is called in the bus notifier callback. It has no way to stop the probe of the device. Unless we could add a direct call in the device probe path of the driver core? Best regards, baolu ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH RFC v2 02/11] iommu: Add iommu_group_singleton_lockdown()
> From: Lu Baolu > Sent: Wednesday, March 30, 2022 1:00 PM > > > > btw I'm not sure whether this is what SVA requires. IIRC the problem with > > SVA is because PASID TLP prefix is not counted in PCI packet routing thus > > a DMA target address with PASID might be treated as P2P if the address > > falls into the MMIO BAR of other devices in the group. This is why the > > original code needs to strictly apply SVA in a group containing a single > > device, instead of a group attached by a single driver, unless we want to > > reserve those MMIO ranges in CPU VA space. > > You are right. But I don't think the IOMMU core is able to guarantee > above in a platform/device-agnostic way. Or any suggestions? > > I guess this should be somewhat off-loaded to the device driver which > knows details of the device. The device driver should know this and > guarantee it before calling > iommu_dev_enable_feature(dev, IOMMU_DEV_FEAT_SVA). How would the device driver know whether SVA requests from a device might be mis-interpreted as p2p by upstreaming ports? > > This patch itself just replaces the existing > "iommu_group_device_count(group) != 1" logic with a new one based on the > group ownership logistics. The former is obviously not friendly to > device hot joined afterward. > IMHO this replacement changes the semantics and device hotplug is something that we must deal with... Thanks Kevin ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH RFC v2 02/11] iommu: Add iommu_group_singleton_lockdown()
> From: Jason Gunthorpe > Sent: Tuesday, March 29, 2022 7:43 PM > > On Tue, Mar 29, 2022 at 08:42:13AM +, Tian, Kevin wrote: > > > btw I'm not sure whether this is what SVA requires. IIRC the problem with > > SVA is because PASID TLP prefix is not counted in PCI packet routing thus > > a DMA target address with PASID might be treated as P2P if the address > > falls into the MMIO BAR of other devices in the group. This is why the > > original code needs to strictly apply SVA in a group containing a single > > device, instead of a group attached by a single driver, unless we want to > > reserve those MMIO ranges in CPU VA space. > > I think it is not such a good idea to mix up group with this test > > Here you want to say that all TLPs from the RID route to the host > bridge - ie ACS is on/etc. This is subtly different from a group with > a single device. Specifically it is an immutable property of the > fabric and doesn't change after hot plug events. Yes, in concept your description is more accurate. However according to pci_device_group() a group is created mainly for lacking of ACS. Without ACS there is no guarantee that all TLPs from the RID in a multi-devices group are routed to the host bridge. In this case only singleton group can meet this requirement. One thing that I'm not very sure is about DMA alias. Even when physically there is only a single device within the group the aliasing could lead to multiple RIDs in the group making it non-singleton. But probably we don't need support SVA on such device until a real demand comes? > > ie if we have a singleton group that doesn't have ACS and someone > hotplugs in another device on a bridge, then our SVA is completely > broken and we get data corruption. Can we capture that in iommu_probe_device() when identifying the group which the probed device will be added to has already been locked down for SVA? i.e. make iommu_group_singleton_lockdown() in this patch to lock down the fact of singleton group instead of the fact of singleton driver... > > Testing the group size is inherently the wrong test to make. > What is your suggestion then? Thanks Kevin ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC v2 02/11] iommu: Add iommu_group_singleton_lockdown()
Hi Kevin,
On 2022/3/29 16:42, Tian, Kevin wrote:
From: Lu Baolu
Sent: Tuesday, March 29, 2022 1:38 PM
Some of the interfaces in the IOMMU core require that only a single
kernel device driver controls the device in the IOMMU group. The
existing method is to check the device count in the IOMMU group in
the interfaces. This is unreliable because any device added to the
IOMMU group later breaks this assumption without notifying the driver
using the interface. This adds iommu_group_singleton_lockdown() that
checks the requirement and locks down the IOMMU group with only single
device driver bound.
Signed-off-by: Lu Baolu
---
drivers/iommu/iommu.c | 30 ++
1 file changed, 18 insertions(+), 12 deletions(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 0c42ece25854..9fb8a5b4491e 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -48,6 +48,7 @@ struct iommu_group {
struct list_head entry;
unsigned int owner_cnt;
void *owner;
+ bool singleton_lockdown;
};
struct group_device {
@@ -968,15 +969,16 @@ void iommu_group_remove_device(struct device
*dev)
}
EXPORT_SYMBOL_GPL(iommu_group_remove_device);
-static int iommu_group_device_count(struct iommu_group *group)
+/* Callers should hold the group->mutex. */
+static bool iommu_group_singleton_lockdown(struct iommu_group *group)
{
- struct group_device *entry;
- int ret = 0;
-
- list_for_each_entry(entry, >devices, list)
- ret++;
+ if (group->owner_cnt != 1 ||
+ group->domain != group->default_domain ||
+ group->owner)
+ return false;
Curious why there will be a case where group uses default_domain
while still having a owner? I have the impression that owner is used
for userspace DMA where a different domain is used.
You are right. The default domain is automatically detached when a user
is claimed. As long as a user is claimed, the group could only use an
empty or user-specified domain.
+ group->singleton_lockdown = true;
- return ret;
+ return true;
}
btw I'm not sure whether this is what SVA requires. IIRC the problem with
SVA is because PASID TLP prefix is not counted in PCI packet routing thus
a DMA target address with PASID might be treated as P2P if the address
falls into the MMIO BAR of other devices in the group. This is why the
original code needs to strictly apply SVA in a group containing a single
device, instead of a group attached by a single driver, unless we want to
reserve those MMIO ranges in CPU VA space.
You are right. But I don't think the IOMMU core is able to guarantee
above in a platform/device-agnostic way. Or any suggestions?
I guess this should be somewhat off-loaded to the device driver which
knows details of the device. The device driver should know this and
guarantee it before calling
iommu_dev_enable_feature(dev, IOMMU_DEV_FEAT_SVA).
This patch itself just replaces the existing
"iommu_group_device_count(group) != 1" logic with a new one based on the
group ownership logistics. The former is obviously not friendly to
device hot joined afterward.
Jean can correct me if my memory is wrong.
Thanks
Kevin
Best regards,
baolu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC v2 02/11] iommu: Add iommu_group_singleton_lockdown()
On Tue, Mar 29, 2022 at 08:42:13AM +, Tian, Kevin wrote: > btw I'm not sure whether this is what SVA requires. IIRC the problem with > SVA is because PASID TLP prefix is not counted in PCI packet routing thus > a DMA target address with PASID might be treated as P2P if the address > falls into the MMIO BAR of other devices in the group. This is why the > original code needs to strictly apply SVA in a group containing a single > device, instead of a group attached by a single driver, unless we want to > reserve those MMIO ranges in CPU VA space. I think it is not such a good idea to mix up group with this test Here you want to say that all TLPs from the RID route to the host bridge - ie ACS is on/etc. This is subtly different from a group with a single device. Specifically it is an immutable property of the fabric and doesn't change after hot plug events. ie if we have a singleton group that doesn't have ACS and someone hotplugs in another device on a bridge, then our SVA is completely broken and we get data corruption. Testing the group size is inherently the wrong test to make. Jason ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH RFC v2 02/11] iommu: Add iommu_group_singleton_lockdown()
> From: Lu Baolu
> Sent: Tuesday, March 29, 2022 1:38 PM
>
> Some of the interfaces in the IOMMU core require that only a single
> kernel device driver controls the device in the IOMMU group. The
> existing method is to check the device count in the IOMMU group in
> the interfaces. This is unreliable because any device added to the
> IOMMU group later breaks this assumption without notifying the driver
> using the interface. This adds iommu_group_singleton_lockdown() that
> checks the requirement and locks down the IOMMU group with only single
> device driver bound.
>
> Signed-off-by: Lu Baolu
> ---
> drivers/iommu/iommu.c | 30 ++
> 1 file changed, 18 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 0c42ece25854..9fb8a5b4491e 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -48,6 +48,7 @@ struct iommu_group {
> struct list_head entry;
> unsigned int owner_cnt;
> void *owner;
> + bool singleton_lockdown;
> };
>
> struct group_device {
> @@ -968,15 +969,16 @@ void iommu_group_remove_device(struct device
> *dev)
> }
> EXPORT_SYMBOL_GPL(iommu_group_remove_device);
>
> -static int iommu_group_device_count(struct iommu_group *group)
> +/* Callers should hold the group->mutex. */
> +static bool iommu_group_singleton_lockdown(struct iommu_group *group)
> {
> - struct group_device *entry;
> - int ret = 0;
> -
> - list_for_each_entry(entry, >devices, list)
> - ret++;
> + if (group->owner_cnt != 1 ||
> + group->domain != group->default_domain ||
> + group->owner)
> + return false;
Curious why there will be a case where group uses default_domain
while still having a owner? I have the impression that owner is used
for userspace DMA where a different domain is used.
> + group->singleton_lockdown = true;
>
> - return ret;
> + return true;
> }
btw I'm not sure whether this is what SVA requires. IIRC the problem with
SVA is because PASID TLP prefix is not counted in PCI packet routing thus
a DMA target address with PASID might be treated as P2P if the address
falls into the MMIO BAR of other devices in the group. This is why the
original code needs to strictly apply SVA in a group containing a single
device, instead of a group attached by a single driver, unless we want to
reserve those MMIO ranges in CPU VA space.
Jean can correct me if my memory is wrong.
Thanks
Kevin
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH RFC v2 02/11] iommu: Add iommu_group_singleton_lockdown()
Some of the interfaces in the IOMMU core require that only a single
kernel device driver controls the device in the IOMMU group. The
existing method is to check the device count in the IOMMU group in
the interfaces. This is unreliable because any device added to the
IOMMU group later breaks this assumption without notifying the driver
using the interface. This adds iommu_group_singleton_lockdown() that
checks the requirement and locks down the IOMMU group with only single
device driver bound.
Signed-off-by: Lu Baolu
---
drivers/iommu/iommu.c | 30 ++
1 file changed, 18 insertions(+), 12 deletions(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 0c42ece25854..9fb8a5b4491e 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -48,6 +48,7 @@ struct iommu_group {
struct list_head entry;
unsigned int owner_cnt;
void *owner;
+ bool singleton_lockdown;
};
struct group_device {
@@ -968,15 +969,16 @@ void iommu_group_remove_device(struct device *dev)
}
EXPORT_SYMBOL_GPL(iommu_group_remove_device);
-static int iommu_group_device_count(struct iommu_group *group)
+/* Callers should hold the group->mutex. */
+static bool iommu_group_singleton_lockdown(struct iommu_group *group)
{
- struct group_device *entry;
- int ret = 0;
-
- list_for_each_entry(entry, >devices, list)
- ret++;
+ if (group->owner_cnt != 1 ||
+ group->domain != group->default_domain ||
+ group->owner)
+ return false;
+ group->singleton_lockdown = true;
- return ret;
+ return true;
}
static int __iommu_group_for_each_dev(struct iommu_group *group, void *data,
@@ -1936,7 +1938,7 @@ int iommu_attach_device(struct iommu_domain *domain,
struct device *dev)
*/
mutex_lock(>mutex);
ret = -EINVAL;
- if (iommu_group_device_count(group) != 1)
+ if (!iommu_group_singleton_lockdown(group))
goto out_unlock;
ret = __iommu_attach_group(domain, group);
@@ -1979,7 +1981,7 @@ void iommu_detach_device(struct iommu_domain *domain,
struct device *dev)
return;
mutex_lock(>mutex);
- if (iommu_group_device_count(group) != 1) {
+ if (!iommu_group_singleton_lockdown(group)) {
WARN_ON(1);
goto out_unlock;
}
@@ -2745,7 +2747,7 @@ iommu_sva_bind_device(struct device *dev, struct
mm_struct *mm, void *drvdata)
* affected by the problems that required IOMMU groups (lack of ACS
* isolation, device ID aliasing and other hardware issues).
*/
- if (iommu_group_device_count(group) != 1)
+ if (!iommu_group_singleton_lockdown(group))
goto out_unlock;
handle = ops->sva_bind(dev, mm, drvdata);
@@ -2842,7 +2844,7 @@ static int iommu_change_dev_def_domain(struct iommu_group
*group,
* case) that has already acquired some of the device locks and might be
* waiting for T1 to release other device locks.
*/
- if (iommu_group_device_count(group) != 1) {
+ if (!iommu_group_singleton_lockdown(group)) {
dev_err_ratelimited(prev_dev, "Cannot change default domain:
Group has more than one device\n");
ret = -EINVAL;
goto out;
@@ -2975,7 +2977,7 @@ static ssize_t iommu_group_store_type(struct iommu_group
*group,
* 2. Get struct *dev which is needed to lock device
*/
mutex_lock(>mutex);
- if (iommu_group_device_count(group) != 1) {
+ if (!iommu_group_singleton_lockdown(group)) {
mutex_unlock(>mutex);
pr_err_ratelimited("Cannot change default domain: Group has
more than one device\n");
return -EINVAL;
@@ -3050,6 +3052,7 @@ int iommu_device_use_default_domain(struct device *dev)
mutex_lock(>mutex);
if (group->owner_cnt) {
if (group->domain != group->default_domain ||
+ group->singleton_lockdown ||
group->owner) {
ret = -EBUSY;
goto unlock_out;
@@ -3084,6 +3087,9 @@ void iommu_device_unuse_default_domain(struct device *dev)
if (!WARN_ON(!group->owner_cnt))
group->owner_cnt--;
+ if (!group->owner_cnt)
+ group->singleton_lockdown = false;
+
mutex_unlock(>mutex);
iommu_group_put(group);
}
--
2.25.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
