On 06/05/2014 12:36 AM, Ian Campbell wrote:
On Wed, 2014-05-07 at 19:58 +0800, Fu Wei wrote:
Thanks for your work on this stuff!
I don't really know grub internals etc so this is mostly higher level comments.
Because we have not stub-xen for now, so I just keep "grub_arm64_disable_caches_mmu" and the relevant code(misc.c and misc_irq.S ) temporarily. Once the stub support for xen is upstreamed, we can delete those code. Do you agree? :-)
FWIW Leif has told me that grub has no equivalent support for direct booting a Linux arm64 Image file, that being the case I'm happy if you want to eventually remove this code from the Xen case[*].
I would like to do so! Once I get stub-xen image, and test it successfully, I will remove these redundant code immediately. But before that, it seems we can not boot xen image without these code.
When we can have stub support in xen image? Please let me know! Thanks :-)
Ideally the eventual bit which calls stub-xen would call into the same helper function(s) as is used for stub-linux or stub-* generally so that as much code as possible can be shared.
[*] TBH though I'm a bit surprised no one wants this ability from the Linux side, which could then also be shared with Xen, but whatever...
From eafa216fa21a0caf87e0f9ddcdcc5bc24d821390 Mon Sep 17 00:00:00 2001 From: Fu Wei fu.wei@linaro.org Date: Wed, 12 Mar 2014 15:39:26 +0800 Subject: [PATCH] Add multiboot/module command support in linux module for aarch64 Xen boot.
But it also can easily expand these commands to support other images by implementing "struct multiboot_function".
Is the reason for this abstraction simply due to the need to select compatibility strings (e.g. "xen,linux-zimage") for the various modules? I expected so but in multiboot_xen.c I see a bunch of other stuff, like the cache syncing stuff and FDT /chosen/ node creation and module handling, all of which I would have expected to be part of the common code. Perhaps I misunderstand something?
The reason for this abstraction is not just due to the compatibility strings. That's also due to how the multiboot kernel get info from GRUB. For now, we use FDT for xen, but maybe some other kernel will use ACPI or other different way. And maybe xen will use ACPI in the future, so I put FDT /chosen/ node creation into multiboot_xen.c, but not multiboot.c, and leave a interface called prepare_*_params.
For the cache syncing stuff, I will remove it once we have stub-xen.
WRT the compatibility strings http://wiki.xen.org/wiki/Xen_ARM_with_Virtualization_Extensions/Multiboot defines things like "multiboot,linux-zimage" and "multiboot,module" while you have implemented what is described in docs/misc/arm/device-tree/booting.txt which is "xen,linux-zimage" etc.
I can see why you did this (it's what Xen actually supports today) but I wonder if in the interests of reducing the special cases I should create a Xen patch which causes it to accept both names so that you can just implement the "multiboot,*" stuff in common code without the special cases? (Although that depends on the reason for the other non-compat string special cases too)
I think the "multiboot,*" stuff will be better, once Xen supports this, I will update this code.
Could optionally extend that with a multiboot --type=xen option to make grub include the xen,* names *in addition to* the mulitboot,* ones -- which would be nice because it would make Xen 4.4 Just Work (although I would also intend to backport the Xen patch I mentioned).
yes, for this version of patch, you can just use --type="multiboot,linux-zimage" to customize the compatibility strings. This feature is in http://wiki.xen.org/wiki/Xen_ARM_with_Virtualization_Extensions/Multiboot, I have implemented it in this patch.
BTW, looking through the patch I saw lots of places where the whitespace appeared a bit off, I don't know grub coding style but e.g.
- if (multiboot_fdt && multiboot_fdt_size > 0) {
- grub_arch_sync_caches ((void *) multiboot_fdt, multiboot_fdt_size);
- }
and
- if (module && module->extra_info)
- {
node_info = (xen_node_info_t) module->extra_info;- } else {
return (grub_error (GRUB_ERR_BAD_ARGUMENT, N_("failed to get kernel")));- }
Don't appear likely to be correct in terms of their indentation.
yes, I will improve the coding style to meet the GRUB coding style.
diff --git a/grub-core/loader/arm64/multiboot.c b/grub-core/loader/arm64/multiboot.c new file mode 100644 index 0000000..6fe5633 --- /dev/null +++ b/grub-core/loader/arm64/multiboot.c
+static grub_err_t +grub_multiboot_boot (void) +{
- void *params_blob;
- kernel_entry_t linuxmain;
- params_blob = finalize_params_multiboot();
- if (!params_blob)
- return (grub_error (GRUB_ERR_BAD_OS, "failed to finalize boot params"));
- grub_dprintf ("linux", "Jumping to Multiboot Kernel @ 0x%lx\n",
kernel->start);I suppose from about this point this function will eventually call some existing "launch an EFI COFF binary" helper instead?
yes, you are right, once you have stub-xen, I can call UEFI boot service instead.
- /* Boot the ARM64 system.
- Arguments to Multiboot Kernel:
x0 - address of params blob (e.g. DTB)
x1 - 0 (reserved for future use)
x2 - 0 (reserved for future use)
x3 - 0 (reserved for future use)- */
- linuxmain = (kernel_entry_t) kernel->start;
- sync_caches();
- grub_arm64_prepare_platform();
- linuxmain (params_blob, 0, 0, 0);
- return (grub_error (GRUB_ERR_BAD_OS, "Multiboot call returned"));
+}
- node_info->reg.reg_64 = module->start;
- node_info->reg.reg_32[0] = grub_cpu_to_be32((grub_uint32_t)node_info->reg.reg_64);
- node_info->reg.reg_32[1] = grub_cpu_to_be32((grub_uint32_t)module->size);
I think this node_info->reg is a union, so don't reg_64 and reg_32 overlap in memory? So assigning to both in this way seems odd. I think this is equivalent to:
- node_info->reg.reg_32[0] = grub_cpu_to_be32((grub_uint32_t)module->start);
- node_info->reg.reg_32[1] = grub_cpu_to_be32((grub_uint32_t)module->size);
isn't it?
Yes , that would be better, Thanks , will improve it! :-)
What happens on systems which only have high RAM above 4GB? (I know of at least one). Won't the module start then be truncated here?
IOW don't you want to use #address-cells = 2? Modules larger than 4GB are probably less likely to occur, but why not use #size-cells = 2 as well.
Yes, of course. That would be better. Please inform me, once you finish this. Then I can improve that in this patch.
[...]
diff --git a/include/grub/arm64/linux.h b/include/grub/arm64/linux.h index 864e5dc..c54dfaf 100644 --- a/include/grub/arm64/linux.h +++ b/include/grub/arm64/linux.h @@ -38,4 +38,12 @@ struct grub_arm64_linux_kernel_header grub_uint32_t hdr_offset; /* Offset of PE/COFF header */ }; +/****The accessor functions for dtb and the "loaded" flag****/ +void EXPORT_FUNC (*grub_arm64_linux_get_fdt) (void);
+int EXPORT_FUNC (grub_arm64_linux_get_loaded) (void);
+void EXPORT_FUNC (grub_arm64_linux_set_loaded) (int loaded_flag);
Aren't all the callers of these two get/set_loaded interfaces in the same file (multiboot.c)?
yes, all the callers are in the multiboot.c
+/************************************/
#endif /* ! GRUB_LINUX_CPU_HEADER */
+#define GRUB_EFI_PAGE_SHIFT 12 +#define BYTES_TO_PAGES(bytes) (((bytes) + 0xfff) >> GRUB_EFI_PAGE_SHIFT)
+#define GRUB_MULTIBOOT_FLAGS_UNIQUE (0x1)
Is this some sort of concept of a module (compatibility string) which must only appear once? I can't see where you actually enforce this, but TBH I'm not sure it necessary, either multiple instances are allowed or its Garbage-In-Garbage-Out and up to the multiboot kernel how it wants to react rather than grubs responsibility to stop it happening.
if the module has this flag, that means the type of module will only have one instance in the multiboot module "list". if you load this type of module again, the code will unload the previous one before loading the new one. and this also depend on the implementation of multiboot(like multiboot_xen.c), you can just ignore it in multiboot_*.c(don't set it in struct multiboot_module),
It's just a option, and default to "unset".
For example, we use a single xen kernel to boot the system. if some one load it again, the code will use the new one. and not need to reload all the components.
I have enforced this in multiboot.c(line 340).
+#define GRUB_MULTIBOOT_FLAGS_CMDLINE (0x2)
I'm not sure if this is intended as "module has a command line" or "module can validly have a command line". The former is equivalent to module->cmdline != NULL, I think and the second is something which I don't think grub needs to worry about -- it's up to the multiboot kernel how it wants to deal with command lines for modules where it doesn't make sense (i.e. ignore them).
it is also a option, if you know some type of module don't need the command line, you can set this, then the code will simply ignore the strings after the module file name, otherwise, the grub will save the arguments to the struct multiboot_module node. But how to deal with this is up to the multiboot kernel.
+#ifndef MULTIBOOT_XEN_HEADER +#define MULTIBOOT_XEN_HEADER 1
+#include <grub/types.h>
+#define XEN_MODULE_DEFAULT_ALIGN (0x0)
+#define XEN_KERNEL_MIN_ALIGN (0x200000) +#define XEN_MODULE_IMAGE_MIN_ALIGN XEN_MODULE_DEFAULT_ALIGN +#define XEN_MODULE_INITRD_MIN_ALIGN XEN_MODULE_DEFAULT_ALIGN +#define XEN_MODULE_XSM_MIN_ALIGN XEN_MODULE_DEFAULT_ALIGN +#define XEN_MODULE_OTHER_MIN_ALIGN XEN_MODULE_DEFAULT_ALIGN +#define XEN_MODULE_CUSTOM_MIN_ALIGN XEN_MODULE_DEFAULT_ALIGN
Perhaps we should just specify that all multiboot modules will be either unaligned or 2M aligned (generically, not just for Xen)?
yes, if we can specify those, that would be better. And it is easy to set here, right?
+#define XEN_KERNEL_BOOTARGS "xen,xen-bootargs" +#define XEN_MODULE_IMAGE_NAME "linux-zimage" +#define XEN_MODULE_IMAGE_COMPATIBLE "xen,linux-zimage\0xen,multiboot-module" +#define XEN_MODULE_INITRD_NAME "linux-initrd" +#define XEN_MODULE_INITRD_COMPATIBLE "xen,linux-initrd\0xen,multiboot-module" +#define XEN_MODULE_XSM_NAME "xsm-policy" +#define XEN_MODULE_XSM_COMPATIBLE "xen,xsm-policy\0xen,multiboot-module" +#define XEN_MODULE_OTHER_NAME "multiboot-module" +#define XEN_MODULE_OTHER_COMPATIBLE "xen,multiboot-module" +#define XEN_MODULE_DOM0_BOOTARGS "xen,dom0-bootargs"
There are several places where Xen will look for the dom0 command line. I'd recommend that for avoiding any need to special casing you just use the bootargs property of every module node instead of special casing the dom0 command line to be /chosen/xen,dom0-bootargs like this.
+#define XEN_MODULE_BOOTARGS "bootargs"
This isn't Xen specific, /chosen/bootargs is always the command line for the multiboot kernel. (again, there are other things which Xen will parse, but for reduction of special casing I think you should use the generic option)
Actually, I used "bootargs" for dom0 before(V1/V2), but I got a info from julieng: Apr 16 20:48:35 <julieng> fuwei: Thanks, FYI bootargs in module is deprecated (see docs/misc/arm/device-tree/booting.txt).
So I turned to use /chosen/xen,dom0-bootargs.
Please let me know which one will be used in the future. IMO, I would like to use "bootargs" in the module node. :-)
+#define XEN_COMPATIBLE_MAX_SIZE (128) +#define XEN_MODULE_NAME_MAX_SIZE (16)
+enum xen_module_type
- {
- XEN_MODULE_IMAGE,
- XEN_MODULE_INITRD,
- XEN_MODULE_XSM,
- XEN_MODULE_OTHER,
- XEN_MODULE_CUSTOM
- };
+typedef enum xen_module_type xen_module_type_t;
What is the purpose of this enum? It seems you parse the module options from strings into this and then translated them back into (the same) strings again when you come to boot. Couldn't you just treat everything using the same scheme as you have used for XEN_MODULE_CUSTOM i.e. store the string?
yes, I can treat all the module just like XEN_MODULE_CUSTOM. If so, the user need to use "--type" with "module" command all the time, and can not load the module by order(this is described in http://wiki.xen.org/wiki/Xen_ARM_with_Virtualization_Extensions/Multiboot).
I also use this enum data to trace the order of the module.
+struct xen_node_info
Nothing in this struct seems especially Xen specific, per the spec in http://wiki.xen.org/wiki/Xen_ARM_with_Virtualization_Extensions/Multiboot the reg and command line are common fields (and indeed this struct seems to duplicate parts of struct multiboot_module).
+{
- xen_module_type_t type;
- const char *compatible;
- char compatible_custom[XEN_COMPATIBLE_MAX_SIZE];
- int compatible_size;
- union {
- grub_uint32_t cells_info[2];
- grub_uint32_t reg_32[2];
- grub_uint64_t reg_64;
- } reg;
- const char *cmdline_name;
+};
For struct xen_node_info, my thought is : For xen, we expect the compatible string, the reg info and cmdline. but we don't know if other kernel(that use multiboot function in the future) need more info or different info, even it doesn't use FDT.
So I just add a "void *extra_info;" in the struct multiboot_module: for Xen, this *extra_info point to xen_node_info; for other kernel, this *extra_info may point to other different data struct.
And struct xen_node_info carries the different info, it is not the duplicate parts of struct multiboot_module. For example, compatible stuff, cmdline_name(not cmdline string). Although, the "reg" carries the load address info of module, but in different format that can be use directly in "prepare_*_params_xen".That is my thought
-------------------------------------------------------------------------------------------------
And great thanks for these feedback above, please let me know if you have other thought or suggestion.
Thanks again!!! :-)
Cheers, Ian.