How can I fix my VBE implementation for my OS?

Question

I'm slowly but surely working on a small operating system and I finally think I have some code in place that should (in theory) output to VESA VBE. I am, however, getting a slew of errors when trying to compile. I'm sure there's something wrong with my code, though I have no clue what. I see the errors, but I don't know how to fix them. For more relevant information, you can check out a few sources for my mess of code:

1. I got the VBE_modeInfo struct from here
2. I got the u32 modeInfoPointer = asm volatile ("eax");, u32 physical_address = modeInfo.PhysBasePtr + y * modeInfo.LinBytesPerScanLine + x * bytesPerPixel;, and a lot of my assembly code from a previous question

Here is my long list of errors, if anyone is curious. I don't think boot.asm is the issue, considering I got no errors mentioning it. Here I'll leave kernel.cpp and my GitHub for the rest of the code:

kernel.cpp:

typedef unsigned char uint8_t;
typedef unsigned char u8;
typedef unsigned short uint16_t;
typedef unsigned int u32;
typedef u32 size_t;
typedef unsigned long phys_bytes;
#define SCREEN_WIDTH (int)(modeInfo.XResolution)
#define SCREEN_HEIGHT (int)(modeInfo.YResolution)
#define BPP 32
#define SCREEN_SIZE (SCREEN_WIDTH * SCREEN_HEIGHT)
#define FPS 30
#define PIT_HERTZ 1193131.666
#define CLOCK_HIT (int)(PIT_HERTZ/FPS)
#define KEY_LEFT 0x4B
#define KEY_UP 0x48
#define KEY_RIGHT 0x4D
#define KEY_DOWN 0x50

typedef struct VBE_modeInfo{
/*  Mandatory information for all VBE revisions */
uint16_t ModeAttributes;   
uint8_t WinAAttributes;    
uint8_t WinBAttributes;    
uint16_t WinGranularity;   
uint16_t WinSize;          
uint16_t WinASegment;      
uint16_t WinBSegment;      
phys_bytes WinFuncPtr;     
uint16_t BytesPerScanLine; 
/* Mandatory information for VBE 1.2 and above */

uint16_t XResolution;       
uint16_t YResolution;       
uint8_t XCharSize;          
uint8_t YCharSize;          
uint8_t NumberOfPlanes;     
uint8_t BitsPerPixel;       
uint8_t NumberOfBanks;      
uint8_t MemoryModel;        
uint8_t BankSize;           
uint8_t NumberOfImagePages; 
uint8_t Reserved1;          
/* Direct Color fields (required for direct/6 and YUV/7 memory models) */

uint8_t RedMaskSize;         /* size of direct color red mask in bits */
uint8_t RedFieldPosition;    /* bit position of lsb of red mask */
uint8_t GreenMaskSize;       /* size of direct color green mask in bits */
uint8_t GreenFieldPosition;  /* bit position of lsb of green mask */
uint8_t BlueMaskSize;        /* size of direct color blue mask in bits */
uint8_t BlueFieldPosition;   /* bit position of lsb of blue mask */
uint8_t RsvdMaskSize;        /* size of direct color reserved mask in bits */
uint8_t RsvdFieldPosition;   /* bit position of lsb of reserved mask */
uint8_t DirectColorModeInfo; /* direct color mode attributes */

/* Mandatory information for VBE 2.0 and above */
phys_bytes PhysBasePtr; 
uint8_t Reserved2[4];   
uint8_t Reserved3[2];   
/* Mandatory information for VBE 3.0 and above */
uint16_t LinBytesPerScanLine;  /* bytes per scan line for linear modes */
uint8_t BnkNumberOfImagePages; /* number of images for banked modes */
uint8_t LinNumberOfImagePages; /* number of images for linear modes */
uint8_t LinRedMaskSize;        /* size of direct color red mask (linear modes) */
uint8_t LinRedFieldPosition;   /* bit position of lsb of red mask (linear modes) */
uint8_t LinGreenMaskSize;      /* size of direct color green mask (linear modes) */
uint8_t LinGreenFieldPosition; /* bit position of lsb of green mask (linear  modes) */
uint8_t LinBlueMaskSize;       /* size of direct color blue mask (linear modes) */
uint8_t LinBlueFieldPosition;  /* bit position of lsb of blue mask (linear modes ) */
uint8_t LinRsvdMaskSize;       /* size of direct color reserved mask (linear modes) */
uint8_t LinRsvdFieldPosition;  /* bit position of lsb of reserved mask (linear modes) */
u32 MaxPixelClock;        /* maximum pixel clock (in Hz) for graphics mode */
uint8_t Reserved4[190];        /* remainder of ModeInfoBlock */
} VBE_modeInfo;

