Re: [PATCH v2 01/22] iommu: Add iommu_user_domain_alloc() interface

[Baolu Lu <baolu.lu@linux.intel.com>]

On 6/4/24 4:03 PM, Yi Liu wrote:
> On 2024/6/4 09:51, Lu Baolu wrote:
>> Commit <909f4abd1097> ("iommu: Add new iommu op to create domains owned
>> by userspace") added a dedicated iommu op to allocate a user domain.
>> While IOMMUFD has already made use of this callback, other frameworks
>> like vfio/type1 and vDPA still use the paging domain allocation 
>> interface.
>>
>> Add a new interface named iommu_user_domain_alloc(), which indicates the
>> allocation of a domain for device DMA managed by user space driver. All
>> device passthrough frameworks could use this interface for their domain
>> allocation.
>>
>> Although it is expected that all iommu drivers could implement their own
>> domain_alloc_user ops, most drivers haven't implemented it yet. Rollback
>> to the paging domain allocation interface if the iommu driver hasn't
>> implemented this op yet.
>>
>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>> ---
>>   include/linux/iommu.h |  6 ++++++
>>   drivers/iommu/iommu.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 48 insertions(+)
>>
>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>> index 7bc8dff7cf6d..6648b2415474 100644
>> --- a/include/linux/iommu.h
>> +++ b/include/linux/iommu.h
>> @@ -780,6 +780,7 @@ extern bool iommu_present(const struct bus_type 
>> *bus);
>>   extern bool device_iommu_capable(struct device *dev, enum iommu_cap 
>> cap);
>>   extern bool iommu_group_has_isolated_msi(struct iommu_group *group);
>>   extern struct iommu_domain *iommu_domain_alloc(const struct bus_type 
>> *bus);
>> +struct iommu_domain *iommu_user_domain_alloc(struct device *dev, u32 
>> flags);
>>   extern void iommu_domain_free(struct iommu_domain *domain);
>>   extern int iommu_attach_device(struct iommu_domain *domain,
>>                      struct device *dev);
>> @@ -1086,6 +1087,11 @@ static inline struct iommu_domain 
>> *iommu_domain_alloc(const struct bus_type *bus
>>       return NULL;
>>   }
>> +static inline struct iommu_domain *iommu_user_domain_alloc(struct 
>> device *dev, u32 flags)
>> +{
>> +    return ERR_PTR(-ENODEV);
>> +}
>> +
>>   static inline void iommu_domain_free(struct iommu_domain *domain)
>>   {
>>   }
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 9df7cc75c1bc..f1416892ef8e 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -2032,6 +2032,48 @@ struct iommu_domain *iommu_domain_alloc(const 
>> struct bus_type *bus)
>>   }
>>   EXPORT_SYMBOL_GPL(iommu_domain_alloc);
>> +/**
>> + * iommu_user_domain_alloc() - Allocate a user domain
>> + * @dev: device for which the domain is allocated
>> + * @flags: iommufd_hwpt_alloc_flags defined in uapi/linux/iommufd.h
>> + *
>> + * Allocate a user domain which will be managed by a userspace 
>> driver. Return
>> + * allocated domain if successful, or a ERR pointer for failure.
> 
> do you want to mention that this is for paging domain allocation?

You are worrying about its confusion with nesting domain allocation,
right? My understanding is that if we want a common interface for
nesting domain allocation, we should make it in another interface. Here,
the user domain is a paging domain for GVA->HPA mapping, which is common
across iommufd, vfio, and vdpa.

> 
>> + */
>> +struct iommu_domain *iommu_user_domain_alloc(struct device *dev, u32 
>> flags)
>> +{
>> +    struct iommu_domain *domain;
>> +    const struct iommu_ops *ops;
>> +
>> +    if (!dev_has_iommu(dev))
>> +        return ERR_PTR(-ENODEV);
>> +
>> +    ops = dev_iommu_ops(dev);
>> +    if (ops->domain_alloc_user) {
>> +        domain = ops->domain_alloc_user(dev, flags, NULL, NULL);
>> +        if (IS_ERR(domain))
>> +            return domain;
>> +
>> +        domain->type = IOMMU_DOMAIN_UNMANAGED;
>> +        domain->owner = ops;
>> +        domain->pgsize_bitmap = ops->pgsize_bitmap;
> 
> this seems to break the iommufd selftest as the mock driver sets extra
> bits in the domain->pgsize_bitmap in allocation. Override it may fail
> something in the testing. you may need to check if domain->pgsize_bitmap
> is set or use &=.
> 
> static struct iommu_domain *mock_domain_alloc_paging(struct device *dev)
> {
>      struct mock_dev *mdev = container_of(dev, struct mock_dev, dev);
>      struct mock_iommu_domain *mock;
> 
>      mock = kzalloc(sizeof(*mock), GFP_KERNEL);
>      if (!mock)
>          return NULL;
>      mock->domain.geometry.aperture_start = MOCK_APERTURE_START;
>      mock->domain.geometry.aperture_end = MOCK_APERTURE_LAST;
>      mock->domain.pgsize_bitmap = MOCK_IO_PAGE_SIZE;
>      if (dev && mdev->flags & MOCK_FLAGS_DEVICE_HUGE_IOVA)
>          mock->domain.pgsize_bitmap |= MOCK_HUGE_PAGE_SIZE;
>      mock->domain.ops = mock_ops.default_domain_ops;
>      mock->domain.type = IOMMU_DOMAIN_UNMANAGED;
>      xa_init(&mock->pfns);
>      return &mock->domain;
> }

You are right. I should follow the code in __iommu_domain_alloc()

         /*
          * If not already set, assume all sizes by default; the driver
          * may override this later
          */
         if (!domain->pgsize_bitmap)
                 domain->pgsize_bitmap = ops->pgsize_bitmap;

Does it work?

Best regards,
baolu