Write README

Makefile

@@ -2,7 +2,7 @@ BUILD=./build
 
 # Default rule
 .PHONY: default
-default: $(BUILD)/disk.img
+default: all
 
 # Remove all build files
 .PHONY: clean
@@ -10,18 +10,23 @@ clean:
 	rm -drf $(BUILD)
 	cd tetros; cargo clean
 
+# Make everything
+# (but don't run qemu)
+.PHONY: all
+all: img
+
 #
-# MARK: disk
+# MARK: boot
 #
 
-# Compile tetros as a library
-# (so that we can link it with a custom linker script)
+# Compile tetros as library
 LIB_SRC = ./tetros/Cargo.toml ./tetros/Cargo.lock $(shell find ./tetros/src -type f)
 $(BUILD)/tetros.lib: $(LIB_SRC)
 	@mkdir -p $(BUILD)
 	cd tetros && \
 		env RUSTFLAGS="-C soft-float" \
 		cargo rustc \
+			--manifest-path="./Cargo.toml" \
 			-Z build-std=core \
 			-Z build-std-features=compiler-builtins-mem \
 			--target "./targets/x86-unknown-none.json" \
@@ -30,7 +35,7 @@ $(BUILD)/tetros.lib: $(LIB_SRC)
 			-- \
 			--emit link="$(CURDIR)/$@"
 
-# Link tetros using custom linker script
+# Link tetros
 BIOS_LD = ./tetros/linkers/x86-unknown-none.ld
 $(BUILD)/tetros.elf: $(BUILD)/tetros.lib $(BIOS_LD)
 	ld \
@@ -44,13 +49,13 @@ $(BUILD)/tetros.elf: $(BUILD)/tetros.lib $(BIOS_LD)
 	objcopy --only-keep-debug "$@" "$@.sym"
 	objcopy --strip-debug "$@"
 
-# Wrap tetros in BIOS loader
+# Wrap tetros in three-stage BIOS loader
 # Parameters:
 # - BIOS_SRC: source directory of bios assembly
 # - STAGE2_SECTOR: the index of the first sector of the stage 2 binary on the disk
 BIOS_SRC = ./bios
 STAGE2_SECTOR = 1
-$(BUILD)/disk.img: $(wildcard $(BIOS_SRC)/*.asm) $(BUILD)/tetros.elf 
+$(BUILD)/bios.bin: $(wildcard $(BIOS_SRC)/*.asm) $(BUILD)/tetros.elf 
 	@mkdir -p "$(BUILD)"
 	nasm \
 		-f bin \
@@ -61,13 +66,31 @@ $(BUILD)/disk.img: $(wildcard $(BIOS_SRC)/*.asm) $(BUILD)/tetros.elf
 		-i "$(BIOS_SRC)" \
 		"$(BIOS_SRC)/main.asm"
 
+# Extract full mbr (first 512 bytes)
+$(BUILD)/mbr.bin: $(BUILD)/bios.bin
+	@mkdir -p "$(BUILD)"
+	@echo ""
+	dd if="$<" bs=512 count=1 of="$@"
+
+# Extract stage 2 (rest of file)
+$(BUILD)/stage2.bin: $(BUILD)/bios.bin
+	@mkdir -p "$(BUILD)"
+	@echo ""
+	dd if="$<" bs=512 skip=1 of="$@"
+
 #
-# MARK: qemu
-#
-# Do not use `-enable-kvm` or `-cpu host`,
-# this confuses gdb.
+# MARK: bundle
 #
 
+# Make full disk image
+.PHONY: img
+img: $(BUILD)/disk.img
+$(BUILD)/disk.img: $(BUILD)/mbr.bin $(BUILD)/stage2.bin
+	@mkdir -p $(BUILD)
+	@echo ""
+	dd if="$(BUILD)/mbr.bin" of=$@ conv=notrunc bs=512
+	dd if="$(BUILD)/stage2.bin" of=$@ conv=notrunc seek=$(STAGE2_SECTOR) bs=512
+
 .PHONY: qemu
 qemu: $(BUILD)/disk.img
 	qemu-system-i386 \
@@ -105,3 +128,6 @@ qemu-gdb: $(BUILD)/disk.img
 		-fda "$<" \
 		-gdb tcp::26000 \
 		-S
+
+# Do not use `-enable-kvm` or `-cpu host`,
+# this confuses gdb.

README.md

@@ -1,11 +1,14 @@
 # TetrOS: bare-metal tetris
 
+## TODO:
+- Fix stage 1 loader
+
 ## Features
 - Compiles to a standalone disk image
 - Written from scratch using only Nasm and Rust
 - Custom BIOS bootloader
 - 32-bit x86 OS
-- Detailed comments. Read the [makefile](./Makefile), then start in [`./bios/main.asm`](./bios/main.asm).
+- 🌟 Detailed comments. Read the [makefile](./Makefile), then start in [`./bios/main.asm`](./bios/main.asm).
 
 
 ## Non-Features

bios/main.asm

@@ -1,25 +1,30 @@
 sectalign off
 
-; The following code expects two external macros:
+; This program expects two external macros:
 ; STAGE3, a path to the stage3 binary
 ; STAGE2_SECTOR, the location of stage 2
 ;   on the disk, in 512-byte sectors.
-;
-; Both of these are set in the makefile.
+; On a gpt disk, this is probably 34.
+
+; Stage 1 is MBR code, and should fit in LBA 0
+; (512 bytes). Layout is as follows:
+; (Format is `offset, length: purpose`)
+; 0, 424: x86 boot code
+; 440, 4: Unique disk signature
+; 444, 2: unknown
+; 446, 16*4: Array of four legacy MBR records
+; 510, 2: signature 0x55 0xAA
+; 512 to end  of logical block: reserved
+; 
+; See https://uefi.org/specs/UEFI/2.10/05_GUID_Partition_Table_Format.html
 
-; BIOS loads stage 1 at 0x7C00
 ORG 0x7C00
 SECTION .text
 
-; Stage 1 is MBR code, and should fit in LBA 0
-; (i.e, in the first 512 bytes).
+; stage 1 is sector 0, loaded into memory at 0x7C00
 %include "stage1.asm"
 
 ; Stage 1 is at most 440 bytes
-; This limit is set by the GPT spec.
-; See https://uefi.org/specs/UEFI/2.10/05_GUID_Partition_Table_Format.html
-;
-; This `times` will throw an error if the subtraction is negative.
 times 440-($-$$) db 0
 db 0xee
 
@@ -27,38 +32,30 @@ db 0xee
 times 510-($-$$) db 0
 
 ; MBR signature.
-; This tells the BIOS that this disk is bootable.
+; This isn't loaded into memory, it's
+; only here for debugging.
 db 0x55
 db 0xaa
 
-
-; Include stage 2. This is loaded into memory by stage 1.
-; (stage 1 loads both stage 2 and stage 3)
-;
-; Stage 2 sets up protected mode, sets up the GDT,
-; and initializes a minimal environment for stage 3.
-;
-; On a "real" boot disk, this data will not immediately follow stage 1.
-; It would be stored in a special disk partition.
-;
-; We don't need this kind of complexity here, though, so we store
-; stage 2 right after stage 1. (This is why STAGE2_SECTOR is 1.)
-;
-; This is nice, because the layout of the code on our boot disk
-; matches the layout of the code in memory. THIS IS NOT USUALLY THE CASE.
 stage2:
     %include "stage2.asm"
     align 512, db 0
 stage2.end:
 
-; Pad to 0x3000.
-; This makes sure that state3 is loaded at the address
-; the linker expects. Must match the value in `tetros/linkers/x86-unknown-none.ld`.
-times (0x8000 - 0x7c00)-($-$$) db 0
+; The maximum size of stage2 is 4 KiB,
+; This fill will throw an error if the subtraction is negative.
+times (4*1024)-($-stage2) db 0
+
+; Pad to 0x9000.
+; This needs to match the value configured in the stage3 linker script
+times (0x9000 - 0x7c00)-($-$$) db 0
 
-; Include stage 3, the binary compiled from Rust sources.
 stage3:
     %defstr STAGE3_STR %[STAGE3]
     incbin STAGE3_STR
     align 512, db 0
 .end:
+
+; TODO: why? Of the disk, or of memory?
+; the maximum size of the boot loader portion is 384 KiB
+times (384*1024)-($-$$) db 0

bios/print.asm

@@ -1,21 +1,22 @@
 SECTION .text
 USE16
 
-; Print a string and a newline
-;
-; Clobbers ax
+; provide function for printing in x86 real mode
+
+; print a string and a newline
+; CLOBBER
+;   ax
 print_line:
     mov al, 13
     call print_char
     mov al, 10
     jmp print_char
 
-; Print a string
-;
-; Input:
+; print a string
+; IN
 ;   si: points at zero-terminated String
-;
-; Clobbers si, ax
+; CLOBBER
+;   si, ax
 print:
     pushf
     cld
@@ -29,9 +30,8 @@ print:
     popf
     ret
 
-; Print a character
-;
-; Input:
+; print a character
+; IN
 ;   al: character to print
 print_char:
     pusha
@@ -42,11 +42,10 @@ print_char:
     ret
 
 ; print a number in hex
-;
-; Input:
+; IN
 ;   bx: the number
-;
-; Clobbers al, cx
+; CLOBBER
+;   al, cx
 print_hex:
     mov cx, 4
 .lp:

bios/protected_mode.asm

@@ -0,0 +1,46 @@
+SECTION .text
+USE16
+
+protected_mode:
+
+.func: dd 0
+
+.entry:
+    ; disable interrupts
+    cli
+
+    ; load protected mode GDT
+    lgdt [gdtr]
+
+    ; set protected mode bit of cr0
+    mov eax, cr0
+    or eax, 1
+    mov cr0, eax
+
+    ; far jump to load CS with 32 bit segment
+    ; (we are in 32-bit mode, but instruction pipeline
+    ; has 16-bit instructions.
+    jmp gdt.pm32_code:.inner
+
+    ; gdt.pm32_code is a multiple of 8, so it always ends with three zero bits.
+    ; The GDT spec abuses this fact, and uses these last three bits to store other
+    ; data (table type and privilege). In this case, 000 is what we need anyway.
+    ;
+    ; Also note that CS isn't an address in protected mode---it's a GDT descriptor.
+ 
+
+
+USE32
+
+.inner:
+    ; load all the other segments with 32 bit data segments
+    mov eax, gdt.pm32_data
+    mov ds, eax
+    mov es, eax
+    mov fs, eax
+    mov gs, eax
+    mov ss, eax
+
+    ; jump to specified function
+    mov eax, [.func]
+    jmp eax

bios/stage1.asm

@@ -1,39 +1,37 @@
 USE16
 
-stage1:
-    ; Initialize segment registers
-    xor ax, ax ; Set ax to 0
+stage1: ; dl comes with disk
+    ; initialize segment registers
+    xor ax, ax
     mov ds, ax
     mov es, ax
     mov ss, ax
 
-    ; Initialize stack pointer
-    ; (stack grows up)
+    ; initialize stack
     mov sp, 0x7C00
 
-    ; Initialize CS
-    ;
-    ; `retf` sets both CS and IP to a known-good state.
-    ; This is necessary because we don't know where the BIOS put us at startup.
-    ; (could be 0x00:0x7C00, could be 0x7C00:0x00. Not everybody follows spec.)
-    push ax ; `ax` is still 0
+    ; initialize CS
+    ; far jump sets both CS and IP to a known-good state,
+    ; we don't know where the BIOS put us at startup.
+    ; (could be 0x00:0x7C00, could be 0x7C00:0x00.
+    ; Not everybody follows spec.)
+    push ax
     push word .set_cs
     retf
 
 .set_cs:
-    ; Save disk number.
-    ; BIOS sets `dl` to the number of
-    ; the disk we're booting from.
+    ; save disk number
     mov [disk], dl
 
-    ; Print "Stage 1"
     mov si, stage_msg
     call print
     mov al, '1'
     call print_char
     call print_line
 
-    ; read CHS gemotry, save into [chs]
+
+
+    ; read CHS gemotry
     ;  CL (bits 0-5) = maximum sector number
     ;  CL (bits 6-7) = high bits of max cylinder number
     ;  CH = low bits of maximum cylinder number
@@ -53,10 +51,11 @@ stage1:
     and cl, 0x3f
     mov [chs.s], cl
 
-    ; First sector of stage 2
+    ; disk address of stage 2
+    ; (start sector)
     mov eax, STAGE2_SECTOR
 
-    ; Where to load stage 2
+    ; where to load stage 2
     mov bx, stage2
 
     ; length of stage2 + stage3
@@ -64,40 +63,36 @@ stage1:
     mov cx, (stage3.end - stage2) / 512
     mov dx, 0
 
-    ; Consume eax, bx, cx, dx
-    ; and load code from disk.
     call load
 
     jmp stage2.entry
 
-; Load sectors from disk to memory.
-; Cannot load more than 1MiB.
-;
-; Input:
+; load some sectors from disk to a buffer in memory
+; buffer has to be below 1MiB
+; IN
 ;   ax: start sector
 ;   bx: offset of buffer
 ;   cx: number of sectors (512 Bytes each)
 ;   dx: segment of buffer
-;
-; Clobbers ax, bx, cx, dx, si
+; CLOBBER
+;   ax, bx, cx, dx, si
+; TODO rewrite to (eventually) move larger parts at once
+; if that is done increase buffer_size_sectors in startup-common to that (max 0x80000 - startup_end)
 load:
-    ; Every "replace 1" comment means that the `1`
-    ; on that line could be bigger.
-    ;
-    ; See https://stackoverflow.com/questions/58564895/problem-with-bios-int-13h-read-sectors-from-drive
-    ; We have to load one sector at a time to avoid the 1K boundary error.
-    ; Would be nice to read more sectors at a time, though, that's faster.
+    ; replaced 127 with 1.
+    ; see https://stackoverflow.com/questions/58564895/problem-with-bios-int-13h-read-sectors-from-drive
+    ; TODO: fix later
 
-    cmp cx, 1 ; replace 1
+    cmp cx, 1 ;127
     jbe .good_size
 
     pusha
-    mov cx, 1 ; replace 1
+    mov cx, 1; 127
     call load
     popa
-    add eax, 1 ; replace 1
-    add dx, 1 * 512 / 16 ; replace 1
-    sub cx, 1 ; replace 1
+    add eax, 1; 127
+    add dx, 1 * 512 / 16 ; 127
+    sub cx, 1;127
 
     jmp load
 .good_size:
@@ -106,37 +101,44 @@ load:
     mov [DAPACK.count], cx
     mov [DAPACK.seg], dx
 
-    ; Print the data we're reading
-    ; Prints AAAAAAAA#BBBB CCCC:DDDD, where:
-    ; - A..A is the lba we're reading (printed in two parts)
-    ; - BBBB is the number of sectors we're reading
-    ; - CCCC is the index we're writing to
-    ; - DDDD is the buffer we're writing to
-    mov bx, [DAPACK.addr + 2] ; last two bytes
+; This should be a subroutine, 
+; but we don't call/ret to save a few bytes.
+; (we only use this once)
+;
+;call print_dapack
+;print_dapack:
+    mov bx, [DAPACK.addr + 2]
     call print_hex
-    mov bx, [DAPACK.addr] ; first two bytes
+
+    mov bx, [DAPACK.addr]
     call print_hex
+
     mov al, '#'
     call print_char
+
     mov bx, [DAPACK.count]
     call print_hex
+
     mov al, ' '
     call print_char
+
     mov bx, [DAPACK.seg]
     call print_hex
+
     mov al, ':'
     call print_char
+
     mov bx, [DAPACK.buf]
     call print_hex
+
     call print_line
+    ;ret
+    ; End of print_dapack
 
 
-    ; Read from disk.
-    ; int13h, ah=0x42 does not work on some disks.
-    ; use int13h, ah=0x02 in this case.
     cmp byte [chs.s], 0
     jne .chs
-    
+    ;INT 0x13 extended read does not work on CDROM!
     mov dl, [disk]
     mov si, DAPACK
     mov ah, 0x42
@@ -186,10 +188,6 @@ load:
     jc error ; carry flag set on error
     ret
 
-;
-; MARK: errors
-;
-
 error_chs:
     mov ah, 0
 
@@ -202,18 +200,13 @@ error:
 
     mov si, stage1_error_msg
     call print
-    call print_line 
+    call print_line
 
-; halt after printing error details
 .halt:
     cli
     hlt
     jmp .halt
 
-;
-; MARK: data
-;
-
 %include "print.asm"
 
 stage_msg: db "Stage ",0
@@ -222,9 +215,9 @@ stage1_error_msg: db " ERROR",0
 disk: db 0
 
 chs:
-    .c: dd 0
-    .h: dd 0
-    .s: dd 0
+.c: dd 0
+.h: dd 0
+.s: dd 0
 
 DAPACK:
         db 0x10
@@ -232,4 +225,6 @@ DAPACK:
 .count: dw 0 ; int 13 resets this to # of blocks actually read/written
 .buf:   dw 0 ; memory buffer destination address (0:7c00)
 .seg:   dw 0 ; in memory page zero
-.addr:  dq 0 ; put the lba to read in this spot
+.addr:  dq 0 ; put the lba to read in this spot
+
+db 0xff

bios/stage2.asm

@@ -1,9 +1,6 @@
 SECTION .text
 USE16
 
-%include "gdt.asm"
-%include "thunk.asm"
-
 stage2.entry:
     mov si, stage_msg
     call print
@@ -16,57 +13,26 @@ stage2.entry:
     or al, 2
     out 0x92, al
 
-protected_mode:
-    ; disable interrupts
-    cli
+    mov dword [protected_mode.func], stage3.entry
+    jmp protected_mode.entry
 
-    ; load protected mode GDT
-    lgdt [gdtr]
+%include "gdt.asm"
+%include "protected_mode.asm"
+%include "thunk.asm"
 
-    ; set protected mode bit of cr0
-    mov eax, cr0
-    or eax, 1
-    mov cr0, eax
-
-    ; far jump to load CS with 32 bit segment
-    ; We need to do this because we are entering 32-bit mode,
-    ; but the instruction pipeline still has 16-bit instructions.
-    ;
-    ; gdt.pm32_code is a multiple of 8, so it always ends with three zero bits.
-    ; The GDT spec abuses this fact, and uses these last three bits to store other
-    ; data (table type and privilege). In this case, 000 is what we need anyway.
-    ;
-    ; Also note that CS isn't an address in protected mode---it's a GDT descriptor.
-    jmp gdt.pm32_code:protected_mode_inner
-
-; We can now use 32-bit instructions!
 USE32
 
-protected_mode_inner:
-    ; load all the other segments with 32 bit data segments
-    mov eax, gdt.pm32_data
-    mov ds, eax
-    mov es, eax
-    mov fs, eax
-    mov gs, eax
-    mov ss, eax
-
-    ; Place stage 3 stack at 448 KiB
-    ; (512KiB minus 64KiB disk buffer)
+stage3.entry:
+    ; stage3 stack at 448 KiB (512KiB minus 64KiB disk buffer)
     mov esp, 0x70000
 
     ; push arguments to `start()`
     mov eax, thunk.int10
     push eax
-
-    ; Call `start()`.
-    ; 0x18 skips ELF headers.
     mov eax, [stage3 + 0x18]
     call eax
-
+    
 .halt:
-    ; Halt if `start()` ever returns (it shouldn't, but just in case)
-    ; Without this, we'll try to execute whatever comes next in memory.
     cli
     hlt
     jmp .halt

bios/thunk.asm

@@ -1,10 +1,3 @@
-; Thunk allows stage 3 (rust code)
-; to use interrupts that are not 
-; usually available in protected mode.
-;
-; "thunk": a subroutine used to inject
-; a calculation into another subroutine.
-
 SECTION .text
 USE32
 

tetros/Cargo.toml

@@ -29,7 +29,7 @@ absolute_paths_not_starting_with_crate = "deny"
 explicit_outlives_requirements = "warn"
 unused_crate_dependencies = "warn"
 redundant_lifetimes = "warn"
-missing_docs = "warn"
+missing_docs = "allow"
 
 [lints.clippy]
 needless_return = "allow"

tetros/linkers/x86-unknown-none.ld

@@ -1,10 +1,9 @@
-/* This is the name of the Rust function we start in */
 ENTRY(start)
 OUTPUT_FORMAT(elf32-i386)
 
 SECTIONS {
-    /* The start address must match main.asm */
-    . = 0x8000;
+    /* The start address must match bootloader.asm */
+    . = 0x9000;
 
     . += SIZEOF_HEADERS;
     . = ALIGN(4096);

tetros/src/drivers/pic.rs

@@ -1,25 +1,18 @@
-//! Control routines for the x86
-//! 8259 Programmable Interrupt Controller
-//!
-//! This helps us configure interrupts that receive
-//! keyboard input and timer pulses.
-
 use crate::os::util::outb;
 
 /// IO base address for master PIC
 const PIC_A: u32 = 0x20;
-/// Command address for master PIC
 const PIC_A_COMMAND: u32 = PIC_A;
-/// Data address for master PIC
 const PIC_A_DATA: u32 = PIC_A + 1;
 
 /// IO base address for slave PIC
 const PIC_B: u32 = 0xA0;
-/// Command address for slave PIC
 const PIC_B_COMMAND: u32 = PIC_B;
-/// Data address for slave PIC
 const PIC_B_DATA: u32 = PIC_B + 1;
 
+/// PIC `EOI` command
+const CMD_EOI: u8 = 0x20;
+
 /// A driver for the PIC
 ///
 /// Reference:
@@ -31,7 +24,6 @@ pub struct PICDriver {
 }
 
 impl PICDriver {
-	/// Create a PIC driver with the given offsets
 	pub const fn new(offset_pic_a: u8, offset_pic_b: u8) -> Self {
 		Self {
 			offset_pic_a,
@@ -55,20 +47,14 @@ impl PICDriver {
 		unsafe { outb(PIC_B_DATA, cmd) }
 	}
 
-	/// Send an EOI for the given IRQ.
-	///
-	/// This needs to be called at the end of each interrupt handler.
-	/// If `both` is true, reset both PICs. This is only necessary
-	/// when we handle interrupts from PIC_B.
-	pub fn send_eoi(&self, both: bool) {
-		if both {
-			self.send_b_cmd(0x20);
+	pub fn send_eoi(&self, irq: u8) {
+		if irq > 8 {
+			self.send_b_cmd(CMD_EOI);
 		}
-		self.send_a_cmd(0x20);
+
+		self.send_a_cmd(CMD_EOI);
 	}
 
-	/// Initialize this PIC driver.
-	/// This should be called as early as possible.
 	pub fn init(&mut self) {
 		const ICW1_ICW4: u8 = 0x01; /* Indicates that ICW4 will be present */
 		const ICW1_INIT: u8 = 0x10; /* Initialization - required! */

tetros/src/drivers/serial.rs

@@ -1,10 +1,3 @@
-//! Serial port driver, for debug.
-//!
-//! This file provides the usual `print`
-//! and `println` macros (which are usually
-//! provided by `std`) that send messages out
-//! of the serial port.
-
 use lazy_static::lazy_static;
 use spin::Mutex;
 use uart_16550::SerialPort;

tetros/src/drivers/vga.rs

@@ -3,6 +3,7 @@ use rand::seq::IndexedRandom;
 
 use crate::RNG;
 
+#[repr(u8)]
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
 pub enum VgaColor {
 	Black,
@@ -72,9 +73,6 @@ impl Vga13h {
 	pub const HEIGHT: usize = 200;
 	pub const ADDR: usize = 0xA0000;
 
-	/// Initialize a new VGA driver.
-	///
-	/// Only one of these should exist.
 	pub const unsafe fn new() -> Self {
 		Self {
 			fb_a: [0; Vga13h::WIDTH * Vga13h::HEIGHT],

tetros/src/game/board.rs

@@ -2,26 +2,21 @@ use crate::drivers::vga::{Vga13h, VgaColor};
 
 use super::FallingTetromino;
 
-/// The state of a cell in the game board
+#[repr(u8)]
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
 pub enum TetrisCell {
 	Empty,
 	Filled { color: VgaColor },
 }
 
-/// The tetris board
 pub struct TetrisBoard {
 	board: [TetrisCell; TetrisBoard::BOARD_WIDTH * TetrisBoard::BOARD_HEIGHT],
 }
 
 impl TetrisBoard {
-	/// The width of this board, in cells
 	const BOARD_WIDTH: usize = 10;
-
-	/// The height of this board, in cells
 	const BOARD_HEIGHT: usize = 20;
 
-	/// The side length of a (square) cell, in pixels
 	const CELL_SIZE: usize = 9;
 
 	pub const fn new() -> Self {
@@ -30,8 +25,6 @@ impl TetrisBoard {
 		}
 	}
 
-	/// Find and remove all filled rows,
-	/// shifting upper rows down.
 	pub fn collapse(&mut self) {
 		let mut y = Self::BOARD_HEIGHT - 1;
 		'outer: loop {
@@ -65,11 +58,6 @@ impl TetrisBoard {
 		}
 	}
 
-	/// Place the given tetromino on the board,
-	/// filling the cells it occupies.
-	///
-	/// If the tetromino cells that overlap
-	/// non-empty board cells are ignored.
 	pub fn place_tetromino(&mut self, tetromino: FallingTetromino) {
 		for (x, y) in tetromino.tiles() {
 			let cell = self.get_cell_mut(x, y);
@@ -94,8 +82,6 @@ impl TetrisBoard {
 		return true;
 	}
 
-	/// Get the value of the cell at the given position.
-	/// Returns [`None`] if (x, y) exceeds the board's bounds.
 	pub fn get_cell(&self, x: usize, y: usize) -> Option<&TetrisCell> {
 		if y >= TetrisBoard::BOARD_HEIGHT {
 			return None;
@@ -105,11 +91,9 @@ impl TetrisBoard {
 			return None;
 		}
 
-		return self.board.get(y * TetrisBoard::BOARD_WIDTH + x);
+		return Some(&self.board[y * TetrisBoard::BOARD_WIDTH + x]);
 	}
 
-	/// Get a mutable reference to the cell at the given position.
-	/// Returns [`None`] if (x, y) exceeds the board's bounds.
 	pub fn get_cell_mut(&mut self, x: usize, y: usize) -> Option<&mut TetrisCell> {
 		if y >= TetrisBoard::BOARD_HEIGHT {
 			return None;
@@ -119,17 +103,11 @@ impl TetrisBoard {
 			return None;
 		}
 
-		return self.board.get_mut(y * TetrisBoard::BOARD_WIDTH + x);
+		return Some(&mut self.board[y * TetrisBoard::BOARD_WIDTH + x]);
 	}
 }
 
-//
-// MARK: draw routines
-//
-
 impl TetrisBoard {
-	/// Draw a cell of the given color on `fb`.
-	/// (x, y) is the pixel position of the cell (NOT board coordinates).
 	fn draw_cell(&self, fb: &mut [u8], color: VgaColor, x: usize, y: usize) {
 		let color = color.as_u8();
 		for yo in 0..TetrisBoard::CELL_SIZE {
@@ -139,7 +117,6 @@ impl TetrisBoard {
 		}
 	}
 
-	/// Draw the tetris board's frame
 	fn draw_frame(&self, fb: &mut [u8], x: usize, y: usize) {
 		let color = VgaColor::Gray.as_u8();
 		for yo in 0..TetrisBoard::CELL_SIZE {
@@ -149,7 +126,6 @@ impl TetrisBoard {
 		}
 	}
 
-	/// Draw this tetris board using the given VGA driver.
 	pub fn draw(&self, vga: &mut Vga13h, falling: Option<&FallingTetromino>) {
 		let fb = vga.get_fb();
 

tetros/src/game/falling.rs

@@ -53,7 +53,6 @@ pub enum Direction {
 }
 
 impl Direction {
-	/// Rotate this direction clockwise
 	pub fn rot_cw(self) -> Self {
 		match self {
 			Self::North => Self::East,
@@ -62,6 +61,17 @@ impl Direction {
 			Self::West => Self::North,
 		}
 	}
+
+	/*
+	pub fn rot_ccw(self) -> Self {
+		match self {
+			Self::North => Self::West,
+			Self::West => Self::South,
+			Self::South => Self::East,
+			Self::East => Self::North,
+		}
+	}
+	*/
 }
 
 #[derive(Debug, Clone)]
@@ -75,7 +85,6 @@ pub struct FallingTetromino {
 }
 
 impl FallingTetromino {
-	/// Make a new falling tetromino
 	pub fn new(tetromino: Tetromino, color: VgaColor, center_x: usize, center_y: usize) -> Self {
 		Self {
 			tetromino,
@@ -86,7 +95,6 @@ impl FallingTetromino {
 		}
 	}
 
-	/// Generate a random tetromino at the given position
 	pub fn random(center_x: usize, center_y: usize) -> Self {
 		Self::new(
 			Tetromino::choose_rand(),
@@ -96,7 +104,6 @@ impl FallingTetromino {
 		)
 	}
 
-	// Move this tetromino
 	pub fn translate(&mut self, x: i16, y: i16) {
 		if x > 0 {
 			let x = usize::try_from(x).unwrap();
@@ -115,11 +122,16 @@ impl FallingTetromino {
 		}
 	}
 
-	/// Rotate this tetromino clockwise
 	pub fn rotate_cw(&mut self) {
 		self.direction = self.direction.rot_cw()
 	}
 
+	/*
+	pub fn rotate_ccw(&mut self) {
+		self.direction = self.direction.rot_ccw()
+	}
+	*/
+
 	/// Returns the positions of this falling tetromino's tiles.
 	pub fn tiles(&self) -> [(usize, usize); 4] {
 		match (&self.tetromino, self.direction) {

tetros/src/game/mod.rs

@@ -1,6 +1,3 @@
-//! This crate contains all tetris game logic.
-//! No low-level magic here.
-
 mod board;
 pub use board::*;
 

tetros/src/idt/stackframe.rs

@@ -1,14 +1,16 @@
 use core::{fmt, ops::Deref};
 
-use super::VirtAddr;
 use crate::os::EFlags;
 
+use super::VirtAddr;
+
 /// Wrapper type for the interrupt stack frame pushed by the CPU.
 ///
 /// This type derefs to an [`InterruptStackFrameValue`], which allows reading the actual values.
 ///
-/// This wrapper ensures that the stack frame cannot be modified.
-/// This prevents undefined behavior.
+/// This wrapper type ensures that no accidental modification of the interrupt stack frame
+/// occurs, which can cause undefined behavior (see the [`as_mut`](InterruptStackFrame::as_mut)
+/// method for more information).
 #[repr(transparent)]
 pub struct InterruptStackFrame(InterruptStackFrameValue);
 

tetros/src/idt/table.rs

@@ -5,6 +5,17 @@ use super::{
 	HandlerFuncWithErrCode, PageFaultHandlerFunc,
 };
 
+// TODO: comments
+#[repr(C, packed(2))]
+struct Idtr {
+	size: u16,
+	offset: u32,
+}
+
+//
+// MARK: idt
+//
+
 // spell:off
 #[derive(Clone, Debug)]
 #[repr(C)]
@@ -456,17 +467,12 @@ impl InterruptDescriptorTable {
 	/// # Safety
 	///
 	/// As long as it is the active IDT, you must ensure that:
+	///
 	/// - `self` is never destroyed.
 	/// - `self` always stays at the same memory location.
+	///   It is recommended to wrap it in a `Box`.
 	#[inline]
 	pub unsafe fn load_unsafe(&self) {
-		/// The data we push to the IDTR register
-		#[repr(C, packed(2))]
-		struct Idtr {
-			size: u16,
-			offset: u32,
-		}
-
 		let idtr = {
 			Idtr {
 				size: (size_of::<InterruptDescriptorTable>() - 1) as u16,

tetros/src/idt/virtaddr.rs

@@ -1,4 +1,7 @@
-use core::fmt::{self};
+use core::{
+	fmt::{self},
+	ops::{Add, AddAssign, Sub, SubAssign},
+};
 
 /// A canonical 32-bit virtual memory address.
 #[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
@@ -54,3 +57,40 @@ impl fmt::UpperHex for VirtAddr {
 		fmt::UpperHex::fmt(&self.0, f)
 	}
 }
+
+impl fmt::Pointer for VirtAddr {
+	#[inline]
+	fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+		fmt::Pointer::fmt(&(self.0 as *const ()), f)
+	}
+}
+
+impl Add<u32> for VirtAddr {
+	type Output = Self;
+	#[inline]
+	fn add(self, rhs: u32) -> Self::Output {
+		VirtAddr(self.0.checked_add(rhs).unwrap())
+	}
+}
+
+impl AddAssign<u32> for VirtAddr {
+	#[inline]
+	fn add_assign(&mut self, rhs: u32) {
+		*self = *self + rhs;
+	}
+}
+
+impl Sub<u32> for VirtAddr {
+	type Output = Self;
+	#[inline]
+	fn sub(self, rhs: u32) -> Self::Output {
+		VirtAddr(self.0.checked_sub(rhs).unwrap())
+	}
+}
+
+impl SubAssign<u32> for VirtAddr {
+	#[inline]
+	fn sub_assign(&mut self, rhs: u32) {
+		*self = *self - rhs;
+	}
+}

tetros/src/lib.rs

@@ -1,5 +1,3 @@
-//! The main code of tetris
-
 #![no_std]
 #![feature(int_roundings)]
 #![feature(lang_items)]
@@ -25,18 +23,12 @@ mod os;
 #[macro_use]
 mod drivers;
 
+const PIC_OFFSET: u8 = 32;
+
 //
 // MARK: globals
 //
-// This code has no parallelism, so we don't _really_
-// need locks. The Mutexes here satisfy Rust's
-// "no mutable global state" rule.
-//
-// They also help prevent bugs, since we get deadlocks
-// instead of hard-to-debug surprising behavior.
-//
 
-const PIC_OFFSET: u8 = 32;
 static VGA: Mutex<Vga13h> = Mutex::new(unsafe { Vga13h::new() });
 static PIC: Mutex<PICDriver> = Mutex::new(PICDriver::new(PIC_OFFSET, PIC_OFFSET + 8));
 static TICK_COUNTER: Mutex<u32> = Mutex::new(0);
@@ -44,8 +36,6 @@ static BOARD: Mutex<TetrisBoard> = Mutex::new(TetrisBoard::new());
 static FALLING: Mutex<Option<FallingTetromino>> = Mutex::new(None);
 static LAST_INPUT: Mutex<Option<InputKey>> = Mutex::new(None);
 
-// These values can't be initialized statically,
-// so we cheat with `lazy_static`
 lazy_static! {
 	static ref RNG: Mutex<SmallRng> = Mutex::new(SmallRng::seed_from_u64(1337));
 	static ref IDT: InterruptDescriptorTable = {
@@ -60,26 +50,10 @@ lazy_static! {
 	};
 }
 
-#[derive(Debug, Clone, Copy, PartialEq, Eq)]
-enum InputKey {
-	Left,
-	Right,
-	Up,
-	Down,
-}
-
 //
 // MARK: interrupts
 //
-// These functions are called when we receive interrupts.
-// This can occur between ANY two instructions---which is
-// why we use `without_interrupts` when acquiring locks.
-//
-// Notice how we do as little work as possible in our
-// interrupt handlers. All our business logic goes into
-// the main loop.
 
-#[expect(missing_docs)]
 #[derive(Debug, Clone, Copy)]
 #[repr(u8)]
 pub enum InterruptIndex {
@@ -98,20 +72,22 @@ impl InterruptIndex {
 }
 
 extern "x86-interrupt" fn divide_handler(stack_frame: InterruptStackFrame) {
-	// Simple interrupt handler, as an example.
-	// This can be triggered manually using `asm!("int 0")`,
-	// even if interrupts are disabled.
 	println!("DIVIDE ERROR {:?}", stack_frame);
 }
 
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+enum InputKey {
+	Left,
+	Right,
+	Up,
+	Down,
+}
+
 extern "x86-interrupt" fn keyboard_handler(_stack_frame: InterruptStackFrame) {
 	{
 		// Re-seed our rng using user input.
 		// This is a simple hack that makes our
 		// "random" tile selector less deterministic.
-		//
-		// Getting random seeds from hardware is
-		// more trouble than its worth.
 		let mut rng = RNG.lock();
 		let past: u64 = rng.random();
 		let tcr = u64::from(*TICK_COUNTER.lock());
@@ -149,13 +125,13 @@ extern "x86-interrupt" fn keyboard_handler(_stack_frame: InterruptStackFrame) {
 		*LAST_INPUT.lock() = key;
 	}
 
-	PIC.lock().send_eoi(false);
+	PIC.lock().send_eoi(InterruptIndex::Keyboard.as_u8());
 }
 
 extern "x86-interrupt" fn timer_handler(_stack_frame: InterruptStackFrame) {
 	let mut t = TICK_COUNTER.lock();
 	*t = (*t).wrapping_add(1);
-	PIC.lock().send_eoi(false);
+	PIC.lock().send_eoi(InterruptIndex::Timer.as_u8());
 }
 
 extern "x86-interrupt" fn double_fault_handler(
@@ -169,7 +145,6 @@ extern "x86-interrupt" fn double_fault_handler(
 // MARK: main
 //
 
-#[expect(missing_docs)]
 #[no_mangle]
 pub unsafe extern "C" fn start(thunk10: extern "C" fn()) -> ! {
 	println!("Entered Rust, serial ready.");
@@ -215,7 +190,6 @@ pub unsafe extern "C" fn start(thunk10: extern "C" fn()) -> ! {
 		}
 		last_t = t;
 
-		// MARK: input
 		// Handle user input
 		without_interrupts(|| {
 			if let Some(fall) = &mut *FALLING.lock() {
@@ -262,7 +236,6 @@ pub unsafe extern "C" fn start(thunk10: extern "C" fn()) -> ! {
 			}
 		});
 
-		// MARK: update board
 		// Update board
 		without_interrupts(|| {
 			let mut v = VGA.lock();

tetros/src/os/eflags.rs

@@ -80,7 +80,6 @@ bitflags! {
 }
 
 impl EFlags {
-	/// Read the EFLAGS register
 	#[inline]
 	pub fn read() -> EFlags {
 		EFlags::from_bits_truncate(EFlags::read_raw())

tetros/src/os/panic.rs

@@ -1,12 +1,8 @@
-//! Rust intrinsics for panic handling.
-//!
-//! These are usually provided by `std`,
-//! but we don't have that luxury!
+//! Intrinsics for panic handling
 
 use core::arch::asm;
 use core::panic::PanicInfo;
 
-// Use serial println
 use crate::println;
 
 #[lang = "eh_personality"]

tetros/src/os/util.rs

@@ -18,13 +18,11 @@ pub fn sti() {
 	}
 }
 
+/// Run a closure with disabled interrupts.
+///
 /// Run the given closure, disabling interrupts before running it (if they aren't already disabled).
 /// Afterwards, interrupts are enabling again if they were enabled before.
 ///
-/// This helps us prevent deadlocks, which can occur if
-/// an interrupt handler tries to acquire a lock that was
-/// locked at the time of the interrupt.
-///
 /// If you have other `enable` and `disable` calls _within_ the closure, things may not work as expected.
 #[inline]
 pub fn without_interrupts<F, R>(f: F) -> R
@@ -48,7 +46,6 @@ where
 	ret
 }
 
-/// Wraps the `in` instruction
 pub unsafe fn inb(port: u32) -> u8 {
 	let mut out;
 
@@ -61,7 +58,6 @@ pub unsafe fn inb(port: u32) -> u8 {
 	return out;
 }
 
-/// Wraps the `out` instruction
 pub unsafe fn outb(port: u32, value: u8) {
 	asm!(
 		"out dx, al",