|
From: | Daniel Henrique Barboza |
Subject: | Re: [PATCH v6 2/4] target/riscv: implement Zicboz extension |
Date: | Sat, 18 Feb 2023 06:28:24 -0300 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.7.2 |
On 2/18/23 00:44, Richard Henderson wrote:
On 2/17/23 10:34, Daniel Henrique Barboza wrote:+void helper_cbo_zero(CPURISCVState *env, target_ulong address) +{ + RISCVCPU *cpu = env_archcpu(env); + uintptr_t ra = GETPC(); + uint16_t cbozlen; + void *mem; + + check_zicbo_envcfg(env, MENVCFG_CBZE, ra); + + /* Get the size of the cache block for zero instructions. */ + cbozlen = cpu->cfg.cboz_blocksize; + + /* Mask off low-bits to align-down to the cache-block. */ + address &= ~(cbozlen - 1); + + mem = tlb_vaddr_to_host(env, address, MMU_DATA_STORE, + cpu_mmu_index(env, false)); + + if (likely(mem)) { + /* Zero the block */ + memset(mem, 0, cbozlen); + } +}Not correct. This fails to zero the block at all under a number of conditions.
Ops. The 'else' conditional in which we would zero the mem in case it's an I/O region got lost in the middle of rebasing it seems .... By the way, looking at the lack of any probing in this particular function and at the probe_writes() that ARM does, I read the spec again. A paragraph in 2.5.2 says: "A cache-block zero instruction is permitted to access the specified cache block whenever a store instruction is permitted to access the corresponding physical addresses and when the PMAs indicate that cache-block zero instructions are a supported access type. If access to the cache block is not permitted, a cache-block zero instruction raises a store page fault or store guest-page fault exception if address translation does not permit write access or raises a store access fault exception otherwise. During address translation, the instruction also checks the accessed and dirty bits and may either raise an exception or set the bits as required." PMA (Physical Memory Access) doesn't seem to be implemented in RISC-V, or at the very least it's not using the PMA acronym, so let's skip that for now. I'll add a comment mentioning it in the code mentioning that PMA for I/O should be checked and we're not doing it. But PMA aside, we have wording and conditionals that resembles the cache-block management permissions and so on, with the exception that we're not caring about LOAD access this time around. So I guess we'll also need here something like what check_zicbom_access() is doing in the next patch. Thanks, Daniel
Please have a closer look at the feedback on v5. r~
[Prev in Thread] | Current Thread | [Next in Thread] |