Re: [PATCH v1 07/14] vfio: Add a device notifier interface
On Fri, Mar 19, 2021 at 04:25:40PM -0600, Alex Williamson wrote: > I've been trying to figure out how, but I think not. A user can have > multiple devices, each with entirely separate IOMMU contexts. For each > device, the user can create an mmap of memory to that device and add it > to every other IOMMU context. That enables peer to peer DMA between > all the devices, across all the IOMMU contexts. But each individual > device has no direct reference to any IOMMU context other than its own. > A callback on the IOMMU can't reach those other contexts either, there's > no guarantee those other contexts are necessarily managed via the same > vfio IOMMU backend driver. A notifier is the best I can come up with, > please suggest if you have other ideas. Thanks, Indeed, reviewing the set again it seems like we need the notifier here.
Re: [PATCH v1 07/14] vfio: Add a device notifier interface
On Wed, 10 Mar 2021 07:56:39 + Christoph Hellwig wrote: > On Mon, Mar 08, 2021 at 02:48:30PM -0700, Alex Williamson wrote: > > Using a vfio device, a notifier block can be registered to receive > > select device events. Notifiers can only be registered for contained > > devices, ie. they are available through a user context. Registration > > of a notifier increments the reference to that container context > > therefore notifiers must minimally respond to the release event by > > asynchronously removing notifiers. > > Notifiers generally are a horrible multiplexed API. Can't we just > add a proper method table for the intended communication channel? I've been trying to figure out how, but I think not. A user can have multiple devices, each with entirely separate IOMMU contexts. For each device, the user can create an mmap of memory to that device and add it to every other IOMMU context. That enables peer to peer DMA between all the devices, across all the IOMMU contexts. But each individual device has no direct reference to any IOMMU context other than its own. A callback on the IOMMU can't reach those other contexts either, there's no guarantee those other contexts are necessarily managed via the same vfio IOMMU backend driver. A notifier is the best I can come up with, please suggest if you have other ideas. Thanks, Alex
Re: [PATCH v1 07/14] vfio: Add a device notifier interface
On Mon, Mar 08, 2021 at 02:48:30PM -0700, Alex Williamson wrote: > Using a vfio device, a notifier block can be registered to receive > select device events. Notifiers can only be registered for contained > devices, ie. they are available through a user context. Registration > of a notifier increments the reference to that container context > therefore notifiers must minimally respond to the release event by > asynchronously removing notifiers. Notifiers generally are a horrible multiplexed API. Can't we just add a proper method table for the intended communication channel?
Re: [PATCH v1 07/14] vfio: Add a device notifier interface
On Tue, Mar 09, 2021 at 08:45:13AM -0700, Alex Williamson wrote: > > I'm having trouble guessing why we need to refcount the group to add a > > notifier to the device's notifier chain? > > > > I suppose it actually has to do with the MMIO mapping? But I don't > > know what the relation is between MMIO mappings in the IOMMU and the > > container? This could deserve a comment? > > Sure, I can add a comment. We want to make sure the device remains > within an IOMMU context so long as we have a DMA mapping to the device > MMIO, which could potentially manipulate the device. IOMMU context is > managed a the group level. I find refcounting is easier to understand if the refcount inc/dec is near the thing that is actually using the object - so I'd suggest to move this to the iommu code. A comment sounds like a good idea since this is security sensitive Jason
Re: [PATCH v1 07/14] vfio: Add a device notifier interface
On Mon, 8 Mar 2021 20:46:27 -0400
Jason Gunthorpe wrote:
> On Mon, Mar 08, 2021 at 02:48:30PM -0700, Alex Williamson wrote:
> > Using a vfio device, a notifier block can be registered to receive
> > select device events. Notifiers can only be registered for contained
> > devices, ie. they are available through a user context. Registration
> > of a notifier increments the reference to that container context
> > therefore notifiers must minimally respond to the release event by
> > asynchronously removing notifiers.
> >
> > Signed-off-by: Alex Williamson
> > drivers/vfio/Kconfig |1 +
> > drivers/vfio/vfio.c | 35 +++
> > include/linux/vfio.h |9 +
> > 3 files changed, 45 insertions(+)
> >
> > diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig
> > index 90c0525b1e0c..9a67675c9b6c 100644
> > +++ b/drivers/vfio/Kconfig
> > @@ -23,6 +23,7 @@ menuconfig VFIO
> > tristate "VFIO Non-Privileged userspace driver framework"
> > select IOMMU_API
> > select VFIO_IOMMU_TYPE1 if (X86 || S390 || ARM || ARM64)
> > + select SRCU
> > help
> > VFIO provides a framework for secure userspace device drivers.
> > See Documentation/driver-api/vfio.rst for more details.
> > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> > index c47895539a1a..7f6d00e54e83 100644
> > +++ b/drivers/vfio/vfio.c
> > @@ -105,6 +105,7 @@ struct vfio_device {
> > struct list_headgroup_next;
> > void*device_data;
> > struct inode*inode;
> > + struct srcu_notifier_head notifier;
> > };
> >
> > #ifdef CONFIG_VFIO_NOIOMMU
> > @@ -601,6 +602,7 @@ struct vfio_device *vfio_group_create_device(struct
> > vfio_group *group,
> > device->ops = ops;
> > device->device_data = device_data;
> > dev_set_drvdata(dev, device);
> > + srcu_init_notifier_head(>notifier);
> >
> > /* No need to get group_lock, caller has group reference */
> > vfio_group_get(group);
> > @@ -1785,6 +1787,39 @@ static const struct file_operations vfio_device_fops
> > = {
> > .mmap = vfio_device_fops_mmap,
> > };
> >
> > +int vfio_device_register_notifier(struct vfio_device *device,
> > + struct notifier_block *nb)
> > +{
> > + int ret;
> > +
> > + /* Container ref persists until unregister on success */
> > + ret = vfio_group_add_container_user(device->group);
>
> I'm having trouble guessing why we need to refcount the group to add a
> notifier to the device's notifier chain?
>
> I suppose it actually has to do with the MMIO mapping? But I don't
> know what the relation is between MMIO mappings in the IOMMU and the
> container? This could deserve a comment?
Sure, I can add a comment. We want to make sure the device remains
within an IOMMU context so long as we have a DMA mapping to the device
MMIO, which could potentially manipulate the device. IOMMU context is
managed a the group level.
> > +void vfio_device_unregister_notifier(struct vfio_device *device,
> > + struct notifier_block *nb)
> > +{
> > + if (!srcu_notifier_chain_unregister(>notifier, nb))
> > + vfio_group_try_dissolve_container(device->group);
> > +}
> > +EXPORT_SYMBOL_GPL(vfio_device_unregister_notifier);
>
> Is the SRCU still needed with the new locking? With a cursory look I
> only noticed this called under the reflck->lock ?
When registering the notifier, the iommu->lock is held. During the
callback, the same lock is acquired, so we'd have AB-BA otherwise.
Thanks,
Alex
Re: [PATCH v1 07/14] vfio: Add a device notifier interface
On Mon, Mar 08, 2021 at 02:48:30PM -0700, Alex Williamson wrote:
> Using a vfio device, a notifier block can be registered to receive
> select device events. Notifiers can only be registered for contained
> devices, ie. they are available through a user context. Registration
> of a notifier increments the reference to that container context
> therefore notifiers must minimally respond to the release event by
> asynchronously removing notifiers.
>
> Signed-off-by: Alex Williamson
> drivers/vfio/Kconfig |1 +
> drivers/vfio/vfio.c | 35 +++
> include/linux/vfio.h |9 +
> 3 files changed, 45 insertions(+)
>
> diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig
> index 90c0525b1e0c..9a67675c9b6c 100644
> +++ b/drivers/vfio/Kconfig
> @@ -23,6 +23,7 @@ menuconfig VFIO
> tristate "VFIO Non-Privileged userspace driver framework"
> select IOMMU_API
> select VFIO_IOMMU_TYPE1 if (X86 || S390 || ARM || ARM64)
> + select SRCU
> help
> VFIO provides a framework for secure userspace device drivers.
> See Documentation/driver-api/vfio.rst for more details.
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index c47895539a1a..7f6d00e54e83 100644
> +++ b/drivers/vfio/vfio.c
> @@ -105,6 +105,7 @@ struct vfio_device {
> struct list_headgroup_next;
> void*device_data;
> struct inode*inode;
> + struct srcu_notifier_head notifier;
> };
>
> #ifdef CONFIG_VFIO_NOIOMMU
> @@ -601,6 +602,7 @@ struct vfio_device *vfio_group_create_device(struct
> vfio_group *group,
> device->ops = ops;
> device->device_data = device_data;
> dev_set_drvdata(dev, device);
> + srcu_init_notifier_head(>notifier);
>
> /* No need to get group_lock, caller has group reference */
> vfio_group_get(group);
> @@ -1785,6 +1787,39 @@ static const struct file_operations vfio_device_fops =
> {
> .mmap = vfio_device_fops_mmap,
> };
>
> +int vfio_device_register_notifier(struct vfio_device *device,
> + struct notifier_block *nb)
> +{
> + int ret;
> +
> + /* Container ref persists until unregister on success */
> + ret = vfio_group_add_container_user(device->group);
I'm having trouble guessing why we need to refcount the group to add a
notifier to the device's notifier chain?
I suppose it actually has to do with the MMIO mapping? But I don't
know what the relation is between MMIO mappings in the IOMMU and the
container? This could deserve a comment?
> +void vfio_device_unregister_notifier(struct vfio_device *device,
> + struct notifier_block *nb)
> +{
> + if (!srcu_notifier_chain_unregister(>notifier, nb))
> + vfio_group_try_dissolve_container(device->group);
> +}
> +EXPORT_SYMBOL_GPL(vfio_device_unregister_notifier);
Is the SRCU still needed with the new locking? With a cursory look I
only noticed this called under the reflck->lock ?
Jason
[PATCH v1 07/14] vfio: Add a device notifier interface
Using a vfio device, a notifier block can be registered to receive
select device events. Notifiers can only be registered for contained
devices, ie. they are available through a user context. Registration
of a notifier increments the reference to that container context
therefore notifiers must minimally respond to the release event by
asynchronously removing notifiers.
Signed-off-by: Alex Williamson
---
drivers/vfio/Kconfig |1 +
drivers/vfio/vfio.c | 35 +++
include/linux/vfio.h |9 +
3 files changed, 45 insertions(+)
diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig
index 90c0525b1e0c..9a67675c9b6c 100644
--- a/drivers/vfio/Kconfig
+++ b/drivers/vfio/Kconfig
@@ -23,6 +23,7 @@ menuconfig VFIO
tristate "VFIO Non-Privileged userspace driver framework"
select IOMMU_API
select VFIO_IOMMU_TYPE1 if (X86 || S390 || ARM || ARM64)
+ select SRCU
help
VFIO provides a framework for secure userspace device drivers.
See Documentation/driver-api/vfio.rst for more details.
diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index c47895539a1a..7f6d00e54e83 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -105,6 +105,7 @@ struct vfio_device {
struct list_headgroup_next;
void*device_data;
struct inode*inode;
+ struct srcu_notifier_head notifier;
};
#ifdef CONFIG_VFIO_NOIOMMU
@@ -601,6 +602,7 @@ struct vfio_device *vfio_group_create_device(struct
vfio_group *group,
device->ops = ops;
device->device_data = device_data;
dev_set_drvdata(dev, device);
+ srcu_init_notifier_head(>notifier);
/* No need to get group_lock, caller has group reference */
vfio_group_get(group);
@@ -1785,6 +1787,39 @@ static const struct file_operations vfio_device_fops = {
.mmap = vfio_device_fops_mmap,
};
+int vfio_device_register_notifier(struct vfio_device *device,
+ struct notifier_block *nb)
+{
+ int ret;
+
+ /* Container ref persists until unregister on success */
+ ret = vfio_group_add_container_user(device->group);
+ if (ret)
+ return ret;
+
+ ret = srcu_notifier_chain_register(>notifier, nb);
+ if (ret)
+ vfio_group_try_dissolve_container(device->group);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(vfio_device_register_notifier);
+
+void vfio_device_unregister_notifier(struct vfio_device *device,
+ struct notifier_block *nb)
+{
+ if (!srcu_notifier_chain_unregister(>notifier, nb))
+ vfio_group_try_dissolve_container(device->group);
+}
+EXPORT_SYMBOL_GPL(vfio_device_unregister_notifier);
+
+int vfio_device_notifier_call(struct vfio_device *device,
+ enum vfio_device_notify_type event)
+{
+ return srcu_notifier_call_chain(>notifier, event, NULL);
+}
+EXPORT_SYMBOL_GPL(vfio_device_notifier_call);
+
/**
* External user API, exported by symbols to be linked dynamically.
*
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index dbd90d0ba713..c3ff36a7fa6f 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -62,6 +62,15 @@ extern void vfio_device_unmap_mapping_range(struct
vfio_device *device,
loff_t start, loff_t len);
extern struct vfio_device *vfio_device_get_from_vma(struct vm_area_struct
*vma);
extern int vfio_vma_to_pfn(struct vm_area_struct *vma, unsigned long *pfn);
+extern int vfio_device_register_notifier(struct vfio_device *device,
+struct notifier_block *nb);
+extern void vfio_device_unregister_notifier(struct vfio_device *device,
+ struct notifier_block *nb);
+enum vfio_device_notify_type {
+ VFIO_DEVICE_RELEASE = 0,
+};
+int vfio_device_notifier_call(struct vfio_device *device,
+ enum vfio_device_notify_type event);
/* events for the backend driver notify callback */
enum vfio_iommu_notify_type {