u32 modeInfoPointer = asm volatile ("eax");
#define modeInfo (struct VBE_modeInfo *)modeInfoPointer

static u32 *BUFFER = (u32 *) modeInfo.PhysBasePtr;

// double buffers
u32 _sbuffers[2][SCREEN_SIZE];
u32 _sback = 0;

#define CURRENT (_sbuffers[_sback])
#define SWAP() (_sback = 1 - _sback)

#define screen_buffer() (_sbuffers[_sback])

static inline void outb(uint16_t port, uint8_t val)
{
    asm volatile ( "outb %0, %1" : : "a"(val), "Nd"(port) );
}

#define bytesPerPixel ((modeInfo->BitsPerPixel + 7) / 8)

void screen_set(u32 color,int x,int y) {
    u32 physical_address = modeInfo.PhysBasePtr + y * modeInfo.LinBytesPerScanLine + x * bytesPerPixel;
    _sbuffers[_sback][physical_address]=color;
}

static inline uint8_t inb(uint16_t port)
{
    uint8_t ret;
    asm volatile ( "inb %1, %0"
                   : "=a"(ret)
                   : "Nd"(port) );
    return ret;
}

const unsigned char font[128-32][8] = {
    { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00},   // U+0020 (space)
         /*deleted to help with length of code...*/
    { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}    // U+007F
};

static inline void *memcpy(void *dst, const void *src, size_t n)
{
    u8 *d = (u8*)dst;
    const u8 *s = (const u8*)src;

    while (n-- > 0) {
        *d++ = *s++;
    }

    return d;
}

void screen_swap() {
    memcpy(BUFFER, CURRENT, SCREEN_SIZE);
    SWAP();
}

unsigned read_pit(void) {
    unsigned count = 0;
 
    // al = channel in bits 6 and 7, remaining bits clear
    outb(0x43,0b0000000);
 
    count = inb(0x40);          // Low byte
    count |= inb(0x40)<<8;      // High byte
 
    return count;
}
 
void draw_char(char c, int x, int y, u32 color)
{
    const unsigned char *glyph = font[(int)c-32];
 
    for(int cy=0;cy<8;cy++){
        for(int cx=0;cx<8;cx++){
            if(((int)glyph[cy]&(1<<cx))==(1<<cx)){
                screen_set(color,x+cx,y+cy);
            }
        } 
    }
}

void draw_string(const char * s, int x, int y, u32 color) {
    int i = 0;
    while(s[i] != false) {
        draw_char(s[i],x+(i*8),y,color);
        i++;
    }
}

void draw_rect(int pos_x, int pos_y, int w, int h, u32 color) {
    for(int y = 0; y<h; y++) {
        for(int x = 0; x<w; x++) {
            screen_set(color,x+pos_x,y+pos_y);
        }
    }
}

static void render(int c0, int c1) {
    //draw_rect(0,0,SCREEN_WIDTH,SCREEN_HEIGHT,0);
    //draw_string("Hello, reader. This is written text.", 100, 180, 16777215);
    //draw_string("If this is displayed, my code works.", 100+c0, 100+c1, 16777215);
}

extern "C" void main(){
    
    int clock = 0;
    int incC1 = 0;
    int incC0 = 0;
    while(true) {
        uint16_t scancode = (uint16_t) inb(0x60);
        clock++;
        if(read_pit()!= 0 && clock == CLOCK_HIT) {
            if(scancode == KEY_LEFT) {
                incC0--;
            }else if(scancode == KEY_RIGHT) {
                incC0++;
            }
            if(scancode == KEY_DOWN) {
                incC1++;
            }else if(scancode == KEY_UP) {
                incC1--;
            }
            clock = 0;
            render(incC0,incC1);
            screen_swap();
        }
    }

    return;
}

EDIT: I'll add the rest of the relevant code here. Shell file and full src on my GitHub:

errors:

kernel.cpp:85:23: error: expected primary-expression before 'asm'
   85 | u32 modeInfoPointer = asm volatile ("eax");
      |                       ^~~
