Messages in this thread | | | Date | Wed, 23 Dec 2020 07:14:36 -0800 | From | isaacm@codeauro ... | Subject | Re: [PATCH v2 1/7] iommu/io-pgtable: Introduce dynamic io-pgtable fmt registration |
| |
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
| |