Re: [PATCH v2 3/5] vfio/iommu_type1: Remove the domain->ops comparison

2022-06-24 Thread Jason Gunthorpe via iommu
On Fri, Jun 24, 2022 at 07:31:47PM +0100, Robin Murphy wrote:

> > > Oh, physical platforms with mixed IOMMUs definitely exist already. The 
> > > main
> > > point is that while bus_set_iommu still exists, the core code effectively
> > > *does* prevent multiple drivers from registering - even in emulated cases
> > > like the example above, virtio-iommu and VT-d would both try to
> > > bus_set_iommu(&pci_bus_type), and one of them will lose. The aspect which
> > > might warrant clarification is that there's no combination of supported
> > > drivers which claim non-overlapping buses *and* could appear in the same
> > > system - even if you tried to contrive something by emulating, say, VT-d
> > > (PCI) alongside rockchip-iommu (platform), you could still only describe 
> > > one
> > > or the other due to ACPI vs. Devicetree.
> > 
> > Right, and that is still something we need to protect against with
> > this ops check. VFIO is not checking that the bus's are the same
> > before attempting to re-use a domain.
> > 
> > So it is actually functional and does protect against systems with
> > multiple iommu drivers on different busses.
> 
> But as above, which systems *are* those? 

IDK it seems wrong that the system today will allow different buses to
have different IOMMU drivers and not provide a trivial protection
check.

> FWIW my iommu/bus dev branch has got as far as the final bus ops removal and
> allowing multiple driver registrations, and before it allows that, it does
> now have the common attach check that I sketched out in the previous
> discussion of this.

If you want to put the check in your series that seems fine too, as
long as we get it in the end.

> It's probably also noteworthy that domain->ops is no longer the same
> domain->ops that this code was written to check, and may now be different
> between domains from the same driver.

Yes, the vfio check is not good anymore.

Jason
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH v2 3/5] vfio/iommu_type1: Remove the domain->ops comparison

2022-06-24 Thread Robin Murphy

On 2022-06-24 14:16, Jason Gunthorpe wrote:

On Wed, Jun 22, 2022 at 08:54:45AM +0100, Robin Murphy wrote:

On 2022-06-16 23:23, Nicolin Chen wrote:

On Thu, Jun 16, 2022 at 06:40:14AM +, Tian, Kevin wrote:


The domain->ops validation was added, as a precaution, for mixed-driver
systems. However, at this moment only one iommu driver is possible. So
remove it.


It's true on a physical platform. But I'm not sure whether a virtual platform
is allowed to include multiple e.g. one virtio-iommu alongside a virtual VT-d
or a virtual smmu. It might be clearer to claim that (as Robin pointed out)
there is plenty more significant problems than this to solve instead of simply
saying that only one iommu driver is possible if we don't have explicit code
to reject such configuration. 😊


Will edit this part. Thanks!


Oh, physical platforms with mixed IOMMUs definitely exist already. The main
point is that while bus_set_iommu still exists, the core code effectively
*does* prevent multiple drivers from registering - even in emulated cases
like the example above, virtio-iommu and VT-d would both try to
bus_set_iommu(&pci_bus_type), and one of them will lose. The aspect which
might warrant clarification is that there's no combination of supported
drivers which claim non-overlapping buses *and* could appear in the same
system - even if you tried to contrive something by emulating, say, VT-d
(PCI) alongside rockchip-iommu (platform), you could still only describe one
or the other due to ACPI vs. Devicetree.


Right, and that is still something we need to protect against with
this ops check. VFIO is not checking that the bus's are the same
before attempting to re-use a domain.

So it is actually functional and does protect against systems with
multiple iommu drivers on different busses.


But as above, which systems *are* those? Everything that's on my radar 
would have drivers all competing for the platform bus - Intel and s390 
are somewhat the odd ones out in that respect, but are also non-issues 
as above. FWIW my iommu/bus dev branch has got as far as the final bus 
ops removal and allowing multiple driver registrations, and before it 
allows that, it does now have the common attach check that I sketched 
out in the previous discussion of this.


