On 18/10/2023 14:04, Suthikulpanit, Suravee wrote:
> 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.
>
Ack
> 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.
>
True; this is what it is in this patch.
> @@ -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.
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 :)
