Skip to content

Commit

Permalink
Fix bugs:
Browse files Browse the repository at this point in the history
1. Read-only parent address space for COW
2. New kernel stack pointer in Trapframe
  • Loading branch information
田凯夫 committed Mar 14, 2023
1 parent d3d8781 commit cff6cd8
Show file tree
Hide file tree
Showing 10 changed files with 69 additions and 54 deletions.
5 changes: 2 additions & 3 deletions debug.sh
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,7 @@

KERNEL=target/riscv64gc-unknown-none-elf/release/tcore-kernel
IMG=fat32.img

cargo xtask make --oscomp --pack-image fat32.img --log trace --dump --test
tmux new-session -d "qemu-system-riscv64 -machine virt -m 2G -nographic -bios default -kernel $KERNEL -drive file=$IMG,if=none,format=raw,id=x0 -device virtio-blk-device,drive=x0,bus=virtio-mmio-bus.0 -s -S -smp 4" && \
cargo xtask make --oscomp --pack-target test/oscomp/judge/ --pack-image fat32.img --dump --build-test --log trace
tmux new-session -d "qemu-system-riscv64 -machine virt -m 2G -nographic -bios default -kernel $KERNEL -drive file=$IMG,if=none,format=raw,id=x0 -device virtio-blk-device,drive=x0,bus=virtio-mmio-bus.0 -s -S" && \
tmux split-window -h "riscv64-unknown-elf-gdb -ex 'set arch riscv:rv64' -ex 'target remote localhost:1234'" && \
tmux -2 attach-session -d
16 changes: 7 additions & 9 deletions kernel/src/arch/riscv64/trap/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ use crate::{
println,
syscall::syscall,
task::do_exit,
task::{do_yield, curr_task, trapframe_base},
task::{do_yield, curr_task, trapframe_base, cpu},
timer::set_next_trigger,
};

