Re: New patch for video subsystem...
Vesa Jääskeläinen <[EMAIL PROTECTED]> writes: > Here is the newest modifications to video subsystem. Hi Vesa. What a suitable name :) Sorry for the delay, and lack of earlier comments, but I've been out of the GRUB loop for a while. But I have a few comments on the video subsystem; Correct me if I am wrong here, but what you call "render target" seems to be what other video systems call "surface" or "drawable". Why use the concept of a "active" render target? Why not instead let all functions that operate on the active render target take a pointer to a specific render target? Poking through your patch, it seems that there a lot of the following (forgive my pseudo-code) grub_video_set_active_render_target (target); // .. fill it with something .. grub_video_fill_rect (color, 0, 0, width, height); grub_video_set_active_render_target (GRUB_VIDEO_RENDER_TARGET_DISPLAY); grub_video_blit_render_target (target, 0, 0, 0, 0, width, height); I would feel more comfortable with the following workflow: grub_video_fill_rect (target, color, 0, 0, width, height); grub_video_blit_render_target (GRUB_VIDEO_RENDER_TARGET_DISPLAY, target, 0, 0, 0, 0, width, height); Also, I think it is important the user can get hold of a pointer to the render targets data, and an exact pixel format, to do private rendering. It would be hard to make a perfect gradient using fill_rect. I'm not sure the concept of "viewports" are needed at all; instead let the 'application' (e.g, the terminal) render into a render target, and blit that to the screen at the desired position. To minimize memory, the videport-render target can be a sub-render target of the main render target (ie, they share the same buffer) unless the user wants any fancy stuff like a background picture. Thanks, Johan ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: New patch for video subsystem...
Vesa Jääskeläinen <[EMAIL PROTECTED]> writes: > But the idea here was to make it easier to make function that operates > on different render targets without knowing about it. eg: > > set_target a > call common_func > set target b > call common_func > set target c > > With your proposal this would require to pass this render target pointer > all around and would make video function calls longer. I do not see a problem with that. The overhead for passing of an extra argument is probably less than the overhead of the function calls (esp since set_active_render_target is an indirect call) > There is also another problem in your proposal. What happens if video > driver get's changed and you still have pointers to render targets (or > to raw memory)? Possibly the same thing as with the current method; I do not see how this would make things different? >> Also, I think it is important the user can get hold of a pointer to >> the render targets data, and an exact pixel format, to do private >> rendering. It would be hard to make a perfect gradient using fill_rect. > > That might not be possible on all architectures. That's the reason there > is a bitmap support (which I am working on currently). Bitmaps are > located on host memory so they can be modified. I'm not sure "bitmap" is a good word here; maybe "image" would be better. > Render targets can be located on host memory or on video memory > depending on arch. If they are located on video memory then there is > only need to have video-video blit functionality and some archs might > even provide hardware accelerated helper functions. If the architecture does not support direct access to the video memory, it just falls back on returning a render target with malloc()ed memory. > One idea of the render targets are that they hide the real bitmap data > format. So one could make a 24 bit RGB picture and then just blit it to > render target and underlying video driver would convert it to correct > format. This frees API user from thinking all possible formats and can > concentrate to make one working implementation. Iter-format blits can of course also be supported if you have direct access to the render target pixel data. > If one needs to modify render target on all platforms there should be > support to direct access to memory or ability to read contents of frame > buffer to host memory and then write it back or requirement to store > data on host memory and then do blit from there to video memory, but > that is a bit costy on performance. I do not see why that should be needed. Have you looked at for example DirectFB's API? Even through their coding style is fucked up, the overall design of the API is quite nice. I'm just afraid that we will see the exact same thing that happened to legacy GRUB; a lot of distribution vendors hack up something of their own. Therefor it is important that we get this right. ~j ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH] THANKS: Update Johan Rydberg's email address.
Marco, Okuji, could someone of you maybe commit this? (Yes, I _KNOW_ I'm using the address I'm changing from, but I'm too lazy to re-subscribe to the list with "new" address.) 2006-06-30 Johan Rydberg <[EMAIL PROTECTED]> * THANKS: Update Johan Rydberg's email address. Index: ChangeLog === RCS file: /sources/grub/grub2/ChangeLog,v retrieving revision 1.284 diff -u -r1.284 ChangeLog --- ChangeLog 13 Jun 2006 22:50:00 - 1.284 +++ ChangeLog 30 Jun 2006 16:04:30 -0000 @@ -1,3 +1,7 @@ +2006-06-30 Johan Rydberg <[EMAIL PROTECTED]> + + * THANKS: Update Johan Rydberg's email address. + 2006-06-14 Yoshinori K. Okuji <[EMAIL PROTECTED]> * util/misc.c: Include config.h. Index: THANKS === RCS file: /sources/grub/grub2/THANKS,v retrieving revision 1.13 diff -u -r1.13 THANKS --- THANKS 13 Jun 2006 22:50:00 - 1.13 +++ THANKS 30 Jun 2006 16:04:30 - @@ -10,7 +10,7 @@ Harley D. Eades III <[EMAIL PROTECTED]> Hollis Blanchard <[EMAIL PROTECTED]> Jeroen Dekkers <[EMAIL PROTECTED]> -Johan Rydberg <[EMAIL PROTECTED]> +Johan Rydberg <[EMAIL PROTECTED]> Marco Gerards <[EMAIL PROTECTED]> NIIBE Yutaka <[EMAIL PROTECTED]> Omniflux <[EMAIL PROTECTED]> ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] THANKS: Update Johan Rydberg's email address.
Johan Rydberg <[EMAIL PROTECTED]> writes: > --- ChangeLog 13 Jun 2006 22:50:00 - 1.284 > +++ ChangeLog 30 Jun 2006 16:04:30 - I did not mean to send in the patch for ChangeLog. Sorry for that. ~j ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH] Do not require console control protocol.
Hi, I would like to have the following patch committed. Some EFI implementations do not provide a console control protocol instance, and it is covered by neither EFI or UEFI specifications. ~j 2006-07-28 Johan Rydberg <[EMAIL PROTECTED]> * kern/efi/efi.c (grub_efi_set_text_mode): Assume console already is in text mode if there is no console control protocol instance available. Index: kern/efi/efi.c === RCS file: /sources/grub/grub2/kern/efi/efi.c,v retrieving revision 1.7 diff -u -r1.7 efi.c --- kern/efi/efi.c 27 May 2006 21:09:25 - 1.7 +++ kern/efi/efi.c 27 Jul 2006 22:01:49 - @@ -125,7 +125,9 @@ c = grub_efi_locate_protocol (&console_control_guid, 0); if (! c) -return 0; +/* No console control protocol instance available, assume it is + already in text mode. */ +return 1; if (c->get_mode (c, &mode, 0, 0) != GRUB_EFI_SUCCESS) return 0; ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Do not require console control protocol.
"Yoshinori K. Okuji" <[EMAIL PROTECTED]> writes: >> I would like to have the following patch committed. Some EFI >> implementations do not provide a console control protocol instance, >> and it is covered by neither EFI or UEFI specifications. > > Right, right. My code was only for testing, so it was not good, generally > speaking. BTW, can I add you into the project members of GRUB so that you can > commit patches yourself? Yes, please do. ~j ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH] Let GCC generate deps with -MD.
Hi,
Instead of generating the dependencies separately it can be done the
first time a file is compiled, with the -MD option to GCC.
Comments?
2006-08-12 Johan Rydberg <[EMAIL PROTECTED]>
* genmk.rb: Let GCC generate dependenceies the first time it
compiles a file; using the -MD option.
Index: genmk.rb
===
RCS file: /sources/grub/grub2/genmk.rb,v
retrieving revision 1.25
diff -u -r1.25 genmk.rb
--- genmk.rb 2 Jun 2006 19:33:58 - 1.25
+++ genmk.rb 12 Aug 2006 15:47:35 -
@@ -71,14 +71,7 @@
dir = File.dirname(src)
"#{obj}: #{src}
- $(TARGET_CC) -I#{dir} -I$(srcdir)/#{dir} $(TARGET_CPPFLAGS) #{extra_flags} $(TARGET_#{flag}) $(#{prefix}_#{flag}) -c -o $@ $<
-
-#{dep}: #{src}
- set -e; \
- $(TARGET_CC) -I#{dir} -I$(srcdir)/#{dir} $(TARGET_CPPFLAGS) #{extra_flags} $(TARGET_#{flag}) $(#{prefix}_#{flag}) -M $< \
- | sed 's,#{Regexp.quote(fake_obj)}[ :]*,#{obj} $@ : ,g' > $@; \
- [ -s $@ ] || rm -f $@
-
+ $(TARGET_CC) -I#{dir} -I$(srcdir)/#{dir} $(TARGET_CPPFLAGS) #{extra_flags} $(TARGET_#{flag}) $(#{prefix}_#{flag}) -MD -c -o $@ $<
-include #{dep}
"
@@ -153,14 +146,7 @@
dir = File.dirname(src)
"#{obj}: #{src}
- $(TARGET_CC) -I#{dir} -I$(srcdir)/#{dir} $(TARGET_CPPFLAGS) $(TARGET_#{flag}) $(#{prefix}_#{flag}) -c -o $@ $<
-
-#{dep}: #{src}
- set -e; \
- $(TARGET_CC) -I#{dir} -I$(srcdir)/#{dir} $(TARGET_CPPFLAGS) $(TARGET_#{flag}) $(#{prefix}_#{flag}) -M $< \
- | sed 's,#{Regexp.quote(fake_obj)}[ :]*,#{obj} $@ : ,g' > $@; \
- [ -s $@ ] || rm -f $@
-
+ $(TARGET_CC) -I#{dir} -I$(srcdir)/#{dir} $(TARGET_CPPFLAGS) $(TARGET_#{flag}) $(#{prefix}_#{flag}) -MD -c -o $@ $<
-include #{dep}
CLEANFILES += #{command} #{fs}
@@ -213,14 +199,7 @@
dir = File.dirname(src)
"#{obj}: #{src}
- $(CC) -I#{dir} -I$(srcdir)/#{dir} $(CPPFLAGS) $(CFLAGS) -DGRUB_UTIL=1 $(#{prefix}_CFLAGS) -c -o $@ $<
-
-#{dep}: #{src}
- set -e; \
- $(CC) -I#{dir} -I$(srcdir)/#{dir} $(CPPFLAGS) $(CFLAGS) -DGRUB_UTIL=1 $(#{prefix}_CFLAGS) -M $< \
- | sed 's,#{Regexp.quote(fake_obj)}[ :]*,#{obj} $@ : ,g' > $@; \
- [ -s $@ ] || rm -f $@
-
+ $(CC) -I#{dir} -I$(srcdir)/#{dir} $(CPPFLAGS) $(CFLAGS) -DGRUB_UTIL=1 $(#{prefix}_CFLAGS) -MD -c -o $@ $<
-include #{dep}
"
@@ -258,14 +237,7 @@
dir = File.dirname(src)
"#{obj}: #{src}
- $(TARGET_CC) -I#{dir} -I$(srcdir)/#{dir} $(TARGET_CPPFLAGS) $(TARGET_CFLAGS) $(#{prefix}_CFLAGS) -c -o $@ $<
-
-#{dep}: #{src}
- set -e; \
- $(TARGET_CC) -I#{dir} -I$(srcdir)/#{dir} $(TARGET_CPPFLAGS) $(TARGET_CFLAGS) $(#{prefix}_CFLAGS) -M $< \
- | sed 's,#{Regexp.quote(fake_obj)}[ :]*,#{obj} $@ : ,g' > $@; \
- [ -s $@ ] || rm -f $@
-
+ $(TARGET_CC) -I#{dir} -I$(srcdir)/#{dir} $(TARGET_CPPFLAGS) $(TARGET_CFLAGS) $(#{prefix}_CFLAGS) -MD -c -o $@ $<
-include #{dep}
"
___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
Re: EFI disk probing problem
"Yoshinori K. Okuji" <[EMAIL PROTECTED]> writes: > I don't remember precisely, but I think I only added disk devices but not > partition devices. On your system, don't disk devices as well as partition > devices get enumerated? I'm not 100% sure about this, but I think EFI does not have to add a disk device node if there is only a single logical partition on the disk. ~j ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: moving away from CVS
Marco Gerards <[EMAIL PROTECTED]> writes: >> I agree; I think there are far better replacements for CVS. Personally I >> would highly recommend mercurial, but I suppose everybody has their own >> preferences. I noticed that Savannah, where the GRUB tree is currently >> hosted, offers subversion support... > > I like subversion because it solves most problems, while I do not have > to learn something new. Subversion still does not track merges, which I think was the original reason to why this discussion started. Personally I would rather use bzr, but I know our maintainer does not like it that much (because of the lack of commit hooks.) But I wouldn't mind switching to subversion, keep in mind that it will not be any better than CVS. ~j ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Let core developers "vote" to accept patches.
Hi, In other projects that I've worked on, we've used a system where the core developers can either give a +1 or a -1 on a contributed patch. If a patch receives two +1's or more, it is accepted (if there are no -1's) and committed by one of the developers. This lowers the pressure on the maintainers, and hopefully will speed up the development. Though, the core developers need to be a bit careful. They should not give +1's or -1's on a patch that they either (1) do not fully understand, or that (2) applies to a section of the code that they are not knowledgeable about. It is then better to either state that they do not know the area well enough, and maybe give the patch a +0, or be silent. The downside is that some developers will never feel that they are knowledgeable enough to comment on a contribution; resulting in that the maintainer will have to do it. The same situation we are in now, more or less. The upside is that the maintainers can focus on other things than just reviewing patches; maybe getting some new features in. Also, trivial and small patches will get quicker review and get merged into the tree in a swift manner. Please note: This is in no way an attempt to overrun any of our maintainers, it is just an attempt to make things go a bit smoother and faster in the times when the maintainers are busy with other things. Please note: I'm not saying I'm in any way a core developer, just sharing my experiences from other projects. Okuji, Marco, others; comments? ~j ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Let GCC generate deps with -MD.
"Yoshinori K. Okuji" <[EMAIL PROTECTED]> writes: > On Saturday 12 August 2006 18:07, Johan Rydberg wrote: >> Instead of generating the dependencies separately it can be done the >> first time a file is compiled, with the -MD option to GCC. > > That sounds good to me. Committed, with the *.mk files regenerated. Thanks, Johan. ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: Let core developers "vote" to accept patches.
"Yoshinori K. Okuji" <[EMAIL PROTECTED]> writes: > Our rules are: > > - When you are a committer and a patch is trivial enough, you can > (and should) check in the patch by yourself. > > - If not, you should ask others in grub-devel. It was for these two scenarios I suggested we could vote on patches. But it might not be needed. I was after all just a suggestion. > - I retain final words as the designer of GRUB 2. For some certain > parts, I completely rely on other developers. For instance, Hollis > would even precede me in PowerPC-specific changes. Agreed. > - If a change is involved with political issues, official > maintainers must approve or disapprove. This is required for the GNU > Project. Of course. ~j ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: include/grub/script.h:27:29: grub_script.tab.h: No such file or director
Marco Gerards <[EMAIL PROTECTED]> writes: >> The last CVS dated June 14 2006 compiles fine. > > Sorry, but I do not really understand you. Which CVS snapshot do you > mean? With the last statement you mean your problem was fixed in CVS? I think he means that a CVS checkout of June 14 worked just fine, and the problem what introduced after that. ~ j ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: CVS fails to build
Robert Millan <[EMAIL PROTECTED]> writes: >> I'll commit it in a few days, or when you (or someone else) confirms >> that it works. > > Confirmed. Committed. ~j ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: Grub for ia64
[EMAIL PROTECTED] writes: > this is a port of grub2 to ia64. ia64 systems (itanium) are EFI > based so this port reuse existing EFI infrastructure. Thank you for offering this contribution. First a few legal comments. I poked through the patches, and it seems that there are a few files that are copyrighted by Hewlett-Packard and David Mosberger-Tang. All developers of GNU GRUB has agreed to sign over their copyright to FSF, so brining non-FSF copyrighted code into the project is a problem (and likely a show-stopper). There are also a few files that are released under LGPL. Maintainers, are there any problems with bringing such files into GNU GRUB? It is not possible to find similar integer division code in glibc for example? And why is this code needed at all, doesn't IA64 have division? If it doesn't, my gut feeling tells me that this should be in libgcc. There were also a few files without any copyright notice what so ever (trampoline.S being one). Please add a boilerplate. I noticed that there were a generic fix for grub_strtoull in the fat patch; maybe you could send that, and other generic fixes as separate patches? Also, do not forget ChangeLog [1] entries for all your changes. If you are not familiar with writing ChangeLogs, we'll of course help you with that. Have you signed over copyright to FSF for your work on GNU GRUB? In other words, have you sent in an assignment record to FSF? If not, let us know and we'll send you a request record. Anyhow, when I get a few more minutes over I'll try to review your patches more in depth. I do not know if there is a policy for how to contribute code to GRUB, but please send unified diffs (-u) instead of context diffs (-c). I at least find them easier to read. ~j [1] http://www.gnu.org/prep/standards/html_node/Change-Logs.html pgpuJqKuDuLuM.pgp Description: PGP signature ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: patch for kern/efi/mm.c (big memmap)
[EMAIL PROTECTED] writes:
> some systems have a really big memmap. This patch remove the memmap
> size limit.
Overall the patch looks good, I have one comment through; (the patch
is for kern/efi/mm.c if someone wonders.)
+ memory_map = grub_efi_allocate_pages
+(0, 2 * BYTES_TO_PAGES (memory_map_size + 0x1000));
I suppose you add 0x1000 to round it up. Maybe we should change the
BYTES_TO_PAGES macro to do roundup.
I'll attach a unified diff, since those is easier to review.
Even through this patch is quite trivial and small, I believe we need
you to sign over the copyright for it to FSF. Okuji, Marco, what are
your opinions?
~j
Index: kern/efi/mm.c
===
RCS file: /sources/grub/grub2/kern/efi/mm.c,v
retrieving revision 1.3
diff -u -r1.3 mm.c
--- kern/efi/mm.c 28 May 2006 23:01:43 - 1.3
+++ kern/efi/mm.c 28 Sep 2006 13:29:16 -
@@ -30,10 +30,6 @@
#define BYTES_TO_PAGES(bytes) ((bytes) >> 12)
#define PAGES_TO_BYTES(pages) ((pages) << 12)
-/* The size of a memory map obtained from the firmware. This must be
- a multiplier of 4KB. */
-#define MEMORY_MAP_SIZE 0x1000
-
/* Maintain the list of allocated pages. */
struct allocated_page
{
@@ -346,6 +342,8 @@
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
@@ -356,15 +354,20 @@
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));
+ memory_map_size = 0;
+ res = grub_efi_get_memory_map (&memory_map_size, NULL, 0, &desc_size, 0);
+ if (res != 0)
+grub_fatal ("cannot get memory map size");
+
+ memory_map = grub_efi_allocate_pages
+(0, 2 * BYTES_TO_PAGES (memory_map_size + 0x1000));
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, memory_map_size);
/* Obtain descriptors for available memory. */
- map_size = MEMORY_MAP_SIZE;
+ map_size = memory_map_size;
if (grub_efi_get_memory_map (&map_size, memory_map, 0, &desc_size, 0) < 0)
grub_fatal ("cannot get memory map");
@@ -373,7 +376,7 @@
filtered_memory_map_end = filter_memory_map (memory_map, filtered_memory_map,
desc_size, memory_map_end);
-
+
/* By default, request a quarter of the available memory. */
total_pages = get_total_pages (filtered_memory_map, desc_size,
filtered_memory_map_end);
@@ -393,7 +396,7 @@
#if 0
/* For debug. */
- map_size = MEMORY_MAP_SIZE;
+ map_size = memory_map_size;
if (grub_efi_get_memory_map (&map_size, memory_map, 0, &desc_size, 0) < 0)
grub_fatal ("cannot get memory map");
@@ -406,7 +409,7 @@
/* Release the memory maps. */
grub_efi_free_pages ((grub_addr_t) memory_map,
- 2 * BYTES_TO_PAGES (MEMORY_MAP_SIZE));
+ 2 * BYTES_TO_PAGES (memory_map_size + 0x1000));
}
void
pgpVb9rWYYhjw.pgp
Description: PGP signature
___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
Re: Grub for ia64
[EMAIL PROTECTED] writes: >> It is not possible to find similar integer division code in glibc for >> example? And why is this code needed at all, doesn't IA64 have >> division? > No, it doesn't have integer division. > >> If it doesn't, my gut feeling tells me that this should be >> in libgcc. > Yes, most should be either in glibc or libgcc. I have not verified this completely, but it seems as libgcc has all these routines at hand. GCC also has the following IA-64 options that might be usefull; `-minline-int-divide-min-latency' Generate code for inline divides of integer values using the minimum latency algorithm. `-minline-int-divide-max-throughput' Generate code for inline divides of integer values using the maximum throughput algorithm. ~j pgp076emLpWSW.pgp Description: PGP signature ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: patch for kern/efi/mm.c (big memmap)
Marco Gerards <[EMAIL PROTECTED]> writes:
>> + memory_map = grub_efi_allocate_pages
>> +(0, 2 * BYTES_TO_PAGES (memory_map_size + 0x1000));
>>
>> I suppose you add 0x1000 to round it up. Maybe we should change
>> the BYTES_TO_PAGES macro to do roundup.
>
> Agreed. Can you make this change as well?
Done. See attached patch.
>> Even through this patch is quite trivial and small, I believe we
>> need you to sign over the copyright for it to FSF. Okuji, Marco,
>> what are your opinions?
>
> This patch is trivial enough to apply without the paperwork, I
> think.
OK.
I'll commit it after I get a few minutes over to test it.
~j
Index: kern/efi/mm.c
===
RCS file: /sources/grub/grub2/kern/efi/mm.c,v
retrieving revision 1.3
diff -u -p -r1.3 mm.c
--- kern/efi/mm.c 28 May 2006 23:01:43 - 1.3
+++ kern/efi/mm.c 28 Sep 2006 13:57:30 -
@@ -27,13 +27,9 @@
#define NEXT_MEMORY_DESCRIPTOR(desc, size) \
((grub_efi_memory_descriptor_t *) ((char *) (desc) + (size)))
-#define BYTES_TO_PAGES(bytes) ((bytes) >> 12)
+#define BYTES_TO_PAGES(bytes) (((bytes) + 4095) >> 12)
#define PAGES_TO_BYTES(pages) ((pages) << 12)
-/* The size of a memory map obtained from the firmware. This must be
- a multiplier of 4KB. */
-#define MEMORY_MAP_SIZE 0x1000
-
/* Maintain the list of allocated pages. */
struct allocated_page
{
@@ -346,6 +342,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 +353,23 @@ 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 = 0;
+ res = grub_efi_get_memory_map (&memory_map_size, NULL, 0, &desc_size, 0);
+ if (res != 0)
+grub_fatal ("cannot get memory map size");
+
+ memory_map = grub_efi_allocate_pages
+(0, 2 * BYTES_TO_PAGES (memory_map_size));
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, memory_map_size);
/* Obtain descriptors for available memory. */
- map_size = MEMORY_MAP_SIZE;
+ map_size = memory_map_size;
if (grub_efi_get_memory_map (&map_size, memory_map, 0, &desc_size, 0) < 0)
grub_fatal ("cannot get memory map");
@@ -393,7 +398,7 @@ grub_efi_mm_init (void)
#if 0
/* For debug. */
- map_size = MEMORY_MAP_SIZE;
+ map_size = memory_map_size;
if (grub_efi_get_memory_map (&map_size, memory_map, 0, &desc_size, 0) < 0)
grub_fatal ("cannot get memory map");
@@ -406,7 +411,7 @@ grub_efi_mm_init (void)
/* Release the memory maps. */
grub_efi_free_pages ((grub_addr_t) memory_map,
- 2 * BYTES_TO_PAGES (MEMORY_MAP_SIZE));
+ 2 * BYTES_TO_PAGES (memory_map_size));
}
void
pgpn2pvoWdXa0.pgp
Description: PGP signature
___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
Re: Fix grub_strtoull
[EMAIL PROTECTED] writes: > 2006-09-28 Tristan Gingold <[EMAIL PROTECTED]> > > * kern/misc.c (grub_strtoull): guess the base only if not specified. > Committed. pgp9sQVY4rill.pgp Description: PGP signature ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: Scripting (IMPORTANT!)
Marco Gerards <[EMAIL PROTECTED]> writes:
> Iterating over files:
> for x in (hd0,3)/foo/* ; do commands ; done
I must say I would prefer this over the other ("foreach") suggestion,
since I'm more used to it.
Anyhow, is your plan to make expansion available through out the whole
shell, or only for "for"? It would be nice to be able to do;
grub> ls (*,*)/boot/vmlinuz*
> One special purpose variable $ERROR will be added. It will contain
> the error strings when $? is set to non-zero. In that case you can do
> error handling in scripts.
I'm afraid there might be situations where the user do not realize
that $ERROR has been overwritten. Bogus example;
if cat (hd0,1)/foo/bar; then
ls (hd0,1)/foo
echo "*** 'bar' is missing: $ERROR"
fi
The execution of 'ls' will overwrite $ERROR.
~j
pgpqCUCKkZeL9.pgp
Description: PGP signature
___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
Re: Scripting (IMPORTANT!)
Marco Gerards <[EMAIL PROTECTED]> writes: > = > Variables > = > > - root: The active disk/partition (rw, global) > ... What about a 'prompt' variable? ~j pgpf6nhSSgEl9.pgp Description: PGP signature ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: Grub for ia64
Johan Rydberg <[EMAIL PROTECTED]> writes: > Anyhow, when I get a few more minutes over I'll try to review your > patches more in depth. A few comments; * Would it be possible for you to rewrite kern/ia64/efi/startup.S, so that you hold the copyright for it? * Same goes for kern/ia64/efi/reloc_ia64.S. (Or maybe it is possible to find similar code in for example GNU libc, that is already copyrighted to FSF.) * Most of the math functions in kern/ia64 should be available in libgcc. Could you try to link it against "gcc -print-libgcc-file-name"? ~j pgpmGj9wr7G5L.pgp Description: PGP signature ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: Grub for ia64
Jeff Bailey <[EMAIL PROTECTED]> writes: >> > Anyhow, when I get a few more minutes over I'll try to review your >> > patches more in depth. >> >> A few comments; >> >> * Would it be possible for you to rewrite kern/ia64/efi/startup.S, >>so that you hold the copyright for it? >> >> * Same goes for kern/ia64/efi/reloc_ia64.S. > > Who holds the copyright now? If it's folks at HP, we can probably get a > disclaimer for it. Some files are both copyrighted by both HP and Mosberger-Tang: + * Copyright (C) 2000 Hewlett-Packard Co + * Copyright (C) 2000 David Mosberger-Tang <[EMAIL PROTECTED]> Files that are from the GNU EFI package seem to be just copyrighted by HP: +Copyright (C) 1999 Hewlett-Packard Co. + Contributed by David Mosberger <[EMAIL PROTECTED]>. How would one go about to get a disclaimer for them? ~j pgpXbo6T20SP6.pgp Description: PGP signature ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: Grub for ia64
Tristan Gingold <[EMAIL PROTECTED]> writes: > the code to relocate is used by grub to relocate itself. On EFI systems > programs can be loaded at any address (physical mode). You're telling me that EFI on your IA-64 system does not relocate the program before start executing it? That sounds strange. ~j pgpJOM8ct8lut.pgp Description: PGP signature ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: Grub for ia64
Tristan Gingold <[EMAIL PROTECTED]> writes: >> You're telling me that EFI on your IA-64 system does not relocate the >> program before start executing it? That sounds strange. > > It does relocate EFI PEI images. Unfortunatly there is no tools to create > EFI PEI objects on Linux. The gnu efi tools are kludgy: they use standard > gcc and ld, create an ELF image and convert it to a PEI image. But the > relocations are not converted, they are simply put into the data section. How hard would it be to write a converter ourselves, starting with util/i386/efi/grub-mkimage.c? ~j pgprBujjv0uKO.pgp Description: PGP signature ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: Grub for ia64
Tristan Gingold <[EMAIL PROTECTED]> writes: > On Thu, Oct 12, 2006 at 02:11:57PM +0200, Johan Rydberg wrote: >> Tristan Gingold <[EMAIL PROTECTED]> writes: >> >> >> You're telling me that EFI on your IA-64 system does not relocate the >> >> program before start executing it? That sounds strange. >> > >> > It does relocate EFI PEI images. Unfortunatly there is no tools to create >> > EFI PEI objects on Linux. The gnu efi tools are kludgy: they use standard >> > gcc and ld, create an ELF image and convert it to a PEI image. But the >> > relocations are not converted, they are simply put into the data section. >> >> How hard would it be to write a converter ourselves, starting with >> util/i386/efi/grub-mkimage.c? > Not that hard, but > * we'd better to have one grub-mkimage.c for all EFI targets I agree. > * working on binutils is even better. The key problem with this as I see it is that it would force people to have a cross-compiler suite installed, at least for i386 hosts. Unless we can magically get the default installation to include PE-support. ~j pgpnKsdi1NXN0.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
"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
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 #3
Hollis Blanchard <[EMAIL PROTECTED]> writes:
> --- grub2-cvs.orig/include/grub/types.h 2006-10-31 19:06:47.0
> -0600
> +++ grub2-cvs/include/grub/types.h2006-10-31 19:06:58.0 -0600
> @@ -23,6 +23,8 @@
> #include
> #include
>
> +#define __unused __attribute__ ((unused))
> +
First when I saw this it made me wanna comment on it, and tell you
that defines should all be in uppercase. But then I poked through the
code, and saw the usage of it, and it looked quite nice, with the
exception of the __-prefix. There are a lot of "common" words that
are keywords in C; inline, const and auto for example. Maybe we
should make "unused" a reserved keyword in GRUB? That would allow
us to write code like;
int
grub_foo (int x, unused int foo)
{
...
}
Just a thought. Otherwise your patch looked just fine Hollis.
~j
pgp4SWqfReJ2K.pgp
Description: PGP signature
___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
Re: __unused
Hollis Blanchard <[EMAIL PROTECTED]> writes:
>> int
>> grub_foo (int x, unused int foo)
>> {
>> ...
>> }
>
> I believe gcc attributes can't be used as naturally as this; it would
> have to be "int unused foo" or "int unused *foo" (from memory).
[EMAIL PROTECTED]:~/tmp/t5 $ gcc -Dunused= -Wall -Wunused-parameter -O2 -c t1.c
t1.c: In function 'foo':
t1.c:6: warning: unused parameter 'x'
t1.c:6: warning: unused parameter 'baz'
[EMAIL PROTECTED]:~/tmp/t5 $ gcc -Wall -Wunused-parameter -O2 -c t1.c
t1.c: In function 'foo':
t1.c:6: warning: unused parameter 'x'
[EMAIL PROTECTED]:~/tmp/t5 $ cat t1.c
#ifndef unused
# define unused __attribute__((unused))
#endif
int
foo (int x, unused int baz)
{
return 0;
}
~j
pgpRZgCTKgf01.pgp
Description: PGP signature
___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
Re: __unused
Jeroen Dekkers <[EMAIL PROTECTED]> writes: > Any ideas for a new name for the define? Something like "grub_unused"? "UNUSED" ? ~j pgpvJUkjmvaQn.pgp Description: PGP signature ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: multiboot2: make multiboot header optional
Hollis Blanchard <[EMAIL PROTECTED]> writes: > For kernels that need to communicate information to GRUB (e.g. > "vga_mode" from my previous email, or a.out load addresses), the > multiboot header would be needed for GRUB to locate the parameter area > within the executable. My two cents; In MB2 we remove the possibility to communicate options from the kernel to the boot loader. The loader has one task; loading the kernel and leave control to it, and possible pass information about the environment. Nothing more. If the operating system kernel is stupid enough to require as special video mode the user should be aware of that and setup the bootloader so that it is in that mode before the kernel is started. ~j pgpENtVJS4k0b.pgp Description: PGP signature ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: OpenSolaris and multiboot
Joe Bonasera <[EMAIL PROTECTED]> writes: > Hollis Blanchard wrote: > >> GRUB2's 'multiboot' command will load ELF64 files, so the "a.out >> hack" >> shouldn't be necessary for you in the future. > > That's fine and will let us delete a small amount of code from both > the kernel startup sequence and the tools used to build kernel. > > Just to be clear. For grub2 on x86, will it be true that there is only > a single grub2 binary that understands both elf32 and elf64? Yes. > Would the different tags i86-pc vs x86_64 (if any) be determined by > knowing if which type elf gets loaded? My personal opinion is that we should not pass any information to the kernel about what format it is provided in. If the image is an ELF64, it will be loaded as an ELF64. So in your case, for x86_64 hosts, provide the kernel as an ELF64 image. For IA-32, provide an ELF32 image. ~j pgpGVaglOicCK.pgp Description: PGP signature ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: multiboot2: variable data size
"Yoshinori K. Okuji" <[EMAIL PROTECTED]> writes: > On Saturday 25 November 2006 04:33, Hollis Blanchard wrote: >> That's exactly the point: there will be no difference. Both >> architectures will use 64-bit types. > > No. Both should use 32-bit, because GRUB transfers control in 32-bit mode. > Passing 64-bit addresses would be useless in this case. Note that the memory > map information is 64-bit even in the previous version of Multiboot Spec. That will not be true for x86_64-efi, which will have to run in long mode. ~j pgpllOc4H2Fni.pgp Description: PGP signature ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: multiboot2: variable data size
"bibo,mao" <[EMAIL PROTECTED]> writes: > If kernel image is bzImage, x64 efi bootloader need switch to 32 bit > protect mode(or real mode) from 64 bit long mode, and if kernel > image is gzipped/plain format, efi bootloader can directly jump to > 64-bit kernel entry address without mode switch. My opinion is that bzImages should be avoided on EFI platforms, or the decompress-code in Linux has to be rewritten. It took me a good couple of hours of debugging to find out that Linux simply ignores the memory layout and assumes that low memory is free to use as it likes. ~j pgp5cBeUfo6bB.pgp Description: PGP signature ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: Google Summer of Code 2007
"Yoshinori K. Okuji" <[EMAIL PROTECTED]> writes: > As you may know, Google Summer of Code 2007 is about starting: Are we suggesting any tasks that could be done by the student(s)? ~j pgpM0fH6Xw1ri.pgp Description: PGP signature ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: GRUB 1.95 cannot read the ufs filesystem
"Hitoshi Ozeki" <[EMAIL PROTECTED]> writes: > GRUB 1.95 cannot read the ufs filesystem. > Because, 1.95 mistakes a usage of char pointer. > This patch fixes this problem, and make it a bit better. Thanks for you contribution. > Attached file: ufs.patch.gz (1Kbytes) In the future, could you maybe attach patches uncompressed, and inlined (or with mime-type text/plain or similar) so we can review the patch directly within the mail, and give you comments. Thanks again, Johan. pgpG2QkJalLV9.pgp Description: PGP signature ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: GRUB 1.95 cannot read the ufs filesystem
"Hitoshi Ozeki" <[EMAIL PROTECTED]> writes: > Hello, all. > > GRUB 1.95 cannot read the ufs filesystem. > Because, 1.95 mistakes a usage of char pointer. > This patch fixes this problem, and make it a bit better. Could you maybe point out the problem for us? Personally, I think the use of pointers make the code more complex. ~j pgpS912Q70CM2.pgp Description: PGP signature ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: GRUB 1.95 cannot read the ufs filesystem
"Hitoshi Ozeki" <[EMAIL PROTECTED]> writes:
> At first, When we use the 'strcmp' for the purpose of comparision C-strings,
> It requires to terminate with the NIL sentry('\0').
>
> begin---
> grub_ufs_find_file (struct grub_ufs_data *data, const char *path)
> {
> char fpath[grub_strlen (path)]; <-- not enough.
> char *name = fpath;
> char *next;
> unsigned int pos = 0;
> int dirino;
>
> grub_strncpy (fpath, path, grub_strlen (path)); <--without NIL.
> end-
It feels easier to just add +1 at both the location. Can anyone else
comment on this, please?
> The '.label' should set to 0.
> On the original code, The 'label' function returns the invalid pointer,
> so the 'ls -l' command gets wrong.
Marco?
~j
pgpMzNdP3VGQQ.pgp
Description: PGP signature
___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
Re: GRUB 1.95 cannot read the ufs filesystem
"Hitoshi Ozeki" <[EMAIL PROTECTED]> writes:
> At first, When we use the 'strcmp' for the purpose of comparision C-strings,
> It requires to terminate with the NIL sentry('\0').
>
> begin---
> grub_ufs_find_file (struct grub_ufs_data *data, const char *path)
> {
> char fpath[grub_strlen (path)]; <-- not enough.
> char *name = fpath;
> char *next;
> unsigned int pos = 0;
> int dirino;
>
> grub_strncpy (fpath, path, grub_strlen (path)); <--without NIL.
> end-
It feels easier to just add +1 at both the location. Can anyone else
comment on this, please?
> The '.label' should set to 0.
> On the original code, The 'label' function returns the invalid pointer,
> so the 'ls -l' command gets wrong.
Marco?
~j
pgpOs6M3jgduy.pgp
Description: PGP signature
___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
Re: CD-ROM booting staus update
"Alex Roman" <[EMAIL PROTECTED]> writes:
> I've cleaned up the code and created a patch which will enable allow
> booting from the first CD-ROM on a system. This is my first patch so I
> checked my code many times... hopefully it won't cause unwanted side
> effects, and hopefully my patch is in the correct format.
Hi Alex,
I haven't seen any reviews of this patch, so I'll give it a go.
Remember that I do not do any active GRUB2 development at the moment,
so I'll mostly comment on style related issues.
> + void EXPORT_FUNC(grub_eltorito_boot) (int drive, void *addr, int size)
> __attribute__ ((noreturn));
> +
If this line is longer than 72 characters, please try to break it.
> + /* We can't do this for CD drives */
> + if ( ! (drive & 0xe0) )
> + {
> +if (grub_biosdisk_get_diskinfo_standard (drive,
> +&data->cylinders,
> +&data->heads,
> +&data->sectors) != 0)
> + {
> +grub_free (data);
> +return grub_error (GRUB_ERR_BAD_DEVICE, "cannot get C/H/S values");
> + }
> +
> +if (! total_sectors)
> + total_sectors = data->cylinders * data->heads * data->sectors;
> + }
I guess the outer braces are misaligned because of whitespace issues
(tab vs space)? Otherwise you need to indent them two steps further.
>
>disk->total_sectors = total_sectors;
>disk->data = data;
> @@ -167,20 +181,23 @@
> unsigned segment)
> {
>struct grub_biosdisk_data *data = disk->data;
> -
> +
Whitespace alterations. No need for those.
> - dap->block = sector;
> -
> + dap->block = sector;
> +
Same here.
> +/*
> + * Decode and print a Boot Record Volume Descriptor structure
> + */
Even though the license header in each file prefixes each line with a
star, we normally don't do that for other multiline comments.
> +/*
> + * The main command function
> + */
If you ask me, no need for this comment.
> +static grub_err_t
> +grub_cmd_eltorito (struct grub_arg_list *state __attribute__ ((unused)),
> + int argc __attribute__ ((unused)),
> + char **args __attribute__ ((unused)))
> +{
> + /* A few structure pointers that we'll need. */
> + grub_eltorito_boot_record_vol_descr brvd;
> + grub_eltorito_validation_entry ve;
> + grub_eltorito_default_entry de;
> +
> + /* Some other variables we'll need. */
> + grub_device_t cd_dev;
> + grub_disk_t disk;
> + grub_err_t err;
Instead of commenting that you have declared those variables, comment
WHY. If a comment is needed at all. The rule of thumb is to comment
why something is done, not how.
> +struct grub_eltorito_validation_entry_tag
> +{
> + // Header ID, must be 0x01.
> + grub_uint8_theader_id;
Please do not use C++ style single-line comments.
Besides these cosmetic comments I think the overall patch looks just
fine. Great work Alex.
~j
pgpALIduaUtwO.pgp
Description: PGP signature
___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
Re: Ethernet support [PATCH]
Marco Gerards <[EMAIL PROTECTED]> writes:
> If I do not get any comments within a week, I'll just commit the
> patch. >
I have a few more comments;
> +#include
> +#include
> +
> +enum grub_memstack_type
> + {
> +GRUB_MEMSTACK_TYPE_ETHERNET,
> +GRUB_MEMSTACK_TYPE_IPV4,
> +GRUB_MEMSTACK_TYPE_ARP,
> +GRUB_MEMSTACK_TYPE_UDP,
> +GRUB_MEMSTACK_TYPE_DATA /* XXX */
> + };
> +typedef enum grub_memstack_type grub_memstack_type_t;
I do not see why this is needed at all? The data should be opaque.
> +grub_memstack_t grub_memstack_new (void);
> +
> +void grub_memstack_free (grub_memstack_t stack);
> +
> +void *grub_memstack_push (grub_memstack_t stack,
> + grub_memstack_type_t type,
> + grub_size_t size);
> +
> +void *grub_memstack_bottom (grub_memstack_t stack,
> + grub_memstack_type_t type,
> + grub_size_t size);
> +
> +void *grub_memstack_pop (grub_memstack_t stack, grub_size_t *size);
> +
> +grub_err_t grub_memstack_popall (grub_memstack_t stack,
> + char *buf, grub_size_t *size);
> +
The network component in a operating system is often seen as a stack,
because packets travel up and down through a logical stack of
functionality layers. That doesn't say that the data that is passed
is really a stack. BSD uses a data structure called "mbufs", which
really just is a linked list of small data chunks. There are
operations to append, prepend and gather data from the "mbuf".
I suggest to remove the word "stack" from our data structure, and
replace it with something more generic; "buf" maybe?
And since it is really a linked list, _push should become prepend, and
_bottom append, respectively.
Also, I do not think that the read operation should be destructive.
Maybe a _read function is in order?
If we do this, we have a shot at adding a data structure that could be
used in other places than just our small network stack.
~j
pgpHDwjGGvD4J.pgp
Description: PGP signature
___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
Re: Ethernet support [PATCH]
Marco Gerards <[EMAIL PROTECTED]> writes:
> If I do not get any comments within a week, I'll just commit the
> patch. >
I have a few more comments;
> +#include
> +#include
> +
> +enum grub_memstack_type
> + {
> +GRUB_MEMSTACK_TYPE_ETHERNET,
> +GRUB_MEMSTACK_TYPE_IPV4,
> +GRUB_MEMSTACK_TYPE_ARP,
> +GRUB_MEMSTACK_TYPE_UDP,
> +GRUB_MEMSTACK_TYPE_DATA /* XXX */
> + };
> +typedef enum grub_memstack_type grub_memstack_type_t;
I do not see why this is needed at all? The data should be opaque.
> +grub_memstack_t grub_memstack_new (void);
> +
> +void grub_memstack_free (grub_memstack_t stack);
> +
> +void *grub_memstack_push (grub_memstack_t stack,
> + grub_memstack_type_t type,
> + grub_size_t size);
> +
> +void *grub_memstack_bottom (grub_memstack_t stack,
> + grub_memstack_type_t type,
> + grub_size_t size);
> +
> +void *grub_memstack_pop (grub_memstack_t stack, grub_size_t *size);
> +
> +grub_err_t grub_memstack_popall (grub_memstack_t stack,
> + char *buf, grub_size_t *size);
> +
The network component in a operating system is often seen as a stack,
because packets travel up and down through a logical stack of
functionality layers. That doesn't say that the data that is passed
is really a stack. BSD uses a data structure called "mbufs", which
really just is a linked list of small data chunks. There are
operations to append, prepend and gather data from the "mbuf".
I suggest to remove the word "stack" from our data structure, and
replace it with something more generic; "buf" maybe?
And since it is really a linked list, _push should become prepend, and
_bottom append, respectively.
Also, I do not think that the read operation should be destructive.
Maybe a _read function is in order?
If we do this, we have a shot at adding a data structure that could be
used in other places than just our small network stack.
~j
pgpvpEH5y7NnU.pgp
Description: PGP signature
___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
Re: pending patches
[EMAIL PROTECTED] writes: > During work on EFI, I have written a standalone ELF to PE/COFF converted. I > still have to mark it work within the grub framework. Not a lot of work but > it requires a few hours. IIRC, we already have a ELF to PE/COFF converter in GRUB. Look at util/i386/efi/grub-mkimage.c . If not, I have one written for GNUFI that could be used. ~j pgp8kOHLS9wLL.pgp Description: PGP signature ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
