| Message ID | 8-v4-9e99b76f3518+3a8-smmuv3_nesting_jgg@nvidia.com |
|---|---|
| State | Accepted |
| Commit | 69d9b312f38aa19f8c801e90bd23d70685be49f0 |
| Headers | show |
| Series | Initial support for SMMUv3 nested translation | expand |
Hi Jason, > -----Original Message----- > From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Thursday, October 31, 2024 12:21 AM > To: acpica-devel@lists.linux.dev; iommu@lists.linux.dev; Joerg Roedel > <joro@8bytes.org>; Kevin Tian <kevin.tian@intel.com>; > kvm@vger.kernel.org; Len Brown <lenb@kernel.org>; linux- > acpi@vger.kernel.org; linux-arm-kernel@lists.infradead.org; Lorenzo > Pieralisi <lpieralisi@kernel.org>; Rafael J. Wysocki <rafael@kernel.org>; > Robert Moore <robert.moore@intel.com>; Robin Murphy > <robin.murphy@arm.com>; Sudeep Holla <sudeep.holla@arm.com>; Will > Deacon <will@kernel.org> > Cc: Alex Williamson <alex.williamson@redhat.com>; Donald Dutile > <ddutile@redhat.com>; Eric Auger <eric.auger@redhat.com>; Guohanjun > (Hanjun Guo) <guohanjun@huawei.com>; Jean-Philippe Brucker <jean- > philippe@linaro.org>; Jerry Snitselaar <jsnitsel@redhat.com>; Moritz > Fischer <mdf@kernel.org>; Michael Shavit <mshavit@google.com>; Nicolin > Chen <nicolinc@nvidia.com>; patches@lists.linux.dev; Rafael J. Wysocki > <rafael.j.wysocki@intel.com>; Shameerali Kolothum Thodi > <shameerali.kolothum.thodi@huawei.com>; Mostafa Saleh > <smostafa@google.com> > Subject: [PATCH v4 08/12] iommu/arm-smmu-v3: Support > IOMMU_VIOMMU_ALLOC [...] > +struct iommufd_viommu *arm_vsmmu_alloc(struct device *dev, > + struct iommu_domain *parent, > + struct iommufd_ctx *ictx, > + unsigned int viommu_type) > +{ > + struct arm_smmu_device *smmu = > + iommu_get_iommu_dev(dev, struct arm_smmu_device, > iommu); > + struct arm_smmu_master *master = dev_iommu_priv_get(dev); > + struct arm_smmu_domain *s2_parent = to_smmu_domain(parent); > + struct arm_vsmmu *vsmmu; > + > + if (viommu_type != IOMMU_VIOMMU_TYPE_ARM_SMMUV3) > + return ERR_PTR(-EOPNOTSUPP); > + > + if (!(smmu->features & ARM_SMMU_FEAT_NESTING)) > + return ERR_PTR(-EOPNOTSUPP); > + > + if (s2_parent->smmu != master->smmu) > + return ERR_PTR(-EINVAL); > + > + /* > + * Must support some way to prevent the VM from bypassing the > cache > + * because VFIO currently does not do any cache maintenance. > canwbs > + * indicates the device is fully coherent and no cache maintenance > is > + * ever required, even for PCI No-Snoop." > + */ > + if (!arm_smmu_master_canwbs(master)) > + return ERR_PTR(-EOPNOTSUPP); > + > + vsmmu = iommufd_viommu_alloc(ictx, struct arm_vsmmu, core, > + &arm_vsmmu_ops); > + if (IS_ERR(vsmmu)) > + return ERR_CAST(vsmmu); > + > + vsmmu->smmu = smmu; > + vsmmu->s2_parent = s2_parent; > + /* FIXME Move VMID allocation from the S2 domain allocation to > here */ I am planning to respin the " iommu/arm-smmu-v3: Use pinned KVM VMID for stage 2" [0] based on the latest IOMMUF code. One of the comment on that RFC was, we should associate the KVM pointer to the sub objects like viommu instead of iommufd itself[1]. But at present the s2 domain is already finalized with a VMID before a viommu object is allocated. So does the above comment indicates that we plan to do another S2 VMID allocation here and replace the old one? Please let me know your thoughts on this. Thanks, Shameer [0] https://lore.kernel.org/linux-iommu/20240208151837.35068-1-shameerali.kolothum.thodi@huawei.com/ [1] https://lore.kernel.org/linux-iommu/BN9PR11MB527662A2AB0A9BABD5E20E6D8CD52@BN9PR11MB5276.namprd11.prod.outlook.com/
On Fri, Nov 29, 2024 at 02:38:10PM +0000, Shameerali Kolothum Thodi wrote: > > + /* FIXME Move VMID allocation from the S2 domain allocation to > > here */ > > I am planning to respin the " iommu/arm-smmu-v3: Use pinned KVM VMID for > stage 2" [0] based on the latest IOMMUF code. One of the comment on that > RFC was, we should associate the KVM pointer to the sub objects like viommu > instead of iommufd itself[1]. Yes > But at present the s2 domain is already finalized > with a VMID before a viommu object is allocated. Right, this needs fixing up. The vmid must come from the viommu and the same s2 domain shared by viommus needs different vmids. > So does the above comment indicates that we plan to do another > S2 VMID allocation here and replace the old one? I have been planning to remove the vmid and asid values from the domains, along with the instance pointers. When the domain is attached in some way the vmid and instance will be recorded in a list. A viommu attach will use the vmid from the viommu, a normal attach will allocate a vmid for each instance. This will also allow S2's to be shared across instances which is another issue we have (10 copies of the S2 on big server HW is wasteful) It is simple enough to explain, but I think the datastructure will be a bit tricky to keep invalidations efficient. I haven't even started typing it in yet. But it is the next step in this project. The vcmdq has the same issue with vmid assignment as you are facing. Jason
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c index 3d2671031c9bb5..60dd9e90759571 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c @@ -29,3 +29,48 @@ void *arm_smmu_hw_info(struct device *dev, u32 *length, u32 *type) return info; } + +static const struct iommufd_viommu_ops arm_vsmmu_ops = { +}; + +struct iommufd_viommu *arm_vsmmu_alloc(struct device *dev, + struct iommu_domain *parent, + struct iommufd_ctx *ictx, + unsigned int viommu_type) +{ + struct arm_smmu_device *smmu = + iommu_get_iommu_dev(dev, struct arm_smmu_device, iommu); + struct arm_smmu_master *master = dev_iommu_priv_get(dev); + struct arm_smmu_domain *s2_parent = to_smmu_domain(parent); + struct arm_vsmmu *vsmmu; + + if (viommu_type != IOMMU_VIOMMU_TYPE_ARM_SMMUV3) + return ERR_PTR(-EOPNOTSUPP); + + if (!(smmu->features & ARM_SMMU_FEAT_NESTING)) + return ERR_PTR(-EOPNOTSUPP); + + if (s2_parent->smmu != master->smmu) + return ERR_PTR(-EINVAL); + + /* + * Must support some way to prevent the VM from bypassing the cache + * because VFIO currently does not do any cache maintenance. canwbs + * indicates the device is fully coherent and no cache maintenance is + * ever required, even for PCI No-Snoop. + */ + if (!arm_smmu_master_canwbs(master)) + return ERR_PTR(-EOPNOTSUPP); + + vsmmu = iommufd_viommu_alloc(ictx, struct arm_vsmmu, core, + &arm_vsmmu_ops); + if (IS_ERR(vsmmu)) + return ERR_CAST(vsmmu); + + vsmmu->smmu = smmu; + vsmmu->s2_parent = s2_parent; + /* FIXME Move VMID allocation from the S2 domain allocation to here */ + vsmmu->vmid = s2_parent->s2_cfg.vmid; + + return &vsmmu->core; +} diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c index b4b03206afbf48..c425fb923eb3de 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -3517,6 +3517,7 @@ static struct iommu_ops arm_smmu_ops = { .dev_disable_feat = arm_smmu_dev_disable_feature, .page_response = arm_smmu_page_response, .def_domain_type = arm_smmu_def_domain_type, + .viommu_alloc = arm_vsmmu_alloc, .pgsize_bitmap = -1UL, /* Restricted during device attach */ .owner = THIS_MODULE, .default_domain_ops = &(const struct iommu_domain_ops) { diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h index c9e5290e995a64..3b8013afcec0de 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h @@ -10,6 +10,7 @@ #include <linux/bitfield.h> #include <linux/iommu.h> +#include <linux/iommufd.h> #include <linux/kernel.h> #include <linux/mmzone.h> #include <linux/sizes.h> @@ -976,10 +977,22 @@ tegra241_cmdqv_probe(struct arm_smmu_device *smmu) } #endif /* CONFIG_TEGRA241_CMDQV */ +struct arm_vsmmu { + struct iommufd_viommu core; + struct arm_smmu_device *smmu; + struct arm_smmu_domain *s2_parent; + u16 vmid; +}; + #if IS_ENABLED(CONFIG_ARM_SMMU_V3_IOMMUFD) void *arm_smmu_hw_info(struct device *dev, u32 *length, u32 *type); +struct iommufd_viommu *arm_vsmmu_alloc(struct device *dev, + struct iommu_domain *parent, + struct iommufd_ctx *ictx, + unsigned int viommu_type); #else #define arm_smmu_hw_info NULL +#define arm_vsmmu_alloc NULL #endif /* CONFIG_ARM_SMMU_V3_IOMMUFD */ #endif /* _ARM_SMMU_V3_H */ diff --git a/include/uapi/linux/iommufd.h b/include/uapi/linux/iommufd.h index b227ac16333fe1..27c5117db985b2 100644 --- a/include/uapi/linux/iommufd.h +++ b/include/uapi/linux/iommufd.h @@ -400,10 +400,12 @@ struct iommu_hwpt_vtd_s1 { * enum iommu_hwpt_data_type - IOMMU HWPT Data Type * @IOMMU_HWPT_DATA_NONE: no data * @IOMMU_HWPT_DATA_VTD_S1: Intel VT-d stage-1 page table + * @IOMMU_HWPT_DATA_ARM_SMMUV3: ARM SMMUv3 Context Descriptor Table */ enum iommu_hwpt_data_type { IOMMU_HWPT_DATA_NONE = 0, IOMMU_HWPT_DATA_VTD_S1 = 1, + IOMMU_HWPT_DATA_ARM_SMMUV3 = 2, }; /** @@ -843,9 +845,11 @@ struct iommu_fault_alloc { /** * enum iommu_viommu_type - Virtual IOMMU Type * @IOMMU_VIOMMU_TYPE_DEFAULT: Reserved for future use + * @IOMMU_VIOMMU_TYPE_ARM_SMMUV3: ARM SMMUv3 driver specific type */ enum iommu_viommu_type { IOMMU_VIOMMU_TYPE_DEFAULT = 0, + IOMMU_VIOMMU_TYPE_ARM_SMMUV3 = 1, }; /**