It's probably also noteworthy that domain->ops is no longer the same 
domain->ops that this code was written to check, and may now be 
different between domains from the same driver.


Thanks,
Robin.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 3/5] vfio/iommu_type1: Remove the domain->ops comparison

2022-06-24 Thread Jason Gunthorpe via iommu
On Wed, Jun 22, 2022 at 08:54:45AM +0100, Robin Murphy wrote:
> On 2022-06-16 23:23, Nicolin Chen wrote:
> > On Thu, Jun 16, 2022 at 06:40:14AM +, Tian, Kevin wrote:
> > 
> > > > The domain->ops validation was added, as a precaution, for mixed-driver
> > > > systems. However, at this moment only one iommu driver is possible. So
> > > > remove it.
> > > 
> > > It's true on a physical platform. But I'm not sure whether a virtual 
> > > platform
> > > is allowed to include multiple e.g. one virtio-iommu alongside a virtual 
> > > VT-d
> > > or a virtual smmu. It might be clearer to claim that (as Robin pointed 
> > > out)
> > > there is plenty more significant problems than this to solve instead of 
> > > simply
> > > saying that only one iommu driver is possible if we don't have explicit 
> > > code
> > > to reject such configuration. 😊
> > 
> > Will edit this part. Thanks!
> 
> Oh, physical platforms with mixed IOMMUs definitely exist already. The main
> point is that while bus_set_iommu still exists, the core code effectively
> *does* prevent multiple drivers from registering - even in emulated cases
> like the example above, virtio-iommu and VT-d would both try to
> bus_set_iommu(&pci_bus_type), and one of them will lose. The aspect which
> might warrant clarification is that there's no combination of supported
> drivers which claim non-overlapping buses *and* could appear in the same
> system - even if you tried to contrive something by emulating, say, VT-d
> (PCI) alongside rockchip-iommu (platform), you could still only describe one
> or the other due to ACPI vs. Devicetree.

Right, and that is still something we need to protect against with
this ops check. VFIO is not checking that the bus's are the same
before attempting to re-use a domain.

So it is actually functional and does protect against systems with
multiple iommu drivers on different busses.

Jason
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 3/5] vfio/iommu_type1: Remove the domain->ops comparison

2022-06-23 Thread Nicolin Chen via iommu
On Thu, Jun 23, 2022 at 03:50:22AM +, Tian, Kevin wrote:
> External email: Use caution opening links or attachments
> 
> 
> > From: Robin Murphy 
> > Sent: Wednesday, June 22, 2022 3:55 PM
> >
> > On 2022-06-16 23:23, Nicolin Chen wrote:
> > > On Thu, Jun 16, 2022 at 06:40:14AM +, Tian, Kevin wrote:
> > >
> > >>> The domain->ops validation was added, as a precaution, for mixed-
> > driver
> > >>> systems. However, at this moment only one iommu driver is possible. So
> > >>> remove it.
> > >>
> > >> It's true on a physical platform. But I'm not sure whether a virtual
> > platform
> > >> is allowed to include multiple e.g. one virtio-iommu alongside a virtual 
> > >> VT-
> > d
> > >> or a virtual smmu. It might be clearer to claim that (as Robin pointed 
> > >> out)
> > >> there is plenty more significant problems than this to solve instead of
> > simply
> > >> saying that only one iommu driver is possible if we don't have explicit
> > code
> > >> to reject such configuration. 😊
> > >
> > > Will edit this part. Thanks!
> >
> > Oh, physical platforms with mixed IOMMUs definitely exist already. The
> > main point is that while bus_set_iommu still exists, the core code
> > effectively *does* prevent multiple drivers from registering - even in
> > emulated cases like the example above, virtio-iommu and VT-d would both
> > try to bus_set_iommu(&pci_bus_type), and one of them will lose. The
> > aspect which might warrant clarification is that there's no combination
> > of supported drivers which claim non-overlapping buses *and* could
> > appear in the same system - even if you tried to contrive something by
> > emulating, say, VT-d (PCI) alongside rockchip-iommu (platform), you
> > could still only describe one or the other due to ACPI vs. Devicetree.
> >
> 
> This explanation is much clearer! thanks.

