Re: [PATCH v3 24/25] iommu/dma: Factor out flush queue init
On Mon, Aug 9, 2021 at 12:59 PM Robin Murphy wrote:
>
> On 2021-08-09 20:05, Rajat Jain wrote:
> > On Wed, Aug 4, 2021 at 10:16 AM 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
> >> Reviewed-by: John Garry
> >> Signed-off-by: Robin Murphy
> >> ---
> >> 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(>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;
>
Re: [PATCH v3 24/25] iommu/dma: Factor out flush queue init
On 2021-08-09 20:05, Rajat Jain wrote:
On Wed, Aug 4, 2021 at 10:16 AM 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
Reviewed-by: John Garry
Signed-off-by: Robin Murphy
---
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(>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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 24/25] iommu/dma: Factor out flush queue init
On Wed, Aug 4, 2021 at 10:16 AM 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
> Reviewed-by: John Garry
> Signed-off-by: Robin Murphy
> ---
> 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(>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
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 24/25] iommu/dma: Factor out flush queue init
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
Reviewed-by: John Garry
Signed-off-by: Robin Murphy
---
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(>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.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 24/25] iommu/dma: Factor out flush queue init
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
> Reviewed-by: John Garry
> Signed-off-by: Robin Murphy
> ---
> 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(>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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v3 24/25] iommu/dma: Factor out flush queue init
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
Reviewed-by: John Garry
Signed-off-by: Robin Murphy
---
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(>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;
--
2.25.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
