diff --git a/issues/a-freshly-written-8-mib-file-reads-back-slower-than-a-16-mib-one-on-the-amd-bench.md b/issues/a-freshly-written-8-mib-file-reads-back-slower-than-a-16-mib-one-on-the-amd-bench.md new file mode 100644 index 00000000000..ced119204a6 --- /dev/null +++ b/issues/a-freshly-written-8-mib-file-reads-back-slower-than-a-16-mib-one-on-the-amd-bench.md @@ -0,0 +1,27 @@ +--- +status: open +kind: finding +opened: 2026-11-11 +--- + +# A freshly written 8 MiB file reads back slower than a 16 MiB one on the AMD bench + +`persist.seq_read_cached`'s pattern writes 8 MiB in 1 MiB calls, syncs, and +reads the file back whole in 1 MiB calls. On the AMD bench, ToyOS at +`422915e2c` with stage timers in a scratch kernel, its paged reads copied +8.4 and 8.7 µs per chunk of 16 pages, against 6.1 and 6.2 µs for 1 MiB reads +of a 16 MiB file written without a sync and read once through first. Both +files were on the memory file system, both buffers at a page's start, so +neither copy is the store-forwarding stall +(`issues/a-userland-forward-copy-a-short-way-above-its-source-modulo-4-kib-stalls-on-amd.md`). +The row's samples ran from 6.3 to 10.8 GB/s within a boot, p50 7.8 and 7.3 +GB/s, against Linux's 9.0 and 9.3. On the T14 the order is the other way: +4.3 and 4.4 µs per chunk for the 8 MiB file, 5.5 for the 16 MiB one +([evidence](https://github.com/ToyOSOrg/evidence/tree/cb8da5fc2168bfe0395b6bce364212b4117a170d/diagnosis)). + +Cause not found: the two differ in the sync, the file's size against the +caches and the frames each file was written into. + +Exit: the cause measured, for example the same 8 MiB file read with and +without the sync and at the 16 MiB file's size, and fixed or recorded where +it lives. diff --git a/issues/a-pipe-gives-back-the-frames-its-reader-empties-and-takes-them-again.md b/issues/a-pipe-gives-back-the-frames-its-reader-empties-and-takes-them-again.md new file mode 100644 index 00000000000..f859e7cde90 --- /dev/null +++ b/issues/a-pipe-gives-back-the-frames-its-reader-empties-and-takes-them-again.md @@ -0,0 +1,27 @@ +--- +status: open +kind: finding +opened: 2026-11-11 +--- + +# A pipe gives back the frames its reader empties and takes them again + +A pipe's bytes live in frames it takes as writes need them and gives back as +reads empty them (`kernel/src/pipe.rs`, `kernel/src/frame_bytes.rs`). Under a +steady stream the same frames leave the pipe and come back: a small write that +finds no room in a frame the pipe holds cannot be put in one hold of the +pipe's lock under its process's (`PipeRef::put_now`), and takes the general +path, which lets the lock go to take frames. + +On the T14, ToyOS at `422915e2c` with stage timers in a scratch kernel, +`pipe.throughput.4096` with osbench's own buffers, two passes of 100 ms +([evidence](https://github.com/ToyOSOrg/evidence/tree/cb8da5fc2168bfe0395b6bce364212b4117a170d/diagnosis)): +- 35,552 of 102,720 and 38,050 of 105,216 writes (35% and 36%) took the + general path, at 609 and 616 ns each, against 447 and 475 ns for a write put + in one hold; +- a read on the general path spent 412 and 418 ns after its copy, consuming + the frames it emptied, giving them back and waking the writer. + +Exit: the cost measured against a pipe that keeps the frames its reader +empties for its next writes, on both benches, and that change made or the +cost recorded at the pipe's module. diff --git a/issues/kernel-forward-copies-and-fills-are-one-rep-movsb-or-stosb-on-every-cpu.md b/issues/kernel-forward-copies-and-fills-are-one-rep-movsb-or-stosb-on-every-cpu.md index 41c173de174..d90a5d9c6a0 100644 --- a/issues/kernel-forward-copies-and-fills-are-one-rep-movsb-or-stosb-on-every-cpu.md +++ b/issues/kernel-forward-copies-and-fills-are-one-rep-movsb-or-stosb-on-every-cpu.md @@ -7,7 +7,8 @@ opened: 2026-09-29 # Kernel forward copies and fills are one `rep movsb` or `rep stosb` on every CPU The kernel's `memcpy` is its own (`kernel/src/arch/x86_64/copy.rs`): one -`rep movsb` on every CPU, but for a copy it sends highest first. It replaced +`rep movsb` on every CPU, but for a copy it sends highest first, and its +user copies go by the same rule (`arch::x86_64::uaccess`). It replaced `compiler_builtins`' `rep movsb; rep movsq; rep movsb`, so every forward copy changed with it, on Zen 2 without ERMS too: a kernel tmpfs read of 4 or 8 KiB took 0.92-0.96 times as long on both benches, at every distance outside the @@ -40,3 +41,19 @@ with no `rep movsq` or `rep stosq`; the T14's pipe figures; and Zen 2's cost recorded, as a family-0x17 figure against the build without `ermsb` or as a `tooling` issue that owes it. **Mutation**: the build without `ermsb` reds the gate. **Oracle**: Linux's pipe figures. + +- **Owed on metal:** #1244, which sends user copies down by this rule, is not + measured on metal; its speed-up is a prediction. Owed: one A/B of main from + before and after it on both benches, four boots a side, each running + `osbench pipeab` (the measurement-only commit on `pipeperf-ab-base`): the + pipe rows by reader placement (`ab.pipe.natural.*`, `ab.pipe..r.w`) + and the positioned cached reads at 0, 16 and 64 bytes into a page + (`ab.fs.memory.pread.*`). A row changes where all four paired boots move one + way and their median ratio is beyond 1 ± 3%. Main after must show, on the + AMD bench, `ab.pipe.natural.4096`, `.16384` and `.65536` (the reader 16 + bytes into a page) up from about 3.0 GB/s to about 7, 15 and 20 GB/s, the + rows with the reader at 0; each read 16 bytes into a page at most 1.25 times + the same read at 0, against 1.9 to 4 times before; and on the T14 none of + those rows changed. And main's `testcases` and `shared-debug` boots pass on + both benches, `user_copy_faults` among them, which on the AMD bench copies + down by its own rule. diff --git a/kernel/src/actuator.rs b/kernel/src/actuator.rs index 8eb81b2cadd..18cfcc961ec 100644 --- a/kernel/src/actuator.rs +++ b/kernel/src/actuator.rs @@ -43,6 +43,11 @@ actuators! { /// x86-64 loads its IDT later, and panics by name instead. test_early_fault = "test-early-fault"; + /// Send every copy of at least 64 bytes whose ends differ modulo 4 KiB + /// highest first, whatever the CPU (`arch::x86_64::copy`). Judged by + /// `a_user_copy_highest_first_faults_or_ends_as_bad_address`. + copies_go_down = "copies-go-down"; + /// Have the first syscall null SS, force a switch, and report whether it reloaded — the /// AMD `SYSRET` SS-attributes workaround's only guest-observable proof. sysret_ss_probe = "sysret-ss-probe"; diff --git a/kernel/src/arch/aarch64/uaccess.rs b/kernel/src/arch/aarch64/uaccess.rs index c5b9e2a6bcf..38580fd699a 100644 --- a/kernel/src/arch/aarch64/uaccess.rs +++ b/kernel/src/arch/aarch64/uaccess.rs @@ -23,6 +23,8 @@ use toyos_userbound::USER_TOP; +use crate::user_ptr::Faulted; + core::arch::global_asm!( ".pushsection .text.toyos_uaccess, \"ax\"", // toyos_copy_from_user(x0 = kernel dst, x1 = user src, x2 = len) -> x0 = bytes not copied @@ -162,34 +164,38 @@ unsafe extern "C" { static toyos_clear_user_end: u8; } -/// Copy `len` bytes from user address `user` to `dst`; answers the bytes not -/// copied. +/// A routine's answer: 0 where it copied every byte. +fn whole(left: usize) -> Result<(), Faulted> { + if left == 0 { Ok(()) } else { Err(Faulted) } +} + +/// Copy `len` bytes from user address `user` to `dst`. On `Err` some or none +/// of them were copied. /// /// # Safety /// `dst` is writable kernel memory for `len` bytes; `[user, user + len)` lies /// below [`USER_TOP`]; no lock is held (`crate::user_ptr`). -pub unsafe fn copy_from_user(dst: *mut u8, user: u64, len: usize) -> usize { +pub unsafe fn copy_from_user(dst: *mut u8, user: u64, len: usize) -> Result<(), Faulted> { // SAFETY: the caller's contract; the routine touches nothing else. - unsafe { toyos_copy_from_user(dst, user, len) } + whole(unsafe { toyos_copy_from_user(dst, user, len) }) } -/// Copy `len` bytes from `src` to user address `user`; answers the bytes not -/// copied. +/// Copy `len` bytes from `src` to user address `user`, as [`copy_from_user`]. /// /// # Safety /// As [`copy_from_user`], with `src` readable. -pub unsafe fn copy_to_user(user: u64, src: *const u8, len: usize) -> usize { +pub unsafe fn copy_to_user(user: u64, src: *const u8, len: usize) -> Result<(), Faulted> { // SAFETY: the caller's contract; the routine touches nothing else. - unsafe { toyos_copy_to_user(user, src, len) } + whole(unsafe { toyos_copy_to_user(user, src, len) }) } -/// Zero `len` bytes at user address `user`; answers the bytes not zeroed. +/// Zero `len` bytes at user address `user`, as [`copy_from_user`]. /// /// # Safety /// As [`copy_from_user`]. -pub unsafe fn clear_user(user: u64, len: usize) -> usize { +pub unsafe fn clear_user(user: u64, len: usize) -> Result<(), Faulted> { // SAFETY: the caller's contract; the routine touches nothing else. - unsafe { toyos_clear_user(user, len) } + whole(unsafe { toyos_clear_user(user, len) }) } /// Where an EL1 abort at `pc` resumes when it cannot be served: `Some` only diff --git a/kernel/src/arch/x86_64/boot.rs b/kernel/src/arch/x86_64/boot.rs index e359803391f..bb2918aea37 100644 --- a/kernel/src/arch/x86_64/boot.rs +++ b/kernel/src/arch/x86_64/boot.rs @@ -67,6 +67,7 @@ pub fn after_console(args: &KernelArgs, _maps: &[MemoryMapEntry]) { microcode::load(0); let rule = match cpu::vendor() { + _ if crate::actuator::copies_go_down() => copy::Rule { window: 4096 }, kernel::cpu::Vendor::Amd => copy::Rule::AMD, kernel::cpu::Vendor::Intel | kernel::cpu::Vendor::Other(_) => copy::Rule::NEVER, }; diff --git a/kernel/src/arch/x86_64/copy.rs b/kernel/src/arch/x86_64/copy.rs index 6186266068a..4b2f0a081b5 100644 --- a/kernel/src/arch/x86_64/copy.rs +++ b/kernel/src/arch/x86_64/copy.rs @@ -1,4 +1,5 @@ -//! The kernel's `memcpy`, in place of `compiler_builtins`' weak one: one +//! The kernel's `memcpy`, in place of `compiler_builtins`' weak one, and the +//! rule and the descending copy its user copies share (`super::uaccess`): one //! `rep movsb`, or a descending copy where a forward one would stall. //! //! A load matching an older in-flight store on address bits 11:0 alone waits @@ -54,7 +55,7 @@ pub fn set_rule(rule: Rule) { } #[cfg(target_os = "none")] -fn rule() -> Rule { +pub(super) fn rule() -> Rule { Rule { window: WINDOW.load(Ordering::Relaxed) } } @@ -101,55 +102,71 @@ pub unsafe fn movsb(dest: *mut u8, src: *const u8, n: usize) { } } -/// Copy `n` bytes, at least 8, between disjoint ranges, highest first: the -/// top 8 bytes in one store, then down 32 and 8 bytes a step from the +/// The descending copy's instructions, for `rdi` the destination, `rsi` the +/// source and `rdx` the length, at least 8: it keeps those three, uses no +/// stack and leaves `rax`, `rcx`, `r8`, `r9` and `r10` changed. The top 8 +/// bytes go in one store, then 32 and 8 bytes a step down from the /// destination's last 8-byte boundary, so no other store splits a line, and /// the bottom 8 bytes last. Stores past the boundary overlap one another, /// with the same bytes. +macro_rules! copy_down_asm { + () => { + concat!( + "lea rcx, [rdi + rdx]\n", + "and rcx, -8\n", + "sub rcx, rdi\n", + "mov rax, [rsi + rdx - 8]\n", + "mov [rdi + rdx - 8], rax\n", + "cmp rcx, 32\n", + "jb 3f\n", + "2:\n", + "sub rcx, 32\n", + "mov rax, [rsi + rcx + 24]\n", + "mov r8, [rsi + rcx + 16]\n", + "mov r9, [rsi + rcx + 8]\n", + "mov r10, [rsi + rcx]\n", + "mov [rdi + rcx + 24], rax\n", + "mov [rdi + rcx + 16], r8\n", + "mov [rdi + rcx + 8], r9\n", + "mov [rdi + rcx], r10\n", + "cmp rcx, 32\n", + "jae 2b\n", + "3:\n", + "cmp rcx, 8\n", + "jb 5f\n", + "4:\n", + "sub rcx, 8\n", + "mov rax, [rsi + rcx]\n", + "mov [rdi + rcx], rax\n", + "cmp rcx, 8\n", + "jae 4b\n", + "5:\n", + "mov rax, [rsi]\n", + "mov [rdi], rax\n", + ) + }; +} +#[cfg(target_os = "none")] +pub(super) use copy_down_asm; + +/// Copy `n` bytes, at least 8, between disjoint ranges, highest first +/// ([`copy_down_asm`]). /// /// # Safety /// As [`copy_by`], with `n` at least 8. pub unsafe fn copy_down(dest: *mut u8, src: *const u8, n: usize) { - let aligned_end = ((dest as usize).wrapping_add(n) & !7).wrapping_sub(dest as usize); // SAFETY: every access is inside `[0, n)` of its range, as the caller promises. unsafe { core::arch::asm!( - "mov {a}, [{s} + {n} - 8]", - "mov [{d} + {n} - 8], {a}", - "cmp {p}, 32", - "jb 3f", - "2:", - "sub {p}, 32", - "mov {a}, [{s} + {p} + 24]", - "mov {b}, [{s} + {p} + 16]", - "mov {c}, [{s} + {p} + 8]", - "mov {e}, [{s} + {p}]", - "mov [{d} + {p} + 24], {a}", - "mov [{d} + {p} + 16], {b}", - "mov [{d} + {p} + 8], {c}", - "mov [{d} + {p}], {e}", - "cmp {p}, 32", - "jae 2b", - "3:", - "cmp {p}, 8", - "jb 5f", - "4:", - "sub {p}, 8", - "mov {a}, [{s} + {p}]", - "mov [{d} + {p}], {a}", - "cmp {p}, 8", - "jae 4b", - "5:", - "mov {a}, [{s}]", - "mov [{d}], {a}", - s = in(reg) src, - d = in(reg) dest, - n = in(reg) n, - p = inout(reg) aligned_end => _, - a = out(reg) _, - b = out(reg) _, - c = out(reg) _, - e = out(reg) _, + copy_down_asm!(), + in("rdi") dest, + in("rsi") src, + in("rdx") n, + out("rax") _, + out("rcx") _, + out("r8") _, + out("r9") _, + out("r10") _, options(nostack), ); } diff --git a/kernel/src/arch/x86_64/idt/exceptions.rs b/kernel/src/arch/x86_64/idt/exceptions.rs index ff31fb7df2c..bd5925cc0e1 100644 --- a/kernel/src/arch/x86_64/idt/exceptions.rs +++ b/kernel/src/arch/x86_64/idt/exceptions.rs @@ -515,7 +515,7 @@ pub(super) fn page_fault_handler(frame: &mut TrapFrame) { /// A user copy's fault on `addr` (`arch::uaccess`): served as the process's /// own fault would be, after which the copy resumes where it stopped, or else -/// resumed at `fixup`, which answers the bytes not copied. The copy was made +/// resumed at `fixup`, which answers that the copy failed. The copy was made /// holding no lock at its syscall's depth (`user_ptr`), so the service runs at /// that depth too. fn user_copy_fault(frame: &mut TrapFrame, addr: u64, fixup: u64) { diff --git a/kernel/src/arch/x86_64/uaccess.rs b/kernel/src/arch/x86_64/uaccess.rs index 7368311373d..5d85738367e 100644 --- a/kernel/src/arch/x86_64/uaccess.rs +++ b/kernel/src/arch/x86_64/uaccess.rs @@ -1,16 +1,17 @@ //! Copies between kernel memory and the running process's user half, through //! that process's own mapping (`crate::user_ptr`'s contract). //! -//! Each routine opens SMAP's door with `stac` around one `rep movsb` or -//! `rep stosb` and closes it with `clac`. "A repeating string operation can be -//! suspended by an exception or interrupt. When this happens, the state of the -//! registers is preserved to allow the string operation to be resumed" (Intel -//! SDM Vol. 2B, REP/REPE/REPZ/REPNE/REPNZ). So a fault the page-fault path -//! serves resumes the copy where it stopped, and one it cannot serve resumes -//! at the instruction after it, the fixup, which closes the door and answers -//! what `RCX` still counts. [`fixup`] is the exception -//! table: only these three instructions may take a Ring 0 fault on a user -//! address and live. +//! Each routine opens SMAP's door with `stac` around its copy and closes it +//! with `clac`. A copy goes as the kernel's `memcpy` does, by its rule +//! (`super::copy`): one `rep movsb`, or the descending copy where a forward +//! one would stall. "A repeating string operation can be suspended by an +//! exception or interrupt. When this happens, the state of the registers is +//! preserved to allow the string operation to be resumed" (Intel SDM Vol. 2B, +//! REP/REPE/REPZ/REPNE/REPNZ), and a faulting `mov` is run again. So a fault +//! the page-fault path serves resumes the copy where it stopped, and one it +//! cannot serve resumes at the copy's fixup, which closes the door and +//! answers that the copy failed. [`fixup`] is the exception table: only these +//! copies' instructions may take a Ring 0 fault on a user address and live. //! //! The user address is checked against the half's top by the caller; each //! routine also replaces an address at or past it with 0 by `cmovae`, without @@ -21,9 +22,12 @@ use toyos_userbound::USER_TOP; +use super::copy::{self, copy_down_asm}; +use crate::user_ptr::Faulted; + core::arch::global_asm!( ".pushsection .text.toyos_uaccess, \"ax\"", - // toyos_copy_from_user(rdi = kernel dst, rsi = user src, rdx = len) -> rax = bytes not copied + // toyos_copy_from_user(rdi = kernel dst, rsi = user src, rdx = len) -> rax = 0 where it copied them all ".balign 16", ".global toyos_copy_from_user", "toyos_copy_from_user:", @@ -39,7 +43,7 @@ core::arch::global_asm!( "clac", "mov rax, rcx", "ret", - // toyos_copy_to_user(rdi = user dst, rsi = kernel src, rdx = len) -> rax = bytes not copied + // toyos_copy_to_user(rdi = user dst, rsi = kernel src, rdx = len) -> rax = 0 where it copied them all ".balign 16", ".global toyos_copy_to_user", "toyos_copy_to_user:", @@ -55,7 +59,46 @@ core::arch::global_asm!( "clac", "mov rax, rcx", "ret", - // toyos_clear_user(rdi = user dst, rsi = len) -> rax = bytes not cleared + // The same two highest first, for a length of at least 8. The copy uses + // no stack, so its fixup returns from wherever it stopped. + ".balign 16", + ".global toyos_copy_from_user_down", + "toyos_copy_from_user_down:", + "xor ecx, ecx", + "mov rax, {top}", + "cmp rsi, rax", + "cmovae rsi, rcx", + "stac", + ".global toyos_copy_from_user_down_access", + "toyos_copy_from_user_down_access:", + copy_down_asm!(), + ".global toyos_copy_from_user_down_end", + "toyos_copy_from_user_down_end:", + "clac", + "xor eax, eax", + "ret", + ".balign 16", + ".global toyos_copy_to_user_down", + "toyos_copy_to_user_down:", + "xor ecx, ecx", + "mov rax, {top}", + "cmp rdi, rax", + "cmovae rdi, rcx", + "stac", + ".global toyos_copy_to_user_down_access", + "toyos_copy_to_user_down_access:", + copy_down_asm!(), + ".global toyos_copy_to_user_down_end", + "toyos_copy_to_user_down_end:", + "clac", + "xor eax, eax", + "ret", + ".global toyos_copy_down_fault", + "toyos_copy_down_fault:", + "clac", + "mov eax, 1", + "ret", + // toyos_clear_user(rdi = user dst, rsi = len) -> rax = 0 where it cleared them all ".balign 16", ".global toyos_clear_user", "toyos_clear_user:", @@ -79,52 +122,85 @@ core::arch::global_asm!( unsafe extern "C" { fn toyos_copy_from_user(dst: *mut u8, user: u64, len: usize) -> usize; fn toyos_copy_to_user(user: u64, src: *const u8, len: usize) -> usize; + fn toyos_copy_from_user_down(dst: *mut u8, user: u64, len: usize) -> usize; + fn toyos_copy_to_user_down(user: u64, src: *const u8, len: usize) -> usize; fn toyos_clear_user(user: u64, len: usize) -> usize; static toyos_copy_from_user_access: u8; static toyos_copy_to_user_access: u8; static toyos_clear_user_access: u8; + static toyos_copy_from_user_down_access: u8; + static toyos_copy_from_user_down_end: u8; + static toyos_copy_to_user_down_access: u8; + static toyos_copy_to_user_down_end: u8; + static toyos_copy_down_fault: u8; } /// Bytes a string instruction takes: its `rep` prefix and its opcode. const REP_STRING_LEN: u64 = 2; -/// Copy `len` bytes from user address `user` to `dst`; answers the bytes not -/// copied. +/// A routine's answer: 0 where it copied every byte. +fn whole(left: usize) -> Result<(), Faulted> { + if left == 0 { Ok(()) } else { Err(Faulted) } +} + +/// Copy `len` bytes from user address `user` to `dst`. On `Err` some or none +/// of them were copied. /// /// # Safety /// `dst` is writable kernel memory for `len` bytes; `[user, user + len)` lies /// below [`USER_TOP`]; no lock is held (`crate::user_ptr`). -pub unsafe fn copy_from_user(dst: *mut u8, user: u64, len: usize) -> usize { - // SAFETY: the caller's contract; the routine touches nothing else. - unsafe { toyos_copy_from_user(dst, user, len) } +pub unsafe fn copy_from_user(dst: *mut u8, user: u64, len: usize) -> Result<(), Faulted> { + // SAFETY: the caller's contract; each routine touches nothing else, and + // the rule sends down only a copy of at least 8 bytes. + whole(unsafe { + match copy::rule().goes_down(dst as usize, user as usize, len) { + true => toyos_copy_from_user_down(dst, user, len), + false => toyos_copy_from_user(dst, user, len), + } + }) } -/// Copy `len` bytes from `src` to user address `user`; answers the bytes not -/// copied. +/// Copy `len` bytes from `src` to user address `user`, as [`copy_from_user`]. /// /// # Safety /// As [`copy_from_user`], with `src` readable. -pub unsafe fn copy_to_user(user: u64, src: *const u8, len: usize) -> usize { - // SAFETY: the caller's contract; the routine touches nothing else. - unsafe { toyos_copy_to_user(user, src, len) } +pub unsafe fn copy_to_user(user: u64, src: *const u8, len: usize) -> Result<(), Faulted> { + // SAFETY: as in `copy_from_user`. + whole(unsafe { + match copy::rule().goes_down(user as usize, src as usize, len) { + true => toyos_copy_to_user_down(user, src, len), + false => toyos_copy_to_user(user, src, len), + } + }) } -/// Zero `len` bytes at user address `user`; answers the bytes not zeroed. +/// Zero `len` bytes at user address `user`, as [`copy_from_user`]. /// /// # Safety /// As [`copy_from_user`]. -pub unsafe fn clear_user(user: u64, len: usize) -> usize { +pub unsafe fn clear_user(user: u64, len: usize) -> Result<(), Faulted> { // SAFETY: the caller's contract; the routine touches nothing else. - unsafe { toyos_clear_user(user, len) } + whole(unsafe { toyos_clear_user(user, len) }) } /// Where a Ring 0 fault at `rip` resumes when it cannot be served: `Some` only -/// for the string instruction of one of the routines above. +/// for the string instruction of one of the routines above, or an instruction +/// of a descending copy. pub fn fixup(rip: u64) -> Option { - let accesses = [ + let strings = [ &raw const toyos_copy_from_user_access as u64, &raw const toyos_copy_to_user_access as u64, &raw const toyos_clear_user_access as u64, ]; - accesses.contains(&rip).then_some(rip + REP_STRING_LEN) + let descending = [ + (&raw const toyos_copy_from_user_down_access as u64)..(&raw const toyos_copy_from_user_down_end as u64), + (&raw const toyos_copy_to_user_down_access as u64)..(&raw const toyos_copy_to_user_down_end as u64), + ]; + if strings.contains(&rip) { + Some(rip + REP_STRING_LEN) + } else if descending.iter().any(|copy| copy.contains(&rip)) { + Some(&raw const toyos_copy_down_fault as u64) + } else { + None + } } diff --git a/kernel/src/user_ptr.rs b/kernel/src/user_ptr.rs index e129405bf9c..71b7ed49279 100644 --- a/kernel/src/user_ptr.rs +++ b/kernel/src/user_ptr.rs @@ -79,11 +79,6 @@ impl From for SyscallError { } } -/// `Ok` where a copy left no byte behind. -fn whole(left: usize) -> Result<(), Faulted> { - if left == 0 { Ok(()) } else { Err(Faulted) } -} - /// The backtrace names the copy's caller. #[cold] #[inline(never)] @@ -192,7 +187,7 @@ impl View { let user = self.at("UserBytes::read", off, len); // SAFETY: the caller's `dst`; `at` keeps the user range in the half, // and `gate` holds no lock across the copy. - whole(unsafe { uaccess::copy_from_user(dst, user, len) }) + unsafe { uaccess::copy_from_user(dst, user, len) } } /// # Safety @@ -200,13 +195,13 @@ impl View { unsafe fn write_raw(&self, off: usize, src: *const u8, len: usize) -> Result<(), Faulted> { let user = self.at("UserBytesMut::write", off, len); // SAFETY: as `read_raw`. - whole(unsafe { uaccess::copy_to_user(user, src, len) }) + unsafe { uaccess::copy_to_user(user, src, len) } } fn fill_zero(&self, off: usize, len: usize) -> Result<(), Faulted> { let user = self.at("UserBytesMut::fill_zero", off, len); // SAFETY: as `read_raw`, with no kernel memory touched. - whole(unsafe { uaccess::clear_user(user, len) }) + unsafe { uaccess::clear_user(user, len) } } } diff --git a/tests/guest/rust-tests/src/bin/user_copy_faults.rs b/tests/guest/rust-tests/src/bin/user_copy_faults.rs index 538747f65f0..6d15bfcbada 100644 --- a/tests/guest/rust-tests/src/bin/user_copy_faults.rs +++ b/tests/guest/rust-tests/src/bin/user_copy_faults.rs @@ -26,14 +26,16 @@ fn fresh(pages: usize) -> *mut u8 { /// A pipe write whose buffer runs into an untouched page: the kernel's read of /// it faults there, is served, and resumes. Then a read into a buffer that runs /// into an untouched page, which faults the same way on the kernel's write. -fn a_fault_served_mid_copy_resumes(len: usize, seed: u8) { +/// The write's buffer starts `from` bytes into a page with `SIDE` bytes of the +/// program's own, and the read's `into` bytes into one. +fn a_fault_served_mid_copy_resumes(len: usize, seed: u8, from: usize, into: usize) -> usize { let (reader, writer) = syscall::pipe().map(|ends| (ends.read, ends.write)).expect("pipe"); - let src = fresh(len.div_ceil(PAGE) + 1); + let src = fresh(len.div_ceil(PAGE) + 2); let expected = pattern(len, seed); - // SAFETY: inside the fresh mapping; only the first page's last `SIDE` - // bytes and none after are touched here. - let start = unsafe { src.add(PAGE - SIDE) }; - // SAFETY: `start..start + SIDE` lies in the first page, which this writes and so populates. + // SAFETY: inside the fresh mapping; only `SIDE` bytes, in its first two + // pages, and none after are touched here. + let start = unsafe { src.add(from) }; + // SAFETY: `start..start + SIDE` lies in the first two pages, which this writes and so populates. unsafe { core::ptr::copy_nonoverlapping(expected.as_ptr(), start, SIDE) }; // SAFETY: the rest of the buffer, in pages never touched: written here would // populate them, so the program's bytes past `SIDE` are written by the kernel's @@ -45,24 +47,24 @@ fn a_fault_served_mid_copy_resumes(len: usize, seed: u8) { while wrote < len { wrote += syscall::write(writer, &buf[wrote..]).unwrap_or_else(|e| panic!("write of {len} from a buffer half untouched: {e:?}")); } - let dst = fresh(len.div_ceil(PAGE) + 1); + let dst = fresh(len.div_ceil(PAGE) + 2); // SAFETY: inside the second mapping; its first page is touched once so - // the read's fault comes past its first `SIDE` bytes. + // the read's fault comes past the bytes it holds. let into = unsafe { - dst.add(PAGE - SIDE).write_volatile(0); - core::slice::from_raw_parts_mut(dst.add(PAGE - SIDE), len) + dst.add(into).write_volatile(0); + core::slice::from_raw_parts_mut(dst.add(into), len) }; let mut read = 0; while read < len { read += syscall::read(reader, &mut into[read..]).unwrap_or_else(|e| panic!("read of {len} into a buffer half untouched: {e:?}")); } assert!(into == want.as_slice(), "{len} bytes through a pipe, both buffers faulting mid-copy, came back changed"); - println!("a fault served mid-copy resumes: {len} bytes"); // SAFETY: the pipe's ends are this function's alone. unsafe { syscall::close(reader); syscall::close(writer); } + len } /// `SYS_NAMESPACE_BUILD` reads its names blob whole before anything else, so a @@ -110,11 +112,17 @@ fn a_byte_nothing_serves_ends_the_copy() { } fn main() { - a_fault_served_mid_copy_resumes(2 * SIDE, 7); + const AT: usize = PAGE - SIDE; + println!("a fault served mid-copy resumes: {} bytes", a_fault_served_mid_copy_resumes(2 * SIDE, 7, AT, AT)); // Past a bounce: the pipe's claimed path copies a chunk at a time. - a_fault_served_mid_copy_resumes(3 * PAGE + 2 * SIDE, 11); + println!("a fault served mid-copy resumes: {} bytes", a_fault_served_mid_copy_resumes(3 * PAGE + 2 * SIDE, 11, AT, AT)); + // The pipe's frames begin a page, so each of these buffers is 16 bytes + // from them modulo a page, on the side an x86-64 kernel copies highest + // first on a CPU whose forward copy stalls there (`arch::x86_64::copy`). + let near = a_fault_served_mid_copy_resumes(3 * PAGE + 2 * SIDE, 17, PAGE - 16, 16); + println!("a fault served mid-copy resumes 16 bytes from the pipe's frames: {near} bytes"); a_byte_nothing_serves_ends_the_copy(); // The kernel still answers after every refusal. - a_fault_served_mid_copy_resumes(2 * SIDE, 13); + a_fault_served_mid_copy_resumes(2 * SIDE, 13, AT, AT); println!("user copies: all checks passed"); } diff --git a/tests/toyos.rs b/tests/toyos.rs index f930b853955..e525dca15b6 100644 --- a/tests/toyos.rs +++ b/tests/toyos.rs @@ -671,6 +671,12 @@ const MACHINE_TESTS: &[&str] = &[ // reaches. One boot of QEMU's x86-64 machine; the shared boots run it on // each bench. "a_user_copy_faults_in_mid_copy_or_ends_as_bad_address", + // The same job with every copy sent highest first, as an x86-64 kernel + // sends some on a CPU whose forward copy stalls: the descending copy's + // fixup, which the guest runner's CPU never takes by its own rule. One + // boot of QEMU's x86-64 machine on the actuator kernel; the AMD bench's + // shared boot takes it by its rule. + "a_user_copy_highest_first_faults_or_ends_as_bad_address", // The boot's log written through usbd onto the stick it booted from, and // still there once the machine has stopped: the path every metal boot's // `/log` takes, which no other QEMU row reads, because their console is @@ -3294,10 +3300,11 @@ fn a_spawn_from_an_image_frees_it_when_the_child_ends(test_config: &Path) -> Res /// `user_copy_faults` alone: its copies that fault mid-copy and are served, /// and its refusals, each said by name. -fn a_user_copy_faults_in_mid_copy_or_ends_as_bad_address(test_config: &Path) -> Result<(), String> { +fn a_user_copy_faults_in_mid_copy_or_ends_as_bad_address(test_config: &Path, kernel_params: &'static [&'static str]) -> Result<(), String> { const JOB: &str = "user_copy_faults"; let bin = qemu::build_toyos_bin(qemu::SUITE_ARCH, &compile::repo_root().join("tests/guest/rust-tests"), JOB); - let mut qemu = QemuInstance::boot_with_options(test_config, &[], &[(JOB.to_string(), bin)], BootOptions::default()); + let options = BootOptions { kernel_params, ..Default::default() }; + let mut qemu = QemuInstance::boot_with_options(test_config, &[], &[(JOB.to_string(), bin)], options); serial::Serial::boot(&qemu).must_be_clean()?; let result = qemu.run_test("test_rs_user_copy_faults", Duration::from_secs(60)); if let Some(why) = &result.error { @@ -3309,6 +3316,7 @@ fn a_user_copy_faults_in_mid_copy_or_ends_as_bad_address(test_config: &Path) -> for claim in [ "a fault served mid-copy resumes: 600 bytes", "a fault served mid-copy resumes: 12888 bytes", + "a fault served mid-copy resumes 16 bytes from the pipe's frames: 12888 bytes", "a byte nothing serves ends the copy as BadAddress", "user copies: all checks passed", ] { @@ -4949,7 +4957,10 @@ fn run_machine_test(name: &str, test_config: &Path) -> Result<(), String> { "the_uart_carries_the_boot_once_across_the_hand_over" => the_uart_carries_the_boot_once_across_the_hand_over(), "a_supervisor_that_fails_early_says_why_on_the_uart" => a_supervisor_that_fails_early_says_why_on_the_uart(), "a_spawn_from_an_image_frees_it_when_the_child_ends" => a_spawn_from_an_image_frees_it_when_the_child_ends(test_config), - "a_user_copy_faults_in_mid_copy_or_ends_as_bad_address" => a_user_copy_faults_in_mid_copy_or_ends_as_bad_address(test_config), + "a_user_copy_faults_in_mid_copy_or_ends_as_bad_address" => a_user_copy_faults_in_mid_copy_or_ends_as_bad_address(test_config, &[]), + "a_user_copy_highest_first_faults_or_ends_as_bad_address" => { + a_user_copy_faults_in_mid_copy_or_ends_as_bad_address(test_config, &["copies-go-down"]) + } "a_bent_supervisor_is_refused_and_nothing_boots" => [qemu::Profile::HeadlessNoConsolePort, qemu::Profile::Virt] .into_iter() .try_for_each(|profile| a_bent_supervisor_is_refused_and_nothing_boots(profile, test_config)),