Re: [PATCH v3 17/19] iommu/amd: Access/Dirty bit support in IOPTEs

Suthikulpanit, Suravee
2023-10-18T13:04:25Z

Joao,

On 10/17/2023 4:54 PM, Joao Martins wrote:
>>> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
>>> index af36c627022f..31b333cc6fe1 100644
>>> --- a/drivers/iommu/amd/iommu.c
>>> +++ b/drivers/iommu/amd/iommu.c
>>> ....
>>> @@ -2156,11 +2160,17 @@ static inline u64 dma_max_address(void)
>>>       return ((1ULL << PM_LEVEL_SHIFT(amd_iommu_gpt_level)) - 1);
>>>   }
>>>   +static bool amd_iommu_hd_support(struct amd_iommu *iommu)
>>> +{
>>> +    return iommu && (iommu->features & FEATURE_HDSUP);
>>> +}
>>> +
>> You can use the newly introduced check_feature(u64 mask) to check the HD support.
>>
> It appears that the check_feature() is logically equivalent to
> check_feature_on_all_iommus(); where this check is per-device/per-iommu check to
> support potentially nature of different IOMMUs with different features. Being
> per-IOMMU would allow you to have firmware to not advertise certain IOMMU
> features on some devices while still supporting for others. I understand this is
> not a thing in x86, but the UAPI supports it. Having said that, you still want
> me to switch to check_feature() ?

So far, AMD does not have system w/ multiple IOMMUs, which have
different EFR/EFR2. However, the AMD IOMMU spec does not enforce
EFR/EFR2 of all IOMMU instances to be the same. There are certain
features, which require consistent support across all IOMMUs. That's why
we introduced the system-wide amd_iommu_efr / amd_iommu_efr2 to simpify
feature checking logic in the driver.

For EFR[HDSup], let's consider a VM with two VFIO pass-through devices
(dev_A and dev_B). Each device is on different IOMMU instance (IOMMU_A,
IOMMU_B), where only IOMMU_A has EFR[HDSUP]=1.

If we call do_iommu_domain_alloc(type, dev_A,
IOMMU_HWPT_ALLOC_ENFORCE_DIRTY), this should return a domain w/
dirty_ops set. Then, if we attach dev_B to the same domain, the
following check should return -EINVAL.

@@ -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.

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.

Thanks,
Suravee