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

[Weiwei Li]

在 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 :

"The seed CSR must be accessed with a read-write instruction. A read-only instruction such as CSRRS/CSRRC
with rs1=x0 or CSRRSI/CSRRCI with uimm=0 will raise an illegal instruction exception. "

So it's suitable to check write_mask here.

> > > +    uint32_t return_status =
> > >           SEED_OPST_ES16;
> > >           
> > >           +
> > >           
> > >           +    *ret_value = return_status;
> > >           
> > >           +    if (return_status == SEED_OPST_ES16) {
> > >           
> > >           +        uint16_t random_number;
> > >           
> > >           +        qemu_guest_getrandom_nofail(&random_number,
> > >           sizeof(random_number));
> > >           
> > >           +        *ret_value = (*ret_value) | random_number;
> > >           
> > >           +    } else if (return_status == SEED_OPST_BIST) {
> > >           
> > >           +        /* Do nothing */
> > >           
> > >           +    } else if (return_status == SEED_OPST_WAIT) {
> > >           
> > >           +        /* Do nothing */
> > >           
> > >           +    } else if (return_status == SEED_OPST_DEAD) {
> > >           
> > >           +        /* Do nothing */
> > >           
> > >           +    }
> > >           
> > >         
> >         
> >         This is also incorrect.  This should be
> >         
> >         
> >             uint32_t result;
> >         
> >             uint16_t random_v;
> >         
> >             Error *random_e = NULL;
> >         
> >             int random_r;
> >         
> >         
> >             random_r = guest_getrandom(&random_v, 2, &random_e);
> >         
> >             if (unlikely(random_r < 0)) {
> >         
> >                 /*
> >         
> >                  * Failed, for unknown reasons in the crypto subsystem.
> >         
> >                  * The best we can do is log the reason and return a
> >         
> >                  * failure indication to the guest.  There is no reason
> >         
> >                  * we know to expect the failure to be transitory, so
> >         
> >                  * indicate DEAD to avoid having the guest spin on WAIT.
> >         
> >                  */
> >         
> >                 qemu_log_mask(LOG_UNIMP, "%s: Crypto failure: %s",
> >         
> >                               __func__, error_get_pretty(random_e));
> >         
> >                 error_free(random_e);
> >         
> >                 result = SEED_OPST_DEAD;
> >         
> >             } else {
> >         
> >                 result = random_v | SEED_OPST_ES16;
> >         
> >             }
> >         
> >         
> >         C.f. target/arm/helper.c, rndr_readfn.
> >         
> >       
>       
>       OK.  I'll fix this.
>       
>       
>       Regards,
>       
>       
>       Weiwei Li
>       
>       
> >         
> >         
> >         r~
> >         
> >       
>