kernel.cpp:88:39: error: request for member 'PhysBasePtr' in 'modeInfoPointer', which is of non-class type 'u32' {aka 'unsigned int'}
   88 | static u32 *BUFFER = (u32 *) modeInfo.PhysBasePtr;
      |                                       ^~~~~~~~~~~
kernel.cpp:17:37: error: request for member 'XResolution' in 'modeInfoPointer', which is of non-class type 'u32' {aka 'unsigned int'}
   17 | #define SCREEN_WIDTH (int)(modeInfo.XResolution)
      |                                     ^~~~~~~~~~~
kernel.cpp:20:22: note: in expansion of macro 'SCREEN_WIDTH'
   20 | #define SCREEN_SIZE (SCREEN_WIDTH * SCREEN_HEIGHT)
      |                      ^~~~~~~~~~~~
kernel.cpp:91:18: note: in expansion of macro 'SCREEN_SIZE'
   91 | u32 _sbuffers[2][SCREEN_SIZE];
      |                  ^~~~~~~~~~~
kernel.cpp:18:38: error: request for member 'YResolution' in 'modeInfoPointer', which is of non-class type 'u32' {aka 'unsigned int'}
   18 | #define SCREEN_HEIGHT (int)(modeInfo.YResolution)
      |                                      ^~~~~~~~~~~
kernel.cpp:20:37: note: in expansion of macro 'SCREEN_HEIGHT'
   20 | #define SCREEN_SIZE (SCREEN_WIDTH * SCREEN_HEIGHT)
      |                                     ^~~~~~~~~~~~~
kernel.cpp:91:18: note: in expansion of macro 'SCREEN_SIZE'
   91 | u32 _sbuffers[2][SCREEN_SIZE];
      |                  ^~~~~~~~~~~
