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.