On Tue, Feb 18, 2025 at 10:10:00PM +1100, Alexey Kardashevskiy wrote:
> @@ -939,6 +939,7 @@ struct iommu_fault_alloc {
> enum iommu_viommu_type {
> IOMMU_VIOMMU_TYPE_DEFAULT = 0,
> IOMMU_VIOMMU_TYPE_ARM_SMMUV3 = 1,
> + IOMMU_VIOMMU_TYPE_TSM = 2,
This should probably be some kind of AMD_TSM and the driver data blob
should carry any additional data needed to create the vIOMMU that is
visible to the guest.
> @@ -2068,7 +2069,18 @@ static void set_dte_entry(struct amd_iommu *iommu,
> new.data[1] |= DTE_FLAG_IOTLB;
>
> old_domid = READ_ONCE(dte->data[1]) & DEV_DOMID_MASK;
> - new.data[1] |= domid;
> +
> + if (domain->aviommu) {
AMD should be implementing viommu natively without CC as well, try to
structure things so it fits together better. This should only trigger
for the CC viommu type..
> + /*
> + * This runs when VFIO is bound to a device but TDI is not yet.
> + * Ideally TSM should change DTE only when TDI is bound.
> + */
> + dev_info(dev_data->dev, "Skip DomainID=%x and set bit96\n", domid);
> + new.data[1] |= 1ULL << (96 - 64);
> + } else {
> + dev_info(dev_data->dev, "Not skip DomainID=%x and not set bit96\n", domid);
> + new.data[1] |= domid;
> + }
>
> /*
> * Restore cached persistent DTE bits, which can be set by information
> @@ -2549,12 +2561,15 @@ amd_iommu_domain_alloc_paging_flags(struct device *dev, u32 flags,
> {
> struct amd_iommu *iommu = get_amd_iommu_from_dev(dev);
> const u32 supported_flags = IOMMU_HWPT_ALLOC_DIRTY_TRACKING |
> + IOMMU_HWPT_ALLOC_PASID |
> + IOMMU_HWPT_ALLOC_NEST_PARENT;
> + const u32 supported_flags2 = IOMMU_HWPT_ALLOC_DIRTY_TRACKING |
> IOMMU_HWPT_ALLOC_PASID;
Just ignore NEST_PARENT? That seems wrong, it should force a V1 page
table??
> +static struct iommufd_viommu *amd_viommu_alloc(struct device *dev,
> + struct iommu_domain *parent,
> + struct iommufd_ctx *ictx,
> + unsigned int viommu_type)
> +{
> + struct amd_viommu *aviommu;
> + struct protection_domain *domain = to_pdomain(parent);
> +
> + if (viommu_type != IOMMU_VIOMMU_TYPE_TSM)
> + return ERR_PTR(-EOPNOTSUPP);
> +
> + aviommu = iommufd_viommu_alloc(ictx, struct amd_viommu, core, &amd_viommu_ops);
> + if (IS_ERR(aviommu))
> + return ERR_CAST(aviommu);
> +
> + aviommu->domain = domain;
This is not OK, the parent domain of the viommu can be used with
multiple viommu objects, it can't just have a naked back reference
like this.
You can get 1:1 domain objects linked to the viommu by creating the
'S1' type domains, maybe that is what you want here. A special domain
type that is TSM that has a special DTE.
Though I'd really rather see the domain attach logic and DTE formation
in the AMD driver be fixed up before we made it more complex :\
It would be nice to see normal nesting and viommu support first too :\
Jason
