Re: [Qemu-devel] [PATCH v1 06/21] RISC-V FPU Support

[Richard Henderson]

On 01/23/2018 01:37 PM, Michael Clark wrote:
> 
> 
> On Wed, Jan 3, 2018 at 12:10 PM, Richard Henderson
> <address@hidden <mailto:address@hidden>> wrote:
> 
>     On 01/02/2018 04:44 PM, Michael Clark wrote:
>     > +/* convert RISC-V rounding mode to IEEE library numbers */
>     > +unsigned int ieee_rm[] = {
> 
>     static const.
> 
> 
> Done. 
> 
>     > +/* obtain rm value to use in computation
>     > + * as the last step, convert rm codes to what the softfloat library 
> expects
>     > + * Adapted from Spike's decode.h:RM
>     > + */
>     > +#define RM ({                                             \
>     > +if (rm == 7) {                                            \
>     > +    rm = env->frm;                               \
>     > +}                                                         \
>     > +if (rm > 4) {                                             \
>     > +    helper_raise_exception(env, RISCV_EXCP_ILLEGAL_INST); \
>     > +}                                                         \
>     > +ieee_rm[rm]; })
> 
>     Please use inline functions and not these macros.
> 
> 
> Done.
> 
> In fact the previous code would, with normal control flow, dereference
> ieee_rm[rm] with an out of bounds round mode so assuming 
> helper_raise_exception
> does a longjmp, i've inserted g_assert_not_reached() after the exception. An
> analyser could detect that ieee_rm has an out of bounds access assuming normal
> control flow. e.g.
> 
> static inline void set_fp_round_mode(CPURISCVState *env, uint64_t rm)
> {
>     if (rm == 7) {
>         rm = env->frm;
>     } else if (rm > 4) {
>         helper_raise_exception(env, RISCV_EXCP_ILLEGAL_INST);
>         g_assert_not_reached();

Yes, raise_exception exits via longjmp.  This is a good change.

> Are translations not implicitly indexed by cpu_mmu_index? i.e. do we also need
> to add cpu_mmu_index to cpu_get_tb_cpu_state flags to prevent code translated
> for one value of mmu_index running in code with another value of mmu_index (in
> the case of riscv, we currently return processor mode / privilege level in
> cpu_mmu_index).

No, there is no implicit indexing.  That's why I've mentioned exactly this at
least twice in review so far.

There are two ways to do this correctly.  One is to add all the bits that
specify processor state (e.g. Alpha stores pal_code bit and supervisor bit).
Another is to actually encode the mmu_idx into the flags (e.g. ARM).


r~