From: isaacm@codeaurora.org To: Robin Murphy <robin.murphy@arm.com> Cc: iommu@lists.linux-foundation.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, will@kernel.org, joro@8bytes.org, pdaly@codeaurora.org, pratikp@codeaurora.org, kernel-team@android.com Subject: Re: [PATCH 1/3] iommu/io-pgtable-arm: Prepare for modularization Date: Fri, 18 Dec 2020 10:59:42 -0800 [thread overview] Message-ID: <34ea1af8569e4115e2dd1de61ae95bb6@codeaurora.org> (raw) In-Reply-To: <309ff39d-5fc5-83c6-d423-2d66f503c60c@arm.com> On 2020-12-18 04:38, Robin Murphy wrote: > On 2020-12-18 08:38, Isaac J. Manjarres wrote: >> The io-pgtable-arm and io-pgtable-arm-v7s source files will >> be compiled as separate modules, along with the io-pgtable >> source. Export the symbols for the io-pgtable init function >> structures for the io-pgtable module to use. > > In my current build tree, the io-pgtable glue itself is a whopping 379 > bytes of code and data - is there really any benefit to all the > additional overhead of making that modular? Given the number of > different users (including AMD now), I think at this point we should > start considering this as part of the IOMMU core, and just tweak the > interface such that formats can register their own init_fns > dynamically instead of the static array that's always horrible. > > Robin. > Thanks for the feedback, Robin. This is an avenue I had explored a bit when modularizing the code. However, I came up with a few problems that I couldn't get around. 1) If we leave the io-pgtable glue as part of the core kernel, we need to ensure that the io-pgtable format modules get loaded prior to any driver that might use them (e.g. IOMMU drivers/other callers of alloc_io_pgtable_ops). a) This can get a bit messy, as there's no symbol dependencies between the callers of the io-pgtable code, and the page table format modules, since everything is through function pointers. This is handled for the IOMMU drivers through the devlink feature, but I don't see how we can leverage something like that here. I guess this isn't too much of a problem when everything is built-in, as the registration can happen in one of the earlier initcall levels. b) If we do run into a scenario where a client of io-pgtable tries to allocate a page table instance prior to the io-pgtable format module being loaded, I couldn't come up with a way of distinguishing between format module is not available at the moment vs format module will never be available. I don't think returning EPROBE_DEFER would be something nice to do in that case. 2) We would have to ensure that the format module cannot be unloaded while other clients are using it. I suppose this isn't as big as point #1 though, since it's something that can probably be handled through a similar ref count mechanism that we're using for modular IOMMU drivers. Given the two reasons above, I went with the current approach, since it avoids both issues by creating symbol dependencies between client drivers, the io-pgtable drivers, and the io-pgtable format drivers, so that ensures that they are loaded in the correct order, and also prevents them from being removed, unless there aren't any users present. Thanks, Isaac >> Signed-off-by: Isaac J. Manjarres <isaacm@codeaurora.org> >> --- >> drivers/iommu/io-pgtable-arm-v7s.c | 4 ++++ >> drivers/iommu/io-pgtable-arm.c | 8 ++++++++ >> 2 files changed, 12 insertions(+) >> >> diff --git a/drivers/iommu/io-pgtable-arm-v7s.c >> b/drivers/iommu/io-pgtable-arm-v7s.c >> index 1d92ac9..f062c1c 100644 >> --- a/drivers/iommu/io-pgtable-arm-v7s.c >> +++ b/drivers/iommu/io-pgtable-arm-v7s.c >> @@ -28,6 +28,7 @@ >> #include <linux/iommu.h> >> #include <linux/kernel.h> >> #include <linux/kmemleak.h> >> +#include <linux/module.h> >> #include <linux/sizes.h> >> #include <linux/slab.h> >> #include <linux/spinlock.h> >> @@ -839,6 +840,7 @@ struct io_pgtable_init_fns >> io_pgtable_arm_v7s_init_fns = { >> .alloc = arm_v7s_alloc_pgtable, >> .free = arm_v7s_free_pgtable, >> }; >> +EXPORT_SYMBOL_GPL(io_pgtable_arm_v7s_init_fns); >> #ifdef CONFIG_IOMMU_IO_PGTABLE_ARMV7S_SELFTEST >> @@ -984,3 +986,5 @@ static int __init arm_v7s_do_selftests(void) >> } >> subsys_initcall(arm_v7s_do_selftests); >> #endif >> + >> +MODULE_LICENSE("GPL v2"); >> diff --git a/drivers/iommu/io-pgtable-arm.c >> b/drivers/iommu/io-pgtable-arm.c >> index 87def58..2623d57 100644 >> --- a/drivers/iommu/io-pgtable-arm.c >> +++ b/drivers/iommu/io-pgtable-arm.c >> @@ -13,6 +13,7 @@ >> #include <linux/bitops.h> >> #include <linux/io-pgtable.h> >> #include <linux/kernel.h> >> +#include <linux/module.h> >> #include <linux/sizes.h> >> #include <linux/slab.h> >> #include <linux/types.h> >> @@ -1047,26 +1048,31 @@ struct io_pgtable_init_fns >> io_pgtable_arm_64_lpae_s1_init_fns = { >> .alloc = arm_64_lpae_alloc_pgtable_s1, >> .free = arm_lpae_free_pgtable, >> }; >> +EXPORT_SYMBOL_GPL(io_pgtable_arm_64_lpae_s1_init_fns); >> struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s2_init_fns = { >> .alloc = arm_64_lpae_alloc_pgtable_s2, >> .free = arm_lpae_free_pgtable, >> }; >> +EXPORT_SYMBOL_GPL(io_pgtable_arm_64_lpae_s2_init_fns); >> struct io_pgtable_init_fns io_pgtable_arm_32_lpae_s1_init_fns = { >> .alloc = arm_32_lpae_alloc_pgtable_s1, >> .free = arm_lpae_free_pgtable, >> }; >> +EXPORT_SYMBOL_GPL(io_pgtable_arm_32_lpae_s1_init_fns); >> struct io_pgtable_init_fns io_pgtable_arm_32_lpae_s2_init_fns = { >> .alloc = arm_32_lpae_alloc_pgtable_s2, >> .free = arm_lpae_free_pgtable, >> }; >> +EXPORT_SYMBOL_GPL(io_pgtable_arm_32_lpae_s2_init_fns); >> struct io_pgtable_init_fns io_pgtable_arm_mali_lpae_init_fns = { >> .alloc = arm_mali_lpae_alloc_pgtable, >> .free = arm_lpae_free_pgtable, >> }; >> +EXPORT_SYMBOL_GPL(io_pgtable_arm_mali_lpae_init_fns); >> #ifdef CONFIG_IOMMU_IO_PGTABLE_LPAE_SELFTEST >> @@ -1252,3 +1258,5 @@ static int __init arm_lpae_do_selftests(void) >> } >> subsys_initcall(arm_lpae_do_selftests); >> #endif >> + >> +MODULE_LICENSE("GPL v2"); >>
WARNING: multiple messages have this Message-ID (diff)
From: isaacm@codeaurora.org To: Robin Murphy <robin.murphy@arm.com> Cc: kernel-team@android.com, pdaly@codeaurora.org, pratikp@codeaurora.org, linux-kernel@vger.kernel.org, iommu@lists.linux-foundation.org, will@kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH 1/3] iommu/io-pgtable-arm: Prepare for modularization Date: Fri, 18 Dec 2020 10:59:42 -0800 [thread overview] Message-ID: <34ea1af8569e4115e2dd1de61ae95bb6@codeaurora.org> (raw) In-Reply-To: <309ff39d-5fc5-83c6-d423-2d66f503c60c@arm.com> On 2020-12-18 04:38, Robin Murphy wrote: > On 2020-12-18 08:38, Isaac J. Manjarres wrote: >> The io-pgtable-arm and io-pgtable-arm-v7s source files will >> be compiled as separate modules, along with the io-pgtable >> source. Export the symbols for the io-pgtable init function >> structures for the io-pgtable module to use. > > In my current build tree, the io-pgtable glue itself is a whopping 379 > bytes of code and data - is there really any benefit to all the > additional overhead of making that modular? Given the number of > different users (including AMD now), I think at this point we should > start considering this as part of the IOMMU core, and just tweak the > interface such that formats can register their own init_fns > dynamically instead of the static array that's always horrible. > > Robin. > Thanks for the feedback, Robin. This is an avenue I had explored a bit when modularizing the code. However, I came up with a few problems that I couldn't get around. 1) If we leave the io-pgtable glue as part of the core kernel, we need to ensure that the io-pgtable format modules get loaded prior to any driver that might use them (e.g. IOMMU drivers/other callers of alloc_io_pgtable_ops). a) This can get a bit messy, as there's no symbol dependencies between the callers of the io-pgtable code, and the page table format modules, since everything is through function pointers. This is handled for the IOMMU drivers through the devlink feature, but I don't see how we can leverage something like that here. I guess this isn't too much of a problem when everything is built-in, as the registration can happen in one of the earlier initcall levels. b) If we do run into a scenario where a client of io-pgtable tries to allocate a page table instance prior to the io-pgtable format module being loaded, I couldn't come up with a way of distinguishing between format module is not available at the moment vs format module will never be available. I don't think returning EPROBE_DEFER would be something nice to do in that case. 2) We would have to ensure that the format module cannot be unloaded while other clients are using it. I suppose this isn't as big as point #1 though, since it's something that can probably be handled through a similar ref count mechanism that we're using for modular IOMMU drivers. Given the two reasons above, I went with the current approach, since it avoids both issues by creating symbol dependencies between client drivers, the io-pgtable drivers, and the io-pgtable format drivers, so that ensures that they are loaded in the correct order, and also prevents them from being removed, unless there aren't any users present. Thanks, Isaac >> Signed-off-by: Isaac J. Manjarres <isaacm@codeaurora.org> >> --- >> drivers/iommu/io-pgtable-arm-v7s.c | 4 ++++ >> drivers/iommu/io-pgtable-arm.c | 8 ++++++++ >> 2 files changed, 12 insertions(+) >> >> diff --git a/drivers/iommu/io-pgtable-arm-v7s.c >> b/drivers/iommu/io-pgtable-arm-v7s.c >> index 1d92ac9..f062c1c 100644 >> --- a/drivers/iommu/io-pgtable-arm-v7s.c >> +++ b/drivers/iommu/io-pgtable-arm-v7s.c >> @@ -28,6 +28,7 @@ >> #include <linux/iommu.h> >> #include <linux/kernel.h> >> #include <linux/kmemleak.h> >> +#include <linux/module.h> >> #include <linux/sizes.h> >> #include <linux/slab.h> >> #include <linux/spinlock.h> >> @@ -839,6 +840,7 @@ struct io_pgtable_init_fns >> io_pgtable_arm_v7s_init_fns = { >> .alloc = arm_v7s_alloc_pgtable, >> .free = arm_v7s_free_pgtable, >> }; >> +EXPORT_SYMBOL_GPL(io_pgtable_arm_v7s_init_fns); >> #ifdef CONFIG_IOMMU_IO_PGTABLE_ARMV7S_SELFTEST >> @@ -984,3 +986,5 @@ static int __init arm_v7s_do_selftests(void) >> } >> subsys_initcall(arm_v7s_do_selftests); >> #endif >> + >> +MODULE_LICENSE("GPL v2"); >> diff --git a/drivers/iommu/io-pgtable-arm.c >> b/drivers/iommu/io-pgtable-arm.c >> index 87def58..2623d57 100644 >> --- a/drivers/iommu/io-pgtable-arm.c >> +++ b/drivers/iommu/io-pgtable-arm.c >> @@ -13,6 +13,7 @@ >> #include <linux/bitops.h> >> #include <linux/io-pgtable.h> >> #include <linux/kernel.h> >> +#include <linux/module.h> >> #include <linux/sizes.h> >> #include <linux/slab.h> >> #include <linux/types.h> >> @@ -1047,26 +1048,31 @@ struct io_pgtable_init_fns >> io_pgtable_arm_64_lpae_s1_init_fns = { >> .alloc = arm_64_lpae_alloc_pgtable_s1, >> .free = arm_lpae_free_pgtable, >> }; >> +EXPORT_SYMBOL_GPL(io_pgtable_arm_64_lpae_s1_init_fns); >> struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s2_init_fns = { >> .alloc = arm_64_lpae_alloc_pgtable_s2, >> .free = arm_lpae_free_pgtable, >> }; >> +EXPORT_SYMBOL_GPL(io_pgtable_arm_64_lpae_s2_init_fns); >> struct io_pgtable_init_fns io_pgtable_arm_32_lpae_s1_init_fns = { >> .alloc = arm_32_lpae_alloc_pgtable_s1, >> .free = arm_lpae_free_pgtable, >> }; >> +EXPORT_SYMBOL_GPL(io_pgtable_arm_32_lpae_s1_init_fns); >> struct io_pgtable_init_fns io_pgtable_arm_32_lpae_s2_init_fns = { >> .alloc = arm_32_lpae_alloc_pgtable_s2, >> .free = arm_lpae_free_pgtable, >> }; >> +EXPORT_SYMBOL_GPL(io_pgtable_arm_32_lpae_s2_init_fns); >> struct io_pgtable_init_fns io_pgtable_arm_mali_lpae_init_fns = { >> .alloc = arm_mali_lpae_alloc_pgtable, >> .free = arm_lpae_free_pgtable, >> }; >> +EXPORT_SYMBOL_GPL(io_pgtable_arm_mali_lpae_init_fns); >> #ifdef CONFIG_IOMMU_IO_PGTABLE_LPAE_SELFTEST >> @@ -1252,3 +1258,5 @@ static int __init arm_lpae_do_selftests(void) >> } >> subsys_initcall(arm_lpae_do_selftests); >> #endif >> + >> +MODULE_LICENSE("GPL v2"); >> _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
next prev parent reply other threads:[~2020-12-18 19:00 UTC|newest] Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-12-18 8:38 [RFC PATCH 0/3] iommu: Permit modular builds of io-pgtable drivers Isaac J. Manjarres 2020-12-18 8:38 ` Isaac J. Manjarres 2020-12-18 8:38 ` [PATCH 1/3] iommu/io-pgtable-arm: Prepare for modularization Isaac J. Manjarres 2020-12-18 8:38 ` Isaac J. Manjarres 2020-12-18 12:38 ` Robin Murphy 2020-12-18 12:38 ` Robin Murphy 2020-12-18 12:38 ` Robin Murphy 2020-12-18 18:59 ` isaacm [this message] 2020-12-18 18:59 ` isaacm 2020-12-21 15:22 ` Robin Murphy 2020-12-21 15:22 ` Robin Murphy 2020-12-21 15:22 ` Robin Murphy 2020-12-22 0:54 ` isaacm 2020-12-22 0:54 ` isaacm 2020-12-18 8:38 ` [PATCH 2/3] iommu/io-pgtable: " Isaac J. Manjarres 2020-12-18 8:38 ` Isaac J. Manjarres 2020-12-18 8:38 ` [PATCH 3/3] iommu/io-pgtable: Allow building as a module Isaac J. Manjarres 2020-12-18 8:38 ` Isaac J. Manjarres
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=34ea1af8569e4115e2dd1de61ae95bb6@codeaurora.org \ --to=isaacm@codeaurora.org \ --cc=iommu@lists.linux-foundation.org \ --cc=joro@8bytes.org \ --cc=kernel-team@android.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=pdaly@codeaurora.org \ --cc=pratikp@codeaurora.org \ --cc=robin.murphy@arm.com \ --cc=will@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.