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. 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. :)
> > 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. Anyways, regardless of the exact placement (which is
subject to "available"), the order remains the same.
> > +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));"
> > +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.
> > +/* 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.
> > - /* 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.
> 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?
> > + 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
|
Home