Kernel support for VESA Bios Extension

Here is attached a patch that adds VBE support to kernel and include file to make it easier to use it. There is diff for both i386-pc.rmk and i386-pc.mk. Latter could be removed and let build process to generate it again, but I think that .mk file is present in CVS so I left both there.

I will make separate patch for the frame buffer console after this is first committed. (And I need to make some cleanups there first)

I would like to hear comments about the patch. I tried to implement most commonly needed functions, so you can't do everything with this. As I didn't need to use every function in my frame buffer console test, some things might not work. But I tried to make my best to proof read every function.

Quoted 5 lines

I would like to hear comments about the patch. I tried to implement most commonly needed functions, so you can't do everything with this. As I didn't need to use every function in my frame buffer console test, some things might not work. But I tried to make my best to proof read every function.

Can you please make sure you are using the GCS for coding style? Please have a look at the other sourcecode and the GCS (GNU Coding Standards) first:

http://www.gnu.org/prep/standards/

It is important for us to have a single and consistent coding style. If you have questions about either GRUB or the coding style you are of course free to ask.

Quoted 5 lines

Quoted 5 lines

I would like to hear comments about the patch. I tried to implement most commonly needed functions, so you can't do everything with this. As I didn't need to use every function in my frame buffer console test, some things might not work. But I tried to make my best to proof read every function.

Quoted 7 lines

Can you please make sure you are using the GCS for coding style? Please have a look at the other sourcecode and the GCS (GNU Coding Standards) first:
http://www.gnu.org/prep/standards/
It is important for us to have a single and consistent coding style. If you have questions about either GRUB or the coding style you are of course free to ask.

Perhaps this is a bit closer to requirement?

If there still are issues on formatting could you specify what issues you are thinking on.

+ grub_uint8_t reserved4[189+1];

* ------------------------------------------------------------

  * |             | |B| |A|       | |   |1|0|E|W|A|            |
- * | BASE 31..24 |G|/|0|V| LIMIT |P|DPL|  TYPE   | BASE 23:16 |
+ * | BASE 31..24 |G|/|L|V| LIMIT |P|DPL|  TYPE   | BASE 23:16 |  4
  * |             | |D| |L| 19..16| |   |1|1|C|R|A|            |
  * ------------------------------------------------------------
  * |                             |                            |
- * |        BASE 15..0           |       LIMIT 15..0          |
+ * |        BASE 15..0           |       LIMIT 15..0          |  0
  * |                             |                            |

* ------------------------------------------------------------ *

I personally prefer not using that many `*'s and believe it is not in the GCS. But I think Okuji uses this as well, but I don't. Perhaps we need our own guidelines for such things, I think consistency is important. Okuji, what you you think?

Review

Quoted 3 lines

(quoted diff) +3 -0

+typedef grub_uint32_t 	grub_vbe_farptr_t;
+typedef grub_uint32_t	grub_vbe_physptr_t;
+typedef grub_uint32_t	grub_vbe_status_t;

Please use a single space before the type name.

Review

Quoted 3 lines

(quoted diff) +3 -0

+struct grub_vbe_mode_info_block
+{
+  /* Mandory information for all VBE revisions */

After the comment add a `.' and two spaces instead of one. This is true for all comments.

Review

Quoted 6 lines

(quoted diff) +6 -0

+  /*
+   * Reserved field to make structure to be 256 bytes long, VESA BIOS 
+   * Extension 3.0 Specification says to reserve 189 bytes here but 
+   * that doesn't make structure to be 256 bytes. So additional one is 
+   * added here.
+   */

I personally prefer not using that many `*'s and believe it is not in the GCS. But I think Okuji uses this as well, but I don't. Perhaps we need our own guidelines for such things, I think consistency is important. Okuji, what you you think?

Quoted 1 line

+ grub_uint8_t reserved4[189+1];

Please put spaces around operators. So this should be:

grub_uint8_t reserved4[189 + 1]

* 31 24 19 16 7 0

Quoted 9 lines

* ------------------------------------------------------------

 * |             | |B| |A|       | |   |1|0|E|W|A|            |
- * | BASE 31..24 |G|/|0|V| LIMIT |P|DPL|  TYPE   | BASE 23:16 |
+ * | BASE 31..24 |G|/|L|V| LIMIT |P|DPL|  TYPE   | BASE 23:16 |  4
 * |             | |D| |L| 19..16| |   |1|1|C|R|A|            |
 * ------------------------------------------------------------
 * |                             |                            |
- * |        BASE 15..0           |       LIMIT 15..0          |
+ * |        BASE 15..0           |       LIMIT 15..0          |  0

Quoted 3 lines

* | | |
* ------------------------------------------------------------ *

What is this? Is this copied from (copyrighted) documentation?

To me it seemed line some lines were too long. Please keep in mind it should be all readable on a 80 columns width terminal according to the GCS.

I will be gone this weekend. After the weekend I will try to understand the patch and proofread the assembler code. But my x86 assembly is *very* rusty, so I hope someone else can review this patch because I am afraid when it comes to assembler my opinion is not worth a bit. ;)

Quoted 2 lines

After the comment add a `.' and two spaces instead of one. This is true for all comments.

Odd... Especially that two spaces. But I don't want to start another "war" with this ;)... So I try to adapt.

It can of course be removed completely in the commit if required to be.

Quoted 3 lines · Vesa Jääskeläinen

Quoted 2 lines

After the comment add a `.' and two spaces instead of one. This is true for all comments.

Quoted 2 lines

Odd... Especially that two spaces. But I don't want to start another "war" with this ;)... So I try to adapt.

According to the GNU Coding Standards, two spaces are useful for Emacs to identify the end of a text. I myself do not see any problem without two spaces in Emacs, so I agree that it looks strange. But... this is what the GNU Coding Standards says, and I have no problem with this rule, so we follow it.