Re: [PATCH v6 2/4] target/riscv: implement Zicboz extension

[Daniel Henrique Barboza]

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~