Message ID | 1429964096-11524-10-git-send-email-aik@ozlabs.ru (mailing list archive) |
---|---|
State | Accepted |
Commit | 22af48596e9c92313d475306b684f844301ea4cd |
Delegated to: | Benjamin Herrenschmidt |
Headers | show |
On Sat, Apr 25, 2015 at 10:14:33PM +1000, Alexey Kardashevskiy wrote: > This is to make extended ownership and multiple groups support patches > simpler for review. > > This should cause no behavioural change. Um.. this doesn't appear to be true. Previously removing a group from an enabled container would fail with EBUSY, now it forces a disable. > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > [aw: for the vfio related changes] > Acked-by: Alex Williamson <alex.williamson@redhat.com> > Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > --- > drivers/vfio/vfio_iommu_spapr_tce.c | 40 ++++++++++++++++++++++--------------- > 1 file changed, 24 insertions(+), 16 deletions(-) > > diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c > index 115d5e6..0fbe03e 100644 > --- a/drivers/vfio/vfio_iommu_spapr_tce.c > +++ b/drivers/vfio/vfio_iommu_spapr_tce.c > @@ -460,16 +460,21 @@ static int tce_iommu_attach_group(void *iommu_data, > iommu_group_id(container->tbl->it_group), > iommu_group_id(iommu_group)); > ret = -EBUSY; > - } else if (container->enabled) { > + goto unlock_exit; > + } > + > + if (container->enabled) { > pr_err("tce_vfio: attaching group #%u to enabled container\n", > iommu_group_id(iommu_group)); > ret = -EBUSY; > - } else { > - ret = iommu_take_ownership(tbl); > - if (!ret) > - container->tbl = tbl; > + goto unlock_exit; > } > > + ret = iommu_take_ownership(tbl); > + if (!ret) > + container->tbl = tbl; > + > +unlock_exit: > mutex_unlock(&container->lock); > > return ret; > @@ -487,19 +492,22 @@ static void tce_iommu_detach_group(void *iommu_data, > pr_warn("tce_vfio: detaching group #%u, expected group is #%u\n", > iommu_group_id(iommu_group), > iommu_group_id(tbl->it_group)); > - } else { > - if (container->enabled) { > - pr_warn("tce_vfio: detaching group #%u from enabled container, forcing disable\n", > - iommu_group_id(tbl->it_group)); > - tce_iommu_disable(container); > - } > + goto unlock_exit; > + } > > - /* pr_debug("tce_vfio: detaching group #%u from iommu %p\n", > - iommu_group_id(iommu_group), iommu_group); */ > - container->tbl = NULL; > - tce_iommu_clear(container, tbl, tbl->it_offset, tbl->it_size); > - iommu_release_ownership(tbl); > + if (container->enabled) { > + pr_warn("tce_vfio: detaching group #%u from enabled container, forcing disable\n", > + iommu_group_id(tbl->it_group)); > + tce_iommu_disable(container); > } > + > + /* pr_debug("tce_vfio: detaching group #%u from iommu %p\n", > + iommu_group_id(iommu_group), iommu_group); */ > + container->tbl = NULL; > + tce_iommu_clear(container, tbl, tbl->it_offset, tbl->it_size); > + iommu_release_ownership(tbl); > + > +unlock_exit: > mutex_unlock(&container->lock); > } >
On 04/29/2015 12:16 PM, David Gibson wrote: > On Sat, Apr 25, 2015 at 10:14:33PM +1000, Alexey Kardashevskiy wrote: >> This is to make extended ownership and multiple groups support patches >> simpler for review. >> >> This should cause no behavioural change. > > Um.. this doesn't appear to be true. Previously removing a group from > an enabled container would fail with EBUSY, now it forces a disable. This is the original tce_iommu_detach_group() where I cannot find EBUSY you are referring to; it did and does enforce disable. What do I miss here? static void tce_iommu_detach_group(void *iommu_data, struct iommu_group *iommu_group) { struct tce_container *container = iommu_data; struct iommu_table *tbl = iommu_group_get_iommudata(iommu_group); BUG_ON(!tbl); mutex_lock(&container->lock); if (tbl != container->tbl) { pr_warn("tce_vfio: detaching group #%u, expected group is #%u\n", iommu_group_id(iommu_group), iommu_group_id(tbl->it_group)); } else { if (container->enabled) { pr_warn("tce_vfio: detaching group #%u from enabled container, forcing disable\n", iommu_group_id(tbl->it_group)); tce_iommu_disable(container); } /* pr_debug("tce_vfio: detaching group #%u from iommu %p\n", iommu_group_id(iommu_group), iommu_group); */ container->tbl = NULL; tce_iommu_clear(container, tbl, tbl->it_offset, tbl->it_size); iommu_release_ownership(tbl); } mutex_unlock(&container->lock); } > >> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >> [aw: for the vfio related changes] >> Acked-by: Alex Williamson <alex.williamson@redhat.com> >> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> >> --- >> drivers/vfio/vfio_iommu_spapr_tce.c | 40 ++++++++++++++++++++++--------------- >> 1 file changed, 24 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c >> index 115d5e6..0fbe03e 100644 >> --- a/drivers/vfio/vfio_iommu_spapr_tce.c >> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c >> @@ -460,16 +460,21 @@ static int tce_iommu_attach_group(void *iommu_data, >> iommu_group_id(container->tbl->it_group), >> iommu_group_id(iommu_group)); >> ret = -EBUSY; >> - } else if (container->enabled) { >> + goto unlock_exit; >> + } >> + >> + if (container->enabled) { >> pr_err("tce_vfio: attaching group #%u to enabled container\n", >> iommu_group_id(iommu_group)); >> ret = -EBUSY; >> - } else { >> - ret = iommu_take_ownership(tbl); >> - if (!ret) >> - container->tbl = tbl; >> + goto unlock_exit; >> } >> >> + ret = iommu_take_ownership(tbl); >> + if (!ret) >> + container->tbl = tbl; >> + >> +unlock_exit: >> mutex_unlock(&container->lock); >> >> return ret; >> @@ -487,19 +492,22 @@ static void tce_iommu_detach_group(void *iommu_data, >> pr_warn("tce_vfio: detaching group #%u, expected group is #%u\n", >> iommu_group_id(iommu_group), >> iommu_group_id(tbl->it_group)); >> - } else { >> - if (container->enabled) { >> - pr_warn("tce_vfio: detaching group #%u from enabled container, forcing disable\n", >> - iommu_group_id(tbl->it_group)); >> - tce_iommu_disable(container); >> - } >> + goto unlock_exit; >> + } >> >> - /* pr_debug("tce_vfio: detaching group #%u from iommu %p\n", >> - iommu_group_id(iommu_group), iommu_group); */ >> - container->tbl = NULL; >> - tce_iommu_clear(container, tbl, tbl->it_offset, tbl->it_size); >> - iommu_release_ownership(tbl); >> + if (container->enabled) { >> + pr_warn("tce_vfio: detaching group #%u from enabled container, forcing disable\n", >> + iommu_group_id(tbl->it_group)); >> + tce_iommu_disable(container); >> } >> + >> + /* pr_debug("tce_vfio: detaching group #%u from iommu %p\n", >> + iommu_group_id(iommu_group), iommu_group); */ >> + container->tbl = NULL; >> + tce_iommu_clear(container, tbl, tbl->it_offset, tbl->it_size); >> + iommu_release_ownership(tbl); >> + >> +unlock_exit: >> mutex_unlock(&container->lock); >> } >> >
On Thu, Apr 30, 2015 at 12:29:30PM +1000, Alexey Kardashevskiy wrote: > On 04/29/2015 12:16 PM, David Gibson wrote: > >On Sat, Apr 25, 2015 at 10:14:33PM +1000, Alexey Kardashevskiy wrote: > >>This is to make extended ownership and multiple groups support patches > >>simpler for review. > >> > >>This should cause no behavioural change. > > > >Um.. this doesn't appear to be true. Previously removing a group from > >an enabled container would fail with EBUSY, now it forces a disable. > > > This is the original tce_iommu_detach_group() where I cannot find EBUSY you > are referring to; it did and does enforce disable. What do I miss > here? Sorry, my mistake. I misread the patch.
diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c index 115d5e6..0fbe03e 100644 --- a/drivers/vfio/vfio_iommu_spapr_tce.c +++ b/drivers/vfio/vfio_iommu_spapr_tce.c @@ -460,16 +460,21 @@ static int tce_iommu_attach_group(void *iommu_data, iommu_group_id(container->tbl->it_group), iommu_group_id(iommu_group)); ret = -EBUSY; - } else if (container->enabled) { + goto unlock_exit; + } + + if (container->enabled) { pr_err("tce_vfio: attaching group #%u to enabled container\n", iommu_group_id(iommu_group)); ret = -EBUSY; - } else { - ret = iommu_take_ownership(tbl); - if (!ret) - container->tbl = tbl; + goto unlock_exit; } + ret = iommu_take_ownership(tbl); + if (!ret) + container->tbl = tbl; + +unlock_exit: mutex_unlock(&container->lock); return ret; @@ -487,19 +492,22 @@ static void tce_iommu_detach_group(void *iommu_data, pr_warn("tce_vfio: detaching group #%u, expected group is #%u\n", iommu_group_id(iommu_group), iommu_group_id(tbl->it_group)); - } else { - if (container->enabled) { - pr_warn("tce_vfio: detaching group #%u from enabled container, forcing disable\n", - iommu_group_id(tbl->it_group)); - tce_iommu_disable(container); - } + goto unlock_exit; + } - /* pr_debug("tce_vfio: detaching group #%u from iommu %p\n", - iommu_group_id(iommu_group), iommu_group); */ - container->tbl = NULL; - tce_iommu_clear(container, tbl, tbl->it_offset, tbl->it_size); - iommu_release_ownership(tbl); + if (container->enabled) { + pr_warn("tce_vfio: detaching group #%u from enabled container, forcing disable\n", + iommu_group_id(tbl->it_group)); + tce_iommu_disable(container); } + + /* pr_debug("tce_vfio: detaching group #%u from iommu %p\n", + iommu_group_id(iommu_group), iommu_group); */ + container->tbl = NULL; + tce_iommu_clear(container, tbl, tbl->it_offset, tbl->it_size); + iommu_release_ownership(tbl); + +unlock_exit: mutex_unlock(&container->lock); }