Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

boot: convert paging from assembly to Zig #19

Closed
wants to merge 26 commits into from
Closed

boot: convert paging from assembly to Zig #19

wants to merge 26 commits into from

Conversation

mewmew
Copy link
Collaborator

@mewmew mewmew commented May 23, 2022

The main focus of this PR is to make it easier to work with paging.

For this reason, the kernel code and data segments are treated separately by dedicated functions, and each function has a start index (e.g. P2_KERNEL_STACK_FIRST_INDEX) to make it easier to rearrange the relative order of page mappings in the future.

src/zasm.zig Outdated Show resolved Hide resolved
src/zasm.zig Outdated Show resolved Hide resolved
src/zasm.zig Outdated Show resolved Hide resolved
src/zasm.zig Outdated Show resolved Hide resolved
src/zasm.zig Outdated Show resolved Hide resolved
src/zasm.zig Outdated Show resolved Hide resolved
src/zasm.zig Outdated Show resolved Hide resolved
src/zasm.zig Outdated Show resolved Hide resolved
src/zasm.zig Outdated Show resolved Hide resolved
src/zasm.zig Outdated Show resolved Hide resolved
src/zasm.zig Outdated Show resolved Hide resolved
@karlek
Copy link
Owner

karlek commented May 24, 2022

I'll ask a ton of question over the coming days to learn zig and review your code :P

@mewmew
Copy link
Collaborator Author

mewmew commented May 24, 2022

I'll ask a ton of question over the coming days to learn zig and review your code :P

Gör så! :) Jag lär mig också så kul att se vad som känns skumt och ointuitivt.

src/zasm.zig Outdated Show resolved Hide resolved
src/zasm.zig Outdated Show resolved Hide resolved
src/zasm.zig Outdated
const TargetType = @TypeOf(target);
const ReturnType = std.meta.Int(.unsigned, number_of_bits);

