[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2 09/15] x86/hyperlaunch: obtain cmdline from device tree
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |