Re: [PATCH] iommu: Add support to filter non-strict/lazy mode based on device names
Hi, On Wed, Aug 26, 2020 at 8:01 AM Sai Prakash Ranjan wrote: > > On 2020-08-26 19:21, Robin Murphy wrote: > > On 2020-08-26 13:17, Sai Prakash Ranjan wrote: > >> On 2020-08-26 17:07, Robin Murphy wrote: > >>> On 2020-08-25 16:42, Sai Prakash Ranjan wrote: > Currently the non-strict or lazy mode of TLB invalidation can only > be set > for all or no domains. This works well for development platforms > where > setting to non-strict/lazy mode is fine for performance reasons but > on > production devices, we need a more fine grained control to allow > only > certain peripherals to support this mode where we can be sure that > it is > safe. So add support to filter non-strict/lazy mode based on the > device > names that are passed via cmdline parameter > "iommu.nonstrict_device". > >>> > >>> There seems to be considerable overlap here with both the existing > >>> patches for per-device default domain control [1], and the broader > >>> ongoing development on how to define, evaluate and handle "trusted" > >>> vs. "untrusted" devices (e.g. [2],[3]). I'd rather see work done to > >>> make sure those integrate properly together and work well for > >>> everyone's purposes, than add more disjoint mechanisms that only > >>> address small pieces of the overall issue. > >>> > >>> Robin. > >>> > >>> [1] > >>> https://lore.kernel.org/linux-iommu/20200824051726.7xaJRTTszJuzdFWGJ8YNsshCtfNR0BNeMrlILAyqt_0@z/ > >>> [2] > >>> https://lore.kernel.org/linux-iommu/20200630044943.3425049-1-raja...@google.com/ > >>> [3] > >>> https://lore.kernel.org/linux-iommu/20200626002710.110200-2-raja...@google.com/ > >> > >> Thanks for the links, [1] definitely sounds interesting, I was under > >> the impression > >> that changing such via sysfs is late, but seems like other Sai has got > >> it working > >> for the default domain type. So we can extend that and add a strict > >> attribute as well, > >> we should be definitely OK with system booting with default strict > >> mode for all > >> peripherals as long as we have an option to change that later, Doug? > > > > Right, IIRC there was initially a proposal of a command line option > > there too, and it faced the same criticism around not being very > > generic or scalable. I believe sysfs works as a reasonable compromise > > since in many cases it can be tweaked relatively early from an initrd, > > and non-essential devices can effectively be switched at any time by > > removing and reprobing their driver. > > > > Ah I see, so the catch is that device must not be bound to the driver > and won't work for the internal devices or builtin drivers probed early. Hrm, that wouldn't work so well for us for eMMC. I don't think I'm going to manage to convince folks that we need an initrd just for this. I'm probably being naive and I haven't looked at the code, but it does seem a little weird that this isn't the kind of thing that could just be tweaked for transfers going forward... > > As for a general approach for internal devices where you do believe > > the hardware is honest but don't necessarily trust whatever firmware > > it happens to be running, I'm pretty sure that's come up already, but > > I'll be sure to mention it at Rajat's imminent LPC talk if nobody else > > does. I'll at least attend. We'll see how useful my contributions are since, as per usual, I'm wandering into an area I'm not an expert in here. ;-) -Doug ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu: Add support to filter non-strict/lazy mode based on device names
On 2020-08-26 19:21, Robin Murphy wrote: On 2020-08-26 13:17, Sai Prakash Ranjan wrote: On 2020-08-26 17:07, Robin Murphy wrote: On 2020-08-25 16:42, Sai Prakash Ranjan wrote: Currently the non-strict or lazy mode of TLB invalidation can only be set for all or no domains. This works well for development platforms where setting to non-strict/lazy mode is fine for performance reasons but on production devices, we need a more fine grained control to allow only certain peripherals to support this mode where we can be sure that it is safe. So add support to filter non-strict/lazy mode based on the device names that are passed via cmdline parameter "iommu.nonstrict_device". There seems to be considerable overlap here with both the existing patches for per-device default domain control [1], and the broader ongoing development on how to define, evaluate and handle "trusted" vs. "untrusted" devices (e.g. [2],[3]). I'd rather see work done to make sure those integrate properly together and work well for everyone's purposes, than add more disjoint mechanisms that only address small pieces of the overall issue. Robin. [1] https://lore.kernel.org/linux-iommu/20200824051726.7xaJRTTszJuzdFWGJ8YNsshCtfNR0BNeMrlILAyqt_0@z/ [2] https://lore.kernel.org/linux-iommu/20200630044943.3425049-1-raja...@google.com/ [3] https://lore.kernel.org/linux-iommu/20200626002710.110200-2-raja...@google.com/ Thanks for the links, [1] definitely sounds interesting, I was under the impression that changing such via sysfs is late, but seems like other Sai has got it working for the default domain type. So we can extend that and add a strict attribute as well, we should be definitely OK with system booting with default strict mode for all peripherals as long as we have an option to change that later, Doug? Right, IIRC there was initially a proposal of a command line option there too, and it faced the same criticism around not being very generic or scalable. I believe sysfs works as a reasonable compromise since in many cases it can be tweaked relatively early from an initrd, and non-essential devices can effectively be switched at any time by removing and reprobing their driver. Ah I see, so the catch is that device must not be bound to the driver and won't work for the internal devices or builtin drivers probed early. -Sai As for a general approach for internal devices where you do believe the hardware is honest but don't necessarily trust whatever firmware it happens to be running, I'm pretty sure that's come up already, but I'll be sure to mention it at Rajat's imminent LPC talk if nobody else does. Robin. -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu: Add support to filter non-strict/lazy mode based on device names
On 2020-08-26 13:17, Sai Prakash Ranjan wrote: On 2020-08-26 17:07, Robin Murphy wrote: On 2020-08-25 16:42, Sai Prakash Ranjan wrote: Currently the non-strict or lazy mode of TLB invalidation can only be set for all or no domains. This works well for development platforms where setting to non-strict/lazy mode is fine for performance reasons but on production devices, we need a more fine grained control to allow only certain peripherals to support this mode where we can be sure that it is safe. So add support to filter non-strict/lazy mode based on the device names that are passed via cmdline parameter "iommu.nonstrict_device". There seems to be considerable overlap here with both the existing patches for per-device default domain control [1], and the broader ongoing development on how to define, evaluate and handle "trusted" vs. "untrusted" devices (e.g. [2],[3]). I'd rather see work done to make sure those integrate properly together and work well for everyone's purposes, than add more disjoint mechanisms that only address small pieces of the overall issue. Robin. [1] https://lore.kernel.org/linux-iommu/20200824051726.7xaJRTTszJuzdFWGJ8YNsshCtfNR0BNeMrlILAyqt_0@z/ [2] https://lore.kernel.org/linux-iommu/20200630044943.3425049-1-raja...@google.com/ [3] https://lore.kernel.org/linux-iommu/20200626002710.110200-2-raja...@google.com/ Thanks for the links, [1] definitely sounds interesting, I was under the impression that changing such via sysfs is late, but seems like other Sai has got it working for the default domain type. So we can extend that and add a strict attribute as well, we should be definitely OK with system booting with default strict mode for all peripherals as long as we have an option to change that later, Doug? Right, IIRC there was initially a proposal of a command line option there too, and it faced the same criticism around not being very generic or scalable. I believe sysfs works as a reasonable compromise since in many cases it can be tweaked relatively early from an initrd, and non-essential devices can effectively be switched at any time by removing and reprobing their driver. As for a general approach for internal devices where you do believe the hardware is honest but don't necessarily trust whatever firmware it happens to be running, I'm pretty sure that's come up already, but I'll be sure to mention it at Rajat's imminent LPC talk if nobody else does. Robin. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu: Add support to filter non-strict/lazy mode based on device names
On 2020-08-26 17:07, Robin Murphy wrote: On 2020-08-25 16:42, Sai Prakash Ranjan wrote: Currently the non-strict or lazy mode of TLB invalidation can only be set for all or no domains. This works well for development platforms where setting to non-strict/lazy mode is fine for performance reasons but on production devices, we need a more fine grained control to allow only certain peripherals to support this mode where we can be sure that it is safe. So add support to filter non-strict/lazy mode based on the device names that are passed via cmdline parameter "iommu.nonstrict_device". There seems to be considerable overlap here with both the existing patches for per-device default domain control [1], and the broader ongoing development on how to define, evaluate and handle "trusted" vs. "untrusted" devices (e.g. [2],[3]). I'd rather see work done to make sure those integrate properly together and work well for everyone's purposes, than add more disjoint mechanisms that only address small pieces of the overall issue. Robin. [1] https://lore.kernel.org/linux-iommu/20200824051726.7xaJRTTszJuzdFWGJ8YNsshCtfNR0BNeMrlILAyqt_0@z/ [2] https://lore.kernel.org/linux-iommu/20200630044943.3425049-1-raja...@google.com/ [3] https://lore.kernel.org/linux-iommu/20200626002710.110200-2-raja...@google.com/ Thanks for the links, [1] definitely sounds interesting, I was under the impression that changing such via sysfs is late, but seems like other Sai has got it working for the default domain type. So we can extend that and add a strict attribute as well, we should be definitely OK with system booting with default strict mode for all peripherals as long as we have an option to change that later, Doug? Thanks, Sai -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu: Add support to filter non-strict/lazy mode based on device names
On 2020-08-25 16:42, Sai Prakash Ranjan wrote:
Currently the non-strict or lazy mode of TLB invalidation can only be set
for all or no domains. This works well for development platforms where
setting to non-strict/lazy mode is fine for performance reasons but on
production devices, we need a more fine grained control to allow only
certain peripherals to support this mode where we can be sure that it is
safe. So add support to filter non-strict/lazy mode based on the device
names that are passed via cmdline parameter "iommu.nonstrict_device".
There seems to be considerable overlap here with both the existing
patches for per-device default domain control [1], and the broader
ongoing development on how to define, evaluate and handle "trusted" vs.
"untrusted" devices (e.g. [2],[3]). I'd rather see work done to make
sure those integrate properly together and work well for everyone's
purposes, than add more disjoint mechanisms that only address small
pieces of the overall issue.
Robin.
[1]
https://lore.kernel.org/linux-iommu/20200824051726.7xaJRTTszJuzdFWGJ8YNsshCtfNR0BNeMrlILAyqt_0@z/
[2]
https://lore.kernel.org/linux-iommu/20200630044943.3425049-1-raja...@google.com/
[3]
https://lore.kernel.org/linux-iommu/20200626002710.110200-2-raja...@google.com/
Example: iommu.nonstrict_device="7c4000.sdhci,a60.dwc3,6048000.etr"
Signed-off-by: Sai Prakash Ranjan
---
drivers/iommu/iommu.c | 37 +
1 file changed, 33 insertions(+), 4 deletions(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 609bd25bf154..fd10a073f557 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -32,6 +32,9 @@ static unsigned int iommu_def_domain_type __read_mostly;
static bool iommu_dma_strict __read_mostly = true;
static u32 iommu_cmd_line __read_mostly;
+#define DEVICE_NAME_LEN 1024
+static char nonstrict_device[DEVICE_NAME_LEN] __read_mostly;
+
struct iommu_group {
struct kobject kobj;
struct kobject *devices_kobj;
@@ -327,6 +330,32 @@ static int __init iommu_dma_setup(char *str)
}
early_param("iommu.strict", iommu_dma_setup);
+static int __init iommu_nonstrict_filter_setup(char *str)
+{
+ strlcpy(nonstrict_device, str, DEVICE_NAME_LEN);
+ return 1;
+}
+__setup("iommu.nonstrict_device=", iommu_nonstrict_filter_setup);
+
+static bool iommu_nonstrict_device(struct device *dev)
+{
+ char *filter, *device;
+
+ if (!dev)
+ return false;
+
+ filter = kstrdup(nonstrict_device, GFP_KERNEL);
+ if (!filter)
+ return false;
+
+ while ((device = strsep(, ","))) {
+ if (!strcmp(device, dev_name(dev)))
+ return true;
+ }
+
+ return false;
+}
+
static ssize_t iommu_group_attr_show(struct kobject *kobj,
struct attribute *__attr, char *buf)
{
@@ -1470,7 +1499,7 @@ 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;
@@ -1489,7 +1518,7 @@ static int iommu_group_alloc_default_domain(struct bus_type *bus,
if (!group->domain)
group->domain = dom;
- if (!iommu_dma_strict) {
+ if (!iommu_dma_strict || iommu_nonstrict_device(dev)) {
int attr = 1;
iommu_domain_set_attr(dom,
DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE,
@@ -1509,7 +1538,7 @@ static int iommu_alloc_default_domain(struct iommu_group
*group,
type = iommu_get_def_domain_type(dev);
- return iommu_group_alloc_default_domain(dev->bus, group, type);
+ return iommu_group_alloc_default_domain(dev->bus, group, type, dev);
}
/**
@@ -1684,7 +1713,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);
}
base-commit: e46b3c0d011eab9933c183d5b47569db8e377281
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu: Add support to filter non-strict/lazy mode based on device names
Hi, On 2020-08-26 03:45, Doug Anderson wrote: Hi, On Tue, Aug 25, 2020 at 12:01 PM Sai Prakash Ranjan wrote: Hi, On 2020-08-25 21:40, Doug Anderson wrote: > Hi, > > On Tue, Aug 25, 2020 at 8:43 AM Sai Prakash Ranjan > wrote: >> >> Currently the non-strict or lazy mode of TLB invalidation can only be >> set >> for all or no domains. This works well for development platforms where >> setting to non-strict/lazy mode is fine for performance reasons but on >> production devices, we need a more fine grained control to allow only >> certain peripherals to support this mode where we can be sure that it >> is >> safe. So add support to filter non-strict/lazy mode based on the >> device >> names that are passed via cmdline parameter "iommu.nonstrict_device". >> >> Example: >> iommu.nonstrict_device="7c4000.sdhci,a60.dwc3,6048000.etr" Just curious: are device names like this really guaranteed to be stable across versions? Good question, AFAIK the device names are based on the DT node address and the node name, for ex: etr@6048000 and device name - "6048000.etr", now I believe these are pretty stable for a particular SoC or board since there is no reason to change the device node name or the address unless something has gone terribly wrong. I don't know about ACPI systems, but however they are described can be specified in this command line parameter. This is an advantage over the DT property where ACPI systems get left out unless someone goes and adds the same/similar property over there. > I have an inherent dislike of jamming things like this onto the > command line. IMHO the command line is the last resort for specifying > configuration and generally should be limited to some specialized > debug options and cases where the person running the kernel needs to > override a config that was set by the person (or company) compiling > the kernel. Specifically, having a long/unwieldy command line makes > it harder to use for the case when an end user actually wants to use > it to override something. It's also just another place to look for > config. > Good thing about command line parameters are that they are optional, they do not specify any default behaviour (I mean they are not mandatory to be set for the system to be functional), so I would like to view it as an optional config. And this command line parameter (nonstrict_device) is strictly optional with default being strict already set in the driver. They can be passed from the bootloader via chosen node for DT platforms or choose a new *bootconfig* as a way to pass the cmdline but finally it does boil down to just another config. Never looked at bootconfig. Unfortunately it seems to require initramfs so that pretty much means it's out for my usage. :( Yes that won't fit everywhere. I agree with general boolean or single value command line parameters being just more messy which could just be Kconfigs instead but for multiple value parameters like these do not fit in Kconfig. As you might already know, command line also gives an advantage to the end user to configure system without building kernel, for this specific command line its very useful because the performance bump is quite noticeable when the iommu.strict is off. Now for end user who would not be interested in building entire kernel(majority) and just cares about good speeds or throughput can find this very beneficial. I am not talking about one specific OS usecase here but more in general term. > The other problem is that this doesn't necessarily scale very well. > While it works OK for embedded cases it doesn't work terribly well for > distributions. I know that in an out-of-band thread you indicated > that it doesn't break anything that's not already broken (AKA this > doesn't fix the distro case but it doesn't make it worse), it would be > better to come up with a more universal solution. > Is the universal solution here referring to fix all the command line parameters in the kernel or this specific command line? Are we going to remove any more addition to the cmdline ;) There are very few cases where a kernel command line parameter is the only way to configure something. Most of the time it's just there to override a config. I wouldn't suggest removing those. I just don't want a kernel command line parameter to be the primary way to enable something. Agreed that command line parameters are not supposed to be some primary way of setting things but an optional way to override settings, in this case to override the strict mode already set by the driver. Then we can probably agree that this command line is just an optional way provided to the end user for his convenience? We would still need to find a primary way to set this non-strict mode in drivers via DT passing the information or some other way. So possible other solution is the *bootconfig* which is again just another place to look for a config. So thing is that this universal
Re: [PATCH] iommu: Add support to filter non-strict/lazy mode based on device names
On Tue, Aug 25, 2020 at 5:24 PM Doug Anderson wrote: > > Hi, > > On Tue, Aug 25, 2020 at 3:54 PM Rob Clark wrote: > > > > On Tue, Aug 25, 2020 at 3:23 PM Doug Anderson wrote: > > > > > > Hi, > > > > > > On Tue, Aug 25, 2020 at 12:01 PM Sai Prakash Ranjan > > > wrote: > > > > > > > > Hi, > > > > > > > > On 2020-08-25 21:40, Doug Anderson wrote: > > > > > Hi, > > > > > > > > > > On Tue, Aug 25, 2020 at 8:43 AM Sai Prakash Ranjan > > > > > wrote: > > > > >> > > > > >> Currently the non-strict or lazy mode of TLB invalidation can only be > > > > >> set > > > > >> for all or no domains. This works well for development platforms > > > > >> where > > > > >> setting to non-strict/lazy mode is fine for performance reasons but > > > > >> on > > > > >> production devices, we need a more fine grained control to allow only > > > > >> certain peripherals to support this mode where we can be sure that it > > > > >> is > > > > >> safe. So add support to filter non-strict/lazy mode based on the > > > > >> device > > > > >> names that are passed via cmdline parameter "iommu.nonstrict_device". > > > > >> > > > > >> Example: > > > > >> iommu.nonstrict_device="7c4000.sdhci,a60.dwc3,6048000.etr" > > > > > > Just curious: are device names like this really guaranteed to be > > > stable across versions? > > > > > > > > > > > I have an inherent dislike of jamming things like this onto the > > > > > command line. IMHO the command line is the last resort for specifying > > > > > configuration and generally should be limited to some specialized > > > > > debug options and cases where the person running the kernel needs to > > > > > override a config that was set by the person (or company) compiling > > > > > the kernel. Specifically, having a long/unwieldy command line makes > > > > > it harder to use for the case when an end user actually wants to use > > > > > it to override something. It's also just another place to look for > > > > > config. > > > > > > > > > > > > > Good thing about command line parameters are that they are optional, > > > > they do > > > > not specify any default behaviour (I mean they are not mandatory to be > > > > set > > > > for the system to be functional), so I would like to view it as an > > > > optional > > > > config. And this command line parameter (nonstrict_device) is strictly > > > > optional > > > > with default being strict already set in the driver. > > > > > > > > They can be passed from the bootloader via chosen node for DT platforms > > > > or choose > > > > a new *bootconfig* as a way to pass the cmdline but finally it does boil > > > > down to > > > > just another config. > > > > > > Never looked at bootconfig. Unfortunately it seems to require > > > initramfs so that pretty much means it's out for my usage. :( > > > > > > > > > > I agree with general boolean or single value command line parameters > > > > being just > > > > more messy which could just be Kconfigs instead but for multiple value > > > > parameters > > > > like these do not fit in Kconfig. > > > > > > > > As you might already know, command line also gives an advantage to the > > > > end user > > > > to configure system without building kernel, for this specific command > > > > line its > > > > very useful because the performance bump is quite noticeable when the > > > > iommu.strict > > > > is off. Now for end user who would not be interested in building entire > > > > kernel(majority) > > > > and just cares about good speeds or throughput can find this very > > > > beneficial. > > > > I am not talking about one specific OS usecase here but more in general > > > > term. > > > > > > > > > The other problem is that this doesn't necessarily scale very well. > > > > > While it works OK for embedded cases it doesn't work terribly well for > > > > > distributions. I know that in an out-of-band thread you indicated > > > > > that it doesn't break anything that's not already broken (AKA this > > > > > doesn't fix the distro case but it doesn't make it worse), it would be > > > > > better to come up with a more universal solution. > > > > > > > > > > > > > Is the universal solution here referring to fix all the command line > > > > parameters > > > > in the kernel or this specific command line? Are we going to remove any > > > > more > > > > addition to the cmdline ;) > > > > > > There are very few cases where a kernel command line parameter is the > > > only way to configure something. Most of the time it's just there to > > > override a config. I wouldn't suggest removing those. I just don't > > > want a kernel command line parameter to be the primary way to enable > > > something. > > > > > > > > > > So possible other solution is the *bootconfig* which is again just > > > > another place > > > > to look for a config. So thing is that this universal solution would > > > > result in > > > > just more new fancy ways of passing configs or adding such configs to > > > > the drivers > > > > or subsystems in kernel
Re: [PATCH] iommu: Add support to filter non-strict/lazy mode based on device names
Hi, On Tue, Aug 25, 2020 at 3:54 PM Rob Clark wrote: > > On Tue, Aug 25, 2020 at 3:23 PM Doug Anderson wrote: > > > > Hi, > > > > On Tue, Aug 25, 2020 at 12:01 PM Sai Prakash Ranjan > > wrote: > > > > > > Hi, > > > > > > On 2020-08-25 21:40, Doug Anderson wrote: > > > > Hi, > > > > > > > > On Tue, Aug 25, 2020 at 8:43 AM Sai Prakash Ranjan > > > > wrote: > > > >> > > > >> Currently the non-strict or lazy mode of TLB invalidation can only be > > > >> set > > > >> for all or no domains. This works well for development platforms where > > > >> setting to non-strict/lazy mode is fine for performance reasons but on > > > >> production devices, we need a more fine grained control to allow only > > > >> certain peripherals to support this mode where we can be sure that it > > > >> is > > > >> safe. So add support to filter non-strict/lazy mode based on the > > > >> device > > > >> names that are passed via cmdline parameter "iommu.nonstrict_device". > > > >> > > > >> Example: > > > >> iommu.nonstrict_device="7c4000.sdhci,a60.dwc3,6048000.etr" > > > > Just curious: are device names like this really guaranteed to be > > stable across versions? > > > > > > > > I have an inherent dislike of jamming things like this onto the > > > > command line. IMHO the command line is the last resort for specifying > > > > configuration and generally should be limited to some specialized > > > > debug options and cases where the person running the kernel needs to > > > > override a config that was set by the person (or company) compiling > > > > the kernel. Specifically, having a long/unwieldy command line makes > > > > it harder to use for the case when an end user actually wants to use > > > > it to override something. It's also just another place to look for > > > > config. > > > > > > > > > > Good thing about command line parameters are that they are optional, > > > they do > > > not specify any default behaviour (I mean they are not mandatory to be > > > set > > > for the system to be functional), so I would like to view it as an > > > optional > > > config. And this command line parameter (nonstrict_device) is strictly > > > optional > > > with default being strict already set in the driver. > > > > > > They can be passed from the bootloader via chosen node for DT platforms > > > or choose > > > a new *bootconfig* as a way to pass the cmdline but finally it does boil > > > down to > > > just another config. > > > > Never looked at bootconfig. Unfortunately it seems to require > > initramfs so that pretty much means it's out for my usage. :( > > > > > > > I agree with general boolean or single value command line parameters > > > being just > > > more messy which could just be Kconfigs instead but for multiple value > > > parameters > > > like these do not fit in Kconfig. > > > > > > As you might already know, command line also gives an advantage to the > > > end user > > > to configure system without building kernel, for this specific command > > > line its > > > very useful because the performance bump is quite noticeable when the > > > iommu.strict > > > is off. Now for end user who would not be interested in building entire > > > kernel(majority) > > > and just cares about good speeds or throughput can find this very > > > beneficial. > > > I am not talking about one specific OS usecase here but more in general > > > term. > > > > > > > The other problem is that this doesn't necessarily scale very well. > > > > While it works OK for embedded cases it doesn't work terribly well for > > > > distributions. I know that in an out-of-band thread you indicated > > > > that it doesn't break anything that's not already broken (AKA this > > > > doesn't fix the distro case but it doesn't make it worse), it would be > > > > better to come up with a more universal solution. > > > > > > > > > > Is the universal solution here referring to fix all the command line > > > parameters > > > in the kernel or this specific command line? Are we going to remove any > > > more > > > addition to the cmdline ;) > > > > There are very few cases where a kernel command line parameter is the > > only way to configure something. Most of the time it's just there to > > override a config. I wouldn't suggest removing those. I just don't > > want a kernel command line parameter to be the primary way to enable > > something. > > > > > > > So possible other solution is the *bootconfig* which is again just > > > another place > > > to look for a config. So thing is that this universal solution would > > > result in > > > just more new fancy ways of passing configs or adding such configs to > > > the drivers > > > or subsystems in kernel which is pretty much similar to implementing > > > policy in > > > kernel which I think is frowned upon and mentioned in the other thread. > > > > > > > Ideally it feels like we should figure out how to tag devices in a > > > > generic manner automatically (hardcode at the driver or in the device > > > > tree).
Re: [PATCH] iommu: Add support to filter non-strict/lazy mode based on device names
On Tue, Aug 25, 2020 at 3:23 PM Doug Anderson wrote: > > Hi, > > On Tue, Aug 25, 2020 at 12:01 PM Sai Prakash Ranjan > wrote: > > > > Hi, > > > > On 2020-08-25 21:40, Doug Anderson wrote: > > > Hi, > > > > > > On Tue, Aug 25, 2020 at 8:43 AM Sai Prakash Ranjan > > > wrote: > > >> > > >> Currently the non-strict or lazy mode of TLB invalidation can only be > > >> set > > >> for all or no domains. This works well for development platforms where > > >> setting to non-strict/lazy mode is fine for performance reasons but on > > >> production devices, we need a more fine grained control to allow only > > >> certain peripherals to support this mode where we can be sure that it > > >> is > > >> safe. So add support to filter non-strict/lazy mode based on the > > >> device > > >> names that are passed via cmdline parameter "iommu.nonstrict_device". > > >> > > >> Example: > > >> iommu.nonstrict_device="7c4000.sdhci,a60.dwc3,6048000.etr" > > Just curious: are device names like this really guaranteed to be > stable across versions? > > > > > I have an inherent dislike of jamming things like this onto the > > > command line. IMHO the command line is the last resort for specifying > > > configuration and generally should be limited to some specialized > > > debug options and cases where the person running the kernel needs to > > > override a config that was set by the person (or company) compiling > > > the kernel. Specifically, having a long/unwieldy command line makes > > > it harder to use for the case when an end user actually wants to use > > > it to override something. It's also just another place to look for > > > config. > > > > > > > Good thing about command line parameters are that they are optional, > > they do > > not specify any default behaviour (I mean they are not mandatory to be > > set > > for the system to be functional), so I would like to view it as an > > optional > > config. And this command line parameter (nonstrict_device) is strictly > > optional > > with default being strict already set in the driver. > > > > They can be passed from the bootloader via chosen node for DT platforms > > or choose > > a new *bootconfig* as a way to pass the cmdline but finally it does boil > > down to > > just another config. > > Never looked at bootconfig. Unfortunately it seems to require > initramfs so that pretty much means it's out for my usage. :( > > > > I agree with general boolean or single value command line parameters > > being just > > more messy which could just be Kconfigs instead but for multiple value > > parameters > > like these do not fit in Kconfig. > > > > As you might already know, command line also gives an advantage to the > > end user > > to configure system without building kernel, for this specific command > > line its > > very useful because the performance bump is quite noticeable when the > > iommu.strict > > is off. Now for end user who would not be interested in building entire > > kernel(majority) > > and just cares about good speeds or throughput can find this very > > beneficial. > > I am not talking about one specific OS usecase here but more in general > > term. > > > > > The other problem is that this doesn't necessarily scale very well. > > > While it works OK for embedded cases it doesn't work terribly well for > > > distributions. I know that in an out-of-band thread you indicated > > > that it doesn't break anything that's not already broken (AKA this > > > doesn't fix the distro case but it doesn't make it worse), it would be > > > better to come up with a more universal solution. > > > > > > > Is the universal solution here referring to fix all the command line > > parameters > > in the kernel or this specific command line? Are we going to remove any > > more > > addition to the cmdline ;) > > There are very few cases where a kernel command line parameter is the > only way to configure something. Most of the time it's just there to > override a config. I wouldn't suggest removing those. I just don't > want a kernel command line parameter to be the primary way to enable > something. > > > > So possible other solution is the *bootconfig* which is again just > > another place > > to look for a config. So thing is that this universal solution would > > result in > > just more new fancy ways of passing configs or adding such configs to > > the drivers > > or subsystems in kernel which is pretty much similar to implementing > > policy in > > kernel which I think is frowned upon and mentioned in the other thread. > > > > > Ideally it feels like we should figure out how to tag devices in a > > > generic manner automatically (hardcode at the driver or in the device > > > tree). I think the out-of-band discussions talked about "external > > > facing" and the like. We could also, perhaps, tag devices that have > > > "binary blob" firmware if we wanted. Then we'd have a policy (set by > > > Kconfig, perhaps overridable via commandline) that indicated the > > >
Re: [PATCH] iommu: Add support to filter non-strict/lazy mode based on device names
Hi, On Tue, Aug 25, 2020 at 12:01 PM Sai Prakash Ranjan wrote: > > Hi, > > On 2020-08-25 21:40, Doug Anderson wrote: > > Hi, > > > > On Tue, Aug 25, 2020 at 8:43 AM Sai Prakash Ranjan > > wrote: > >> > >> Currently the non-strict or lazy mode of TLB invalidation can only be > >> set > >> for all or no domains. This works well for development platforms where > >> setting to non-strict/lazy mode is fine for performance reasons but on > >> production devices, we need a more fine grained control to allow only > >> certain peripherals to support this mode where we can be sure that it > >> is > >> safe. So add support to filter non-strict/lazy mode based on the > >> device > >> names that are passed via cmdline parameter "iommu.nonstrict_device". > >> > >> Example: > >> iommu.nonstrict_device="7c4000.sdhci,a60.dwc3,6048000.etr" Just curious: are device names like this really guaranteed to be stable across versions? > > I have an inherent dislike of jamming things like this onto the > > command line. IMHO the command line is the last resort for specifying > > configuration and generally should be limited to some specialized > > debug options and cases where the person running the kernel needs to > > override a config that was set by the person (or company) compiling > > the kernel. Specifically, having a long/unwieldy command line makes > > it harder to use for the case when an end user actually wants to use > > it to override something. It's also just another place to look for > > config. > > > > Good thing about command line parameters are that they are optional, > they do > not specify any default behaviour (I mean they are not mandatory to be > set > for the system to be functional), so I would like to view it as an > optional > config. And this command line parameter (nonstrict_device) is strictly > optional > with default being strict already set in the driver. > > They can be passed from the bootloader via chosen node for DT platforms > or choose > a new *bootconfig* as a way to pass the cmdline but finally it does boil > down to > just another config. Never looked at bootconfig. Unfortunately it seems to require initramfs so that pretty much means it's out for my usage. :( > I agree with general boolean or single value command line parameters > being just > more messy which could just be Kconfigs instead but for multiple value > parameters > like these do not fit in Kconfig. > > As you might already know, command line also gives an advantage to the > end user > to configure system without building kernel, for this specific command > line its > very useful because the performance bump is quite noticeable when the > iommu.strict > is off. Now for end user who would not be interested in building entire > kernel(majority) > and just cares about good speeds or throughput can find this very > beneficial. > I am not talking about one specific OS usecase here but more in general > term. > > > The other problem is that this doesn't necessarily scale very well. > > While it works OK for embedded cases it doesn't work terribly well for > > distributions. I know that in an out-of-band thread you indicated > > that it doesn't break anything that's not already broken (AKA this > > doesn't fix the distro case but it doesn't make it worse), it would be > > better to come up with a more universal solution. > > > > Is the universal solution here referring to fix all the command line > parameters > in the kernel or this specific command line? Are we going to remove any > more > addition to the cmdline ;) There are very few cases where a kernel command line parameter is the only way to configure something. Most of the time it's just there to override a config. I wouldn't suggest removing those. I just don't want a kernel command line parameter to be the primary way to enable something. > So possible other solution is the *bootconfig* which is again just > another place > to look for a config. So thing is that this universal solution would > result in > just more new fancy ways of passing configs or adding such configs to > the drivers > or subsystems in kernel which is pretty much similar to implementing > policy in > kernel which I think is frowned upon and mentioned in the other thread. > > > Ideally it feels like we should figure out how to tag devices in a > > generic manner automatically (hardcode at the driver or in the device > > tree). I think the out-of-band discussions talked about "external > > facing" and the like. We could also, perhaps, tag devices that have > > "binary blob" firmware if we wanted. Then we'd have a policy (set by > > Kconfig, perhaps overridable via commandline) that indicated the > > strictness level for the various classes of devices. So policy would > > be decided by KConfig and/or command line. > > > > How is tagging in driver or device tree better than the simple command > line > approach to pass the same list of devices which otherwise you would > hardcode >
Re: [PATCH] iommu: Add support to filter non-strict/lazy mode based on device names
Hi, On 2020-08-25 21:40, Doug Anderson wrote: Hi, On Tue, Aug 25, 2020 at 8:43 AM Sai Prakash Ranjan wrote: Currently the non-strict or lazy mode of TLB invalidation can only be set for all or no domains. This works well for development platforms where setting to non-strict/lazy mode is fine for performance reasons but on production devices, we need a more fine grained control to allow only certain peripherals to support this mode where we can be sure that it is safe. So add support to filter non-strict/lazy mode based on the device names that are passed via cmdline parameter "iommu.nonstrict_device". Example: iommu.nonstrict_device="7c4000.sdhci,a60.dwc3,6048000.etr" I have an inherent dislike of jamming things like this onto the command line. IMHO the command line is the last resort for specifying configuration and generally should be limited to some specialized debug options and cases where the person running the kernel needs to override a config that was set by the person (or company) compiling the kernel. Specifically, having a long/unwieldy command line makes it harder to use for the case when an end user actually wants to use it to override something. It's also just another place to look for config. Good thing about command line parameters are that they are optional, they do not specify any default behaviour (I mean they are not mandatory to be set for the system to be functional), so I would like to view it as an optional config. And this command line parameter (nonstrict_device) is strictly optional with default being strict already set in the driver. They can be passed from the bootloader via chosen node for DT platforms or choose a new *bootconfig* as a way to pass the cmdline but finally it does boil down to just another config. I agree with general boolean or single value command line parameters being just more messy which could just be Kconfigs instead but for multiple value parameters like these do not fit in Kconfig. As you might already know, command line also gives an advantage to the end user to configure system without building kernel, for this specific command line its very useful because the performance bump is quite noticeable when the iommu.strict is off. Now for end user who would not be interested in building entire kernel(majority) and just cares about good speeds or throughput can find this very beneficial. I am not talking about one specific OS usecase here but more in general term. The other problem is that this doesn't necessarily scale very well. While it works OK for embedded cases it doesn't work terribly well for distributions. I know that in an out-of-band thread you indicated that it doesn't break anything that's not already broken (AKA this doesn't fix the distro case but it doesn't make it worse), it would be better to come up with a more universal solution. Is the universal solution here referring to fix all the command line parameters in the kernel or this specific command line? Are we going to remove any more addition to the cmdline ;) So possible other solution is the *bootconfig* which is again just another place to look for a config. So thing is that this universal solution would result in just more new fancy ways of passing configs or adding such configs to the drivers or subsystems in kernel which is pretty much similar to implementing policy in kernel which I think is frowned upon and mentioned in the other thread. Ideally it feels like we should figure out how to tag devices in a generic manner automatically (hardcode at the driver or in the device tree). I think the out-of-band discussions talked about "external facing" and the like. We could also, perhaps, tag devices that have "binary blob" firmware if we wanted. Then we'd have a policy (set by Kconfig, perhaps overridable via commandline) that indicated the strictness level for the various classes of devices. So policy would be decided by KConfig and/or command line. How is tagging in driver or device tree better than the simple command line approach to pass the same list of devices which otherwise you would hardcode in the corresponding drivers and device tree all over the kernel other than the scalability part for command line? IMHO it is too much churn. Device tree could be used but then we have a problem with it being for only describing hardware and it doesn't work for ACPI based systems. Command line approach works for all systems (both DT and ACPI) without having to add too much churn to drivers. Lastly, I think we can have both options, it doesn't hurt to add command line parameter since it is optional. Thanks, Sai -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu: Add support to filter non-strict/lazy mode based on device names
Hi, On Tue, Aug 25, 2020 at 8:43 AM Sai Prakash Ranjan wrote: > > Currently the non-strict or lazy mode of TLB invalidation can only be set > for all or no domains. This works well for development platforms where > setting to non-strict/lazy mode is fine for performance reasons but on > production devices, we need a more fine grained control to allow only > certain peripherals to support this mode where we can be sure that it is > safe. So add support to filter non-strict/lazy mode based on the device > names that are passed via cmdline parameter "iommu.nonstrict_device". > > Example: iommu.nonstrict_device="7c4000.sdhci,a60.dwc3,6048000.etr" I have an inherent dislike of jamming things like this onto the command line. IMHO the command line is the last resort for specifying configuration and generally should be limited to some specialized debug options and cases where the person running the kernel needs to override a config that was set by the person (or company) compiling the kernel. Specifically, having a long/unwieldy command line makes it harder to use for the case when an end user actually wants to use it to override something. It's also just another place to look for config. The other problem is that this doesn't necessarily scale very well. While it works OK for embedded cases it doesn't work terribly well for distributions. I know that in an out-of-band thread you indicated that it doesn't break anything that's not already broken (AKA this doesn't fix the distro case but it doesn't make it worse), it would be better to come up with a more universal solution. Ideally it feels like we should figure out how to tag devices in a generic manner automatically (hardcode at the driver or in the device tree). I think the out-of-band discussions talked about "external facing" and the like. We could also, perhaps, tag devices that have "binary blob" firmware if we wanted. Then we'd have a policy (set by Kconfig, perhaps overridable via commandline) that indicated the strictness level for the various classes of devices. So policy would be decided by KConfig and/or command line. -Doug ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH] iommu: Add support to filter non-strict/lazy mode based on device names
Currently the non-strict or lazy mode of TLB invalidation can only be set
for all or no domains. This works well for development platforms where
setting to non-strict/lazy mode is fine for performance reasons but on
production devices, we need a more fine grained control to allow only
certain peripherals to support this mode where we can be sure that it is
safe. So add support to filter non-strict/lazy mode based on the device
names that are passed via cmdline parameter "iommu.nonstrict_device".
Example: iommu.nonstrict_device="7c4000.sdhci,a60.dwc3,6048000.etr"
Signed-off-by: Sai Prakash Ranjan
---
drivers/iommu/iommu.c | 37 +
1 file changed, 33 insertions(+), 4 deletions(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 609bd25bf154..fd10a073f557 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -32,6 +32,9 @@ static unsigned int iommu_def_domain_type __read_mostly;
static bool iommu_dma_strict __read_mostly = true;
static u32 iommu_cmd_line __read_mostly;
+#define DEVICE_NAME_LEN1024
+static char nonstrict_device[DEVICE_NAME_LEN] __read_mostly;
+
struct iommu_group {
struct kobject kobj;
struct kobject *devices_kobj;
@@ -327,6 +330,32 @@ static int __init iommu_dma_setup(char *str)
}
early_param("iommu.strict", iommu_dma_setup);
+static int __init iommu_nonstrict_filter_setup(char *str)
+{
+ strlcpy(nonstrict_device, str, DEVICE_NAME_LEN);
+ return 1;
+}
+__setup("iommu.nonstrict_device=", iommu_nonstrict_filter_setup);
+
+static bool iommu_nonstrict_device(struct device *dev)
+{
+ char *filter, *device;
+
+ if (!dev)
+ return false;
+
+ filter = kstrdup(nonstrict_device, GFP_KERNEL);
+ if (!filter)
+ return false;
+
+ while ((device = strsep(, ","))) {
+ if (!strcmp(device, dev_name(dev)))
+ return true;
+ }
+
+ return false;
+}
+
static ssize_t iommu_group_attr_show(struct kobject *kobj,
struct attribute *__attr, char *buf)
{
@@ -1470,7 +1499,7 @@ 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;
@@ -1489,7 +1518,7 @@ static int iommu_group_alloc_default_domain(struct
bus_type *bus,
if (!group->domain)
group->domain = dom;
- if (!iommu_dma_strict) {
+ if (!iommu_dma_strict || iommu_nonstrict_device(dev)) {
int attr = 1;
iommu_domain_set_attr(dom,
DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE,
@@ -1509,7 +1538,7 @@ static int iommu_alloc_default_domain(struct iommu_group
*group,
type = iommu_get_def_domain_type(dev);
- return iommu_group_alloc_default_domain(dev->bus, group, type);
+ return iommu_group_alloc_default_domain(dev->bus, group, type, dev);
}
/**
@@ -1684,7 +1713,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);
}
base-commit: e46b3c0d011eab9933c183d5b47569db8e377281
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
