Skip to content

Nx bit support + minor fixes - #1484

Closed
DixelU wants to merge 29 commits into
copy:masterfrom
DixelU:nx-support-v2
Closed

DixelU wants to merge 29 commits into
copy:masterfrom
DixelU:nx-support-v2

Conversation

@DixelU

@DixelU DixelU commented Jan 11, 2026

Copy link
Copy Markdown
Contributor

That's like the second attempt at cracking this thing 🐈, and there's still some things to work on left.

The good part:

  • Implemented the NX bit itself.
    • And the most minimal test in NASM possible.
  • Faced the issue with RSVD Page faults being unimplemented (which prevented the boot up of linux distributions and windows 7) - implemented though I did not test it though properly yet.
  • Did fix some funky stuff IDE was suggesting in the NASM tests run script along the support for NX test itself ... and god knows what else (I did pull AI for that one, I'll later look into reverting most of its changes though)
  • Tests - all seemed to complete with return code 0, and I was able to load a small build of windows 7 (from CD rom, later bout that) with NX bit enabled:
    image

The problems left (and noticed)

  • Standard windows 7 installation did not boot up being stuck at
    16:06:52+762 [ACPI] ACPI raise irq
    16:06:52+762 [CPU ] No route: level interrupt and remote IRR still set
    
    repeating endlessly. Did waste an entire evening on that one, without a thing out of it. Only a cut down version did boot up properly. Not sure why.
  • Whilst comparing how systems boot on that branch with currently released version of v86 - I did notice that in release the system hangs seemingly randomly inside net2k implementation (might need a separate issue for that) - cannot reproduce rn, but I'll create a separate issue once I face it again.
  • The mess in test scripts.

@YD711YT

YD711YT commented Jan 16, 2026

Copy link
Copy Markdown

Will windows 8 work?

@DixelU

DixelU commented Jan 16, 2026

Copy link
Copy Markdown
Contributor Author

Will windows 8 work?

I'll check it out this weekend

@DimaThenekov

Copy link
Copy Markdown
Contributor

Wow, this is great! I think Windows 7+ bootloaders require something else besides the NX bit. I couldn't start Windows 8 and 10 😢...

@DimaThenekov

Copy link
Copy Markdown
Contributor
image

In particular, I get a #GP exception

@DixelU

DixelU commented Mar 19, 2026

Copy link
Copy Markdown
Contributor Author

Alr, I'm halfway back from the dead, hope to look into it again soon 🕊️

@DixelU

DixelU commented May 17, 2026

Copy link
Copy Markdown
Contributor Author

An unrelated note, having this popping out from time to time: unimplemented ATA command 0xB0: ABORT [ST=0x50 ER=0x0 SC=0x1 LL=0x1 LM=0x4F LH=0xC2 FE=0xD8]

Maybe i'll look into it later
Btw successfully booted up normal Windows 7 distro
Testing out windows 8.1 rn

@DixelU

DixelU commented May 17, 2026

Copy link
Copy Markdown
Contributor Author

Okay, 8.1 seems to boot fine. I'll check other OS later then :D

@DixelU

DixelU commented Jun 3, 2026

Copy link
Copy Markdown
Contributor Author

Booting linux seems to show some signs of PAE being either broken in this exact pull request or generally ...
Really hope it's something i am doing wrong there 🤔

@DixelU

DixelU commented Jun 3, 2026 •

Copy link
Copy Markdown
Contributor Author

Welp, i count that as a success then 💁‍♂️
image

Aaand, i guess i'll need to look at what parts of the CI this PR fails 🤔

- CR4 fix: covers enabling PAE on a system that already has paging active — load_pdpte fires only if CR0.PG=1
- CR3 fix: covers writing CR3 before paging is enabled — load_pdpte fires only if CR0.PG=1
@DixelU
DixelU marked this pull request as ready for review June 3, 2026 08:19
Comment thread tools/rust-lld-wrapper Outdated
@DixelU

DixelU commented Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

Damn it, from windows i can't properly reset the mode for rust-lld-wrapper

Revert (filter changes with mode reset)

The revert did not go well
@DixelU

DixelU commented Jun 16, 2026

Copy link
Copy Markdown
Contributor Author

Yep, now it should be fine - squashed the last 3 funky commits

@DixelU

DixelU commented Aug 10, 2026

Copy link
Copy Markdown
Contributor Author

@copy Can you run CI job and check whether the PR is generally fine in terms of the code-style 👀

@copy copy left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks! This looks good overall. I have left some comments.

The nasmtest doesn't seem to fit in that testsuite very well. It could work better if added to kvm-unit-tests. tests/jit-paging would also make sense if Linux allows you to configure and catch NX faults.

Could you upload one or more disk images (small if possible) that exercise this feature? E.g. OSes that didn't boot before and now do.

Comment thread src/ide.js Outdated
this.push_irq();
break;
}

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Could you send this as a separate PR?

