Re: [PATCH v3 3/4] RISC-V: support vector extension csr

[LIU Zhiwei]

On 2020/1/7 6:00, Jim Wilson wrote:
> On 1/2/20 7:33 PM, LIU Zhiwei wrote:
> > Until v0.7.1 specification, vector status is still not defined for
> > mstatus.
>
> The v0.8 spec does define a VS bit in mstatus.
Yes, I will also support v0.8 spec after the v0.7.1 spec.
> > @@ -107,11 +112,6 @@ static int pmp(CPURISCVState *env, int csrno)
> >   /* User Floating-Point CSRs */
> >   static int read_fflags(CPURISCVState *env, int csrno, target_ulong 
> > *val)
> >   {
> > -#if !defined(CONFIG_USER_ONLY)
> > -    if (!env->debugger && !(env->mstatus & MSTATUS_FS)) {
> > -        return -1;
> > -    }
> > -#endif
> >       *val = riscv_cpu_get_fflags(env);
> >       return 0;
> >   }
>
> This allows reads of fflags when it doesn't exist, and hence does not 
> make much sense.  Instead of removing the code, you should add a check 
> for the vector extension, since the vector extension requires that 
> fcsr exist even if the base architecture doesn't include FP support.  
> Ideally this should use the VS bit, but if you don't have it then you 
> can just check to see if the vector extension was enabled as a command 
> line option.
I' sorry that there is some ambiguous here. The reason to remove these 
code is that they are redundant, and has nothing to do with the vector 
extension.  I just delete them by hand.

As you can see, all float csr has a predicate function.

static int fs(CPURISCVState *env, int csrno)
{
#if !defined(CONFIG_USER_ONLY)
    if (!env->debugger && !(env->mstatus & MSTATUS_FS)) {
        return -1;
    }
#endif
    return 0;
}

The read or write function must be called after the predicate return 0. 
So no need to check (!env->debugger && !(env->mstatus & MSTATUS_FS)  again.
> While the vector spec says that fcsr must exist, it doesn't specify 
> that the FP fields in fcsr are necessarily readable or writable when 
> there is no FP.  It also doesn't specify whether the other FP related 
> shadows of fcsr exist, like fflags.  This appears to have been left 
> unspecified.  I don't think that you should be making fflags reads and 
> writes work for a target with vector but without float.  I think it 
> would make more sense to have fcsr behave 3 different ways depending 
> on whether we have only F, only V, or both F and V.  And then we can 
> support reads and writes of only the valid fields.
Thanks. Maybe I should just only loose the check condition for fcsr.

Best Regards,
Zhiwei
> Jim