Re: [PATCH v2 10/14] target/riscv: Adjust vector address with mask

[LIU Zhiwei]

On 2021/11/10 下午7:11, Richard Henderson wrote:
> On 11/10/21 8:04 AM, LIU Zhiwei wrote:
> > The mask comes from the pointer masking extension, or the max value
> > corresponding to XLEN bits.
> >
> > Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com>
> > ---
> >   target/riscv/cpu.c           |  1 +
> >   target/riscv/cpu.h           |  4 ++++
> >   target/riscv/cpu_helper.c    | 40 ++++++++++++++++++++++++++++++++++++
> >   target/riscv/csr.c           | 19 +++++++++++++++++
> >   target/riscv/machine.c       | 10 +++++++++
> >   target/riscv/vector_helper.c | 23 +++++++++++++--------
> >   6 files changed, 88 insertions(+), 9 deletions(-)
> >
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index 0d2d175fa2..886388f066 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -378,6 +378,7 @@ static void riscv_cpu_reset(DeviceState *dev)
> >   #ifndef CONFIG_USER_ONLY
> >       env->misa_mxl = env->misa_mxl_max;
> >       env->priv = PRV_M;
> > +    riscv_cpu_update_mask(env);
> >       env->mstatus &= ~(MSTATUS_MIE | MSTATUS_MPRV);
> >       if (env->misa_mxl > MXL_RV32) {
> >           /*
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index 11590a510e..73d7aa9ad7 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -252,6 +252,8 @@ struct CPURISCVState {
> >       target_ulong upmmask;
> >       target_ulong upmbase;
> >   #endif
> > +    target_ulong mask;
> > +    target_ulong base;
>
> I think the name here isn't great.  Without the context of the 
> preceeding elements, the question becomes: mask of what?
>
> Better might be cur_pmmask, cur_pmbase.
>
> Broader than that, you're doing too many things in this patch. The 
> subject is "adjust vector address with mask", but you're also creating 
> new fields and updating them at priv changes, etc.  Too much.
>
> > +void riscv_cpu_update_mask(CPURISCVState *env)
>
> ... update_pmmask?
>
> > +}
> > +
> > +
> > +
>
> Watch the extra spaces.
>
> > @@ -1571,6 +1572,9 @@ static RISCVException 
> > write_mpmmask(CPURISCVState *env, int csrno,
> >       uint64_t mstatus;
> >         env->mpmmask = val;
> > +    if ((env->priv == PRV_M) && (env->mmte & M_PM_ENABLE)) {
> > +        env->mask = val;
> > +    }
>
> This needs to use the function; there are pieces missing here, notably 
> the zero-extend for RV32.

Zero-extend has been done automatically here, as these operations are in 
csr instruction.

In my opinion, it is enough here to check env->priv and pm_enable.

> I don't see any updates to the exception entry and exception return 
> paths.

Oops. I forgot this.  We should update at these places. Exception entry 
and exception return will call one same function
to change privilege,  we can update it there.

> You'll want to update the translator to use these new fields instead 
> of using the [msu]pmmask / [msu]pmbase fields directly. (Which means 
> that we will have fewer tcg variables, and need not copy the "current" 
> into DisasContext.)
Do you mean we can remove the global TCG variables pm_mask[] and pc_base[]?
If then how to transport env->cur_pmmask and env->cur_pmbase to 
DisasContext?

Thanks,
Zhiwei

> > diff --git a/target/riscv/vector_helper.c b/target/riscv/vector_helper.c
> > index 60006b1b1b..0b297f6bc8 100644
> > --- a/target/riscv/vector_helper.c
> > +++ b/target/riscv/vector_helper.c
> > @@ -123,6 +123,11 @@ static inline uint32_t vext_maxsz(uint32_t desc)
> >       return simd_maxsz(desc) << vext_lmul(desc);
> >   }
> >   +static inline target_ulong adjust_addr(CPURISCVState *env, 
> > target_ulong addr)
> > +{
> > +    return (addr & env->mask) | env->base;
> > +}
>
> The code here in vector_helper.c looks fine as a patch by itself, 
> under the subject that you have given.
>
>
> r~