Hi Luisccc, Thank you for this patch. I have two main questions about the implementation:
The SPMP CSR write functions don't flush the TLB after modifying entries: - spmpcfg_csr_write() - spmpaddr_csr_write() - sspmpen_csr_write() PMP implements granularity handling for address matching (masking low bits in TOR mode, reinterpreting NA4 as NAPOT, etc.), but SPMP doesn't. https://lists.nongnu.org/archive/html/qemu-riscv/2025-10/msg00416.html <https://lists.nongnu.org/archive/html/qemu-riscv/2025-10/msg00416.html> +/* > + * Accessor method to extract address matching type 'a field' from cfg reg > + */ > +uint8_t spmp_get_a_field(uint8_t cfg) > +{ > + uint8_t a = cfg >> 3; > + return a & 0x3; > +} + > +/* > + * Check whether mstatus.sum is set. > + */ > +static inline int sum_is_set(CPURISCVState *env) > +{ > + if (env->mstatus & MSTATUS_SUM) { > + return 1; > + } > + > + return 0; > +} > These two helper functions could be simplified: uint8_t spmp_get_a_field(uint8_t cfg) { return (cfg & SPMP_AMATCH) >> 3; } static inline bool sum_is_set(CPURISCVState *env) { return !!(env->mstatus & MSTATUS_SUM); } +static void spmp_update_rule_addr(CPURISCVState *env, uint32_t spmp_index) > +{ > + uint8_t this_cfg = env->spmp_state.spmp[spmp_index].cfg_reg; > + target_ulong this_addr = env->spmp_state.spmp[spmp_index].addr_reg; > + target_ulong prev_addr = 0u; > + target_ulong sa = 0u; > + target_ulong ea = 0u; > + > + if (spmp_index >= 1u) { > + prev_addr = env->spmp_state.spmp[spmp_index - 1].addr_reg; > + } > + > + switch (spmp_get_a_field(this_cfg)) { > + case SPMP_AMATCH_OFF: > + sa = 0u; > + ea = -1; > + break; > + > + case SPMP_AMATCH_TOR: > + sa = prev_addr << 2; /* shift up from [xx:0] to [xx+2:2] */ > + ea = (this_addr << 2) - 1u; > + break; > + > + case SPMP_AMATCH_NA4: > + sa = this_addr << 2; /* shift up from [xx:0] to [xx+2:2] */ > + ea = (sa + 4u) - 1u; > + break; > + > + case SPMP_AMATCH_NAPOT: > + spmp_decode_napot(this_addr, &sa, &ea); > + break; > + > + default: > + sa = 0u; > + ea = 0u; > + break; > + } > + > + env->spmp_state.addr[spmp_index].sa = sa; > + env->spmp_state.addr[spmp_index].ea = ea; > +} The current SPMP implementation doesn't handle granularity configuration like PMP does. int g = spmp_get_granularity_g(env); case SPMP_AMATCH_TOR: /* Bits spmpaddr[G-1:0] do not affect the TOR address-matching logic. */ if (g >= 1) { prev_addr &= ~((1ULL << g) - 1ULL); this_addr &= ~((1ULL << g) - 1ULL); } if (prev_addr >= this_addr) { sa = ea = 0u; break; } case SPMP_AMATCH_NAPOT: /* Align to pmp_granularity */ if (g >= 2) { this_addr |= ((1ul << (g - 1ul)) - 1ul); } spmp_decode_napot(this_addr, &sa, &ea); > + > +static void spmp_update_rule_nums(CPURISCVState *env) > +{ > + int i; > + > + env->spmp_state.num_active_rules = 0; > + for (i = 0; i < MAX_RISCV_SPMPS; i++) { > + const uint8_t a_field = > + spmp_get_a_field(env->spmp_state.spmp[i].cfg_reg); > + if (SPMP_AMATCH_OFF != a_field) { > + env->spmp_state.num_active_rules++; > + } > + } > +} > Optimize spmp_update_rule_nums() to O(1) Currently this function scans all 64 entries on every config write. We can optimize it to O(1) by incrementally updating the counter: static void spmp_update_rule_nums(CPURISCVState *env, uint32_t spmp_index, uint8_t old_a_field) { uint8_t new_a_field = spmp_get_a_field(env->spmp_state.spmp[spmp_index].cfg_reg); if (old_a_field == SPMP_AMATCH_OFF && new_a_field != SPMP_AMATCH_OFF) { env->spmp_state.num_active_rules++; } else if (old_a_field != SPMP_AMATCH_OFF && new_a_field == SPMP_AMATCH_OFF) { env->spmp_state.num_active_rules--; } } > + > +static uint8_t spmp_is_in_range(CPURISCVState *env, int spmp_index, > + target_ulong addr) > +{ > + if ((addr >= env->spmp_state.addr[spmp_index].sa) > + && (addr <= env->spmp_state.addr[spmp_index].ea)) { > + return 1; > + } > + > + return 0; > +} > + > It would be clearer to use bool instead of uint8_t for the return type. return (addr >= env->spmp_state.addr[spmp_index].sa) && (addr <= env->spmp_state.addr[spmp_index].ea); > +static bool spmp_hart_has_privs_default(CPURISCVState *env, target_ulong addr, > + spmp_priv_t *allowed_privs, > + target_ulong mode) > +{ > + bool ret; > + mode = env->virt_enabled ? PRV_U : mode; > + > + if ((!riscv_cpu_cfg(env)->spmp) || !(mode == PRV_U)) { Can we remove mode = env->virt_enabled ? PRV_U : mode; here ? +/* > + * Check if the address has required RWX privs to complete desired > operation > + */ > +bool spmp_hart_has_privs(CPURISCVState *env, target_ulong addr, > + target_ulong size, spmp_priv_t privs, spmp_priv_t *allowed_privs, > + target_ulong mode) > +{ > + int i = 0; > + int ret = -1; > + int spmp_size = 0; > + target_ulong s = 0; > + target_ulong e = 0; > + bool spmpen_en = false; > + > + /* If it is either VS or VU mode, we treat it as U mode */ > + mode = env->virt_enabled ? PRV_U : mode; > + > + /* Short cut for M-mode access */ > + if (mode == PRV_M) { > + *allowed_privs = SPMP_READ | SPMP_WRITE | SPMP_EXEC; > + return true; > + } > + > + /* Short cut if no rules */ > + if (env->spmp_state.num_active_rules == 0) { > + return spmp_hart_has_privs_default(env, addr, allowed_privs, > mode); > + } > We need to check this, num_active_rules == 0 doesn't mean no implemented entries (OFF). SEPC : If the effective privilege mode of the access is S/U and no SPMP entry matches, but at least one SPMP entry is implemented, the access is denied. > + > + if (size == 0) { > + if (riscv_cpu_cfg(env)->mmu) { > + /* > + * If size is unknown (0), assume that all bytes > + * from addr to the end of the page will be accessed. > + */ > + spmp_size = -(addr | TARGET_PAGE_MASK); > + } else { > + spmp_size = sizeof(target_ulong); > + } > + } else { > + spmp_size = size; > + } > + > + /* It depends on mpmpdeleg */ > + for (i = 0; i < env->spmp_state.num_deleg_rules; i++) { > + s = spmp_is_in_range(env, i, addr); > + e = spmp_is_in_range(env, i, addr + spmp_size - 1); > + spmpen_en = spmp_get_spmpen_bit(env, i); > + > + /* partially inside */ > + if ((s + e) == 1) { > + qemu_log_mask(LOG_GUEST_ERROR, > + "%s: spmp violation - access is partially > inside\n", > + __func__); > + ret = 0; > + break; > + } > return false directly ? > + > + /* fully inside */ > + const uint8_t a_field = > + spmp_get_a_field(env->spmp_state.spmp[i].cfg_reg); > + > + /* > + * Convert the SPMP permissions to match the truth table in the > + * SPMP spec. > + */ > + const uint8_t spmp_operation = > + (env->spmp_state.spmp[i].cfg_reg & > SPMP_EXEC) > + | (env->spmp_state.spmp[i].cfg_reg & > SPMP_WRITE) > + | (env->spmp_state.spmp[i].cfg_reg & > SPMP_READ); > + > + if (((s + e) == 2) && (SPMP_AMATCH_OFF != a_field) && spmpen_en) { > + /* > + * If the SPMP entry is not off, spmpen bit is set, and the > address > + * is in range, do the priv check > + */ > + > + /* Shared not set */ > + if (!(env->spmp_state.spmp[i].cfg_reg & SPMP_SHARED)) { > + /* > + * Deny if: > + * S mode access, with SUM not set, and UMODE set. > + * U mode access, with UMODE not set. > + */ > + if ((mode == PRV_S && !sum_is_set(env) && > + (env->spmp_state.spmp[i].cfg_reg & SPMP_UMODE)) || > + (mode == PRV_U && > + !(env->spmp_state.spmp[i].cfg_reg & SPMP_UMODE))) > { > + *allowed_privs = 0; > + } > + /* > + * EnforceNoX if: > + * S mode access, with SUM set, and UMODE set. > + * Note: The specification has the table in RWX, the > oposite > + * of the order in the cfg reg. > + */ > + else if (mode == PRV_S && sum_is_set(env) && > + (env->spmp_state.spmp[i].cfg_reg & SPMP_UMODE)) { > + switch (spmp_operation) { > + case 0: > + case 2: > + case 4: > + case 6: > + *allowed_privs = 0; > + break; > + case 1: > + case 5: > + *allowed_privs = SPMP_READ; > + break; > + case 3: > + case 7: > + *allowed_privs = SPMP_READ | SPMP_WRITE; > + break; > + default: > + g_assert_not_reached(); > + } > + } else { > + /* Check for reserved configs */ > + if (spmp_operation == 2 || spmp_operation == 6) { > + *allowed_privs = 0; > + } else { > + /* U mode falls here - Enforce */ > + *allowed_privs = spmp_operation & 0x7; > + } > + } > + } > + /* Set Shared bit */ > + else { > Lack of spmpcfg.U == 0. if (!(env->spmp_state.spmp[i].cfg_reg & SPMP_UMODE)) { *allowed_privs = 0; } else if (mode == PRV_S) { +/* > + * Accessor to set the cfg reg for a specific SPMP/HART > + * Bounds checks. > + */ > +void spmpcfg_csr_write(CPURISCVState *env, uint32_t reg_index, > + target_ulong val, bool m_mode_access) > +{ > + bool locked = m_mode_access ? false : is_entry_locked(env, reg_index); > + > + /* If within bounds and not locked */ > + if (reg_index < env->spmp_state.num_deleg_rules && !locked) { > No blank > + env->spmp_state.spmp[reg_index].cfg_reg = val; > + /* Storing this allows for faster switching with the sspmpen ext > */ > + env->spmp_state.locked_rules |= > + ((val & SPMP_LOCK) >> 7 & 0x1) << > reg_index; > + > + spmp_update_rule(env, reg_index); > + } else { > + if (locked) { > + qemu_log_mask(LOG_GUEST_ERROR, > + "%s: ignoring spmpcfg write - locked entry\n", > __func__); > + } else { > + qemu_log_mask(LOG_GUEST_ERROR, > + "%s: ignoring spmpcfg write - out of bounds\n", > __func__); > + } > + } > +} 1. SPMP configuration changes require TLB flush (currently missing) When SPMP entries are modified, cached TLB entries may still contain old permission information 2. NA4 mode should be reinterpreted as NAPOT when granularity > 4 bytes if (reg_index < env->spmp_state.num_deleg_rules && !locked) { if (env->spmp_state.spmp[reg_index].cfg_reg != new_val) { uint8_t old_a_field = spmp_get_a_field(env->spmp_state.spmp[reg_index].cfg_reg); uint8_t a_field = spmp_get_a_field(new_val); /* * When granularity g >= 1 (i.e., granularity > 4 bytes), * the NA4 (Naturally Aligned 4-byte) mode is not selectable. * In this case, an NA4 setting is reinterpreted as a NAPOT mode. */ if ((riscv_cpu_cfg(env)->pmp_granularity > MIN_RISCV_PMP_GRANULARITY) && (a_field == SPMP_AMATCH_NA4)) { new_val |= SPMP_AMATCH; } env->spmp_state.spmp[reg_index].cfg_reg = new_val; spmp_update_rule(env, reg_index, old_a_field); tlb_flush(env_cpu(env)); > > +/* > + * Handle a write to a spmpaddr CSR > + */ > +void spmpaddr_csr_write(CPURISCVState *env, uint32_t addr_index, > + target_ulong val, bool m_mode_access) > +{ > + bool locked = m_mode_access ? false : is_entry_locked(env, > addr_index); > + > + /* If within bounds and not locked */ > + if (addr_index < env->spmp_state.num_deleg_rules && !locked) { > + > + env->spmp_state.spmp[addr_index].addr_reg = val; > + spmp_update_rule(env, addr_index); > + } else { > + if (locked) { > + qemu_log_mask(LOG_GUEST_ERROR, > + "%s: ignoring spmpaddr write - locked entry\n", > __func__); > + } else { > + qemu_log_mask(LOG_GUEST_ERROR, > + "%s: ignoring spmpaddr write - out of bounds\n", > __func__); > + } > + } > +} We need to handle flush here if (env->spmp_state.spmp[addr_index].addr_reg != new_val) { uint8_t old_a_field = spmp_get_a_field(env->spmp_state.spmp[addr_index].cfg_reg); env->spmp_state.spmp[addr_index].addr_reg = new_val; spmp_update_rule(env, addr_index, old_a_field); tlb_flush(env_cpu(env)); } > +/* > + * Handle a read from a spmpaddr CSR > + */ > +target_ulong spmpaddr_csr_read(CPURISCVState *env, uint32_t addr_index) > +{ > + target_ulong val = 0; > + > + if (addr_index < env->spmp_state.num_deleg_rules) { > + val = env->spmp_state.spmp[addr_index].addr_reg; Handle granularity int g = spmp_get_granularity_g(env); switch (spmp_get_a_field(env->spmp_state.spmp[addr_index].cfg_reg)) { case SPMP_AMATCH_OFF: /* fallthrough */ case SPMP_AMATCH_TOR: /* Bit [g-1:0] read all zero */ if (g >= 1 && g < TARGET_LONG_BITS) { val = deposit64(val, 0, g, 0); } break; case SPMP_AMATCH_NAPOT: /* Bit [g-2:0] read all one */ if (g >= 2 && g < TARGET_LONG_BITS) { val = deposit64(val, 0, g - 1, -1ULL); } break; default: break; } > +/* > + * Handle a write to the sspmpen CSR > + */ > +void sspmpen_csr_write(CPURISCVState *env, uint64_t new_val) > +{ > + uint64_t mask = (env->spmp_state.num_deleg_rules == MAX_RISCV_SPMPS) ? > + ~0ULL : ((1ULL << env->spmp_state.num_deleg_rules) - > 1); > + > + /* If the rule is locked, the bit cannot be changed */ > + env->spmp_state.spmpen = > + (env->spmp_state.spmpen & > env->spmp_state.locked_rules) | > + (new_val & ~env->spmp_state.locked_rules); > + env->spmp_state.spmpen &= mask; > +} > flush() > + > +/* > + * Convert SPMP privilege to TLB page privilege. > + */ > +int spmp_priv_to_page_prot(spmp_priv_t spmp_priv) > +{ > + int prot = 0; > + > + if (spmp_priv & SPMP_READ) { > + prot |= PAGE_READ; > + } > + if (spmp_priv & SPMP_WRITE) { > + prot |= PAGE_WRITE; > + } > + if (spmp_priv & SPMP_EXEC) { > + prot |= PAGE_EXEC; > + } > + > + return prot; > +} > int spmp_priv_to_page_prot(spmp_priv_t spmp_priv) { return spmp_priv & PAGE_RWX; } > + > +void spmp_unlock_entries(CPURISCVState *env) > +{ > + /* Reset everything */ > + for (int i = 0; i < MAX_RISCV_SPMPS; i++) { > + env->spmp_state.spmp[i].cfg_reg &= ~(SPMP_LOCK | SPMP_AMATCH); > + } > Maybe we can clear it all ? void spmp_unlock_entries(CPURISCVState *env) { /* Reset everything */ memset(&env->spmp_state, 0, sizeof(env->spmp_state)); } > + > + env->spmp_state.locked_rules = 0; > + env->spmp_state.num_active_rules = 0; > +} > > >
