| Message ID | c62c9405ff31ab3c97a9165ff710dce68217fe22.1678348754.git.nicolinc@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add Nested Translation Support for SMMUv3 | expand |
Hi Nicolin, On 3/9/23 11:53, Nicolin Chen wrote: > IOMMUFD designs two iommu_domain pointers to represent two stages. The S1 s/designs/uses? > iommu_domain (IOMMU_DOMAIN_NESTED type) represents the Context Descriptor > table in the user space. The S2 iommu_domain (IOMMU_DOMAIN_UNMANAGED type) > represents the translation table in the kernel, owned by a hypervisor. > > So there comes to no use case of the ARM_SMMU_DOMAIN_NESTED. Drop it, and > use the type IOMMU_DOMAIN_NESTED instead. last sentence may be rephrased as this patch does not use IOMMU_DOMAIN_NESTED anywhere: Generic IOMMU_DOMAIN_NESTED type will be used in nested SMMU implementation instead. > > Also drop the unused arm_smmu_enable_nesting(). One following patche will > configure the correct smmu_domain->stage. > > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> > --- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 18 ------------------ > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 1 - If you go this way you may also remove it from arm/arm-smmu/arm-smmu.c. Then if I am not wrong no other driver does implement enable_nesting cb. Shouldn't we also remove it and fellow iommu_enable_nesting()? Thanks Eric > 2 files changed, 19 deletions(-) > > 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 c1aac695ae0d..c5616145e2a3 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > @@ -1279,7 +1279,6 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid, > s1_cfg = &smmu_domain->s1_cfg; > break; > case ARM_SMMU_DOMAIN_S2: > - case ARM_SMMU_DOMAIN_NESTED: > s2_cfg = &smmu_domain->s2_cfg; > break; > default: > @@ -2220,7 +2219,6 @@ static int arm_smmu_domain_finalise(struct iommu_domain *domain, > fmt = ARM_64_LPAE_S1; > finalise_stage_fn = arm_smmu_domain_finalise_s1; > break; > - case ARM_SMMU_DOMAIN_NESTED: > case ARM_SMMU_DOMAIN_S2: > ias = smmu->ias; > oas = smmu->oas; > @@ -2747,21 +2745,6 @@ static struct iommu_group *arm_smmu_device_group(struct device *dev) > return group; > } > > -static int arm_smmu_enable_nesting(struct iommu_domain *domain) > -{ > - struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain); > - int ret = 0; > - > - mutex_lock(&smmu_domain->init_mutex); > - if (smmu_domain->smmu) > - ret = -EPERM; > - else > - smmu_domain->stage = ARM_SMMU_DOMAIN_NESTED; > - mutex_unlock(&smmu_domain->init_mutex); > - > - return ret; > -} > - > static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args *args) > { > return iommu_fwspec_add_ids(dev, args->args, 1); > @@ -2890,7 +2873,6 @@ static struct iommu_ops arm_smmu_ops = { > .flush_iotlb_all = arm_smmu_flush_iotlb_all, > .iotlb_sync = arm_smmu_iotlb_sync, > .iova_to_phys = arm_smmu_iova_to_phys, > - .enable_nesting = arm_smmu_enable_nesting, > .free = arm_smmu_domain_free, > } > }; > 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 ba2b4562f4b2..233bfc377267 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h > @@ -704,7 +704,6 @@ struct arm_smmu_master { > enum arm_smmu_domain_stage { > ARM_SMMU_DOMAIN_S1 = 0, > ARM_SMMU_DOMAIN_S2, > - ARM_SMMU_DOMAIN_NESTED, > ARM_SMMU_DOMAIN_BYPASS, > }; >
On Fri, Mar 10, 2023 at 05:39:22PM +0100, Eric Auger wrote: > > Also drop the unused arm_smmu_enable_nesting(). One following patche will > > configure the correct smmu_domain->stage. > > > > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> > > --- > > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 18 ------------------ > > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 1 - > If you go this way you may also remove it from arm/arm-smmu/arm-smmu.c. > Then if I am not wrong no other driver does implement enable_nesting cb. > Shouldn't we also remove it and fellow iommu_enable_nesting()? Yes, lets just put this patch in the series please: https://lore.kernel.org/kvm/0-v1-0093c9b0e345+19-vfio_no_nesting_jgg@nvidia.com/ Jason
On Fri, Mar 10, 2023 at 05:39:22PM +0100, Eric Auger wrote: > External email: Use caution opening links or attachments > > > Hi Nicolin, > > On 3/9/23 11:53, Nicolin Chen wrote: > > IOMMUFD designs two iommu_domain pointers to represent two stages. The S1 > s/designs/uses? > > iommu_domain (IOMMU_DOMAIN_NESTED type) represents the Context Descriptor > > table in the user space. The S2 iommu_domain (IOMMU_DOMAIN_UNMANAGED type) > > represents the translation table in the kernel, owned by a hypervisor. > > > > So there comes to no use case of the ARM_SMMU_DOMAIN_NESTED. Drop it, and > > use the type IOMMU_DOMAIN_NESTED instead. > last sentence may be rephrased as this patch does not use > IOMMU_DOMAIN_NESTED anywhere: > Generic IOMMU_DOMAIN_NESTED type will be used in nested SMMU > implementation instead. > > > > Also drop the unused arm_smmu_enable_nesting(). One following patche will > > configure the correct smmu_domain->stage. > > > > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> > > --- > > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 18 ------------------ > > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 1 - > If you go this way you may also remove it from arm/arm-smmu/arm-smmu.c. > Then if I am not wrong no other driver does implement enable_nesting cb. > Shouldn't we also remove it and fellow iommu_enable_nesting()? We had a small discussion before this community version, where Robin mentioned that we can remove that too after the nesting series gets merged. Yet, I didn't want to touch the v2 driver with this series since there's no nesting change adding to it. And a few month ago, Jason had a patch removing everything of that API from the top. Perhaps that one can be resent after all? Thanks Nic
On Fri, Mar 10, 2023 at 01:05:36PM -0400, Jason Gunthorpe wrote: > On Fri, Mar 10, 2023 at 05:39:22PM +0100, Eric Auger wrote: > > > > Also drop the unused arm_smmu_enable_nesting(). One following patche will > > > configure the correct smmu_domain->stage. > > > > > > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> > > > --- > > > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 18 ------------------ > > > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 1 - > > If you go this way you may also remove it from arm/arm-smmu/arm-smmu.c. > > Then if I am not wrong no other driver does implement enable_nesting cb. > > Shouldn't we also remove it and fellow iommu_enable_nesting()? > > Yes, lets just put this patch in the series please: > > https://lore.kernel.org/kvm/0-v1-0093c9b0e345+19-vfio_no_nesting_jgg@nvidia.com/ Oh. Didn't read this before sending my previous reply.. Will do that. Nic
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 c1aac695ae0d..c5616145e2a3 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -1279,7 +1279,6 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid, s1_cfg = &smmu_domain->s1_cfg; break; case ARM_SMMU_DOMAIN_S2: - case ARM_SMMU_DOMAIN_NESTED: s2_cfg = &smmu_domain->s2_cfg; break; default: @@ -2220,7 +2219,6 @@ static int arm_smmu_domain_finalise(struct iommu_domain *domain, fmt = ARM_64_LPAE_S1; finalise_stage_fn = arm_smmu_domain_finalise_s1; break; - case ARM_SMMU_DOMAIN_NESTED: case ARM_SMMU_DOMAIN_S2: ias = smmu->ias; oas = smmu->oas; @@ -2747,21 +2745,6 @@ static struct iommu_group *arm_smmu_device_group(struct device *dev) return group; } -static int arm_smmu_enable_nesting(struct iommu_domain *domain) -{ - struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain); - int ret = 0; - - mutex_lock(&smmu_domain->init_mutex); - if (smmu_domain->smmu) - ret = -EPERM; - else - smmu_domain->stage = ARM_SMMU_DOMAIN_NESTED; - mutex_unlock(&smmu_domain->init_mutex); - - return ret; -} - static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args *args) { return iommu_fwspec_add_ids(dev, args->args, 1); @@ -2890,7 +2873,6 @@ static struct iommu_ops arm_smmu_ops = { .flush_iotlb_all = arm_smmu_flush_iotlb_all, .iotlb_sync = arm_smmu_iotlb_sync, .iova_to_phys = arm_smmu_iova_to_phys, - .enable_nesting = arm_smmu_enable_nesting, .free = arm_smmu_domain_free, } }; 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 ba2b4562f4b2..233bfc377267 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h @@ -704,7 +704,6 @@ struct arm_smmu_master { enum arm_smmu_domain_stage { ARM_SMMU_DOMAIN_S1 = 0, ARM_SMMU_DOMAIN_S2, - ARM_SMMU_DOMAIN_NESTED, ARM_SMMU_DOMAIN_BYPASS, };
IOMMUFD designs two iommu_domain pointers to represent two stages. The S1 iommu_domain (IOMMU_DOMAIN_NESTED type) represents the Context Descriptor table in the user space. The S2 iommu_domain (IOMMU_DOMAIN_UNMANAGED type) represents the translation table in the kernel, owned by a hypervisor. So there comes to no use case of the ARM_SMMU_DOMAIN_NESTED. Drop it, and use the type IOMMU_DOMAIN_NESTED instead. Also drop the unused arm_smmu_enable_nesting(). One following patche will configure the correct smmu_domain->stage. Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> --- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 18 ------------------ drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 1 - 2 files changed, 19 deletions(-)