Expand Down Expand Up @@ -81,11 +81,10 @@ pub fn user_trap_handler() -> ! {
match scause.cause() {
Trap::Exception(Exception::UserEnvCall) => {
// pc + 4
let curr = curr_task().unwrap();
let curr = cpu().curr.as_ref().unwrap();
let trapframe = curr.trapframe();
trapframe.next_epc();
// Syscall may change the flow
drop(curr);
trap_info();
match syscall(trapframe.syscall_args().unwrap()) {
Ok(ret) => trapframe.set_a0(ret),
Err(errno) => {
Expand All @@ -95,7 +94,7 @@ pub fn user_trap_handler() -> ! {
};
}
Trap::Exception(Exception::StorePageFault) => {
let curr = curr_task().unwrap();
let curr = cpu().curr.as_ref().unwrap();
let mut curr_mm = curr.mm.lock();
// show_trapframe(&current.trapframe());
trap_info();
Expand All @@ -106,7 +105,6 @@ pub fn user_trap_handler() -> ! {
) {
fatal_info(err);
drop(curr_mm);
drop(curr);
unsafe { do_exit(-1) };
}
}
Expand All @@ -118,10 +116,9 @@ pub fn user_trap_handler() -> ! {
}
}
_ => {
let curr = curr_task().unwrap();
let curr = cpu().curr.as_ref().unwrap();
show_trapframe(curr.trapframe());
trap_info();
drop(curr);
unsafe { do_exit(-1) };
}
}
Expand All @@ -146,7 +143,8 @@ pub fn user_trap_return() -> ! {
fn __userret();
}
let (satp, trapframe_base, userret) = {
let curr = curr_task().unwrap();
let curr = cpu().curr.as_ref().unwrap();
log::info!("Trap Return {:?} {:x?}", &curr, curr.trapframe());
let curr_mm = curr.mm.lock();
(
curr_mm.page_table.satp(),
Expand Down
12 changes: 11 additions & 1 deletion kernel/src/arch/riscv64/trap/trapframe.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,19 @@ impl TrapFrame {
}

/// Copies from the old one when we clone a task and initialize its trap frame.
pub fn copy_from(&mut self, orig: &TrapFrame, flags: CloneFlags, stack: usize, tls: usize) {
pub fn copy_from(
&mut self,
orig: &TrapFrame,
flags: CloneFlags,
stack: usize,
tls: usize,
kstack: usize,
) {
*self = *orig;

// Sets new kernel stack
self.kernel_sp = kstack;

// Child task returns zero
self.set_a0(0);

Expand Down
2 changes: 1 addition & 1 deletion kernel/src/config/kernel.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ pub const GUARD_PAGE: usize = PAGE_SIZE;
pub const TRAMPOLINE_VA: usize = MAX_VA - PAGE_SIZE + 1;

/// CPUs
pub const CPU_NUM: usize = 4;
pub const CPU_NUM: usize = 1;

/// Use cpu0 as main hart
pub const MAIN_HART: usize = 0;
Expand Down
3 changes: 0 additions & 3 deletions kernel/src/mm/flags.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,6 @@ bitflags::bitflags! {

/* Unstandard flags */

/// Cloned with [`CloneFlags::CLONE_VM`].
const CLONED = 1 << 61;

/// Identical memory maps with no frame allocated
const IDENTICAL = 1 << 62;

Expand Down
35 changes: 14 additions & 21 deletions kernel/src/mm/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -94,23 +94,29 @@ impl MM {
///
/// Uses the copy-on-write technique (COW) to prevent all data of the parent process from being copied
/// when fork is executed.
pub fn clone(&self) -> KernelResult<Self> {
pub fn clone(&mut self) -> KernelResult<Self> {
let mut page_table = PageTable::new().map_err(|_| KernelError::FrameAllocFailed)?;
let mut new_vma_list = Vec::new();
for vma in self.vma_list.iter() {
for vma in self.vma_list.iter_mut() {
if let Some(vma) = vma {
let mut new_vma = VMArea {
flags: vma.flags | VMFlags::CLONED,
flags: vma.flags,
start_va: vma.start_va,
end_va: vma.end_va,
frames: vma.frames.clone(),
file: vma.file.clone(),
};

// read-only
let mut flags = PTEFlags::from(vma.flags);
// Read-only for new area.
flags.remove(PTEFlags::WRITABLE);

// map the new vma of child process
new_vma.map_all(&mut page_table, flags, false)?;
new_vma_list.push(Some(new_vma));

// remap the old vma of parent process
vma.map_all(&mut page_table, flags, false)?;
} else {
new_vma_list.push(None);
}
Expand Down Expand Up @@ -354,18 +360,7 @@ impl MM {
/// - `va`: starting virtual address.
pub fn alloc_frame(&mut self, va: VirtAddr) -> KernelResult<Frame> {
self.get_vma(va, |vma, pt, _| {
vma.alloc_frame(Page::from(va), pt, vma.flags.contains(VMFlags::CLONED))
.map(|(frame, _)| frame)
})
}

/// Forces to allocate a frame, overwriting the page table with the new frame if the map already exists in the page table.
///
/// This function is mainly used for Copy-on-Write (COW) mechanism.
pub fn force_alloc_frame(&mut self, va: VirtAddr) -> KernelResult<Frame> {
self.get_vma(va, |vma, pt, _| {
vma.alloc_frame(Page::from(va), pt, true)
.map(|(frame, _)| frame)
vma.alloc_frame(Page::from(va), pt).map(|(frame, _)| frame)
})
}

Expand All @@ -382,10 +377,8 @@ impl MM {
let mut frames = Vec::new();
for page in PageRange::from_virt_addr(start_va, (end_va - start_va).value()) {
frames.push(
self.get_vma(page.start_address(), |vma, pt, _| {
vma.alloc_frame(page, pt, false)
})
.map(|(frame, _)| frame)?,
self.get_vma(page.start_address(), |vma, pt, _| vma.alloc_frame(page, pt))
.map(|(frame, _)| frame)?,
);
}
Ok(frames)
Expand Down Expand Up @@ -752,7 +745,7 @@ pub fn do_handle_page_fault(mm: &mut MM, va: VirtAddr, flags: VMFlags) -> Kernel
return Err(KernelError::FatalPageFault);
}

let (_, alloc) = vma.alloc_frame(Page::from(va), pt, true)?;
let (_, alloc) = vma.alloc_frame(Page::from(va), pt)?;

if !alloc {
return Err(KernelError::FatalPageFault);
Expand Down
20 changes: 7 additions & 13 deletions kernel/src/mm/vma.rs
Original file line number Diff line number Diff line change
Expand Up @@ -158,9 +158,9 @@ impl VMArea {
if self.flags.contains(VMFlags::IDENTICAL) {
let start = Frame::from(Page::from(self.start_va).number());
Ok(FrameRange::new(start, start + self.size_in_pages())
.range()
.map(|frame| Some(frame))
.collect())
.range()
.map(|frame| Some(frame))
.collect())
} else {
let mut v = Vec::new();
for frame in &mut self.frames {
Expand Down Expand Up @@ -223,24 +223,18 @@ impl VMArea {
&mut self,
page: Page,
pt: &mut PageTable,
overwrite: bool,
) -> KernelResult<(Frame, bool)> {
let (pte_pa, mut pte) = pt.create(page).map_err(|_| KernelError::PageTableInvalid)?;
if !pte.flags().is_valid() || overwrite {
if !pte.flags().is_valid()
|| (!pte.flags().contains(PTEFlags::WRITABLE) && self.flags.contains(VMFlags::WRITE)) // COW
{
let index = page.number() - Page::from(self.start_va).number();

// reclaims the old frame
let frame = if pte.flags().is_valid() {
let old = self.get_frame(index, false)?;
self.reclaim_frame(index);
self.reclaim_frame(index); // drop rc to old frame
let new = self.get_frame(index, true)?;

// copy on write
new.as_slice_mut().copy_from_slice(old.as_slice());

// no cow from now on
self.flags.remove(VMFlags::CLONED);

new
} else {
self.get_frame(index, true)?
Expand Down
12 changes: 9 additions & 3 deletions kernel/src/task/clone.rs
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ pub fn do_clone(
let curr = curr_task().unwrap();
trace!("CLONE {:?} {:?}", &curr, flags);

if flags.contains(CloneFlags::CLONE_NEWNS | CloneFlags::CLONE_FS) {
if flags.intersects(CloneFlags::CLONE_NEWNS | CloneFlags::CLONE_FS) {
return Err(Errno::EINVAL);
}

Expand All @@ -111,7 +111,7 @@ pub fn do_clone(
let mm = if flags.contains(CloneFlags::CLONE_VM) {
curr.mm.clone()
} else {
let orig = curr.mm.lock();
let mut orig = curr.mm.lock();
Arc::new(SpinLock::new(orig.clone()?))
};

Expand All @@ -125,7 +125,13 @@ pub fn do_clone(
let mut mm = mm.lock();
let trapframe_pa = init_trapframe(&mut mm, kstack)?;
let trapframe = TrapFrame::from(trapframe_pa);
trapframe.copy_from(TrapFrame::from(curr.trapframe.0), flags, stack, tls);
trapframe.copy_from(
TrapFrame::from(curr.trapframe.0),
flags,
stack,
tls,
kstack_base,
);
trapframe_pa
};
let trapframe = TrapFrameTracker(trapframe_pa); // for unwinding
Expand Down
10 changes: 10 additions & 0 deletions kernel/src/task/exit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,10 +72,18 @@ pub fn handle_zombie(task: Arc<Task>) {
locked_inner.children.clear();
locked_inner.state = TaskState::ZOMBIE;

let orphan = locked_inner.parent.is_none();
drop(locked_inner);

#[cfg(feature = "test")]
if task.tid.0 == task.pid {
finish_test(task.inner().exit_code, &task.name);
}

if orphan {
let mut init = INIT_TASK.locked_inner();
init.children.push_back(task);
}
}

bitflags::bitflags! {
Expand Down Expand Up @@ -207,6 +215,8 @@ pub fn do_wait(
i32
)?;
}

break;
}
}

Expand Down
8 changes: 8 additions & 0 deletions kernel/src/task/sched.rs
Original file line number Diff line number Diff line change
Expand Up @@ -147,13 +147,21 @@ pub fn init() {
INIT_TASK.clone();
}

/// Reclaim resources delegated to [`INIT_TASK`].
pub fn init_reclaim() {
let mut init = INIT_TASK.locked_inner();
init.children.clear();
}

/// IDLE task:
///
/// 1. Each cpu tries to acquire the lock of global task manager.
/// 2. Each cpu runs the task fetched from schedule queue.
/// 3. Handle the final state after a task finishes `do_yield` or `do_exit`.
pub unsafe fn idle() -> ! {
loop {
init_reclaim();

let mut task_manager = TASK_MANAGER.lock();

if let Some(task) = task_manager.fetch() {
Expand Down

0 comments on commit cff6cd8

Please sign in to comment.