Re: [PATCH 4/6] iommu: Combine device strictness requests with the global default
On Tue, Jun 22, 2021 at 9:46 AM Doug Anderson wrote: > > Hi, > > On Mon, Jun 21, 2021 at 7:56 PM Saravana Kannan wrote: > > > > On Mon, Jun 21, 2021 at 4:53 PM 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. > > > * 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. > > > > > > 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 > > > --- > > > > > > 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, _dma_strict); > > > + bool strict; > > > + int ret = kstrtobool(str, ); > > > > > > 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
Re: [PATCH 4/6] iommu: Combine device strictness requests with the global default
Hi, On Tue, Jun 22, 2021 at 11:45 AM Rajat Jain wrote: > > On Mon, Jun 21, 2021 at 4:53 PM 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. > > * 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. > > > > 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). > > Along the same lines, I believe a platform (device tree / ACPI) should > also be able to have a say in this. I assume in your proposal, a > platform would expose a property in device tree which the device > driver would need to parse and then use it to set these bits in the > "struct device"? Nothing would prevent creating a device tree or ACPI property that caused either "force-strict" or "request-non-strict" from being set if everyone agrees that it's a good idea. I wouldn't reject the idea myself, but I do worry that we'd devolve into the usual bikeshed for exactly how this should look. I talked about this a bit in my response to Saravana, but basically: * If there was some generic property, would we call it "untrusted", "external", or something else? * How do you describe "trust" in a generic "objective" way? It's not really boolean and trying to describe exactly how trustworthy something should be considered is hard. * At least for the device tree there's a general requirement that it describes the hardware and not so much how the software should configure the hardware. As I understand it there is _some_ leeway here where it's OK to describe how the hardware was designed for the OS to configure it, but it's a pretty high bar and a hard sell. In general the device tree isn't supposed to be used to describe "policy". In other words: if one OS might decide on one setting and another OS on another then it doesn't really belong in the device tree. * In general the kernel is also not really supposed to have policy hardcoded in either, though it feels like we can get away with having a good default/sane policy and allowing overriding the policy with command line parameters (like iommu.strict). In the case where something has to be configured at bootup there's not many ways to do better. tl;dr: I have no plans to try to make an overarching property, but my patch series does allow subsystems to come up with and easily implement their own rules as it makes sense. While this might seem hodgepodge I prefer to see it as "flexible" since I'm not convinced that we're going to be able to come up with an overarching trust framework. -Doug ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 4/6] iommu: Combine device strictness requests with the global default
On Mon, Jun 21, 2021 at 4:53 PM 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. > * 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. > > 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). Along the same lines, I believe a platform (device tree / ACPI) should also be able to have a say in this. I assume in your proposal, a platform would expose a property in device tree which the device driver would need to parse and then use it to set these bits in the "struct device"? Thanks, Rajat > > Signed-off-by: Douglas Anderson > --- > > 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, _dma_strict); > + bool strict; > + int ret = kstrtobool(str, ); > > 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
Re: [PATCH 4/6] iommu: Combine device strictness requests with the global default
Hi, On Tue, Jun 22, 2021 at 9:53 AM Doug Anderson wrote: > > Hi, > > On Mon, Jun 21, 2021 at 7:05 PM Lu Baolu wrote: > > > > On 6/22/21 7:52 AM, Douglas Anderson wrote: > > > @@ -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; > > > + } > > > + > > > > An iommu default domain might be used by multiple devices which might > > have different "strict" attributions. Then who could override who? > > My gut instinct would be that if multiple devices were part of a given > domain that it would be combined like this: > > 1. Any device that requests strict makes the domain strict force strict. > > 2. To request non-strict all of the devices in the domain would have > to request non-strict. > > To do that I'd have to change my patchset obviously, but I don't think > it should be hard. We can just keep a count of devices and a count of > the strict vs. non-strict requests? If there are no other blockers > I'll try to do that in my v2. One issue, I guess, is that we might need to transition a non-strict domain to become strict. Is that possible? We'd end up with an extra "flush queue" that we didn't need to allocate, but are there other problems? Actually, in general would it be possible to transition between strict and non-strict at runtime as long as we called init_iova_flush_queue()? Maybe that's a better solution than all this--we just boot in strict mode and can just transition to non-strict mode after-the-fact? -Doug ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 4/6] iommu: Combine device strictness requests with the global default
Hi, On Mon, Jun 21, 2021 at 7:05 PM Lu Baolu wrote: > > On 6/22/21 7:52 AM, Douglas Anderson wrote: > > @@ -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; > > + } > > + > > An iommu default domain might be used by multiple devices which might > have different "strict" attributions. Then who could override who? My gut instinct would be that if multiple devices were part of a given domain that it would be combined like this: 1. Any device that requests strict makes the domain strict force strict. 2. To request non-strict all of the devices in the domain would have to request non-strict. To do that I'd have to change my patchset obviously, but I don't think it should be hard. We can just keep a count of devices and a count of the strict vs. non-strict requests? If there are no other blockers I'll try to do that in my v2. -Doug ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 4/6] iommu: Combine device strictness requests with the global default
Hi, On Mon, Jun 21, 2021 at 7:56 PM Saravana Kannan wrote: > > On Mon, Jun 21, 2021 at 4:53 PM 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. > > * 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. > > > > 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 > > --- > > > > 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, _dma_strict); > > + bool strict; > > + int ret = kstrtobool(str, ); > > > > 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 */ > > If iommu.strict is not set in the command line, upstream treats it as > iommu.strict=1. Meaning, no drivers can override it. > > If I understand it correctly, with your series, if iommu.strict=1 is > not set,
Re: [PATCH 4/6] iommu: Combine device strictness requests with the global default
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 --- 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, _dma_strict); + bool strict; + int ret = kstrtobool(str, ); 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 */
Re: [PATCH 4/6] iommu: Combine device strictness requests with the global default
On Mon, Jun 21, 2021 at 4:53 PM 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. > * 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. > > 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 > --- > > 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, _dma_strict); > + bool strict; > + int ret = kstrtobool(str, ); > > 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 */ If iommu.strict is not set in the command line, upstream treats it as iommu.strict=1. Meaning, no drivers can override it. If I understand it correctly, with your series, if iommu.strict=1 is not set, drivers can override the "default strict mode" and ask for non-strict mode for their domain. So if this series gets in and future driver changes start asking for non-strict mode, systems that are expected to operate in fully strict mode will now have devices operating in non-strict mode. That's breaking backward
Re: [PATCH 4/6] iommu: Combine device strictness requests with the global default
On 6/22/21 7:52 AM, Douglas Anderson wrote: @@ -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; + } + An iommu default domain might be used by multiple devices which might have different "strict" attributions. Then who could override who? Best regards, baolu ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 4/6] iommu: Combine device strictness requests with the global default
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. * 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. 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 --- 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, _dma_strict); + bool strict; + int ret = kstrtobool(str, ); 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) { +