| Message ID | 3b5284ee394f267ba966839173f874fc9a996bb2.1628094601.git.robin.murphy@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iommu: Refactor DMA domain strictness | expand |
On Wed, Aug 04, 2021 at 06:15:52PM +0100, Robin Murphy 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; I do find this a bit odd: we assert that the caller has set domain->type to IOMMU_DOMAIN_DMA_FQ but then on failure we reset it to IOMMU_DOMAIN_DMA here. I think it would be less error-prone if the setting of domain->type was handled in the same function. Will
On 2021-08-09 13:52, Will Deacon wrote: > On Wed, Aug 04, 2021 at 06:15:52PM +0100, Robin Murphy 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; > > I do find this a bit odd: we assert that the caller has set domain->type > to IOMMU_DOMAIN_DMA_FQ but then on failure we reset it to IOMMU_DOMAIN_DMA > here. I think it would be less error-prone if the setting of domain->type > was handled in the same function. On reflection I think I agree. For some reason I settled on the idea of doing this to make the callers simpler, but it turns out that unpicking it to flow logically is in fact a +4/-5 diff essentially just moving all the same statements to different places, and that's before I update comments since that theoretical race between the sysfs and DMA ops paths only exists because of sysfs having to dance around the type check here... I'll send v4 later today or possibly tomorrow, but not in such a hurry that I skimp on the build-testing this time! Cheers, Robin.
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? 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 >
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. 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 >>
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 > >>
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); - 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;