Re: [PATCH v3 24/25] iommu/dma: Factor out flush queue init

From: Rajat Jain <rajatja@google.com>
To: Robin Murphy <robin.murphy@arm.com>
Cc: joro@8bytes.org, will@kernel.org,
	iommu@lists.linux-foundation.org,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, suravee.suthikulpanit@amd.com,
	baolu.lu@linux.intel.com, john.garry@huawei.com,
	dianders@chromium.org, chenxiang66@hisilicon.com
Subject: Re: [PATCH v3 24/25] iommu/dma: Factor out flush queue init
Date: Mon, 9 Aug 2021 13:15:46 -0700	[thread overview]
Message-ID: <CACK8Z6Gb1O8Ok1GEy80nQJ4ZQeFJ3TOgFHRYfH3Yb=8CNT=ysw@mail.gmail.com> (raw)
In-Reply-To: <00baf83a-5eb3-4d72-309c-5adfd3242c07@arm.com>

On Mon, Aug 9, 2021 at 12:59 PM Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 2021-08-09 20:05, Rajat Jain wrote:
> > On Wed, Aug 4, 2021 at 10:16 AM Robin Murphy <robin.murphy@arm.com> wrote:
> >>
> >> Factor out flush queue setup from the initial domain init so that we
> >> can potentially trigger it from sysfs later on in a domain's lifetime.
> >>
> >> Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
> >> Reviewed-by: John Garry <john.garry@huawei.com>
> >> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> >> ---
> >>   drivers/iommu/dma-iommu.c | 30 ++++++++++++++++++++----------
> >>   include/linux/dma-iommu.h |  9 ++++++---
> >>   2 files changed, 26 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> >> index 2e19505dddf9..f51b8dc99ac6 100644
> >> --- a/drivers/iommu/dma-iommu.c
> >> +++ b/drivers/iommu/dma-iommu.c
> >> @@ -310,6 +310,25 @@ static bool dev_is_untrusted(struct device *dev)
> >>          return dev_is_pci(dev) && to_pci_dev(dev)->untrusted;
> >>   }
> >>
> >> +int iommu_dma_init_fq(struct iommu_domain *domain)
> >> +{
> >> +       struct iommu_dma_cookie *cookie = domain->iova_cookie;
> >> +
> >> +       if (domain->type != IOMMU_DOMAIN_DMA_FQ)
> >> +               return -EINVAL;
> >> +       if (cookie->fq_domain)
> >> +               return 0;
> >> +
> >> +       if (init_iova_flush_queue(&cookie->iovad, iommu_dma_flush_iotlb_all,
> >> +                                 iommu_dma_entry_dtor)) {
> >> +               pr_warn("iova flush queue initialization failed\n");
> >> +               domain->type = IOMMU_DOMAIN_DMA;
> >> +               return -ENODEV;
> >> +       }
> >> +       cookie->fq_domain = domain;
> >> +       return 0;
> >> +}
> >> +
> >>   /**
> >>    * iommu_dma_init_domain - Initialise a DMA mapping domain
> >>    * @domain: IOMMU domain previously prepared by iommu_get_dma_cookie()
> >> @@ -362,16 +381,7 @@ static int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base,
> >>          }
> >>
> >>          init_iova_domain(iovad, 1UL << order, base_pfn);
> >> -
> >> -       if (domain->type == IOMMU_DOMAIN_DMA_FQ && !cookie->fq_domain) {
> >> -               if (init_iova_flush_queue(iovad, iommu_dma_flush_iotlb_all,
> >> -                                         iommu_dma_entry_dtor)) {
> >> -                       pr_warn("iova flush queue initialization failed\n");
> >> -                       domain->type = IOMMU_DOMAIN_DMA;
> >> -               } else {
> >> -                       cookie->fq_domain = domain;
> >> -               }
> >> -       }
> >> +       iommu_dma_init_fq(domain);
> >>
> >>          return iova_reserve_iommu_regions(dev, domain);
> >>   }
> >> diff --git a/include/linux/dma-iommu.h b/include/linux/dma-iommu.h
> >> index 758ca4694257..81ab647f1618 100644
> >> --- a/include/linux/dma-iommu.h
> >> +++ b/include/linux/dma-iommu.h
> >> @@ -20,6 +20,7 @@ void iommu_put_dma_cookie(struct iommu_domain *domain);
> >>
> >>   /* Setup call for arch DMA mapping code */
> >>   void iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 dma_limit);
> >> +int iommu_dma_init_fq(struct iommu_domain *domain);
> >>
> >>   /* The DMA API isn't _quite_ the whole story, though... */
> >>   /*
> >> @@ -37,9 +38,6 @@ void iommu_dma_compose_msi_msg(struct msi_desc *desc,
> >>
> >>   void iommu_dma_get_resv_regions(struct device *dev, struct list_head *list);
> >>
> >> -void iommu_dma_free_cpu_cached_iovas(unsigned int cpu,
> >> -               struct iommu_domain *domain);
> >> -
> >
> > This looks like an unrelated code cleanup. Should this be a separate patch?
>
> Ha, busted! Much of this was done in the "stream of consciousness" style
> where I made a big sprawling mess then split it up into patches and
> branches afterwards. TBH it was already feeling pretty tenuous having a
> separate patch just to move this one function, and it only gets more so
> with the simplification Will pointed out earlier. I think I'll squash
> iommu_dma_init_fq() into the next patch then do a thorough header sweep,
> since I've now spotted some things in iova.h which could probably go as
> well.

Thank you. I chanced upon this only because I've backported your
patchset (and some other changes that it depends on) to 5.10 which is
the kernel we currently use for our Intel platforms, and this cleanup
hunk was creating a problem (since 5.10 still uses the symbol you
removed). I'll be giving your v3 patchset a spin in my setup and
update you in case I see any issue.

Thanks,

Rajat

>
> Thanks for the poke!
>
> Robin.
>
> >
> > Thanks,
> >
> > Rajat
> >
> >
> >>   extern bool iommu_dma_forcedac;
> >>
> >>   #else /* CONFIG_IOMMU_DMA */
> >> @@ -54,6 +52,11 @@ static inline void iommu_setup_dma_ops(struct device *dev, u64 dma_base,
> >>   {
> >>   }
> >>
> >> +static inline int iommu_dma_init_fq(struct iommu_domain *domain)
> >> +{
> >> +       return -EINVAL;
> >> +}
> >> +
> >>   static inline int iommu_get_dma_cookie(struct iommu_domain *domain)
> >>   {
> >>          return -ENODEV;
> >> --
> >> 2.25.1
> >>