Hi Leif, I have make a multiboot support patch V3.1 following your suggestion for V3.
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? :-)
Great thanks!
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[*].
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?
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)
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).
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.
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?
- /* 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?
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.
[...]
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)?
+/************************************/
#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.
+#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).
+#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)?
+#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)
+#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?
+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;
+};
Cheers, Ian.
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.
On Thu, 2014-06-05 at 15:46 +0800, Fu Wei wrote:
On 06/05/2014 12:36 AM, Ian Campbell wrote:
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 :-)
Roy Franz from Linaro is working on this, he's making good progress, but I don't want to guess a deadline for him ;-).
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.
The specification which we have defined here is that it will use device tree, something which uses another mechanism would not be following this spec. Even when Xen uses ACPI it will still receive a device tree as part of the (existing, Linux) boot protocol which includes the chosen node and hence the multiboot modules.
multiboot_fdt.c would be a better name than multiboot_xen.c, if that's what you intend the abstraction to be about, but TBH this seems like a premature refactoring to me. If/when someone invents up "multiboot over $SOMETHING_ELSE" then refactoring can be done then.
For the cache syncing stuff, I will remove it once we have stub-xen.
OK.
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.
I have a prototype patch. I'll send it separately in a moment.
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.
What I meant was that we could arrange that: multiboot /xen-4.1-amd64.gz [...] module /vmlinuz-3.10-3-amd64 [...]
Would produce a module with compatible = "multiboot,linux-zimage", "boot,module"
But: multiboot --type=xen /xen-4.1-amd64.gz [...] module /vmlinuz-3.10-3-amd64 [...] would produce: compatible = "xen,linux-zimage", "multiboot,linux-zimage", "boot,module"
i.e enable additional Xen compatibility strings. This would be an extension to the spec as described in the wiki which is why I described it as optional/would be nice. Feel free to ignore this suggested extension.
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.
Via some existing grub interfaces rather than duplicating the tail of the stub-linux code here, right?
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.
AFAIK the above change has no dependency on me finishing anything -- Xen already copes with #address-cells/#size-cells = 2 correctly, since we support platforms which only have RAM above 4GB already.
(If you find otherwise please report as a bug...)
[...]
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
So you can just have a static global in that file I think (or static accessors if that's a grub style thing).
+/************************************/
#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.
So these are flags on the kernel and not the modules?
I think the kernel is inherently unique, so doesn't need a flag. For the modules I don't think it is grub's business to enforce uniqueness. If I specify multiboot /xen.gz module --type "multiboot,linux-zimage" --type "boot,module" /vmlinuz1 module --type "multiboot,linux-initrd" --type "boot,module" /initrd1 module --type "multiboot,linux-zimage" --type "boot,module" /vmlinuz2 then that's up to me and grub shouldn't second guess what I'm doing.
(this is the sort of thing a multiboot kernel might implement support for if it wanted to give the ability to create 1 domain created at boot time, I think it is within the spec to do so)
I have enforced this in multiboot.c(line 340).
Please don't ;-)
+#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.
I don't think it is up to grub to enforce this sort of thing. If I want to give a command line to my initrd then that's my business, even if it might be meaningless to the kernel ... Including a command line for something which doesn't logically require one is fine -- it'll be ignored by the multiboot kernel.
I think you should remove both of these GRUB_MULTIBOOT_FLAGS_*. It'll make the code simpler and remove unnecessary restrictions which aren't defined by the spec.
+#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.
If we are agreed on this then I think we can just update the wiki. Eventually we should probably move this spec to somewhere more controlled and/or refrain from changing it.
Since in effect grub defines the other forms of multiboot anyway it's possible that there should be a separate documentation patch which adds the spec to the grub docs?
And it is easy to set here, right?
I think if we specify the module alignment you don't need to define it for each module type.
In the future we could consider expanding the spec to support modules --alignment=XXX, but I don't think you need to do that now, unless you are keen to do so.
+#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. :-)
Please do, it is in no way deprecated, I don't know what Julien was talking about.
The order in which Xen will look at various nodes/properties for the bootargs for itself and/or the modules is well defined (see the list in booting.txt) and most definitely includes /chosen/module@XXX/bootargs as an option, exactly for multiboot implementations such as this.
Support for the other property names is there mainly to make it possible to write a generic u-boot environment which can be used with both Xen and native Linux boot.scr, but of course you don't care one jot about that here. Using /chosen/modules@XXX/bootargs is absolutely the right thing for you to be using in the context of multiboot.
+#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.
You don't need an enum for this. Just do const char *default_compat_types[] = { "multiboot,linux-zimage", "multiboot,linux-ramdisk" } int default_type_nr = 0;
...
if ( there is a --type argument ) { module->type = strdup(the argument) } else { if ( default_type_nr > ARRAY_SIZE(default_compat_types) module->type = strdup("multiboot,module"); else module->type = strdupandcat(default_compat_types[default_type_nr++], "multiboot,module");
or something like that, (I'm sure my handling of "multiboot,module" is wrong, but you get the general idea).
or maybe: #define COMPAT(x) = { .size = sizeof(x) , .compat = x } struct { size_t size; const char *compat; } default_compat_types[] = { COMPAT("multiboot,linux-zimage\0multiboot,module") COMPAT("multiboot,linux-ramdisk\0multiboot,module") COMPAT("multiboot,module") } ...etc...
+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.
Like I said near the top I think this is a premature refactoring, in practice FDT is the only defined transport for multiboot on ARM.
I think at the very least you need to s/xen/fdt/ everywhere in this patch but IMHO you could fold it all into multiboot.c and leave the abstraction for the person implementing the (right now hypothetical) future transport mechanism.
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).
I think cmdline_name is not needed if you use /chosen/module@N/bootargs everywhere.
Although, the "reg" carries the load address info of module, but in different format that can be use directly in "prepare_*_params_xen".
I'd expect that this could be handled entirely as a conversion within that function, rather than carrying two copied of the addresses about.
Cheers, Ian.
On 06/05/2014 05:03 PM, Ian Campbell wrote:
On Thu, 2014-06-05 at 15:46 +0800, Fu Wei wrote:
On 06/05/2014 12:36 AM, Ian Campbell wrote:
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 :-)
Roy Franz from Linaro is working on this, he's making good progress, but I don't want to guess a deadline for him ;-).
np, just wait for the good news from him.
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.
The specification which we have defined here is that it will use device tree, something which uses another mechanism would not be following this spec. Even when Xen uses ACPI it will still receive a device tree as part of the (existing, Linux) boot protocol which includes the chosen node and hence the multiboot modules.
multiboot_fdt.c would be a better name than multiboot_xen.c, if that's what you intend the abstraction to be about, but TBH this seems like a premature refactoring to me. If/when someone invents up "multiboot over $SOMETHING_ELSE" then refactoring can be done then.
For multiboot_xen.c, it includes (1)the boot info injection into FDT for multi binaries(following xen specific boot binaries order) (2)the xen specific compatibility strings and other info
Although it includes some FDT-related code, it is not for fdt. It is for Xen.
For the cache syncing stuff, I will remove it once we have stub-xen.
OK.
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.
I have a prototype patch. I'll send it separately in a moment.
Got it from xen-devel mail list, Great thanks! Will use this patch to test my new multiboot patch!
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.
What I meant was that we could arrange that: multiboot /xen-4.1-amd64.gz [...] module /vmlinuz-3.10-3-amd64 [...]
Would produce a module with compatible = "multiboot,linux-zimage", "boot,module" But: multiboot --type=xen /xen-4.1-amd64.gz [...] module /vmlinuz-3.10-3-amd64 [...] would produce: compatible = "xen,linux-zimage", "multiboot,linux-zimage", "boot,module" i.e enable additional Xen compatibility strings. This would be an extension to the spec as described in the wiki which is why I described it as optional/would be nice. Feel free to ignore this suggested extension.
OK, you want to use "--type" in multiboot command to identify multiboot kernel. please update wiki page.
It could be a option, will try to implement it in next patch.
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.
Via some existing grub interfaces rather than duplicating the tail of the stub-linux code here, right?
Yes, just like How grub boot the stub-linux(aarch64) now.
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.
AFAIK the above change has no dependency on me finishing anything -- Xen already copes with #address-cells/#size-cells = 2 correctly, since we support platforms which only have RAM above 4GB already.
(If you find otherwise please report as a bug...)
OK, will try
[...]
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
So you can just have a static global in that file I think (or static accessors if that's a grub style thing).
Will try
+/************************************/
#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.
So these are flags on the kernel and not the modules?
For kernel and modules.
I think the kernel is inherently unique, so doesn't need a flag. For the modules I don't think it is grub's business to enforce uniqueness. If I specify multiboot /xen.gz module --type "multiboot,linux-zimage" --type "boot,module" /vmlinuz1 module --type "multiboot,linux-initrd" --type "boot,module" /initrd1 module --type "multiboot,linux-zimage" --type "boot,module" /vmlinuz2 then that's up to me and grub shouldn't second guess what I'm doing.
(this is the sort of thing a multiboot kernel might implement support for if it wanted to give the ability to create 1 domain created at boot time, I think it is within the spec to do so)
I put the kernel and modules in the same "named list". so the flag just a common item in the data structure.
Since Xen will use multiple zimage(or initrd/xsm), I will remove this flag from multiboot_xen.c.
I have enforced this in multiboot.c(line 340).
Please don't ;-)
+#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.
I don't think it is up to grub to enforce this sort of thing. If I want to give a command line to my initrd then that's my business, even if it might be meaningless to the kernel ... Including a command line for something which doesn't logically require one is fine -- it'll be ignored by the multiboot kernel.
I think you should remove both of these GRUB_MULTIBOOT_FLAGS_*. It'll make the code simpler and remove unnecessary restrictions which aren't defined by the spec.
I can remove these flags. The original thought for the flags is to avoid unnecessary process and potential faulty input. If I already know there is not cmdline for initrd/xsm, why I still put these meaningless strings into buffer?
+#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.
If we are agreed on this then I think we can just update the wiki. Eventually we should probably move this spec to somewhere more controlled and/or refrain from changing it.
yes, once you update the wiki or spec, I can update these immediately
Since in effect grub defines the other forms of multiboot anyway it's possible that there should be a separate documentation patch which adds the spec to the grub docs?
the definitions above will just affect Xen.
And it is easy to set here, right?
I think if we specify the module alignment you don't need to define it for each module type.
if Xen doc can specify a unique module alignment for all modules, we can just use XEN_MODULE_DEFAULT_ALIGN. But the definitions above do the same thing. :-)
In the future we could consider expanding the spec to support modules --alignment=XXX, but I don't think you need to do that now, unless you are keen to do so.
It's up to Xen project, once this is in the spec, I will make the code support this ASAP.
+#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. :-)
Please do, it is in no way deprecated, I don't know what Julien was talking about.
The order in which Xen will look at various nodes/properties for the bootargs for itself and/or the modules is well defined (see the list in booting.txt) and most definitely includes /chosen/module@XXX/bootargs as an option, exactly for multiboot implementations such as this.
Support for the other property names is there mainly to make it possible to write a generic u-boot environment which can be used with both Xen and native Linux boot.scr, but of course you don't care one jot about that here. Using /chosen/modules@XXX/bootargs is absolutely the right thing for you to be using in the context of multiboot.
If so, please check docs/misc/arm/device-tree/booting.txt (master branch of git://xenbits.xen.org/xen.git) ------------------- - bootargs (optional)
Command line associated with this module. This is deprecated and should be replaced by the bootargs variations described below. ------------------
I hope I did not misunderstand this. :-)
+#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.
You don't need an enum for this. Just do const char *default_compat_types[] = { "multiboot,linux-zimage", "multiboot,linux-ramdisk" } int default_type_nr = 0;
...
if ( there is a --type argument ) { module->type = strdup(the argument) } else { if ( default_type_nr > ARRAY_SIZE(default_compat_types) module->type = strdup("multiboot,module"); else module->type = strdupandcat(default_compat_types[default_type_nr++], "multiboot,module");
or something like that, (I'm sure my handling of "multiboot,module" is wrong, but you get the general idea).
or maybe: #define COMPAT(x) = { .size = sizeof(x) , .compat = x } struct { size_t size; const char *compat; } default_compat_types[] = { COMPAT("multiboot,linux-zimage\0multiboot,module") COMPAT("multiboot,linux-ramdisk\0multiboot,module") COMPAT("multiboot,module") } ...etc...
Thanks for the COMPAT(x) idea :-) will try in my new patch.
+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.
Like I said near the top I think this is a premature refactoring, in practice FDT is the only defined transport for multiboot on ARM.
I think at the very least you need to s/xen/fdt/ everywhere in this patch but IMHO you could fold it all into multiboot.c and leave the abstraction for the person implementing the (right now hypothetical) future transport mechanism.
When I submitted the patch, I felt this abstraction maybe overdone, too. will consider fold multiboot_xen.c into multiboot.c
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).
I think cmdline_name is not needed if you use /chosen/module@N/bootargs everywhere.
yes, you are right.
Although, the "reg" carries the load address info of module, but in different format that can be use directly in "prepare_*_params_xen".
I'd expect that this could be handled entirely as a conversion within that function, rather than carrying two copied of the addresses about.
will try :-)
Cheers, Ian.
On Fri, 2014-06-06 at 17:00 +0800, Fu Wei wrote:
multiboot_fdt.c would be a better name than multiboot_xen.c, if that's what you intend the abstraction to be about, but TBH this seems like a premature refactoring to me. If/when someone invents up "multiboot over $SOMETHING_ELSE" then refactoring can be done then.
For multiboot_xen.c, it includes (1)the boot info injection into FDT for multi binaries(following xen specific boot binaries order) (2)the xen specific compatibility strings and other info
The intention with http://wiki.xen.org/wiki/Xen_ARM_with_Virtualization_Extensions/Multiboot was that nothing there was really Xen specific. I think this is the case.
Although it includes some FDT-related code, it is not for fdt. It is for Xen.
Well, Xen is a consumer of this interface but the interface itself is supposed to be more widely useful than that.
Remember that this multiboot stuff is intended to be generic, even if Xen people are involved in defining it and Xen is currently the only user. I really want to avoid defining something which cannot be used by someone else if they want.
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.
What I meant was that we could arrange that: multiboot /xen-4.1-amd64.gz [...] module /vmlinuz-3.10-3-amd64 [...]
Would produce a module with compatible = "multiboot,linux-zimage", "boot,module" But: multiboot --type=xen /xen-4.1-amd64.gz [...] module /vmlinuz-3.10-3-amd64 [...] would produce: compatible = "xen,linux-zimage", "multiboot,linux-zimage", "boot,module" i.e enable additional Xen compatibility strings. This would be an extension to the spec as described in the wiki which is why I described it as optional/would be nice. Feel free to ignore this suggested extension.
OK, you want to use "--type" in multiboot command to identify multiboot kernel. please update wiki page.
That's not quite what I meant. So...
It could be a option, will try to implement it in next patch.
... I think this extension is just going to confuse things right now. Lets leave it out.
+#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.
I don't think it is up to grub to enforce this sort of thing. If I want to give a command line to my initrd then that's my business, even if it might be meaningless to the kernel ... Including a command line for something which doesn't logically require one is fine -- it'll be ignored by the multiboot kernel.
I think you should remove both of these GRUB_MULTIBOOT_FLAGS_*. It'll make the code simpler and remove unnecessary restrictions which aren't defined by the spec.
I can remove these flags. The original thought for the flags is to avoid unnecessary process and potential faulty input. If I already know there is not cmdline for initrd/xsm, why I still put these meaningless strings into buffer?
It might be unusual to pass a command line with an initrd/xsm but it's by no means completely implausible that a multiboot capable kernel might want to consume such a thing.
(In other words "because the spec says that any module can have a command line")
Since in effect grub defines the other forms of multiboot anyway it's possible that there should be a separate documentation patch which adds the spec to the grub docs?
the definitions above will just affect Xen.
Support for the other property names is there mainly to make it possible to write a generic u-boot environment which can be used with both Xen and native Linux boot.scr, but of course you don't care one jot about that here. Using /chosen/modules@XXX/bootargs is absolutely the right thing for you to be using in the context of multiboot.
If so, please check docs/misc/arm/device-tree/booting.txt (master branch of git://xenbits.xen.org/xen.git)
bootargs (optional)
Command line associated with this module. This is deprecated and should be replaced by the bootargs variations described below.
I hope I did not misunderstand this. :-)
This is wrong and confusing. Especially since the "variations described below" explicitly include this field as an option!
I shall send a patch to fix it.
Ian.
On 06/06/2014 07:10 PM, Ian Campbell wrote:
On Fri, 2014-06-06 at 17:00 +0800, Fu Wei wrote:
multiboot_fdt.c would be a better name than multiboot_xen.c, if that's what you intend the abstraction to be about, but TBH this seems like a premature refactoring to me. If/when someone invents up "multiboot over $SOMETHING_ELSE" then refactoring can be done then.
For multiboot_xen.c, it includes (1)the boot info injection into FDT for multi binaries(following xen specific boot binaries order) (2)the xen specific compatibility strings and other info
The intention with http://wiki.xen.org/wiki/Xen_ARM_with_Virtualization_Extensions/Multiboot was that nothing there was really Xen specific. I think this is the case.
Although it includes some FDT-related code, it is not for fdt. It is for Xen.
Well, Xen is a consumer of this interface but the interface itself is supposed to be more widely useful than that.
Remember that this multiboot stuff is intended to be generic, even if Xen people are involved in defining it and Xen is currently the only user. I really want to avoid defining something which cannot be used by someone else if they want.
For making this multiboot stuff to be generic(multiboot_fdt.c), I suggest that:
We avoid using xen specific boot binaries order, only use --type="multiboot,*" to define the compatibility strings of the module.
if the user did not provide --type="multiboot,*", we assume the compatibility string is "boot,module".
By this way, we can s/xen/fdt/ everywhere.
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.
What I meant was that we could arrange that: multiboot /xen-4.1-amd64.gz [...] module /vmlinuz-3.10-3-amd64 [...]
Would produce a module with compatible = "multiboot,linux-zimage", "boot,module" But: multiboot --type=xen /xen-4.1-amd64.gz [...] module /vmlinuz-3.10-3-amd64 [...] would produce: compatible = "xen,linux-zimage", "multiboot,linux-zimage", "boot,module" i.e enable additional Xen compatibility strings. This would be an extension to the spec as described in the wiki which is why I described it as optional/would be nice. Feel free to ignore this suggested extension.
OK, you want to use "--type" in multiboot command to identify multiboot kernel. please update wiki page.
That's not quite what I meant. So...
It could be a option, will try to implement it in next patch.
... I think this extension is just going to confuse things right now. Lets leave it out.
Agree, let's leave it out.
+#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.
I don't think it is up to grub to enforce this sort of thing. If I want to give a command line to my initrd then that's my business, even if it might be meaningless to the kernel ... Including a command line for something which doesn't logically require one is fine -- it'll be ignored by the multiboot kernel.
I think you should remove both of these GRUB_MULTIBOOT_FLAGS_*. It'll make the code simpler and remove unnecessary restrictions which aren't defined by the spec.
I can remove these flags. The original thought for the flags is to avoid unnecessary process and potential faulty input. If I already know there is not cmdline for initrd/xsm, why I still put these meaningless strings into buffer?
It might be unusual to pass a command line with an initrd/xsm but it's by no means completely implausible that a multiboot capable kernel might want to consume such a thing.
(In other words "because the spec says that any module can have a command line")
If so, will remove both of these GRUB_MULTIBOOT_FLAGS_*.
Since in effect grub defines the other forms of multiboot anyway it's possible that there should be a separate documentation patch which adds the spec to the grub docs?
the definitions above will just affect Xen.
Support for the other property names is there mainly to make it possible to write a generic u-boot environment which can be used with both Xen and native Linux boot.scr, but of course you don't care one jot about that here. Using /chosen/modules@XXX/bootargs is absolutely the right thing for you to be using in the context of multiboot.
If so, please check docs/misc/arm/device-tree/booting.txt (master branch of git://xenbits.xen.org/xen.git)
bootargs (optional)
Command line associated with this module. This is deprecated and should be replaced by the bootargs variations described below.
I hope I did not misunderstand this. :-)
This is wrong and confusing. Especially since the "variations described below" explicitly include this field as an option!
I shall send a patch to fix it.
Thanks for clarifying this, will always use "bootargs".
Ian.
On Fri, 2014-06-06 at 19:48 +0800, Fu Wei wrote:
On 06/06/2014 07:10 PM, Ian Campbell wrote:
On Fri, 2014-06-06 at 17:00 +0800, Fu Wei wrote:
multiboot_fdt.c would be a better name than multiboot_xen.c, if that's what you intend the abstraction to be about, but TBH this seems like a premature refactoring to me. If/when someone invents up "multiboot over $SOMETHING_ELSE" then refactoring can be done then.
For multiboot_xen.c, it includes (1)the boot info injection into FDT for multi binaries(following xen specific boot binaries order) (2)the xen specific compatibility strings and other info
The intention with http://wiki.xen.org/wiki/Xen_ARM_with_Virtualization_Extensions/Multiboot was that nothing there was really Xen specific. I think this is the case.
Although it includes some FDT-related code, it is not for fdt. It is for Xen.
Well, Xen is a consumer of this interface but the interface itself is supposed to be more widely useful than that.
Remember that this multiboot stuff is intended to be generic, even if Xen people are involved in defining it and Xen is currently the only user. I really want to avoid defining something which cannot be used by someone else if they want.
For making this multiboot stuff to be generic(multiboot_fdt.c), I suggest that:
We avoid using xen specific boot binaries order, only use --type="multiboot,*" to define the compatibility strings of the module.
if the user did not provide --type="multiboot,*", we assume the compatibility string is "boot,module".
I think we should only go halfway to what you propose here.
We should avoid the "xen,*" strings for sure, and use the "multiboot,*" names instead. But I think it is still OK to define a useful set of default compat strings based on the common case, i.e. first module is "multiboot,kernel" by default and second is "multiboot,ramdisk" in addition to the "multiboot,module" which is common to everything.
Dropping this defaulting behaviour would require increasing the scope of this work to cause update-grub on ARM to inject the correct --type arguments when creating the multiboot entries.
Ian.
On 06/06/2014 08:05 PM, Ian Campbell wrote:
On Fri, 2014-06-06 at 19:48 +0800, Fu Wei wrote:
On 06/06/2014 07:10 PM, Ian Campbell wrote:
On Fri, 2014-06-06 at 17:00 +0800, Fu Wei wrote:
multiboot_fdt.c would be a better name than multiboot_xen.c, if that's what you intend the abstraction to be about, but TBH this seems like a premature refactoring to me. If/when someone invents up "multiboot over $SOMETHING_ELSE" then refactoring can be done then.
For multiboot_xen.c, it includes (1)the boot info injection into FDT for multi binaries(following xen specific boot binaries order) (2)the xen specific compatibility strings and other info
The intention with http://wiki.xen.org/wiki/Xen_ARM_with_Virtualization_Extensions/Multiboot was that nothing there was really Xen specific. I think this is the case.
Although it includes some FDT-related code, it is not for fdt. It is for Xen.
Well, Xen is a consumer of this interface but the interface itself is supposed to be more widely useful than that.
Remember that this multiboot stuff is intended to be generic, even if Xen people are involved in defining it and Xen is currently the only user. I really want to avoid defining something which cannot be used by someone else if they want.
For making this multiboot stuff to be generic(multiboot_fdt.c), I suggest that:
We avoid using xen specific boot binaries order, only use --type="multiboot,*" to define the compatibility strings of the module.
if the user did not provide --type="multiboot,*", we assume the compatibility string is "boot,module".
I think we should only go halfway to what you propose here.
We should avoid the "xen,*" strings for sure, and use the "multiboot,*" names instead. But I think it is still OK to define a useful set of default compat strings based on the common case, i.e. first module is "multiboot,kernel" by default and second is "multiboot,ramdisk" in addition to the "multiboot,module" which is common to everything.
Agree! :-) Will do this way.
Dropping this defaulting behaviour would require increasing the scope of this work to cause update-grub on ARM to inject the correct --type arguments when creating the multiboot entries.
Ian.
On Thu, 2014-06-05 at 15:46 +0800, Fu Wei wrote:
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.
I've only compile tested this, but I *think* this does the right thing.
Ian.
8<-----------------
From d0acce53a086869420c2d8870d1a8a058013d6b5 Mon Sep 17 00:00:00 2001
From: Ian Campbell ian.campbell@citrix.com Date: Wed, 4 Jun 2014 18:17:10 +0100 Subject: [PATCH] xen: arm: implement generic multiboot compatibility strings
This causes Xen to accept the more generic names originally proposed by Andre in http://thread.gmane.org/gmane.linux.linaro.announce.boot/326 and incorporated into the proposal in http://wiki.xen.org/wiki/Xen_ARM_with_Virtualization_Extensions/Multiboot
This will allow bootloaders to not special case Xen (or at least to reduce the amount which is required).
Signed-off-by: Ian Campbell ian.campbell@citrix.com --- xen/common/device_tree.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c index f0b17a3..5040097 100644 --- a/xen/common/device_tree.c +++ b/xen/common/device_tree.c @@ -338,9 +338,11 @@ static void __init process_multiboot_node(const void *fdt, int node, struct dt_mb_module *mod; int len;
- if ( fdt_node_check_compatible(fdt, node, "xen,linux-zimage") == 0 ) + if ( fdt_node_check_compatible(fdt, node, "xen,linux-zimage") == 0 || + fdt_node_check_compatible(fdt, node, "multiboot,linux-zimage") == 0 ) nr = MOD_KERNEL; - else if ( fdt_node_check_compatible(fdt, node, "xen,linux-initrd") == 0) + else if ( fdt_node_check_compatible(fdt, node, "xen,linux-initrd") == 0 || + fdt_node_check_compatible(fdt, node, "multiboot,linux-initrd") == 0 ) nr = MOD_INITRD; else if ( fdt_node_check_compatible(fdt, node, "xen,xsm-policy") == 0 ) nr = MOD_XSM; @@ -433,7 +435,8 @@ static int __init early_scan_node(const void *fdt, { if ( device_tree_node_matches(fdt, node, "memory") ) process_memory_node(fdt, node, name, address_cells, size_cells); - else if ( device_tree_node_compatible(fdt, node, "xen,multiboot-module" ) ) + else if ( device_tree_node_compatible(fdt, node, "xen,multiboot-module" ) || + device_tree_node_compatible(fdt, node, "boot,module" )) process_multiboot_node(fdt, node, name, address_cells, size_cells); else if ( depth == 1 && device_tree_node_matches(fdt, node, "chosen") ) process_chosen_node(fdt, node, name, address_cells, size_cells);
Hi Ian,
On 06/05/2014 12:56 PM, Ian Campbell wrote:
On Thu, 2014-06-05 at 15:46 +0800, Fu Wei wrote:
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.
I've only compile tested this, but I *think* this does the right thing.
Ian.
8<-----------------
From d0acce53a086869420c2d8870d1a8a058013d6b5 Mon Sep 17 00:00:00 2001 From: Ian Campbell ian.campbell@citrix.com Date: Wed, 4 Jun 2014 18:17:10 +0100 Subject: [PATCH] xen: arm: implement generic multiboot compatibility strings
This causes Xen to accept the more generic names originally proposed by Andre in http://thread.gmane.org/gmane.linux.linaro.announce.boot/326 and incorporated into the proposal in http://wiki.xen.org/wiki/Xen_ARM_with_Virtualization_Extensions/Multiboot
This will allow bootloaders to not special case Xen (or at least to reduce the amount which is required).
Signed-off-by: Ian Campbell ian.campbell@citrix.com
xen/common/device_tree.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c index f0b17a3..5040097 100644 --- a/xen/common/device_tree.c +++ b/xen/common/device_tree.c @@ -338,9 +338,11 @@ static void __init process_multiboot_node(const void *fdt, int node, struct dt_mb_module *mod; int len;
- if ( fdt_node_check_compatible(fdt, node, "xen,linux-zimage") == 0 )
- if ( fdt_node_check_compatible(fdt, node, "xen,linux-zimage") == 0 ||
fdt_node_check_compatible(fdt, node, "multiboot,linux-zimage") == 0 )
While we are modifying the protocol, "linux-zImage" is confusing in the name. Actually we can use it for an ELF, another OS... I don't think Xen will change his behavior depending of the DOM0 image.
I would rename it to something more generic such as "multiboot,kernel". This will avoid adding a new compatible string every time we support a new format/OS.
Regards,
On Thu, 2014-06-05 at 17:36 +0100, Julien Grall wrote:
Hi Ian,
On 06/05/2014 12:56 PM, Ian Campbell wrote:
On Thu, 2014-06-05 at 15:46 +0800, Fu Wei wrote:
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.
I've only compile tested this, but I *think* this does the right thing.
Ian.
8<-----------------
From d0acce53a086869420c2d8870d1a8a058013d6b5 Mon Sep 17 00:00:00 2001 From: Ian Campbell ian.campbell@citrix.com Date: Wed, 4 Jun 2014 18:17:10 +0100 Subject: [PATCH] xen: arm: implement generic multiboot compatibility strings
This causes Xen to accept the more generic names originally proposed by Andre in http://thread.gmane.org/gmane.linux.linaro.announce.boot/326 and incorporated into the proposal in http://wiki.xen.org/wiki/Xen_ARM_with_Virtualization_Extensions/Multiboot
This will allow bootloaders to not special case Xen (or at least to reduce the amount which is required).
Signed-off-by: Ian Campbell ian.campbell@citrix.com
xen/common/device_tree.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c index f0b17a3..5040097 100644 --- a/xen/common/device_tree.c +++ b/xen/common/device_tree.c @@ -338,9 +338,11 @@ static void __init process_multiboot_node(const void *fdt, int node, struct dt_mb_module *mod; int len;
- if ( fdt_node_check_compatible(fdt, node, "xen,linux-zimage") == 0 )
- if ( fdt_node_check_compatible(fdt, node, "xen,linux-zimage") == 0 ||
fdt_node_check_compatible(fdt, node, "multiboot,linux-zimage") == 0 )While we are modifying the protocol, "linux-zImage" is confusing in the name. Actually we can use it for an ELF, another OS... I don't think Xen will change his behavior depending of the DOM0 image.
zImage defines the boot protocol to use. Since the protocol is defined by Linux as zImage I think that is the appropriate name, if some other OS wants to mimic Linux then fine. But if we end up supporting some OS with its own boot protocol which doesn't match Linux's then that should have a distinct name of its own.
The "ELF boot protocol" is as yet undefined. It may or may not end up resembling zImage. In any case it will be a standard made up entirely by us and as such xen-elf or something would be the correct name (unless it happens to match zImage).
I would rename it to something more generic such as "multiboot,kernel". This will avoid adding a new compatible string every time we support a new format/OS.
That implies that there is some sort of standard kernel boot interface, which is not the case, the protocol is defined as linux-zimage.
Perhaps there is scope for a generic compat name which probes based on magic numbers. You are free to propose and implement such a thing if you like, but it would be inappropriate to use that with the current implementation I think.
Ian.
On 06/05/2014 05:55 PM, Ian Campbell wrote:
On Thu, 2014-06-05 at 17:36 +0100, Julien Grall wrote:
Hi Ian,
On 06/05/2014 12:56 PM, Ian Campbell wrote:
On Thu, 2014-06-05 at 15:46 +0800, Fu Wei wrote:
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.
I've only compile tested this, but I *think* this does the right thing.
Ian.
8<-----------------
From d0acce53a086869420c2d8870d1a8a058013d6b5 Mon Sep 17 00:00:00 2001 From: Ian Campbell ian.campbell@citrix.com Date: Wed, 4 Jun 2014 18:17:10 +0100 Subject: [PATCH] xen: arm: implement generic multiboot compatibility strings
This causes Xen to accept the more generic names originally proposed by Andre in http://thread.gmane.org/gmane.linux.linaro.announce.boot/326 and incorporated into the proposal in http://wiki.xen.org/wiki/Xen_ARM_with_Virtualization_Extensions/Multiboot
This will allow bootloaders to not special case Xen (or at least to reduce the amount which is required).
Signed-off-by: Ian Campbell ian.campbell@citrix.com
xen/common/device_tree.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c index f0b17a3..5040097 100644 --- a/xen/common/device_tree.c +++ b/xen/common/device_tree.c @@ -338,9 +338,11 @@ static void __init process_multiboot_node(const void *fdt, int node, struct dt_mb_module *mod; int len;
- if ( fdt_node_check_compatible(fdt, node, "xen,linux-zimage") == 0 )
- if ( fdt_node_check_compatible(fdt, node, "xen,linux-zimage") == 0 ||
fdt_node_check_compatible(fdt, node, "multiboot,linux-zimage") == 0 )While we are modifying the protocol, "linux-zImage" is confusing in the name. Actually we can use it for an ELF, another OS... I don't think Xen will change his behavior depending of the DOM0 image.
zImage defines the boot protocol to use. Since the protocol is defined by Linux as zImage I think that is the appropriate name, if some other OS wants to mimic Linux then fine. But if we end up supporting some OS with its own boot protocol which doesn't match Linux's then that should have a distinct name of its own.
zImage is the boot protocol for arm32, this is different for arm64. Here we are talking about the protocol for dom0, why the bootloader is involved in it?
How the interface will be exposed by grub to the user? Is it the "multiboot ..." line like in x86? If so, how grub will choose what is the boot protocol for the guest?
The "ELF boot protocol" is as yet undefined. It may or may not end up resembling zImage. In any case it will be a standard made up entirely by us and as such xen-elf or something would be the correct name (unless it happens to match zImage).
IHMO, the bootloader should only give the kernel blob to the hypervisor. Then it will decide what is the best protocol to use for booting dom0.
Regards,
On Thu, 2014-06-05 at 18:03 +0100, Julien Grall wrote:
On 06/05/2014 05:55 PM, Ian Campbell wrote:
On Thu, 2014-06-05 at 17:36 +0100, Julien Grall wrote:
Hi Ian,
On 06/05/2014 12:56 PM, Ian Campbell wrote:
On Thu, 2014-06-05 at 15:46 +0800, Fu Wei wrote:
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.
I've only compile tested this, but I *think* this does the right thing.
Ian.
8<-----------------
From d0acce53a086869420c2d8870d1a8a058013d6b5 Mon Sep 17 00:00:00 2001 From: Ian Campbell ian.campbell@citrix.com Date: Wed, 4 Jun 2014 18:17:10 +0100 Subject: [PATCH] xen: arm: implement generic multiboot compatibility strings
This causes Xen to accept the more generic names originally proposed by Andre in http://thread.gmane.org/gmane.linux.linaro.announce.boot/326 and incorporated into the proposal in http://wiki.xen.org/wiki/Xen_ARM_with_Virtualization_Extensions/Multiboot
This will allow bootloaders to not special case Xen (or at least to reduce the amount which is required).
Signed-off-by: Ian Campbell ian.campbell@citrix.com
xen/common/device_tree.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c index f0b17a3..5040097 100644 --- a/xen/common/device_tree.c +++ b/xen/common/device_tree.c @@ -338,9 +338,11 @@ static void __init process_multiboot_node(const void *fdt, int node, struct dt_mb_module *mod; int len;
- if ( fdt_node_check_compatible(fdt, node, "xen,linux-zimage") == 0 )
- if ( fdt_node_check_compatible(fdt, node, "xen,linux-zimage") == 0 ||
fdt_node_check_compatible(fdt, node, "multiboot,linux-zimage") == 0 )While we are modifying the protocol, "linux-zImage" is confusing in the name. Actually we can use it for an ELF, another OS... I don't think Xen will change his behavior depending of the DOM0 image.
zImage defines the boot protocol to use. Since the protocol is defined by Linux as zImage I think that is the appropriate name, if some other OS wants to mimic Linux then fine. But if we end up supporting some OS with its own boot protocol which doesn't match Linux's then that should have a distinct name of its own.
zImage is the boot protocol for arm32, this is different for arm64.
They are essentially the same thing, modulo arch differences. It doesn't seem worth defining linux-image as well as linux-zimage.
Here we are talking about the protocol for dom0, why the bootloader is involved in it?
Ultimately the dom0 kernel is specified, by the user, in the bootloader configuration file.
How the interface will be exposed by grub to the user? Is it the "multiboot ..." line like in x86? If so, how grub will choose what is the boot protocol for the guest?
This is documented in the wiki. The grub syntax has defaults, but it also supports explicit specification of the type.
The "ELF boot protocol" is as yet undefined. It may or may not end up resembling zImage. In any case it will be a standard made up entirely by us and as such xen-elf or something would be the correct name (unless it happens to match zImage).
IHMO, the bootloader should only give the kernel blob to the hypervisor. Then it will decide what is the best protocol to use for booting dom0.
That might be nice, but is not how things are defined to work right now.
The fact is that this patch is a minor change which makes the existing scheme less Xen specific and allows the grub implementation of multiboot to not be to Xen specific.
If you want to make larger changes to the boot protocol then nothing being proposed here stops you doing that in the future, we aren't painting ourselves into any more corners than we are in without it.
Ian.
On Thu, 2014-06-05 at 18:03 +0100, Julien Grall wrote:
While we are modifying the protocol, "linux-zImage" is confusing in the name. Actually we can use it for an ELF, another OS... I don't think Xen will change his behavior depending of the DOM0 image.
Actually thinking about this some more I think you are right. Xen already probes the kernel it gets so we can safely implement this as multiboot,kernel, since we don't really need the more specific type. If in the future some non-probable kernel comes along which we want to support we still have the option of adding more specific compatibility strings.
Fu Wei -- if this is OK with you I will modify the wiki page to s/multiboot,linux-zimage/multiboot,kernel/ and rev this patch to suit.
Can we do something similar with linux-ramdisk? I'm not sure since we cannot easily probe the ramdisk contents. We could base the ramdisk behaviour on the probed behaviour of the kernel. Anyone got any thoughts?
Ian.
On 06/05/2014 07:31 PM, Ian Campbell wrote:
On Thu, 2014-06-05 at 18:03 +0100, Julien Grall wrote:
While we are modifying the protocol, "linux-zImage" is confusing in the name. Actually we can use it for an ELF, another OS... I don't think Xen will change his behavior depending of the DOM0 image.
Actually thinking about this some more I think you are right. Xen already probes the kernel it gets so we can safely implement this as multiboot,kernel, since we don't really need the more specific type. If in the future some non-probable kernel comes along which we want to support we still have the option of adding more specific compatibility strings.
Fu Wei -- if this is OK with you I will modify the wiki page to s/multiboot,linux-zimage/multiboot,kernel/ and rev this patch to suit.
Can we do something similar with linux-ramdisk? I'm not sure since we cannot easily probe the ramdisk contents. We could base the ramdisk behaviour on the probed behaviour of the kernel. Anyone got any thoughts?
I have only check FreeBSD, and they don't have any bindings for the ramdisk for now. It seems they use the command line for this purpose.
Probing the ramdisk won't help here because the magic and the underlying filesystem might be the same.
I was about to say, we should do add a "multiboot,ramdisk" (or another name) but we have to add the linux,initrd-* foo in the device tree.
In another hand keeping the actual properties with properties from another ramdisk protocol won't harm here. Each kernel will deal with the property it would like to use.
Regards,
On Thu, 2014-06-05 at 22:00 +0100, Julien Grall wrote:
On 06/05/2014 07:31 PM, Ian Campbell wrote:
On Thu, 2014-06-05 at 18:03 +0100, Julien Grall wrote:
While we are modifying the protocol, "linux-zImage" is confusing in the name. Actually we can use it for an ELF, another OS... I don't think Xen will change his behavior depending of the DOM0 image.
Actually thinking about this some more I think you are right. Xen already probes the kernel it gets so we can safely implement this as multiboot,kernel, since we don't really need the more specific type. If in the future some non-probable kernel comes along which we want to support we still have the option of adding more specific compatibility strings.
Fu Wei -- if this is OK with you I will modify the wiki page to s/multiboot,linux-zimage/multiboot,kernel/ and rev this patch to suit.
Can we do something similar with linux-ramdisk? I'm not sure since we cannot easily probe the ramdisk contents. We could base the ramdisk behaviour on the probed behaviour of the kernel. Anyone got any thoughts?
I have only check FreeBSD, and they don't have any bindings for the ramdisk for now. It seems they use the command line for this purpose.
Probing the ramdisk won't help here because the magic and the underlying filesystem might be the same.
I was about to say, we should do add a "multiboot,ramdisk" (or another name) but we have to add the linux,initrd-* foo in the device tree.
In another hand keeping the actual properties with properties from another ramdisk protocol won't harm here. Each kernel will deal with the property it would like to use.
Having thought about this I think the way I see it is that the module contains a ramdisk and that is what should be described by the compatibility string. The method by which this thing is then passed down to the kernel is actually a function of the kernel in question, which we have decided can be probed for. Something which is mimicking the Linux arm/zImage or arm64/Image format can be expected to be mimicking the Linux initrd protocol as well IMHO.
So I therefore intend to update the wiki with "multiboot,kernel" and "multiboot,ramdisk" in place of "multiboot,linux-zimage" and "multiboot,linux-ramdisk" respectively.
I think "boot,module" should also be "multiboot,module" for consistency. I intend to make that substitution as well.
I will send out an updated Xen patch with those renamings in place.
Fu Wei, I'm very aware that we are redrafting this under your feet. I'm sorry about this. I think what is being proposed here amounts to changing a few string constants on your end, but if you have any concerns at all please shout at me.
I was also planning to clarify the introduction to the wiki page to make it clearer that the spec is intended to be generic in nature. I don't think this should make any difference to the implementation, but again do shout at me.
Ian.
On 06/06/2014 07:47 PM, Ian Campbell wrote:
On Thu, 2014-06-05 at 22:00 +0100, Julien Grall wrote:
On 06/05/2014 07:31 PM, Ian Campbell wrote:
On Thu, 2014-06-05 at 18:03 +0100, Julien Grall wrote:
While we are modifying the protocol, "linux-zImage" is confusing in the name. Actually we can use it for an ELF, another OS... I don't think Xen will change his behavior depending of the DOM0 image.
Actually thinking about this some more I think you are right. Xen already probes the kernel it gets so we can safely implement this as multiboot,kernel, since we don't really need the more specific type. If in the future some non-probable kernel comes along which we want to support we still have the option of adding more specific compatibility strings.
Fu Wei -- if this is OK with you I will modify the wiki page to s/multiboot,linux-zimage/multiboot,kernel/ and rev this patch to suit.
Can we do something similar with linux-ramdisk? I'm not sure since we cannot easily probe the ramdisk contents. We could base the ramdisk behaviour on the probed behaviour of the kernel. Anyone got any thoughts?
I have only check FreeBSD, and they don't have any bindings for the ramdisk for now. It seems they use the command line for this purpose.
Probing the ramdisk won't help here because the magic and the underlying filesystem might be the same.
I was about to say, we should do add a "multiboot,ramdisk" (or another name) but we have to add the linux,initrd-* foo in the device tree.
In another hand keeping the actual properties with properties from another ramdisk protocol won't harm here. Each kernel will deal with the property it would like to use.
Having thought about this I think the way I see it is that the module contains a ramdisk and that is what should be described by the compatibility string. The method by which this thing is then passed down to the kernel is actually a function of the kernel in question, which we have decided can be probed for. Something which is mimicking the Linux arm/zImage or arm64/Image format can be expected to be mimicking the Linux initrd protocol as well IMHO.
So I therefore intend to update the wiki with "multiboot,kernel" and "multiboot,ramdisk" in place of "multiboot,linux-zimage" and "multiboot,linux-ramdisk" respectively.
I think "boot,module" should also be "multiboot,module" for consistency. I intend to make that substitution as well.
I will send out an updated Xen patch with those renamings in place.
Fu Wei, I'm very aware that we are redrafting this under your feet. I'm sorry about this. I think what is being proposed here amounts to changing a few string constants on your end, but if you have any concerns at all please shout at me.
I was also planning to clarify the introduction to the wiki page to make it clearer that the spec is intended to be generic in nature. I don't think this should make any difference to the implementation, but again do shout at me.
Sorry for late response, happy to see the code become more generic.
And your modification for the wiki page is good for me , will follow it to update my patch! :-)
Thanks
Ian.
On 06/06/2014 02:31 AM, Ian Campbell wrote:
On Thu, 2014-06-05 at 18:03 +0100, Julien Grall wrote:
While we are modifying the protocol, "linux-zImage" is confusing in the name. Actually we can use it for an ELF, another OS... I don't think Xen will change his behavior depending of the DOM0 image.
Actually thinking about this some more I think you are right. Xen already probes the kernel it gets so we can safely implement this as multiboot,kernel, since we don't really need the more specific type. If in the future some non-probable kernel comes along which we want to support we still have the option of adding more specific compatibility strings.
Fu Wei -- if this is OK with you I will modify the wiki page to s/multiboot,linux-zimage/multiboot,kernel/ and rev this patch to suit.
This is OK for me, And I think the "multiboot,kernel" is better and more generic. :-)
Can we do something similar with linux-ramdisk? I'm not sure since we cannot easily probe the ramdisk contents. We could base the ramdisk behaviour on the probed behaviour of the kernel. Anyone got any thoughts?
My thought looks exactly the same as yours : The cpio utility can detect the cpio file format. Maybe we can just probe the file, see if this is a cpio or cpio.gz.
Ian.
On 06/06/2014 01:24 PM, Fu Wei wrote:
On 06/06/2014 02:31 AM, Ian Campbell wrote:
On Thu, 2014-06-05 at 18:03 +0100, Julien Grall wrote:
While we are modifying the protocol, "linux-zImage" is confusing in the name. Actually we can use it for an ELF, another OS... I don't think Xen will change his behavior depending of the DOM0 image.
Actually thinking about this some more I think you are right. Xen already probes the kernel it gets so we can safely implement this as multiboot,kernel, since we don't really need the more specific type. If in the future some non-probable kernel comes along which we want to support we still have the option of adding more specific compatibility strings.
Fu Wei -- if this is OK with you I will modify the wiki page to s/multiboot,linux-zimage/multiboot,kernel/ and rev this patch to suit.
This is OK for me, And I think the "multiboot,kernel" is better and more generic. :-)
Can we do something similar with linux-ramdisk? I'm not sure since we cannot easily probe the ramdisk contents. We could base the ramdisk behaviour on the probed behaviour of the kernel. Anyone got any thoughts?
My thought looks exactly the same as yours : The cpio utility can detect the cpio file format. Maybe we can just probe the file, see if this is a cpio or cpio.gz.
cpio is not Linux specific. Probing just the file won't help here to to determine if we have to add the properties linux,initrd-* or another set.
Regards,
On Fri, 2014-06-06 at 13:28 +0100, Julien Grall wrote:
On 06/06/2014 01:24 PM, Fu Wei wrote:
On 06/06/2014 02:31 AM, Ian Campbell wrote:
On Thu, 2014-06-05 at 18:03 +0100, Julien Grall wrote:
While we are modifying the protocol, "linux-zImage" is confusing in the name. Actually we can use it for an ELF, another OS... I don't think Xen will change his behavior depending of the DOM0 image.
Actually thinking about this some more I think you are right. Xen already probes the kernel it gets so we can safely implement this as multiboot,kernel, since we don't really need the more specific type. If in the future some non-probable kernel comes along which we want to support we still have the option of adding more specific compatibility strings.
Fu Wei -- if this is OK with you I will modify the wiki page to s/multiboot,linux-zimage/multiboot,kernel/ and rev this patch to suit.
This is OK for me, And I think the "multiboot,kernel" is better and more generic. :-)
Can we do something similar with linux-ramdisk? I'm not sure since we cannot easily probe the ramdisk contents. We could base the ramdisk behaviour on the probed behaviour of the kernel. Anyone got any thoughts?
My thought looks exactly the same as yours : The cpio utility can detect the cpio file format. Maybe we can just probe the file, see if this is a cpio or cpio.gz.
cpio is not Linux specific. Probing just the file won't help here to to determine if we have to add the properties linux,initrd-* or another set.
This can/should be determined by probing the type of kernel which the initrd is being passed to, it is not a property of the initrd.
Ian.
On 06/06/2014 01:32 PM, Ian Campbell wrote:
On Fri, 2014-06-06 at 13:28 +0100, Julien Grall wrote:
On 06/06/2014 01:24 PM, Fu Wei wrote:
On 06/06/2014 02:31 AM, Ian Campbell wrote:
On Thu, 2014-06-05 at 18:03 +0100, Julien Grall wrote:
> While we are modifying the protocol, "linux-zImage" is confusing in the > name. Actually we can use it for an ELF, another OS... I don't think Xen > will change his behavior depending of the DOM0 image.
Actually thinking about this some more I think you are right. Xen already probes the kernel it gets so we can safely implement this as multiboot,kernel, since we don't really need the more specific type. If in the future some non-probable kernel comes along which we want to support we still have the option of adding more specific compatibility strings.
Fu Wei -- if this is OK with you I will modify the wiki page to s/multiboot,linux-zimage/multiboot,kernel/ and rev this patch to suit.
This is OK for me, And I think the "multiboot,kernel" is better and more generic. :-)
Can we do something similar with linux-ramdisk? I'm not sure since we cannot easily probe the ramdisk contents. We could base the ramdisk behaviour on the probed behaviour of the kernel. Anyone got any thoughts?
My thought looks exactly the same as yours : The cpio utility can detect the cpio file format. Maybe we can just probe the file, see if this is a cpio or cpio.gz.
cpio is not Linux specific. Probing just the file won't help here to to determine if we have to add the properties linux,initrd-* or another set.
This can/should be determined by probing the type of kernel which the initrd is being passed to, it is not a property of the initrd.
I don't find anything documentation in Linux tree that claim the linux,initrd-* properties is part of the zImage protocol.
FYI, the device tree documentation in Linux (Documentation/devicetree/usage-model.txt) is talking about initrd-{start,end}, not linux,initrd-{start,end}.
If we plan to assume that the zImage always go with linux,initrd* properties then we have to document this specification somewhere.
Regards,
On Fri, 2014-06-06 at 14:25 +0100, Julien Grall wrote:
On 06/06/2014 01:32 PM, Ian Campbell wrote:
On Fri, 2014-06-06 at 13:28 +0100, Julien Grall wrote:
On 06/06/2014 01:24 PM, Fu Wei wrote:
On 06/06/2014 02:31 AM, Ian Campbell wrote:
On Thu, 2014-06-05 at 18:03 +0100, Julien Grall wrote:
>> While we are modifying the protocol, "linux-zImage" is confusing in the >> name. Actually we can use it for an ELF, another OS... I don't think Xen >> will change his behavior depending of the DOM0 image.
Actually thinking about this some more I think you are right. Xen already probes the kernel it gets so we can safely implement this as multiboot,kernel, since we don't really need the more specific type. If in the future some non-probable kernel comes along which we want to support we still have the option of adding more specific compatibility strings.
Fu Wei -- if this is OK with you I will modify the wiki page to s/multiboot,linux-zimage/multiboot,kernel/ and rev this patch to suit.
This is OK for me, And I think the "multiboot,kernel" is better and more generic. :-)
Can we do something similar with linux-ramdisk? I'm not sure since we cannot easily probe the ramdisk contents. We could base the ramdisk behaviour on the probed behaviour of the kernel. Anyone got any thoughts?
My thought looks exactly the same as yours : The cpio utility can detect the cpio file format. Maybe we can just probe the file, see if this is a cpio or cpio.gz.
cpio is not Linux specific. Probing just the file won't help here to to determine if we have to add the properties linux,initrd-* or another set.
This can/should be determined by probing the type of kernel which the initrd is being passed to, it is not a property of the initrd.
I don't find anything documentation in Linux tree that claim the linux,initrd-* properties is part of the zImage protocol.
FYI, the device tree documentation in Linux (Documentation/devicetree/usage-model.txt) is talking about initrd-{start,end}, not linux,initrd-{start,end}.
If we plan to assume that the zImage always go with linux,initrd* properties then we have to document this specification somewhere.
If something is going to pretend to be Linux (by pretending to be a Linux zImage) then it should behave like Linux on boot. I think this extends to initrd handling.
If what Linux does is undocumented (or the docs are out of step with reality), well, that's too bad.
Feel free to work on improving Linux's documentation. I think it is orthogonal to the questions raised in this thread though.
Ian.
On 06/06/14 15:24, Ian Campbell wrote:
If we plan to assume that the zImage always go with linux,initrd* properties then we have to document this specification somewhere.
If something is going to pretend to be Linux (by pretending to be a Linux zImage) then it should behave like Linux on boot. I think this extends to initrd handling.
If what Linux does is undocumented (or the docs are out of step with reality), well, that's too bad.
Feel free to work on improving Linux's documentation. I think it is orthogonal to the questions raised in this thread though.
Thanks I will try to send a patch for the Linux documentation to clarify it.
On Thu, Jun 05, 2014 at 05:55:31PM +0100, Ian Campbell wrote:
- if ( fdt_node_check_compatible(fdt, node, "xen,linux-zimage") == 0 )
- if ( fdt_node_check_compatible(fdt, node, "xen,linux-zimage") == 0 ||
fdt_node_check_compatible(fdt, node, "multiboot,linux-zimage") == 0 )While we are modifying the protocol, "linux-zImage" is confusing in the name. Actually we can use it for an ELF, another OS... I don't think Xen will change his behavior depending of the DOM0 image.
zImage defines the boot protocol to use. Since the protocol is defined by Linux as zImage I think that is the appropriate name, if some other OS wants to mimic Linux then fine. But if we end up supporting some OS with its own boot protocol which doesn't match Linux's then that should have a distinct name of its own.
Actually, there is no zImage support in arm64 - only Image.
/ Leif
On Thu, 2014-06-05 at 18:05 +0100, Leif Lindholm wrote:
On Thu, Jun 05, 2014 at 05:55:31PM +0100, Ian Campbell wrote:
- if ( fdt_node_check_compatible(fdt, node, "xen,linux-zimage") == 0 )
- if ( fdt_node_check_compatible(fdt, node, "xen,linux-zimage") == 0 ||
fdt_node_check_compatible(fdt, node, "multiboot,linux-zimage") == 0 )While we are modifying the protocol, "linux-zImage" is confusing in the name. Actually we can use it for an ELF, another OS... I don't think Xen will change his behavior depending of the DOM0 image.
zImage defines the boot protocol to use. Since the protocol is defined by Linux as zImage I think that is the appropriate name, if some other OS wants to mimic Linux then fine. But if we end up supporting some OS with its own boot protocol which doesn't match Linux's then that should have a distinct name of its own.
Actually, there is no zImage support in arm64 - only Image.
Right, that's a slight wrinkle in the naming. arm64's image is essentially equivalent (from a calling into it PoV) to zImage, it doesn't seem worth dupping the name here.
Ian.
On Wed, 2014-05-07 at 19:58 +0800, Fu Wei wrote:
+static grub_err_t +set_xen_module_type(multiboot_module_t module, int *argc, char **argv[]) +{ [...]
- } else {
node_info->type = module_type++;if (module_type > XEN_MODULE_OTHER)module_type = XEN_MODULE_OTHER;
This will, I think, cause the third and subsequent non-explicitly typed modules to automatically get assigned XEN_MODULE_XSM etc, which I think is not intended/expected by http://wiki.xen.org/wiki/Xen_ARM_with_Virtualization_Extensions/Multiboot. The only things which should be automatically assigned are the kernel and initrd, anything else should just get the fallback compatibility string unless one is explicitly requested.
Ian.
On 06/05/2014 12:53 AM, Ian Campbell wrote:
On Wed, 2014-05-07 at 19:58 +0800, Fu Wei wrote:
+static grub_err_t +set_xen_module_type(multiboot_module_t module, int *argc, char **argv[]) +{ [...]
- } else {
node_info->type = module_type++;if (module_type > XEN_MODULE_OTHER)module_type = XEN_MODULE_OTHER;This will, I think, cause the third and subsequent non-explicitly typed modules to automatically get assigned XEN_MODULE_XSM etc, which I think is not intended/expected by http://wiki.xen.org/wiki/Xen_ARM_with_Virtualization_Extensions/Multiboot. The only things which should be automatically assigned are the kernel and initrd, anything else should just get the fallback compatibility string unless one is explicitly requested.
Actually, the code is for "giving a default compatibility property based on the order". it is described in http://wiki.xen.org/wiki/Xen_ARM_with_Virtualization_Extensions/Multiboot#Co.... I hope my understanding is correct. :-)
Perhaps I misunderstood something, please correct me :-)
Ian.
(switching to my citrix address which I forgot to do for this subthread)
On Thu, 2014-06-05 at 16:16 +0800, Fu Wei wrote:
On 06/05/2014 12:53 AM, Ian Campbell wrote:
On Wed, 2014-05-07 at 19:58 +0800, Fu Wei wrote:
+static grub_err_t +set_xen_module_type(multiboot_module_t module, int *argc, char **argv[]) +{ [...]
- } else {
node_info->type = module_type++;if (module_type > XEN_MODULE_OTHER)module_type = XEN_MODULE_OTHER;This will, I think, cause the third and subsequent non-explicitly typed modules to automatically get assigned XEN_MODULE_XSM etc, which I think is not intended/expected by http://wiki.xen.org/wiki/Xen_ARM_with_Virtualization_Extensions/Multiboot. The only things which should be automatically assigned are the kernel and initrd, anything else should just get the fallback compatibility string unless one is explicitly requested.
Actually, the code is for "giving a default compatibility property based on the order". it is described in http://wiki.xen.org/wiki/Xen_ARM_with_Virtualization_Extensions/Multiboot#Co.... I hope my understanding is correct. :-)
Perhaps I misunderstood something, please correct me :-)
Your code will assign additional defaults over and above what is described there. The spec says:
The first module specified in this way will be:
compatible = "multiboot,linux-zimage", "boot,module"
The second module specified in this way will be:
compatible = "multiboot,linux-ramdisk", "boot,module"
All subsequent modules will be:
compatible = "boot,module"
You implementation effectively adds: The third module specified in this way will be:
compatible = "xen,xsm-policy", "xen,multiboot-module"
Which I think is undesirable.
Ian.
On 06/05/2014 04:20 PM, Ian Campbell wrote:
(switching to my citrix address which I forgot to do for this subthread)
On Thu, 2014-06-05 at 16:16 +0800, Fu Wei wrote:
On 06/05/2014 12:53 AM, Ian Campbell wrote:
On Wed, 2014-05-07 at 19:58 +0800, Fu Wei wrote:
+static grub_err_t +set_xen_module_type(multiboot_module_t module, int *argc, char **argv[]) +{ [...]
- } else {
node_info->type = module_type++;if (module_type > XEN_MODULE_OTHER)module_type = XEN_MODULE_OTHER;This will, I think, cause the third and subsequent non-explicitly typed modules to automatically get assigned XEN_MODULE_XSM etc, which I think is not intended/expected by http://wiki.xen.org/wiki/Xen_ARM_with_Virtualization_Extensions/Multiboot. The only things which should be automatically assigned are the kernel and initrd, anything else should just get the fallback compatibility string unless one is explicitly requested.
Actually, the code is for "giving a default compatibility property based on the order". it is described in http://wiki.xen.org/wiki/Xen_ARM_with_Virtualization_Extensions/Multiboot#Co.... I hope my understanding is correct. :-)
Perhaps I misunderstood something, please correct me :-)
Your code will assign additional defaults over and above what is described there. The spec says:
The first module specified in this way will be: compatible = "multiboot,linux-zimage", "boot,module" The second module specified in this way will be: compatible = "multiboot,linux-ramdisk", "boot,module" All subsequent modules will be: compatible = "boot,module"You implementation effectively adds: The third module specified in this way will be: compatible = "xen,xsm-policy", "xen,multiboot-module"
Which I think is undesirable.
Yes, I added a "xen,xsm-policy" after comparing wiki page with xen/docs/misc/arm/device-tree/booting.txt, I thought the wiki page is dated.
what I got from wiki page is we have two ways to decide the type of module: (1) by order (2) by "--type"
if "xen,xsm-policy" will be only loaded by "--type" way, I will improve the code ASAP.
Actually, what I should do is just following the xen multiboot protocol that decided by Xen Project. :-)
Please let me know : should I delete "xen,xsm-policy" from the "(1) by order"?
Thanks a lot! :-)
Ian.
On Thu, 2014-06-05 at 16:40 +0800, Fu Wei wrote:
On 06/05/2014 04:20 PM, Ian Campbell wrote:
(switching to my citrix address which I forgot to do for this subthread)
On Thu, 2014-06-05 at 16:16 +0800, Fu Wei wrote:
On 06/05/2014 12:53 AM, Ian Campbell wrote:
On Wed, 2014-05-07 at 19:58 +0800, Fu Wei wrote:
+static grub_err_t +set_xen_module_type(multiboot_module_t module, int *argc, char **argv[]) +{ [...]
- } else {
node_info->type = module_type++;if (module_type > XEN_MODULE_OTHER)module_type = XEN_MODULE_OTHER;This will, I think, cause the third and subsequent non-explicitly typed modules to automatically get assigned XEN_MODULE_XSM etc, which I think is not intended/expected by http://wiki.xen.org/wiki/Xen_ARM_with_Virtualization_Extensions/Multiboot. The only things which should be automatically assigned are the kernel and initrd, anything else should just get the fallback compatibility string unless one is explicitly requested.
Actually, the code is for "giving a default compatibility property based on the order". it is described in http://wiki.xen.org/wiki/Xen_ARM_with_Virtualization_Extensions/Multiboot#Co.... I hope my understanding is correct. :-)
Perhaps I misunderstood something, please correct me :-)
Your code will assign additional defaults over and above what is described there. The spec says:
The first module specified in this way will be: compatible = "multiboot,linux-zimage", "boot,module" The second module specified in this way will be: compatible = "multiboot,linux-ramdisk", "boot,module" All subsequent modules will be: compatible = "boot,module"You implementation effectively adds: The third module specified in this way will be: compatible = "xen,xsm-policy", "xen,multiboot-module"
Which I think is undesirable.
Yes, I added a "xen,xsm-policy" after comparing wiki page with xen/docs/misc/arm/device-tree/booting.txt, I thought the wiki page is dated.
what I got from wiki page is we have two ways to decide the type of module: (1) by order (2) by "--type"
if "xen,xsm-policy" will be only loaded by "--type" way, I will improve the code ASAP. Actually, what I should do is just following the xen multiboot protocol that decided by Xen Project. :-)
Please let me know : should I delete "xen,xsm-policy" from the "(1) by order"?
Yes, I think so. The spec is intended to be generic and XSM doesn't really fit into that. I think update-grub (which the spec is supposed to be remaining syntax compatible with) won't ever generate an XSM module line anyway so people will already need to edit it manually.
(Aside: it's possible that xsm,xsm-policy as a specific compat value was a mistake and Xen should be probing for the magic number like X86 does, but you need not worry about that)
Ian.
On 06/05/2014 05:10 PM, Ian Campbell wrote:
On Thu, 2014-06-05 at 16:40 +0800, Fu Wei wrote:
On 06/05/2014 04:20 PM, Ian Campbell wrote:
(switching to my citrix address which I forgot to do for this subthread)
On Thu, 2014-06-05 at 16:16 +0800, Fu Wei wrote:
On 06/05/2014 12:53 AM, Ian Campbell wrote:
On Wed, 2014-05-07 at 19:58 +0800, Fu Wei wrote:
+static grub_err_t +set_xen_module_type(multiboot_module_t module, int *argc, char **argv[]) +{ [...]
- } else {
node_info->type = module_type++;if (module_type > XEN_MODULE_OTHER)module_type = XEN_MODULE_OTHER;This will, I think, cause the third and subsequent non-explicitly typed modules to automatically get assigned XEN_MODULE_XSM etc, which I think is not intended/expected by http://wiki.xen.org/wiki/Xen_ARM_with_Virtualization_Extensions/Multiboot. The only things which should be automatically assigned are the kernel and initrd, anything else should just get the fallback compatibility string unless one is explicitly requested.
Actually, the code is for "giving a default compatibility property based on the order". it is described in http://wiki.xen.org/wiki/Xen_ARM_with_Virtualization_Extensions/Multiboot#Co.... I hope my understanding is correct. :-)
Perhaps I misunderstood something, please correct me :-)
Your code will assign additional defaults over and above what is described there. The spec says:
The first module specified in this way will be: compatible = "multiboot,linux-zimage", "boot,module" The second module specified in this way will be: compatible = "multiboot,linux-ramdisk", "boot,module" All subsequent modules will be: compatible = "boot,module"You implementation effectively adds: The third module specified in this way will be: compatible = "xen,xsm-policy", "xen,multiboot-module"
Which I think is undesirable.
Yes, I added a "xen,xsm-policy" after comparing wiki page with xen/docs/misc/arm/device-tree/booting.txt, I thought the wiki page is dated.
what I got from wiki page is we have two ways to decide the type of module: (1) by order (2) by "--type"
if "xen,xsm-policy" will be only loaded by "--type" way, I will improve the code ASAP. Actually, what I should do is just following the xen multiboot protocol that decided by Xen Project. :-)
Please let me know : should I delete "xen,xsm-policy" from the "(1) by order"?
Yes, I think so. The spec is intended to be generic and XSM doesn't really fit into that. I think update-grub (which the spec is supposed to be remaining syntax compatible with) won't ever generate an XSM module line anyway so people will already need to edit it manually.
OK, will do so
Could you please mention this in the wiki page or xen doc?
(Aside: it's possible that xsm,xsm-policy as a specific compat value was a mistake and Xen should be probing for the magic number like X86 does, but you need not worry about that)
Ian.
On Fri, 2014-06-06 at 17:03 +0800, Fu Wei wrote:
On 06/05/2014 05:10 PM, Ian Campbell wrote:
Yes, I think so. The spec is intended to be generic and XSM doesn't really fit into that. I think update-grub (which the spec is supposed to be remaining syntax compatible with) won't ever generate an XSM module line anyway so people will already need to edit it manually.
OK, will do so
Could you please mention this in the wiki page or xen doc?
I think the wiki spec is pretty clear about what types should be automatically assigned. In particular "All subsequent modules will be: * compatible = "boot,module" for the third and subsequent node.
Ian.
On 06/06/2014 06:40 PM, Ian Campbell wrote:
On Fri, 2014-06-06 at 17:03 +0800, Fu Wei wrote:
On 06/05/2014 05:10 PM, Ian Campbell wrote:
Yes, I think so. The spec is intended to be generic and XSM doesn't really fit into that. I think update-grub (which the spec is supposed to be remaining syntax compatible with) won't ever generate an XSM module line anyway so people will already need to edit it manually.
OK, will do so
Could you please mention this in the wiki page or xen doc?
I think the wiki spec is pretty clear about what types should be automatically assigned. In particular "All subsequent modules will be: * compatible = "boot,module" for the third and subsequent node.
OK, got you. :-)
The XSM module is included in "All subsequent modules"
Can the XSM module's compatible stream be "boot,module"?
Ian.
On Fri, 2014-06-06 at 19:05 +0800, Fu Wei wrote:
On 06/06/2014 06:40 PM, Ian Campbell wrote:
On Fri, 2014-06-06 at 17:03 +0800, Fu Wei wrote:
On 06/05/2014 05:10 PM, Ian Campbell wrote:
Yes, I think so. The spec is intended to be generic and XSM doesn't really fit into that. I think update-grub (which the spec is supposed to be remaining syntax compatible with) won't ever generate an XSM module line anyway so people will already need to edit it manually.
OK, will do so
Could you please mention this in the wiki page or xen doc?
I think the wiki spec is pretty clear about what types should be automatically assigned. In particular "All subsequent modules will be: * compatible = "boot,module" for the third and subsequent node.
OK, got you. :-)
The XSM module is included in "All subsequent modules"
Can the XSM module's compatible stream be "boot,module"?
Unless the module command in grub.cfg is given the --type option that is exactly what it should be.
Ian.
On 06/06/2014 07:11 PM, Ian Campbell wrote:
On Fri, 2014-06-06 at 19:05 +0800, Fu Wei wrote:
On 06/06/2014 06:40 PM, Ian Campbell wrote:
On Fri, 2014-06-06 at 17:03 +0800, Fu Wei wrote:
On 06/05/2014 05:10 PM, Ian Campbell wrote:
Yes, I think so. The spec is intended to be generic and XSM doesn't really fit into that. I think update-grub (which the spec is supposed to be remaining syntax compatible with) won't ever generate an XSM module line anyway so people will already need to edit it manually.
OK, will do so
Could you please mention this in the wiki page or xen doc?
I think the wiki spec is pretty clear about what types should be automatically assigned. In particular "All subsequent modules will be: * compatible = "boot,module" for the third and subsequent node.
OK, got you. :-)
The XSM module is included in "All subsequent modules"
Can the XSM module's compatible stream be "boot,module"?
Unless the module command in grub.cfg is given the --type option that is exactly what it should be.
OK, Roger that, will follow this to update the patch.
Ian.
On Wed, 2014-05-07 at 19:58 +0800, Fu Wei wrote:
+#define XEN_MODULE_IMAGE_NAME "linux-zimage" +#define XEN_MODULE_INITRD_NAME "linux-initrd" +#define XEN_MODULE_XSM_NAME "xsm-policy" +#define XEN_MODULE_OTHER_NAME "multiboot-module"
These get used as the node names I think, as in /chosen/module@linux-zimage. AIUI Device Tree convention would be /chosen/module@<address> where <address> is the physical address (i.e. the same as the base address in the reg property).
Ian.
On 06/05/2014 04:23 PM, Ian Campbell wrote:
On Wed, 2014-05-07 at 19:58 +0800, Fu Wei wrote:
+#define XEN_MODULE_IMAGE_NAME "linux-zimage" +#define XEN_MODULE_INITRD_NAME "linux-initrd" +#define XEN_MODULE_XSM_NAME "xsm-policy" +#define XEN_MODULE_OTHER_NAME "multiboot-module"
These get used as the node names I think, as in /chosen/module@linux-zimage. AIUI Device Tree convention would be /chosen/module@<address> where <address> is the physical address (i.e. the same as the base address in the reg property).
yes, my patch is doing /chosen/module@<address>, but not /chosen/module@<module_name>
Please check : static grub_err_t prepare_module_params_xen (multiboot_module_t module) in multiboot_xen.c : --------------------------- char module_name[XEN_MODULE_NAME_MAX_SIZE];
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 module"))); }
retval = grub_snprintf(module_name, XEN_MODULE_NAME_MAX_SIZE, "module@%d", module_index++); if (retval <(int) sizeof("module@")) return (grub_error(GRUB_ERR_BAD_OS, N_("failed to set node name for module")));
module_node = grub_fdt_find_subnode (multiboot_fdt, chosen_node, module_name); if (module_node < 0) module_node = grub_fdt_add_subnode (multiboot_fdt, chosen_node, module_name); ---------------------------
These "XEN_MODULE_*_NAME"s is just for the named list I used in multiboot.c/multiboot.h. It's easy to get a module node in the named list by "name".
struct multiboot_module { struct multiboot_module *next; struct multiboot_module **prev; const char *name;
int flags;
grub_addr_t start_unaligned; grub_addr_t start; grub_size_t size; grub_size_t align;
char *cmdline; int cmdline_size;
/* for more info */ void *extra_info; };
Ian.
On 06/05/2014 04:56 PM, Fu Wei wrote:
On 06/05/2014 04:23 PM, Ian Campbell wrote:
On Wed, 2014-05-07 at 19:58 +0800, Fu Wei wrote:
+#define XEN_MODULE_IMAGE_NAME "linux-zimage" +#define XEN_MODULE_INITRD_NAME "linux-initrd" +#define XEN_MODULE_XSM_NAME "xsm-policy" +#define XEN_MODULE_OTHER_NAME "multiboot-module"
These get used as the node names I think, as in /chosen/module@linux-zimage. AIUI Device Tree convention would be /chosen/module@<address> where <address> is the physical address (i.e. the same as the base address in the reg property).
yes, my patch is doing /chosen/module@<address>, but not /chosen/module@<module_name>
Please check : static grub_err_t prepare_module_params_xen (multiboot_module_t module) in multiboot_xen.c :
char module_name[XEN_MODULE_NAME_MAX_SIZE];
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 module"))); }
retval = grub_snprintf(module_name, XEN_MODULE_NAME_MAX_SIZE, "module@%d", module_index++);
Sorry, here maybe a bug, I should use "address", but not "index", will fix it in next version.
Thanks for pointing it out. :-)
if (retval <(int) sizeof("module@")) return (grub_error(GRUB_ERR_BAD_OS, N_("failed to set node name for module")));
module_node = grub_fdt_find_subnode (multiboot_fdt, chosen_node, module_name); if (module_node < 0) module_node = grub_fdt_add_subnode (multiboot_fdt, chosen_node, module_name);
These "XEN_MODULE_*_NAME"s is just for the named list I used in multiboot.c/multiboot.h. It's easy to get a module node in the named list by "name".
struct multiboot_module { struct multiboot_module *next; struct multiboot_module **prev; const char *name;
int flags;
grub_addr_t start_unaligned; grub_addr_t start; grub_size_t size; grub_size_t align;
char *cmdline; int cmdline_size;
/* for more info */ void *extra_info; };
Ian.
On Thu, 2014-06-05 at 17:01 +0800, Fu Wei wrote:
On 06/05/2014 04:56 PM, Fu Wei wrote:
On 06/05/2014 04:23 PM, Ian Campbell wrote:
On Wed, 2014-05-07 at 19:58 +0800, Fu Wei wrote:
+#define XEN_MODULE_IMAGE_NAME "linux-zimage" +#define XEN_MODULE_INITRD_NAME "linux-initrd" +#define XEN_MODULE_XSM_NAME "xsm-policy" +#define XEN_MODULE_OTHER_NAME "multiboot-module"
These get used as the node names I think, as in /chosen/module@linux-zimage. AIUI Device Tree convention would be /chosen/module@<address> where <address> is the physical address (i.e. the same as the base address in the reg property).
yes, my patch is doing /chosen/module@<address>, but not /chosen/module@<module_name>
Please check : static grub_err_t prepare_module_params_xen (multiboot_module_t module) in multiboot_xen.c :
char module_name[XEN_MODULE_NAME_MAX_SIZE];
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 module"))); }
retval = grub_snprintf(module_name, XEN_MODULE_NAME_MAX_SIZE, "module@%d", module_index++);
Sorry, here maybe a bug, I should use "address", but not "index", will fix it in next version.
Thanks for pointing it out. :-)
I was pointing out a bug which didn't exist, but at least I caused you to notice an actual bug ;-)
if (retval <(int) sizeof("module@")) return (grub_error(GRUB_ERR_BAD_OS, N_("failed to set node name for module")));
module_node = grub_fdt_find_subnode (multiboot_fdt, chosen_node, module_name); if (module_node < 0) module_node = grub_fdt_add_subnode (multiboot_fdt, chosen_node, module_name);
These "XEN_MODULE_*_NAME"s is just for the named list I used in multiboot.c/multiboot.h. It's easy to get a module node in the named list by "name".
My mistake. So it's just for debug output then I think? Seems reasonable.
BTW due to the \0 embedded in the compat string you can just printf module->compatible which will give you nearly the same thing. You'd get an extra "multiboot," in each of your debug lines using this, but that's not the end of the world I think and saves you having to worry about another field etc.
Ian.
On 06/05/2014 05:16 PM, Ian Campbell wrote:
On Thu, 2014-06-05 at 17:01 +0800, Fu Wei wrote:
On 06/05/2014 04:56 PM, Fu Wei wrote:
On 06/05/2014 04:23 PM, Ian Campbell wrote:
On Wed, 2014-05-07 at 19:58 +0800, Fu Wei wrote:
+#define XEN_MODULE_IMAGE_NAME "linux-zimage" +#define XEN_MODULE_INITRD_NAME "linux-initrd" +#define XEN_MODULE_XSM_NAME "xsm-policy" +#define XEN_MODULE_OTHER_NAME "multiboot-module"
These get used as the node names I think, as in /chosen/module@linux-zimage. AIUI Device Tree convention would be /chosen/module@<address> where <address> is the physical address (i.e. the same as the base address in the reg property).
yes, my patch is doing /chosen/module@<address>, but not /chosen/module@<module_name>
Please check : static grub_err_t prepare_module_params_xen (multiboot_module_t module) in multiboot_xen.c :
char module_name[XEN_MODULE_NAME_MAX_SIZE];
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 module"))); }
retval = grub_snprintf(module_name, XEN_MODULE_NAME_MAX_SIZE, "module@%d", module_index++);
Sorry, here maybe a bug, I should use "address", but not "index", will fix it in next version.
Thanks for pointing it out. :-)
I was pointing out a bug which didn't exist, but at least I caused you to notice an actual bug ;-)
Thanks for mentioning this! :-)
if (retval <(int) sizeof("module@")) return (grub_error(GRUB_ERR_BAD_OS, N_("failed to set node name for module")));
module_node = grub_fdt_find_subnode (multiboot_fdt, chosen_node, module_name); if (module_node < 0) module_node = grub_fdt_add_subnode (multiboot_fdt, chosen_node, module_name);
These "XEN_MODULE_*_NAME"s is just for the named list I used in multiboot.c/multiboot.h. It's easy to get a module node in the named list by "name".
My mistake. So it's just for debug output then I think? Seems reasonable.
it's not just for debug. this is a element of the named list data structure. this make multiboot.c can easily get a module/kernel node in the list.
BTW due to the \0 embedded in the compat string you can just printf module->compatible which will give you nearly the same thing. You'd get an extra "multiboot," in each of your debug lines using this, but that's not the end of the world I think and saves you having to worry about another field etc.
yes, will try this , Thanks ! :-)
Ian.
On Fri, 2014-06-06 at 17:12 +0800, Fu Wei wrote:
if (retval <(int) sizeof("module@")) return (grub_error(GRUB_ERR_BAD_OS, N_("failed to set node name for module")));
module_node = grub_fdt_find_subnode (multiboot_fdt, chosen_node, module_name); if (module_node < 0) module_node = grub_fdt_add_subnode (multiboot_fdt, chosen_node, module_name);
These "XEN_MODULE_*_NAME"s is just for the named list I used in multiboot.c/multiboot.h. It's easy to get a module node in the named list by "name".
My mistake. So it's just for debug output then I think? Seems reasonable.
it's not just for debug. this is a element of the named list data structure. this make multiboot.c can easily get a module/kernel node in the list.
I see now that it is used as part of the uniqueness check of the kernel module. As mentioned elsewhere I don't think this should be enforced by grub, in which case I think these names are no longer needed either.
Ian.
On 06/06/2014 06:55 PM, Ian Campbell wrote:
On Fri, 2014-06-06 at 17:12 +0800, Fu Wei wrote:
if (retval <(int) sizeof("module@")) return (grub_error(GRUB_ERR_BAD_OS, N_("failed to set node name for module")));
module_node = grub_fdt_find_subnode (multiboot_fdt, chosen_node, module_name); if (module_node < 0) module_node = grub_fdt_add_subnode (multiboot_fdt, chosen_node, module_name);
These "XEN_MODULE_*_NAME"s is just for the named list I used in multiboot.c/multiboot.h. It's easy to get a module node in the named list by "name".
My mistake. So it's just for debug output then I think? Seems reasonable.
it's not just for debug. this is a element of the named list data structure. this make multiboot.c can easily get a module/kernel node in the list.
I see now that it is used as part of the uniqueness check of the kernel module. As mentioned elsewhere I don't think this should be enforced by grub, in which case I think these names are no longer needed either.
Also be used here : finalize_params_multiboot (void), not just for the uniqueness check
Ian.
-
Fu Wei -
Ian Campbell -
Ian Campbell
-
Ian Campbell -
Ian Campbell -
Julien Grall -
Leif Lindholm