Re: [RFC PATCH 40/45] iommu/arm-smmu-v3-kvm: Add IOMMU ops

Mostafa Saleh
2023-09-26T09:55:14Z

Hi Jean,

On Mon, Sep 25, 2023 at 06:18:53PM +0100, Jean-Philippe Brucker wrote:
> On Wed, Sep 20, 2023 at 04:27:41PM +0000, Mostafa Saleh wrote:
> > > +static void kvm_arm_smmu_domain_free(struct iommu_domain *domain)
> > > +{
> > > + int ret;
> > > + struct kvm_arm_smmu_domain *kvm_smmu_domain = to_kvm_smmu_domain(domain);
> > > + struct arm_smmu_device *smmu = kvm_smmu_domain->smmu;
> > > +
> > > + if (smmu) {
> > > + struct host_arm_smmu_device *host_smmu = smmu_to_host(smmu);
> > > +
> > > + ret = kvm_call_hyp_nvhe(__pkvm_host_iommu_free_domain,
> > > + host_smmu->id, kvm_smmu_domain->id);
> > > + /*
> > > + * On failure, leak the pgd because it probably hasn't been
> > > + * reclaimed by the host.
> > > + */
> > > + if (!WARN_ON(ret))
> > > + free_pages(kvm_smmu_domain->pgd, host_smmu->pgd_order);
> > I believe this doube-free the pgd in case of attatch_dev fails, as it
> > would try to free it their also (in kvm_arm_smmu_domain_finalize).
> >
> > I think this is right place to free the pgd.
>
> Since this depends on kvm_smmu_domain->smmu being non-NULL, which is only
> true if finalize() succeeded, then we shouldn't get a double-free.

Yes, the other free was comming from an experiment I was making to use
the IOMMU layer with guest VMs, so this is correct. Sorry about that.

> But finalize() does leak kvm_smmu_domain->id if the pgd allocation fails,
> I fixed that.

Thanks!

> Thanks,
> Jean