Comment thread src/rust/cpu/instructions_0f.rs Outdated
},

0x80000008 => {
eax = 32; // physical address width

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Should probably also set virtual address bits

Comment thread src/rust/cpu/instructions_0f.rs Outdated
// cmpss xmm, xmm/m32
let destination = read_xmm_f32(r);
let source: f32 = f32::from_bits(i32::cast_unsigned(source));
let source: f32 = f32::from_bits(source as u32);

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

What's the purpose of this change? I try to avoid as because it tends to do have unintended side-effects if you got the types wrong.

Comment thread tests/nasm/run.js Outdated
{
const array = new Array(8 + 1 + 8 + 16 + 32 + (STACK_TOP - BSS >> 2) + 3).fill(0);
array[8] = 0x1000;
return { img_name, fixture: { array, exception: "PF" } };

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

It's unclear to me why this special case for the nx image is necessary.

Comment thread tests/nasm/nx.asm Outdated
mov eax, 0x80000001
cpuid
test edx, 1 << 20
jz .no_nx

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I don't think this NX check is necessary. It's gonna be enabled forever once this is merged.

Comment thread src/rust/cpu/cpu.rs Outdated
clear_tlb_code(page);
tlb_data[page as usize] = 0;
let error_code = (user as i32) << 2 | (write as i32) << 1 | present as i32;
let error_code = (is_instruction_fetch as i32) << 4

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Comment thread src/rust/cpu/cpu.rs Outdated

pub unsafe fn safe_read_f32(addr: i32) -> OrPageFault<f32> {
Ok(f32::from_bits(i32::cast_unsigned(safe_read32s(addr)?)))
Ok(f32::from_bits(safe_read32s(addr)? as u32))

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

See my other comment regarding casts

Comment thread src/rust/cpu/cpu.rs Outdated
for_writing: bool,
user: bool,
jit: bool,
is_instruction_fetch: bool,

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Make the argument order consistent between this and translate_address.

Comment thread src/rust/cpu/cpu.rs Outdated
pub unsafe fn translate_address_fetch(address: i32) -> OrPageFault<u32> {
translate_address(address, false, *cpl == 3, false, true, true)
}
pub unsafe fn translate_address_read_jit(address: i32) -> OrPageFault<u32> {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This name is confusing, because historically and in the non-jit case, it means read without instruction access. The naming should be translate_address_{read,write,fetch}{,_jit}

Comment thread src/rust/cpu/cpu.rs Outdated
if side_effects {
trigger_pagefault(addr, true, for_writing, user, jit, true, false);
}
return Err(());

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Check if this is correct: From my understanding, if a lower table is not present, we should fault with not-present error code. Also check how the reserved bits error code should be handled.

- KVM test with and w/o JIT
- Separate out the ATA_CMD_READ_LOG_EXT implementation patch
- Consistent argument ordering (historically had a little issue with amount of arguments passed, as it did change dramatically in a different local branch)
- Reverted cast changes proposed by IDE
- Integrate NX into codegen, several renaming changes
- Drop test name changes introduced before tests refactoring
- And tons of other things ; - ;
@DixelU

DixelU commented Sep 24, 2026

Copy link
Copy Markdown
Contributor Author

It's kind of funny how to prove that NX bit indeed is supported by booting previously unsupported OS-es
You basically have to add like 3 other patches on top 🐈

@DixelU

DixelU commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor Author

Make it 4 patches;
And Windows 10 2303 - https://archive.org/details/tiny-10_202301
Can be installed and booted from within v86;
I'll be polishing the code later and probably put final disk image somewhere on the cloud

image

@DixelU

DixelU commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor Author

W10 Image (though i am not sure it's okay to distribute it like that -): https://drive.google.com/file/d/1wQ4vIXJPmZTuiHGXedxGWNVdJTdp5Cda/view?usp=sharing

@copy

copy commented Oct 4, 2026

Copy link
Copy Markdown
Owner

Squashed and merged in #1647

Thanks a lot @DixelU, nice work!

@copy copy closed this Oct 4, 2026
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.

4 participants