[PATCH] iommufd: Deal with IOMMU_HWPT_FAULT_ID_VALID in iommufd core

[PATCH] iommufd: Deal with IOMMU_HWPT_FAULT_ID_VALID in iommufd core

[Yi Liu]

IOMMU_HWPT_FAULT_ID_VALID is used to mark if the fault_id field of
iommu_hwp_alloc is valid or not. As the fault_id field is handled in
the iommufd core, so it makes sense to sanitize the
IOMMU_HWPT_FAULT_ID_VALID flag in the iommufd core, and mask it out
before passing the user flags to the iommu drivers.

Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c |  8 +-------
 drivers/iommu/intel/iommu.c                         |  3 +--
 drivers/iommu/iommufd/hw_pagetable.c                | 10 +++++++---
 drivers/iommu/iommufd/selftest.c                    |  2 +-
 4 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
index 6cc14d82399f..72050bb507fd 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
@@ -178,18 +178,12 @@ arm_vsmmu_alloc_domain_nested(struct iommufd_viommu *viommu, u32 flags,
 			      const struct iommu_user_data *user_data)
 {
 	struct arm_vsmmu *vsmmu = container_of(viommu, struct arm_vsmmu, core);
-	const u32 SUPPORTED_FLAGS = IOMMU_HWPT_FAULT_ID_VALID;
 	struct arm_smmu_nested_domain *nested_domain;
 	struct iommu_hwpt_arm_smmuv3 arg;
 	bool enable_ats = false;
 	int ret;
 
-	/*
-	 * Faults delivered to the nested domain are faults that originated by
-	 * the S1 in the domain. The core code will match all PASIDs when
-	 * delivering the fault due to user_pasid_table
-	 */
-	if (flags & ~SUPPORTED_FLAGS)
+	if (flags)
 		return ERR_PTR(-EOPNOTSUPP);
 
 	ret = iommu_copy_struct_from_user(&arg, user_data,
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 7d0acb74d5a5..c8f9c70a04ab 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -3340,8 +3340,7 @@ intel_iommu_domain_alloc_paging_flags(struct device *dev, u32 flags,
 	bool first_stage;
 
 	if (flags &
-	    (~(IOMMU_HWPT_ALLOC_NEST_PARENT | IOMMU_HWPT_ALLOC_DIRTY_TRACKING
-	       | IOMMU_HWPT_FAULT_ID_VALID)))
+	    (~(IOMMU_HWPT_ALLOC_NEST_PARENT | IOMMU_HWPT_ALLOC_DIRTY_TRACKING)))
 		return ERR_PTR(-EOPNOTSUPP);
 	if (nested_parent && !nested_supported(iommu))
 		return ERR_PTR(-EOPNOTSUPP);
diff --git a/drivers/iommu/iommufd/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c
index ce03c3804651..598be26a14e2 100644
--- a/drivers/iommu/iommufd/hw_pagetable.c
+++ b/drivers/iommu/iommufd/hw_pagetable.c
@@ -140,8 +140,8 @@ iommufd_hwpt_paging_alloc(struct iommufd_ctx *ictx, struct iommufd_ioas *ioas,
 	hwpt_paging->nest_parent = flags & IOMMU_HWPT_ALLOC_NEST_PARENT;
 
 	if (ops->domain_alloc_paging_flags) {
-		hwpt->domain = ops->domain_alloc_paging_flags(idev->dev, flags,
-							      user_data);
+		hwpt->domain = ops->domain_alloc_paging_flags(idev->dev,
+				flags & ~IOMMU_HWPT_FAULT_ID_VALID, user_data);
 		if (IS_ERR(hwpt->domain)) {
 			rc = PTR_ERR(hwpt->domain);
 			hwpt->domain = NULL;
@@ -280,6 +280,8 @@ iommufd_viommu_alloc_hwpt_nested(struct iommufd_viommu *viommu, u32 flags,
 	struct iommufd_hw_pagetable *hwpt;
 	int rc;
 
+	if (flags & ~IOMMU_HWPT_FAULT_ID_VALID)
+		return ERR_PTR(-EOPNOTSUPP);
 	if (!user_data->len)
 		return ERR_PTR(-EOPNOTSUPP);
 	if (!viommu->ops || !viommu->ops->alloc_domain_nested)
@@ -296,7 +298,9 @@ iommufd_viommu_alloc_hwpt_nested(struct iommufd_viommu *viommu, u32 flags,
 	hwpt_nested->parent = viommu->hwpt;
 
 	hwpt->domain =
-		viommu->ops->alloc_domain_nested(viommu, flags, user_data);
+		viommu->ops->alloc_domain_nested(viommu,
+				flags & ~IOMMU_HWPT_FAULT_ID_VALID,
+				user_data);
 	if (IS_ERR(hwpt->domain)) {
 		rc = PTR_ERR(hwpt->domain);
 		hwpt->domain = NULL;
diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c
index a0de6d6d4e68..658fecae3464 100644
--- a/drivers/iommu/iommufd/selftest.c
+++ b/drivers/iommu/iommufd/selftest.c
@@ -595,7 +595,7 @@ mock_viommu_alloc_domain_nested(struct iommufd_viommu *viommu, u32 flags,
 	struct mock_viommu *mock_viommu = to_mock_viommu(viommu);
 	struct mock_iommu_domain_nested *mock_nested;
 
-	if (flags & ~IOMMU_HWPT_FAULT_ID_VALID)
+	if (flags)
 		return ERR_PTR(-EOPNOTSUPP);
 
 	mock_nested = __mock_domain_alloc_nested(user_data);
-- 
2.34.1

Re: [PATCH] iommufd: Deal with IOMMU_HWPT_FAULT_ID_VALID in iommufd core

[Baolu Lu]

On 12/7/24 20:01, Yi Liu wrote:
> IOMMU_HWPT_FAULT_ID_VALID is used to mark if the fault_id field of
> iommu_hwp_alloc is valid or not. As the fault_id field is handled in
> the iommufd core, so it makes sense to sanitize the
> IOMMU_HWPT_FAULT_ID_VALID flag in the iommufd core, and mask it out
> before passing the user flags to the iommu drivers.

Is it a valid use case for an iommu driver to intercept the
IOMMU_HWPT_FAULT_ID_VALID flag as an indication that user space requests
an iopf-capable domain?

If the device and its associated iommu do not support PRI, the domain
allocation should be aborted and a failure returned?

Thanks,
baolu

Re: [PATCH] iommufd: Deal with IOMMU_HWPT_FAULT_ID_VALID in iommufd core

[Yi Liu]

On 2024/12/9 09:08, Baolu Lu wrote:
> On 12/7/24 20:01, Yi Liu wrote:
>> IOMMU_HWPT_FAULT_ID_VALID is used to mark if the fault_id field of
>> iommu_hwp_alloc is valid or not. As the fault_id field is handled in
>> the iommufd core, so it makes sense to sanitize the
>> IOMMU_HWPT_FAULT_ID_VALID flag in the iommufd core, and mask it out
>> before passing the user flags to the iommu drivers.
> 
> Is it a valid use case for an iommu driver to intercept the
> IOMMU_HWPT_FAULT_ID_VALID flag as an indication that user space requests
> an iopf-capable domain? >
> If the device and its associated iommu do not support PRI, the domain
> allocation should be aborted and a failure returned?
> 

Agreed, it is better to fail the hwpt allocation. BTW. We may also check it
by reading the enable bit in PCI config space of the device. Which one
would be better?

Regards,
Yi Liu

Re: [PATCH] iommufd: Deal with IOMMU_HWPT_FAULT_ID_VALID in iommufd core

[Jason Gunthorpe]

On Mon, Dec 09, 2024 at 10:51:51AM +0800, Yi Liu wrote:
> On 2024/12/9 09:08, Baolu Lu wrote:
> > On 12/7/24 20:01, Yi Liu wrote:
> > > IOMMU_HWPT_FAULT_ID_VALID is used to mark if the fault_id field of
> > > iommu_hwp_alloc is valid or not. As the fault_id field is handled in
> > > the iommufd core, so it makes sense to sanitize the
> > > IOMMU_HWPT_FAULT_ID_VALID flag in the iommufd core, and mask it out
> > > before passing the user flags to the iommu drivers.
> > 
> > Is it a valid use case for an iommu driver to intercept the
> > IOMMU_HWPT_FAULT_ID_VALID flag as an indication that user space requests
> > an iopf-capable domain?

For now we have no use case, so we can drop it from drivers, I think.

PRI support or not is decided when the domain is attached based on the
fault_handler in the struct iommu_domain.

As a general rule drivers should not know the *userspace* is doing
something as much as we can manage.

> > If the device and its associated iommu do not support PRI, the domain
> > allocation should be aborted and a failure returned?
 
> Agreed, it is better to fail the hwpt allocation. BTW. We may also check it
> by reading the enable bit in PCI config space of the device. Which one
> would be better?

We should not be reading pci config space in iommufd. iommufd is
generic and not tied to PCI.

It would be nice if domain allocation fails, but also I wouldn't
contort things to achieve that. Failing attach is also OK and must
also be supported as a fault capable domain allocated on a fault
capable device can still fail to be attached to a non fault capable
device.

"fault capable" is complicated unfortunately because we have driver
specific page fault reporting, and it may be that vfio userspace is
processing fault...

Jason

Re: [PATCH] iommufd: Deal with IOMMU_HWPT_FAULT_ID_VALID in iommufd core

[Yi Liu]

On 2024/12/9 23:05, Jason Gunthorpe wrote:
> On Mon, Dec 09, 2024 at 10:51:51AM +0800, Yi Liu wrote:
>> On 2024/12/9 09:08, Baolu Lu wrote:
>>> On 12/7/24 20:01, Yi Liu wrote:
>>>> IOMMU_HWPT_FAULT_ID_VALID is used to mark if the fault_id field of
>>>> iommu_hwp_alloc is valid or not. As the fault_id field is handled in
>>>> the iommufd core, so it makes sense to sanitize the
>>>> IOMMU_HWPT_FAULT_ID_VALID flag in the iommufd core, and mask it out
>>>> before passing the user flags to the iommu drivers.
>>>
>>> Is it a valid use case for an iommu driver to intercept the
>>> IOMMU_HWPT_FAULT_ID_VALID flag as an indication that user space requests
>>> an iopf-capable domain?
> 
> For now we have no use case, so we can drop it from drivers, I think.
> 
> PRI support or not is decided when the domain is attached based on the
> fault_handler in the struct iommu_domain.

hmmm. It sounds reasonable. But I failed to see such checks in iommu
drivers. So attaching a non-fault capable domain to a device that has
enabled PRI can succeed. But the PRQs from this device will be responded
as bad PRQ. Might need a patch to fix this gap. is it?

> As a general rule drivers should not know the *userspace* is doing
> something as much as we can manage.
> 
>>> If the device and its associated iommu do not support PRI, the domain
>>> allocation should be aborted and a failure returned?
>   
>> Agreed, it is better to fail the hwpt allocation. BTW. We may also check it
>> by reading the enable bit in PCI config space of the device. Which one
>> would be better?
> 
> We should not be reading pci config space in iommufd. iommufd is
> generic and not tied to PCI.

I see. :)

> It would be nice if domain allocation fails, but also I wouldn't
> contort things to achieve that. Failing attach is also OK and must
> also be supported as a fault capable domain allocated on a fault
> capable device can still fail to be attached to a non fault capable
> device.

yes, iommu driver should be able to fail such attempts in attach phase.
So I may keep this patch as it is since IOMMU_HWPT_FAULT_ID_VALID is not
supposed to be used by iommu drivers.

Regards,
Yi Liu

Re: [PATCH] iommufd: Deal with IOMMU_HWPT_FAULT_ID_VALID in iommufd core

[Jason Gunthorpe]

On Tue, Dec 10, 2024 at 01:52:28PM +0800, Yi Liu wrote:
> On 2024/12/9 23:05, Jason Gunthorpe wrote:
> > On Mon, Dec 09, 2024 at 10:51:51AM +0800, Yi Liu wrote:
> > > On 2024/12/9 09:08, Baolu Lu wrote:
> > > > On 12/7/24 20:01, Yi Liu wrote:
> > > > > IOMMU_HWPT_FAULT_ID_VALID is used to mark if the fault_id field of
> > > > > iommu_hwp_alloc is valid or not. As the fault_id field is handled in
> > > > > the iommufd core, so it makes sense to sanitize the
> > > > > IOMMU_HWPT_FAULT_ID_VALID flag in the iommufd core, and mask it out
> > > > > before passing the user flags to the iommu drivers.
> > > > 
> > > > Is it a valid use case for an iommu driver to intercept the
> > > > IOMMU_HWPT_FAULT_ID_VALID flag as an indication that user space requests
> > > > an iopf-capable domain?
> > 
> > For now we have no use case, so we can drop it from drivers, I think.
> > 
> > PRI support or not is decided when the domain is attached based on the
> > fault_handler in the struct iommu_domain.
> 
> hmmm. It sounds reasonable. But I failed to see such checks in iommu
> drivers. 

It would not surprise me, but also we are deliberately being very lax
on fault support to allow devices with internal fault queues.

> So attaching a non-fault capable domain to a device that has
> enabled PRI can succeed. 

Yes

> But the PRQs from this device will be responded as bad PRQ.

The core code should be doing this in the fault handler when it
fails to find an appropriate domain to deliver the fault to.

Inside iommu_report_device_fault() find_fault_handler() will fail:

	if (!attach_handle->domain->iopf_handler)
		return NULL;

Which should permanently abort the PRI to the device.

> yes, iommu driver should be able to fail such attempts in attach phase.
> So I may keep this patch as it is since IOMMU_HWPT_FAULT_ID_VALID is not
> supposed to be used by iommu drivers.

It makes sense

IMHO we don't have a clear set of rules when the iommu driver should
permit a fault capable domain to be attached. We don't have any API to
tell if the device will handle faults internally or if the device
requires PRI.

A device that can do internal faults only requires ATS support from
the iommu. Otherwise you need PRI enabled at the device and PRI
support in the iommu HW.

Then there is the special ARM stall mode too..

For now I would leave this as is.

Jason

RE: [PATCH] iommufd: Deal with IOMMU_HWPT_FAULT_ID_VALID in iommufd core

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Tuesday, December 10, 2024 1:52 PM
> 
> On 2024/12/9 23:05, Jason Gunthorpe wrote:
> > On Mon, Dec 09, 2024 at 10:51:51AM +0800, Yi Liu wrote:
> >> On 2024/12/9 09:08, Baolu Lu wrote:
> >>> On 12/7/24 20:01, Yi Liu wrote:
> >>>> IOMMU_HWPT_FAULT_ID_VALID is used to mark if the fault_id field of
> >>>> iommu_hwp_alloc is valid or not. As the fault_id field is handled in
> >>>> the iommufd core, so it makes sense to sanitize the
> >>>> IOMMU_HWPT_FAULT_ID_VALID flag in the iommufd core, and mask it
> out
> >>>> before passing the user flags to the iommu drivers.
> >>>
> >>> Is it a valid use case for an iommu driver to intercept the
> >>> IOMMU_HWPT_FAULT_ID_VALID flag as an indication that user space
> requests
> >>> an iopf-capable domain?
> >
> > For now we have no use case, so we can drop it from drivers, I think.
> >
> > PRI support or not is decided when the domain is attached based on the
> > fault_handler in the struct iommu_domain.
> 
> hmmm. It sounds reasonable. But I failed to see such checks in iommu
> drivers. So attaching a non-fault capable domain to a device that has
> enabled PRI can succeed. But the PRQs from this device will be responded
> as bad PRQ. Might need a patch to fix this gap. is it?
> 

PRI is enabled when the device is attached to a fault-capable domain:

static inline int iommufd_hwpt_attach_device(struct iommufd_hw_pagetable *hwpt,
                                             struct iommufd_device *idev)
{
        if (hwpt->fault)
                return iommufd_fault_domain_attach_dev(hwpt, idev);

        return iommu_attach_group(hwpt->domain, idev->igroup->group);
}

int iommufd_fault_domain_attach_dev(struct iommufd_hw_pagetable *hwpt,
                                    struct iommufd_device *idev)
{
        int ret;

        if (!hwpt->fault)
                return -EINVAL;               

        ret = iommufd_fault_iopf_enable(idev);
        if (ret)
                return ret;

Re: [PATCH] iommufd: Deal with IOMMU_HWPT_FAULT_ID_VALID in iommufd core

[Yi Liu]

On 2024/12/11 16:58, Tian, Kevin wrote:
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Sent: Tuesday, December 10, 2024 1:52 PM
>>
>> On 2024/12/9 23:05, Jason Gunthorpe wrote:
>>> On Mon, Dec 09, 2024 at 10:51:51AM +0800, Yi Liu wrote:
>>>> On 2024/12/9 09:08, Baolu Lu wrote:
>>>>> On 12/7/24 20:01, Yi Liu wrote:
>>>>>> IOMMU_HWPT_FAULT_ID_VALID is used to mark if the fault_id field of
>>>>>> iommu_hwp_alloc is valid or not. As the fault_id field is handled in
>>>>>> the iommufd core, so it makes sense to sanitize the
>>>>>> IOMMU_HWPT_FAULT_ID_VALID flag in the iommufd core, and mask it
>> out
>>>>>> before passing the user flags to the iommu drivers.
>>>>>
>>>>> Is it a valid use case for an iommu driver to intercept the
>>>>> IOMMU_HWPT_FAULT_ID_VALID flag as an indication that user space
>> requests
>>>>> an iopf-capable domain?
>>>
>>> For now we have no use case, so we can drop it from drivers, I think.
>>>
>>> PRI support or not is decided when the domain is attached based on the
>>> fault_handler in the struct iommu_domain.
>>
>> hmmm. It sounds reasonable. But I failed to see such checks in iommu
>> drivers. So attaching a non-fault capable domain to a device that has
>> enabled PRI can succeed. But the PRQs from this device will be responded
>> as bad PRQ. Might need a patch to fix this gap. is it?
>>
> 
> PRI is enabled when the device is attached to a fault-capable domain:

I see. So attach would be failed if device/iommu does not support it.

> static inline int iommufd_hwpt_attach_device(struct iommufd_hw_pagetable *hwpt,
>                                               struct iommufd_device *idev)
> {
>          if (hwpt->fault)
>                  return iommufd_fault_domain_attach_dev(hwpt, idev);
> 
>          return iommu_attach_group(hwpt->domain, idev->igroup->group);
> }
> 
> int iommufd_fault_domain_attach_dev(struct iommufd_hw_pagetable *hwpt,
>                                      struct iommufd_device *idev)
> {
>          int ret;
> 
>          if (!hwpt->fault)
>                  return -EINVAL;
> 
>          ret = iommufd_fault_iopf_enable(idev);
>          if (ret)
>                  return ret;
> 

-- 
Regards,
Yi Liu

RE: [PATCH] iommufd: Deal with IOMMU_HWPT_FAULT_ID_VALID in iommufd core

[Tian, Kevin]

> From: Yi Liu <yi.l.liu@intel.com>
> Sent: Saturday, December 7, 2024 8:01 PM
>
> IOMMU_HWPT_FAULT_ID_VALID is used to mark if the fault_id field of
> iommu_hwp_alloc is valid or not. As the fault_id field is handled in
> the iommufd core, so it makes sense to sanitize the
> IOMMU_HWPT_FAULT_ID_VALID flag in the iommufd core, and mask it out
> before passing the user flags to the iommu drivers.
>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

Re: [PATCH] iommufd: Deal with IOMMU_HWPT_FAULT_ID_VALID in iommufd core

[Jason Gunthorpe]

On Sat, Dec 07, 2024 at 04:01:08AM -0800, Yi Liu wrote:
> IOMMU_HWPT_FAULT_ID_VALID is used to mark if the fault_id field of
> iommu_hwp_alloc is valid or not. As the fault_id field is handled in
> the iommufd core, so it makes sense to sanitize the
> IOMMU_HWPT_FAULT_ID_VALID flag in the iommufd core, and mask it out
> before passing the user flags to the iommu drivers.
> 
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c |  8 +-------
>  drivers/iommu/intel/iommu.c                         |  3 +--
>  drivers/iommu/iommufd/hw_pagetable.c                | 10 +++++++---
>  drivers/iommu/iommufd/selftest.c                    |  2 +-
>  4 files changed, 10 insertions(+), 13 deletions(-)

I'm going to take this, but there is still something missing - drivers
do not check domain->iopf_handler during attach and will happily
install these domains without any HW support for fault reporting.

Not sure this is so great.. I suggest adding a flag to the iommu_ops
'supports_fault_reporting', as just a global flag if the driver could
*possibly* support this. Core code will fail attach (and maybe alloc?)
if it is not set. If it is set driver must check domain->iopf_handler
during attach against HW capability.

Jason

Re: [PATCH] iommufd: Deal with IOMMU_HWPT_FAULT_ID_VALID in iommufd core

[Yi Liu]

On 2024/12/12 04:02, Jason Gunthorpe wrote:
> On Sat, Dec 07, 2024 at 04:01:08AM -0800, Yi Liu wrote:
>> IOMMU_HWPT_FAULT_ID_VALID is used to mark if the fault_id field of
>> iommu_hwp_alloc is valid or not. As the fault_id field is handled in
>> the iommufd core, so it makes sense to sanitize the
>> IOMMU_HWPT_FAULT_ID_VALID flag in the iommufd core, and mask it out
>> before passing the user flags to the iommu drivers.
>>
>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>> ---
>>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c |  8 +-------
>>   drivers/iommu/intel/iommu.c                         |  3 +--
>>   drivers/iommu/iommufd/hw_pagetable.c                | 10 +++++++---
>>   drivers/iommu/iommufd/selftest.c                    |  2 +-
>>   4 files changed, 10 insertions(+), 13 deletions(-)
> 
> I'm going to take this, but there is still something missing - drivers
> do not check domain->iopf_handler during attach and will happily
> install these domains without any HW support for fault reporting.

Apologize about it, per Kevin's remark[1] attach fault-capable domain to
non-PRI capable device shall fail when trying to enable PRI on the
devices that do not support it. So the current code does not have the
gap I mentioned in prior email [2].

[1] 
https://lore.kernel.org/linux-iommu/BN9PR11MB5276FE72F52332E1BD40AE288C3E2@BN9PR11MB5276.namprd11.prod.outlook.com/
[2] 
https://lore.kernel.org/linux-iommu/95a85b9b-a17b-4df5-a1ac-7a0880950960@intel.com/

> Not sure this is so great.. I suggest adding a flag to the iommu_ops
> 'supports_fault_reporting', as just a global flag if the driver could
> *possibly* support this. Core code will fail attach (and maybe alloc?)
> if it is not set. If it is set driver must check domain->iopf_handler
> during attach against HW capability.
> 

This flag should be per device I guess. But as above remark, it's not
urgent to add this for now. :)

-- 
Regards,
Yi Liu