Re: [PATCH v2 1/7] iommu/io-pgtable: Introduce dynamic io-pgtable fmt registration

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 */