On 26.12.2024 17:57, Daniel P. Smith wrote:
> If a command line is not provided through the bootloader's mechanism, e.g.
> muiltboot module string field, then use one from the device tree if present.
> The device tree command line is located in the bootargs property of the
> `multiboot,kernel` node.
This reads as if it can be mix-and-match (and the code looks to confirm
this), which doesn't sound quite right. If you deem it right, please add
justification here.
> --- a/xen/arch/x86/domain-builder/core.c
> +++ b/xen/arch/x86/domain-builder/core.c
> @@ -8,9 +8,37 @@
> #include <xen/lib.h>
>
> #include <asm/bootinfo.h>
> +#include <asm/setup.h>
>
> #include "fdt.h"
>
> +size_t __init builder_get_cmdline_size(struct boot_info *bi, int offset)
> +{
> +#ifdef CONFIG_DOMAIN_BUILDER
> + const void *fdt = bootstrap_map_bm(&bi->mods[HYPERLAUNCH_MODULE_IDX]);
> + int size = fdt_cmdline_prop_size(fdt, offset);
> +
> + bootstrap_unmap();
> + return size < 0 ? 0 : (size_t) size;
Nit: While I wouldn't insist, we don't normally put blanks after casts.
(The cast also isn't really needed here anyway.)
> +#else
> + return 0;
> +#endif
> +}
> +
> +int __init builder_get_cmdline(
> + struct boot_info *bi, int offset, char *cmdline, size_t size)
> +{
> +#ifdef CONFIG_DOMAIN_BUILDER
> + const void *fdt = bootstrap_map_bm(&bi->mods[HYPERLAUNCH_MODULE_IDX]);
> + int ret = fdt_cmdline_prop_copy(fdt, offset, cmdline, size);
> +
> + bootstrap_unmap();
> + return ret;
> +#else
> + return 0;
> +#endif
> +}
Such #ifdef-ary is better to be avoided by providing stubs in the header.
Which then also works towards not needing to build in domain-builder/ when
COMFIG_DOMAIN_BUILDER=n.
> --- a/xen/arch/x86/domain-builder/fdt.c
> +++ b/xen/arch/x86/domain-builder/fdt.c
> @@ -109,6 +109,16 @@ static int __init process_domain_node(
> printk(" kernel: boot module %d\n", idx);
> bi->mods[idx].type = BOOTMOD_KERNEL;
> bd->kernel = &bi->mods[idx];
> +
> + /* If bootloader didn't set cmdline, see if FDT provides one. */
> + if ( bd->kernel->cmdline_pa &&
> + !((char *)__va(bd->kernel->cmdline_pa))[0] )
> + {
> + int ret = fdt_get_prop_by_offset(
> + fdt, node, "bootargs", &bd->kernel->cmdline_pa);
> + if ( ret > 0 )
> + bd->kernel->fdt_cmdline = true;
Is there a guarantee that fdt_get_prop_by_offset() won't alter its output
(passed in by indirection) in case of failure? Otherwise ...
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -981,7 +981,10 @@ static size_t __init domain_cmdline_size(
> {
> size_t s = bi->kextra ? strlen(bi->kextra) : 0;
>
> - s += bd->kernel->cmdline_pa ? strlen(__va(bd->kernel->cmdline_pa)) : 0;
> + if ( bd->kernel->fdt_cmdline )
> + s += builder_get_cmdline_size(bi, bd->kernel->cmdline_pa);
> + else
> + s += strlen(__va(bd->kernel->cmdline_pa));
... you'll be hosed here (and elsewhere).
> --- a/xen/include/xen/libfdt/libfdt-xen.h
> +++ b/xen/include/xen/libfdt/libfdt-xen.h
> @@ -28,6 +28,30 @@ static inline int __init fdt_cell_as_u64(const fdt32_t
> *cell, uint64_t *val)
> return 0;
> }
>
> +static inline int __init fdt_get_prop_by_offset(
> + const void *fdt, int node, const char *name, unsigned long *offset)
> +{
> + int ret, poffset;
> + const char *pname;
> + size_t nsize = strlen(name);
> +
> + fdt_for_each_property_offset(poffset, fdt, node)
> + {
> + fdt_getprop_by_offset(fdt, poffset, &pname, &ret);
> +
> + if ( ret < 0 || strlen(pname) != nsize )
> + continue;
> +
> + if ( !strncmp(pname, name, nsize) )
I find this slightly odd: By now we know strlen(name) == strlen(pname) == nsize.
You could then use the usually more efficient memcmp() or the easier to invoke
strcmp().
Jan
