[3/9] iommu: Add common code to handle IO page faults

  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 <baolu.lu@linux.intel.com>
---
 drivers/iommu/iommu.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)
  

> From: Lu Baolu <baolu.lu@linux.intel.com>
> 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 <baolu.lu@linux.intel.com>
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 <baolu.lu@linux.intel.com>
> ---
>  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 <baolu.lu@linux.intel.com>
>> 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 <baolu.lu@linux.intel.com>
> 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 <baolu.lu@linux.intel.com>
>> ---
>>   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