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

[Jason Gunthorpe <jgg@nvidia.com>]

On Wed, Oct 18, 2023 at 08:04:07PM +0700, Suthikulpanit, Suravee wrote:

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

I've argued this seems like a shortcut. The general design of the
iommu subsystem has everything be per-iommu and the only cross
instance sharing should be with the domain.

I understood AMD had some global things where it needed to interact
with the other arch code that had to be global.

However, at least for domain centric features this is how things
should be coded in drivers - store the data in the domain and reject
attach to incompatible iommus. Try to minimize the use of globals.

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

Which is correct, the HW cannot support dirty tracking.

The VMM world can handle this, it knows that the domain is
incompatible so it can choose to use another way to do dirty tracking
and associate a non-dirty tracking domain to this device. Or decide to
give up.

Other platforms will require this code anyhow as they don't have the
guarentee of uniformity.

Jason