comptime {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perform extra checks if we can during compile time? Otherwise, skip these checks if we call this function with runtime resolved values?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are always run at compile time. And, we cannot call this function with runtime resolved values, they must be compile-time known. That is number_of_bits is marked as comptime in the function signature, The TargetType type is known for each call invocation at compile time, etc.

src/zasm.zig Outdated Show resolved Hide resolved
src/zasm.zig Outdated
// ~~~ [ zig-x86_64/src/instructions/tlb.zig ] ~~~

/// Structure of a PCID. A PCID has to be <= 4096 for x86_64.
pub const Pcid = packed struct {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this seem overly boilerplate:y / overly verbose? Can't we just have a type synonym like:

PcidType = u12

Copy link
Collaborator Author

@mewmew mewmew May 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. This feels like unnecessary boiler plate. It would make sense if they added methods to Pcid.

We can do an alias like this:

const PcidType = u12;

However, I still propose that we keep the code as is, as it makes maintenance easier when we update the code against the x86_64 repo. Make minimal changes unless warranted. And in this case, the "make it slightly cleaner", vs. "simplify maintenance" does not seem worth it.

However, I can be persuaded of course :)

src/zasm.zig Outdated Show resolved Hide resolved
src/zasm.zig Outdated Show resolved Hide resolved
src/zasm.zig Outdated

/// Creates a new physical address, throwing bits 52..64 away.
pub fn initTruncate(addr: u64) PhysAddr {
return PhysAddr{ .value = addr % TRUNCATE_CONST };
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting choice to use modulo (or does zig use remainder?) instead of bit mask AND which seems like the more common pattern.

Copy link
Collaborator Author

@mewmew mewmew May 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, this is a strange use of modulo. Had I written the code myself, I would also have preferred using a bit mask with AND.

src/zasm.zig Outdated Show resolved Hide resolved
src/zasm.zig Outdated

fn Msr(comptime register: u32) type {
return struct {
pub inline fn read() u64 {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why explicitly make this method inline?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid function prologue and epilogue of essentially single-line assembly functions.

In some cases it may be for performance, but the compiler should be able to figure those out.

However, sometimes using inline is essential for the correct handling of assembly code. For instance, if we wish to read or write the rsp value. Lets say we do this without using inline, then the function prologue and epilogue would trash the rsp value before we have time to read/write to it. So, in these cases, just inline the asm code into the caller function.

src/zasm.zig Outdated Show resolved Hide resolved
src/zasm.zig Outdated
}
};

fn Msr(comptime register: u32) type {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this type private? Or how does exporting work in zig?

Copy link
Collaborator Author

@mewmew mewmew May 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Msr function is private, since it does not have the pub attribute.

As such, all invocations to Msr are done from within this source file. However, their return value (note: in Zig, types are values and can be passed as parameters or returned by functions) may be exported if stored in e.g. a field of an exported struct.

This is the case with the Efer struct, which is exported (i.e. has pub attribute):

/// The Extended Feature Enable Register.
pub const Efer = packed struct {
    ...
    const REGISTER = Msr(0xC000_0080);

src/zasm.zig Outdated
// ~~~ [ zig-x86_64/src/registers/model_specific.zig ] ~~~

/// The Extended Feature Enable Register.
pub const Efer = packed struct {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sweet lord, this is nice haha

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Packed structs?

Makefile Outdated
# -O ReleaseFast
bin/boot_zig.o: src/boot/boot.zig | bin
zig build-obj -target x86_64-freestanding-gnu -static -I./src/kernel -mno-red-zone -femit-bin=$@ $<
PKGS=--pkg-begin zasm ./src/zasm.zig --pkg-end
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we call these source files zasm? So they are then imported as @import("zasm")?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we give the source file ./src/zasm.zig the package name zasm. Note, the source file could have been called hello_world.zig and we could still assign it to the package name zasm.

This is a getto way to handle packages, until the package manager is implemented in Zig (ziglang/zig#943).

$<

bin/%_elf64_zig.o: bin/%_32_zig.o
@objcopy --output-target elf64-x86-64 $< $@
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hack-hackli-hack

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a todo to remove this when we can solve this in zig?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure! You don't like hack-hackiilihack?? ;P

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done dc2d8cb.

; elf64 format. The call from multiboot_start to init_long_mode is converted
; to call init_long_mode+4 after converting from elf32 to elf64. Thus we
; insert four nops.
times 4 nop
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hack-hackli-hack 🐓

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:huge: 🐘 :in: :room:

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done ed18c7d.

@@ -0,0 +1,31 @@
const zasm = @import("zasm");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My guess was confirmed!


export fn enable_paging() void {
// Load P4 to CR3 register. (CPU uses this to access the P4 table).
const raw_p4_addr = @intCast(u64, @ptrToInt(&p4_table[0]));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it &p4_table[0] and not just &p4_table?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type is automatically assumed to be u64 from the @intCast?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it &p4_table[0] and not just &p4_table?

&p4_table[0] results in a single-item pointer, whereas &p4_table results in a pointer to the array (This would be the same as taking the address of an array (&array has type *[4]int, whereas &array[0] has type *int) or a slice in Go, which gives a pointer to a reflect.SliceHeader struct).

In Zig there is also the notion of many-item pointers. If we have a slice and use the ptr field, we get a many-item pointer which essentially means a pointer to zero or more items of the slice element type. This is essentially what all pointers in C mean :P

Note: a single-item pointer and a multi-item pointer to the first element of a slice both have the same pointer address, they just have different types.

See example below :)

const std = @import("std");

pub fn main() anyerror!void {
    var array = [4]u8{ 1, 2, 3, 4 };
    var slice = array[1..3];
    foo(slice);
    bar(array);
}

pub fn foo(slice: []u8) void {
    std.debug.print("=== [ slice ] ===\n", .{});

    std.debug.print("ptrs:\n", .{});
    std.debug.print("   {any}\n", .{&slice[0]});
    std.debug.print("   {any}\n", .{&slice});
    std.debug.print("   {any}\n", .{slice.ptr});
    std.debug.print("types:\n", .{});
    std.debug.print("   {any}\n", .{@TypeOf(&slice[0])});
    std.debug.print("   {any}\n", .{@TypeOf(&slice)});
    std.debug.print("   {any}\n", .{@TypeOf(slice.ptr)});

    std.debug.print("\n", .{});
}

pub fn bar(array: [4]u8) void {
    std.debug.print("=== [ array ] ===\n", .{});

    std.debug.print("ptrs:\n", .{});
    std.debug.print("   {any}\n", .{&array[0]});
    std.debug.print("   {any}\n", .{&array});

    std.debug.print("types:\n", .{});
    std.debug.print("   {any}\n", .{@TypeOf(&array[0])});
    std.debug.print("   {any}\n", .{@TypeOf(&array)});

    std.debug.print("\n", .{});
}

Output:

$ zig run bar.zig
=== [ slice ] ===
ptrs:
   u8@7ffc9d803c2b
   []u8@7ffc9d803c00
   u8@7ffc9d803c2b
types:
   *u8
   *const []u8
   [*]u8

=== [ array ] ===
ptrs:
   u8@7ffc9d803c2a
   [4]u8@7ffc9d803c2a
types:
   *const u8
   *const [4]u8

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type is automatically assumed to be u64 from the @intcast?

The type is converted to u64 while ensuring that the result type fits all the bits of the input value, otherwise it panics. In our case, this would either be a u64 to u64 conversion or an u32 to u64 conversion, so in both cases the source value is preserved.

ref: https://ziglang.org/documentation/master/#intCast

export fn enable_paging() void {
// Load P4 to CR3 register. (CPU uses this to access the P4 table).
const raw_p4_addr = @intCast(u64, @ptrToInt(&p4_table[0]));
var p4_addr = zasm.PhysAddr.initUnchecked(raw_p4_addr);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not const?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not const?

Done d85d018.

Comment on lines 8 to 13
var p4_addr = zasm.PhysAddr.initUnchecked(raw_p4_addr);
var cr3_value = zasm.Cr3.Contents{
.phys_frame = zasm.PhysFrame.fromStartAddressUnchecked(p4_addr),
.cr3_flags = zasm.Cr3Flags.fromU64(0),
};
zasm.Cr3.write(cr3_value);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we move this closer to the enable paging functionality at the end of the function for closer logical distance?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done 5b2cf08.

// Set the NX bit.
efer_value.no_execute_enable = true; // 1 << 11
efer_value.write();

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: Add assert zasm.Efer.read().long_mode_active == true?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done c0a85cb.


extern var p4_table: [4096]u8;

export fn enable_paging() void {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well this was readable hahaah

src/zasm.zig Outdated
/// - `FrameError::FrameNotPresent` if the entry doesn't have the `present` flag set.
/// - `FrameError::HugeFrame` if the entry has the `huge_page` flag set (for huge pages the
/// `addr` function must be used)
pub fn getFrame(self: PageTableEntry) FrameError!PhysFrame {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is FrameError the same as error{FrameNotPresent, HugeFrame}, or is it a new type?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FrameError is an alias for the error set error{FrameNotPresent, HugeFrame}, so they are one and the same and can be used interchangeably.

src/zasm.zig Outdated

/// Map the entry to the specified physical address
pub fn setAddr(self: *PageTableEntry, addr: PhysAddr) void {
//std.debug.assert(addr.isAligned(PageSize.Size4KiB.bytes()));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assert seems reasonable, re-introduce it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commented out for now, since it works in 64-bit, but not 32-bit. And we are compiling paging_32.asm for 32-bit.

$ make clean; make run
./src/zasm.zig:734:64: error: expected type 'usize', found 'u64'
        std.debug.assert(addr.isAligned(PageSize.Size4KiB.bytes()));
                                                               ^
./src/zasm.zig:734:64: note: unsigned 32-bit int cannot represent all possible unsigned 64-bit values
        std.debug.assert(addr.isAligned(PageSize.Size4KiB.bytes()));
                                                               ^
./src/zasm.zig:623:38: error: expected type 'usize', found 'u64'
        return std.mem.isAligned(self.value, alignment);
                                     ^
./src/zasm.zig:623:38: note: unsigned 32-bit int cannot represent all possible unsigned 64-bit values
        return std.mem.isAligned(self.value, alignment);
                                     ^
make: *** [Makefile:18: bin/paging_32_zig.o] Error 1
rm bin/boot_32_zig.o

Copy link
Collaborator Author

@mewmew mewmew May 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added TODO to re-enable assert for setAddr in ab5cc59.

mewmew added a commit that referenced this pull request May 25, 2022
@@ -61,7 +61,10 @@ export fn map_kernel_code_segment() void {
page_table_entry.setAddr(addr);
// Set page table entry.
const p2_index = page_num + p2_index_offset;
p2_table[p2_index] = page_table_entry.entry;
// TODO: uncomment when Zig 32-bit codegen bug is fixed; use work-around for now.
Copy link
Collaborator Author

@mewmew mewmew May 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this is a bug in ld, not Zig. The ELF32 object file produced by Zig contains the correct assembly. And the ELF64 object file produced by objcopy also contains the correct assembly. But the assembly code produced by ld is trashed, such that the high 32-bits of page_table_entry when stored in p2_table are trashed (overwritten by the low 32-bits). Furthermore, using the LLVM linker ld.lld produces correct assembly. So, we can conclude that there is a bug in ld that we manage to tickle in just the right way.

CORRECT ASM: map_kernel_stack (ELF32 object produced by Zig):

 16b:   89 14 c5 04 00 00 00    mov    DWORD PTR [eax*8+0x4],edx
 172:   89 0c c5 00 00 00 00    mov    DWORD PTR [eax*8+0x0],ecx

CORRECT ASM: map_kernel_stack (ELF64 object produced by objcopy):

 16b:   89 14 c5 04 00 00 00    mov    DWORD PTR [rax*8+0x4],edx
 172:   89 0c c5 00 00 00 00    mov    DWORD PTR [rax*8+0x0],ecx

INCORRECT ASM: map_kernel_stack (ELF64 executable produced by ld):

  10016b:       89 14 c5 00 20 10 00    mov    DWORD PTR [rax*8+0x102000],edx ; INCORRECT: should be `mov    DWORD PTR [rax*8+0x102004],edx`
  100172:       89 0c c5 00 20 10 00    mov    DWORD PTR [rax*8+0x102000],ecx

CORRECT ASM: map_kernel_stack (ELF64 executable produced by ld.lld (LLVM's linker)):

  10016b:       89 14 c5 04 20 10 00    mov    DWORD PTR [rax*8+0x102004],edx
  100172:       89 0c c5 00 20 10 00    mov    DWORD PTR [rax*8+0x102000],ecx

@mewmew mewmew force-pushed the less-asm branch 2 times, most recently from cdf0142 to d229b12 Compare May 26, 2022 20:33
This was needed for debugging before the jump to long mode was
fully functional.
This was used to switch from 16-bit segmented mode to
32-bit protected mode.

As Lappis is now using Multiboot, the switch from 16-bit to
32-bit mode is done by the multiboot boot loader.
This was used for debug printing in 16-bit mode.

As Lappis is now using multiboot, the boot loader handles the switch
from 16-bit to 32-bit mode.
Prior to this commit, set_up_page_tables was used to both initialize the
page table tree mapping and to map kernel code and data segment pages.

Split out the latter two into dedicated functions.
The the foo_32.zig files are compiled for 32-bit architecture, and
functions are exported in the object file using the `export` keyword.

To avoid re-exporting functions and facilitate re-use of declared
constants, move paging constants to their own source file and
include this source file where needed.
Turns out there is a codegen bug in Zig 0.9.1 when
generating 32-bit code that operate on 64-bit arrays.

In particular, `foo[index] = bar`, where `foo` is an
`[512]u64` array, causes the high 32-bit of `bar` to be
discarded.

The assembly code actually attempts to set the high 32-bit
of bar, but uses the incorrect offset, and thus overwrites
the value with the low 32-bit part of bar.

This issue was identified as the NX bit was _not_ set in
the p2_table, even though the Zig code contained a
`page_table_entry.no_execute = true` statement.

Gotta love compiler bugs that makes mapped pages w+x :)

Tribute to Ken on Trusting Trust <3

Prior to this fix, `info tlb` gave:

	(qemu) info tlb
	// kernel code
	0000000000000000: 0000000000000000 --PDA--UW
	// kernel data:
	0000000000200000: 0000000000200000 --PDA--UW
	...
	0000000003e00000: 0000000003e00000 --P----UW
	// kernel stack (guard)
	0000000004000000: 0000000004000000 --P----U-
	// kernel stack
	0000000004200000: 0000000004200000 --P----UW
	0000000004400000: 0000000004400000 --PDA--UW
	// kernel stack (guard)
	0000000004600000: 0000000004600000 --P----U-
	// frame buffer:
	0000000004800000: 00000000fd000000 --PDA--UW
	0000000004a00000: 00000000fd200000 --PDA--UW
	0000000004c00000: 00000000fd400000 --PDA--UW

After this commit, `info tlb` gives:

	(qemu) info tlb
	// kernel code
	0000000000000000: 0000000000000000 --PDA--UW
	// kernel data:
	0000000000200000: 0000000000200000 X-PDA--UW
	...
	0000000003e00000: 0000000003e00000 X-P----UW
	// kernel stack (guard)
	0000000004000000: 0000000004000000 X-P----U-
	// kernel stack
	0000000004200000: 0000000004200000 X-P----UW
	0000000004400000: 0000000004400000 X-PDA--UW
	// kernel stack (guard)
	0000000004600000: 0000000004600000 X-P----U-
	// frame buffer:
	0000000004800000: 00000000fd000000 X-PDA--UW
	0000000004a00000: 00000000fd200000 X-PDA--UW
	0000000004c00000: 00000000fd400000 X-PDA--UW

For the morbidly curious, this is the incorrect assembly as
generated by Zig 0.9.1 32-bit codegen:

	0x1006a5 <map_kernel_stack+341> mov    ecx, DWORD PTR [rbp-0x8]        ; 0x4200087
	0x1006a8 <map_kernel_stack+344> mov    edx, DWORD PTR [rbp-0x4]        ; 0x80000000
	0x1006ab <map_kernel_stack+347> mov    DWORD PTR [rax*8+0x112000], edx ; BUG! writes high 32-bit part of page_table_entry (including NX bit) to incorrect offset.
	0x1006b2 <map_kernel_stack+354> mov    DWORD PTR [rax*8+0x112000], ecx ; overwrites previous write with low 32-bit part of page_table_entry (overwrites NX bit).
In Zig 0.11.0, the package management system was rewritten,
and as such the support for --begin-pkg was removed.

To simplify the story of package management now, simply include
zasm.zig as a file (rather than placing it in a separate Git
repository).

Since Zig doesn't allow local import of files outside of the
root directory of the Zig file that is using the import, move
zasm.zig to the boot/ directory.
@mewmew
Copy link
Collaborator Author

mewmew commented Mar 22, 2024

Nu är less-asm branchen re-base'ad på main och bygger med Zig 0.11.0.

Fick lägga till ett till "hack" för att få compilen att funka, då vi kör statisk länkning så var det en runtime funktion till Zig som inte följde med när vi länka vår kernel:

# NOTE: use `-fcompiler-rt` to include __zig_probe_stack symbol when linking

För tillfället så bygger den som den ska och kan köra för att visa doggie : )

@mewmew
Copy link
Collaborator Author

mewmew commented Apr 15, 2024

This PR has been superseded by #23.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants