From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-17.8 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id BEFB3C433DB for ; Tue, 22 Dec 2020 19:29:07 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 539CF23130 for ; Tue, 22 Dec 2020 19:29:07 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 539CF23130 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Type: Content-Transfer-Encoding:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Date:Message-ID:From: References:To:Subject:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=kmVW3PQBY3uME0I4BWFEFtAFgNft3t+s7Y+U5lgR2zM=; b=xdZi4yncWdARnpJcFc6xOqLIP mzI5TJYc7pBhschBMzd2Fygssu8GTBX09Wxa8Q/y+JoYfU999y9cHSc1wDQbLZ6ym1qy7Abk2lIbF qQb0IobsivZEYFQBuCNMI2/0ymIiN0Lld19wU84Jhd2oI/8OqWlZ9OBPzwaHAR7N5VB0O3x6WWem0 BHPrEz0xf01o+fXuC6XhbJtFhBMolHoizhOfk9w5WxhYdScKjqcqDmVHHfg5q2+MATV2eGWUyT9sG 59JuvzARtlYEN3z8r0t4Rp2WAMm07ihnAq8ZKTiL003Sy/kANe29VevQvRkh5LknsdKrJmgolpCN/ ganaq7b7w==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1krnK0-0000D7-9r; Tue, 22 Dec 2020 19:27:48 +0000 Received: from foss.arm.com ([217.140.110.172]) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1krnJx-0000Cg-9N for linux-arm-kernel@lists.infradead.org; Tue, 22 Dec 2020 19:27:46 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 47A3F1042; Tue, 22 Dec 2020 11:27:44 -0800 (PST) Received: from [10.57.34.90] (unknown [10.57.34.90]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id DBD083F6CF; Tue, 22 Dec 2020 11:27:41 -0800 (PST) Subject: Re: [PATCH v2 1/7] iommu/io-pgtable: Introduce dynamic io-pgtable fmt registration To: "Isaac J. Manjarres" , iommu@lists.linux-foundation.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, freedreno@lists.freedesktop.org, dri-devel@lists.freedesktop.org, linux-arm-msm@vger.kernel.org References: <1608597876-32367-1-git-send-email-isaacm@codeaurora.org> <1608597876-32367-2-git-send-email-isaacm@codeaurora.org> From: Robin Murphy Message-ID: <4f388512-c3df-e9fc-ebd4-624ab36a6858@arm.com> Date: Tue, 22 Dec 2020 19:27:41 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 MIME-Version: 1.0 In-Reply-To: <1608597876-32367-2-git-send-email-isaacm@codeaurora.org> Content-Language: en-GB X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201222_142745_452065_FC2BED86 X-CRM114-Status: GOOD ( 37.65 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: pratikp@codeaurora.org, joro@8bytes.org, kernel-team@android.com, will@kernel.org, pdaly@codeaurora.org Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org 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 > --- > 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 > #include > #include > +#include > #include > #include > #include > @@ -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 > #include > #include > +#include > #include > #include > #include > @@ -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 > #include > #include > +#include > +#include > #include > > -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 */ > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel