address review

Author: arihant2math

etc/syscalls_linux_aarch64.md

@@ -158,7 +158,7 @@
 | 0x9b (155)  | getpgid                 | (pid_t pid)                                                                                                                                | __arm64_sys_getpgid                 | true        |
 | 0x9c (156)  | getsid                  | (pid_t pid)                                                                                                                                | __arm64_sys_getsid                  | true        |
 | 0x9d (157)  | setsid                  | ()                                                                                                                                         | __arm64_sys_setsid                  | true        |
-| 0x9e (158)  | getgroups               | (int gidsetsize, gid_t *grouplist)                                                                                                         | __arm64_sys_getgroups               | false       |
+| 0x9e (158)  | getgroups               | (int gidsetsize, gid_t *grouplist)                                                                                                         | __arm64_sys_getgroups               | stub        |
 | 0x9f (159)  | setgroups               | (int gidsetsize, gid_t *grouplist)                                                                                                         | __arm64_sys_setgroups               | false       |
 | 0xa0 (160)  | newuname                | (struct new_utsname *name)                                                                                                                 | __arm64_sys_newuname                | true        |
 | 0xa1 (161)  | sethostname             | (char *name, int len)                                                                                                                      | __arm64_sys_sethostname             | true        |

libkernel/src/arch/arm64/memory/pg_walk.rs

@@ -2,18 +2,20 @@
 
 use super::{
     pg_descriptors::L3Descriptor,
-    pg_tables::{L0Table, L3Table},
+    pg_tables::{L0Table, L1Table, L3Table},
 };
 use crate::{
     error::{MapError, Result},
     memory::{
         PAGE_SIZE,
-        address::{TPA, VA},
+        address::{PA, TPA, VA},
         paging::{
-            NullTlbInvalidator, PageTableEntry, PageTableMapper, PgTable, PgTableArray,
-            walk::{RecursiveWalker, WalkContext},
+            NullTlbInvalidator, PaMapper, PageTableEntry, PageTableMapper, PgTable, PgTableArray,
+            TableMapper,
+            permissions::PtePermissions,
+            walk::{RecursiveWalker, Translator, WalkContext},
         },
-        region::VirtMemoryRegion,
+        region::{PhysMemoryRegion, VirtMemoryRegion},
     },
 };
 
@@ -111,6 +113,71 @@ pub fn get_pte<PM: PageTableMapper>(
     Ok(descriptor)
 }
 
+impl Translator for L0Table {
+    fn translate<PM: PageTableMapper>(
+        table_pa: TPA<PgTableArray<Self>>,
+        va: VA,
+        ctx: &mut WalkContext<PM>,
+    ) -> Result<Option<(PA, usize, PtePermissions)>> {
+        let desc = unsafe {
+            ctx.mapper
+                .with_page_table(table_pa, |pgtable| Self::from_ptr(pgtable).get_desc(va))?
+        };
+
+        match desc.next_table_address() {
+            Some(next_pa) => L1Table::translate(next_pa, va, ctx),
+            None if desc.is_valid() => Err(MapError::InvalidDescriptor.into()),
+            None => Ok(None),
+        }
+    }
+}
+
+impl Translator for L3Table {
+    fn translate<PM: PageTableMapper>(
+        table_pa: TPA<PgTableArray<Self>>,
+        va: VA,
+        ctx: &mut WalkContext<PM>,
+    ) -> Result<Option<(PA, usize, PtePermissions)>> {
+        let desc = unsafe {
+            ctx.mapper
+                .with_page_table(table_pa, |pgtable| Self::from_ptr(pgtable).get_desc(va))?
+        };
+
+        match desc.mapped_address() {
+            Some(pa) => Ok(Some((
+                pa,
+                1 << Self::Descriptor::MAP_SHIFT,
+                desc.permissions().unwrap(),
+            ))),
+            None if desc.is_valid() => Err(MapError::InvalidDescriptor.into()),
+            None => Ok(None),
+        }
+    }
+}
+
+/// Translates the VA into a physical region plus an offset and permissions.
+pub fn translate<PM: PageTableMapper>(
+    l0_table: TPA<PgTableArray<L0Table>>,
+    va: VA,
+    mapper: &mut PM,
+) -> Result<Option<(PhysMemoryRegion, usize, PtePermissions)>> {
+    let mut walk_ctx = WalkContext {
+        mapper,
+        // Safe to not invalidate the TLB, as we are not modifying any PTEs.
+        invalidator: &NullTlbInvalidator {},
+    };
+
+    if let Some((pa, blk_sz, perms)) = L0Table::translate(l0_table, va, &mut walk_ctx)? {
+        debug_assert!(blk_sz.is_power_of_two());
+
+        let offset = va.value() & (blk_sz - 1);
+
+        Ok(Some((PhysMemoryRegion::new(pa, blk_sz), offset, perms)))
+    } else {
+        Ok(None)
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;

src/arch/arm64/memory/address_space.rs

@@ -14,7 +14,7 @@ use libkernel::{
         pg_descriptors::{L3Descriptor, MemoryType},
         pg_tables::{L0Table, MapAttributes, MappingContext, map_range},
         pg_tear_down::tear_down_address_space,
-        pg_walk::{get_pte, walk_and_modify_region},
+        pg_walk::{translate as translate_va, walk_and_modify_region},
     },
     error::{KernelError, MapError, Result},
     memory::{
@@ -137,16 +137,15 @@ impl UserAddressSpace for Arm64ProcessAddressSpace {
     }
 
     fn translate(&self, va: VA) -> Option<PageInfo> {
-        let pte = get_pte(
-            self.l0_table,
-            va.page_aligned(),
-            &mut PageOffsetPgTableMapper {},
-        )
-        .unwrap()?;
+        let (region, offset, perms) =
+            translate_va(self.l0_table, va, &mut PageOffsetPgTableMapper {})
+                .ok()
+                .flatten()?;
+        let pa = region.start_address().add_bytes(offset);
 
         Some(PageInfo {
-            pfn: pte.mapped_address()?.to_pfn(),
-            perms: pte.permissions()?,
+            pfn: pa.to_pfn(),
+            perms,
         })
     }
 

src/arch/arm64/memory/mmu.rs

@@ -4,12 +4,12 @@ use libkernel::{
     arch::arm64::memory::{
         pg_descriptors::MemoryType,
         pg_tables::{L0Table, MapAttributes, MappingContext, map_range},
-        pg_walk::get_pte,
+        pg_walk::translate as translate_va,
     },
     error::Result,
     memory::{
         address::{PA, TPA, VA},
-        paging::{PaMapper, PgTableArray, permissions::PtePermissions},
+        paging::{PgTableArray, permissions::PtePermissions},
         proc_vm::address_space::KernAddressSpace,
         region::{PhysMemoryRegion, VirtMemoryRegion},
     },
@@ -39,16 +39,11 @@ impl Arm64KernelAddressSpace {
         map_range(self.kernel_l0, map_attrs, &mut ctx)
     }
 
-    pub fn translate(&self, va: VA) -> Option<PA> {
-        let pg_offset = va.page_offset();
-
-        let pte = get_pte(self.kernel_l0, va, &mut PageOffsetPgTableMapper {})
+    pub fn translate(&self, va: VA) -> Option<(PhysMemoryRegion, usize)> {
+        translate_va(self.kernel_l0, va, &mut PageOffsetPgTableMapper {})
             .ok()
-            .flatten()?;
-
-        let pa = pte.mapped_address()?;
-
-        Some(pa.add_bytes(pg_offset))
+            .flatten()
+            .map(|(region, offset, _)| (region, offset))
     }
 
     pub fn table_pa(&self) -> PA {

src/drivers/virtio_hal.rs

@@ -26,39 +26,36 @@ impl VirtioHal {
         rounded.ilog2() as u8
     }
 
-    fn translated_phys_addr(vaddr: VA) -> Option<PhysAddr> {
-        ArchImpl::kern_address_space()
-            .lock_save_irq()
-            .translate(vaddr)
-            .map(|pa| pa.value() as PhysAddr)
-    }
-
     fn translate_buffer(vaddr: VA, len: usize) -> Option<PhysAddr> {
         debug_assert!(len > 0);
 
-        let first_page_va = vaddr.page_aligned();
-        let last_byte_va = vaddr.add_bytes(len - 1);
-        let last_page_va = last_byte_va.page_aligned();
-
-        let first_page_pa = Self::translated_phys_addr(first_page_va)?;
-        let mut page_va = first_page_va;
-        let mut expected_page_pa = first_page_pa;
-
-        loop {
-            let page_pa = Self::translated_phys_addr(page_va)?;
-            if page_pa != expected_page_pa {
-                return None;
+        let addr_space = ArchImpl::kern_address_space().lock_save_irq();
+        let mut next_va = vaddr;
+        let mut remaining = len;
+        let mut start_pa = None;
+        let mut expected_next_pa = None;
+
+        while remaining > 0 {
+            let (phys_region, offset) = addr_space.translate(next_va)?;
+            let translated_pa = phys_region.start_address().add_bytes(offset).value() as PhysAddr;
+
+            if let Some(expected_pa) = expected_next_pa {
+                if translated_pa != expected_pa {
+                    return None;
+                }
+            } else {
+                start_pa = Some(translated_pa);
             }
 
-            if page_va == last_page_va {
-                break;
-            }
+            let mapped_len = phys_region.size() - offset;
+            let covered_len = mapped_len.min(remaining);
 
-            page_va = page_va.add_pages(1);
-            expected_page_pa += PAGE_SIZE as PhysAddr;
+            next_va = next_va.add_bytes(covered_len);
+            remaining -= covered_len;
+            expected_next_pa = Some(translated_pa + covered_len as PhysAddr);
         }
 
-        Some(first_page_pa + vaddr.page_offset() as PhysAddr)
+        start_pa
     }
 
     fn bounce_copy_in(paddr: PhysAddr, src: &[u8]) {
@@ -98,12 +95,6 @@ impl VirtioHal {
             .lock_save_irq()
             .push(BouncedShare { paddr, pages });
 
-        // trace!(
-        //     "virtio share: bounced {:p} len={} direction={direction:?} to paddr={paddr:#x}",
-        //     buffer.as_ptr(),
-        //     buffer.len(),
-        // );
-
         paddr
     }
 }
@@ -130,13 +121,10 @@ unsafe impl Hal for VirtioHal {
             core::ptr::write_bytes(vaddr.as_ptr(), 0, pages * PAGE_SIZE);
         }
 
-        // trace!("alloc DMA: paddr={paddr:#x}, pages={pages}, order={order}");
         (paddr, vaddr)
     }
 
     unsafe fn dma_dealloc(paddr: PhysAddr, _vaddr: NonNull<u8>, pages: usize) -> i32 {
-        // trace!("dealloc DMA: paddr={paddr:#x}, pages={pages}");
-
         let order = Self::pages_to_order(pages);
         let region = PhysMemoryRegion::new(
             PA::from_value(paddr as usize),

src/fs/syscalls/pivot_root.rs

@@ -1,6 +1,4 @@
-use crate::{
-    fs::VFS, memory::uaccess::cstr::UserCStr, process::TASK_LIST, sched::syscall_ctx::ProcessCtx,
-};
+use crate::{fs::VFS, memory::uaccess::cstr::UserCStr, sched::syscall_ctx::ProcessCtx};
 use alloc::sync::Arc;
 use core::ffi::c_char;
 use libkernel::{
@@ -110,29 +108,7 @@ pub async fn sys_pivot_root(
         return Err(KernelError::InUse);
     }
 
-    let (old_root_inode, new_root_inode) =
-        VFS.pivot_root(new_root_attachment, put_old_attachment)?;
-
-    let tasks: alloc::vec::Vec<_> = TASK_LIST
-        .lock_save_irq()
-        .values()
-        .filter_map(|work| work.upgrade())
-        .collect();
-
-    for work in tasks {
-        let task = work.task.t_shared.clone();
-
-        let mut root = task.root.lock_save_irq();
-        if root.0.id() == old_root_inode.id() {
-            *root = (new_root_inode.clone(), "/".into());
-        }
-        drop(root);
-
-        let mut cwd = task.cwd.lock_save_irq();
-        if cwd.0.id() == old_root_inode.id() {
-            *cwd = (new_root_inode.clone(), "/".into());
-        }
-    }
+    let _ = VFS.pivot_root(new_root_attachment, put_old_attachment)?;
 
     Ok(0)
 }