Re: [PATCH 4/4] linux-user: Emulate /proc/cpuinfo on s390x

[David Hildenbrand <david@redhat.com>]

On 01.06.23 18:25, Ilya Leoshkevich wrote:
> Some s390x userspace programs are confused when seeing a foreign
> /proc/cpuinfo [1]. Add the emulation for s390x; follow the respective
> kernel code structure where possible.
> 
> [1] https://bugzilla.redhat.com/show_bug.cgi?id=2211472
> 

Would be nice to have an example output here.

> Reported-by: Tulio Magno Quites Machado Filho <tuliom@redhat.com>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>   linux-user/syscall.c | 106 ++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 105 insertions(+), 1 deletion(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 3e6ed51ce62..d142e09dc70 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -8339,6 +8339,109 @@ static int open_cpuinfo(CPUArchState *cpu_env, int fd)
>   }
>   #endif
>   

[...]

> +
> +static void show_cpuinfo(CPUArchState *cpu_env, int fd, unsigned long n)
> +{
> +    if (n == 0) {
> +        show_cpu_summary(cpu_env, fd);
> +    }
> +    dprintf(fd, "\ncpu number      : %ld\n", n);
> +    show_cpu_ids(cpu_env, fd, n);
> +}

Considering the n==0 special case, it might be cleaner to just place 
that code directly into open_cpuinfo and get rid of show_cpuinfo().

> +
> +static int open_cpuinfo(CPUArchState *cpu_env, int fd)
> +{
> +    int num_cpus = sysconf(_SC_NPROCESSORS_ONLN);
> +    int i;
> +
> +    for (i = 0; i < num_cpus; i++) {
> +        show_cpuinfo(cpu_env, fd, i);
> +    }
> +    return 0;
> +}
> +#endif
> +
>   #if defined(TARGET_M68K)
>   static int open_hardware(CPUArchState *cpu_env, int fd)
>   {
> @@ -8364,7 +8467,8 @@ int do_guest_openat(CPUArchState *cpu_env, int dirfd, const char *pathname,
>   #if HOST_BIG_ENDIAN != TARGET_BIG_ENDIAN
>           { "/proc/net/route", open_net_route, is_proc },
>   #endif
> -#if defined(TARGET_SPARC) || defined(TARGET_HPPA) || defined(TARGET_RISCV)
> +#if defined(TARGET_SPARC) || defined(TARGET_HPPA) || \
> +    defined(TARGET_RISCV) || defined(TARGET_S390X)
>           { "/proc/cpuinfo", open_cpuinfo, is_proc },
>   #endif
>   #if defined(TARGET_M68K)

In general, looks very good to me, thanks! With the fixup:

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb