Re: [PATCH v7 12/14] target/riscv: rvk: add CSR support for Zkr

[Richard Henderson <richard.henderson@linaro.org>]

On 2/28/22 16:27, Weiwei Li wrote:
> 
> 在 2022/3/1 上午9:44, Weiwei Li 写道:
>>
>> 在 2022/3/1 上午4:11, Richard Henderson 写道:
>>> On 2/28/22 04:48, Weiwei Li wrote:
>>>> +/* Crypto Extension */
>>>> +static RISCVException rmw_seed(CPURISCVState *env, int csrno,
>>>> +                              target_ulong *ret_value,
>>>> +                              target_ulong new_value, target_ulong write_mask)
>>>> +{
>>>> +    if (!write_mask) {
>>>> +        return RISCV_EXCP_ILLEGAL_INST;
>>>> +    }
>>>
>>> This is incorrect.  The error should only be with a write-mask of the actual x0 
>>> register, not another register which happens to contain 0.  There is in fact no way to 
>>> diagnose exactly what you want here, which IIRC has an existing fixme comment somewhere.
>> Yeah. write_mask is also used in riscv_csrrw_check to check whether the read-only csr is 
>> written. We cannot distinguish x0 and reg which contains 0  here without changing total 
>> progress of csr read/write.
>>>
> I seems misunderstand the code for csr read/write:  write_mask will be set zero only for 
> read-only operation (CSRRS/CSRRC with rs1=x0 or CSRRSI/CSRRCI with uimm=0) via do_csrr --> 
> helper_csrr -> riscv_csrrw call-chain.
> 
> The write_mask for do_csrw and do_csrrw will not be zero.
> 
> As said in the spec :
> 
> "TheseedCSR must be accessed with a read-write instruction. A read-only instruction such 
> asCSRRS/CSRRC
> withrs1=x0orCSRRSI/CSRRCIwithuimm=0will raise an illegal instruction exception. "
> 
> So it's suitable to check write_mask here.

Consider CSRRS with rs1=x31.  In that case mask will be the value in x31.  Even if the 
value is 0, this is still considered a read-write instruction.


r~