| Message ID | 5b9b608af21b3c4353af042355973bac55397962.1657034828.git.robin.murphy@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iommu: Retire bus_set_iommu() | expand |
On 2022/7/6 01:08, Robin Murphy wrote: > @@ -202,12 +210,32 @@ int iommu_device_register(struct iommu_device *iommu, > spin_lock(&iommu_device_lock); > list_add_tail(&iommu->list, &iommu_device_list); > spin_unlock(&iommu_device_lock); > + > + for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++) { > + struct bus_type *bus = iommu_buses[i]; > + int err; > + > + if (bus->iommu_ops && bus->iommu_ops != ops) { > + err = -EBUSY; > + } else { > + bus->iommu_ops = ops; > + err = bus_iommu_probe(bus); > + } > + if (err) { > + iommu_device_unregister(iommu); > + return err; > + } > + } > + > return 0; > } > EXPORT_SYMBOL_GPL(iommu_device_register); With bus_set_iommu() retired, my understanding is that now we embrace the first-come-first-serve policy for bus->iommu_ops setting. This will lead to problem in different iommu_ops for different bus case. Did I overlook anything? Best regards, baolu
On 2022-07-06 03:35, Baolu Lu wrote: > On 2022/7/6 01:08, Robin Murphy wrote: >> @@ -202,12 +210,32 @@ int iommu_device_register(struct iommu_device >> *iommu, >> spin_lock(&iommu_device_lock); >> list_add_tail(&iommu->list, &iommu_device_list); >> spin_unlock(&iommu_device_lock); >> + >> + for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++) { >> + struct bus_type *bus = iommu_buses[i]; >> + int err; >> + >> + if (bus->iommu_ops && bus->iommu_ops != ops) { >> + err = -EBUSY; >> + } else { >> + bus->iommu_ops = ops; >> + err = bus_iommu_probe(bus); >> + } >> + if (err) { >> + iommu_device_unregister(iommu); >> + return err; >> + } >> + } >> + >> return 0; >> } >> EXPORT_SYMBOL_GPL(iommu_device_register); > > With bus_set_iommu() retired, my understanding is that now we embrace > the first-come-first-serve policy for bus->iommu_ops setting. This will > lead to problem in different iommu_ops for different bus case. Did I > overlook anything? This is just formalising the de-facto situation that we don't actually have any combination of drivers that could load on the same system without already attempting to claim at least one bus in common. It's also only temporary until the bus ops are removed completely and we fully support multiple drivers coexisting, which only actually takes a handful more patches - I've realised I could even bring that change *ahead* of the big job of converting iommu_domain_alloc() (I'm not convinced that the tree-wide flag-day patch for that I currently have in the dev branch is really viable, nor that I've actually got the correct device at some of the callsites), although whether it's worth the potentially-surprising behaviour that might result I'm less sure. If we already had systems where in-tree drivers successfully coexisted on different buses then I'd have split this up and done something a bit more involved to keep a vestigial bus_set_iommu() around until the final bus ops removal, but since we don't, it seemed neatest to do all the related work in one go. Thanks, Robin.
On 2022/7/6 22:37, Robin Murphy wrote: > On 2022-07-06 03:35, Baolu Lu wrote: >> On 2022/7/6 01:08, Robin Murphy wrote: >>> @@ -202,12 +210,32 @@ int iommu_device_register(struct iommu_device >>> *iommu, >>> spin_lock(&iommu_device_lock); >>> list_add_tail(&iommu->list, &iommu_device_list); >>> spin_unlock(&iommu_device_lock); >>> + >>> + for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++) { >>> + struct bus_type *bus = iommu_buses[i]; >>> + int err; >>> + >>> + if (bus->iommu_ops && bus->iommu_ops != ops) { >>> + err = -EBUSY; >>> + } else { >>> + bus->iommu_ops = ops; >>> + err = bus_iommu_probe(bus); >>> + } >>> + if (err) { >>> + iommu_device_unregister(iommu); >>> + return err; >>> + } >>> + } >>> + >>> return 0; >>> } >>> EXPORT_SYMBOL_GPL(iommu_device_register); >> >> With bus_set_iommu() retired, my understanding is that now we embrace >> the first-come-first-serve policy for bus->iommu_ops setting. This will >> lead to problem in different iommu_ops for different bus case. Did I >> overlook anything? > > This is just formalising the de-facto situation that we don't actually > have any combination of drivers that could load on the same system > without already attempting to claim at least one bus in common. It's > also only temporary until the bus ops are removed completely and we > fully support multiple drivers coexisting, which only actually takes a > handful more patches - I've realised I could even bring that change > *ahead* of the big job of converting iommu_domain_alloc() (I'm not > convinced that the tree-wide flag-day patch for that I currently have in > the dev branch is really viable, nor that I've actually got the correct > device at some of the callsites), although whether it's worth the > potentially-surprising behaviour that might result I'm less sure. > > If we already had systems where in-tree drivers successfully coexisted > on different buses then I'd have split this up and done something a bit > more involved to keep a vestigial bus_set_iommu() around until the final > bus ops removal, but since we don't, it seemed neatest to do all the > related work in one go. Fair enough. I've never seen a mixed system as far. It's fine for us to retire bus_set_iommu() for now and then formally support mixed IOMMU drivers later. Best regards, baolu
> From: Robin Murphy <robin.murphy@arm.com> > Sent: Wednesday, July 6, 2022 1:08 AM > > @@ -202,12 +210,32 @@ int iommu_device_register(struct iommu_device > *iommu, > spin_lock(&iommu_device_lock); > list_add_tail(&iommu->list, &iommu_device_list); > spin_unlock(&iommu_device_lock); > + > + for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++) { > + struct bus_type *bus = iommu_buses[i]; > + int err; > + > + if (bus->iommu_ops && bus->iommu_ops != ops) { > + err = -EBUSY; > + } else { > + bus->iommu_ops = ops; > + err = bus_iommu_probe(bus); > + } > + if (err) { > + iommu_device_unregister(iommu); > + return err; > + } > + } > + Probably move above into a new function bus_iommu_probe_all(): /* probe all buses for devices associated with this iommu */ err = bus_iommu_probe_all(); if (err) { iommu_device_unregister(iommu); return err; } Just my personal preference on leaving logic in iommu_device_register() more relevant to the iommu instance itself. Apart from that: Reviewed-by: Kevin Tian <kevin.tian@intel.com>
On 2022-07-07 07:51, Tian, Kevin wrote: >> From: Robin Murphy <robin.murphy@arm.com> >> Sent: Wednesday, July 6, 2022 1:08 AM >> >> @@ -202,12 +210,32 @@ int iommu_device_register(struct iommu_device >> *iommu, >> spin_lock(&iommu_device_lock); >> list_add_tail(&iommu->list, &iommu_device_list); >> spin_unlock(&iommu_device_lock); >> + >> + for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++) { >> + struct bus_type *bus = iommu_buses[i]; >> + int err; >> + >> + if (bus->iommu_ops && bus->iommu_ops != ops) { >> + err = -EBUSY; >> + } else { >> + bus->iommu_ops = ops; >> + err = bus_iommu_probe(bus); >> + } >> + if (err) { >> + iommu_device_unregister(iommu); >> + return err; >> + } >> + } >> + > > Probably move above into a new function bus_iommu_probe_all(): > > /* probe all buses for devices associated with this iommu */ > err = bus_iommu_probe_all(); > if (err) { > iommu_device_unregister(iommu); > return err; > } > > Just my personal preference on leaving logic in iommu_device_register() > more relevant to the iommu instance itself. On reflection I think it makes sense to pull the iommu_device_unregister() out of the loop anyway - I think that's really a left-over from between v1 and v2 when that error case briefly jumped to another cleanup loop, before I realised it was actually trivial for iommu_device_unregister() to clean up for itself. However I now see I've also missed another opportunity, and the -EBUSY case should be hoisted out of the loop as well, since checking iommu_buses[0] is sufficient. Then it's hopefully much clearer that once the bus ops go away we'll be left with just a single extra line for the loop, as in iommu_device_unregister(). Does that sound reasonable? > Apart from that: > > Reviewed-by: Kevin Tian <kevin.tian@intel.com> Thanks! (and for the others as well) Robin.
> From: Robin Murphy <robin.murphy@arm.com> > Sent: Thursday, July 7, 2022 6:58 PM > > On 2022-07-07 07:51, Tian, Kevin wrote: > >> From: Robin Murphy <robin.murphy@arm.com> > >> Sent: Wednesday, July 6, 2022 1:08 AM > >> > >> @@ -202,12 +210,32 @@ int iommu_device_register(struct > iommu_device > >> *iommu, > >> spin_lock(&iommu_device_lock); > >> list_add_tail(&iommu->list, &iommu_device_list); > >> spin_unlock(&iommu_device_lock); > >> + > >> + for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++) { > >> + struct bus_type *bus = iommu_buses[i]; > >> + int err; > >> + > >> + if (bus->iommu_ops && bus->iommu_ops != ops) { > >> + err = -EBUSY; > >> + } else { > >> + bus->iommu_ops = ops; > >> + err = bus_iommu_probe(bus); > >> + } > >> + if (err) { > >> + iommu_device_unregister(iommu); > >> + return err; > >> + } > >> + } > >> + > > > > Probably move above into a new function bus_iommu_probe_all(): > > > > /* probe all buses for devices associated with this iommu */ > > err = bus_iommu_probe_all(); > > if (err) { > > iommu_device_unregister(iommu); > > return err; > > } > > > > Just my personal preference on leaving logic in iommu_device_register() > > more relevant to the iommu instance itself. > > On reflection I think it makes sense to pull the > iommu_device_unregister() out of the loop anyway - I think that's really > a left-over from between v1 and v2 when that error case briefly jumped > to another cleanup loop, before I realised it was actually trivial for > iommu_device_unregister() to clean up for itself. > > However I now see I've also missed another opportunity, and the -EBUSY > case should be hoisted out of the loop as well, since checking > iommu_buses[0] is sufficient. Then it's hopefully much clearer that once > the bus ops go away we'll be left with just a single extra line for the > loop, as in iommu_device_unregister(). Does that sound reasonable? > Yes, sounds good to me.
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index 514edc0eaa94..acb7b2ab0b79 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -180,6 +180,14 @@ static int __init iommu_subsys_init(void) } subsys_initcall(iommu_subsys_init); +static int remove_iommu_group(struct device *dev, void *data) +{ + if (dev->iommu && dev->iommu->iommu_dev == data) + iommu_release_device(dev); + + return 0; +} + /** * iommu_device_register() - Register an IOMMU hardware instance * @iommu: IOMMU handle for the instance @@ -202,12 +210,32 @@ int iommu_device_register(struct iommu_device *iommu, spin_lock(&iommu_device_lock); list_add_tail(&iommu->list, &iommu_device_list); spin_unlock(&iommu_device_lock); + + for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++) { + struct bus_type *bus = iommu_buses[i]; + int err; + + if (bus->iommu_ops && bus->iommu_ops != ops) { + err = -EBUSY; + } else { + bus->iommu_ops = ops; + err = bus_iommu_probe(bus); + } + if (err) { + iommu_device_unregister(iommu); + return err; + } + } + return 0; } EXPORT_SYMBOL_GPL(iommu_device_register); void iommu_device_unregister(struct iommu_device *iommu) { + for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++) + bus_for_each_dev(iommu_buses[i], NULL, iommu, remove_iommu_group); + spin_lock(&iommu_device_lock); list_del(&iommu->list); spin_unlock(&iommu_device_lock); @@ -1633,13 +1661,6 @@ static int probe_iommu_group(struct device *dev, void *data) return ret; } -static int remove_iommu_group(struct device *dev, void *data) -{ - iommu_release_device(dev); - - return 0; -} - static int iommu_bus_notifier(struct notifier_block *nb, unsigned long action, void *data) { @@ -1828,27 +1849,12 @@ static int iommu_bus_init(struct bus_type *bus) */ int bus_set_iommu(struct bus_type *bus, const struct iommu_ops *ops) { - int err; - - if (ops == NULL) { - bus->iommu_ops = NULL; - return 0; - } - - if (bus->iommu_ops != NULL) + if (bus->iommu_ops && ops && bus->iommu_ops != ops) return -EBUSY; bus->iommu_ops = ops; - /* Do IOMMU specific setup for this bus-type */ - err = bus_iommu_probe(bus); - if (err) { - /* Clean up */ - bus_for_each_dev(bus, NULL, NULL, remove_iommu_group); - bus->iommu_ops = NULL; - } - - return err; + return 0; } EXPORT_SYMBOL_GPL(bus_set_iommu);