On 2025-04-14 17:05, Jan Beulich wrote:
On 14.04.2025 15:37, Alejandro Vallejo wrote:
On Thu Apr 10, 2025 at 11:42 AM BST, Jan Beulich wrote:
On 08.04.2025 18:07, Alejandro Vallejo wrote:
+/*
+ * Locate a multiboot module given its node offset in the FDT.
+ *
+ * The module location may be given via either FDT property:
+ * * reg = <address, size>
+ * * Mutates `bi` to append the module.
+ * * module-index = <idx>
+ * * Leaves `bi` unchanged.
+ *
+ * @param fdt Pointer to the full FDT.
+ * @param node Offset for the module node.
+ * @param address_cells Number of 4-octet cells that make up an
"address".
+ * @param size_cells Number of 4-octet cells that make up a
"size".
+ * @param bi[inout] Xen's representation of the boot
parameters.
+ * @return -EINVAL on malformed nodes, otherwise
+ * index inside `bi->mods`
+ */
+int __init fdt_read_multiboot_module(const void *fdt, int node,
+ int address_cells, int
size_cells,
+ struct boot_info *bi)
Functions without callers and non-static ones without declarations
are
disliked by Misra.
Can't do much about it if I want them to stand alone in a single
patch.
Otherwise the following ones become quite unwieldy to look at. All I
can
say is that this function becomes static and with a caller on the next
patch.
Which means you need to touch this again anyway. Perhaps we need a
Misra
deviation for __maybe_unused functions / data, in which case you could
use that here and strip it along with making the function static.
Cc-ing
Bugseng folks.
There is already an exception for __maybe_unused on labels (Rule 2.6).
In principle it could be easily extended to encompass unused functions
(which are verified by another rule), with a suitable rationale.
+ /* Otherwise location given as a `reg` property. */
+ prop = fdt_get_property(fdt, node, "reg", NULL);
+
+ if ( !prop )
+ {
+ printk(" No location for multiboot,module\n");
+ return -EINVAL;
+ }
+ if ( fdt_get_property(fdt, node, "module-index", NULL) )
+ {
+ printk(" Location of multiboot,module defined multiple
times\n");
+ return -EINVAL;
+ }
+
+ ret = read_fdt_prop_as_reg(prop, address_cells, size_cells,
&addr, &size);
+
+ if ( ret < 0 )
+ {
+ printk(" Failed reading reg for multiboot,module\n");
+ return -EINVAL;
+ }
+
+ idx = bi->nr_modules + 1;
This at least looks like an off-by-one. If the addition of 1 is
really
intended, I think it needs commenting on.
Seems to be, yes. The underlying array is a bit bizarre. It's sizes as
MAX_NR_BOOTMODS + 1, with the first one being the DTB itself. I guess
the intent was to take it into account, but bi->nr_modules is
initialised to the number of multiboot modules, so it SHOULD be
already
taking it into account.
Also, the logic for bounds checking seems... off (because of the + 1 I
mentioned before). Or at least confusing, so I've moved to using
ARRAY_SIZE(bi->mods) rather than explicitly comparing against
MAX_NR_BOOTMODS.
The array is MAX_NR_BOOTMODS + 1 in length, so it's just more
cognitive
load than I'm comfortable with.
If I'm not mistaken the +1 is inherited from the modules array we had
in
the past, where we wanted 1 extra slot for Xen itself. Hence before you
move to using ARRAY_SIZE() everywhere it needs to really be clear what
the +1 here is used for.
--- a/xen/include/xen/libfdt/libfdt-xen.h
+++ b/xen/include/xen/libfdt/libfdt-xen.h
@@ -13,6 +13,63 @@kkk
#include <xen/libfdt/libfdt.h>
+static inline int __init fdt_cell_as_u32(const fdt32_t *cell)
Why plain int here, but ...
+{
+ return fdt32_to_cpu(*cell);
+}
+
+static inline uint64_t __init fdt_cell_as_u64(const fdt32_t *cell)
... a fixed-width and unsigned type here? Question is whether the
former
helper is really warranted.
Also nit: Stray double blank.
+{
+ return ((uint64_t)fdt32_to_cpu(cell[0]) << 32) |
fdt32_to_cpu(cell[1]);
That is - uniformly big endian?
These helpers are disappearing, so it doesn't matter. This is
basically
an open coded:
fdt64_to_cpu(*(const fdt64_t *)fdt32)
And, yes. DTBs are standardised as having big-endian properties, for
better or worse :/
+}
Marking such relatively generic inline functions __init is also
somewhat
risky.
They were originally in domain-builder/fdt.c and moved here as a
result
of a request to have them on libfdt. libfdt proved to be somewhat
annoying because it would be hard to distinguish accessors for the
flattened and the unflattened tree.
I'd personally have them in domain-builder instead, where they are
used.
Should they be needed somewhere else, we can always fator them out
somewhere else.
Thoughts?
As long as they're needed only by domain-builder, it's probably fine to
have
them just there.
Jan
--
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253
