Re: [PATCH v3 19/19] iommu/intel: Access/Dirty bit support for SL domains

Baolu Lu
2023-10-18T02:09:59Z

On 10/17/23 10:25 PM, Joao Martins wrote:
> On 17/10/2023 15:16, Joao Martins wrote:
>> On 17/10/2023 13:41, Baolu Lu wrote:
>>> On 2023/10/17 18:51, Joao Martins wrote:
>>>> On 16/10/2023 14:01, Baolu Lu wrote:
>>>>> On 2023/10/16 20:59, Jason Gunthorpe wrote:
>>>>>> On Mon, Oct 16, 2023 at 08:58:42PM +0800, Baolu Lu wrote:
>>>>>>> On 2023/10/16 19:42, Jason Gunthorpe wrote:
>>>>>>>> On Mon, Oct 16, 2023 at 11:57:34AM +0100, Joao Martins wrote:
>>>>>>>>
>>>>>>>>> True. But to be honest, I thought we weren't quite there yet in PASID
>>>>>>>>> support
>>>>>>>>> from IOMMUFD perspective; hence why I didn't aim at it. Or do I have the
>>>>>>>>> wrong
>>>>>>>>> impression? From the code below, it clearly looks the driver does.
>>>>>>>> I think we should plan that this series will go before the PASID
>>>>>>>> series
>>>>>>> I know that PASID support in IOMMUFD is not yet available, but the VT-d
>>>>>>> driver already supports attaching a domain to a PASID, as required by
>>>>>>> the idxd driver for kernel DMA with PASID. Therefore, from the driver's
>>>>>>> perspective, dirty tracking should also be enabled for PASIDs.
>>>>>> As long as the driver refuses to attach a dirty track enabled domain
>>>>>> to PASID it would be fine for now.
>>>>> Yes. This works.
>>>> Baolu Lu, I am blocking PASID attachment this way; let me know if this matches
>>>> how would you have the driver refuse dirty tracking on PASID.
>>>
>>> Joao, how about blocking pasid attachment in intel_iommu_set_dev_pasid()
>>> directly?
>>>
>>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>>> index c86ba5a3e75c..392b6ca9ce90 100644
>>> --- a/drivers/iommu/intel/iommu.c
>>> +++ b/drivers/iommu/intel/iommu.c
>>> @@ -4783,6 +4783,9 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain
>>> *domain,
>>>         if (context_copied(iommu, info->bus, info->devfn))
>>>                 return -EBUSY;
>>>
>>> +       if (domain->dirty_ops)
>>> +               return -EOPNOTSUPP;
>>> +
>>>         ret = prepare_domain_attach_device(domain, dev);
>>>         if (ret)
>>>                 return ret;
>>
>> I was trying to centralize all the checks, but I can place it here if you prefer
>> this way.

We will soon remove this check when pasid is supported in iommufd. So
less code change is better for future work.

>>
> Minor change, I'm changing error code to -EINVAL to align with non-PASID case.

Yes. Make sense. -EINVAL means "not compatible". The caller can retry.

Best regards,
baolu