RE: [RFC PATCH v3 4/5] iommu/arm-smmu-v3-iommufd: Use KVM VMID for s2 stage

[Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>]

> -----Original Message-----
> From: Jason Gunthorpe <jgg@ziepe.ca>
> Sent: Wednesday, March 19, 2025 11:40 PM
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: kvmarm@lists.linux.dev; iommu@lists.linux.dev; linux-arm-
> kernel@lists.infradead.org; Linuxarm <linuxarm@huawei.com>;
> kevin.tian@intel.com; alex.williamson@redhat.com; maz@kernel.org;
> oliver.upton@linux.dev; will@kernel.org; robin.murphy@arm.com;
> nicolinc@nvidia.com; jean-philippe@linaro.org; Jonathan Cameron
> <jonathan.cameron@huawei.com>
> Subject: Re: [RFC PATCH v3 4/5] iommu/arm-smmu-v3-iommufd: Use KVM
> VMID for s2 stage
> 
> On Wed, Mar 19, 2025 at 05:32:01PM +0000, Shameer Kolothum wrote:
> > +static int arm_vsmmu_alloc_vmid(struct arm_smmu_device *smmu,
> struct kvm *kvm,
> > +				bool *kvm_used)
> > +{
> > +#ifdef CONFIG_KVM
> > +	/*
> > +	 * There can only be one allocator for VMIDs active at once. If BTM
> is
> > +	 * turned on then KVM's allocator always supplies the VMID, and
> the
> > +	 * VMID is matched by CPU invalidation of the KVM S2. Right now
> there
> > +	 * is no API to get an unused VMID from KVM so this also means
> BTM systems
> > +	 * cannot support S2 without an associated KVM.
> > +	 */
> > +	if ((smmu->features & ARM_SMMU_FEAT_BTM)) {
> > +		int vmid;
> > +
> > +		if (!kvm || !kvm_get_kvm_safe(kvm))
> > +			return -EOPNOTSUPP;
> 
> Isn't kvm_get_kvm_safe() in modular KVM code most of the time? You can't
> call it like this right?

I think it is safe since KVM is not modular for ARM64. And that is one of the
reasons I thought I will hold the KVM reference here as we don't have to go
through the symbol_get/put calls here. Also IOMMUFD is not using the kvm
pointer at the moment. Probably I should add some comments in IOMMUFD
to make it clear.

However I just realized that in patch #1 I should EXPORT the
kvm_arm_pinned_vmid_get/put functions as smmuv3 driver can be built
modular. 

> 
> Per my prior comments, I'm a little nervous to make drivers keep track
> of this refcount instead of the core code but this does seem like it
> could work..
> 
> I also think you should block using the S2 without a viommu & KVM in
> BTM mode.. Maybe like this:
> 
> @@ -2938,8 +2938,16 @@ static int arm_smmu_attach_dev(struct
> iommu_domain *domain, struct device *dev)
>                 cdptr = arm_smmu_alloc_cd_ptr(master, IOMMU_NO_PASID);
>                 if (!cdptr)
>                         return -ENOMEM;
> -       } else if (arm_smmu_ssids_in_use(&master->cd_table))
> -               return -EBUSY;
> +       } else {
> +               if (arm_smmu_ssids_in_use(&master->cd_table))
> +                       return -EBUSY;
> +               /*
> +                * S2 cannot be used when BTM is turned on without a VIOMMU
> and
> +                * VMID shared with KVM
> +                */
> +               if (smmu->features & ARM_SMMU_FEAT_BTM)
> +                       return -EOPNOTSUPP;
> +       }
> 
> That more closely matches how this will eventually work when the VMID
> allocation is properly made to be local to the viommu.

Ok. Will update.

Thanks,
Shameer