When a user-managed page table is attached to an IOMMU, it is necessary
to deliver IO page faults to user space so that they can be handled
appropriately. One use case for this is nested translation, which is
currently being discussed in the mailing list.
I have posted a RFC series [1] that describes the implementation of
delivering page faults to user space through IOMMUFD. This series has
received several comments on the IOMMU refactoring, which I have
addressed in this series.
The major refactoring includes:
- Removing include/uapi/linux/iommu.h.
- Removing iommu_[un]register_device_fault_handler().
- Making fault_param always available between iommu device probe and
release.
- Using fault cookie to store temporary data used for processing faults.
This is also available at github [2]. I would appreciate your feedback
on this series.
[1] https://lore.kernel.org/linux-iommu/[email protected]/
[2] https://github.com/LuBaolu/intel-iommu/commits/preparatory-io-pgfault-delivery-v1
Best regards,
baolu
Lu Baolu (9):
iommu: Move iommu fault data to linux/iommu.h
iommu: Add device parameter to iopf handler
iommu: Add common code to handle IO page faults
iommu: Change the return value of dev_iommu_get()
iommu: Make fault_param generic
iommu: Remove iommu_[un]register_device_fault_handler()
iommu: Add helper to set iopf handler for domain
iommu: Add iommu page fault cookie helpers
iommu: Use fault cookie to store iopf_param
include/linux/iommu.h | 206 +++++++++++++++---
drivers/iommu/iommu-sva.h | 8 +-
include/uapi/linux/iommu.h | 161 --------------
.../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 13 +-
drivers/iommu/intel/iommu.c | 18 +-
drivers/iommu/io-pgfault.c | 55 +++--
drivers/iommu/iommu-sva.c | 2 +-
drivers/iommu/iommu.c | 199 ++++++++---------
MAINTAINERS | 1 -
9 files changed, 320 insertions(+), 343 deletions(-)
delete mode 100644 include/uapi/linux/iommu.h
--
2.34.1
The individual iommu drivers report iommu faults by calling
iommu_report_device_fault(), where a pre-registered device fault handler
is called to route the fault to another fault handler installed on the
corresponding iommu domain.
The pre-registered device fault handler is static and won't be dynamic
as the fault handler is eventually per iommu domain. Replace the device
fault handler with a static common code to avoid unnecessary code.
Signed-off-by: Lu Baolu <[email protected]>
---
drivers/iommu/iommu.c | 24 +++++++++++++++++++++++-
1 file changed, 23 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index da340f11c5f5..41328f03e8b4 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1278,6 +1278,28 @@ int iommu_unregister_device_fault_handler(struct device *dev)
}
EXPORT_SYMBOL_GPL(iommu_unregister_device_fault_handler);
+static int iommu_handle_io_pgfault(struct device *dev,
+ struct iommu_fault *fault)
+{
+ struct iommu_domain *domain;
+
+ if (fault->type != IOMMU_FAULT_PAGE_REQ)
+ return -EINVAL;
+
+ if (fault->prm.flags & IOMMU_FAULT_PAGE_REQUEST_PASID_VALID)
+ domain = iommu_get_domain_for_dev_pasid(dev, fault->prm.pasid, 0);
+ else
+ domain = iommu_get_domain_for_dev(dev);
+
+ if (!domain || !domain->iopf_handler)
+ return -ENODEV;
+
+ if (domain->iopf_handler == iommu_sva_handle_iopf)
+ return iommu_queue_iopf(fault, dev);
+
+ return domain->iopf_handler(fault, dev, domain->fault_data);
+}
+
/**
* iommu_report_device_fault() - Report fault event to device driver
* @dev: the device
@@ -1320,7 +1342,7 @@ int iommu_report_device_fault(struct device *dev, struct iommu_fault_event *evt)
mutex_unlock(&fparam->lock);
}
- ret = fparam->handler(&evt->fault, fparam->data);
+ ret = iommu_handle_io_pgfault(dev, &evt->fault);
if (ret && evt_pending) {
mutex_lock(&fparam->lock);
list_del(&evt_pending->list);
--
2.34.1
> From: Lu Baolu <[email protected]>
> Sent: Tuesday, July 11, 2023 9:07 AM
>
> +static int iommu_handle_io_pgfault(struct device *dev,
> + struct iommu_fault *fault)
> +{
> + struct iommu_domain *domain;
> +
> + if (fault->type != IOMMU_FAULT_PAGE_REQ)
> + return -EINVAL;
> +
> + if (fault->prm.flags & IOMMU_FAULT_PAGE_REQUEST_PASID_VALID)
> + domain = iommu_get_domain_for_dev_pasid(dev, fault-
> >prm.pasid, 0);
> + else
> + domain = iommu_get_domain_for_dev(dev);
> +
> + if (!domain || !domain->iopf_handler)
> + return -ENODEV;
> +
> + if (domain->iopf_handler == iommu_sva_handle_iopf)
> + return iommu_queue_iopf(fault, dev);
You can avoid the special check by directly making iommu_queue_iopf
as the iopf_handler for sva domain.
> +
> + return domain->iopf_handler(fault, dev, domain->fault_data);
> +}
btw is there value of moving the group handling logic from
iommu_queue_iopf() to this common function?
I wonder whether there is any correctness issue if not forwarding
partial request to iommufd. If not this can also help reduce
notifications to the user until the group is ready.
Hi Lu,
On Tue, 11 Jul 2023 09:06:36 +0800, Lu Baolu <[email protected]>
wrote:
> The individual iommu drivers report iommu faults by calling
> iommu_report_device_fault(), where a pre-registered device fault handler
> is called to route the fault to another fault handler installed on the
> corresponding iommu domain.
>
> The pre-registered device fault handler is static and won't be dynamic
> as the fault handler is eventually per iommu domain. Replace the device
> fault handler with a static common code to avoid unnecessary code.
>
> Signed-off-by: Lu Baolu <[email protected]>
> ---
> drivers/iommu/iommu.c | 24 +++++++++++++++++++++++-
> 1 file changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index da340f11c5f5..41328f03e8b4 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -1278,6 +1278,28 @@ int iommu_unregister_device_fault_handler(struct
> device *dev) }
> EXPORT_SYMBOL_GPL(iommu_unregister_device_fault_handler);
>
> +static int iommu_handle_io_pgfault(struct device *dev,
> + struct iommu_fault *fault)
> +{
> + struct iommu_domain *domain;
> +
> + if (fault->type != IOMMU_FAULT_PAGE_REQ)
> + return -EINVAL;
> +
> + if (fault->prm.flags & IOMMU_FAULT_PAGE_REQUEST_PASID_VALID)
> + domain = iommu_get_domain_for_dev_pasid(dev,
> fault->prm.pasid, 0);
> + else
> + domain = iommu_get_domain_for_dev(dev);
we don't support IOPF w/o PASID yet, right?
> +
> + if (!domain || !domain->iopf_handler)
> + return -ENODEV;
> +
> + if (domain->iopf_handler == iommu_sva_handle_iopf)
> + return iommu_queue_iopf(fault, dev);
Just wondering why have a special treatment for SVA domain. Can
iommu_queue_iopf() be used as SVA domain->iopf_handler?
> +
> + return domain->iopf_handler(fault, dev, domain->fault_data);
> +}
> +
> /**
> * iommu_report_device_fault() - Report fault event to device driver
> * @dev: the device
> @@ -1320,7 +1342,7 @@ int iommu_report_device_fault(struct device *dev,
> struct iommu_fault_event *evt) mutex_unlock(&fparam->lock);
> }
>
> - ret = fparam->handler(&evt->fault, fparam->data);
> + ret = iommu_handle_io_pgfault(dev, &evt->fault);
> if (ret && evt_pending) {
> mutex_lock(&fparam->lock);
> list_del(&evt_pending->list);
Thanks,
Jacob
On 2023/7/11 14:12, Tian, Kevin wrote:
>> From: Lu Baolu <[email protected]>
>> Sent: Tuesday, July 11, 2023 9:07 AM
>>
>> +static int iommu_handle_io_pgfault(struct device *dev,
>> + struct iommu_fault *fault)
>> +{
>> + struct iommu_domain *domain;
>> +
>> + if (fault->type != IOMMU_FAULT_PAGE_REQ)
>> + return -EINVAL;
>> +
>> + if (fault->prm.flags & IOMMU_FAULT_PAGE_REQUEST_PASID_VALID)
>> + domain = iommu_get_domain_for_dev_pasid(dev, fault-
>>> prm.pasid, 0);
>> + else
>> + domain = iommu_get_domain_for_dev(dev);
>> +
>> + if (!domain || !domain->iopf_handler)
>> + return -ENODEV;
>> +
>> + if (domain->iopf_handler == iommu_sva_handle_iopf)
>> + return iommu_queue_iopf(fault, dev);
>
> You can avoid the special check by directly making iommu_queue_iopf
> as the iopf_handler for sva domain.
Yeah, good catch!
>
>> +
>> + return domain->iopf_handler(fault, dev, domain->fault_data);
>> +}
>
> btw is there value of moving the group handling logic from
> iommu_queue_iopf() to this common function?
>
> I wonder whether there is any correctness issue if not forwarding
> partial request to iommufd. If not this can also help reduce
> notifications to the user until the group is ready.
I don't think there's any correctness issue. But it should be better if
we can inject the page faults to vm guests as soon as possible. There's
no requirement to put page requests to vIOMMU's hardware page request
queue at the granularity of a fault group. Thoughts?
Best regards,
baolu
On 2023/7/12 4:50, Jacob Pan wrote:
> Hi Lu,
>
> On Tue, 11 Jul 2023 09:06:36 +0800, Lu Baolu <[email protected]>
> wrote:
>
>> The individual iommu drivers report iommu faults by calling
>> iommu_report_device_fault(), where a pre-registered device fault handler
>> is called to route the fault to another fault handler installed on the
>> corresponding iommu domain.
>>
>> The pre-registered device fault handler is static and won't be dynamic
>> as the fault handler is eventually per iommu domain. Replace the device
>> fault handler with a static common code to avoid unnecessary code.
>>
>> Signed-off-by: Lu Baolu <[email protected]>
>> ---
>> drivers/iommu/iommu.c | 24 +++++++++++++++++++++++-
>> 1 file changed, 23 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index da340f11c5f5..41328f03e8b4 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -1278,6 +1278,28 @@ int iommu_unregister_device_fault_handler(struct
>> device *dev) }
>> EXPORT_SYMBOL_GPL(iommu_unregister_device_fault_handler);
>>
>> +static int iommu_handle_io_pgfault(struct device *dev,
>> + struct iommu_fault *fault)
>> +{
>> + struct iommu_domain *domain;
>> +
>> + if (fault->type != IOMMU_FAULT_PAGE_REQ)
>> + return -EINVAL;
>> +
>> + if (fault->prm.flags & IOMMU_FAULT_PAGE_REQUEST_PASID_VALID)
>> + domain = iommu_get_domain_for_dev_pasid(dev,
>> fault->prm.pasid, 0);
>> + else
>> + domain = iommu_get_domain_for_dev(dev);
> we don't support IOPF w/o PASID yet, right?
It's the individual driver that decides whether iopf w/o pasid is
supported or not. The iommu core doesn't need to make such assumption.
>
>> +
>> + if (!domain || !domain->iopf_handler)
>> + return -ENODEV;
>> +
>> + if (domain->iopf_handler == iommu_sva_handle_iopf)
>> + return iommu_queue_iopf(fault, dev);
> Just wondering why have a special treatment for SVA domain. Can
> iommu_queue_iopf() be used as SVA domain->iopf_handler?
Yes. I will make this change according to Kevin's suggestion in this
thread.
>
>> +
>> + return domain->iopf_handler(fault, dev, domain->fault_data);
>> +}
>> +
>> /**
>> * iommu_report_device_fault() - Report fault event to device driver
>> * @dev: the device
>> @@ -1320,7 +1342,7 @@ int iommu_report_device_fault(struct device *dev,
>> struct iommu_fault_event *evt) mutex_unlock(&fparam->lock);
>> }
>>
>> - ret = fparam->handler(&evt->fault, fparam->data);
>> + ret = iommu_handle_io_pgfault(dev, &evt->fault);
>> if (ret && evt_pending) {
>> mutex_lock(&fparam->lock);
>> list_del(&evt_pending->list);
>
>
> Thanks,
>
> Jacob
>
Best regards,
baolu
On Wed, Jul 12, 2023 at 10:32:13AM +0800, Baolu Lu wrote:
> > btw is there value of moving the group handling logic from
> > iommu_queue_iopf() to this common function?
> >
> > I wonder whether there is any correctness issue if not forwarding
> > partial request to iommufd. If not this can also help reduce
> > notifications to the user until the group is ready.
>
> I don't think there's any correctness issue. But it should be better if
> we can inject the page faults to vm guests as soon as possible. There's
> no requirement to put page requests to vIOMMU's hardware page request
> queue at the granularity of a fault group. Thoughts?
Not sure I understand you correctly, but we can't inject partial fault
groups: if the HW PRI queue overflows, the last fault in a group may be
lost, so the non-last faults in that group already injected won't be
completed (until PRGI reuse), leaking PRI request credits and guest
resources.
Thanks,
Jean
On 2023/7/12 17:45, Jean-Philippe Brucker wrote:
> On Wed, Jul 12, 2023 at 10:32:13AM +0800, Baolu Lu wrote:
>>> btw is there value of moving the group handling logic from
>>> iommu_queue_iopf() to this common function?
>>>
>>> I wonder whether there is any correctness issue if not forwarding
>>> partial request to iommufd. If not this can also help reduce
>>> notifications to the user until the group is ready.
>>
>> I don't think there's any correctness issue. But it should be better if
>> we can inject the page faults to vm guests as soon as possible. There's
>> no requirement to put page requests to vIOMMU's hardware page request
>> queue at the granularity of a fault group. Thoughts?
>
> Not sure I understand you correctly, but we can't inject partial fault
> groups: if the HW PRI queue overflows, the last fault in a group may be
> lost, so the non-last faults in that group already injected won't be
> completed (until PRGI reuse), leaking PRI request credits and guest
> resources.
Yeah, that's how vIOMMU injects the faults. On host/hypervisor side, my
understanding is that faults should be uploaded as soon as possible.
Best regards,
baolu