| Message ID | 1608597876-32367-2-git-send-email-isaacm@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iommu: Permit modular builds of io-pgtable drivers | expand |
On 2020-12-22 00:44, Isaac J. Manjarres wrote: > The io-pgtable code constructs an array of init functions for each > page table format at compile time. This is not ideal, as this > increases the footprint of the io-pgtable code, as well as prevents > io-pgtable formats from being built as kernel modules. > > In preparation for modularizing the io-pgtable formats, switch to a > dynamic registration scheme, where each io-pgtable format can register > their init functions with the io-pgtable code at boot or module > insertion time. > > Signed-off-by: Isaac J. Manjarres <isaacm@codeaurora.org> > --- > drivers/iommu/io-pgtable-arm-v7s.c | 34 +++++++++++++- > drivers/iommu/io-pgtable-arm.c | 90 ++++++++++++++++++++++++++---------- > drivers/iommu/io-pgtable.c | 94 ++++++++++++++++++++++++++++++++------ > include/linux/io-pgtable.h | 51 +++++++++++++-------- > 4 files changed, 209 insertions(+), 60 deletions(-) > > diff --git a/drivers/iommu/io-pgtable-arm-v7s.c b/drivers/iommu/io-pgtable-arm-v7s.c > index 1d92ac9..89aad2f 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> > @@ -835,7 +836,8 @@ static struct io_pgtable *arm_v7s_alloc_pgtable(struct io_pgtable_cfg *cfg, > return NULL; > } > > -struct io_pgtable_init_fns io_pgtable_arm_v7s_init_fns = { > +static struct io_pgtable_init_fns io_pgtable_arm_v7s_init_fns = { > + .fmt = ARM_V7S, > .alloc = arm_v7s_alloc_pgtable, > .free = arm_v7s_free_pgtable, > }; > @@ -982,5 +984,33 @@ static int __init arm_v7s_do_selftests(void) > pr_info("self test ok\n"); > return 0; > } > -subsys_initcall(arm_v7s_do_selftests); > +#else > +static int arm_v7s_do_selftests(void) > +{ > + return 0; > +} > #endif > + > +static int __init arm_v7s_init(void) > +{ > + int ret; > + > + ret = io_pgtable_ops_register(&io_pgtable_arm_v7s_init_fns); > + if (ret < 0) { > + pr_err("Failed to register ARM V7S format\n"); Super-nit: I think "v7s" should probably be lowercase there. Also general consistency WRT to showing the error code and whether or not to abbreviate "format" would be nice. > + return ret; > + } > + > + ret = arm_v7s_do_selftests(); > + if (ret < 0) > + io_pgtable_ops_unregister(&io_pgtable_arm_v7s_init_fns); > + > + return ret; > +} > +core_initcall(arm_v7s_init); > + > +static void __exit arm_v7s_exit(void) > +{ > + io_pgtable_ops_unregister(&io_pgtable_arm_v7s_init_fns); > +} > +module_exit(arm_v7s_exit); > diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c > index 87def58..ff0ea2f 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> > @@ -1043,29 +1044,32 @@ arm_mali_lpae_alloc_pgtable(struct io_pgtable_cfg *cfg, void *cookie) > return NULL; > } > > -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, > -}; > - > -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, > -}; > - > -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, > -}; > - > -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, > -}; > - > -struct io_pgtable_init_fns io_pgtable_arm_mali_lpae_init_fns = { > - .alloc = arm_mali_lpae_alloc_pgtable, > - .free = arm_lpae_free_pgtable, > +static struct io_pgtable_init_fns io_pgtable_arm_lpae_init_fns[] = { > + { > + .fmt = ARM_32_LPAE_S1, > + .alloc = arm_32_lpae_alloc_pgtable_s1, > + .free = arm_lpae_free_pgtable, > + }, > + { > + .fmt = ARM_32_LPAE_S2, > + .alloc = arm_32_lpae_alloc_pgtable_s2, > + .free = arm_lpae_free_pgtable, > + }, > + { > + .fmt = ARM_64_LPAE_S1, > + .alloc = arm_64_lpae_alloc_pgtable_s1, > + .free = arm_lpae_free_pgtable, > + }, > + { > + .fmt = ARM_64_LPAE_S2, > + .alloc = arm_64_lpae_alloc_pgtable_s2, > + .free = arm_lpae_free_pgtable, > + }, > + { > + .fmt = ARM_MALI_LPAE, > + .alloc = arm_mali_lpae_alloc_pgtable, > + .free = arm_lpae_free_pgtable, > + }, > }; > > #ifdef CONFIG_IOMMU_IO_PGTABLE_LPAE_SELFTEST > @@ -1250,5 +1254,43 @@ static int __init arm_lpae_do_selftests(void) > pr_info("selftest: completed with %d PASS %d FAIL\n", pass, fail); > return fail ? -EFAULT : 0; > } > -subsys_initcall(arm_lpae_do_selftests); > +#else > +static int __init arm_lpae_do_selftests(void) > +{ > + return 0; > +} > #endif > + > +static int __init arm_lpae_init(void) > +{ > + int ret, i; > + > + for (i = 0; i < ARRAY_SIZE(io_pgtable_arm_lpae_init_fns); i++) { > + ret = io_pgtable_ops_register(&io_pgtable_arm_lpae_init_fns[i]); > + if (ret < 0) { > + pr_err("Failed to register ARM LPAE fmt: %d\n"); > + goto err_io_pgtable_register; > + } > + } > + > + ret = arm_lpae_do_selftests(); > + if (ret < 0) > + goto err_io_pgtable_register; > + > + return 0; > + > +err_io_pgtable_register: > + for (i = i - 1; i >= 0; i--) Personally I find "while (i--)" a bit clearer for this kind of unwinding, but maybe post-decrement isn't to everyone's taste. > + io_pgtable_ops_unregister(&io_pgtable_arm_lpae_init_fns[i]); > + return ret; > +} > +core_initcall(arm_lpae_init); > + > +static void __exit arm_lpae_exit(void) > +{ > + int i; > + > + for (i = 0; i < ARRAY_SIZE(io_pgtable_arm_lpae_init_fns); i++) > + io_pgtable_ops_unregister(&io_pgtable_arm_lpae_init_fns[i]); > +} > +module_exit(arm_lpae_exit); > diff --git a/drivers/iommu/io-pgtable.c b/drivers/iommu/io-pgtable.c > index 94394c8..2c6eb2e 100644 > --- a/drivers/iommu/io-pgtable.c > +++ b/drivers/iommu/io-pgtable.c > @@ -10,33 +10,45 @@ > #include <linux/bug.h> > #include <linux/io-pgtable.h> > #include <linux/kernel.h> > +#include <linux/rwlock.h> > +#include <linux/slab.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 > - [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 > - [ARM_V7S] = &io_pgtable_arm_v7s_init_fns, > -#endif > +struct io_pgtable_init_fns_node { > + struct io_pgtable_init_fns *fns; > + struct list_head list; > }; > > +static LIST_HEAD(io_pgtable_init_fns_list); > +static DEFINE_RWLOCK(io_pgtable_init_fns_list_lock); > + > +static struct io_pgtable_init_fns *io_pgtable_get_init_fns(enum io_pgtable_fmt fmt) > +{ > + struct io_pgtable_init_fns_node *iter; > + struct io_pgtable_init_fns *fns = NULL; > + > + read_lock(&io_pgtable_init_fns_list_lock); > + list_for_each_entry(iter, &io_pgtable_init_fns_list, list) > + if (iter->fns->fmt == fmt) { > + fns = iter->fns; > + break; > + } > + read_unlock(&io_pgtable_init_fns_list_lock); > + > + return fns; > +} I think it would be a lot easier to stick with a simple array indexed by enum - that way you can just set/clear/test entries without needing to worry about locking. Basically just remove the const and the initialisers from the existing one ;) (and if you think you're concerned about memory, consider that just the list head plus lock is already half the size of the table) Other than that, I think this all looks pretty promising - I'd suggest sending a non-RFC after rc1 so that it gets everyone's proper attention. Thanks, Robin. > + > struct io_pgtable_ops *alloc_io_pgtable_ops(enum io_pgtable_fmt fmt, > struct io_pgtable_cfg *cfg, > void *cookie) > { > struct io_pgtable *iop; > - const struct io_pgtable_init_fns *fns; > + struct io_pgtable_init_fns *fns; > > if (fmt >= IO_PGTABLE_NUM_FMTS) > return NULL; > > - fns = io_pgtable_init_table[fmt]; > + fns = io_pgtable_get_init_fns(fmt); > if (!fns) > return NULL; > > @@ -59,12 +71,64 @@ EXPORT_SYMBOL_GPL(alloc_io_pgtable_ops); > void free_io_pgtable_ops(struct io_pgtable_ops *ops) > { > struct io_pgtable *iop; > + struct io_pgtable_init_fns *fns; > > if (!ops) > return; > > iop = io_pgtable_ops_to_pgtable(ops); > io_pgtable_tlb_flush_all(iop); > - io_pgtable_init_table[iop->fmt]->free(iop); > + fns = io_pgtable_get_init_fns(iop->fmt); > + if (fns) > + fns->free(iop); > } > EXPORT_SYMBOL_GPL(free_io_pgtable_ops); > + > +int io_pgtable_ops_register(struct io_pgtable_init_fns *init_fns) > +{ > + struct io_pgtable_init_fns_node *iter, *fns_node; > + int ret = 0; > + > + if (!init_fns || init_fns->fmt >= IO_PGTABLE_NUM_FMTS || > + !init_fns->alloc || !init_fns->free) > + return -EINVAL; > + > + fns_node = kzalloc(sizeof(*fns_node), GFP_KERNEL); > + if (!fns_node) > + return -ENOMEM; > + > + write_lock(&io_pgtable_init_fns_list_lock); > + list_for_each_entry(iter, &io_pgtable_init_fns_list, list) > + if (iter->fns->fmt == init_fns->fmt) { > + ret = -EEXIST; > + kfree(fns_node); > + break; > + } > + > + if (!ret) { > + fns_node->fns = init_fns; > + INIT_LIST_HEAD(&fns_node->list); > + list_add_tail(&fns_node->list, &io_pgtable_init_fns_list); > + } > + write_unlock(&io_pgtable_init_fns_list_lock); > + return ret; > +} > +EXPORT_SYMBOL_GPL(io_pgtable_ops_register); > + > +void io_pgtable_ops_unregister(struct io_pgtable_init_fns *init_fns) > +{ > + struct io_pgtable_init_fns_node *iter, *tmp; > + > + if (!init_fns) > + return; > + > + write_lock(&io_pgtable_init_fns_list_lock); > + list_for_each_entry_safe(iter, tmp, &io_pgtable_init_fns_list, list) > + if (iter->fns == init_fns) { > + list_del(&iter->list); > + kfree(iter); > + break; > + } > + write_unlock(&io_pgtable_init_fns_list_lock); > +} > +EXPORT_SYMBOL_GPL(io_pgtable_ops_unregister); > diff --git a/include/linux/io-pgtable.h b/include/linux/io-pgtable.h > index ea727eb..45b367ce 100644 > --- a/include/linux/io-pgtable.h > +++ b/include/linux/io-pgtable.h > @@ -163,6 +163,38 @@ struct io_pgtable_ops { > }; > > /** > + * struct io_pgtable_init_fns - Alloc/free a set of page tables for a > + * particular format. > + * > + * @fmt: The page table format. > + * @alloc: Allocate a set of page tables described by cfg. > + * @free: Free the page tables associated with iop. > + */ > +struct io_pgtable_init_fns { > + enum io_pgtable_fmt fmt; > + struct io_pgtable *(*alloc)(struct io_pgtable_cfg *cfg, void *cookie); > + void (*free)(struct io_pgtable *iop); > +}; > + > +/** > + * io_pgtable_ops_register() - Register the page table routines for a page table > + * format. > + * > + * @init_fns: The functions for allocating and freeing the page tables of > + * a particular format. > + */ > +int io_pgtable_ops_register(struct io_pgtable_init_fns *init_fns); > + > +/** > + * io_pgtable_ops_unregister() - Unregister the page table routines for a page > + * table format. > + * > + * @init_fns: The functions for allocating and freeing the page tables of > + * a particular format. > + */ > +void io_pgtable_ops_unregister(struct io_pgtable_init_fns *init_fns); > + > +/** > * alloc_io_pgtable_ops() - Allocate a page table allocator for use by an IOMMU. > * > * @fmt: The page table format. > @@ -233,23 +265,4 @@ io_pgtable_tlb_add_page(struct io_pgtable *iop, > iop->cfg.tlb->tlb_add_page(gather, iova, granule, iop->cookie); > } > > -/** > - * struct io_pgtable_init_fns - Alloc/free a set of page tables for a > - * particular format. > - * > - * @alloc: Allocate a set of page tables described by cfg. > - * @free: Free the page tables associated with iop. > - */ > -struct io_pgtable_init_fns { > - struct io_pgtable *(*alloc)(struct io_pgtable_cfg *cfg, void *cookie); > - void (*free)(struct io_pgtable *iop); > -}; > - > -extern struct io_pgtable_init_fns io_pgtable_arm_32_lpae_s1_init_fns; > -extern struct io_pgtable_init_fns io_pgtable_arm_32_lpae_s2_init_fns; > -extern struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s1_init_fns; > -extern struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s2_init_fns; > -extern struct io_pgtable_init_fns io_pgtable_arm_v7s_init_fns; > -extern struct io_pgtable_init_fns io_pgtable_arm_mali_lpae_init_fns; > - > #endif /* __IO_PGTABLE_H */ >
On 2020-12-22 11:27, Robin Murphy wrote: > On 2020-12-22 00:44, Isaac J. Manjarres wrote: >> The io-pgtable code constructs an array of init functions for each >> page table format at compile time. This is not ideal, as this >> increases the footprint of the io-pgtable code, as well as prevents >> io-pgtable formats from being built as kernel modules. >> >> In preparation for modularizing the io-pgtable formats, switch to a >> dynamic registration scheme, where each io-pgtable format can register >> their init functions with the io-pgtable code at boot or module >> insertion time. >> >> Signed-off-by: Isaac J. Manjarres <isaacm@codeaurora.org> >> --- >> drivers/iommu/io-pgtable-arm-v7s.c | 34 +++++++++++++- >> drivers/iommu/io-pgtable-arm.c | 90 >> ++++++++++++++++++++++++++---------- >> drivers/iommu/io-pgtable.c | 94 >> ++++++++++++++++++++++++++++++++------ >> include/linux/io-pgtable.h | 51 +++++++++++++-------- >> 4 files changed, 209 insertions(+), 60 deletions(-) >> >> diff --git a/drivers/iommu/io-pgtable-arm-v7s.c >> b/drivers/iommu/io-pgtable-arm-v7s.c >> index 1d92ac9..89aad2f 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> >> @@ -835,7 +836,8 @@ static struct io_pgtable >> *arm_v7s_alloc_pgtable(struct io_pgtable_cfg *cfg, >> return NULL; >> } >> -struct io_pgtable_init_fns io_pgtable_arm_v7s_init_fns = { >> +static struct io_pgtable_init_fns io_pgtable_arm_v7s_init_fns = { >> + .fmt = ARM_V7S, >> .alloc = arm_v7s_alloc_pgtable, >> .free = arm_v7s_free_pgtable, >> }; >> @@ -982,5 +984,33 @@ static int __init arm_v7s_do_selftests(void) >> pr_info("self test ok\n"); >> return 0; >> } >> -subsys_initcall(arm_v7s_do_selftests); >> +#else >> +static int arm_v7s_do_selftests(void) >> +{ >> + return 0; >> +} >> #endif >> + >> +static int __init arm_v7s_init(void) >> +{ >> + int ret; >> + >> + ret = io_pgtable_ops_register(&io_pgtable_arm_v7s_init_fns); >> + if (ret < 0) { >> + pr_err("Failed to register ARM V7S format\n"); > > Super-nit: I think "v7s" should probably be lowercase there. Also > general consistency WRT to showing the error code and whether or not > to abbreviate "format" would be nice. > Ok, I can fix this accordingly. >> + return ret; >> + } >> + >> + ret = arm_v7s_do_selftests(); >> + if (ret < 0) >> + io_pgtable_ops_unregister(&io_pgtable_arm_v7s_init_fns); >> + >> + return ret; >> +} >> +core_initcall(arm_v7s_init); >> + >> +static void __exit arm_v7s_exit(void) >> +{ >> + io_pgtable_ops_unregister(&io_pgtable_arm_v7s_init_fns); >> +} >> +module_exit(arm_v7s_exit); >> diff --git a/drivers/iommu/io-pgtable-arm.c >> b/drivers/iommu/io-pgtable-arm.c >> index 87def58..ff0ea2f 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> >> @@ -1043,29 +1044,32 @@ arm_mali_lpae_alloc_pgtable(struct >> io_pgtable_cfg *cfg, void *cookie) >> return NULL; >> } >> -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, >> -}; >> - >> -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, >> -}; >> - >> -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, >> -}; >> - >> -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, >> -}; >> - >> -struct io_pgtable_init_fns io_pgtable_arm_mali_lpae_init_fns = { >> - .alloc = arm_mali_lpae_alloc_pgtable, >> - .free = arm_lpae_free_pgtable, >> +static struct io_pgtable_init_fns io_pgtable_arm_lpae_init_fns[] = { >> + { >> + .fmt = ARM_32_LPAE_S1, >> + .alloc = arm_32_lpae_alloc_pgtable_s1, >> + .free = arm_lpae_free_pgtable, >> + }, >> + { >> + .fmt = ARM_32_LPAE_S2, >> + .alloc = arm_32_lpae_alloc_pgtable_s2, >> + .free = arm_lpae_free_pgtable, >> + }, >> + { >> + .fmt = ARM_64_LPAE_S1, >> + .alloc = arm_64_lpae_alloc_pgtable_s1, >> + .free = arm_lpae_free_pgtable, >> + }, >> + { >> + .fmt = ARM_64_LPAE_S2, >> + .alloc = arm_64_lpae_alloc_pgtable_s2, >> + .free = arm_lpae_free_pgtable, >> + }, >> + { >> + .fmt = ARM_MALI_LPAE, >> + .alloc = arm_mali_lpae_alloc_pgtable, >> + .free = arm_lpae_free_pgtable, >> + }, >> }; >> #ifdef CONFIG_IOMMU_IO_PGTABLE_LPAE_SELFTEST >> @@ -1250,5 +1254,43 @@ static int __init arm_lpae_do_selftests(void) >> pr_info("selftest: completed with %d PASS %d FAIL\n", pass, fail); >> return fail ? -EFAULT : 0; >> } >> -subsys_initcall(arm_lpae_do_selftests); >> +#else >> +static int __init arm_lpae_do_selftests(void) >> +{ >> + return 0; >> +} >> #endif >> + >> +static int __init arm_lpae_init(void) >> +{ >> + int ret, i; >> + >> + for (i = 0; i < ARRAY_SIZE(io_pgtable_arm_lpae_init_fns); i++) { >> + ret = io_pgtable_ops_register(&io_pgtable_arm_lpae_init_fns[i]); >> + if (ret < 0) { >> + pr_err("Failed to register ARM LPAE fmt: %d\n"); >> + goto err_io_pgtable_register; >> + } >> + } >> + >> + ret = arm_lpae_do_selftests(); >> + if (ret < 0) >> + goto err_io_pgtable_register; >> + >> + return 0; >> + >> +err_io_pgtable_register: >> + for (i = i - 1; i >= 0; i--) > > Personally I find "while (i--)" a bit clearer for this kind of > unwinding, but maybe post-decrement isn't to everyone's taste. > I'm not particularly attached to the current approach, so a while loop is okay with me :). >> + io_pgtable_ops_unregister(&io_pgtable_arm_lpae_init_fns[i]); >> + return ret; >> +} >> +core_initcall(arm_lpae_init); >> + >> +static void __exit arm_lpae_exit(void) >> +{ >> + int i; >> + >> + for (i = 0; i < ARRAY_SIZE(io_pgtable_arm_lpae_init_fns); i++) >> + io_pgtable_ops_unregister(&io_pgtable_arm_lpae_init_fns[i]); >> +} >> +module_exit(arm_lpae_exit); >> diff --git a/drivers/iommu/io-pgtable.c b/drivers/iommu/io-pgtable.c >> index 94394c8..2c6eb2e 100644 >> --- a/drivers/iommu/io-pgtable.c >> +++ b/drivers/iommu/io-pgtable.c >> @@ -10,33 +10,45 @@ >> #include <linux/bug.h> >> #include <linux/io-pgtable.h> >> #include <linux/kernel.h> >> +#include <linux/rwlock.h> >> +#include <linux/slab.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 >> - [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 >> - [ARM_V7S] = &io_pgtable_arm_v7s_init_fns, >> -#endif >> +struct io_pgtable_init_fns_node { >> + struct io_pgtable_init_fns *fns; >> + struct list_head list; >> }; >> +static LIST_HEAD(io_pgtable_init_fns_list); >> +static DEFINE_RWLOCK(io_pgtable_init_fns_list_lock); >> + >> +static struct io_pgtable_init_fns *io_pgtable_get_init_fns(enum >> io_pgtable_fmt fmt) >> +{ >> + struct io_pgtable_init_fns_node *iter; >> + struct io_pgtable_init_fns *fns = NULL; >> + >> + read_lock(&io_pgtable_init_fns_list_lock); >> + list_for_each_entry(iter, &io_pgtable_init_fns_list, list) >> + if (iter->fns->fmt == fmt) { >> + fns = iter->fns; >> + break; >> + } >> + read_unlock(&io_pgtable_init_fns_list_lock); >> + >> + return fns; >> +} > > I think it would be a lot easier to stick with a simple array indexed > by enum - that way you can just set/clear/test entries without needing > to worry about locking. Basically just remove the const and the > initialisers from the existing one ;) > > (and if you think you're concerned about memory, consider that just > the list head plus lock is already half the size of the table) > > Other than that, I think this all looks pretty promising - I'd suggest > sending a non-RFC after rc1 so that it gets everyone's proper > attention. > > Thanks, > Robin. > Thanks for all of the feedback! With respect to the comment about keeping the current array, don't we need some sort of lock to protect access to the data structure (e.g. concurrent access if adding two ops for the same format)? Or do you mean that we should use atomic operations to handle this cleanly? Thanks, Isaac >> + >> struct io_pgtable_ops *alloc_io_pgtable_ops(enum io_pgtable_fmt fmt, >> struct io_pgtable_cfg *cfg, >> void *cookie) >> { >> struct io_pgtable *iop; >> - const struct io_pgtable_init_fns *fns; >> + struct io_pgtable_init_fns *fns; >> if (fmt >= IO_PGTABLE_NUM_FMTS) >> return NULL; >> - fns = io_pgtable_init_table[fmt]; >> + fns = io_pgtable_get_init_fns(fmt); >> if (!fns) >> return NULL; >> @@ -59,12 +71,64 @@ EXPORT_SYMBOL_GPL(alloc_io_pgtable_ops); >> void free_io_pgtable_ops(struct io_pgtable_ops *ops) >> { >> struct io_pgtable *iop; >> + struct io_pgtable_init_fns *fns; >> if (!ops) >> return; >> iop = io_pgtable_ops_to_pgtable(ops); >> io_pgtable_tlb_flush_all(iop); >> - io_pgtable_init_table[iop->fmt]->free(iop); >> + fns = io_pgtable_get_init_fns(iop->fmt); >> + if (fns) >> + fns->free(iop); >> } >> EXPORT_SYMBOL_GPL(free_io_pgtable_ops); >> + >> +int io_pgtable_ops_register(struct io_pgtable_init_fns *init_fns) >> +{ >> + struct io_pgtable_init_fns_node *iter, *fns_node; >> + int ret = 0; >> + >> + if (!init_fns || init_fns->fmt >= IO_PGTABLE_NUM_FMTS || >> + !init_fns->alloc || !init_fns->free) >> + return -EINVAL; >> + >> + fns_node = kzalloc(sizeof(*fns_node), GFP_KERNEL); >> + if (!fns_node) >> + return -ENOMEM; >> + >> + write_lock(&io_pgtable_init_fns_list_lock); >> + list_for_each_entry(iter, &io_pgtable_init_fns_list, list) >> + if (iter->fns->fmt == init_fns->fmt) { >> + ret = -EEXIST; >> + kfree(fns_node); >> + break; >> + } >> + >> + if (!ret) { >> + fns_node->fns = init_fns; >> + INIT_LIST_HEAD(&fns_node->list); >> + list_add_tail(&fns_node->list, &io_pgtable_init_fns_list); >> + } >> + write_unlock(&io_pgtable_init_fns_list_lock); >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(io_pgtable_ops_register); >> + >> +void io_pgtable_ops_unregister(struct io_pgtable_init_fns *init_fns) >> +{ >> + struct io_pgtable_init_fns_node *iter, *tmp; >> + >> + if (!init_fns) >> + return; >> + >> + write_lock(&io_pgtable_init_fns_list_lock); >> + list_for_each_entry_safe(iter, tmp, &io_pgtable_init_fns_list, list) >> + if (iter->fns == init_fns) { >> + list_del(&iter->list); >> + kfree(iter); >> + break; >> + } >> + write_unlock(&io_pgtable_init_fns_list_lock); >> +} >> +EXPORT_SYMBOL_GPL(io_pgtable_ops_unregister); >> diff --git a/include/linux/io-pgtable.h b/include/linux/io-pgtable.h >> index ea727eb..45b367ce 100644 >> --- a/include/linux/io-pgtable.h >> +++ b/include/linux/io-pgtable.h >> @@ -163,6 +163,38 @@ struct io_pgtable_ops { >> }; >> /** >> + * struct io_pgtable_init_fns - Alloc/free a set of page tables for a >> + * particular format. >> + * >> + * @fmt: The page table format. >> + * @alloc: Allocate a set of page tables described by cfg. >> + * @free: Free the page tables associated with iop. >> + */ >> +struct io_pgtable_init_fns { >> + enum io_pgtable_fmt fmt; >> + struct io_pgtable *(*alloc)(struct io_pgtable_cfg *cfg, void >> *cookie); >> + void (*free)(struct io_pgtable *iop); >> +}; >> + >> +/** >> + * io_pgtable_ops_register() - Register the page table routines for a >> page table >> + * format. >> + * >> + * @init_fns: The functions for allocating and freeing the page >> tables of >> + * a particular format. >> + */ >> +int io_pgtable_ops_register(struct io_pgtable_init_fns *init_fns); >> + >> +/** >> + * io_pgtable_ops_unregister() - Unregister the page table routines >> for a page >> + * table format. >> + * >> + * @init_fns: The functions for allocating and freeing the page >> tables of >> + * a particular format. >> + */ >> +void io_pgtable_ops_unregister(struct io_pgtable_init_fns *init_fns); >> + >> +/** >> * alloc_io_pgtable_ops() - Allocate a page table allocator for use >> by an IOMMU. >> * >> * @fmt: The page table format. >> @@ -233,23 +265,4 @@ io_pgtable_tlb_add_page(struct io_pgtable *iop, >> iop->cfg.tlb->tlb_add_page(gather, iova, granule, iop->cookie); >> } >> -/** >> - * struct io_pgtable_init_fns - Alloc/free a set of page tables for a >> - * particular format. >> - * >> - * @alloc: Allocate a set of page tables described by cfg. >> - * @free: Free the page tables associated with iop. >> - */ >> -struct io_pgtable_init_fns { >> - struct io_pgtable *(*alloc)(struct io_pgtable_cfg *cfg, void >> *cookie); >> - void (*free)(struct io_pgtable *iop); >> -}; >> - >> -extern struct io_pgtable_init_fns io_pgtable_arm_32_lpae_s1_init_fns; >> -extern struct io_pgtable_init_fns io_pgtable_arm_32_lpae_s2_init_fns; >> -extern struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s1_init_fns; >> -extern struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s2_init_fns; >> -extern struct io_pgtable_init_fns io_pgtable_arm_v7s_init_fns; >> -extern struct io_pgtable_init_fns io_pgtable_arm_mali_lpae_init_fns; >> - >> #endif /* __IO_PGTABLE_H */ >> > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 2020-12-22 19:54, isaacm@codeaurora.org wrote: > On 2020-12-22 11:27, Robin Murphy wrote: >> On 2020-12-22 00:44, Isaac J. Manjarres wrote: >>> The io-pgtable code constructs an array of init functions for each >>> page table format at compile time. This is not ideal, as this >>> increases the footprint of the io-pgtable code, as well as prevents >>> io-pgtable formats from being built as kernel modules. >>> >>> In preparation for modularizing the io-pgtable formats, switch to a >>> dynamic registration scheme, where each io-pgtable format can register >>> their init functions with the io-pgtable code at boot or module >>> insertion time. >>> >>> Signed-off-by: Isaac J. Manjarres <isaacm@codeaurora.org> >>> --- >>> drivers/iommu/io-pgtable-arm-v7s.c | 34 +++++++++++++- >>> drivers/iommu/io-pgtable-arm.c | 90 >>> ++++++++++++++++++++++++++---------- >>> drivers/iommu/io-pgtable.c | 94 >>> ++++++++++++++++++++++++++++++++------ >>> include/linux/io-pgtable.h | 51 +++++++++++++-------- >>> 4 files changed, 209 insertions(+), 60 deletions(-) >>> >>> diff --git a/drivers/iommu/io-pgtable-arm-v7s.c >>> b/drivers/iommu/io-pgtable-arm-v7s.c >>> index 1d92ac9..89aad2f 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> >>> @@ -835,7 +836,8 @@ static struct io_pgtable >>> *arm_v7s_alloc_pgtable(struct io_pgtable_cfg *cfg, >>> return NULL; >>> } >>> -struct io_pgtable_init_fns io_pgtable_arm_v7s_init_fns = { >>> +static struct io_pgtable_init_fns io_pgtable_arm_v7s_init_fns = { >>> + .fmt = ARM_V7S, >>> .alloc = arm_v7s_alloc_pgtable, >>> .free = arm_v7s_free_pgtable, >>> }; >>> @@ -982,5 +984,33 @@ static int __init arm_v7s_do_selftests(void) >>> pr_info("self test ok\n"); >>> return 0; >>> } >>> -subsys_initcall(arm_v7s_do_selftests); >>> +#else >>> +static int arm_v7s_do_selftests(void) >>> +{ >>> + return 0; >>> +} >>> #endif >>> + >>> +static int __init arm_v7s_init(void) >>> +{ >>> + int ret; >>> + >>> + ret = io_pgtable_ops_register(&io_pgtable_arm_v7s_init_fns); >>> + if (ret < 0) { >>> + pr_err("Failed to register ARM V7S format\n"); >> >> Super-nit: I think "v7s" should probably be lowercase there. Also >> general consistency WRT to showing the error code and whether or not >> to abbreviate "format" would be nice. >> > Ok, I can fix this accordingly. > >>> + return ret; >>> + } >>> + >>> + ret = arm_v7s_do_selftests(); >>> + if (ret < 0) >>> + io_pgtable_ops_unregister(&io_pgtable_arm_v7s_init_fns); >>> + >>> + return ret; >>> +} >>> +core_initcall(arm_v7s_init); >>> + >>> +static void __exit arm_v7s_exit(void) >>> +{ >>> + io_pgtable_ops_unregister(&io_pgtable_arm_v7s_init_fns); >>> +} >>> +module_exit(arm_v7s_exit); >>> diff --git a/drivers/iommu/io-pgtable-arm.c >>> b/drivers/iommu/io-pgtable-arm.c >>> index 87def58..ff0ea2f 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> >>> @@ -1043,29 +1044,32 @@ arm_mali_lpae_alloc_pgtable(struct >>> io_pgtable_cfg *cfg, void *cookie) >>> return NULL; >>> } >>> -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, >>> -}; >>> - >>> -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, >>> -}; >>> - >>> -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, >>> -}; >>> - >>> -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, >>> -}; >>> - >>> -struct io_pgtable_init_fns io_pgtable_arm_mali_lpae_init_fns = { >>> - .alloc = arm_mali_lpae_alloc_pgtable, >>> - .free = arm_lpae_free_pgtable, >>> +static struct io_pgtable_init_fns io_pgtable_arm_lpae_init_fns[] = { >>> + { >>> + .fmt = ARM_32_LPAE_S1, >>> + .alloc = arm_32_lpae_alloc_pgtable_s1, >>> + .free = arm_lpae_free_pgtable, >>> + }, >>> + { >>> + .fmt = ARM_32_LPAE_S2, >>> + .alloc = arm_32_lpae_alloc_pgtable_s2, >>> + .free = arm_lpae_free_pgtable, >>> + }, >>> + { >>> + .fmt = ARM_64_LPAE_S1, >>> + .alloc = arm_64_lpae_alloc_pgtable_s1, >>> + .free = arm_lpae_free_pgtable, >>> + }, >>> + { >>> + .fmt = ARM_64_LPAE_S2, >>> + .alloc = arm_64_lpae_alloc_pgtable_s2, >>> + .free = arm_lpae_free_pgtable, >>> + }, >>> + { >>> + .fmt = ARM_MALI_LPAE, >>> + .alloc = arm_mali_lpae_alloc_pgtable, >>> + .free = arm_lpae_free_pgtable, >>> + }, >>> }; >>> #ifdef CONFIG_IOMMU_IO_PGTABLE_LPAE_SELFTEST >>> @@ -1250,5 +1254,43 @@ static int __init arm_lpae_do_selftests(void) >>> pr_info("selftest: completed with %d PASS %d FAIL\n", pass, fail); >>> return fail ? -EFAULT : 0; >>> } >>> -subsys_initcall(arm_lpae_do_selftests); >>> +#else >>> +static int __init arm_lpae_do_selftests(void) >>> +{ >>> + return 0; >>> +} >>> #endif >>> + >>> +static int __init arm_lpae_init(void) >>> +{ >>> + int ret, i; >>> + >>> + for (i = 0; i < ARRAY_SIZE(io_pgtable_arm_lpae_init_fns); i++) { >>> + ret = >>> io_pgtable_ops_register(&io_pgtable_arm_lpae_init_fns[i]); >>> + if (ret < 0) { >>> + pr_err("Failed to register ARM LPAE fmt: %d\n"); >>> + goto err_io_pgtable_register; >>> + } >>> + } >>> + >>> + ret = arm_lpae_do_selftests(); >>> + if (ret < 0) >>> + goto err_io_pgtable_register; >>> + >>> + return 0; >>> + >>> +err_io_pgtable_register: >>> + for (i = i - 1; i >= 0; i--) >> >> Personally I find "while (i--)" a bit clearer for this kind of >> unwinding, but maybe post-decrement isn't to everyone's taste. >> > I'm not particularly attached to the current approach, so a while loop > is okay with me :). > >>> + io_pgtable_ops_unregister(&io_pgtable_arm_lpae_init_fns[i]); >>> + return ret; >>> +} >>> +core_initcall(arm_lpae_init); >>> + >>> +static void __exit arm_lpae_exit(void) >>> +{ >>> + int i; >>> + >>> + for (i = 0; i < ARRAY_SIZE(io_pgtable_arm_lpae_init_fns); i++) >>> + io_pgtable_ops_unregister(&io_pgtable_arm_lpae_init_fns[i]); >>> +} >>> +module_exit(arm_lpae_exit); >>> diff --git a/drivers/iommu/io-pgtable.c b/drivers/iommu/io-pgtable.c >>> index 94394c8..2c6eb2e 100644 >>> --- a/drivers/iommu/io-pgtable.c >>> +++ b/drivers/iommu/io-pgtable.c >>> @@ -10,33 +10,45 @@ >>> #include <linux/bug.h> >>> #include <linux/io-pgtable.h> >>> #include <linux/kernel.h> >>> +#include <linux/rwlock.h> >>> +#include <linux/slab.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 >>> - [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 >>> - [ARM_V7S] = &io_pgtable_arm_v7s_init_fns, >>> -#endif >>> +struct io_pgtable_init_fns_node { >>> + struct io_pgtable_init_fns *fns; >>> + struct list_head list; >>> }; >>> +static LIST_HEAD(io_pgtable_init_fns_list); >>> +static DEFINE_RWLOCK(io_pgtable_init_fns_list_lock); >>> + >>> +static struct io_pgtable_init_fns *io_pgtable_get_init_fns(enum >>> io_pgtable_fmt fmt) >>> +{ >>> + struct io_pgtable_init_fns_node *iter; >>> + struct io_pgtable_init_fns *fns = NULL; >>> + >>> + read_lock(&io_pgtable_init_fns_list_lock); >>> + list_for_each_entry(iter, &io_pgtable_init_fns_list, list) >>> + if (iter->fns->fmt == fmt) { >>> + fns = iter->fns; >>> + break; >>> + } >>> + read_unlock(&io_pgtable_init_fns_list_lock); >>> + >>> + return fns; >>> +} >> >> I think it would be a lot easier to stick with a simple array indexed >> by enum - that way you can just set/clear/test entries without needing >> to worry about locking. Basically just remove the const and the >> initialisers from the existing one ;) >> >> (and if you think you're concerned about memory, consider that just >> the list head plus lock is already half the size of the table) >> >> Other than that, I think this all looks pretty promising - I'd suggest >> sending a non-RFC after rc1 so that it gets everyone's proper >> attention. >> >> Thanks, >> Robin. >> > > Thanks for all of the feedback! With respect to the comment about > keeping the current array, > don't we need some sort of lock to protect access to the data structure > (e.g. concurrent access > if adding two ops for the same format)? Or do you mean that we should > use atomic operations to > handle this cleanly? I don't think any races are realistically possible - built-in initcalls should run sequentially, while module init should be serialised by the module loader lock, so registrations should never be able to race against each other. And if there could be a race between registering a format and somebody trying to use it, then something's fundamentally wrong anyway - sure we could use READ_ONCE/WRITE_ONCE for accessing the table, but that would be basically be admitting that we've failed at the primary purpose of making sure formats are loaded *before* the users that depend on them. With a directly-indexed table there's no contention between looking up one format and registering another unrelated format, unlike with a list. Robin. > > Thanks, > Isaac >>> + >>> struct io_pgtable_ops *alloc_io_pgtable_ops(enum io_pgtable_fmt fmt, >>> struct io_pgtable_cfg *cfg, >>> void *cookie) >>> { >>> struct io_pgtable *iop; >>> - const struct io_pgtable_init_fns *fns; >>> + struct io_pgtable_init_fns *fns; >>> if (fmt >= IO_PGTABLE_NUM_FMTS) >>> return NULL; >>> - fns = io_pgtable_init_table[fmt]; >>> + fns = io_pgtable_get_init_fns(fmt); >>> if (!fns) >>> return NULL; >>> @@ -59,12 +71,64 @@ EXPORT_SYMBOL_GPL(alloc_io_pgtable_ops); >>> void free_io_pgtable_ops(struct io_pgtable_ops *ops) >>> { >>> struct io_pgtable *iop; >>> + struct io_pgtable_init_fns *fns; >>> if (!ops) >>> return; >>> iop = io_pgtable_ops_to_pgtable(ops); >>> io_pgtable_tlb_flush_all(iop); >>> - io_pgtable_init_table[iop->fmt]->free(iop); >>> + fns = io_pgtable_get_init_fns(iop->fmt); >>> + if (fns) >>> + fns->free(iop); >>> } >>> EXPORT_SYMBOL_GPL(free_io_pgtable_ops); >>> + >>> +int io_pgtable_ops_register(struct io_pgtable_init_fns *init_fns) >>> +{ >>> + struct io_pgtable_init_fns_node *iter, *fns_node; >>> + int ret = 0; >>> + >>> + if (!init_fns || init_fns->fmt >= IO_PGTABLE_NUM_FMTS || >>> + !init_fns->alloc || !init_fns->free) >>> + return -EINVAL; >>> + >>> + fns_node = kzalloc(sizeof(*fns_node), GFP_KERNEL); >>> + if (!fns_node) >>> + return -ENOMEM; >>> + >>> + write_lock(&io_pgtable_init_fns_list_lock); >>> + list_for_each_entry(iter, &io_pgtable_init_fns_list, list) >>> + if (iter->fns->fmt == init_fns->fmt) { >>> + ret = -EEXIST; >>> + kfree(fns_node); >>> + break; >>> + } >>> + >>> + if (!ret) { >>> + fns_node->fns = init_fns; >>> + INIT_LIST_HEAD(&fns_node->list); >>> + list_add_tail(&fns_node->list, &io_pgtable_init_fns_list); >>> + } >>> + write_unlock(&io_pgtable_init_fns_list_lock); >>> + return ret; >>> +} >>> +EXPORT_SYMBOL_GPL(io_pgtable_ops_register); >>> + >>> +void io_pgtable_ops_unregister(struct io_pgtable_init_fns *init_fns) >>> +{ >>> + struct io_pgtable_init_fns_node *iter, *tmp; >>> + >>> + if (!init_fns) >>> + return; >>> + >>> + write_lock(&io_pgtable_init_fns_list_lock); >>> + list_for_each_entry_safe(iter, tmp, &io_pgtable_init_fns_list, >>> list) >>> + if (iter->fns == init_fns) { >>> + list_del(&iter->list); >>> + kfree(iter); >>> + break; >>> + } >>> + write_unlock(&io_pgtable_init_fns_list_lock); >>> +} >>> +EXPORT_SYMBOL_GPL(io_pgtable_ops_unregister); >>> diff --git a/include/linux/io-pgtable.h b/include/linux/io-pgtable.h >>> index ea727eb..45b367ce 100644 >>> --- a/include/linux/io-pgtable.h >>> +++ b/include/linux/io-pgtable.h >>> @@ -163,6 +163,38 @@ struct io_pgtable_ops { >>> }; >>> /** >>> + * struct io_pgtable_init_fns - Alloc/free a set of page tables for a >>> + * particular format. >>> + * >>> + * @fmt: The page table format. >>> + * @alloc: Allocate a set of page tables described by cfg. >>> + * @free: Free the page tables associated with iop. >>> + */ >>> +struct io_pgtable_init_fns { >>> + enum io_pgtable_fmt fmt; >>> + struct io_pgtable *(*alloc)(struct io_pgtable_cfg *cfg, void >>> *cookie); >>> + void (*free)(struct io_pgtable *iop); >>> +}; >>> + >>> +/** >>> + * io_pgtable_ops_register() - Register the page table routines for >>> a page table >>> + * format. >>> + * >>> + * @init_fns: The functions for allocating and freeing the page >>> tables of >>> + * a particular format. >>> + */ >>> +int io_pgtable_ops_register(struct io_pgtable_init_fns *init_fns); >>> + >>> +/** >>> + * io_pgtable_ops_unregister() - Unregister the page table routines >>> for a page >>> + * table format. >>> + * >>> + * @init_fns: The functions for allocating and freeing the page >>> tables of >>> + * a particular format. >>> + */ >>> +void io_pgtable_ops_unregister(struct io_pgtable_init_fns *init_fns); >>> + >>> +/** >>> * alloc_io_pgtable_ops() - Allocate a page table allocator for use >>> by an IOMMU. >>> * >>> * @fmt: The page table format. >>> @@ -233,23 +265,4 @@ io_pgtable_tlb_add_page(struct io_pgtable *iop, >>> iop->cfg.tlb->tlb_add_page(gather, iova, granule, >>> iop->cookie); >>> } >>> -/** >>> - * struct io_pgtable_init_fns - Alloc/free a set of page tables for a >>> - * particular format. >>> - * >>> - * @alloc: Allocate a set of page tables described by cfg. >>> - * @free: Free the page tables associated with iop. >>> - */ >>> -struct io_pgtable_init_fns { >>> - struct io_pgtable *(*alloc)(struct io_pgtable_cfg *cfg, void >>> *cookie); >>> - void (*free)(struct io_pgtable *iop); >>> -}; >>> - >>> -extern struct io_pgtable_init_fns io_pgtable_arm_32_lpae_s1_init_fns; >>> -extern struct io_pgtable_init_fns io_pgtable_arm_32_lpae_s2_init_fns; >>> -extern struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s1_init_fns; >>> -extern struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s2_init_fns; >>> -extern struct io_pgtable_init_fns io_pgtable_arm_v7s_init_fns; >>> -extern struct io_pgtable_init_fns io_pgtable_arm_mali_lpae_init_fns; >>> - >>> #endif /* __IO_PGTABLE_H */ >>> >> >> _______________________________________________ >> linux-arm-kernel mailing list >> linux-arm-kernel@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > _______________________________________________ > iommu mailing list > iommu@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/iommu
On 2020-12-23 05:44, Robin Murphy wrote: > On 2020-12-22 19:54, isaacm@codeaurora.org wrote: >> On 2020-12-22 11:27, Robin Murphy wrote: >>> On 2020-12-22 00:44, Isaac J. Manjarres wrote: >>>> The io-pgtable code constructs an array of init functions for each >>>> page table format at compile time. This is not ideal, as this >>>> increases the footprint of the io-pgtable code, as well as prevents >>>> io-pgtable formats from being built as kernel modules. >>>> >>>> In preparation for modularizing the io-pgtable formats, switch to a >>>> dynamic registration scheme, where each io-pgtable format can >>>> register >>>> their init functions with the io-pgtable code at boot or module >>>> insertion time. >>>> >>>> Signed-off-by: Isaac J. Manjarres <isaacm@codeaurora.org> >>>> --- >>>> drivers/iommu/io-pgtable-arm-v7s.c | 34 +++++++++++++- >>>> drivers/iommu/io-pgtable-arm.c | 90 >>>> ++++++++++++++++++++++++++---------- >>>> drivers/iommu/io-pgtable.c | 94 >>>> ++++++++++++++++++++++++++++++++------ >>>> include/linux/io-pgtable.h | 51 +++++++++++++-------- >>>> 4 files changed, 209 insertions(+), 60 deletions(-) >>>> >>>> diff --git a/drivers/iommu/io-pgtable-arm-v7s.c >>>> b/drivers/iommu/io-pgtable-arm-v7s.c >>>> index 1d92ac9..89aad2f 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> >>>> @@ -835,7 +836,8 @@ static struct io_pgtable >>>> *arm_v7s_alloc_pgtable(struct io_pgtable_cfg *cfg, >>>> return NULL; >>>> } >>>> -struct io_pgtable_init_fns io_pgtable_arm_v7s_init_fns = { >>>> +static struct io_pgtable_init_fns io_pgtable_arm_v7s_init_fns = { >>>> + .fmt = ARM_V7S, >>>> .alloc = arm_v7s_alloc_pgtable, >>>> .free = arm_v7s_free_pgtable, >>>> }; >>>> @@ -982,5 +984,33 @@ static int __init arm_v7s_do_selftests(void) >>>> pr_info("self test ok\n"); >>>> return 0; >>>> } >>>> -subsys_initcall(arm_v7s_do_selftests); >>>> +#else >>>> +static int arm_v7s_do_selftests(void) >>>> +{ >>>> + return 0; >>>> +} >>>> #endif >>>> + >>>> +static int __init arm_v7s_init(void) >>>> +{ >>>> + int ret; >>>> + >>>> + ret = io_pgtable_ops_register(&io_pgtable_arm_v7s_init_fns); >>>> + if (ret < 0) { >>>> + pr_err("Failed to register ARM V7S format\n"); >>> >>> Super-nit: I think "v7s" should probably be lowercase there. Also >>> general consistency WRT to showing the error code and whether or not >>> to abbreviate "format" would be nice. >>> >> Ok, I can fix this accordingly. >> >>>> + return ret; >>>> + } >>>> + >>>> + ret = arm_v7s_do_selftests(); >>>> + if (ret < 0) >>>> + io_pgtable_ops_unregister(&io_pgtable_arm_v7s_init_fns); >>>> + >>>> + return ret; >>>> +} >>>> +core_initcall(arm_v7s_init); >>>> + >>>> +static void __exit arm_v7s_exit(void) >>>> +{ >>>> + io_pgtable_ops_unregister(&io_pgtable_arm_v7s_init_fns); >>>> +} >>>> +module_exit(arm_v7s_exit); >>>> diff --git a/drivers/iommu/io-pgtable-arm.c >>>> b/drivers/iommu/io-pgtable-arm.c >>>> index 87def58..ff0ea2f 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> >>>> @@ -1043,29 +1044,32 @@ arm_mali_lpae_alloc_pgtable(struct >>>> io_pgtable_cfg *cfg, void *cookie) >>>> return NULL; >>>> } >>>> -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, >>>> -}; >>>> - >>>> -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, >>>> -}; >>>> - >>>> -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, >>>> -}; >>>> - >>>> -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, >>>> -}; >>>> - >>>> -struct io_pgtable_init_fns io_pgtable_arm_mali_lpae_init_fns = { >>>> - .alloc = arm_mali_lpae_alloc_pgtable, >>>> - .free = arm_lpae_free_pgtable, >>>> +static struct io_pgtable_init_fns io_pgtable_arm_lpae_init_fns[] = >>>> { >>>> + { >>>> + .fmt = ARM_32_LPAE_S1, >>>> + .alloc = arm_32_lpae_alloc_pgtable_s1, >>>> + .free = arm_lpae_free_pgtable, >>>> + }, >>>> + { >>>> + .fmt = ARM_32_LPAE_S2, >>>> + .alloc = arm_32_lpae_alloc_pgtable_s2, >>>> + .free = arm_lpae_free_pgtable, >>>> + }, >>>> + { >>>> + .fmt = ARM_64_LPAE_S1, >>>> + .alloc = arm_64_lpae_alloc_pgtable_s1, >>>> + .free = arm_lpae_free_pgtable, >>>> + }, >>>> + { >>>> + .fmt = ARM_64_LPAE_S2, >>>> + .alloc = arm_64_lpae_alloc_pgtable_s2, >>>> + .free = arm_lpae_free_pgtable, >>>> + }, >>>> + { >>>> + .fmt = ARM_MALI_LPAE, >>>> + .alloc = arm_mali_lpae_alloc_pgtable, >>>> + .free = arm_lpae_free_pgtable, >>>> + }, >>>> }; >>>> #ifdef CONFIG_IOMMU_IO_PGTABLE_LPAE_SELFTEST >>>> @@ -1250,5 +1254,43 @@ static int __init arm_lpae_do_selftests(void) >>>> pr_info("selftest: completed with %d PASS %d FAIL\n", pass, >>>> fail); >>>> return fail ? -EFAULT : 0; >>>> } >>>> -subsys_initcall(arm_lpae_do_selftests); >>>> +#else >>>> +static int __init arm_lpae_do_selftests(void) >>>> +{ >>>> + return 0; >>>> +} >>>> #endif >>>> + >>>> +static int __init arm_lpae_init(void) >>>> +{ >>>> + int ret, i; >>>> + >>>> + for (i = 0; i < ARRAY_SIZE(io_pgtable_arm_lpae_init_fns); i++) >>>> { >>>> + ret = >>>> io_pgtable_ops_register(&io_pgtable_arm_lpae_init_fns[i]); >>>> + if (ret < 0) { >>>> + pr_err("Failed to register ARM LPAE fmt: %d\n"); >>>> + goto err_io_pgtable_register; >>>> + } >>>> + } >>>> + >>>> + ret = arm_lpae_do_selftests(); >>>> + if (ret < 0) >>>> + goto err_io_pgtable_register; >>>> + >>>> + return 0; >>>> + >>>> +err_io_pgtable_register: >>>> + for (i = i - 1; i >= 0; i--) >>> >>> Personally I find "while (i--)" a bit clearer for this kind of >>> unwinding, but maybe post-decrement isn't to everyone's taste. >>> >> I'm not particularly attached to the current approach, so a while loop >> is okay with me :). >> >>>> + >>>> io_pgtable_ops_unregister(&io_pgtable_arm_lpae_init_fns[i]); >>>> + return ret; >>>> +} >>>> +core_initcall(arm_lpae_init); >>>> + >>>> +static void __exit arm_lpae_exit(void) >>>> +{ >>>> + int i; >>>> + >>>> + for (i = 0; i < ARRAY_SIZE(io_pgtable_arm_lpae_init_fns); i++) >>>> + >>>> io_pgtable_ops_unregister(&io_pgtable_arm_lpae_init_fns[i]); >>>> +} >>>> +module_exit(arm_lpae_exit); >>>> diff --git a/drivers/iommu/io-pgtable.c b/drivers/iommu/io-pgtable.c >>>> index 94394c8..2c6eb2e 100644 >>>> --- a/drivers/iommu/io-pgtable.c >>>> +++ b/drivers/iommu/io-pgtable.c >>>> @@ -10,33 +10,45 @@ >>>> #include <linux/bug.h> >>>> #include <linux/io-pgtable.h> >>>> #include <linux/kernel.h> >>>> +#include <linux/rwlock.h> >>>> +#include <linux/slab.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 >>>> - [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 >>>> - [ARM_V7S] = &io_pgtable_arm_v7s_init_fns, >>>> -#endif >>>> +struct io_pgtable_init_fns_node { >>>> + struct io_pgtable_init_fns *fns; >>>> + struct list_head list; >>>> }; >>>> +static LIST_HEAD(io_pgtable_init_fns_list); >>>> +static DEFINE_RWLOCK(io_pgtable_init_fns_list_lock); >>>> + >>>> +static struct io_pgtable_init_fns *io_pgtable_get_init_fns(enum >>>> io_pgtable_fmt fmt) >>>> +{ >>>> + struct io_pgtable_init_fns_node *iter; >>>> + struct io_pgtable_init_fns *fns = NULL; >>>> + >>>> + read_lock(&io_pgtable_init_fns_list_lock); >>>> + list_for_each_entry(iter, &io_pgtable_init_fns_list, list) >>>> + if (iter->fns->fmt == fmt) { >>>> + fns = iter->fns; >>>> + break; >>>> + } >>>> + read_unlock(&io_pgtable_init_fns_list_lock); >>>> + >>>> + return fns; >>>> +} >>> >>> I think it would be a lot easier to stick with a simple array indexed >>> by enum - that way you can just set/clear/test entries without >>> needing >>> to worry about locking. Basically just remove the const and the >>> initialisers from the existing one ;) >>> >>> (and if you think you're concerned about memory, consider that just >>> the list head plus lock is already half the size of the table) >>> >>> Other than that, I think this all looks pretty promising - I'd >>> suggest >>> sending a non-RFC after rc1 so that it gets everyone's proper >>> attention. >>> >>> Thanks, >>> Robin. >>> >> >> Thanks for all of the feedback! With respect to the comment about >> keeping the current array, >> don't we need some sort of lock to protect access to the data >> structure (e.g. concurrent access >> if adding two ops for the same format)? Or do you mean that we should >> use atomic operations to >> handle this cleanly? > > I don't think any races are realistically possible - built-in > initcalls should run sequentially, while module init should be > serialised by the module loader lock, so registrations should never be > able to race against each other. And if there could be a race between > registering a format and somebody trying to use it, then something's > fundamentally wrong anyway - sure we could use READ_ONCE/WRITE_ONCE > for accessing the table, but that would be basically be admitting that > we've failed at the primary purpose of making sure formats are loaded > *before* the users that depend on them. > > With a directly-indexed table there's no contention between looking up > one format and registering another unrelated format, unlike with a > list. > > Robin. > Got it. I'll stick with the array as suggested. Thanks again for the feedback. I'll be sending the version with your suggestions when 5.11-rc1 comes out as a non-RFC. --Isaac >> >> Thanks, >> Isaac >>>> + >>>> struct io_pgtable_ops *alloc_io_pgtable_ops(enum io_pgtable_fmt >>>> fmt, >>>> struct io_pgtable_cfg *cfg, >>>> void *cookie) >>>> { >>>> struct io_pgtable *iop; >>>> - const struct io_pgtable_init_fns *fns; >>>> + struct io_pgtable_init_fns *fns; >>>> if (fmt >= IO_PGTABLE_NUM_FMTS) >>>> return NULL; >>>> - fns = io_pgtable_init_table[fmt]; >>>> + fns = io_pgtable_get_init_fns(fmt); >>>> if (!fns) >>>> return NULL; >>>> @@ -59,12 +71,64 @@ EXPORT_SYMBOL_GPL(alloc_io_pgtable_ops); >>>> void free_io_pgtable_ops(struct io_pgtable_ops *ops) >>>> { >>>> struct io_pgtable *iop; >>>> + struct io_pgtable_init_fns *fns; >>>> if (!ops) >>>> return; >>>> iop = io_pgtable_ops_to_pgtable(ops); >>>> io_pgtable_tlb_flush_all(iop); >>>> - io_pgtable_init_table[iop->fmt]->free(iop); >>>> + fns = io_pgtable_get_init_fns(iop->fmt); >>>> + if (fns) >>>> + fns->free(iop); >>>> } >>>> EXPORT_SYMBOL_GPL(free_io_pgtable_ops); >>>> + >>>> +int io_pgtable_ops_register(struct io_pgtable_init_fns *init_fns) >>>> +{ >>>> + struct io_pgtable_init_fns_node *iter, *fns_node; >>>> + int ret = 0; >>>> + >>>> + if (!init_fns || init_fns->fmt >= IO_PGTABLE_NUM_FMTS || >>>> + !init_fns->alloc || !init_fns->free) >>>> + return -EINVAL; >>>> + >>>> + fns_node = kzalloc(sizeof(*fns_node), GFP_KERNEL); >>>> + if (!fns_node) >>>> + return -ENOMEM; >>>> + >>>> + write_lock(&io_pgtable_init_fns_list_lock); >>>> + list_for_each_entry(iter, &io_pgtable_init_fns_list, list) >>>> + if (iter->fns->fmt == init_fns->fmt) { >>>> + ret = -EEXIST; >>>> + kfree(fns_node); >>>> + break; >>>> + } >>>> + >>>> + if (!ret) { >>>> + fns_node->fns = init_fns; >>>> + INIT_LIST_HEAD(&fns_node->list); >>>> + list_add_tail(&fns_node->list, &io_pgtable_init_fns_list); >>>> + } >>>> + write_unlock(&io_pgtable_init_fns_list_lock); >>>> + return ret; >>>> +} >>>> +EXPORT_SYMBOL_GPL(io_pgtable_ops_register); >>>> + >>>> +void io_pgtable_ops_unregister(struct io_pgtable_init_fns >>>> *init_fns) >>>> +{ >>>> + struct io_pgtable_init_fns_node *iter, *tmp; >>>> + >>>> + if (!init_fns) >>>> + return; >>>> + >>>> + write_lock(&io_pgtable_init_fns_list_lock); >>>> + list_for_each_entry_safe(iter, tmp, &io_pgtable_init_fns_list, >>>> list) >>>> + if (iter->fns == init_fns) { >>>> + list_del(&iter->list); >>>> + kfree(iter); >>>> + break; >>>> + } >>>> + write_unlock(&io_pgtable_init_fns_list_lock); >>>> +} >>>> +EXPORT_SYMBOL_GPL(io_pgtable_ops_unregister); >>>> diff --git a/include/linux/io-pgtable.h b/include/linux/io-pgtable.h >>>> index ea727eb..45b367ce 100644 >>>> --- a/include/linux/io-pgtable.h >>>> +++ b/include/linux/io-pgtable.h >>>> @@ -163,6 +163,38 @@ struct io_pgtable_ops { >>>> }; >>>> /** >>>> + * struct io_pgtable_init_fns - Alloc/free a set of page tables for >>>> a >>>> + * particular format. >>>> + * >>>> + * @fmt: The page table format. >>>> + * @alloc: Allocate a set of page tables described by cfg. >>>> + * @free: Free the page tables associated with iop. >>>> + */ >>>> +struct io_pgtable_init_fns { >>>> + enum io_pgtable_fmt fmt; >>>> + struct io_pgtable *(*alloc)(struct io_pgtable_cfg *cfg, void >>>> *cookie); >>>> + void (*free)(struct io_pgtable *iop); >>>> +}; >>>> + >>>> +/** >>>> + * io_pgtable_ops_register() - Register the page table routines for >>>> a page table >>>> + * format. >>>> + * >>>> + * @init_fns: The functions for allocating and freeing the page >>>> tables of >>>> + * a particular format. >>>> + */ >>>> +int io_pgtable_ops_register(struct io_pgtable_init_fns *init_fns); >>>> + >>>> +/** >>>> + * io_pgtable_ops_unregister() - Unregister the page table routines >>>> for a page >>>> + * table format. >>>> + * >>>> + * @init_fns: The functions for allocating and freeing the page >>>> tables of >>>> + * a particular format. >>>> + */ >>>> +void io_pgtable_ops_unregister(struct io_pgtable_init_fns >>>> *init_fns); >>>> + >>>> +/** >>>> * alloc_io_pgtable_ops() - Allocate a page table allocator for >>>> use by an IOMMU. >>>> * >>>> * @fmt: The page table format. >>>> @@ -233,23 +265,4 @@ io_pgtable_tlb_add_page(struct io_pgtable *iop, >>>> iop->cfg.tlb->tlb_add_page(gather, iova, granule, >>>> iop->cookie); >>>> } >>>> -/** >>>> - * struct io_pgtable_init_fns - Alloc/free a set of page tables for >>>> a >>>> - * particular format. >>>> - * >>>> - * @alloc: Allocate a set of page tables described by cfg. >>>> - * @free: Free the page tables associated with iop. >>>> - */ >>>> -struct io_pgtable_init_fns { >>>> - struct io_pgtable *(*alloc)(struct io_pgtable_cfg *cfg, void >>>> *cookie); >>>> - void (*free)(struct io_pgtable *iop); >>>> -}; >>>> - >>>> -extern struct io_pgtable_init_fns >>>> io_pgtable_arm_32_lpae_s1_init_fns; >>>> -extern struct io_pgtable_init_fns >>>> io_pgtable_arm_32_lpae_s2_init_fns; >>>> -extern struct io_pgtable_init_fns >>>> io_pgtable_arm_64_lpae_s1_init_fns; >>>> -extern struct io_pgtable_init_fns >>>> io_pgtable_arm_64_lpae_s2_init_fns; >>>> -extern struct io_pgtable_init_fns io_pgtable_arm_v7s_init_fns; >>>> -extern struct io_pgtable_init_fns >>>> io_pgtable_arm_mali_lpae_init_fns; >>>> - >>>> #endif /* __IO_PGTABLE_H */ >>>> >>> >>> _______________________________________________ >>> linux-arm-kernel mailing list >>> linux-arm-kernel@lists.infradead.org >>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >> _______________________________________________ >> iommu mailing list >> iommu@lists.linux-foundation.org >> https://lists.linuxfoundation.org/mailman/listinfo/iommu
Hi Isaac, On 22.12.20 2:44, Isaac J. Manjarres wrote: > The io-pgtable code constructs an array of init functions for each > page table format at compile time. This is not ideal, as this > increases the footprint of the io-pgtable code, as well as prevents > io-pgtable formats from being built as kernel modules. > > In preparation for modularizing the io-pgtable formats, switch to a > dynamic registration scheme, where each io-pgtable format can register > their init functions with the io-pgtable code at boot or module > insertion time. > > Signed-off-by: Isaac J. Manjarres <isaacm@codeaurora.org> > --- > drivers/iommu/io-pgtable-arm-v7s.c | 34 +++++++++++++- > drivers/iommu/io-pgtable-arm.c | 90 ++++++++++++++++++++++++++---------- > drivers/iommu/io-pgtable.c | 94 ++++++++++++++++++++++++++++++++------ > include/linux/io-pgtable.h | 51 +++++++++++++-------- > 4 files changed, 209 insertions(+), 60 deletions(-) > > diff --git a/drivers/iommu/io-pgtable-arm-v7s.c b/drivers/iommu/io-pgtable-arm-v7s.c > index 1d92ac9..89aad2f 100644 > --- a/drivers/iommu/io-pgtable-arm-v7s.c > +++ b/drivers/iommu/io-pgtable-arm-v7s.c > @@ -28,6 +28,7 @@ [..] > +static int __init arm_lpae_init(void) > +{ > + int ret, i; > + > + for (i = 0; i < ARRAY_SIZE(io_pgtable_arm_lpae_init_fns); i++) { > + ret = io_pgtable_ops_register(&io_pgtable_arm_lpae_init_fns[i]); > + if (ret < 0) { > + pr_err("Failed to register ARM LPAE fmt: %d\n"); I guess we want to print the format here? Thanks, Georgi
Hi "Isaac,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on iommu/next]
[also build test WARNING on soc/for-next xlnx/master linus/master v5.10 next-20201223]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Isaac-J-Manjarres/iommu-Permit-modular-builds-of-io-pgtable-drivers/20201222-085121
base: https://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git next
config: alpha-randconfig-r023-20201221 (attached as .config)
compiler: alpha-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/9cc3cfc5f79e5c9072aea2218dd9080227933caa
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Isaac-J-Manjarres/iommu-Permit-modular-builds-of-io-pgtable-drivers/20201222-085121
git checkout 9cc3cfc5f79e5c9072aea2218dd9080227933caa
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=alpha
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from include/linux/printk.h:7,
from include/linux/kernel.h:16,
from include/asm-generic/bug.h:20,
from arch/alpha/include/asm/bug.h:23,
from include/linux/bug.h:5,
from include/linux/scatterlist.h:7,
from include/linux/iommu.h:10,
from include/linux/io-pgtable.h:6,
from drivers/iommu/io-pgtable-arm.c:14:
drivers/iommu/io-pgtable-arm.c: In function 'arm_lpae_init':
>> include/linux/kern_levels.h:5:18: warning: format '%d' expects a matching 'int' argument [-Wformat=]
5 | #define KERN_SOH "\001" /* ASCII Start Of Header */
| ^~~~~~
include/linux/kern_levels.h:11:18: note: in expansion of macro 'KERN_SOH'
11 | #define KERN_ERR KERN_SOH "3" /* error conditions */
| ^~~~~~~~
include/linux/printk.h:343:9: note: in expansion of macro 'KERN_ERR'
343 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~~
drivers/iommu/io-pgtable-arm.c:1257:4: note: in expansion of macro 'pr_err'
1257 | pr_err("Failed to register ARM LPAE fmt: %d\n");
| ^~~~~~
drivers/iommu/io-pgtable-arm.c:1257:46: note: format string is defined here
1257 | pr_err("Failed to register ARM LPAE fmt: %d\n");
| ~^
| |
| int
vim +5 include/linux/kern_levels.h
314ba3520e513a7 Joe Perches 2012-07-30 4
04d2c8c83d0e3ac Joe Perches 2012-07-30 @5 #define KERN_SOH "\001" /* ASCII Start Of Header */
04d2c8c83d0e3ac Joe Perches 2012-07-30 6 #define KERN_SOH_ASCII '\001'
04d2c8c83d0e3ac Joe Perches 2012-07-30 7
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff --git a/drivers/iommu/io-pgtable-arm-v7s.c b/drivers/iommu/io-pgtable-arm-v7s.c index 1d92ac9..89aad2f 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> @@ -835,7 +836,8 @@ static struct io_pgtable *arm_v7s_alloc_pgtable(struct io_pgtable_cfg *cfg, return NULL; } -struct io_pgtable_init_fns io_pgtable_arm_v7s_init_fns = { +static struct io_pgtable_init_fns io_pgtable_arm_v7s_init_fns = { + .fmt = ARM_V7S, .alloc = arm_v7s_alloc_pgtable, .free = arm_v7s_free_pgtable, }; @@ -982,5 +984,33 @@ static int __init arm_v7s_do_selftests(void) pr_info("self test ok\n"); return 0; } -subsys_initcall(arm_v7s_do_selftests); +#else +static int arm_v7s_do_selftests(void) +{ + return 0; +} #endif + +static int __init arm_v7s_init(void) +{ + int ret; + + ret = io_pgtable_ops_register(&io_pgtable_arm_v7s_init_fns); + if (ret < 0) { + pr_err("Failed to register ARM V7S format\n"); + return ret; + } + + ret = arm_v7s_do_selftests(); + if (ret < 0) + io_pgtable_ops_unregister(&io_pgtable_arm_v7s_init_fns); + + return ret; +} +core_initcall(arm_v7s_init); + +static void __exit arm_v7s_exit(void) +{ + io_pgtable_ops_unregister(&io_pgtable_arm_v7s_init_fns); +} +module_exit(arm_v7s_exit); diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c index 87def58..ff0ea2f 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> @@ -1043,29 +1044,32 @@ arm_mali_lpae_alloc_pgtable(struct io_pgtable_cfg *cfg, void *cookie) return NULL; } -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, -}; - -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, -}; - -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, -}; - -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, -}; - -struct io_pgtable_init_fns io_pgtable_arm_mali_lpae_init_fns = { - .alloc = arm_mali_lpae_alloc_pgtable, - .free = arm_lpae_free_pgtable, +static struct io_pgtable_init_fns io_pgtable_arm_lpae_init_fns[] = { + { + .fmt = ARM_32_LPAE_S1, + .alloc = arm_32_lpae_alloc_pgtable_s1, + .free = arm_lpae_free_pgtable, + }, + { + .fmt = ARM_32_LPAE_S2, + .alloc = arm_32_lpae_alloc_pgtable_s2, + .free = arm_lpae_free_pgtable, + }, + { + .fmt = ARM_64_LPAE_S1, + .alloc = arm_64_lpae_alloc_pgtable_s1, + .free = arm_lpae_free_pgtable, + }, + { + .fmt = ARM_64_LPAE_S2, + .alloc = arm_64_lpae_alloc_pgtable_s2, + .free = arm_lpae_free_pgtable, + }, + { + .fmt = ARM_MALI_LPAE, + .alloc = arm_mali_lpae_alloc_pgtable, + .free = arm_lpae_free_pgtable, + }, }; #ifdef CONFIG_IOMMU_IO_PGTABLE_LPAE_SELFTEST @@ -1250,5 +1254,43 @@ static int __init arm_lpae_do_selftests(void) pr_info("selftest: completed with %d PASS %d FAIL\n", pass, fail); return fail ? -EFAULT : 0; } -subsys_initcall(arm_lpae_do_selftests); +#else +static int __init arm_lpae_do_selftests(void) +{ + return 0; +} #endif + +static int __init arm_lpae_init(void) +{ + int ret, i; + + for (i = 0; i < ARRAY_SIZE(io_pgtable_arm_lpae_init_fns); i++) { + ret = io_pgtable_ops_register(&io_pgtable_arm_lpae_init_fns[i]); + if (ret < 0) { + pr_err("Failed to register ARM LPAE fmt: %d\n"); + goto err_io_pgtable_register; + } + } + + ret = arm_lpae_do_selftests(); + if (ret < 0) + goto err_io_pgtable_register; + + return 0; + +err_io_pgtable_register: + for (i = i - 1; i >= 0; i--) + io_pgtable_ops_unregister(&io_pgtable_arm_lpae_init_fns[i]); + return ret; +} +core_initcall(arm_lpae_init); + +static void __exit arm_lpae_exit(void) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(io_pgtable_arm_lpae_init_fns); i++) + io_pgtable_ops_unregister(&io_pgtable_arm_lpae_init_fns[i]); +} +module_exit(arm_lpae_exit); diff --git a/drivers/iommu/io-pgtable.c b/drivers/iommu/io-pgtable.c index 94394c8..2c6eb2e 100644 --- a/drivers/iommu/io-pgtable.c +++ b/drivers/iommu/io-pgtable.c @@ -10,33 +10,45 @@ #include <linux/bug.h> #include <linux/io-pgtable.h> #include <linux/kernel.h> +#include <linux/rwlock.h> +#include <linux/slab.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 - [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 - [ARM_V7S] = &io_pgtable_arm_v7s_init_fns, -#endif +struct io_pgtable_init_fns_node { + struct io_pgtable_init_fns *fns; + struct list_head list; }; +static LIST_HEAD(io_pgtable_init_fns_list); +static DEFINE_RWLOCK(io_pgtable_init_fns_list_lock); + +static struct io_pgtable_init_fns *io_pgtable_get_init_fns(enum io_pgtable_fmt fmt) +{ + struct io_pgtable_init_fns_node *iter; + struct io_pgtable_init_fns *fns = NULL; + + read_lock(&io_pgtable_init_fns_list_lock); + list_for_each_entry(iter, &io_pgtable_init_fns_list, list) + if (iter->fns->fmt == fmt) { + fns = iter->fns; + break; + } + read_unlock(&io_pgtable_init_fns_list_lock); + + return fns; +} + struct io_pgtable_ops *alloc_io_pgtable_ops(enum io_pgtable_fmt fmt, struct io_pgtable_cfg *cfg, void *cookie) { struct io_pgtable *iop; - const struct io_pgtable_init_fns *fns; + struct io_pgtable_init_fns *fns; if (fmt >= IO_PGTABLE_NUM_FMTS) return NULL; - fns = io_pgtable_init_table[fmt]; + fns = io_pgtable_get_init_fns(fmt); if (!fns) return NULL; @@ -59,12 +71,64 @@ EXPORT_SYMBOL_GPL(alloc_io_pgtable_ops); void free_io_pgtable_ops(struct io_pgtable_ops *ops) { struct io_pgtable *iop; + struct io_pgtable_init_fns *fns; if (!ops) return; iop = io_pgtable_ops_to_pgtable(ops); io_pgtable_tlb_flush_all(iop); - io_pgtable_init_table[iop->fmt]->free(iop); + fns = io_pgtable_get_init_fns(iop->fmt); + if (fns) + fns->free(iop); } EXPORT_SYMBOL_GPL(free_io_pgtable_ops); + +int io_pgtable_ops_register(struct io_pgtable_init_fns *init_fns) +{ + struct io_pgtable_init_fns_node *iter, *fns_node; + int ret = 0; + + if (!init_fns || init_fns->fmt >= IO_PGTABLE_NUM_FMTS || + !init_fns->alloc || !init_fns->free) + return -EINVAL; + + fns_node = kzalloc(sizeof(*fns_node), GFP_KERNEL); + if (!fns_node) + return -ENOMEM; + + write_lock(&io_pgtable_init_fns_list_lock); + list_for_each_entry(iter, &io_pgtable_init_fns_list, list) + if (iter->fns->fmt == init_fns->fmt) { + ret = -EEXIST; + kfree(fns_node); + break; + } + + if (!ret) { + fns_node->fns = init_fns; + INIT_LIST_HEAD(&fns_node->list); + list_add_tail(&fns_node->list, &io_pgtable_init_fns_list); + } + write_unlock(&io_pgtable_init_fns_list_lock); + return ret; +} +EXPORT_SYMBOL_GPL(io_pgtable_ops_register); + +void io_pgtable_ops_unregister(struct io_pgtable_init_fns *init_fns) +{ + struct io_pgtable_init_fns_node *iter, *tmp; + + if (!init_fns) + return; + + write_lock(&io_pgtable_init_fns_list_lock); + list_for_each_entry_safe(iter, tmp, &io_pgtable_init_fns_list, list) + if (iter->fns == init_fns) { + list_del(&iter->list); + kfree(iter); + break; + } + write_unlock(&io_pgtable_init_fns_list_lock); +} +EXPORT_SYMBOL_GPL(io_pgtable_ops_unregister); diff --git a/include/linux/io-pgtable.h b/include/linux/io-pgtable.h index ea727eb..45b367ce 100644 --- a/include/linux/io-pgtable.h +++ b/include/linux/io-pgtable.h @@ -163,6 +163,38 @@ struct io_pgtable_ops { }; /** + * struct io_pgtable_init_fns - Alloc/free a set of page tables for a + * particular format. + * + * @fmt: The page table format. + * @alloc: Allocate a set of page tables described by cfg. + * @free: Free the page tables associated with iop. + */ +struct io_pgtable_init_fns { + enum io_pgtable_fmt fmt; + struct io_pgtable *(*alloc)(struct io_pgtable_cfg *cfg, void *cookie); + void (*free)(struct io_pgtable *iop); +}; + +/** + * io_pgtable_ops_register() - Register the page table routines for a page table + * format. + * + * @init_fns: The functions for allocating and freeing the page tables of + * a particular format. + */ +int io_pgtable_ops_register(struct io_pgtable_init_fns *init_fns); + +/** + * io_pgtable_ops_unregister() - Unregister the page table routines for a page + * table format. + * + * @init_fns: The functions for allocating and freeing the page tables of + * a particular format. + */ +void io_pgtable_ops_unregister(struct io_pgtable_init_fns *init_fns); + +/** * alloc_io_pgtable_ops() - Allocate a page table allocator for use by an IOMMU. * * @fmt: The page table format. @@ -233,23 +265,4 @@ io_pgtable_tlb_add_page(struct io_pgtable *iop, iop->cfg.tlb->tlb_add_page(gather, iova, granule, iop->cookie); } -/** - * struct io_pgtable_init_fns - Alloc/free a set of page tables for a - * particular format. - * - * @alloc: Allocate a set of page tables described by cfg. - * @free: Free the page tables associated with iop. - */ -struct io_pgtable_init_fns { - struct io_pgtable *(*alloc)(struct io_pgtable_cfg *cfg, void *cookie); - void (*free)(struct io_pgtable *iop); -}; - -extern struct io_pgtable_init_fns io_pgtable_arm_32_lpae_s1_init_fns; -extern struct io_pgtable_init_fns io_pgtable_arm_32_lpae_s2_init_fns; -extern struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s1_init_fns; -extern struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s2_init_fns; -extern struct io_pgtable_init_fns io_pgtable_arm_v7s_init_fns; -extern struct io_pgtable_init_fns io_pgtable_arm_mali_lpae_init_fns; - #endif /* __IO_PGTABLE_H */
The io-pgtable code constructs an array of init functions for each page table format at compile time. This is not ideal, as this increases the footprint of the io-pgtable code, as well as prevents io-pgtable formats from being built as kernel modules. In preparation for modularizing the io-pgtable formats, switch to a dynamic registration scheme, where each io-pgtable format can register their init functions with the io-pgtable code at boot or module insertion time. Signed-off-by: Isaac J. Manjarres <isaacm@codeaurora.org> --- drivers/iommu/io-pgtable-arm-v7s.c | 34 +++++++++++++- drivers/iommu/io-pgtable-arm.c | 90 ++++++++++++++++++++++++++---------- drivers/iommu/io-pgtable.c | 94 ++++++++++++++++++++++++++++++++------ include/linux/io-pgtable.h | 51 +++++++++++++-------- 4 files changed, 209 insertions(+), 60 deletions(-)