Re: [RFC v2] linux-user/riscv: Add syscall riscv_hwprobe

[Robbin Ehn <rehn@rivosinc.com>]

On Fri, 2023-06-02 at 16:02 +0200, Andrew Jones wrote:
> On Fri, Jun 02, 2023 at 11:41:11AM +0200, Robbin Ehn wrote:
> > This patch adds the new syscall for the
> > "RISC-V Hardware Probing Interface"
> > (https://docs.kernel.org/riscv/hwprobe.html).
> > 
> > Signed-off-by: Robbin Ehn <rehn@rivosinc.com>
> > ---
> > v1->v2: Moved to syscall.c
> > ---
> >  linux-user/riscv/syscall32_nr.h |   1 +
> >  linux-user/riscv/syscall64_nr.h |   1 +
> >  linux-user/syscall.c            | 109 ++++++++++++++++++++++++++++++++
> >  3 files changed, 111 insertions(+)
> > 
> > diff --git a/linux-user/riscv/syscall32_nr.h b/linux-user/riscv/syscall32_nr.h
> > index 1327d7dffa..412e58e5b2 100644
> > --- a/linux-user/riscv/syscall32_nr.h
> > +++ b/linux-user/riscv/syscall32_nr.h
> 
> This file should not be modified, it should be generated, but this is an
> RFC, so hacking it is OK, but the hack should be in a separate patch.

Ok, thanks.

> 
> > @@ -228,6 +228,7 @@
> >  #define TARGET_NR_accept4 242
> >  #define TARGET_NR_arch_specific_syscall 244
> >  #define TARGET_NR_riscv_flush_icache (TARGET_NR_arch_specific_syscall + 15)
> > +#define TARGET_NR_riscv_hwprobe (TARGET_NR_arch_specific_syscall + 14)
> >  #define TARGET_NR_prlimit64 261
> >  #define TARGET_NR_fanotify_init 262
> >  #define TARGET_NR_fanotify_mark 263
> > diff --git a/linux-user/riscv/syscall64_nr.h b/linux-user/riscv/syscall64_nr.h
> > index 6659751933..29e1eb2075 100644
> > --- a/linux-user/riscv/syscall64_nr.h
> > +++ b/linux-user/riscv/syscall64_nr.h
> 
> Same

Ok, thanks.

> 
> > @@ -251,6 +251,7 @@
> >  #define TARGET_NR_recvmmsg 243
> >  #define TARGET_NR_arch_specific_syscall 244
> >  #define TARGET_NR_riscv_flush_icache (TARGET_NR_arch_specific_syscall + 15)
> > +#define TARGET_NR_riscv_hwprobe (TARGET_NR_arch_specific_syscall + 14)
> >  #define TARGET_NR_wait4 260
> >  #define TARGET_NR_prlimit64 261
> >  #define TARGET_NR_fanotify_init 262
> > diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> > index 89b58b386b..cd394bbe26 100644
> > --- a/linux-user/syscall.c
> > +++ b/linux-user/syscall.c
> > @@ -8772,6 +8772,74 @@ static int do_getdents64(abi_long dirfd, abi_long arg2, abi_long count)
> >  }
> >  #endif /* TARGET_NR_getdents64 */
> >  
> > +#if defined(TARGET_RISCV)
> > +
> > +#define RISCV_HWPROBE_KEY_MVENDORID     0
> > +#define RISCV_HWPROBE_KEY_MARCHID       1
> > +#define RISCV_HWPROBE_KEY_MIMPID        2
> > +
> > +#define RISCV_HWPROBE_KEY_BASE_BEHAVIOR 3
> > +#define     RISCV_HWPROBE_BASE_BEHAVIOR_IMA (1 << 0)
> > +
> > +#define RISCV_HWPROBE_KEY_IMA_EXT_0     4
> > +#define     RISCV_HWPROBE_IMA_FD       (1 << 0)
> > +#define     RISCV_HWPROBE_IMA_C        (1 << 1)
> > +
> > +#define RISCV_HWPROBE_KEY_CPUPERF_0     5
> > +#define     RISCV_HWPROBE_MISALIGNED_UNKNOWN     (0 << 0)
> > +#define     RISCV_HWPROBE_MISALIGNED_EMULATED    (1 << 0)
> > +#define     RISCV_HWPROBE_MISALIGNED_SLOW        (2 << 0)
> > +#define     RISCV_HWPROBE_MISALIGNED_FAST        (3 << 0)
> > +#define     RISCV_HWPROBE_MISALIGNED_UNSUPPORTED (4 << 0)
> > +#define     RISCV_HWPROBE_MISALIGNED_MASK        (7 << 0)
> > +
> > +struct riscv_hwprobe {
> > +    int64_t  key;
> > +    uint64_t value;
> > +};
> 
> The above is all uapi so Linux's arch/riscv/include/uapi/asm/hwprobe.h
> should be picked up on Linux header update. You'll need to modify the
> script, scripts/update-linux-headers.sh, to do that by adding a new
> riscv-specific block. Hacking this by importing the header file manually
> is fine for an RFC, but that should be a separate patch or part of the
> syscall define hack patch. And hack patches should be clearly tagged as
> "NOT FOR MERGE".

