Loading a file from a FAT32 filesystem in 16 bit real mode on x86 (Intel syntax assembly)

Question

I am writing a bootloader that needs to load a file from a FAT32 filesytem. Since I need to load both the root directory first to find the start of the file, and then the actual file, I wrote a function to load a sequence of clusters from the disk to 0x8000 in memory (since the bootloader is loaded from 0x7C00-0x7DFF and I use 0x7E00-0x7FFF to load sections of the FAT).

loadClusterChain: ; ARGS: eax = first cluster number
    pusha

    ; Set the memory location to load the file to 0x8000
    mov dx, 0x8000

    loadClusterChainLoop:

    ; Store the cluster number (eax) and additional offset (dx) for later
    push dx
    push eax

    ; Update the disk address packet
    mov word [dapDataOffset], dx

    ; Account for the cluster numbers being shifted by 2
    sub eax, 2

    ; Calculate how many sectors the cluster is from the start of the data area
    xor ebx, ebx
    mov bl, byte [bpbSectorsPerCluster]
    mul ebx

    ; Add the start of data area to the root directory offset
    add eax, dword [diskDataAreaStart]

    ; Update the disk address packet start sector
    mov dword [dapStartSector], eax

    ; Update the disk address packet sector count
    push eax
    xor ax, ax
    mov al, byte [bpbSectorsPerCluster]
    mov word [dapSectorCount], ax
    pop eax

    ; Load the file cluster
    mov ah, 0x42
    mov dl, byte [bpbDriveNumber]
    mov si, diskAddressPacket
    int 0x13
    jc diskError

    ; Retrieve the value of eax but keep it in the stack
    pop eax
    push eax

    ; Divide by 128 to calculate the FAT sector to load
    shr eax, 7

    ; Get the number of reserved sectors
    xor ebx, ebx
    mov bx, word [bpbReservedSectors]

    ; Add the reserved sectors to the FAT sector, so eax = relevant FAT sector
    add eax, ebx

    ; Update the disk address packet start sector
    mov dword [dapStartSector], eax

    ; Update the disk address packet sector count
    mov word [dapSectorCount], 1

    ; Update the disk address packet data offset to load the fat to 0x7E00
    mov word [dapDataOffset], 0x7E00

    ; Load the FAT sector
    mov ah, 0x42
    mov dl, byte [bpbDriveNumber]
    mov si, diskAddressPacket
    int 0x13
    jc diskError

    ; Get the offset of clusters into the FAT sector by getting the remainder when divided by 128
    pop eax
    and ax, 0x007F

    ; Multiply offset by 4 to get it in bytes
    shl ax, 2

    ; Calculate the memory address of the cluster in the FAT
    add ax, 0x7E00

    ; Read cluster value
    mov bx, ax
    mov eax, dword [bx]

    ; Set bx = 512*bpbSectorsPerCluster
    xor bx, bx
    mov bl, byte [bpbSectorsPerCluster]
    shl bx, 9

    ; Restore the value of dx and add bx to move dx to the memory location of the end of the file parts loaded so far
    pop dx
    add dx, bx

    ; See if this is the end of the chain
    cmp eax, 0x0FFFFFFF
    jne loadClusterChainLoop

    ; Ensure the file ends with 0, so when scanning the root entries we will definitely stop
    mov bx, dx
    mov [bx], byte 0

    popa
    ret

Here are some of the variables used. diskDataAreaStart is not 0 when the code runs as the actual value is calculated before the function is called. The other variable names are mostly self-explanatory.

diskDataAreaStart       dd 0

diskAddressPacket:
    dapSize         db 0x10
    dapReserved     db 0x00
    dapSectorCount      dw 0
    dapDataOffset       dw 0
    dapDataSegment      dw 0
    dapStartSector      dq 0

I know it's a fairly niche piece of code, but I would be very grateful if someone could give me feedback on it - if I could do something more efficiently or even feedback on how clear my comments were and how easy the code was to follow along would be highly appreciated.

Answer 1

Style

feedback on how clear my comments were and how easy the code was to follow

I'm not a big fan of line comments. I think they make the code a bit harder to follow. That's why I prefer tail comments along with a nice tabular layout of the labels, instructions, operands, and tail comments.
And of course don't write redundant comments like "Update the disk address packet sector count" when looking at the names of the identifiers already tells what is happening.

You've got some pushes and pops that are fairly distant apart. If you would number these (or tag them somehow), verifying what the stack holds and whether the stack was correctly balanced becomes a bit easier.

push    dx                              ; (1)
push    eax                             ; (2)

...

push    ax                              ; (3)
movzx   ax, byte [bpbSectorsPerCluster]
mov     [dapSectorCount], ax
pop     ax                              ; (3)

...

pop     eax                             ; -(2)-
push    eax                             ; -(2)-

...

pop     eax                             ; (2)

...

pop     dx                              ; (1)

Optimizations

mov word [dapDataOffset], dx

Most assemblers will not require you to write this word size tag because the use of the DX register made already clear that this has to be a word-sized operation. Not writing the redundant size tag makes your code a bit clearer.

mov     [dapDataOffset], dx

You did this several times.

---

xor ebx, ebx
mov bl, byte [bpbSectorsPerCluster]

The instruction set has the MOVZX instruction for this kind of situations. It will also reduce the code's footprint which is important in a bootloader that is limited to just 512 bytes. Notice that here the byte size tag is required.

movzx   ebx, byte [bpbSectorsPerCluster]

You did this several times.

---

push eax
xor ax, ax
mov al, byte [bpbSectorsPerCluster]
mov word [dapSectorCount], ax
pop eax

Pushing and popping the 32-bit register EAX from real address mode uses an operand size prefix. In this case the high word was never changed and so you can shave off 2 bytes if you only push/pop AX.

Better still, the program doesn't need you to preserve AX at this point. Again 2 bytes less.

movzx   ax, byte [bpbSectorsPerCluster]
mov     [dapSectorCount], ax

---

pop eax
and ax, 0x007F

; Multiply offset by 4 to get it in bytes
shl ax, 2

; Calculate the memory address of the cluster in the FAT
add ax, 0x7E00

; Read cluster value
mov bx, ax
mov eax, dword [bx]

Here's another opportunity to shave off a byte. Pop the value directly in the register that you'll use to retrieve the cluster number:

pop     ebx
and     bx, 0x7F
shl     bx, 2      ; Multiply offset by 4 to get it in bytes
add     bx, 0x7E00 ; Calculate the memory address of the cluster in the FAT
mov     eax, [bx]  ; Read cluster value

Exactly the same length but less instructions:

pop     ebx
and     ebx, 0x7F
mov     eax, [ebx*4 + 0x00007E00]  ; Read cluster value

---

; Update the disk address packet sector count
mov word [dapSectorCount], 1

; Update the disk address packet data offset to load the fat to 0x7E00
mov word [dapDataOffset], 0x7E00

The above instructions total 12 bytes. Because dapSectorCount and dapDataOffset follow each other in memory, you can combine the writes. The total then becomes 9 bytes.

; Update dapSectorCount=1 and dapDataOffset=0x7E00 together
mov     dword [dapSectorCount], 0x7E000001

This last optimization is of course killing readability in favor of code size. Your choice!

Beware

Your loadClusterChain routine uses some 32-bit registers. Given that this is 16-bit code the pusha and popa instructions will not preserve the high words. Maybe this can become a problem for the rest of your program!