[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 2/2] arm/efi: Use dom0less configuration when using EFI boot
A number of nits, sorry: On 15.09.2021 16:26, Luca Fancellu wrote: > --- a/xen/arch/arm/efi/efi-boot.h > +++ b/xen/arch/arm/efi/efi-boot.h > @@ -8,9 +8,39 @@ > #include <asm/setup.h> > #include <asm/smp.h> > > +typedef struct { > + char* name; Misplaced *. > + int name_len; Surely this can't go negative? (Same issue elsewhere.) > +} dom0less_module_name; > + > +/* > + * Binaries will be translated into bootmodules, the maximum number for them > is > + * MAX_MODULES where we should remove a unit for Xen and one for Xen DTB > + */ > +#define MAX_DOM0LESS_MODULES (MAX_MODULES - 2) > +static struct file __initdata dom0less_files[MAX_DOM0LESS_MODULES]; > +static dom0less_module_name __initdata > dom0less_bin_names[MAX_DOM0LESS_MODULES]; > +static uint32_t __initdata dom0less_modules_available = MAX_DOM0LESS_MODULES; > +static uint32_t __initdata dom0less_modules_idx = 0; Please see ./CODING_STYLE for your (ab)use of uint32_t here and elsewhere. > +#define ERROR_DOM0LESS_FILE_NOT_FOUND -1 Macros expanding to more than a single token should be suitably parenthesized at least when the expression can possibly be mistaken precedence wise (i.e. array[n] is in principle fine without parentheses, because the meaning won't change no matter how it's used in an expression). > void noreturn efi_xen_start(void *fdt_ptr, uint32_t fdt_size); > void __flush_dcache_area(const void *vaddr, unsigned long size); > > +static int __init get_dom0less_file_index(const char* name, int name_len); > +static uint32_t __init allocate_dom0less_file(EFI_FILE_HANDLE dir_handle, > + const char* name, int > name_len); > +static void __init handle_dom0less_module_node(EFI_FILE_HANDLE dir_handle, > + int module_node_offset, > + int reg_addr_cells, > + int reg_size_cells); > +static void __init handle_dom0less_domain_node(EFI_FILE_HANDLE dir_handle, > + int domain_node, > + int addr_cells, > + int size_cells); > +static bool __init check_dom0less_efi_boot(EFI_FILE_HANDLE dir_handle); There are attributes (e.g. __must_check) which belong on the declarations. __init, however, belongs on the definitions. > --- a/xen/common/efi/boot.c > +++ b/xen/common/efi/boot.c > @@ -1134,8 +1134,9 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE > *SystemTable) > EFI_GRAPHICS_OUTPUT_PROTOCOL *gop = NULL; > union string section = { NULL }, name; > bool base_video = false; > - const char *option_str; > + const char *option_str = NULL; > bool use_cfg_file; > + bool dom0less_found = false; > > __set_bit(EFI_BOOT, &efi_flags); > __set_bit(EFI_LOADER, &efi_flags); > @@ -1285,14 +1286,21 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE > *SystemTable) > efi_bs->FreePool(name.w); > } > > - if ( !name.s ) > - blexit(L"No Dom0 kernel image specified."); > - > efi_arch_cfg_file_early(loaded_image, dir_handle, section.s); > > - option_str = split_string(name.s); > +#ifdef CONFIG_ARM > + /* dom0less feature is supported only on ARM */ > + dom0less_found = check_dom0less_efi_boot(dir_handle); > +#endif > + > + if ( !name.s && !dom0less_found ) Here you (properly ) use !name.s, > + blexit(L"No Dom0 kernel image specified."); > + > + if ( name.s != NULL ) Here you then mean to omit the "!= NULL" for consistency and brevity. > + option_str = split_string(name.s); > > - if ( !read_section(loaded_image, L"kernel", &kernel, option_str) ) > + if ( (!read_section(loaded_image, L"kernel", &kernel, option_str)) && Stray parentheses. > + (name.s != NULL) ) See above. I also don't think this logic is quite right: If you're dom0less, why would you want to look for an embedded Dom0 kernel image? Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |