linux-user/riscv/syscall32_nr.h | 1 + linux-user/riscv/syscall64_nr.h | 1 + linux-user/syscall.c | 109 ++++++++++++++++++++++++++++++++ 3 files changed, 111 insertions(+)
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
@@ -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
@@ -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;
+};
+
+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:
+ {
+ 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;
+ }
+
+ 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.2On 6/2/23 02:41, Robbin Ehn wrote:
> +struct riscv_hwprobe {
> + int64_t key;
> + uint64_t value;
> +};
This needs to use abi_llong and abi_ullong, as the guest may not have the same alignment
requirements as the host.
> + case RISCV_HWPROBE_KEY_MVENDORID:
> + pair->value = cfg->mvendorid;
> + break;
You must use __get_user and __put_user to handle host vs guest endianness. All over.
> + case RISCV_HWPROBE_KEY_CPUPERF_0:
> + pair->value = RISCV_HWPROBE_MISALIGNED_UNKNOWN;
Is that really what you want to expose here? FAST is always going to be true, in that
handling the unaligned access in the host is going to be faster than in the emulated guest.
> + default:
> + pair->key = -1;
> + break;
Misalignment.
> +#if defined(TARGET_RISCV)
> + case TARGET_NR_riscv_hwprobe:
> + {
> + 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);
Where does CPU_ALLOC_SIZE and CPU_COUNT_S come from?
> + unlock_user(host_cpus, arg4, cpu_setsize);
> + /* no selected cpu */
> + if (ccpu == 0) {
> + return -TARGET_EINVAL;
> + }
I suppose you're just looking to see that the set is not empty?
r~
On Fri, 2023-06-02 at 19:57 -0700, Richard Henderson wrote:
>
> > + case RISCV_HWPROBE_KEY_CPUPERF_0:
> > + pair->value = RISCV_HWPROBE_MISALIGNED_UNKNOWN;
>
> Is that really what you want to expose here? FAST is always going to be true, in that
> handling the unaligned access in the host is going to be faster than in the emulated guest.
The plan was to add this in the cpu cfg in a later patch.
This setting e.g. changes jitted code and therefore it's helpful if such generated code
is the same in the emulated guest as it would be on that actual cpu.
I'll change to FAST as the hardcoded value until then.
>
>
> Where does CPU_ALLOC_SIZE and CPU_COUNT_S come from?
>
> > + unlock_user(host_cpus, arg4, cpu_setsize);
> > + /* no selected cpu */
> > + if (ccpu == 0) {
> > + return -TARGET_EINVAL;
> > + }
>
> I suppose you're just looking to see that the set is not empty?
Yes, exactly.
Thanks again! I'll send out an update.
/Robbin
>
>
> r~
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.
> @@ -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
> @@ -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".
> +
> +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.
> + 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.
> +
> + 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.
Thanks,
drew
On 6/2/23 07:02, Andrew Jones wrote:
>> +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".
Not true. linux-user/ never looks at linux-headers/.
r~
On Fri, Jun 02, 2023 at 07:58:30PM -0700, Richard Henderson wrote:
> On 6/2/23 07:02, Andrew Jones wrote:
> > > +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".
>
>
> Not true. linux-user/ never looks at linux-headers/.
Ah, thanks. I should have known better than to try and review a linux-user
patch, since I know almost nothing about it! Is uapi like this usually
duplicated, as was done in this patch?
Thanks,
drew
On 6/3/23 08:50, Andrew Jones wrote:
> On Fri, Jun 02, 2023 at 07:58:30PM -0700, Richard Henderson wrote:
>> On 6/2/23 07:02, Andrew Jones wrote:
>>>> +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".
>>
>>
>> Not true. linux-user/ never looks at linux-headers/.
>
> Ah, thanks. I should have known better than to try and review a linux-user
> patch, since I know almost nothing about it! Is uapi like this usually
> duplicated, as was done in this patch?
Yes, because linux-headers is only for the "native" compiler, whereas linux-user requires
the ABI of the guest.
If we were doing this all from scratch, we would mechanically parse and rewrite linux-headers.
r~
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
On Fri, Jun 02, 2023 at 04:39:20PM +0200, Robbin Ehn wrote:
> 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:
...
> > > +#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.
Hmm, I see. This function does have many cases with the indented format,
but not all of them, and the rest of the code base doesn't indent. I won't
insist on changing this, as long as checkpatch isn't complaining.
Thanks,
drew
On 6/2/23 08:07, Andrew Jones wrote:
> On Fri, Jun 02, 2023 at 04:39:20PM +0200, Robbin Ehn wrote:
>> 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:
> ...
>>>> +#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.
>
> Hmm, I see. This function does have many cases with the indented format,
> but not all of them, and the rest of the code base doesn't indent. I won't
> insist on changing this, as long as checkpatch isn't complaining.
Splitting the entire thing out to a helper function is even cleaner.
We have lots of those, but certainly not universal.
I have, from time to time, tried to clean all of this up, but no one wanted to look at a
100+ RFC patch set which only scratched the surface.
r~
On Fri, 2023-06-02 at 20:00 -0700, Richard Henderson wrote:
> On 6/2/23 08:07, Andrew Jones wrote:
> > On Fri, Jun 02, 2023 at 04:39:20PM +0200, Robbin Ehn wrote:
> > > 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:
> > ...
> > > > > +#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.
> >
> > Hmm, I see. This function does have many cases with the indented format,
> > but not all of them, and the rest of the code base doesn't indent. I won't
> > insist on changing this, as long as checkpatch isn't complaining.
>
> Splitting the entire thing out to a helper function is even cleaner.
> We have lots of those, but certainly not universal.
This was my initial reaction, even move something out of this file.
Yes I'll put it in a helper.
Thank you!
/Robbin
>
> I have, from time to time, tried to clean all of this up, but no one wanted to look at a
> 100+ RFC patch set which only scratched the surface.
>
>
> r~
>
© 2016 - 2024 Red Hat, Inc.