Re: [PATCH v2 1/1] target/riscv: add VILL field for vtype register macro definition

[Chao Liu]

On 2024/12/11 23:55, Richard Henderson wrote:

> On 12/11/24 09:48, Chao Liu wrote:
> > Signed-off-by: Chao Liu <lc00631@tecorigin.com>
> > ---
> >   target/riscv/cpu.h | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index 284b112821..0d74ee4581 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -170,7 +170,8 @@ FIELD(VTYPE, VSEW, 3, 3)
> >   FIELD(VTYPE, VTA, 6, 1)
> >   FIELD(VTYPE, VMA, 7, 1)
> >   FIELD(VTYPE, VEDIV, 8, 2)
> > -FIELD(VTYPE, RESERVED, 10, sizeof(target_ulong) * 8 - 11)
> > +FIELD(VTYPE, RESERVED, 10, sizeof(target_ulong) * 8 - 12)
> > +FIELD(VTYPE, VILL, sizeof(target_ulong) * 8 - 1, 1)
>
> Still incorrect, for the same reason.
>
> We support execution of rv32 cpus with qemu-system-riscv64, where 
> sizeof(target_ulong) is always 8.  We support SXL/UXL, which changes 
> the behaviour at runtime.
>
> And, really, why do you want to add a define that is never used?
>
>
> r~

Thank you for your reply, I have understood it now.

Initially, by reading the RISC-V architecture manual and the QEMU source 
code, I noticed that the definition of vtype.vill was missing.

After your explanation, I reviewed the source code again and found that 
QEMU defines the vill field within the CPURISCVState. According to 
MXL_RV64 and MXL_RV32, when reading vtype, vill is incorporated into it.

Therefore, there is no need to add this definition, as it is not 
actually being used.

However, I still have one question:

since the goal is to enable qemu-system-riscv64 to support RV32, it 
seems somewhat unreasonable to have vtype.RESERVED fixed at 
"sizeof(target_ulong) * 8 - 12" and let the compiler calculate its bit 
width. The vtype.RESERVED field is [11:62] under RV64 and [11:30] under 
RV32.

Currently, it is used in the following context:

//target/riscv/vector_helper.c:34
target_ulong HELPER(vsetvl)(CPURISCVState *env, target_ulong s1,
                            target_ulong s2)
{
    ...
    target_ulong reserved = s2 &
MAKE_64BIT_MASK(R_VTYPE_RESERVED_SHIFT,
          xlen - 1 - R_VTYPE_RESERVED_SHIFT);
    ...

Regards,
Chao