On 2020-12-18 04:38, Robin Murphy wrote:
Thanks for the feedback, Robin. This is an avenue I had explored a bit when modularizing the code. However,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.
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.
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.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.
Thanks, Isaac
Signed-off-by: Isaac J. Manjarres <isa...@codeaurora.org> --- 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.cindex 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.cindex 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");
