From a627666e0627b6897c8d89be2e8b4eefea4071f7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mart=C3=AD=20Bol=C3=ADvar?= Date: Fri, 24 Sep 2021 12:54:17 -0700 Subject: [PATCH] device: add fudge factor for handle padding MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When CONFIG_USERSPACE is enabled, the ELF file from linker pass 1 is used to create a hash table that identifies kernel objects by address. We therefore can't allow the size of any object in the pass 2 ELF to change in a way that would change those addresses, or we would create a garbage hash table. Simultaneously (and regardless of CONFIG_USERSPACE's value), gen_handles.py must transform arrays of handles from their pass 1 values to their pass 2 values; see the file's docstring for more details on that transformation. The way this works is that gen_handles.py just pads out each pass 2 array so its length is the same as its pass 1 value. The padding value is a repeated run of DEVICE_HANDLE_ENDS values. This value is the terminator which we look for at runtime in places like device_required_handles_get(), so there must be at least one, and we error out in gen_handles.py if there's no room in the pass 2 array for at least one such value. (If there is extra room, we just keep inserting extra DEVICE_HANDLE_ENDS values to pad the array to its original length.) However, it is possible that a device has more direct dependencies in the pass 2 handles array than its corresponding devicetree node had in the pass 1 array. When this happens, users have no recourse, so that's a potential showstopper. To work around this possibility for now, add a new config option, CONFIG_DEVICE_HANDLE_PADDING, whose value defaults to 0. When nonzero, it is a count of padding handles that are inserted into each device handles array. When gen_handles.py errors out due to lack of room, its error message now tells the user how much to increase CONFIG_DEVICE_HANDLE_PADDING by to work around the problem. It looks like a real fix for this is to allocate kernel objects whose addresses are required for hash tables in CONFIG_USERSPACE=y configurations *before* the handle arrays. The handle arrays could then be resized as needed in pass 2, which saves ROM by avoiding unnecessary padding, and would avoid the need for CONFIG_DEVICE_HANDLE_PADDING altogether. However, this 'real fix' is not available and we are facing a deadline to get a temporary solution in for Zephyr v2.7.0, so this is a good enough workaround for now. Signed-off-by: Martí Bolívar --- include/device.h | 45 ++++++++++++++++++++++++++++++++++++++++++ kernel/Kconfig | 16 +++++++++++++++ scripts/gen_handles.py | 10 ++++++++-- 3 files changed, 69 insertions(+), 2 deletions(-) diff --git a/include/device.h b/include/device.h index 3c71f6a0c1561f..87599d6086d81a 100644 --- a/include/device.h +++ b/include/device.h @@ -699,6 +699,50 @@ static inline bool device_is_ready(const struct device *dev) Z_DEVICE_STATE_DEFINE(node_id, dev_name) \ Z_DEVICE_DEFINE_PM_SLOT(dev_name) +/* Helper macros needed for CONFIG_DEVICE_HANDLE_PADDING. These should + * be deleted when that option is removed. + * + * This is implemented "by hand" -- rather than using a helper macro + * like UTIL_LISTIFY() -- because we need to allow users to wrap + * DEVICE_DT_DEFINE with UTIL_LISTIFY, like this: + * + * #define DEFINE_FOO_DEVICE(...) DEVICE_DT_DEFINE(...) + * UTIL_LISTIFY(N, DEFINE_FOO_DEVICE) + * + * If Z_DEVICE_HANDLE_PADDING uses UTIL_LISTIFY, this type of code + * would fail, because the UTIL_LISTIFY token within the + * Z_DEVICE_DEFINE_HANDLES expansion would not be expanded again, + * since it appears in a context where UTIL_LISTIFY is already being + * expanded. Standard C does not reexpand macros appearing in their + * own expansion; this would lead to infinite recursions in general. + */ +#define Z_DEVICE_HANDLE_PADDING \ + Z_DEVICE_HANDLE_PADDING_(CONFIG_DEVICE_HANDLE_PADDING) +#define Z_DEVICE_HANDLE_PADDING_(count) \ + Z_DEVICE_HANDLE_PADDING__(count) +#define Z_DEVICE_HANDLE_PADDING__(count) \ + Z_DEVICE_HANDLE_PADDING_ ## count +#define Z_DEVICE_HANDLE_PADDING_10 \ + DEVICE_HANDLE_ENDS, Z_DEVICE_HANDLE_PADDING_9 +#define Z_DEVICE_HANDLE_PADDING_9 \ + DEVICE_HANDLE_ENDS, Z_DEVICE_HANDLE_PADDING_8 +#define Z_DEVICE_HANDLE_PADDING_8 \ + DEVICE_HANDLE_ENDS, Z_DEVICE_HANDLE_PADDING_7 +#define Z_DEVICE_HANDLE_PADDING_7 \ + DEVICE_HANDLE_ENDS, Z_DEVICE_HANDLE_PADDING_6 +#define Z_DEVICE_HANDLE_PADDING_6 \ + DEVICE_HANDLE_ENDS, Z_DEVICE_HANDLE_PADDING_5 +#define Z_DEVICE_HANDLE_PADDING_5 \ + DEVICE_HANDLE_ENDS, Z_DEVICE_HANDLE_PADDING_4 +#define Z_DEVICE_HANDLE_PADDING_4 \ + DEVICE_HANDLE_ENDS, Z_DEVICE_HANDLE_PADDING_3 +#define Z_DEVICE_HANDLE_PADDING_3 \ + DEVICE_HANDLE_ENDS, Z_DEVICE_HANDLE_PADDING_2 +#define Z_DEVICE_HANDLE_PADDING_2 \ + DEVICE_HANDLE_ENDS, Z_DEVICE_HANDLE_PADDING_1 +#define Z_DEVICE_HANDLE_PADDING_1 \ + DEVICE_HANDLE_ENDS, Z_DEVICE_HANDLE_PADDING_0 +#define Z_DEVICE_HANDLE_PADDING_0 EMPTY /* Initial build provides a record that associates the device object * with its devicetree ordinal, and provides the dependency ordinals. @@ -735,6 +779,7 @@ BUILD_ASSERT(sizeof(device_handle_t) == 2, "fix the linker scripts"); )) \ DEVICE_HANDLE_SEP, \ Z_DEVICE_EXTRA_HANDLES(__VA_ARGS__) \ + Z_DEVICE_HANDLE_PADDING \ }; #ifdef CONFIG_PM_DEVICE diff --git a/kernel/Kconfig b/kernel/Kconfig index 9bf3d6d01051df..d9480ec44c999f 100644 --- a/kernel/Kconfig +++ b/kernel/Kconfig @@ -303,6 +303,22 @@ config WAITQ_DUMB endchoice # WAITQ_ALGORITHM +config DEVICE_HANDLE_PADDING + int "Number of additional device handles to use for padding" + default 0 + range 0 10 + help + This is a "fudge factor" which works around build system + limitations. It is safe to ignore unless you get a specific + build system error about it. + + The option's value is the number of superfluous device handle + values which are introduced into the array pointed to by each + device's 'handles' member during the first linker pass. + + Each handle uses 2 bytes, so a value of 3 would use an extra + 6 bytes of ROM for every device. + menu "Kernel Debugging and Metrics" config INIT_STACKS diff --git a/scripts/gen_handles.py b/scripts/gen_handles.py index 1d19619d7d1e75..558ce82c707c45 100755 --- a/scripts/gen_handles.py +++ b/scripts/gen_handles.py @@ -311,9 +311,15 @@ def main(): # address. We can't allow the size of any object in the # final elf to change. We also must make sure at least one # DEVICE_HANDLE_ENDS is inserted. - assert len(hdls) < len(hs.handles), "%s no DEVICE_HANDLE_ENDS inserted" % (dev.sym.name,) - while len(hdls) < len(hs.handles): + padding = len(hs.handles) - len(hdls) + assert padding > 0, \ + (f"device {dev.sym.name}: " + "linker pass 1 left no room to insert DEVICE_HANDLE_ENDS. " + "To work around, increase CONFIG_DEVICE_HANDLE_PADDING by " + + str(1 + (-padding))) + while padding > 0: hdls.append(DEVICE_HANDLE_ENDS) + padding -= 1 assert len(hdls) == len(hs.handles), "%s handle overflow" % (dev.sym.name,) lines = [