Ok, thanks.

> 
> > +
> > +static void risc_hwprobe_fill_pairs(CPURISCVState *env,
> > +                                    struct riscv_hwprobe *pair,
> > +                                    size_t pair_count)
> > +{
> > +    const RISCVCPUConfig *cfg = riscv_cpu_cfg(env);
> > +
> > +    for (; pair_count > 0; pair_count--, pair++) {
> > +        pair->value = 0;
> > +        switch (pair->key) {
> > +        case RISCV_HWPROBE_KEY_MVENDORID:
> > +            pair->value = cfg->mvendorid;
> > +            break;
> > +        case RISCV_HWPROBE_KEY_MARCHID:
> > +            pair->value = cfg->marchid;
> > +            break;
> > +        case RISCV_HWPROBE_KEY_MIMPID:
> > +            pair->value = cfg->mimpid;
> > +            break;
> > +        case RISCV_HWPROBE_KEY_BASE_BEHAVIOR:
> > +            pair->value = riscv_has_ext(env, RVI) &&
> > +                          riscv_has_ext(env, RVM) &&
> > +                          riscv_has_ext(env, RVA) ?
> > +                          RISCV_HWPROBE_BASE_BEHAVIOR_IMA : 0;
> > +            break;
> > +        case RISCV_HWPROBE_KEY_IMA_EXT_0:
> > +            pair->value = riscv_has_ext(env, RVF) &&
> > +                          riscv_has_ext(env, RVD) ?
> > +                          RISCV_HWPROBE_IMA_FD : 0;
> > +            pair->value |= riscv_has_ext(env, RVC) ?
> > +                           RISCV_HWPROBE_IMA_C : pair->value;
> > +            break;
> > +        case RISCV_HWPROBE_KEY_CPUPERF_0:
> > +            pair->value = RISCV_HWPROBE_MISALIGNED_UNKNOWN;
> > +            break;
> > +        default:
> > +            pair->key = -1;
> > +        break;
> > +        }
> > +    }
> > +}
> > +#endif
> > +
> >  #if defined(TARGET_NR_pivot_root) && defined(__NR_pivot_root)
> >  _syscall2(int, pivot_root, const char *, new_root, const char *, put_old)
> >  #endif
> > @@ -13469,6 +13537,47 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1,
> >          return ret;
> >  #endif
> >  
> > +#if defined(TARGET_RISCV)
> > +    case TARGET_NR_riscv_hwprobe:
> > +        {
> 
> The { goes under the c of case, which will shift all the below four spaces
> left as well.

This was an attempt to blend in, i.e. same style as the preceding case.
I'll change, thanks.

> 
> > +            struct riscv_hwprobe *host_pairs;
> > +
> > +            /* flags must be 0 */
> > +            if (arg5 != 0) {
> > +                return -TARGET_EINVAL;
> > +            }
> > +
> > +            /* check cpu_set */
> > +            if (arg3 != 0) {
> > +                int ccpu;
> > +                size_t cpu_setsize = CPU_ALLOC_SIZE(arg3);
> > +                cpu_set_t *host_cpus = lock_user(VERIFY_READ, arg4,
> > +                                                 cpu_setsize, 0);
> > +                if (!host_cpus) {
> > +                    return -TARGET_EFAULT;
> > +                }
> > +                ccpu = CPU_COUNT_S(cpu_setsize, host_cpus);
> > +                unlock_user(host_cpus, arg4, cpu_setsize);
> > +                /* no selected cpu */
> > +                if (ccpu == 0) {
> > +                    return -TARGET_EINVAL;
> > +                }
> > +            } else if (arg4 != 0) {
> > +                return -TARGET_EINVAL;
> > +            }
> 
> I think we want
> 
>  if (arg2 == 0)
>     return 0;
> 
> here.

Ok, thanks.

> 
> > +
> > +            host_pairs = lock_user(VERIFY_WRITE, arg1,
> > +                                   sizeof(*host_pairs) * (size_t)arg2, 0);
> > +            if (host_pairs == NULL) {
> > +                return -TARGET_EFAULT;
> > +            }
> > +            risc_hwprobe_fill_pairs(cpu_env, host_pairs, arg2);
> > +            unlock_user(host_pairs, arg1, sizeof(*host_pairs) * (size_t)arg2);
> > +            ret = 0;
> > +        }
> > +        return ret;
> > +#endif
> > +
> >      default:
> >          qemu_log_mask(LOG_UNIMP, "Unsupported syscall: %d\n", num);
> >          return -TARGET_ENOSYS;
> > -- 
> > 2.39.2
> > 
> > 
> 
> Otherwise this looks good to me.

Thank you!

/Robbin

> 
> Thanks,
> drew