Re: [PATCH 2/5] nolibc: add support for s390

[Sven Schnelle]

Willy Tarreau <w@xxxxxx> writes:

> On Fri, Dec 09, 2022 at 03:19:36PM +0100, Sven Schnelle wrote:
>> Use arch-x86_64 as a template. Not really different, but
>> we have our own mmap syscall which takes a structure instead
>> of discrete arguments.
>
> I'm fine with placing the mmap syscall inside the arch-s390 file, though
> it differs from what's done for other syscalls. But I admit that mmap is
> one of these syscalls that differ between various archs and that it's not
> shocking to leave it per-arch.
>
> However you're having an issue here:
>
>> diff --git a/tools/include/nolibc/arch-s390.h b/tools/include/nolibc/arch-s390.h
>> new file mode 100644
>> index 000000000000..34b744e2f7d6
>> --- /dev/null
>> +++ b/tools/include/nolibc/arch-s390.h
>                              ^^^^^^^^^
> vs:
>
>> diff --git a/tools/include/nolibc/arch.h b/tools/include/nolibc/arch.h
>> index 4c6992321b0d..fcded65b98d7 100644
>> --- a/tools/include/nolibc/arch.h
>> +++ b/tools/include/nolibc/arch.h
>> @@ -27,6 +27,8 @@
>>  #include "arch-mips.h"
>>  #elif defined(__riscv)
>>  #include "arch-riscv.h"
>> +#elif defined(__s390x__)
>> +#include "arch-s390x.h"
>              ^^^^^^^^^^
>
> As you see the file is not the same so if you build by including nolibc.h
> directly it will not work. The difference between s390 and s390x has never
> been very clear to me, so I can't easily suggest which name is the most
> suitable, but you'll definitely have to choose one :-)  If it's just a
> matter of dropping that trailing 'x' there, I think we can fix your patch
> here without re-submitting, let us know.

Whoops. One of my colleagues suggested that i should name the file
arch-390x.h. Reason for this is that the architecture name "s390"
describes the 31bit (compat) architecture mode in userspace, and "s390x"
the 64 bit mode. To make it a bit more complicated, the architecture in
the kernel is named "s390". That's because in the beginning the kernel
could run in 31bit or 64 bit mode, and there can be only one
architecture name. When support for running 31bit kernels was removed
years ago, the architecture name s390 was kept because renaming the
architecture would have been quite some fun. So in short:

Kernel s390 == 64 bit only
Userspace s390 == 31 bit
Userspace s390x == 64 bit

So i tried renaming the header file, noted that the Makefile just uses:

nolibc_arch := $(patsubst arm64,aarch64,$(ARCH))
arch_file := arch-$(nolibc_arch).h

which would then need special handling. Obviously i failed to revert the
change completely during rebase.

So it should be:

>> +#elif defined(__s390x__)
>> +#include "arch-s390.h"

I'm fine with both - either you fixing it up or me sending a v2.

Thanks!
Sven