[patch 2/3] grub efi initrd image memory allocation 4K alignment
Hi,
On grub efi platform, initrd image memory allocation start
address must be 4K alignment, else it will fail to alloc memory
for initrd image.
thanks
bibo,mao
diff -Nruap grub2.org/loader/i386/efi/linux.c grub2/loader/i386/efi/linux.c
--- grub2.org/loader/i386/efi/linux.c 2006-10-24 13:24:46.0 +0800
+++ grub2/loader/i386/efi/linux.c 2006-10-24 13:25:35.0 +0800
@@ -587,7 +587,6 @@ grub_rescue_cmd_initrd (int argc, char *
desc = NEXT_MEMORY_DESCRIPTOR (desc, desc_size))
{
if (desc->type == GRUB_EFI_CONVENTIONAL_MEMORY
- && desc->physical_start >= addr_min
&& desc->physical_start + size < addr_max
&& desc->num_pages >= initrd_pages)
{
@@ -598,7 +597,7 @@ grub_rescue_cmd_initrd (int argc, char *
physical_end = addr_max;
if (physical_end > addr)
- addr = physical_end - page_align (size);
+ addr = PAGE_DOWN(physical_end - page_align (size));
}
}
___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 3/3] grub EFI disk device enumberating
Hi,
On EFI platform, every partition is regarded as one block device and
responding EFI device path. EFI device patch has the same hierarchical
relationship with the partition. This patch will search root EFI device
path by current device patch, but not parent device path.
Original grub can not find efi disk when grub.efi is executed on
logical partition.
Previously I posted this patch, now I repost again.
Any suggestion is welcome.
thanks
bibo,mao
diff -Nruap grub2.org/disk/efi/efidisk.c grub2/disk/efi/efidisk.c
--- grub2.org/disk/efi/efidisk.c2006-10-24 13:23:42.0 +0800
+++ grub2/disk/efi/efidisk.c2006-10-24 14:10:32.0 +0800
@@ -87,6 +87,52 @@ find_last_device_path (const grub_efi_de
return p;
}
+static int compare_ancestor_path(const grub_efi_device_path_t *parent,
+const grub_efi_device_path_t *dp2)
+{
+ /* Return non-zero. */
+ if (! parent || ! dp2)
+return 1;
+
+ while (1)
+{
+ grub_efi_uint8_t type1, type2;
+ grub_efi_uint8_t subtype1, subtype2;
+ grub_efi_uint16_t len1, len2;
+ int ret;
+
+ if (GRUB_EFI_END_ENTIRE_DEVICE_PATH(parent))
+ break;
+
+ type1 = GRUB_EFI_DEVICE_PATH_TYPE (parent);
+ type2 = GRUB_EFI_DEVICE_PATH_TYPE (dp2);
+
+ if (type1 != type2)
+ return (int) type2 - (int) type1;
+
+ subtype1 = GRUB_EFI_DEVICE_PATH_SUBTYPE (parent);
+ subtype2 = GRUB_EFI_DEVICE_PATH_SUBTYPE (dp2);
+
+ if (subtype1 != subtype2)
+ return (int) subtype1 - (int) subtype2;
+
+ len1 = GRUB_EFI_DEVICE_PATH_LENGTH (parent);
+ len2 = GRUB_EFI_DEVICE_PATH_LENGTH (dp2);
+
+ if (len1 != len2)
+ return (int) len1 - (int) len2;
+
+ ret = grub_memcmp (parent, dp2, len1);
+ if (ret != 0)
+ return ret;
+
+ parent = (grub_efi_device_path_t *) ((char *) parent + len1);
+ dp2 = (grub_efi_device_path_t *) ((char *) dp2 + len2);
+}
+
+ return 0;
+}
+
/* Compare device paths. */
static int
compare_device_paths (const grub_efi_device_path_t *dp1,
@@ -197,44 +243,6 @@ make_devices (void)
return devices;
}
-/* Find the parent device. */
-static struct grub_efidisk_data *
-find_parent_device (struct grub_efidisk_data *devices,
- struct grub_efidisk_data *d)
-{
- grub_efi_device_path_t *dp, *ldp;
- struct grub_efidisk_data *parent;
-
- dp = duplicate_device_path (d->device_path);
- if (! dp)
-return 0;
-
- ldp = find_last_device_path (dp);
- ldp->type = GRUB_EFI_END_DEVICE_PATH_TYPE;
- ldp->subtype = GRUB_EFI_END_ENTIRE_DEVICE_PATH_SUBTYPE;
- ldp->length[0] = sizeof (*ldp);
- ldp->length[1] = 0;
-
- for (parent = devices; parent; parent = parent->next)
-{
- /* Ignore itself. */
- if (parent == d)
- continue;
-
- if (compare_device_paths (parent->device_path, dp) == 0)
- {
- /* Found. */
- if (! parent->last_device_path)
- parent = 0;
-
- break;
- }
-}
-
- grub_free (dp);
- return parent;
-}
-
static int
iterate_child_devices (struct grub_efidisk_data *devices,
struct grub_efidisk_data *d,
@@ -301,107 +309,36 @@ add_device (struct grub_efidisk_data **d
}
/* Name the devices. */
-static void
-name_devices (struct grub_efidisk_data *devices)
-{
+static void name_devices (struct grub_efidisk_data *devices){
struct grub_efidisk_data *d;
-
- /* First, identify devices by media device paths. */
- for (d = devices; d; d = d->next)
-{
- grub_efi_device_path_t *dp;
-
- dp = d->last_device_path;
- if (! dp)
- continue;
-
- if (GRUB_EFI_DEVICE_PATH_TYPE (dp) == GRUB_EFI_MEDIA_DEVICE_PATH_TYPE)
- {
- int is_hard_drive = 0;
-
- switch (GRUB_EFI_DEVICE_PATH_SUBTYPE (dp))
- {
- case GRUB_EFI_HARD_DRIVE_DEVICE_PATH_SUBTYPE:
- is_hard_drive = 1;
- /* Fall through by intention. */
- case GRUB_EFI_CDROM_DEVICE_PATH_SUBTYPE:
- {
- struct grub_efidisk_data *parent;
-
- parent = find_parent_device (devices, d);
- if (parent)
- {
- if (is_hard_drive)
- {
-#if 0
- grub_printf ("adding a hard drive by a partition: ");
- grub_print_device_path (parent->device_path);
-#endif
- add_device (&hd_devices, parent);
- }
- else
- {
-#if 0
- grub_printf ("adding a cdrom by a partition: ");
- grub_print_device_path (parent->device_path);
-#endif
- add_device (&cd_devices, parent);
- }
-
- /* Mark the parent as used. */
- parent->last_device_path = 0;
- }
- }
- /* Mark itself as used. */
- d->last_device_path = 0;
-
Pot 41980USD
Hey Grub-devel!.! Roller Casino, we are so surey ou are going to love our games that we are giving you up to USD888.00 FREE just fo rtrying our Casino. $ 888.00 FREE! Clikc Here Now! http://laseert.com/c/63 +++ climbed the ladder. I followed at a far more moderate pace, stoppingsay that it can be broken because of expediency. It would seem thatlong behind me and I had nothing but an aching and empty void for afly through the air, not to be found again. I plowed through sand ___ Guile-cvs mailing list [EMAIL PROTECTED] http://lists.gnu.org/mailman/listinfo/guile-cvs
Re: [PATCH 3/3] grub EFI disk device enumberating
"bibo,mao" <[EMAIL PROTECTED]> writes:
> Previously I posted this patch, now I repost again.
> Any suggestion is welcome.
I will just give a comment here. Please do not treat it as a full
review, I'll leave that to Okuji.
> +static int compare_ancestor_path(const grub_efi_device_path_t *parent,
> +const grub_efi_device_path_t *dp2)
> +{
> [...]
> + while (1)
> +{
> + grub_efi_uint8_t type1, type2;
> + grub_efi_uint8_t subtype1, subtype2;
> + grub_efi_uint16_t len1, len2;
> + int ret;
> +
> + if (GRUB_EFI_END_ENTIRE_DEVICE_PATH(parent))
> + break;
> +
> + type1 = GRUB_EFI_DEVICE_PATH_TYPE (parent);
> + type2 = GRUB_EFI_DEVICE_PATH_TYPE (dp2);
> +
> + if (type1 != type2)
> + return (int) type2 - (int) type1;
> +
> + subtype1 = GRUB_EFI_DEVICE_PATH_SUBTYPE (parent);
> + subtype2 = GRUB_EFI_DEVICE_PATH_SUBTYPE (dp2);
> +
> + if (subtype1 != subtype2)
> + return (int) subtype1 - (int) subtype2;
> +
> + len1 = GRUB_EFI_DEVICE_PATH_LENGTH (parent);
> + len2 = GRUB_EFI_DEVICE_PATH_LENGTH (dp2);
> +
> + if (len1 != len2)
> + return (int) len1 - (int) len2;
> +
> + ret = grub_memcmp (parent, dp2, len1);
> + if (ret != 0)
> + return ret;
> +
> + parent = (grub_efi_device_path_t *) ((char *) parent + len1);
> + dp2 = (grub_efi_device_path_t *) ((char *) dp2 + len2);
This code can be shorter; you only have to compare the lengths. If
they match, you can do a memcmp on the whole device path.
Also, can you guarantee that the child path "dp2" is longer than the
parent path? What it does is that it checks if "parent" is a subpath
of "dp2", so a function name reflecting that probably fits better.
Name functions after what they do, not what they are used to. Better
argument names are also welcome.
~j
pgpFtuLL19gJJ.pgp
Description: PGP signature
___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 1/3] grub efi memory map patch
"bibo,mao" <[EMAIL PROTECTED]> writes:
> This patch moves find_mmap_size from i386/efi/linux.c to
> kern/efi/mm.c, and renamed as grub_efi_find_mmap_size, so that
> other arch on EFI platform can use this function.
Good call.
> Also this function solves memory map too small problem,
> on some EFI platform MEMORY_MAP_SIZE(0x1000) is a little smaller
> than EFI memory map table.
>
> any suggestion is welcome.
Just a few comments here. I will leave a full review to Okuji.
> +#define GRUB_EFI_PAGE_SHIFT 12
> +#define GRUB_EFI_PAGE_SIZE (0x1UL << GRUB_EFI_PAGE_SHIFT)
> +#define GRUB_EFI_PAGES(addr) (addr >> GRUB_EFI_PAGE_SHIFT)
> +#define GRUB_EFI_PAGES_UP(addr) ((addr + GRUB_EFI_PAGE_SIZE - 1) >>
> GRUB_EFI_PAGE_SHIFT)
> +#define PAGE_DOWN(addr) ((addr) & (~(GRUB_EFI_PAGE_SIZE - 1)))
> +#define PAGE_UP(addr)PAGE_DOWN(addr + GRUB_EFI_PAGE_SIZE - 1)
The PAGE_DOWN and PAGE_UP macros should be named something else, with
a GRUB_EFI prefix. What about GRUB_EFI_PAGE_TRUNC and
GRUB_EFI_PAGE_ROUND? Not sure GRUB_EFI_PAGES_UP is needed, since
GRUB_EFI_PAGES (GRUB_EFI_PAGE_ROUND (value)) can be used instead. If
you find it too long, add a local macro to the file where you use it.
Also, please slap a few parenthesis round marco arguments.
> +/* Find the optimal number of pages for the memory map. */
> +grub_efi_uintn_t
> +grub_efi_find_mmap_size (void)
> +{ + static grub_efi_uintn_t mmap_size = 0;
> ++ if (mmap_size != 0)
> +return mmap_size;
> ++ mmap_size = GRUB_EFI_PAGE_SIZE;
> + while (1)
> +{
> + int ret;
> + grub_efi_memory_descriptor_t *mmap;
> + grub_efi_uintn_t desc_size;
> + + mmap = grub_efi_allocate_pages ( 0,
> GRUB_EFI_PAGES_UP(mmap_size));
> + if (! mmap)
> +return 0;
> + + ret = grub_efi_get_memory_map (&mmap_size, mmap, 0,
> &desc_size, 0);
> + grub_efi_free_pages ((grub_addr_t) mmap, GRUB_EFI_PAGES_UP(mmap_size));
> + + if (ret < 0)
> +grub_fatal ("cannot get memory map");
> + else if (ret > 0)
> +break;
> + + mmap_size += GRUB_EFI_PAGE_SIZE;
> +}
> + + /* Increase the size a bit for safety, because GRUB allocates
> more on
> + later, and EFI itself may allocate more. */
> + mmap_size += GRUB_EFI_PAGE_SIZE;
> + mmap_size = PAGE_UP(mmap_size);
> + + return mmap_size;
> +}
Is this some patching gone wrong? What are all the "+"'s doing there?
The specification states that GetMemoryMap will return
EFI_BUFFER_TOO_SMALL and the memory map size in *MemoryMapSize if the
memory map could not fit in the buffer (which may be NULL if provided
buffer size is too small). Meaning that you can get the size of the
memory map by simply doing;
grub_efi_uintn_t
grub_efi_find_mmap_size (void)
{
mmap_size = 0;
err = grub_efi_get_memory_map (&mmap_size, NULL, 0, NULL, 0);
if (err != GRUB_EFI_BUFFER_TOO_SMALL)
return err;
return mmap_size;
}
(code not tested, nor compiled.)
I think Tristan did this in his patch that addresses the same problem
of that the memory map does not fit in a single page.
> +
> void
> grub_efi_mm_init (void)
Whitespace changes are normally not welcome. But they could be
removed by the maintainer who commits it. No worries.
B> {
> @@ -346,6 +381,8 @@ grub_efi_mm_init (void)
> grub_efi_uintn_t desc_size;
> grub_efi_uint64_t total_pages;
> grub_efi_uint64_t required_pages;
> + grub_efi_uintn_t memory_map_size;
> + int res;
>
> /* First of all, allocate pages to maintain allocations. */
> allocated_pages
> @@ -355,16 +392,20 @@ grub_efi_mm_init (void)
>
> grub_memset (allocated_pages, 0, ALLOCATED_PAGES_SIZE);
> - /* Prepare a memory region to store two memory maps. */
> - memory_map = grub_efi_allocate_pages (0,
> - 2 * BYTES_TO_PAGES (MEMORY_MAP_SIZE));
> + /* Prepare a memory region to store two memory maps. Obtain size of
> + memory map by passing a NULL buffer and a buffer size of
> + zero. */
> + memory_map_size = grub_efi_find_mmap_size( );
> + memory_map = grub_efi_allocate_pages(0, 2 * PAGE_UP(memory_map_size));
> +
One space between function name and parenthesis, and no spaces within
a empty argument list.
> if (! memory_map)
> grub_fatal ("cannot allocate memory");
>
> - filtered_memory_map = NEXT_MEMORY_DESCRIPTOR (memory_map, MEMORY_MAP_SIZE);
> + filtered_memory_map = NEXT_MEMORY_DESCRIPTOR (memory_map,
> +
> PAGE_UP(memory_map_size));
I suppose this is a line-wrapped patch?
pgpPYV9wnr7sS.pgp
Description: PGP signature
___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 3/3] grub EFI disk device enumberating
Johan Rydberg <[EMAIL PROTECTED]> writes:
> This code can be shorter; you only have to compare the lengths. If
> they match, you can do a memcmp on the whole device path.
It could look something like this;
/* Returns zero if device path SUBPATH is a subpath of device path
PATH. */
static int
compare_subpath (const grub_efi_device_path_t *subpath,
const grub_efi_device_path_t *path)
{
if (! subpath || ! path)
return 1;
while (1)
{
int len, ret;
if (GRUB_EFI_END_ENTIRE_DEVICE_PATH (subpath))
return 0;
else if (GRUB_EFI_END_ENTIRE_DEVICE_PATH (path))
return 1;
if (GRUB_EFI_DEVICE_PATH_LENGTH (subpath)
!= GRUB_EFI_DEVICE_PATH_LENGTH (path))
return ((int) GRUB_EFI_DEVICE_PATH_LENGTH (subpath)
- (int) GRUB_EFI_DEVICE_PATH_LENGTH (path));
len = GRUB_EFI_DEVICE_PATH_LENGTH (path);
ret = grub_memcmp (subpath, path, len);
if (ret)
return ret;
path = (grub_efi_device_path_t *) ((char *) path + len);
subpath = (grub_efi_device_path_t *) ((char *) subpath + len);
}
}
I guess that should work at least, it is not tested.
In gnufi I have a device_path_iterate function that could be used for
these kind of things. Maybe we should bring it in to GRUB2.
/* Iterate nodes of the device path. *PATHP should be set to point to
the path that is to be iterated. NULL will be returned when the
end of the path has been reached. */
efi_device_path_t *
efi_device_path_iterate (efi_device_path_t **pathp)
{
efi_device_path_t *p = *pathp;
if (EFI_END_ENTIRE_DEVICE_PATH (p))
return NULL;
else
{
efi_uint_t len = EFI_DEVICE_PATH_LENGTH (p);
*pathp = (efi_device_path_t *) (((char *) p) + len);
return p;
}
}
~j
pgpfuh7RNvMft.pgp
Description: PGP signature
___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] generic ELF loading
On Sat, 2006-10-14 at 17:33 +0200, Yoshinori K. Okuji wrote:
> On Saturday 14 October 2006 00:37, Hollis Blanchard wrote:
> > This patch adds generic ELF loading infrastructure for both 32-bit and
> > 64-bit ELF. It provides an "iterate" function for program headers, and a
> > "load" function for convenience.
>
> The idea is very good. But I don't like that loaded areas are always
> allocated
> from the heap. GRUB has a staging area for OS images on i386-pc, and I prefer
> to load an image directly instead of consuming the heap.
Actually I'm not using the heap, I'm just directly copying wherever
phdr->p_paddr says to. That's not a good thing actually; in the future
we should add some error checking to make sure we don't clobber GRUB
itself.
Also, I made sure that the load hook could veto loading each segment. So
the x86 loader would do something like this:
int x86_load_hook (Elf32_Phdr *phdr)
{
if (phdr->p_paddr not in x86 OS load area)
return 1;
return 0;
}
grub_elf32_load (elf, x86_load_hook);
-Hollis
___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] generic ELF loading
Hollis Blanchard <[EMAIL PROTECTED]> writes: >> The idea is very good. But I don't like that loaded areas are always >> allocated >> from the heap. GRUB has a staging area for OS images on i386-pc, and I >> prefer >> to load an image directly instead of consuming the heap. > > Actually I'm not using the heap, I'm just directly copying wherever > phdr->p_paddr says to. That's not a good thing actually; in the future > we should add some error checking to make sure we don't clobber GRUB > itself. Have you looked at how EFI solves this? They keep track of all memory regions, and with each region is a "memory type" associated. Whenever you allocate memory you change the type of the region (from "free") to some that makes sense (could be "loader data", "disk cache", ...). You can only allocate memory that is marked "conventional", meaning it is considered free. The memory region database is later feed to the operating system. We could do the same. Is this case we could allocate the regions specified by the ELF image. If any of the allocations fail we know there is something already loaded there, or the image is faulty. ~j pgpg1QzNSXV0d.pgp Description: PGP signature ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
RE: [PATCH] generic ELF loading
On Sat, 2006-10-14 at 11:03 +0800, Mao, Bibo wrote:
>I do not know whether it is possible to add one element in
> structure grub_elf_file structure to identify ELF type
> (ELFCLASS32/ELFCLASS64) and ELF machine type, this element can be set
> at function grub_elf_open.
I'm not sure it saves much, since you'd still want a wrapper function,
and the info is already present in `ehdr' anyways. It comes down to
int grub_elf_is_elf32 (grub_elf_t elf)
{
return elf->ehdr.ehdr32.e_ident[EI_CLASS] == ELFCLASS32;
}
vs
int grub_elf_is_elf32 (grub_elf_t elf)
{
return elf->class == ELFCLASS32;
}
-Hollis
___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] generic ELF loading
On Sat, 2006-10-14 at 19:23 +0200, Tristan Gingold wrote: > On Sat, Oct 14, 2006 at 05:33:44PM +0200, Yoshinori K. Okuji wrote: > > On Saturday 14 October 2006 00:37, Hollis Blanchard wrote: > > > This patch adds generic ELF loading infrastructure for both 32-bit and > > > 64-bit ELF. It provides an "iterate" function for program headers, and a > > > "load" function for convenience. > > > > The idea is very good. But I don't like that loaded areas are always > > allocated > > from the heap. GRUB has a staging area for OS images on i386-pc, and I > > prefer > > to load an image directly instead of consuming the heap. > Two points for ia64: > * the area must be allocated from EFI. What does this mean? Open Firmware has a "claim" call, which reserves memory and makes it available to the application. I assume EFI must have something similar. This can be called via a hook. For error handling, you'd probably need to iterate once to claim the memory, iterate once to copy the ELF file there, and in case of error iterate again to free the claimed memory. > * we need to support relocation: loading the ELF file with an offset > (this feature can be on/off/forced). This is provided via `load_hook' in grub_elf32_load(). In fact, PowerPC already does this. Please see offset_phdr() in my second mail (Subject: Re: [PATCH] ppc64 Linux ELF loader). -Hollis ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] generic ELF loading
On Tue, 2006-10-24 at 22:48 +0200, Johan Rydberg wrote: > Hollis Blanchard <[EMAIL PROTECTED]> writes: > > >> The idea is very good. But I don't like that loaded areas are always > >> allocated > >> from the heap. GRUB has a staging area for OS images on i386-pc, and I > >> prefer > >> to load an image directly instead of consuming the heap. > > > > Actually I'm not using the heap, I'm just directly copying wherever > > phdr->p_paddr says to. That's not a good thing actually; in the future > > we should add some error checking to make sure we don't clobber GRUB > > itself. > > Have you looked at how EFI solves this? > > They keep track of all memory regions, and with each region is a > "memory type" associated. Whenever you allocate memory you change the > type of the region (from "free") to some that makes sense (could be > "loader data", "disk cache", ...). You can only allocate memory that > is marked "conventional", meaning it is considered free. The memory > region database is later feed to the operating system. We could do > the same. > > Is this case we could allocate the regions specified by the ELF image. > If any of the allocations fail we know there is something already > loaded there, or the image is faulty. That's a great point. Doing a "claim" (as described in my reply to Tristan) will return an error if the area is already claimed, so that will accomplish the check. On x86, the load hook will manually check to make sure the address is within the OS load area. -Hollis ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: Bug#390473: grub2: menuentry stanza doesn't accept $ { } in variable substitutions
reopen 390473
retitle 390473 grub2: menuentry stanza with additional $ or { causes boot panic
found 390473 1.95-1
thanks
Hi Robert!
NB: forwarding again to grub-devel, but setting M-F-T and R-T to the
bug, Robert (I hope you don't mind) and myself.
On Tue, 24 Oct 2006 15:29:09 +0200, Robert Millan wrote:
> Closing due to lack of response.
I'm sorry, I was quite busy in the last weeks and I never finished my
answer.
>
> On Sat, Oct 14, 2006 at 09:36:57PM +0200, Robert Millan wrote:
[something already present in the bug report]
>> According to upstream (IRC), this behaviour is consistent with GRUB2 having
>> its
>> own variable support (and namespace..). So you really need to use this new
>> syntax (I'll take this into account for my work on update-grub2).
>>
>> Can we close this bug now?
While the original bug can be considered close, the boot panic
remains, as I explained in my first post:
On Sun, 01 Oct 2006 15:54:00 +0200, Luca Capello wrote:
> After a `grub-install /dev/hda` and a reboot, grub2 entered in a
> panic state at boot. Hopefully, the panic is reproducible, even on
> qemu: create the following stanza (you can substitute $ with a
> second { ):
>
> menuentry "test" { $ }
>
> The panic is similar to the error generated with a $ at the
> beginning of the grub.cfg file. While in the latter case grub2 can
> continue booting, in n the former case the only way to solve it is
> to boot with a rescue CD and remove the offending characters from
> grub2.cfg.
I reopened the bug and I changed the title to reflect the situation.
BTW, I just tested with the latest grub_1.95-1, the error is still
present, thus I added the information to the BTS.
Thx, bye,
Gismo / Luca
pgpstsVAaXgsu.pgp
Description: PGP signature
___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 1/3] grub efi memory map patch
hi johan,
Thanks for review my patch, I reply in lines.
Johan Rydberg wrote:
"bibo,mao" <[EMAIL PROTECTED]> writes:
> This patch moves find_mmap_size from i386/efi/linux.c to
> kern/efi/mm.c, and renamed as grub_efi_find_mmap_size, so that
> other arch on EFI platform can use this function.
Good call.
> Also this function solves memory map too small problem,
> on some EFI platform MEMORY_MAP_SIZE(0x1000) is a little smaller
> than EFI memory map table.
>
> any suggestion is welcome.
Just a few comments here. I will leave a full review to Okuji.
> +#define GRUB_EFI_PAGE_SHIFT 12
> +#define GRUB_EFI_PAGE_SIZE (0x1UL << GRUB_EFI_PAGE_SHIFT)
> +#define GRUB_EFI_PAGES(addr) (addr >> GRUB_EFI_PAGE_SHIFT)
> +#define GRUB_EFI_PAGES_UP(addr) ((addr + GRUB_EFI_PAGE_SIZE - 1) >>
GRUB_EFI_PAGE_SHIFT)
> +#define PAGE_DOWN(addr) ((addr) & (~(GRUB_EFI_PAGE_SIZE - 1)))
> +#define PAGE_UP(addr)PAGE_DOWN(addr + GRUB_EFI_PAGE_SIZE -
1)
The PAGE_DOWN and PAGE_UP macros should be named something else, with
a GRUB_EFI prefix. What about GRUB_EFI_PAGE_TRUNC and
GRUB_EFI_PAGE_ROUND? Not sure GRUB_EFI_PAGES_UP is needed, since
GRUB_EFI_PAGES (GRUB_EFI_PAGE_ROUND (value)) can be used instead. If
you find it too long, add a local macro to the file where you use it.
It sounds better for me, I rename the macro name as GRUB_EFI_PAGE_TRUNC
and GRUB_EFI_PAGE_ROUND, and delete GRUB_EFI_PAGES_UP macro.
Also, please slap a few parenthesis round marco arguments.
> +/* Find the optimal number of pages for the memory map. */
> +grub_efi_uintn_t
> +grub_efi_find_mmap_size (void)
> +{ + static grub_efi_uintn_t mmap_size = 0;
> ++ if (mmap_size != 0)
> +return mmap_size;
> ++ mmap_size = GRUB_EFI_PAGE_SIZE;
> + while (1)
> +{
> + int ret;
> + grub_efi_memory_descriptor_t *mmap;
> + grub_efi_uintn_t desc_size;
> + + mmap = grub_efi_allocate_pages ( 0,
> GRUB_EFI_PAGES_UP(mmap_size));
> + if (! mmap)
> +return 0;
> + + ret = grub_efi_get_memory_map (&mmap_size, mmap, 0,
> &desc_size, 0);
> + grub_efi_free_pages ((grub_addr_t) mmap,
GRUB_EFI_PAGES_UP(mmap_size));
> + + if (ret < 0)
> +grub_fatal ("cannot get memory map");
> + else if (ret > 0)
> +break;
> + + mmap_size += GRUB_EFI_PAGE_SIZE;
> +}
> + + /* Increase the size a bit for safety, because GRUB allocates
> more on
> + later, and EFI itself may allocate more. */
> + mmap_size += GRUB_EFI_PAGE_SIZE;
> + mmap_size = PAGE_UP(mmap_size);
> + + return mmap_size;
> +}
Is this some patching gone wrong? What are all the "+"'s doing there?
The specification states that GetMemoryMap will return
EFI_BUFFER_TOO_SMALL and the memory map size in *MemoryMapSize if the
memory map could not fit in the buffer (which may be NULL if provided
buffer size is too small). Meaning that you can get the size of the
memory map by simply doing;
grub_efi_uintn_t
grub_efi_find_mmap_size (void)
{
mmap_size = 0;
err = grub_efi_get_memory_map (&mmap_size, NULL, 0, NULL, 0);
if (err != GRUB_EFI_BUFFER_TOO_SMALL)
return err;
return mmap_size;
}
(code not tested, nor compiled.)
I think Tristan did this in his patch that addresses the same problem
of that the memory map does not fit in a single page.
Yes, it is better. But there is one point reserved to notice,
grub_efi_find_mmap_size will return current memory map size, if there is
memory alloc function, map size will be larger. so allocated
(mmap_size + one extra page) buffer for memory map buffer is more reasonable.
> +
> void
> grub_efi_mm_init (void)
Whitespace changes are normally not welcome. But they could be
removed by the maintainer who commits it. No worries.
B> {
> @@ -346,6 +381,8 @@ grub_efi_mm_init (void)
> grub_efi_uintn_t desc_size;
> grub_efi_uint64_t total_pages;
> grub_efi_uint64_t required_pages;
> + grub_efi_uintn_t memory_map_size;
> + int res;
>
> /* First of all, allocate pages to maintain allocations. */
> allocated_pages
> @@ -355,16 +392,20 @@ grub_efi_mm_init (void)
>
> grub_memset (allocated_pages, 0, ALLOCATED_PAGES_SIZE);
> - /* Prepare a memory region to store two memory maps. */
> - memory_map = grub_efi_allocate_pages (0,
> - 2 * BYTES_TO_PAGES (MEMORY_MAP_SIZE));
> + /* Prepare a memory region to store two memory maps. Obtain size of
> + memory map by passing a NULL buffer and a buffer size of
> + zero. */
> + memory_map_size = grub_efi_find_mmap_size( );
> + memory_map = grub_efi_allocate_pages(0, 2 * PAGE_UP(memory_map_size));
> +
One space between function name and parenthesis, and no spaces within
a empty argument list.
I am not familiar with GNU coding style:(
The second patch corrects the point this time.
> if (! memory_map)
> grub_fatal ("cannot all
Re: [patch 2/3] grub efi initrd image memory allocation 4K alignment
This patch is sent for second time.
thanks
bibo,mao
diff -Nrup grub2.org/loader/i386/efi/linux.c grub2/loader/i386/efi/linux.c
--- grub2.org/loader/i386/efi/linux.c 2006-10-25 12:15:25.0 +0800
+++ grub2/loader/i386/efi/linux.c 2006-10-25 11:40:24.0 +0800
@@ -587,7 +587,6 @@ grub_rescue_cmd_initrd (int argc, char *
desc = NEXT_MEMORY_DESCRIPTOR (desc, desc_size))
{
if (desc->type == GRUB_EFI_CONVENTIONAL_MEMORY
- && desc->physical_start >= addr_min
&& desc->physical_start + size < addr_max
&& desc->num_pages >= initrd_pages)
{
@@ -598,7 +597,7 @@ grub_rescue_cmd_initrd (int argc, char *
physical_end = addr_max;
if (physical_end > addr)
- addr = physical_end - page_align (size);
+ addr = GRUB_EFI_PAGE_TRUNC (physical_end - page_align (size));
}
}
Mao, Bibo wrote:
Hi,
On grub efi platform, initrd image memory allocation start
address must be 4K alignment, else it will fail to alloc memory
for initrd image.
thanks
bibo,mao
diff -Nruap grub2.org/loader/i386/efi/linux.c grub2/loader/i386/efi/linux.c
--- grub2.org/loader/i386/efi/linux.c 2006-10-24 13:24:46.0 +0800
+++ grub2/loader/i386/efi/linux.c 2006-10-24 13:25:35.0 +0800
@@ -587,7 +587,6 @@ grub_rescue_cmd_initrd (int argc, char *
desc = NEXT_MEMORY_DESCRIPTOR (desc, desc_size))
{
if (desc->type == GRUB_EFI_CONVENTIONAL_MEMORY
- && desc->physical_start >= addr_min
&& desc->physical_start + size < addr_max
&& desc->num_pages >= initrd_pages)
{
@@ -598,7 +597,7 @@ grub_rescue_cmd_initrd (int argc, char *
physical_end = addr_max;
if (physical_end > addr)
- addr = physical_end - page_align (size);
+ addr = PAGE_DOWN(physical_end - page_align (size));
}
}
___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 3/3] grub EFI disk device enumberating
Johan Rydberg wrote:
Johan Rydberg <[EMAIL PROTECTED]> writes:
> This code can be shorter; you only have to compare the lengths. If
> they match, you can do a memcmp on the whole device path.
It could look something like this;
/* Returns zero if device path SUBPATH is a subpath of device path
PATH. */
static int
compare_subpath (const grub_efi_device_path_t *subpath,
const grub_efi_device_path_t *path)
{
if (! subpath || ! path)
return 1;
while (1)
{
int len, ret;
if (GRUB_EFI_END_ENTIRE_DEVICE_PATH (subpath))
return 0;
else if (GRUB_EFI_END_ENTIRE_DEVICE_PATH (path))
return 1;
if (GRUB_EFI_DEVICE_PATH_LENGTH (subpath)
!= GRUB_EFI_DEVICE_PATH_LENGTH (path))
return ((int) GRUB_EFI_DEVICE_PATH_LENGTH (subpath)
- (int) GRUB_EFI_DEVICE_PATH_LENGTH (path));
len = GRUB_EFI_DEVICE_PATH_LENGTH (path);
ret = grub_memcmp (subpath, path, len);
if (ret)
return ret;
path = (grub_efi_device_path_t *) ((char *) path + len);
subpath = (grub_efi_device_path_t *) ((char *) subpath + len);
}
}
I guess that should work at least, it is not tested.
I tested on my EFI IA32 bios, it works for me, your method is better
than me, I incorporated your function in my second patch.
thanks
bibo,mao
In gnufi I have a device_path_iterate function that could be used for
these kind of things. Maybe we should bring it in to GRUB2.
/* Iterate nodes of the device path. *PATHP should be set to point to
the path that is to be iterated. NULL will be returned when the
end of the path has been reached. */
efi_device_path_t *
efi_device_path_iterate (efi_device_path_t **pathp)
{
efi_device_path_t *p = *pathp;
if (EFI_END_ENTIRE_DEVICE_PATH (p))
return NULL;
else
{
efi_uint_t len = EFI_DEVICE_PATH_LENGTH (p);
*pathp = (efi_device_path_t *) (((char *) p) + len);
return p;
}
}
~j
--- grub2.org/disk/efi/efidisk.c2006-10-25 12:47:28.0 +0800
+++ grub2/disk/efi/efidisk.c2006-10-25 12:50:19.0 +0800
@@ -87,6 +87,39 @@ find_last_device_path (const grub_efi_de
return p;
}
+/* Returns zero if device path SUBPATH is a subpath of device path
+ PATH. */
+static int
+compare_subpath (const grub_efi_device_path_t *subpath,
+ const grub_efi_device_path_t *path)
+{
+ if (! subpath || ! path)
+return 1;
+
+ while (1)
+{
+ int len, ret;
+
+ if (GRUB_EFI_END_ENTIRE_DEVICE_PATH (subpath))
+return 0;
+ else if (GRUB_EFI_END_ENTIRE_DEVICE_PATH (path))
+return 1;
+
+ if (GRUB_EFI_DEVICE_PATH_LENGTH (subpath)
+ != GRUB_EFI_DEVICE_PATH_LENGTH (path))
+return ((int) GRUB_EFI_DEVICE_PATH_LENGTH (subpath)
+- (int) GRUB_EFI_DEVICE_PATH_LENGTH (path));
+
+ len = GRUB_EFI_DEVICE_PATH_LENGTH (path);
+ ret = grub_memcmp (subpath, path, len);
+ if (ret)
+return ret;
+
+ path = (grub_efi_device_path_t *) ((char *) path + len);
+ subpath = (grub_efi_device_path_t *) ((char *) subpath + len);
+}
+}
+
/* Compare device paths. */
static int
compare_device_paths (const grub_efi_device_path_t *dp1,
@@ -197,44 +230,6 @@ make_devices (void)
return devices;
}
-/* Find the parent device. */
-static struct grub_efidisk_data *
-find_parent_device (struct grub_efidisk_data *devices,
- struct grub_efidisk_data *d)
-{
- grub_efi_device_path_t *dp, *ldp;
- struct grub_efidisk_data *parent;
-
- dp = duplicate_device_path (d->device_path);
- if (! dp)
-return 0;
-
- ldp = find_last_device_path (dp);
- ldp->type = GRUB_EFI_END_DEVICE_PATH_TYPE;
- ldp->subtype = GRUB_EFI_END_ENTIRE_DEVICE_PATH_SUBTYPE;
- ldp->length[0] = sizeof (*ldp);
- ldp->length[1] = 0;
-
- for (parent = devices; parent; parent = parent->next)
-{
- /* Ignore itself. */
- if (parent == d)
- continue;
-
- if (compare_device_paths (parent->device_path, dp) == 0)
- {
- /* Found. */
- if (! parent->last_device_path)
- parent = 0;
-
- break;
- }
-}
-
- grub_free (dp);
- return parent;
-}
-
static int
iterate_child_devices (struct grub_efidisk_data *devices,
struct grub_efidisk_data *d,
@@ -305,63 +300,6 @@ static void
name_devices (struct grub_efidisk_data *devices)
{
struct grub_efidisk_data *d;
-
- /* First, identify devices by media device paths. */
- for (d = devices; d; d = d->next)
-{
- grub_efi_device_path_t *dp;
-
- dp = d->last_device_path;
- if (! dp)
- continue;
-
- if (GRUB_EFI_DEVICE_PATH_TYPE (dp) == GRUB_EFI_MEDIA_DEVICE_PATH_TYPE)
- {
- int is_hard_drive = 0;
-
- switch (GRUB_EFI_DEVICE_PATH_SUBTYPE (dp))
- {
- case GRUB_EFI_HARD_DRIVE_DEVICE_PATH_SUBTYPE:
- is_hard_drive = 1;
- /* Fall throug
Re: [PATCH] generic ELF loading
On Tuesday 24 October 2006 22:03, Hollis Blanchard wrote: > Actually I'm not using the heap, I'm just directly copying wherever > phdr->p_paddr says to. That's not a good thing actually; in the future > we should add some error checking to make sure we don't clobber GRUB > itself. OK, then it's even worse. :( You must not assume that GRUB can always load an OS image to an appropriate location directly. I know this is the case for the current implementation of the Multiboot loader, but it is a very bad idea, generally speaking. What GRUB should do is first to load an image to somewhere then relocate it to the right place at boot time. On i386-pc, the OS area is used for this very purpose. The x86 Multiboot loader is just a mistake, and that's why we must rewrite it. Okuji ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