kernel.cpp: In function 'void screen_set(u32, int, int)':
kernel.cpp:107:37: error: request for member 'PhysBasePtr' in 'modeInfoPointer', which is of non-class type 'u32' {aka
unsigned int'}
  107 |     u32 physical_address = modeInfo.PhysBasePtr + y * modeInfo.LinBytesPerScanLine + x * bytesPerPixel;
      |                                     ^~~~~~~~~~~
kernel.cpp:107:64: error: request for member 'LinBytesPerScanLine' in 'modeInfoPointer', which is of non-class type 'u3
' {aka 'unsigned int'}
  107 |     u32 physical_address = modeInfo.PhysBasePtr + y * modeInfo.LinBytesPerScanLine + x * bytesPerPixel;
      |                                                                ^~~~~~~~~~~~~~~~~~~
kernel.cpp:104:33: error: base operand of '->' is not a pointer
  104 | #define bytesPerPixel ((modeInfo->BitsPerPixel + 7) / 8)
      |                                 ^~
kernel.cpp:107:90: note: in expansion of macro 'bytesPerPixel'
  107 |     u32 physical_address = modeInfo.PhysBasePtr + y * modeInfo.LinBytesPerScanLine + x * bytesPerPixel;
      |                                                                                          ^~~~~~~~~~~~~
kernel.cpp:108:5: error: '_sbuffers' was not declared in this scope
  108 |     _sbuffers[_sback][physical_address]=color;
      |     ^~~~~~~~~
kernel.cpp: In function 'void screen_swap()':
kernel.cpp:94:18: error: '_sbuffers' was not declared in this scope
   94 | #define CURRENT (_sbuffers[_sback])
      |                  ^~~~~~~~~
kernel.cpp:232:20: note: in expansion of macro 'CURRENT'
  232 |     memcpy(BUFFER, CURRENT, SCREEN_SIZE);
      |                    ^~~~~~~
kernel.cpp:17:37: error: request for member 'XResolution' in 'modeInfoPointer', which is of non-class type 'u32' {aka 'unsigned int'}
   17 | #define SCREEN_WIDTH (int)(modeInfo.XResolution)
      |                                     ^~~~~~~~~~~
kernel.cpp:20:22: note: in expansion of macro 'SCREEN_WIDTH'
   20 | #define SCREEN_SIZE (SCREEN_WIDTH * SCREEN_HEIGHT)
      |                      ^~~~~~~~~~~~
kernel.cpp:232:29: note: in expansion of macro 'SCREEN_SIZE'
  232 |     memcpy(BUFFER, CURRENT, SCREEN_SIZE);
      |                             ^~~~~~~~~~~
kernel.cpp:18:38: error: request for member 'YResolution' in 'modeInfoPointer', which is of non-class type 'u32' {aka 'unsigned int'}
   18 | #define SCREEN_HEIGHT (int)(modeInfo.YResolution)
      |                                      ^~~~~~~~~~~
kernel.cpp:20:37: note: in expansion of macro 'SCREEN_HEIGHT'
   20 | #define SCREEN_SIZE (SCREEN_WIDTH * SCREEN_HEIGHT)
      |                                     ^~~~~~~~~~~~~
kernel.cpp:232:29: note: in expansion of macro 'SCREEN_SIZE'
  232 |     memcpy(BUFFER, CURRENT, SCREEN_SIZE);
      |                             ^~~~~~~~~~~

boot.asm:

[org 0x7c00]                        
KERNEL_LOCATION equ 0x1000
                                    
xor ax, ax
mov ds, ax
mov es, ax
mov ss, ax
mov sp, 0x8000
mov [BOOT_DISK], dl

;Get video mode info
mov ax, 4f01h
mov cx, 105h
mov di, modeInfo 
int 10h

mov eax, modeInfo
mov bx, KERNEL_LOCATION
mov dh, 32

mov ah, 0x02
mov al, dh 
mov ch, 0x00
mov dh, 0x00
mov cl, 0x02
mov dl, [BOOT_DISK]
int 0x13

                                    
mov ax, 0x4F02   ; set VBE mode
mov bx, 0x4118   ; VBE mode number
int 0x10         ; call VBE BIOS
cmp ax, 0x004F   ; test for error
jne error

CODE_SEG equ GDT_code - GDT_start
DATA_SEG equ GDT_data - GDT_start

cli
lgdt [GDT_descriptor]
mov eax, cr0
or eax, 1
mov cr0, eax
jmp CODE_SEG:start_protected_mode

jmp $

modeInfo    TIMES 256 db 0

error:
   stc
   ret

BOOT_DISK: db 0

GDT_start:
    GDT_null:
        dd 0x0
        dd 0x0

    GDT_code:
        dw 0xffff
        dw 0x0
        db 0x0
        db 0b10011010
        db 0b11001111
        db 0x0

    GDT_data:
        dw 0xffff
        dw 0x0
        db 0x0
        db 0b10010010
        db 0b11001111
        db 0x0

GDT_end:

GDT_descriptor:
    dw GDT_end - GDT_start - 1
    dd GDT_start


[bits 32]
start_protected_mode:
    mov ax, DATA_SEG
    mov ds, ax
    mov es, ax
    mov fs, ax
    mov gs, ax
    mov ss, ax
    mov esp, 0x00090000     ; 32 bit stack base pointer
    mov ebp, esp            ; Only if you need this!

    jmp KERNEL_LOCATION

                                     
 
times 510-($-$$) db 0              
dw 0xaa55

Answer 1

Okay, based on all the top comments ...

We want modeInfoPointer to be set from eax because that holds the VBE pointer.

In boot.asm, it gets this from int 0x10. But, we have to move this somewhere. It gets clobbered when we jump to start_protected_mode and do:

mov ax, DATA_SEG

So, we need (as the first instruction of that):

mov edi, eax ; save VBE pointer

Then, just before jmp KERNEL_LOCATION, we need:

push edi ; save as first argument to main

Also, we want call instead of jmp

Then, in kernel.cpp, we want:

struct VBE_modeInfo *modeInfoPointer;
extern "C" void main(struct VBE_modeInfo *vbe)
{
    modeInfoPointer = vbe;

There are other adjustments. Use of modeInfo is inconsistent with modeInfo. and modeInfo->

I've corrected the compilation errors as best I can. It is almost certainly still broken but should get you a bit farther.

It should fix the issue that we want modeInfoPointer to point to the VBE block [AFAICT].

---

In the code below, I've used cpp conditionals to denote old vs. new code:

#if 0
// old code
#else
// new code
#endif

#if 1
// new code
#endif

Note: this can be cleaned up by running the file through unifdef -k

---

Here is a patch against the github repo:

diff --git a/src/OS.bin b/src/OS.bin
deleted file mode 100644
index 2957814..0000000
Binary files a/src/OS.bin and /dev/null differ
diff --git a/src/boot.asm b/src/boot.asm
index 0ee58af..a41730a 100644
--- a/src/boot.asm
+++ b/src/boot.asm
@@ -83,6 +83,7 @@ GDT_descriptor:

 [bits 32]
 start_protected_mode:
+   mov edi, eax    ; save VBE pointer
     mov ax, DATA_SEG
     mov ds, ax
     mov es, ax
@@ -92,9 +93,8 @@ start_protected_mode:
     mov esp, 0x00090000     ; 32 bit stack base pointer
     mov ebp, esp            ; Only if you need this!

-    jmp KERNEL_LOCATION
-
-
+   push edi                ; save as first argument to main
+    call KERNEL_LOCATION

 times 510-($-$$) db 0
 dw 0xaa55
diff --git a/src/kernel.cpp b/src/kernel.cpp
index 79ef409..49282bb 100644
--- a/src/kernel.cpp
+++ b/src/kernel.cpp
@@ -14,8 +14,13 @@ typedef unsigned short uint16_t;
 typedef unsigned int u32;
 typedef u32 size_t;
 typedef unsigned long phys_bytes;
+#if 0
 #define SCREEN_WIDTH (int)(modeInfo.XResolution)
 #define SCREEN_HEIGHT (int)(modeInfo.YResolution)
+#else
+#define SCREEN_WIDTH (int)(modeInfo->XResolution)
+#define SCREEN_HEIGHT (int)(modeInfo->YResolution)
+#endif
 #define BPP 32
 #define SCREEN_SIZE (SCREEN_WIDTH * SCREEN_HEIGHT)
 #define FPS 30
@@ -82,16 +87,28 @@ u32 MaxPixelClock;        /* maximum pixel clock (in Hz) for graphics mode */
 uint8_t Reserved4[190];        /* remainder of ModeInfoBlock */
 } VBE_modeInfo;

+#if 0
 u32 modeInfoPointer = asm volatile ("eax");
 #define modeInfo (struct VBE_modeInfo *)modeInfoPointer
+#else
+struct VBE_modeInfo *modeInfoPointer;
+#define modeInfo modeInfoPointer
+#endif

-static u32 *BUFFER = (u32 *) modeInfo.PhysBasePtr;
+static u32 *BUFFER = (u32 *) modeInfo->PhysBasePtr;

 // double buffers
+#if 0
 u32 _sbuffers[2][SCREEN_SIZE];
+#else
+u32 *_sbuffers;
+#endif
 u32 _sback = 0;

-#define CURRENT (_sbuffers[_sback])
+#define SBUF(_y,_x) \
+   _sbuffers[((_y) * SCREEN_SIZE) + _x]
+
+#define CURRENT &SBUF(0,0)
 #define SWAP() (_sback = 1 - _sback)

 #define screen_buffer() (_sbuffers[_sback])
@@ -103,9 +120,26 @@ static inline void outb(uint16_t port, uint8_t val)

 #define bytesPerPixel ((modeInfo->BitsPerPixel + 7) / 8)

+#ifndef NULL
+//#define NULL nullptr
+#include <cstddef>
+#include <cstdlib>
+#endif
+
 void screen_set(u32 color,int x,int y) {
+#if 0
     u32 physical_address = modeInfo.PhysBasePtr + y * modeInfo.LinBytesPerScanLine + x * bytesPerPixel;
+#else
+    u32 physical_address = modeInfo->PhysBasePtr + y * modeInfo->LinBytesPerScanLine + x * bytesPerPixel;
+#endif
+#if 0
     _sbuffers[_sback][physical_address]=color;
+#else
+   if (_sbuffers == NULL)
+       _sbuffers = (u32 *) malloc(sizeof(u32) * 2 * SCREEN_SIZE);
+
+    SBUF(_sback,physical_address)=color;
+#endif
 }

 static inline uint8_t inb(uint16_t port)
@@ -280,8 +314,13 @@ static void render(int c0, int c1) {
     //draw_string("If this is displayed, my code works.", 100+c0, 100+c1, 16777215);
 }

+#if 0
 extern "C" void main(){
-
+#else
+extern "C" void main(struct VBE_modeInfo *vbe) {
+   modeInfoPointer = vbe;
+#endif
+
     int clock = 0;
     int incC1 = 0;
     int incC0 = 0;