On 18/10/2023 14:17, Joao Martins wrote:
> On 18/10/2023 14:04, Suthikulpanit, Suravee wrote:
>> On 10/17/2023 4:54 PM, Joao Martins wrote:
>> @@ -2252,6 +2268,9 @@ static int amd_iommu_attach_device(struct iommu_domain *dom,
>> return 0;
>>
>> dev_data->defer_attach = false;
>> + if (dom->dirty_ops && iommu &&
>> + !(iommu->features & FEATURE_HDSUP))
>> + return -EINVAL;
>>
>> which means dev_A and dev_B cannot be in the same VFIO domain.
>>
> Correct; and that's by design.
>
>> In this case, since we can prevent devices on IOMMUs w/ different EFR[HDSUP] bit
>> to share the same domain, it should be safe to support dirty tracking on such
>> system, and it makes sense to just check the per-IOMMU EFR value (i.e.
>> iommu->features). If we decide to keep this, we probably should put comment in
>> the code to describe this.
>
> OK, I'll leave a comment, but note that this isn't an odd case, we are supposed
> to enforce dirty tracking on the iommu domain, and then nack any non-supported
> IOMMUs such taht only devices with dirty-tracking supported IOMMUs are present.
> But the well-behaved userspace app will also check accordingly if the capability
> is there in the IOMMU.
>
FWIW, I added this comment:
@@ -2254,6 +2270,13 @@ static int amd_iommu_attach_device(struct iommu_domain *dom,
dev_data->defer_attach = false;
+ /*
+ * Restrict to devices with compatible IOMMU hardware support
+ * when enforcement of dirty tracking is enabled.
+ */
> Btw, I understand that this is not the case for current AMD systems where
> everything is homogeneous. We can simplify this with check_feature() afterwards,
> but the fact that the spec doesn't prevent it makes me wonder too :)
