* [RFC PATCH 0/3] iommu: Permit modular builds of io-pgtable drivers @ 2020-12-18 8:38 Isaac J. Manjarres 2020-12-18 8:38 ` [PATCH 1/3] iommu/io-pgtable-arm: Prepare for modularization Isaac J. Manjarres ` (2 more replies) 0 siblings, 3 replies; 8+ messages in thread From: Isaac J. Manjarres @ 2020-12-18 8:38 UTC (permalink / raw) To: iommu, linux-arm-kernel, linux-kernel Cc: Isaac J. Manjarres, will, joro, robin.murphy, pdaly, pratikp, kernel-team The goal of the Generic Kernel Image (GKI) effort is to have a common image that works across multiple Android devices. This involves generating a kernel image that has core features integrated into it, while SoC specific functionality can be added to the kernel for the device as a module. Along with modularizing IOMMU drivers, this also means building the io-pgtable code as modules, which allows for SoC vendors to only include the io-pgtable implementations that they use. For example, GKI for arm64 must include support for both the IOMMU ARM LPAE/V7S formats at the moment. Having the code for both formats as modules allows SoC vendors to only provide the page table format that they use, along with their IOMMU driver. Modularizing both io-pgtable.c, as well as the io-pgtable-arm[-v7s].c files, works out rather nicely, as the main interface that clients use to interact with the page tables is already exported (i.e. alloc_io_pgtable_ops and free_io_pgtable_ops). It also makes it so that neither the io-pgtable-arm[-v7s] modules or the io-pgtable modules can be unloaded without unloading the IOMMU driver, which can only happen when there aren't any references to the IOMMU driver module. Thanks in advance for the feedback, Isaac J. Manjarres Isaac J. Manjarres (3): iommu/io-pgtable-arm: Prepare for modularization iommu/io-pgtable: Prepare for modularization iommu/io-pgtable: Allow building as a module drivers/iommu/Kconfig | 6 +++--- drivers/iommu/io-pgtable-arm-v7s.c | 4 ++++ drivers/iommu/io-pgtable-arm.c | 8 ++++++++ drivers/iommu/io-pgtable.c | 7 +++++-- 4 files changed, 20 insertions(+), 5 deletions(-) -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/3] iommu/io-pgtable-arm: Prepare for modularization 2020-12-18 8:38 [RFC PATCH 0/3] iommu: Permit modular builds of io-pgtable drivers Isaac J. Manjarres @ 2020-12-18 8:38 ` Isaac J. Manjarres 2020-12-18 12:38 ` Robin Murphy 2020-12-18 8:38 ` [PATCH 2/3] iommu/io-pgtable: " Isaac J. Manjarres 2020-12-18 8:38 ` [PATCH 3/3] iommu/io-pgtable: Allow building as a module Isaac J. Manjarres 2 siblings, 1 reply; 8+ messages in thread From: Isaac J. Manjarres @ 2020-12-18 8:38 UTC (permalink / raw) To: iommu, linux-arm-kernel, linux-kernel Cc: Isaac J. Manjarres, will, joro, robin.murphy, pdaly, pratikp, kernel-team 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. Signed-off-by: Isaac J. Manjarres <isaacm@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.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"); -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/3] iommu/io-pgtable-arm: Prepare for modularization 2020-12-18 8:38 ` [PATCH 1/3] iommu/io-pgtable-arm: Prepare for modularization Isaac J. Manjarres @ 2020-12-18 12:38 ` Robin Murphy 2020-12-18 18:59 ` isaacm 0 siblings, 1 reply; 8+ messages in thread From: Robin Murphy @ 2020-12-18 12:38 UTC (permalink / raw) To: Isaac J. Manjarres, iommu, linux-arm-kernel, linux-kernel Cc: will, joro, pdaly, pratikp, kernel-team 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. > Signed-off-by: Isaac J. Manjarres <isaacm@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.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"); > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/3] iommu/io-pgtable-arm: Prepare for modularization 2020-12-18 12:38 ` Robin Murphy @ 2020-12-18 18:59 ` isaacm 2020-12-21 15:22 ` Robin Murphy 0 siblings, 1 reply; 8+ messages in thread From: isaacm @ 2020-12-18 18:59 UTC (permalink / raw) To: Robin Murphy Cc: iommu, linux-arm-kernel, linux-kernel, will, joro, pdaly, pratikp, kernel-team 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. 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 <isaacm@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.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"); >> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/3] iommu/io-pgtable-arm: Prepare for modularization 2020-12-18 18:59 ` isaacm @ 2020-12-21 15:22 ` Robin Murphy 2020-12-22 0:54 ` isaacm 0 siblings, 1 reply; 8+ messages in thread From: Robin Murphy @ 2020-12-21 15:22 UTC (permalink / raw) To: isaacm Cc: iommu, linux-arm-kernel, linux-kernel, will, joro, pdaly, pratikp, kernel-team On 2020-12-18 18:59, isaacm@codeaurora.org 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? > 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... > 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. > > Thanks, > Isaac >>> Signed-off-by: Isaac J. Manjarres <isaacm@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.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"); >>> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/3] iommu/io-pgtable-arm: Prepare for modularization 2020-12-21 15:22 ` Robin Murphy @ 2020-12-22 0:54 ` isaacm 0 siblings, 0 replies; 8+ messages in thread From: isaacm @ 2020-12-22 0:54 UTC (permalink / raw) To: Robin Murphy Cc: iommu, linux-arm-kernel, linux-kernel, will, joro, pdaly, pratikp, kernel-team On 2020-12-21 07:22, Robin Murphy wrote: > On 2020-12-18 18:59, isaacm@codeaurora.org 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@codeaurora.org/T/#t Thanks, Isaac >> >> Thanks, >> Isaac >>>> Signed-off-by: Isaac J. Manjarres <isaacm@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.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"); >>>> ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/3] iommu/io-pgtable: Prepare for modularization 2020-12-18 8:38 [RFC PATCH 0/3] iommu: Permit modular builds of io-pgtable drivers Isaac J. Manjarres 2020-12-18 8:38 ` [PATCH 1/3] iommu/io-pgtable-arm: Prepare for modularization Isaac J. Manjarres @ 2020-12-18 8:38 ` Isaac J. Manjarres 2020-12-18 8:38 ` [PATCH 3/3] iommu/io-pgtable: Allow building as a module Isaac J. Manjarres 2 siblings, 0 replies; 8+ messages in thread From: Isaac J. Manjarres @ 2020-12-18 8:38 UTC (permalink / raw) To: iommu, linux-arm-kernel, linux-kernel Cc: Isaac J. Manjarres, will, joro, robin.murphy, pdaly, pratikp, kernel-team The io-pgtable source file uses the #ifdef preprocessor macro to construct the io_pgtable_init_table structure. However, the #ifdef macro evaluates to true if the config it is testing is set to y. This is not ideal when the configs that the io-pgtable code checks for can be m, so use IS_ENABLED() instead. Also, add the GPL v2 module license to the io-pgtable source file. Signed-off-by: Isaac J. Manjarres <isaacm@codeaurora.org> --- drivers/iommu/io-pgtable.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/drivers/iommu/io-pgtable.c b/drivers/iommu/io-pgtable.c index 94394c8..867ecb4 100644 --- a/drivers/iommu/io-pgtable.c +++ b/drivers/iommu/io-pgtable.c @@ -10,18 +10,19 @@ #include <linux/bug.h> #include <linux/io-pgtable.h> #include <linux/kernel.h> +#include <linux/module.h> #include <linux/types.h> static const struct io_pgtable_init_fns * io_pgtable_init_table[IO_PGTABLE_NUM_FMTS] = { -#ifdef CONFIG_IOMMU_IO_PGTABLE_LPAE +#if IS_ENABLED(CONFIG_IOMMU_IO_PGTABLE_LPAE) [ARM_32_LPAE_S1] = &io_pgtable_arm_32_lpae_s1_init_fns, [ARM_32_LPAE_S2] = &io_pgtable_arm_32_lpae_s2_init_fns, [ARM_64_LPAE_S1] = &io_pgtable_arm_64_lpae_s1_init_fns, [ARM_64_LPAE_S2] = &io_pgtable_arm_64_lpae_s2_init_fns, [ARM_MALI_LPAE] = &io_pgtable_arm_mali_lpae_init_fns, #endif -#ifdef CONFIG_IOMMU_IO_PGTABLE_ARMV7S +#if IS_ENABLED(CONFIG_IOMMU_IO_PGTABLE_ARMV7S) [ARM_V7S] = &io_pgtable_arm_v7s_init_fns, #endif }; @@ -68,3 +69,5 @@ void free_io_pgtable_ops(struct io_pgtable_ops *ops) io_pgtable_init_table[iop->fmt]->free(iop); } EXPORT_SYMBOL_GPL(free_io_pgtable_ops); + +MODULE_LICENSE("GPL v2"); -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 3/3] iommu/io-pgtable: Allow building as a module 2020-12-18 8:38 [RFC PATCH 0/3] iommu: Permit modular builds of io-pgtable drivers Isaac J. Manjarres 2020-12-18 8:38 ` [PATCH 1/3] iommu/io-pgtable-arm: Prepare for modularization Isaac J. Manjarres 2020-12-18 8:38 ` [PATCH 2/3] iommu/io-pgtable: " Isaac J. Manjarres @ 2020-12-18 8:38 ` Isaac J. Manjarres 2 siblings, 0 replies; 8+ messages in thread From: Isaac J. Manjarres @ 2020-12-18 8:38 UTC (permalink / raw) To: iommu, linux-arm-kernel, linux-kernel Cc: Isaac J. Manjarres, will, joro, robin.murphy, pdaly, pratikp, kernel-team Now that all of the required symbols have been exported, and the io-pgtable code can correctly refer to the io-pgtable init functions when their source files are built as modules, allow the io-pgtable code to be built as a module. The expectation is that the io-pgtable core code, along with the descriptor format (either or both ARM LPAE and ARMV7S) can be built as modules. Signed-off-by: Isaac J. Manjarres <isaacm@codeaurora.org> --- drivers/iommu/Kconfig | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig index 192ef8f..d7de6db 100644 --- a/drivers/iommu/Kconfig +++ b/drivers/iommu/Kconfig @@ -27,10 +27,10 @@ menu "Generic IOMMU Pagetable Support" # Selected by the actual pagetable implementations config IOMMU_IO_PGTABLE - bool + tristate config IOMMU_IO_PGTABLE_LPAE - bool "ARMv7/v8 Long Descriptor Format" + tristate "ARMv7/v8 Long Descriptor Format" select IOMMU_IO_PGTABLE depends on ARM || ARM64 || (COMPILE_TEST && !GENERIC_ATOMIC64) help @@ -49,7 +49,7 @@ config IOMMU_IO_PGTABLE_LPAE_SELFTEST If unsure, say N here. config IOMMU_IO_PGTABLE_ARMV7S - bool "ARMv7/v8 Short Descriptor Format" + tristate "ARMv7/v8 Short Descriptor Format" select IOMMU_IO_PGTABLE depends on ARM || ARM64 || COMPILE_TEST help -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-12-22 0:56 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-12-18 8:38 [RFC PATCH 0/3] iommu: Permit modular builds of io-pgtable drivers Isaac J. Manjarres 2020-12-18 8:38 ` [PATCH 1/3] iommu/io-pgtable-arm: Prepare for modularization Isaac J. Manjarres 2020-12-18 12:38 ` Robin Murphy 2020-12-18 18:59 ` isaacm 2020-12-21 15:22 ` Robin Murphy 2020-12-22 0:54 ` isaacm 2020-12-18 8:38 ` [PATCH 2/3] iommu/io-pgtable: " Isaac J. Manjarres 2020-12-18 8:38 ` [PATCH 3/3] iommu/io-pgtable: Allow building as a module Isaac J. Manjarres
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).