Re: [PATCH 1/3] iommu/io-pgtable-arm: Prepare for modularization
From: isaacm
Date: Mon Dec 21 2020 - 20:05:34 EST
On 2020-12-21 07:22, Robin Murphy wrote:
On 2020-12-18 18:59, isaacm@xxxxxxxxxxxxxx wrote:
On 2020-12-18 04:38, Robin Murphy wrote:
On 2020-12-18 08:38, Isaac J. Manjarres wrote:
The io-pgtable-arm and io-pgtable-arm-v7s source files will
be compiled as separate modules, along with the io-pgtable
source. Export the symbols for the io-pgtable init function
structures for the io-pgtable module to use.
In my current build tree, the io-pgtable glue itself is a whopping
379
bytes of code and data - is there really any benefit to all the
additional overhead of making that modular? Given the number of
different users (including AMD now), I think at this point we should
start considering this as part of the IOMMU core, and just tweak the
interface such that formats can register their own init_fns
dynamically instead of the static array that's always horrible.
Robin.
Thanks for the feedback, Robin. This is an avenue I had explored a bit
when modularizing the code. However,
I came up with a few problems that I couldn't get around.
1) If we leave the io-pgtable glue as part of the core kernel, we need
to ensure that the io-pgtable format
modules get loaded prior to any driver that might use them (e.g. IOMMU
drivers/other callers of alloc_io_pgtable_ops).
a) This can get a bit messy, as there's no symbol dependencies
between the callers of the io-pgtable
code, and the page table format modules, since everything is
through function pointers. This is handled
for the IOMMU drivers through the devlink feature, but I don't
see how we can leverage something like that
here. I guess this isn't too much of a problem when everything
is built-in, as the registration can happen
in one of the earlier initcall levels.
b) If we do run into a scenario where a client of io-pgtable
tries to allocate a page table instance prior
to the io-pgtable format module being loaded, I couldn't come
up with a way of distinguishing between
format module is not available at the moment vs format module
will never be available. I don't think
returning EPROBE_DEFER would be something nice to do in that
case.
Urgh, I see... yes, the current approach does work out as an
unexpectedly neat way to avoid many of the pitfalls. However I'm not
sure it actually avoids all of them - say you have a config like this:
IPMMU_VMSA=y
-> IO_PGTABLE_ARM_LPAE=y
-> IO_PGTABLE=y
MTK_IOMMU=m
-> IO_PGTABLE_ARMV7S=m
won't that still fail to link io-pgtable.o?
Yes, you are correct, that would be problematic.
2) We would have to ensure that the format module cannot be unloaded
while other clients are using it. I suppose
this isn't as big as point #1 though, since it's something that can
probably be handled through a similar ref count
mechanism that we're using for modular IOMMU drivers.
FWIW I think that would come out in the wash from resolving 1b - I'd
assume there would have to be some sort of module_get() in there
somewhere. I should probably go and look at how the crypto API handles
its modular algorithms for more inspiration...
So I looked through the crypto dir, and it seems like they--along with a
few other kernel drivers--are using MODULE_SOFTDEP()
to sort out these dependencies.
Given the two reasons above, I went with the current approach, since
it avoids both issues by creating symbol dependencies
between client drivers, the io-pgtable drivers, and the io-pgtable
format drivers, so that ensures that they are loaded
in the correct order, and also prevents them from being removed,
unless there aren't any users present.
Having thought all that over, I'm now wondering what we really gain
from this either way - if vendors can build and ship SoC-tailored
configs, then they can already turn off formats they don't care about.
If the aim is to ship a single config everywhere, then you'll still
have to provision and load all possible formats on any system that
needs any one of them, thanks to those "convenient" symbol
dependencies. The promise in the cover letter doesn't seem to
materialise :/
Robin.
Given the feedback, this makes sense. I've come up with a second version
of the patches that leaves
the io-pgtable code in the kernel, and allows the formats to be modules,
which better achieves what
the cover-letter is trying to express :) I believe that with the second
patch, we should be able to
get to a place where the kernel just needs to provide io-pgtable, while
vendors can provide either
io-pgtable-arm or io-pgtable-arm-v7s or both, as needed.
Here are the patches:
https://lore.kernel.org/linux-iommu/1608597876-32367-1-git-send-email-isaacm@xxxxxxxxxxxxxx/T/#t
Thanks,
Isaac
Thanks,
Isaac
Signed-off-by: Isaac J. Manjarres <isaacm@xxxxxxxxxxxxxx>
---
drivers/iommu/io-pgtable-arm-v7s.c | 4 ++++
drivers/iommu/io-pgtable-arm.c | 8 ++++++++
2 files changed, 12 insertions(+)
diff --git a/drivers/iommu/io-pgtable-arm-v7s.c
b/drivers/iommu/io-pgtable-arm-v7s.c
index 1d92ac9..f062c1c 100644
--- a/drivers/iommu/io-pgtable-arm-v7s.c
+++ b/drivers/iommu/io-pgtable-arm-v7s.c
@@ -28,6 +28,7 @@
#include <linux/iommu.h>
#include <linux/kernel.h>
#include <linux/kmemleak.h>
+#include <linux/module.h>
#include <linux/sizes.h>
#include <linux/slab.h>
#include <linux/spinlock.h>
@@ -839,6 +840,7 @@ struct io_pgtable_init_fns
io_pgtable_arm_v7s_init_fns = {
.alloc = arm_v7s_alloc_pgtable,
.free = arm_v7s_free_pgtable,
};
+EXPORT_SYMBOL_GPL(io_pgtable_arm_v7s_init_fns);
#ifdef CONFIG_IOMMU_IO_PGTABLE_ARMV7S_SELFTEST
@@ -984,3 +986,5 @@ static int __init arm_v7s_do_selftests(void)
}
subsys_initcall(arm_v7s_do_selftests);
#endif
+
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/iommu/io-pgtable-arm.c
b/drivers/iommu/io-pgtable-arm.c
index 87def58..2623d57 100644
--- a/drivers/iommu/io-pgtable-arm.c
+++ b/drivers/iommu/io-pgtable-arm.c
@@ -13,6 +13,7 @@
#include <linux/bitops.h>
#include <linux/io-pgtable.h>
#include <linux/kernel.h>
+#include <linux/module.h>
#include <linux/sizes.h>
#include <linux/slab.h>
#include <linux/types.h>
@@ -1047,26 +1048,31 @@ struct io_pgtable_init_fns
io_pgtable_arm_64_lpae_s1_init_fns = {
.alloc = arm_64_lpae_alloc_pgtable_s1,
.free = arm_lpae_free_pgtable,
};
+EXPORT_SYMBOL_GPL(io_pgtable_arm_64_lpae_s1_init_fns);
struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s2_init_fns =
{
.alloc = arm_64_lpae_alloc_pgtable_s2,
.free = arm_lpae_free_pgtable,
};
+EXPORT_SYMBOL_GPL(io_pgtable_arm_64_lpae_s2_init_fns);
struct io_pgtable_init_fns io_pgtable_arm_32_lpae_s1_init_fns =
{
.alloc = arm_32_lpae_alloc_pgtable_s1,
.free = arm_lpae_free_pgtable,
};
+EXPORT_SYMBOL_GPL(io_pgtable_arm_32_lpae_s1_init_fns);
struct io_pgtable_init_fns io_pgtable_arm_32_lpae_s2_init_fns =
{
.alloc = arm_32_lpae_alloc_pgtable_s2,
.free = arm_lpae_free_pgtable,
};
+EXPORT_SYMBOL_GPL(io_pgtable_arm_32_lpae_s2_init_fns);
struct io_pgtable_init_fns io_pgtable_arm_mali_lpae_init_fns = {
.alloc = arm_mali_lpae_alloc_pgtable,
.free = arm_lpae_free_pgtable,
};
+EXPORT_SYMBOL_GPL(io_pgtable_arm_mali_lpae_init_fns);
#ifdef CONFIG_IOMMU_IO_PGTABLE_LPAE_SELFTEST
@@ -1252,3 +1258,5 @@ static int __init
arm_lpae_do_selftests(void)
}
subsys_initcall(arm_lpae_do_selftests);
#endif
+
+MODULE_LICENSE("GPL v2");