Skip to content

Use new assembly syntax#165

Merged
josephlr merged 8 commits into
masterfrom
unknown repository
Sep 30, 2020
Merged

Use new assembly syntax#165
josephlr merged 8 commits into
masterfrom
unknown repository

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Jun 27, 2020

This pull request updates x86_64 to use the new asm! macro. This will break older nightlies, but makes the syntax easier to read.

Fixes #164

@ghost
Copy link
Copy Markdown
Author

ghost commented Jun 27, 2020

This PR should be ready to merge. I have tested it with Blog OS and it works fine, but this does not mean that it will always work

We really need to write a full test suite for this crate. Also can I please be added to the x86_64 team phil?

cc @phil-opp

@ghost ghost marked this pull request as ready for review June 27, 2020 21:50
Copy link
Copy Markdown
Member

@phil-opp phil-opp left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the pull request! I have some minor comments, but overall this looks good to me.

Comment thread Changelog.md Outdated
Comment thread src/instructions/interrupts.rs Outdated
Comment thread src/registers/mod.rs Outdated
Comment thread src/registers/model_specific.rs Outdated
Comment thread src/registers/rflags.rs
@phil-opp
Copy link
Copy Markdown
Member

We really need to write a full test suite for this crate.

Absolutely! This would be very useful for changes like this.

Also can I please be added to the x86_64 team phil?

Done!

@ghost
Copy link
Copy Markdown
Author

ghost commented Jun 30, 2020

All of those have been fixed, feel free to merge when you want

@ghost ghost requested a review from phil-opp June 30, 2020 12:29
@phil-opp
Copy link
Copy Markdown
Member

Sorry for the late reply! The code looks good to me overall, but the double_fault_stack_overflow test seems to fail on CI:

Running: qemu-system-x86_64 -drive format=raw,file=/home/runner/work/x86_64/x86_64/testing/target/x86_64-bare-metal/debug/deps/bootimage-double_fault_stack_overflow-0254781a0bba0dab.bin -device isa-debug-exit,iobase=0xf4,iosize=0x04 -serial stdio -display none
qemu-system-x86_64: warning: TCG doesn't support requested feature: CPUID.01H:ECX.vmx [bit 5]
qemu-system-x86_64: Trying to execute code outside RAM or ROM at 0x0000000108234489

I'm not sure what's the cause of this, but it seems like something goes wrong with the IDT or TSS loading.

@ghost
Copy link
Copy Markdown
Author

ghost commented Jul 17, 2020

I believe that's caused by the code that reloads cs, but I can't identify why. I'll do some testing today and then merge this when CI passes

@phil-opp
Copy link
Copy Markdown
Member

phil-opp commented Aug 4, 2020

@rybot666 Any updates on this?

@ghost
Copy link
Copy Markdown
Author

ghost commented Aug 5, 2020

Sorry I totally forgot about this! I'll take another look

@phil-opp
Copy link
Copy Markdown
Member

phil-opp commented Aug 5, 2020

No worries, thanks!

@ghost
Copy link
Copy Markdown
Author

ghost commented Aug 8, 2020

For some reason cargo xtest doesn't run on my machine (it can't find rust-src despite me having rust-src installed on nightly)

@phil-opp
Copy link
Copy Markdown
Member

For some reason cargo xtest doesn't run on my machine (it can't find rust-src despite me having rust-src installed on nightly)

There was a breaking change in the Rust repository lately (change of the directory layout), so you might need to update your nightly version and/or cargo-xbuild. See the note from the cargo-xbuild readme:

Note: The latest version of cargo-xbuild supports all nightlies after 2020-07-30. If you are on an older nightly, you need to install version 0.5.35: cargo install cargo-xbuild --version 0.5.35.

If you're on a newer nightly, you need cargo-xbuild version 0.6.0.

Comment thread src/instructions/segmentation.rs Outdated
Comment on lines +21 to +28
llvm_asm!("pushq $0; \
leaq 1f(%rip), %rax; \
pushq %rax; \
lretq; \
1:" :: "ri" (u64::from(sel.0)) : "rax" "memory");
// Returns
unsafe fn ret_sym() {
asm!("ret");
}

// Pushes the new CS and the address of the "ret" function
asm!("push {cs}; lea {out_reg}, [{ret_sym}]; push {out_reg}; retf",
cs = in(reg) u64::from(sel.0), ret_sym = sym ret_sym, out_reg = out(reg) _);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this might be why we were seeing test failures. We can just directly use the existing implementation with the new asm! syntax:

asm!(
    "push {cs}",
    "lea {tmp}, [1f + rip]",
    "push {tmp}",
    "retfq",
    "1:",
    cs = in(reg) u64::from(sel),
    tmp = lateout(reg) _,
    options(nomem, preserves_flags),
);

I compiled this locally, and got the same assembly output as the existing implementation.

@josephlr
Copy link
Copy Markdown
Contributor

@phil-opp @rybot666 I rebased this, implemented the missing commands, and fixed the set_cs issue.

PTAL

Copy link
Copy Markdown
Member

@phil-opp phil-opp left a comment

Choose a reason for hiding this comment

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

The updates look reasonable to me, but I didn't look at the set_cs assembly in detail. The fact that the tests pass again is a good sign though.

@josephlr
Copy link
Copy Markdown
Contributor

The updates look reasonable to me, but I didn't look at the set_cs assembly in detail. The fact that the tests pass again is a good sign though.

I disassembled the double_fault_stack_overflow test, and the code that sets cs is identical before and after this change:

movzx  ecx,WORD PTR [rax+0x48]
push   rcx
lea    rax,[rip+0x3]
push   rax
rex.W  retf 
mov    rdi,rbx

So this PR (at least) didn't mess things up.

@phil-opp
Copy link
Copy Markdown
Member

Great, thanks a lot for checking!

@josephlr josephlr merged commit 0afc070 into rust-osdev:master Sep 30, 2020
phil-opp added a commit that referenced this pull request Oct 31, 2020
phil-opp added a commit that referenced this pull request Nov 11, 2020
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.

New assembly syntax

2 participants