MapPerms: Consistency between map and iomap

tool/microkit/src/sdf.rs

@@ -49,7 +49,7 @@ pub(crate) use consts::*;
 pub(crate) use cspace::CapMapType;
 pub(crate) use iommu::IommuDeviceIdentifier;
 pub(crate) use irq::{SysIrq, SysIrqKind};
-pub(crate) use memory_region::{Map, SysMapPerms};
+pub(crate) use memory_region::Map;
 pub(crate) use pd_vm::{CpuCore, SysSetVarKind};
 
 // Public re-exports

tool/microkit/src/sdf/memory_region.rs

@@ -21,34 +21,80 @@ use crate::util::round_up;
 use crate::util::str_to_bool;
 use crate::{Config, PageSize};
 
-#[repr(u8)]
-pub enum SysMapPerms {
-    Read = 1,
-    Write = 2,
-    Execute = 4,
+// We do not include attributes (execute/cached) here since seL4 does not treat them like vm_rights.
+// Note on seL4 you can't mint a write only frame. However you can request a write only mapping, for the IOMMU on x86.
+#[derive(Debug, PartialEq, Eq, Clone, Copy)]
+pub enum FrameRights {
+    Read,
+    Write,
+    ReadWrite,
+    None,
+}
+
+impl FrameRights {
+    fn from_bools(read: bool, write: bool) -> Self {
+        match (read, write) {
+            (true, false) => Self::Read,
+            (false, true) => Self::Write,
+            (true, true) => Self::ReadWrite,
+            (false, false) => Self::None,
+        }
+    }
+    pub fn read(self) -> bool {
+        matches!(self, Self::Read | Self::ReadWrite)
+    }
+    pub fn write(self) -> bool {
+        matches!(self, Self::Write | Self::ReadWrite)
+    }
+}
+
+#[derive(Debug, PartialEq, Eq, Clone, Copy)]
+pub struct SysMapPerms {
+    rights: FrameRights,
+    execute: bool,
+}
+
+pub enum SysMapPermsParseError {
+    InvalidChar,
+    WriteOnly,
 }
 
 impl SysMapPerms {
-    fn from_str(s: &str) -> Result<u8, ()> {
-        let mut perms = 0;
+    fn from_str(s: &str) -> Result<Self, SysMapPermsParseError> {
+        let mut read = false;
+        let mut write = false;
+        let mut execute = false;
         for c in s.chars() {
             match c {
-                'r' => perms |= SysMapPerms::Read as u8,
-                'w' => perms |= SysMapPerms::Write as u8,
-                'x' => perms |= SysMapPerms::Execute as u8,
-                _ => return Err(()),
+                'r' => read = true,
+                'w' => write = true,
+                'x' => execute = true,
+                _ => return Err(SysMapPermsParseError::InvalidChar),
             }
         }
+        let rights = match FrameRights::from_bools(read, write) {
+            FrameRights::Write => return Err(SysMapPermsParseError::WriteOnly),
+            frame_rights => frame_rights,
+        };
 
-        Ok(perms)
+        Ok(Self { rights, execute })
+    }
+    pub fn read(self) -> bool {
+        self.rights.read()
+    }
+    pub fn write(self) -> bool {
+        self.rights.write()
+    }
+    pub fn execute(self) -> bool {
+        self.execute
     }
 }
 
 #[derive(Debug, Clone, PartialEq, Eq)]
 pub struct SysMap {
     pub mr: String,
     pub vaddr: u64,
-    pub perms: u8,
+    pub perms: SysMapPerms,
     pub cached: bool,
     /// Location in the parsed SDF file. Because this struct is
     /// used in a non-XML context, we make the position optional.
@@ -94,15 +140,15 @@ impl Map for SysMap {
     }
 
     fn read(&self) -> bool {
-        self.perms & SysMapPerms::Read as u8 != 0
+        self.perms.read()
     }
 
     fn write(&self) -> bool {
-        self.perms & SysMapPerms::Write as u8 != 0
+        self.perms.write()
     }
 
     fn execute(&self) -> bool {
-        self.perms & SysMapPerms::Execute as u8 != 0
+        self.perms.execute()
     }
 
     fn cached(&self) -> bool {
@@ -136,15 +182,15 @@ impl Map for SysIOMap {
     }
 
     fn read(&self) -> bool {
-        matches!(self.perms, SysIOMapPerms::Read | SysIOMapPerms::ReadWrite)
+        self.perms.read()
     }
 
     fn write(&self) -> bool {
-        matches!(self.perms, SysIOMapPerms::Write | SysIOMapPerms::ReadWrite)
+        self.perms.write()
     }
 
     fn execute(&self) -> bool {
-        false
+        self.perms.execute()
     }
 
     fn cached(&self) -> bool {
@@ -214,31 +260,34 @@ impl SysMemoryRegion {
 }
 
 #[derive(Debug, PartialEq, Eq, Clone, Copy)]
-pub enum SysIOMapPerms {
-    Read,
-    Write,
-    ReadWrite,
-}
+pub struct SysIOMapPerms(FrameRights);
 
 impl SysIOMapPerms {
-    fn from_str(s: &str) -> Result<Self, ()> {
+    fn from_str(s: &str) -> Result<Self, String> {
         let mut read = false;
         let mut write = false;
 
         for c in s.chars() {
             match c {
                 'r' => read = true,
                 'w' => write = true,
-                _ => return Err(()),
+                _ => return Err(format!("Invalid character in string {s}")),
             }
         }
-
-        match (read, write) {
-            (true, true) => Ok(SysIOMapPerms::ReadWrite),
-            (true, false) => Ok(SysIOMapPerms::Read),
-            (false, true) => Ok(SysIOMapPerms::Write),
-            (false, false) => Err(()),
-        }
+        let frame_rights = match FrameRights::from_bools(read, write) {
+            FrameRights::None => return Err("Invalid frame right for IOMap".into()),
+            frame_rights => frame_rights,
+        };
+        Ok(SysIOMapPerms(frame_rights))
+    }
+    pub fn read(self) -> bool {
+        self.0.read()
+    }
+    pub fn write(self) -> bool {
+        self.0.write()
+    }
+    pub fn execute(self) -> bool {
+        false
     }
 }
 
@@ -282,7 +331,15 @@ impl SysMap {
         let perms = if let Some(xml_perms) = node.attribute("perms") {
             match SysMapPerms::from_str(xml_perms) {
                 Ok(parsed_perms) => parsed_perms,
-                Err(()) => {
+                // On all architectures, the kernel does not allow write-only mappings
+                Err(SysMapPermsParseError::WriteOnly) => {
+                    return Err(value_error(
+                        xml_sdf,
+                        node,
+                        "perms must not be 'w', write-only mappings are not allowed".to_string(),
+                    ));
+                }
+                Err(_) => {
                     return Err(value_error(
                         xml_sdf,
                         node,
@@ -292,18 +349,12 @@ impl SysMap {
             }
         } else {
             // Default to read-write
-            SysMapPerms::Read as u8 | SysMapPerms::Write as u8
+            SysMapPerms {
+                rights: FrameRights::ReadWrite,
+                execute: false,
+            }
         };
 
-        // On all architectures, the kernel does not allow write-only mappings
-        if perms == SysMapPerms::Write as u8 {
-            return Err(value_error(
-                xml_sdf,
-                node,
-                "perms must not be 'w', write-only mappings are not allowed".to_string(),
-            ));
-        }
-
         let cached = if let Some(xml_cached) = node.attribute("cached") {
             match str_to_bool(xml_cached) {
                 Some(val) => val,
@@ -360,7 +411,7 @@ impl SysIOMap {
         let perms = if let Some(xml_perms) = node.attribute("perms") {
             match SysIOMapPerms::from_str(xml_perms) {
                 Ok(parsed_perms) => parsed_perms,
-                Err(()) => {
+                Err(_) => {
                     return Err(value_error(
                         xml_sdf,
                         node,
@@ -371,7 +422,7 @@ impl SysIOMap {
             }
         } else {
             // Default to read-write
-            SysIOMapPerms::ReadWrite
+            SysIOMapPerms(FrameRights::ReadWrite)
         };
 
         Ok(SysIOMap {

tool/microkit/src/viper.rs

@@ -6,7 +6,6 @@
 use sel4_capdl_initializer_types::{Cap, Object};
 
 use crate::capdl::CapDLSpecContainer;
-use crate::sdf::SysMapPerms;
 use crate::sdf::SystemDescription;
 
 fn export_define_set(name: &'static str, vector: &[u64], target: &mut String) {
@@ -312,7 +311,7 @@ pub fn get_mem_view(system: &SystemDescription, current_pd: usize) -> Option<Mem
             let mem: Mem = Mem { name, start, end };
             view.read.push(mem.clone());
 
-            let writeable = (mmap.perms & SysMapPerms::Write as u8) != 0;
+            let writeable = mmap.perms.write();
             if writeable {
                 view.readwrite.push(mem);
             }