[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [XenPPC] [patch] multiboot2 support
On Jan 4, 2007, at 3:02 PM, Hollis Blanchard wrote:
Hi Jimi, thanks for the comments.
I'm really not interested in reworking all this stuff, which is why I
took shortcuts like relocating the modules rather than spending effort
on your preferred solution.
Ok, then it can wait till grub is ready and stable?
Otherwise the code does not add anything other than a new data
structure.
I thought you intended on pushing this soon?
Unfortunately, all this code was intimately
linked with the multiboot structure, so I had to change it.
On Thu, 2007-01-04 at 12:27 -0500, Jimi Xenidis wrote:
On Jan 3, 2007, at 12:32 PM, Hollis Blanchard wrote:
Xen on x86 uses GRUB's "multiboot" capabilities to load an arbitrary
number of images, including Xen, dom0 kernel, dom0 initrd, and ACM
policy. On PowerPC, we've had to build Xen, the dom0 kernel and the
dom0
initrd all into the same file to get them loaded.
We also boot from yaboot which allows us to separate dom0 from xen,
as does newer PIBS firmware via r3 & r4 and trivial to teach SLOF and
other OF how as well. Please do not assume that we are fully linked
only and do not remove that logic. Using your new boot loader case
should be additive.
I believe you yourself told me those other methods were unimportant
and
could be removed. :)
IIRC I agreed that defining location in the cmdline could certainly
go, but the r3,r4 pairing should stay.
With this patch, our memory map now looks
like this:
exception handlers (0x0 to 0x4000)
RTAS
Open Firmware device tree
RTAS and OFD are subject to "avail" properties in from OF and whether
or not they can be claimed. I don't think you should write code that
assumes they are here.
The code does not.
actually your patch in setup.c assumes the memory after _end is free
and clear, this is not the case if the OFD was placed after the image
which is certainly possible, see below.
Anyways, regardless of the exact placement (which is
subject to "available"), the order remains the same.
well, not really, RTAS could fit but OFD could easily be after the
image. It does now because we I made it smaller becaus SLOF/PIBS
devtrees are small. Unfortunately apple devtrees are HUGE and the
original size that we allocated (to accommodate them) cause it to
exist after the xen image.
+static void *alloc_tag(int key, int len)
+{
+ struct mb2_tag_header *tag;
+
Does len include sizeof(*tag)? it looks like it does, why not make it
implicit?
Since the the largest member of *tag is a uint32_t then it must be 4
byte aligned, please make sure your allocations are rounded up to 4
bytes, unless you _know_ that len is, but I'd do it anyway.
It's easy to go back and forth on this issue (I have already). In the
end this is best because you can "alloc_tag(FOO, sizeof(foo));"
I'm sure you have, but please don't forget to round up your allocation.
+static char *boot_of_bootargs(char **dom0_cmdline)
+{
+ static const char *sepr[] = {" -- ", " || "};
+ char *p;
+ int sepr_index;
int rc;
if (builtin_cmdline[0] == '\0') {
@@ -620,10 +644,22 @@ static void boot_of_bootargs(multiboot_i
of_panic("bootargs[] not big enough for /chosen/
bootargs\n");
}
- mbi->flags |= MBI_CMDLINE;
- mbi->cmdline = (ulong)builtin_cmdline;
-
of_printf("bootargs = %s\n", builtin_cmdline);
+
+ /* look for delimiter: "--" or "||" */
+ for (sepr_index = 0; sepr_index < ARRAY_SIZE(sepr); sepr_index+
+){
+ p = strstr((char *)builtin_cmdline, sepr[sepr_index]);
Is the cast necessary?
Doesn't look like it.
+ if (p != NULL) {
+ /* Xen proper should never know about the dom0
args. */
+ *(char *)p = '\0';
Why casting here?
This code was moved from another location where p was const. I'll fix.
+static int __init boot_of_rtas(void)
{
int rtas_node;
int rtas_instance;
@@ -1026,12 +1060,10 @@ static int __init boot_of_rtas(module_t
rtas_end = mem + size;
rtas_msr = of_msr;
- mod->mod_start = rtas_base;
- mod->mod_end = rtas_end;
return 1;
hmm, aren't you going to create a tag so you know where RTAS has been
placed and how big it is?
No. RTAS is not a module GRUB passes to kernels.
right, no tag is necessary since you have rtas_{base,end}.
+/* Create a structure as if we were loaded by a multiboot
bootloader. */
+struct mb2_tag_header __init *boot_of_multiboot(void)
+{
+ struct mb2_tag_start *start;
+ struct mb2_tag_name *name;
+ struct mb2_tag_module *xenmod;
+ struct mb2_tag_end *end;
+ char *xen_cmdline;
+ char *dom0_cmdline = NULL;
+ static char *namestr = "xen";
+
+ /* create "start" tag. start->size is filled in later. */
+ start = alloc_tag(MB2_TAG_START, sizeof(*start));
+
+ /* create "name" tag. */
+ name = alloc_tag(MB2_TAG_NAME, sizeof(*name) + strlen
(namestr));
Are you intentionally skipping the '\0' that in the alloc
Nope, good catch!
@@ -141,43 +143,9 @@ static void ofd_walk_mem(void *m, walk_m
}
}
-static void setup_xenheap(module_t *mod, int mcount)
-{
- int i;
- ulong freemem;
-
- freemem = ALIGN_UP((ulong)_end, PAGE_SIZE);
-
- for (i = 0; i < mcount; i++) {
- u32 s;
-
- if (mod[i].mod_end == mod[i].mod_start)
- continue;
-
- s = ALIGN_DOWN(mod[i].mod_start, PAGE_SIZE);
-
- if (mod[i].mod_start > (ulong)_start &&
- mod[i].mod_start < (ulong)_end) {
- /* mod was linked in */
- continue;
- }
-
- if (s < freemem)
- panic("module addresses must assend\n");
-
- free_xenheap(freemem, s);
- freemem = ALIGN_UP(mod[i].mod_end, PAGE_SIZE);
-
- }
-
- /* the rest of the xenheap, starting at the end of modules */
- free_xenheap(freemem, xenheap_phys_end);
-}
-
-void memory_init(module_t *mod, int mcount)
We did a lot of work here so that stuff could be placed anywhere. I
admit it was not pretty but I'd expect this patch to replace/improve
not remove.
The memmove below means this logic is unnecessary.
I'd prefer some logic over blind moves of several megabytes (3-16),
this will kill simulators.
- /* Parse the command-line options. */
- if ((mbi->flags & MBI_CMDLINE) && (mbi->cmdline != 0))
- cmdline_parse(__va((ulong)mbi->cmdline));
+ /* parse the command-line options. */
Can you make the following a function:
struct mb2_tag_module *find_tag_mod_type(uint32_t key, const char
*type)
It will be handy as more tags are created.
I can do that.
+ /* copy modules to a contiguous space immediately after Xen */
We stopped doing this copy because it was unnecessary, I believe it
still is what the reason for putting it back?
The trouble is that interface for adding memory to the heap is start,
len.
You have a list of an arbitrary number of modules in arbitrary
locations
of memory. I don't think it's worth it to sort them so you can use
that
interface.
IMHO, sorting would be trivial, or even better a bitmap would do
nicely and be the most efficient.
We'll need boot_of_alloc_init() to remove all the modules from
allocation consideration before we do any kind of allocation for RTAS
of OFD. We need to do something here anyway.
I do not think it is fair to assume that you can have that space.
As of now, if the modules are linked in we have a large hunk of
memory (in the xen image) that will not go away.
How do the original pages get released back into the xenheap?
The free_xenheap() calls in memory_init().
+
+ if (r3 == MB2_BOOTLOADER_MAGIC) {
+ tags = (struct mb2_tag_header *)r4;
+ if (tags->key != MB2_TAG_START)
If this fails, why not just continue with the fake up?
Because that would be a pretty serious error, don't you think?
yup.
+ return;
} else {
- /* booted by someone else that hopefully has a trap
handler */
- __builtin_trap();
- }
-
- __start_xen(mbi);
-
the fake multiboot stuff should be in its own file and we can keep
boot_of.c clean.
OK.
--
Hollis Blanchard
IBM Linux Technology Center
_______________________________________________
Xen-ppc-devel mailing list
Xen-ppc-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-ppc-devel
|
