From: Robin Murphy <robin.murphy@arm.com> To: Douglas Anderson <dianders@chromium.org>, gregkh@linuxfoundation.org, rafael@kernel.org, rafael.j.wysocki@intel.com, will@kernel.org, joro@8bytes.org, bjorn.andersson@linaro.org, ulf.hansson@linaro.org, adrian.hunter@intel.com, bhelgaas@google.com Cc: robdclark@chromium.org, linux-arm-msm@vger.kernel.org, linux-pci@vger.kernel.org, quic_c_gdjako@quicinc.com, iommu@lists.linux-foundation.org, sonnyrao@chromium.org, saiprakash.ranjan@codeaurora.org, linux-mmc@vger.kernel.org, vbadigan@codeaurora.org, rajatja@google.com, saravanak@google.com, joel@joelfernandes.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 4/6] iommu: Combine device strictness requests with the global default Date: Tue, 22 Jun 2021 12:49:10 +0100 [thread overview] Message-ID: <a023af85-5060-0a3c-4648-b00f8b8c0430@arm.com> (raw) In-Reply-To: <20210621165230.4.Id84a954e705fcad3fdb35beb2bc372e4bf2108c7@changeid> On 2021-06-22 00:52, Douglas Anderson wrote: > In the patch ("drivers: base: Add bits to struct device to control > iommu strictness") we add the ability for devices to tell us about > their IOMMU strictness requirements. Let's now take that into account > in the IOMMU layer. > > A few notes here: > * Presumably this is always how iommu_get_dma_strict() was intended to > behave. Had this not been the intention then it never would have > taken a domain as a parameter. FWIW strictness does have the semantic of being a per-domain property, but mostly in the sense that it's only relevant to IOMMU_DOMAIN_DMA domains, so the main thing was encapsulating that check rather than duplicating it all over callsites. > * The iommu_set_dma_strict() feels awfully non-symmetric now. That > function sets the _default_ strictness globally in the system > whereas iommu_get_dma_strict() returns the value for a given domain > (falling back to the default). Presumably, at least, the fact that > iommu_set_dma_strict() doesn't take a domain makes this obvious. It *is* asymmetric - one is for IOMMU core code and individual driver internals to know whether they need to do whatever bits of setting up a flush queue for a given domain they are responsible for, while the other is specifically for two drivers to force the global default in order to preserve legacy driver-specific behaviour. Maybe that should have been called something like iommu_set_dma_default_strict instead... :/ Robin. > The function iommu_get_dma_strict() should now make it super obvious > where strictness comes from and who overides who. Though the function > changed a bunch to make the logic clearer, the only two new rules > should be: > * Devices can force strictness for themselves, overriding the cmdline > "iommu.strict=0" or a call to iommu_set_dma_strict(false)). > * Devices can request non-strictness for themselves, assuming there > was no cmdline "iommu.strict=1" or a call to > iommu_set_dma_strict(true). > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > drivers/iommu/iommu.c | 56 +++++++++++++++++++++++++++++++++---------- > include/linux/iommu.h | 2 ++ > 2 files changed, 45 insertions(+), 13 deletions(-) > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > index 808ab70d5df5..0c84a4c06110 100644 > --- a/drivers/iommu/iommu.c > +++ b/drivers/iommu/iommu.c > @@ -28,8 +28,19 @@ > static struct kset *iommu_group_kset; > static DEFINE_IDA(iommu_group_ida); > > +enum iommu_strictness { > + IOMMU_DEFAULT_STRICTNESS = -1, > + IOMMU_NOT_STRICT = 0, > + IOMMU_STRICT = 1, > +}; > +static inline enum iommu_strictness bool_to_strictness(bool strictness) > +{ > + return (enum iommu_strictness)strictness; > +} > + > static unsigned int iommu_def_domain_type __read_mostly; > -static bool iommu_dma_strict __read_mostly = true; > +static enum iommu_strictness cmdline_dma_strict __read_mostly = IOMMU_DEFAULT_STRICTNESS; > +static enum iommu_strictness driver_dma_strict __read_mostly = IOMMU_DEFAULT_STRICTNESS; > static u32 iommu_cmd_line __read_mostly; > > struct iommu_group { > @@ -69,7 +80,6 @@ static const char * const iommu_group_resv_type_string[] = { > }; > > #define IOMMU_CMD_LINE_DMA_API BIT(0) > -#define IOMMU_CMD_LINE_STRICT BIT(1) > > static int iommu_alloc_default_domain(struct iommu_group *group, > struct device *dev); > @@ -336,25 +346,38 @@ early_param("iommu.passthrough", iommu_set_def_domain_type); > > static int __init iommu_dma_setup(char *str) > { > - int ret = kstrtobool(str, &iommu_dma_strict); > + bool strict; > + int ret = kstrtobool(str, &strict); > > if (!ret) > - iommu_cmd_line |= IOMMU_CMD_LINE_STRICT; > + cmdline_dma_strict = bool_to_strictness(strict); > return ret; > } > early_param("iommu.strict", iommu_dma_setup); > > void iommu_set_dma_strict(bool strict) > { > - if (strict || !(iommu_cmd_line & IOMMU_CMD_LINE_STRICT)) > - iommu_dma_strict = strict; > + /* A driver can request strictness but not the other way around */ > + if (driver_dma_strict != IOMMU_STRICT) > + driver_dma_strict = bool_to_strictness(strict); > } > > bool iommu_get_dma_strict(struct iommu_domain *domain) > { > - /* only allow lazy flushing for DMA domains */ > - if (domain->type == IOMMU_DOMAIN_DMA) > - return iommu_dma_strict; > + /* Non-DMA domains or anyone forcing it to strict makes it strict */ > + if (domain->type != IOMMU_DOMAIN_DMA || > + cmdline_dma_strict == IOMMU_STRICT || > + driver_dma_strict == IOMMU_STRICT || > + domain->force_strict) > + return true; > + > + /* Anyone requesting non-strict (if no forces) makes it non-strict */ > + if (cmdline_dma_strict == IOMMU_NOT_STRICT || > + driver_dma_strict == IOMMU_NOT_STRICT || > + domain->request_non_strict) > + return false; > + > + /* Nobody said anything, so it's strict by default */ > return true; > } > EXPORT_SYMBOL_GPL(iommu_get_dma_strict); > @@ -1519,7 +1542,8 @@ static int iommu_get_def_domain_type(struct device *dev) > > static int iommu_group_alloc_default_domain(struct bus_type *bus, > struct iommu_group *group, > - unsigned int type) > + unsigned int type, > + struct device *dev) > { > struct iommu_domain *dom; > > @@ -1534,6 +1558,12 @@ static int iommu_group_alloc_default_domain(struct bus_type *bus, > if (!dom) > return -ENOMEM; > > + /* Save the strictness requests from the device */ > + if (dev && type == IOMMU_DOMAIN_DMA) { > + dom->request_non_strict = dev->request_non_strict_iommu; > + dom->force_strict = dev->force_strict_iommu; > + } > + > group->default_domain = dom; > if (!group->domain) > group->domain = dom; > @@ -1550,7 +1580,7 @@ static int iommu_alloc_default_domain(struct iommu_group *group, > > type = iommu_get_def_domain_type(dev) ? : iommu_def_domain_type; > > - return iommu_group_alloc_default_domain(dev->bus, group, type); > + return iommu_group_alloc_default_domain(dev->bus, group, type, dev); > } > > /** > @@ -1721,7 +1751,7 @@ static void probe_alloc_default_domain(struct bus_type *bus, > if (!gtype.type) > gtype.type = iommu_def_domain_type; > > - iommu_group_alloc_default_domain(bus, group, gtype.type); > + iommu_group_alloc_default_domain(bus, group, gtype.type, NULL); > > } > > @@ -3130,7 +3160,7 @@ static int iommu_change_dev_def_domain(struct iommu_group *group, > } > > /* Sets group->default_domain to the newly allocated domain */ > - ret = iommu_group_alloc_default_domain(dev->bus, group, type); > + ret = iommu_group_alloc_default_domain(dev->bus, group, type, dev); > if (ret) > goto out; > > diff --git a/include/linux/iommu.h b/include/linux/iommu.h > index 32d448050bf7..0bddef77f415 100644 > --- a/include/linux/iommu.h > +++ b/include/linux/iommu.h > @@ -81,6 +81,8 @@ struct iommu_domain_geometry { > > struct iommu_domain { > unsigned type; > + bool force_strict:1; > + bool request_non_strict:1; > const struct iommu_ops *ops; > unsigned long pgsize_bitmap; /* Bitmap of page sizes in use */ > iommu_fault_handler_t handler; >
WARNING: multiple messages have this Message-ID (diff)
From: Robin Murphy <robin.murphy@arm.com> To: Douglas Anderson <dianders@chromium.org>, gregkh@linuxfoundation.org, rafael@kernel.org, rafael.j.wysocki@intel.com, will@kernel.org, joro@8bytes.org, bjorn.andersson@linaro.org, ulf.hansson@linaro.org, adrian.hunter@intel.com, bhelgaas@google.com Cc: robdclark@chromium.org, saravanak@google.com, linux-arm-msm@vger.kernel.org, linux-mmc@vger.kernel.org, quic_c_gdjako@quicinc.com, linux-kernel@vger.kernel.org, iommu@lists.linux-foundation.org, linux-pci@vger.kernel.org, joel@joelfernandes.org, rajatja@google.com, sonnyrao@chromium.org, vbadigan@codeaurora.org Subject: Re: [PATCH 4/6] iommu: Combine device strictness requests with the global default Date: Tue, 22 Jun 2021 12:49:10 +0100 [thread overview] Message-ID: <a023af85-5060-0a3c-4648-b00f8b8c0430@arm.com> (raw) In-Reply-To: <20210621165230.4.Id84a954e705fcad3fdb35beb2bc372e4bf2108c7@changeid> On 2021-06-22 00:52, Douglas Anderson wrote: > In the patch ("drivers: base: Add bits to struct device to control > iommu strictness") we add the ability for devices to tell us about > their IOMMU strictness requirements. Let's now take that into account > in the IOMMU layer. > > A few notes here: > * Presumably this is always how iommu_get_dma_strict() was intended to > behave. Had this not been the intention then it never would have > taken a domain as a parameter. FWIW strictness does have the semantic of being a per-domain property, but mostly in the sense that it's only relevant to IOMMU_DOMAIN_DMA domains, so the main thing was encapsulating that check rather than duplicating it all over callsites. > * The iommu_set_dma_strict() feels awfully non-symmetric now. That > function sets the _default_ strictness globally in the system > whereas iommu_get_dma_strict() returns the value for a given domain > (falling back to the default). Presumably, at least, the fact that > iommu_set_dma_strict() doesn't take a domain makes this obvious. It *is* asymmetric - one is for IOMMU core code and individual driver internals to know whether they need to do whatever bits of setting up a flush queue for a given domain they are responsible for, while the other is specifically for two drivers to force the global default in order to preserve legacy driver-specific behaviour. Maybe that should have been called something like iommu_set_dma_default_strict instead... :/ Robin. > The function iommu_get_dma_strict() should now make it super obvious > where strictness comes from and who overides who. Though the function > changed a bunch to make the logic clearer, the only two new rules > should be: > * Devices can force strictness for themselves, overriding the cmdline > "iommu.strict=0" or a call to iommu_set_dma_strict(false)). > * Devices can request non-strictness for themselves, assuming there > was no cmdline "iommu.strict=1" or a call to > iommu_set_dma_strict(true). > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > drivers/iommu/iommu.c | 56 +++++++++++++++++++++++++++++++++---------- > include/linux/iommu.h | 2 ++ > 2 files changed, 45 insertions(+), 13 deletions(-) > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > index 808ab70d5df5..0c84a4c06110 100644 > --- a/drivers/iommu/iommu.c > +++ b/drivers/iommu/iommu.c > @@ -28,8 +28,19 @@ > static struct kset *iommu_group_kset; > static DEFINE_IDA(iommu_group_ida); > > +enum iommu_strictness { > + IOMMU_DEFAULT_STRICTNESS = -1, > + IOMMU_NOT_STRICT = 0, > + IOMMU_STRICT = 1, > +}; > +static inline enum iommu_strictness bool_to_strictness(bool strictness) > +{ > + return (enum iommu_strictness)strictness; > +} > + > static unsigned int iommu_def_domain_type __read_mostly; > -static bool iommu_dma_strict __read_mostly = true; > +static enum iommu_strictness cmdline_dma_strict __read_mostly = IOMMU_DEFAULT_STRICTNESS; > +static enum iommu_strictness driver_dma_strict __read_mostly = IOMMU_DEFAULT_STRICTNESS; > static u32 iommu_cmd_line __read_mostly; > > struct iommu_group { > @@ -69,7 +80,6 @@ static const char * const iommu_group_resv_type_string[] = { > }; > > #define IOMMU_CMD_LINE_DMA_API BIT(0) > -#define IOMMU_CMD_LINE_STRICT BIT(1) > > static int iommu_alloc_default_domain(struct iommu_group *group, > struct device *dev); > @@ -336,25 +346,38 @@ early_param("iommu.passthrough", iommu_set_def_domain_type); > > static int __init iommu_dma_setup(char *str) > { > - int ret = kstrtobool(str, &iommu_dma_strict); > + bool strict; > + int ret = kstrtobool(str, &strict); > > if (!ret) > - iommu_cmd_line |= IOMMU_CMD_LINE_STRICT; > + cmdline_dma_strict = bool_to_strictness(strict); > return ret; > } > early_param("iommu.strict", iommu_dma_setup); > > void iommu_set_dma_strict(bool strict) > { > - if (strict || !(iommu_cmd_line & IOMMU_CMD_LINE_STRICT)) > - iommu_dma_strict = strict; > + /* A driver can request strictness but not the other way around */ > + if (driver_dma_strict != IOMMU_STRICT) > + driver_dma_strict = bool_to_strictness(strict); > } > > bool iommu_get_dma_strict(struct iommu_domain *domain) > { > - /* only allow lazy flushing for DMA domains */ > - if (domain->type == IOMMU_DOMAIN_DMA) > - return iommu_dma_strict; > + /* Non-DMA domains or anyone forcing it to strict makes it strict */ > + if (domain->type != IOMMU_DOMAIN_DMA || > + cmdline_dma_strict == IOMMU_STRICT || > + driver_dma_strict == IOMMU_STRICT || > + domain->force_strict) > + return true; > + > + /* Anyone requesting non-strict (if no forces) makes it non-strict */ > + if (cmdline_dma_strict == IOMMU_NOT_STRICT || > + driver_dma_strict == IOMMU_NOT_STRICT || > + domain->request_non_strict) > + return false; > + > + /* Nobody said anything, so it's strict by default */ > return true; > } > EXPORT_SYMBOL_GPL(iommu_get_dma_strict); > @@ -1519,7 +1542,8 @@ static int iommu_get_def_domain_type(struct device *dev) > > static int iommu_group_alloc_default_domain(struct bus_type *bus, > struct iommu_group *group, > - unsigned int type) > + unsigned int type, > + struct device *dev) > { > struct iommu_domain *dom; > > @@ -1534,6 +1558,12 @@ static int iommu_group_alloc_default_domain(struct bus_type *bus, > if (!dom) > return -ENOMEM; > > + /* Save the strictness requests from the device */ > + if (dev && type == IOMMU_DOMAIN_DMA) { > + dom->request_non_strict = dev->request_non_strict_iommu; > + dom->force_strict = dev->force_strict_iommu; > + } > + > group->default_domain = dom; > if (!group->domain) > group->domain = dom; > @@ -1550,7 +1580,7 @@ static int iommu_alloc_default_domain(struct iommu_group *group, > > type = iommu_get_def_domain_type(dev) ? : iommu_def_domain_type; > > - return iommu_group_alloc_default_domain(dev->bus, group, type); > + return iommu_group_alloc_default_domain(dev->bus, group, type, dev); > } > > /** > @@ -1721,7 +1751,7 @@ static void probe_alloc_default_domain(struct bus_type *bus, > if (!gtype.type) > gtype.type = iommu_def_domain_type; > > - iommu_group_alloc_default_domain(bus, group, gtype.type); > + iommu_group_alloc_default_domain(bus, group, gtype.type, NULL); > > } > > @@ -3130,7 +3160,7 @@ static int iommu_change_dev_def_domain(struct iommu_group *group, > } > > /* Sets group->default_domain to the newly allocated domain */ > - ret = iommu_group_alloc_default_domain(dev->bus, group, type); > + ret = iommu_group_alloc_default_domain(dev->bus, group, type, dev); > if (ret) > goto out; > > diff --git a/include/linux/iommu.h b/include/linux/iommu.h > index 32d448050bf7..0bddef77f415 100644 > --- a/include/linux/iommu.h > +++ b/include/linux/iommu.h > @@ -81,6 +81,8 @@ struct iommu_domain_geometry { > > struct iommu_domain { > unsigned type; > + bool force_strict:1; > + bool request_non_strict:1; > const struct iommu_ops *ops; > unsigned long pgsize_bitmap; /* Bitmap of page sizes in use */ > iommu_fault_handler_t handler; > _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
next prev parent reply other threads:[~2021-06-22 11:49 UTC|newest] Thread overview: 68+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-06-21 23:52 [PATCH 0/6] iommu: Enable devices to request non-strict DMA, starting with QCom SD/MMC Douglas Anderson 2021-06-21 23:52 ` Douglas Anderson 2021-06-21 23:52 ` [PATCH 1/6] drivers: base: Add the concept of "pre_probe" to drivers Douglas Anderson 2021-06-21 23:52 ` Douglas Anderson 2021-06-24 13:35 ` Greg KH 2021-06-24 13:35 ` Greg KH 2021-06-21 23:52 ` [PATCH 2/6] drivers: base: Add bits to struct device to control iommu strictness Douglas Anderson 2021-06-21 23:52 ` Douglas Anderson 2021-06-24 13:36 ` Greg KH 2021-06-24 13:36 ` Greg KH 2021-06-24 13:42 ` Doug Anderson 2021-06-24 13:42 ` Doug Anderson 2021-06-21 23:52 ` [PATCH 3/6] PCI: Indicate that we want to force strict DMA for untrusted devices Douglas Anderson 2021-06-21 23:52 ` Douglas Anderson 2021-06-24 13:38 ` Greg KH 2021-06-24 13:38 ` Greg KH 2021-06-24 13:46 ` Doug Anderson 2021-06-24 13:46 ` Doug Anderson 2021-06-21 23:52 ` [PATCH 4/6] iommu: Combine device strictness requests with the global default Douglas Anderson 2021-06-21 23:52 ` Douglas Anderson 2021-06-22 2:03 ` Lu Baolu 2021-06-22 2:03 ` Lu Baolu 2021-06-22 16:53 ` Doug Anderson 2021-06-22 16:53 ` Doug Anderson 2021-06-22 17:01 ` Doug Anderson 2021-06-22 17:01 ` Doug Anderson 2021-06-22 2:55 ` Saravana Kannan 2021-06-22 2:55 ` Saravana Kannan via iommu 2021-06-22 16:40 ` Doug Anderson 2021-06-22 16:40 ` Doug Anderson 2021-06-22 19:50 ` Saravana Kannan 2021-06-22 19:50 ` Saravana Kannan via iommu 2021-06-22 11:49 ` Robin Murphy [this message] 2021-06-22 11:49 ` Robin Murphy 2021-06-22 18:45 ` Rajat Jain 2021-06-22 18:45 ` Rajat Jain via iommu 2021-06-22 19:35 ` Doug Anderson 2021-06-22 19:35 ` Doug Anderson 2021-06-21 23:52 ` [PATCH 5/6] iommu: Stop reaching into PCIe devices to decide strict vs. non-strict Douglas Anderson 2021-06-21 23:52 ` Douglas Anderson 2021-06-21 23:52 ` [PATCH 6/6] mmc: sdhci-msm: Request non-strict IOMMU mode Douglas Anderson 2021-06-21 23:52 ` Douglas Anderson 2021-06-24 13:43 ` Greg KH 2021-06-24 13:43 ` Greg KH 2021-06-24 14:00 ` Doug Anderson 2021-06-24 14:00 ` Doug Anderson 2021-06-22 11:35 ` [PATCH 0/6] iommu: Enable devices to request non-strict DMA, starting with QCom SD/MMC Robin Murphy 2021-06-22 11:35 ` Robin Murphy 2021-06-22 16:06 ` Doug Anderson 2021-06-22 16:06 ` Doug Anderson 2021-06-22 20:02 ` Rob Herring 2021-06-22 20:02 ` Rob Herring 2021-06-22 20:05 ` Saravana Kannan 2021-06-22 20:05 ` Saravana Kannan via iommu 2021-06-22 20:10 ` Doug Anderson 2021-06-22 20:10 ` Doug Anderson 2021-06-23 13:54 ` Rob Herring 2021-06-23 13:54 ` Rob Herring 2021-06-22 22:10 ` Robin Murphy 2021-06-22 22:10 ` Robin Murphy 2021-06-23 17:29 ` Doug Anderson 2021-06-23 17:29 ` Doug Anderson 2021-06-24 17:23 ` Doug Anderson 2021-06-24 17:23 ` Doug Anderson 2021-06-22 17:39 ` John Garry 2021-06-22 17:39 ` John Garry 2021-06-22 19:50 ` Doug Anderson 2021-06-22 19:50 ` Doug Anderson
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=a023af85-5060-0a3c-4648-b00f8b8c0430@arm.com \ --to=robin.murphy@arm.com \ --cc=adrian.hunter@intel.com \ --cc=bhelgaas@google.com \ --cc=bjorn.andersson@linaro.org \ --cc=dianders@chromium.org \ --cc=gregkh@linuxfoundation.org \ --cc=iommu@lists.linux-foundation.org \ --cc=joel@joelfernandes.org \ --cc=joro@8bytes.org \ --cc=linux-arm-msm@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mmc@vger.kernel.org \ --cc=linux-pci@vger.kernel.org \ --cc=quic_c_gdjako@quicinc.com \ --cc=rafael.j.wysocki@intel.com \ --cc=rafael@kernel.org \ --cc=rajatja@google.com \ --cc=robdclark@chromium.org \ --cc=saiprakash.ranjan@codeaurora.org \ --cc=saravanak@google.com \ --cc=sonnyrao@chromium.org \ --cc=ulf.hansson@linaro.org \ --cc=vbadigan@codeaurora.org \ --cc=will@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.