Re: [Intel-gfx] [PATCH 05/13] vfio/fsl: Move to the device set infrastructure
On Tue, Jul 20, 2021 at 07:23:35PM +0300, Diana Craciun OSS wrote: > I have tested the changes and everything works as expected. Great, thanks, I added a Tested-by for you Jason ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 05/13] vfio/fsl: Move to the device set infrastructure
On Tue, Jul 20, 2021 at 07:12:26PM +0300, Diana Craciun OSS wrote:
> On 7/15/2021 3:20 AM, Jason Gunthorpe wrote:
> > FSL uses the internal reflck to implement the open_device() functionality,
> > conversion to the core code is straightforward.
> >
> > The decision on which set to be part of is trivially based on the
> > is_fsl_mc_bus_dprc() and we use a 'struct device *' pointer as the set_id.
> >
> > It isn't entirely clear what the device set lock is actually protecting,
> > but I think it is related to the interrupt setup.
>
> Yes, it is protecting the interrupts setup. The FSL MC devices are using
> MSIs and only the DPRC device is allocating the MSIs from the MSI domain.
> The other devices just take interrupts from a pool. The lock is protecting
> the access to this pool.
It would be much clearer if the lock was near the data it was
protecting, the DPRC pool seems in an entirely different layer..
> > -static void vfio_fsl_mc_release(struct vfio_device *core_vdev)
> > +static void vfio_fsl_mc_close_device(struct vfio_device *core_vdev)
> > {
> > struct vfio_fsl_mc_device *vdev =
> > container_of(core_vdev, struct vfio_fsl_mc_device, vdev);
> > + struct fsl_mc_device *mc_dev = vdev->mc_dev;
> > + struct device *cont_dev = fsl_mc_cont_dev(_dev->dev);
> > + struct fsl_mc_device *mc_cont = to_fsl_mc_device(cont_dev);
> > int ret;
> > - mutex_lock(>reflck->lock);
> > + vfio_fsl_mc_regions_cleanup(vdev);
> > - if (!(--vdev->refcnt)) {
> > - struct fsl_mc_device *mc_dev = vdev->mc_dev;
> > - struct device *cont_dev = fsl_mc_cont_dev(_dev->dev);
> > - struct fsl_mc_device *mc_cont = to_fsl_mc_device(cont_dev);
> > -
> > - vfio_fsl_mc_regions_cleanup(vdev);
> > + /* reset the device before cleaning up the interrupts */
> > + ret = dprc_reset_container(mc_cont->mc_io, 0, mc_cont->mc_handle,
> > + mc_cont->obj_desc.id,
> > + DPRC_RESET_OPTION_NON_RECURSIVE);
> > - /* reset the device before cleaning up the interrupts */
> > - ret = dprc_reset_container(mc_cont->mc_io, 0,
> > - mc_cont->mc_handle,
> > - mc_cont->obj_desc.id,
> > - DPRC_RESET_OPTION_NON_RECURSIVE);
> > + if (WARN_ON(ret))
> > + dev_warn(_cont->dev,
> > +"VFIO_FLS_MC: reset device has failed (%d)\n", ret);
> > - if (ret) {
> > - dev_warn(_cont->dev, "VFIO_FLS_MC: reset device has
> > failed (%d)\n",
> > -ret);
> > - WARN_ON(1);
> > - }
> > + vfio_fsl_mc_irqs_cleanup(vdev);
> > - vfio_fsl_mc_irqs_cleanup(vdev);
> > -
> > - fsl_mc_cleanup_irq_pool(mc_cont);
>
> There is also a need for the lock here. Eventhough the close function is
> called only once, there might be a race between the devices in the
> set.
vfio_fsl_mc_close_device() is already called under this lock:
mutex_lock(>dev_set->lock);
if (!--device->open_count && device->ops->close_device)
device->ops->close_device(device);
mutex_unlock(>dev_set->lock);
Thanks,
Jason
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 05/13] vfio/fsl: Move to the device set infrastructure
On 7/15/2021 3:20 AM, Jason Gunthorpe wrote:
FSL uses the internal reflck to implement the open_device() functionality,
conversion to the core code is straightforward.
The decision on which set to be part of is trivially based on the
is_fsl_mc_bus_dprc() and we use a 'struct device *' pointer as the set_id.
It isn't entirely clear what the device set lock is actually protecting,
but I think it is related to the interrupt setup.
Yes, it is protecting the interrupts setup. The FSL MC devices are using
MSIs and only the DPRC device is allocating the MSIs from the MSI
domain. The other devices just take interrupts from a pool. The lock is
protecting the access to this pool.
Signed-off-by: Yishai Hadas
Signed-off-by: Jason Gunthorpe
---
drivers/vfio/fsl-mc/vfio_fsl_mc.c | 152 --
drivers/vfio/fsl-mc/vfio_fsl_mc_intr.c| 6 +-
drivers/vfio/fsl-mc/vfio_fsl_mc_private.h | 7 -
3 files changed, 26 insertions(+), 139 deletions(-)
diff --git a/drivers/vfio/fsl-mc/vfio_fsl_mc.c
b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
index 3d2be06e1bc146..49b93de05d5d62 100644
--- a/drivers/vfio/fsl-mc/vfio_fsl_mc.c
+++ b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
@@ -19,81 +19,10 @@
static struct fsl_mc_driver vfio_fsl_mc_driver;
-static DEFINE_MUTEX(reflck_lock);
-
-static void vfio_fsl_mc_reflck_get(struct vfio_fsl_mc_reflck *reflck)
-{
- kref_get(>kref);
-}
-
-static void vfio_fsl_mc_reflck_release(struct kref *kref)
-{
- struct vfio_fsl_mc_reflck *reflck = container_of(kref,
- struct vfio_fsl_mc_reflck,
- kref);
-
- mutex_destroy(>lock);
- kfree(reflck);
- mutex_unlock(_lock);
-}
-
-static void vfio_fsl_mc_reflck_put(struct vfio_fsl_mc_reflck *reflck)
-{
- kref_put_mutex(>kref, vfio_fsl_mc_reflck_release, _lock);
-}
-
-static struct vfio_fsl_mc_reflck *vfio_fsl_mc_reflck_alloc(void)
-{
- struct vfio_fsl_mc_reflck *reflck;
-
- reflck = kzalloc(sizeof(*reflck), GFP_KERNEL);
- if (!reflck)
- return ERR_PTR(-ENOMEM);
-
- kref_init(>kref);
- mutex_init(>lock);
-
- return reflck;
-}
-
-static int vfio_fsl_mc_reflck_attach(struct vfio_fsl_mc_device *vdev)
-{
- int ret = 0;
-
- mutex_lock(_lock);
- if (is_fsl_mc_bus_dprc(vdev->mc_dev)) {
- vdev->reflck = vfio_fsl_mc_reflck_alloc();
- ret = PTR_ERR_OR_ZERO(vdev->reflck);
- } else {
- struct device *mc_cont_dev = vdev->mc_dev->dev.parent;
- struct vfio_device *device;
- struct vfio_fsl_mc_device *cont_vdev;
-
- device = vfio_device_get_from_dev(mc_cont_dev);
- if (!device) {
- ret = -ENODEV;
- goto unlock;
- }
-
- cont_vdev =
- container_of(device, struct vfio_fsl_mc_device, vdev);
- if (!cont_vdev || !cont_vdev->reflck) {
- vfio_device_put(device);
- ret = -ENODEV;
- goto unlock;
- }
- vfio_fsl_mc_reflck_get(cont_vdev->reflck);
- vdev->reflck = cont_vdev->reflck;
- vfio_device_put(device);
- }
-
-unlock:
- mutex_unlock(_lock);
- return ret;
-}
-
-static int vfio_fsl_mc_regions_init(struct vfio_fsl_mc_device *vdev)
+static int vfio_fsl_mc_open_device(struct vfio_device *core_vdev)
{
+ struct vfio_fsl_mc_device *vdev =
+ container_of(core_vdev, struct vfio_fsl_mc_device, vdev);
struct fsl_mc_device *mc_dev = vdev->mc_dev;
int count = mc_dev->obj_desc.region_count;
int i;
@@ -136,58 +65,30 @@ static void vfio_fsl_mc_regions_cleanup(struct
vfio_fsl_mc_device *vdev)
kfree(vdev->regions);
}
-static int vfio_fsl_mc_open(struct vfio_device *core_vdev)
-{
- struct vfio_fsl_mc_device *vdev =
- container_of(core_vdev, struct vfio_fsl_mc_device, vdev);
- int ret = 0;
-
- mutex_lock(>reflck->lock);
- if (!vdev->refcnt) {
- ret = vfio_fsl_mc_regions_init(vdev);
- if (ret)
- goto out;
- }
- vdev->refcnt++;
-out:
- mutex_unlock(>reflck->lock);
- return ret;
-}
-
-static void vfio_fsl_mc_release(struct vfio_device *core_vdev)
+static void vfio_fsl_mc_close_device(struct vfio_device *core_vdev)
{
struct vfio_fsl_mc_device *vdev =
container_of(core_vdev, struct vfio_fsl_mc_device, vdev);
+ struct fsl_mc_device *mc_dev = vdev->mc_dev;
+ struct device *cont_dev = fsl_mc_cont_dev(_dev->dev);
+ struct fsl_mc_device *mc_cont = to_fsl_mc_device(cont_dev);
int ret;
- mutex_lock(>reflck->lock);
+ vfio_fsl_mc_regions_cleanup(vdev);
- if (!(--vdev->refcnt)) {
- struct
Re: [Intel-gfx] [PATCH 05/13] vfio/fsl: Move to the device set infrastructure
On 7/20/2021 7:17 PM, Jason Gunthorpe wrote:
On Tue, Jul 20, 2021 at 07:12:26PM +0300, Diana Craciun OSS wrote:
On 7/15/2021 3:20 AM, Jason Gunthorpe wrote:
FSL uses the internal reflck to implement the open_device() functionality,
conversion to the core code is straightforward.
The decision on which set to be part of is trivially based on the
is_fsl_mc_bus_dprc() and we use a 'struct device *' pointer as the set_id.
It isn't entirely clear what the device set lock is actually protecting,
but I think it is related to the interrupt setup.
Yes, it is protecting the interrupts setup. The FSL MC devices are using
MSIs and only the DPRC device is allocating the MSIs from the MSI domain.
The other devices just take interrupts from a pool. The lock is protecting
the access to this pool.
It would be much clearer if the lock was near the data it was
protecting, the DPRC pool seems in an entirely different layer..
Yes, I agree. I will think about of a more clearer design for a future
improvement.
-static void vfio_fsl_mc_release(struct vfio_device *core_vdev)
+static void vfio_fsl_mc_close_device(struct vfio_device *core_vdev)
{
struct vfio_fsl_mc_device *vdev =
container_of(core_vdev, struct vfio_fsl_mc_device, vdev);
+ struct fsl_mc_device *mc_dev = vdev->mc_dev;
+ struct device *cont_dev = fsl_mc_cont_dev(_dev->dev);
+ struct fsl_mc_device *mc_cont = to_fsl_mc_device(cont_dev);
int ret;
- mutex_lock(>reflck->lock);
+ vfio_fsl_mc_regions_cleanup(vdev);
- if (!(--vdev->refcnt)) {
- struct fsl_mc_device *mc_dev = vdev->mc_dev;
- struct device *cont_dev = fsl_mc_cont_dev(_dev->dev);
- struct fsl_mc_device *mc_cont = to_fsl_mc_device(cont_dev);
-
- vfio_fsl_mc_regions_cleanup(vdev);
+ /* reset the device before cleaning up the interrupts */
+ ret = dprc_reset_container(mc_cont->mc_io, 0, mc_cont->mc_handle,
+ mc_cont->obj_desc.id,
+ DPRC_RESET_OPTION_NON_RECURSIVE);
- /* reset the device before cleaning up the interrupts */
- ret = dprc_reset_container(mc_cont->mc_io, 0,
- mc_cont->mc_handle,
- mc_cont->obj_desc.id,
- DPRC_RESET_OPTION_NON_RECURSIVE);
+ if (WARN_ON(ret))
+ dev_warn(_cont->dev,
+"VFIO_FLS_MC: reset device has failed (%d)\n", ret);
- if (ret) {
- dev_warn(_cont->dev, "VFIO_FLS_MC: reset device has
failed (%d)\n",
-ret);
- WARN_ON(1);
- }
+ vfio_fsl_mc_irqs_cleanup(vdev);
- vfio_fsl_mc_irqs_cleanup(vdev);
-
- fsl_mc_cleanup_irq_pool(mc_cont);
There is also a need for the lock here. Eventhough the close function is
called only once, there might be a race between the devices in the
set.
vfio_fsl_mc_close_device() is already called under this lock:
mutex_lock(>dev_set->lock);
if (!--device->open_count && device->ops->close_device)
device->ops->close_device(device);
mutex_unlock(>dev_set->lock);
OK, I missed that.
Thanks,
Jason
I have tested the changes and everything works as expected.
Thanks,
Diana
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 05/13] vfio/fsl: Move to the device set infrastructure
FSL uses the internal reflck to implement the open_device() functionality,
conversion to the core code is straightforward.
The decision on which set to be part of is trivially based on the
is_fsl_mc_bus_dprc() and we use a 'struct device *' pointer as the set_id.
It isn't entirely clear what the device set lock is actually protecting,
but I think it is related to the interrupt setup.
Signed-off-by: Yishai Hadas
Signed-off-by: Jason Gunthorpe
---
drivers/vfio/fsl-mc/vfio_fsl_mc.c | 152 --
drivers/vfio/fsl-mc/vfio_fsl_mc_intr.c| 6 +-
drivers/vfio/fsl-mc/vfio_fsl_mc_private.h | 7 -
3 files changed, 26 insertions(+), 139 deletions(-)
diff --git a/drivers/vfio/fsl-mc/vfio_fsl_mc.c
b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
index 3d2be06e1bc146..49b93de05d5d62 100644
--- a/drivers/vfio/fsl-mc/vfio_fsl_mc.c
+++ b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
@@ -19,81 +19,10 @@
static struct fsl_mc_driver vfio_fsl_mc_driver;
-static DEFINE_MUTEX(reflck_lock);
-
-static void vfio_fsl_mc_reflck_get(struct vfio_fsl_mc_reflck *reflck)
-{
- kref_get(>kref);
-}
-
-static void vfio_fsl_mc_reflck_release(struct kref *kref)
-{
- struct vfio_fsl_mc_reflck *reflck = container_of(kref,
- struct vfio_fsl_mc_reflck,
- kref);
-
- mutex_destroy(>lock);
- kfree(reflck);
- mutex_unlock(_lock);
-}
-
-static void vfio_fsl_mc_reflck_put(struct vfio_fsl_mc_reflck *reflck)
-{
- kref_put_mutex(>kref, vfio_fsl_mc_reflck_release, _lock);
-}
-
-static struct vfio_fsl_mc_reflck *vfio_fsl_mc_reflck_alloc(void)
-{
- struct vfio_fsl_mc_reflck *reflck;
-
- reflck = kzalloc(sizeof(*reflck), GFP_KERNEL);
- if (!reflck)
- return ERR_PTR(-ENOMEM);
-
- kref_init(>kref);
- mutex_init(>lock);
-
- return reflck;
-}
-
-static int vfio_fsl_mc_reflck_attach(struct vfio_fsl_mc_device *vdev)
-{
- int ret = 0;
-
- mutex_lock(_lock);
- if (is_fsl_mc_bus_dprc(vdev->mc_dev)) {
- vdev->reflck = vfio_fsl_mc_reflck_alloc();
- ret = PTR_ERR_OR_ZERO(vdev->reflck);
- } else {
- struct device *mc_cont_dev = vdev->mc_dev->dev.parent;
- struct vfio_device *device;
- struct vfio_fsl_mc_device *cont_vdev;
-
- device = vfio_device_get_from_dev(mc_cont_dev);
- if (!device) {
- ret = -ENODEV;
- goto unlock;
- }
-
- cont_vdev =
- container_of(device, struct vfio_fsl_mc_device, vdev);
- if (!cont_vdev || !cont_vdev->reflck) {
- vfio_device_put(device);
- ret = -ENODEV;
- goto unlock;
- }
- vfio_fsl_mc_reflck_get(cont_vdev->reflck);
- vdev->reflck = cont_vdev->reflck;
- vfio_device_put(device);
- }
-
-unlock:
- mutex_unlock(_lock);
- return ret;
-}
-
-static int vfio_fsl_mc_regions_init(struct vfio_fsl_mc_device *vdev)
+static int vfio_fsl_mc_open_device(struct vfio_device *core_vdev)
{
+ struct vfio_fsl_mc_device *vdev =
+ container_of(core_vdev, struct vfio_fsl_mc_device, vdev);
struct fsl_mc_device *mc_dev = vdev->mc_dev;
int count = mc_dev->obj_desc.region_count;
int i;
@@ -136,58 +65,30 @@ static void vfio_fsl_mc_regions_cleanup(struct
vfio_fsl_mc_device *vdev)
kfree(vdev->regions);
}
-static int vfio_fsl_mc_open(struct vfio_device *core_vdev)
-{
- struct vfio_fsl_mc_device *vdev =
- container_of(core_vdev, struct vfio_fsl_mc_device, vdev);
- int ret = 0;
-
- mutex_lock(>reflck->lock);
- if (!vdev->refcnt) {
- ret = vfio_fsl_mc_regions_init(vdev);
- if (ret)
- goto out;
- }
- vdev->refcnt++;
-out:
- mutex_unlock(>reflck->lock);
- return ret;
-}
-
-static void vfio_fsl_mc_release(struct vfio_device *core_vdev)
+static void vfio_fsl_mc_close_device(struct vfio_device *core_vdev)
{
struct vfio_fsl_mc_device *vdev =
container_of(core_vdev, struct vfio_fsl_mc_device, vdev);
+ struct fsl_mc_device *mc_dev = vdev->mc_dev;
+ struct device *cont_dev = fsl_mc_cont_dev(_dev->dev);
+ struct fsl_mc_device *mc_cont = to_fsl_mc_device(cont_dev);
int ret;
- mutex_lock(>reflck->lock);
+ vfio_fsl_mc_regions_cleanup(vdev);
- if (!(--vdev->refcnt)) {
- struct fsl_mc_device *mc_dev = vdev->mc_dev;
- struct device *cont_dev = fsl_mc_cont_dev(_dev->dev);
- struct fsl_mc_device *mc_cont = to_fsl_mc_device(cont_dev);
-
- vfio_fsl_mc_regions_cleanup(vdev);
+ /* reset the device before cleaning up the interrupts */
+
