2021-01-25 09:54:27

by Shenming Lu

Subject: [RFC PATCH v1 3/4] vfio: Try to enable IOPF for VFIO devices

If IOMMU_DEV_FEAT_IOPF is set for the VFIO device, which means that
the delivering of page faults of this device from the IOMMU is enabled,
we register the VFIO page fault handler to complete the whole faulting
path (HW+SW). And add a iopf_enabled field in struct vfio_device to
record it.

Signed-off-by: Shenming Lu <[email protected]>
---
drivers/vfio/vfio.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)

diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index ff7797260d0f..fd885d99ee0f 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -97,6 +97,7 @@ struct vfio_device {
struct vfio_group *group;
struct list_head group_next;
void *device_data;
+ bool iopf_enabled;
};

#ifdef CONFIG_VFIO_NOIOMMU
@@ -532,6 +533,21 @@ static struct vfio_group *vfio_group_get_from_dev(struct device *dev)
/**
* Device objects - create, release, get, put, search
*/
+
+static void vfio_device_enable_iopf(struct vfio_device *device)
+{
+ struct device *dev = device->dev;
+
+ if (!iommu_dev_has_feature(dev, IOMMU_DEV_FEAT_IOPF))
+ return;
+
+ if (WARN_ON(iommu_register_device_fault_handler(dev,
+ vfio_iommu_dev_fault_handler, dev)))
+ return;
+
+ device->iopf_enabled = true;
+}
+
static
struct vfio_device *vfio_group_create_device(struct vfio_group *group,
struct device *dev,
@@ -549,6 +565,8 @@ struct vfio_device *vfio_group_create_device(struct vfio_group *group,
device->group = group;
device->ops = ops;
device->device_data = device_data;
+ /* By default try to enable IOPF */
+ vfio_device_enable_iopf(device);
dev_set_drvdata(dev, device);

/* No need to get group_lock, caller has group reference */
@@ -573,6 +591,8 @@ static void vfio_device_release(struct kref *kref)
mutex_unlock(&group->device_lock);

dev_set_drvdata(device->dev, NULL);
+ if (device->iopf_enabled)
+ WARN_ON(iommu_unregister_device_fault_handler(device->dev));

kfree(device);

--
2.19.1

2021-01-29 22:44:58

by Alex Williamson

Subject: Re: [RFC PATCH v1 3/4] vfio: Try to enable IOPF for VFIO devices

On Mon, 25 Jan 2021 17:04:01 +0800
Shenming Lu <[email protected]> wrote:

> If IOMMU_DEV_FEAT_IOPF is set for the VFIO device, which means that
> the delivering of page faults of this device from the IOMMU is enabled,
> we register the VFIO page fault handler to complete the whole faulting
> path (HW+SW). And add a iopf_enabled field in struct vfio_device to
> record it.
>
> Signed-off-by: Shenming Lu <[email protected]>
> ---
> drivers/vfio/vfio.c | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index ff7797260d0f..fd885d99ee0f 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -97,6 +97,7 @@ struct vfio_device {
> struct vfio_group *group;
> struct list_head group_next;
> void *device_data;
> + bool iopf_enabled;
> };
>
> #ifdef CONFIG_VFIO_NOIOMMU
> @@ -532,6 +533,21 @@ static struct vfio_group *vfio_group_get_from_dev(struct device *dev)
> /**
> * Device objects - create, release, get, put, search
> */
> +
> +static void vfio_device_enable_iopf(struct vfio_device *device)
> +{
> + struct device *dev = device->dev;
> +
> + if (!iommu_dev_has_feature(dev, IOMMU_DEV_FEAT_IOPF))
> + return;
> +
> + if (WARN_ON(iommu_register_device_fault_handler(dev,
> + vfio_iommu_dev_fault_handler, dev)))

The layering here is wrong, vfio-core doesn't manage the IOMMU, we have
backend IOMMU drivers for that. We can't even assume we have IOMMU API
support here, that's what the type1 backend handles. Thanks,

Alex

> + return;
> +
> + device->iopf_enabled = true;
> +}
> +
> static
> struct vfio_device *vfio_group_create_device(struct vfio_group *group,
> struct device *dev,
> @@ -549,6 +565,8 @@ struct vfio_device *vfio_group_create_device(struct vfio_group *group,
> device->group = group;
> device->ops = ops;
> device->device_data = device_data;
> + /* By default try to enable IOPF */
> + vfio_device_enable_iopf(device);
> dev_set_drvdata(dev, device);
>
> /* No need to get group_lock, caller has group reference */
> @@ -573,6 +591,8 @@ static void vfio_device_release(struct kref *kref)
> mutex_unlock(&group->device_lock);
>
> dev_set_drvdata(device->dev, NULL);
> + if (device->iopf_enabled)
> + WARN_ON(iommu_unregister_device_fault_handler(device->dev));
>
> kfree(device);
>

2021-01-30 09:36:33

by Shenming Lu

Subject: Re: [RFC PATCH v1 3/4] vfio: Try to enable IOPF for VFIO devices

On 2021/1/30 6:42, Alex Williamson wrote:
> On Mon, 25 Jan 2021 17:04:01 +0800
> Shenming Lu <[email protected]> wrote:
>
>> If IOMMU_DEV_FEAT_IOPF is set for the VFIO device, which means that
>> the delivering of page faults of this device from the IOMMU is enabled,
>> we register the VFIO page fault handler to complete the whole faulting
>> path (HW+SW). And add a iopf_enabled field in struct vfio_device to
>> record it.
>>
>> Signed-off-by: Shenming Lu <[email protected]>
>> ---
>> drivers/vfio/vfio.c | 20 ++++++++++++++++++++
>> 1 file changed, 20 insertions(+)
>>
>> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
>> index ff7797260d0f..fd885d99ee0f 100644
>> --- a/drivers/vfio/vfio.c
>> +++ b/drivers/vfio/vfio.c
>> @@ -97,6 +97,7 @@ struct vfio_device {
>> struct vfio_group *group;
>> struct list_head group_next;
>> void *device_data;
>> + bool iopf_enabled;
>> };
>>
>> #ifdef CONFIG_VFIO_NOIOMMU
>> @@ -532,6 +533,21 @@ static struct vfio_group *vfio_group_get_from_dev(struct device *dev)
>> /**
>> * Device objects - create, release, get, put, search
>> */
>> +
>> +static void vfio_device_enable_iopf(struct vfio_device *device)
>> +{
>> + struct device *dev = device->dev;
>> +
>> + if (!iommu_dev_has_feature(dev, IOMMU_DEV_FEAT_IOPF))
>> + return;
>> +
>> + if (WARN_ON(iommu_register_device_fault_handler(dev,
>> + vfio_iommu_dev_fault_handler, dev)))
>
> The layering here is wrong, vfio-core doesn't manage the IOMMU, we have
> backend IOMMU drivers for that. We can't even assume we have IOMMU API
> support here, that's what the type1 backend handles. Thanks,

Thanks for pointing it out, I will correct it: maybe do the enabling via
the VFIO_IOMMU_ENABLE_IOPF ioctl mentioned in the cover and suggest the
user to call it before VFIO_IOMMU_MAP_DMA, also move the iopf_enabled field
from struct vfio_device to struct vfio_iommu...

Thanks,
Shenming

>
> Alex
>
>> + return;
>> +
>> + device->iopf_enabled = true;
>> +}
>> +
>> static
>> struct vfio_device *vfio_group_create_device(struct vfio_group *group,
>> struct device *dev,
>> @@ -549,6 +565,8 @@ struct vfio_device *vfio_group_create_device(struct vfio_group *group,
>> device->group = group;
>> device->ops = ops;
>> device->device_data = device_data;
>> + /* By default try to enable IOPF */
>> + vfio_device_enable_iopf(device);
>> dev_set_drvdata(dev, device);
>>
>> /* No need to get group_lock, caller has group reference */
>> @@ -573,6 +591,8 @@ static void vfio_device_release(struct kref *kref)
>> mutex_unlock(&group->device_lock);
>>
>> dev_set_drvdata(device->dev, NULL);
>> + if (device->iopf_enabled)
>> + WARN_ON(iommu_unregister_device_fault_handler(device->dev));
>>
>> kfree(device);
>>
>
> .
>