Thanks +1

I've also updated the commit log.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH v2 3/5] vfio/iommu_type1: Remove the domain->ops comparison

2022-06-22 Thread Tian, Kevin
> From: Robin Murphy 
> Sent: Wednesday, June 22, 2022 3:55 PM
> 
> On 2022-06-16 23:23, Nicolin Chen wrote:
> > On Thu, Jun 16, 2022 at 06:40:14AM +, Tian, Kevin wrote:
> >
> >>> The domain->ops validation was added, as a precaution, for mixed-
> driver
> >>> systems. However, at this moment only one iommu driver is possible. So
> >>> remove it.
> >>
> >> It's true on a physical platform. But I'm not sure whether a virtual
> platform
> >> is allowed to include multiple e.g. one virtio-iommu alongside a virtual 
> >> VT-
> d
> >> or a virtual smmu. It might be clearer to claim that (as Robin pointed out)
> >> there is plenty more significant problems than this to solve instead of
> simply
> >> saying that only one iommu driver is possible if we don't have explicit
> code
> >> to reject such configuration. 😊
> >
> > Will edit this part. Thanks!
> 
> Oh, physical platforms with mixed IOMMUs definitely exist already. The
> main point is that while bus_set_iommu still exists, the core code
> effectively *does* prevent multiple drivers from registering - even in
> emulated cases like the example above, virtio-iommu and VT-d would both
> try to bus_set_iommu(&pci_bus_type), and one of them will lose. The
> aspect which might warrant clarification is that there's no combination
> of supported drivers which claim non-overlapping buses *and* could
> appear in the same system - even if you tried to contrive something by
> emulating, say, VT-d (PCI) alongside rockchip-iommu (platform), you
> could still only describe one or the other due to ACPI vs. Devicetree.
> 

This explanation is much clearer! thanks.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 3/5] vfio/iommu_type1: Remove the domain->ops comparison

2022-06-22 Thread Robin Murphy

On 2022-06-16 23:23, Nicolin Chen wrote:

On Thu, Jun 16, 2022 at 06:40:14AM +, Tian, Kevin wrote:


The domain->ops validation was added, as a precaution, for mixed-driver
systems. However, at this moment only one iommu driver is possible. So
remove it.


It's true on a physical platform. But I'm not sure whether a virtual platform
is allowed to include multiple e.g. one virtio-iommu alongside a virtual VT-d
or a virtual smmu. It might be clearer to claim that (as Robin pointed out)
there is plenty more significant problems than this to solve instead of simply
saying that only one iommu driver is possible if we don't have explicit code
to reject such configuration. 😊


Will edit this part. Thanks!


Oh, physical platforms with mixed IOMMUs definitely exist already. The 
main point is that while bus_set_iommu still exists, the core code 
effectively *does* prevent multiple drivers from registering - even in 
emulated cases like the example above, virtio-iommu and VT-d would both 
try to bus_set_iommu(&pci_bus_type), and one of them will lose. The 
aspect which might warrant clarification is that there's no combination 
of supported drivers which claim non-overlapping buses *and* could 
appear in the same system - even if you tried to contrive something by 
emulating, say, VT-d (PCI) alongside rockchip-iommu (platform), you 
could still only describe one or the other due to ACPI vs. Devicetree.


Thanks,
Robin.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 3/5] vfio/iommu_type1: Remove the domain->ops comparison

2022-06-16 Thread Nicolin Chen via iommu
On Thu, Jun 16, 2022 at 06:40:14AM +, Tian, Kevin wrote:

