Skip to content

Use new assembly syntax #165

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

Merged
merged 8 commits into from
Sep 30, 2020
Merged

Conversation

katietheqt-old
Copy link
Member

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

@katietheqt-old
Copy link
Member Author

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

@katietheqt-old katietheqt-old marked this pull request as ready for review June 27, 2020 21:50
Copy link
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.

@phil-opp
Copy link
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!

@katietheqt-old
Copy link
Member Author

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

@katietheqt-old katietheqt-old requested a review from phil-opp June 30, 2020 12:29
@phil-opp
Copy link
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.

@katietheqt-old
Copy link
Member Author

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
Member

phil-opp commented Aug 4, 2020

@rybot666 Any updates on this?

@katietheqt-old
Copy link
Member Author

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

@phil-opp
Copy link
Member

phil-opp commented Aug 5, 2020

No worries, thanks!

@katietheqt-old
Copy link
Member Author

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
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 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
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
Contributor

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

PTAL

Copy link
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
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
Member

Great, thanks a lot for checking!

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
3 participants