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

[Weiwei Li <liweiwei@iscas.ac.cn>]

在 2022/3/1 下午11:59, Richard Henderson 写道:
> 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.
Yeah. I lost this kind of case . So this is a bug. Maybe we can add a 
new parameter like  'write' to helper_csrrw to indicate that write 
operation should be done.
>
>
> r~