> > The domain->ops validation was added, as a precaution, for mixed-driver
> > systems. However, at this moment only one iommu driver is possible. So
> > remove it.
> 
> It's true on a physical platform. But I'm not sure whether a virtual platform
> is allowed to include multiple e.g. one virtio-iommu alongside a virtual VT-d
> or a virtual smmu. It might be clearer to claim that (as Robin pointed out)
> there is plenty more significant problems than this to solve instead of simply
> saying that only one iommu driver is possible if we don't have explicit code
> to reject such configuration. 😊

Will edit this part. Thanks!
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH v2 3/5] vfio/iommu_type1: Remove the domain->ops comparison

2022-06-15 Thread Tian, Kevin
> From: Nicolin Chen 
> Sent: Thursday, June 16, 2022 8:03 AM
> 
> The domain->ops validation was added, as a precaution, for mixed-driver
> systems. However, at this moment only one iommu driver is possible. So
> remove it.

It's true on a physical platform. But I'm not sure whether a virtual platform
is allowed to include multiple e.g. one virtio-iommu alongside a virtual VT-d
or a virtual smmu. It might be clearer to claim that (as Robin pointed out)
there is plenty more significant problems than this to solve instead of simply
saying that only one iommu driver is possible if we don't have explicit code
to reject such configuration. 😊

> 
> Per discussion with Robin, in future when many can be permitted we will
> rely on the IOMMU core code to check the domain->ops:
> https://lore.kernel.org/linux-iommu/6575de6d-94ba-c427-5b1e-
> 967750ddf...@arm.com/
> 
> Signed-off-by: Nicolin Chen 

Apart from that,

Reviewed-by: Kevin Tian 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v2 3/5] vfio/iommu_type1: Remove the domain->ops comparison

2022-06-15 Thread Nicolin Chen via iommu
The domain->ops validation was added, as a precaution, for mixed-driver
systems. However, at this moment only one iommu driver is possible. So
remove it.

Per discussion with Robin, in future when many can be permitted we will
rely on the IOMMU core code to check the domain->ops:
https://lore.kernel.org/linux-iommu/6575de6d-94ba-c427-5b1e-967750ddf...@arm.com/

Signed-off-by: Nicolin Chen 
---
 drivers/vfio/vfio_iommu_type1.c | 32 +++-
 1 file changed, 11 insertions(+), 21 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index f4e3b423a453..11be5f95580b 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -2277,29 +2277,19 @@ static int vfio_iommu_type1_attach_group(void 
*iommu_data,
domain->domain->ops->enforce_cache_coherency(
domain->domain);
 
-   /*
-* Try to match an existing compatible domain.  We don't want to
-* preclude an IOMMU driver supporting multiple bus_types and being
-* able to include different bus_types in the same IOMMU domain, so
-* we test whether the domains use the same iommu_ops rather than
-* testing if they're on the same bus_type.
-*/
+   /* Try to match an existing compatible domain */
list_for_each_entry(d, &iommu->domain_list, next) {
-   if (d->domain->ops == domain->domain->ops) {
-   iommu_detach_group(domain->domain, group->iommu_group);
-   if (!iommu_attach_group(d->domain,
-   group->iommu_group)) {
-   list_add(&group->next, &d->group_list);
-   iommu_domain_free(domain->domain);
-   kfree(domain);
-   goto done;
-   }
-
-   ret = iommu_attach_group(domain->domain,
-group->iommu_group);
-   if (ret)
-   goto out_domain;
+   iommu_detach_group(domain->domain, group->iommu_group);
+   if (!iommu_attach_group(d->domain, group->iommu_group)) {
+   list_add(&group->next, &d->group_list);
+   iommu_domain_free(domain->domain);
+   kfree(domain);
+   goto done;
}
+
+   ret = iommu_attach_group(domain->domain,  group->iommu_group);
+   if (ret)
+   goto out_domain;
}
 
vfio_test_domain_fgsp(domain);
-- 
2.17.1

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu