diff mbox series

[for,6.2,23/49] bsd-user: pull in target_arch_thread.h update target_arch_elf.h

Message ID 20210807214242.82385-24-imp@bsdimp.com
State New
Headers show
Series bsd-user updates to run hello world | expand

Commit Message

Warner Losh Aug. 7, 2021, 9:42 p.m. UTC 
From: Warner Losh <imp@FreeBSD.org>

Update target_arch_elf.h to remove thread_init. Move its contents to
target_arch_thread.h and rename to target_thread_init(). Update
elfload.c to call it. Create thread_os_thread.h to hold the os specific
parts of the thread and threat manipulation routines. Currently, it just
includes target_arch_thread.h. target_arch_thread.h contains the at the
moment unused target_thread_set_upcall which will be used in the future
when creating actual thread (i386 has this stubbed, but other
architectures in the bsd-user tree have real ones).

These changes are all interrelated and could be brokend own, but seem to
represent a reviewable changeset since most of the change is boiler
plate.

Signed-off-by: Stacey Son <sson@FreeBSD.org>
Signed-off-by: Warner Losh <imp@bsdimp.com>
---
 bsd-user/elfload.c                   |  4 ++-
 bsd-user/freebsd/target_os_thread.h  | 25 +++++++++++++
 bsd-user/i386/target_arch_elf.h      | 52 ++--------------------------
 bsd-user/i386/target_arch_thread.h   | 47 +++++++++++++++++++++++++
 bsd-user/netbsd/target_os_thread.h   | 25 +++++++++++++
 bsd-user/openbsd/target_os_thread.h  | 25 +++++++++++++
 bsd-user/x86_64/target_arch_elf.h    | 38 ++------------------
 bsd-user/x86_64/target_arch_thread.h | 40 +++++++++++++++++++++
 8 files changed, 169 insertions(+), 87 deletions(-)
 create mode 100644 bsd-user/freebsd/target_os_thread.h
 create mode 100644 bsd-user/i386/target_arch_thread.h
 create mode 100644 bsd-user/netbsd/target_os_thread.h
 create mode 100644 bsd-user/openbsd/target_os_thread.h
 create mode 100644 bsd-user/x86_64/target_arch_thread.h

Comments

Richard Henderson Aug. 8, 2021, 6:24 a.m. UTC | #1 
On 8/7/21 11:42 AM, Warner Losh wrote:
> +++ b/bsd-user/x86_64/target_arch_elf.h
> @@ -19,48 +19,14 @@
>   #ifndef_TARGET_ARCH_ELF_H_
>   #define_TARGET_ARCH_ELF_H_
>   
> -#define ELF_PLATFORM get_elf_platform()
> -
> -static const char *get_elf_platform(void)
> -{
> -    static char elf_platform[] = "i386";
> -    int family = object_property_get_int(OBJECT(thread_cpu), "family", NULL);
> -    if (family > 6) {
> -        family = 6;
> -    }
> -    if (family >= 3) {
> -        elf_platform[1] = '0' + family;
> -    }
> -    return elf_platform;
> -}
> -
> -#define ELF_HWCAP get_elf_hwcap()
> -
> -static uint32_t get_elf_hwcap(void)
> -{
> -    X86CPU *cpu = X86_CPU(thread_cpu);
> -
> -    return cpu->env.features[FEAT_1_EDX];
> -}
> -
>   #define ELF_START_MMAP 0x2aaaaab000ULL
> -#define elf_check_arch(x) (((x) == ELF_ARCH))
> +#define ELF_ET_DYN_LOAD_ADDR    0x01021000
> +#define elf_check_arch(x) ( ((x) == ELF_ARCH) )

This appears to be unrelated?  Should this have been in a previous patch?  Or is this a 
rebase mistake that gets corrected later?

I'll trust the target_os_thread.h thing becomes useful later.
So, modulo the target_arch_elf.h weirdness,

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~
Warner Losh Aug. 8, 2021, 9:43 p.m. UTC | #2 
On Sun, Aug 8, 2021 at 12:24 AM Richard Henderson <
richard.henderson@linaro.org> wrote:

> On 8/7/21 11:42 AM, Warner Losh wrote:
> > +++ b/bsd-user/x86_64/target_arch_elf.h
> > @@ -19,48 +19,14 @@
> >   #ifndef_TARGET_ARCH_ELF_H_
> >   #define_TARGET_ARCH_ELF_H_
> >
> > -#define ELF_PLATFORM get_elf_platform()
> > -
> > -static const char *get_elf_platform(void)
> > -{
> > -    static char elf_platform[] = "i386";
> > -    int family = object_property_get_int(OBJECT(thread_cpu), "family",
> NULL);
> > -    if (family > 6) {
> > -        family = 6;
> > -    }
> > -    if (family >= 3) {
> > -        elf_platform[1] = '0' + family;
> > -    }
> > -    return elf_platform;
> > -}
> > -
> > -#define ELF_HWCAP get_elf_hwcap()
> > -
> > -static uint32_t get_elf_hwcap(void)
> > -{
> > -    X86CPU *cpu = X86_CPU(thread_cpu);
> > -
> > -    return cpu->env.features[FEAT_1_EDX];
> > -}
> > -
> >   #define ELF_START_MMAP 0x2aaaaab000ULL
> > -#define elf_check_arch(x) (((x) == ELF_ARCH))
> > +#define ELF_ET_DYN_LOAD_ADDR    0x01021000
> > +#define elf_check_arch(x) ( ((x) == ELF_ARCH) )
>
> This appears to be unrelated?  Should this have been in a previous patch?
> Or is this a
> rebase mistake that gets corrected later?
>

This turns out to be a very good question. It's exactly what I intended to
do. It's a
copy of the latest development branch. However, digging into it shows that
it is
none-the-less incorrect. It wasn't a mismerge, but it was code commented
out 8
years ago which I removed in the merge. I had thought this meant it was
unused.
In fact, it had been for some time. However, it's been wrong all that time
and that
code should be restored not only here, but for other architectures I've not
submitted.
It was unused, but not unneeded...


> I'll trust the target_os_thread.h thing becomes useful later.
> So, modulo the target_arch_elf.h weirdness,
>

Yes. It works well enough on x86, but since x86 isn't used by our project
for more
than a basic sanity check, this breakage went unnoticed for some time...

I'll update with the next round. BTW, I've started to notice that many of
the
items flagged by the style commitcheck.pl script originated in the
linux-user
code and it's still that way today...  Do you have any advice for what I
should
do about that, if anything?

Warner


> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>
> r~
>
Warner Losh Aug. 8, 2021, 10:56 p.m. UTC | #3 
On Sun, Aug 8, 2021 at 3:43 PM Warner Losh <imp@bsdimp.com> wrote:

>
>
> On Sun, Aug 8, 2021 at 12:24 AM Richard Henderson <
> richard.henderson@linaro.org> wrote:
>
>> On 8/7/21 11:42 AM, Warner Losh wrote:
>> > +++ b/bsd-user/x86_64/target_arch_elf.h
>> > @@ -19,48 +19,14 @@
>> >   #ifndef_TARGET_ARCH_ELF_H_
>> >   #define_TARGET_ARCH_ELF_H_
>> >
>> > -#define ELF_PLATFORM get_elf_platform()
>> > -
>> > -static const char *get_elf_platform(void)
>> > -{
>> > -    static char elf_platform[] = "i386";
>> > -    int family = object_property_get_int(OBJECT(thread_cpu), "family",
>> NULL);
>> > -    if (family > 6) {
>> > -        family = 6;
>> > -    }
>> > -    if (family >= 3) {
>> > -        elf_platform[1] = '0' + family;
>> > -    }
>> > -    return elf_platform;
>> > -}
>> > -
>> > -#define ELF_HWCAP get_elf_hwcap()
>> > -
>> > -static uint32_t get_elf_hwcap(void)
>> > -{
>> > -    X86CPU *cpu = X86_CPU(thread_cpu);
>> > -
>> > -    return cpu->env.features[FEAT_1_EDX];
>> > -}
>> > -
>> >   #define ELF_START_MMAP 0x2aaaaab000ULL
>> > -#define elf_check_arch(x) (((x) == ELF_ARCH))
>> > +#define ELF_ET_DYN_LOAD_ADDR    0x01021000
>> > +#define elf_check_arch(x) ( ((x) == ELF_ARCH) )
>>
>> This appears to be unrelated?  Should this have been in a previous
>> patch?  Or is this a
>> rebase mistake that gets corrected later?
>>
>
> This turns out to be a very good question. It's exactly what I intended to
> do. It's a
> copy of the latest development branch. However, digging into it shows that
> it is
> none-the-less incorrect. It wasn't a mismerge, but it was code commented
> out 8
> years ago which I removed in the merge. I had thought this meant it was
> unused.
> In fact, it had been for some time. However, it's been wrong all that time
> and that
> code should be restored not only here, but for other architectures I've
> not submitted.
> It was unused, but not unneeded...
>

Ah, it turns out this showed problems only in arm* and riscv on FreeBSD.
The others
publish HWCAP = 0 for some reason. And ELF_PLATFORM() is used in the
most-recent
bsd-user code. Some tweaks are needed, and I'll do those in V2 with the
other style
fixes.

Warner


> I'll trust the target_os_thread.h thing becomes useful later.
>> So, modulo the target_arch_elf.h weirdness,
>>
>
> Yes. It works well enough on x86, but since x86 isn't used by our project
> for more
> than a basic sanity check, this breakage went unnoticed for some time...
>
> I'll update with the next round. BTW, I've started to notice that many of
> the
> items flagged by the style commitcheck.pl script originated in the
> linux-user
> code and it's still that way today...  Do you have any advice for what I
> should
> do about that, if anything?
>
> Warner
>
>
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>>
>> r~
>>
>
Richard Henderson Aug. 9, 2021, 5:53 p.m. UTC | #4 
On 8/8/21 11:43 AM, Warner Losh wrote:
> BTW, I've started to notice that many of the
> items flagged by the style commitcheck.pl <http://commitcheck.pl> script originated in the 
> linux-user
> code and it's still that way today...  Do you have any advice for what I should
> do about that, if anything?

Fix em on the bsd side.

We have so far resisted style fixes for their own sake, only fixing them up locally when 
we have to make nearby changes for some other reason.


r~
diff mbox series

Patch

diff --git a/bsd-user/elfload.c b/bsd-user/elfload.c
index 8a6a72bf05..70a0f81f3d 100644
--- a/bsd-user/elfload.c
+++ b/bsd-user/elfload.c
@@ -24,6 +24,7 @@ 
 #include "qemu/path.h"
 
 #include "target_arch_elf.h"
+#include "target_os_thread.h"
 
 /* this flag is uneffective under linux too, should be deleted */
 #ifndef MAP_DENYWRITE
@@ -1001,5 +1002,6 @@  int load_elf_binary(struct bsd_binprm *bprm, struct target_pt_regs *regs,
 
 void do_init_thread(struct target_pt_regs *regs, struct image_info *infop)
 {
-    init_thread(regs, infop);
+
+    target_thread_init(regs, infop);
 }
diff --git a/bsd-user/freebsd/target_os_thread.h b/bsd-user/freebsd/target_os_thread.h
new file mode 100644
index 0000000000..77433acdff
--- /dev/null
+++ b/bsd-user/freebsd/target_os_thread.h
@@ -0,0 +1,25 @@ 
+/*
+ *  FreeBSD thread dependent code and definitions
+ *
+ *  Copyright (c) 2013 Stacey D. Son
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef _TARGET_OS_THREAD_H_
+#define _TARGET_OS_THREAD_H_
+
+#include "target_arch_thread.h"
+
+#endif /* !_TARGET_OS_THREAD_H_ */
diff --git a/bsd-user/i386/target_arch_elf.h b/bsd-user/i386/target_arch_elf.h
index f322fc174b..7a9744140f 100644
--- a/bsd-user/i386/target_arch_elf.h
+++ b/bsd-user/i386/target_arch_elf.h
@@ -19,62 +19,14 @@ 
 #ifndef _TARGET_ARCH_ELF_H_
 #define _TARGET_ARCH_ELF_H_
 
-#define ELF_PLATFORM get_elf_platform()
-
-static const char *get_elf_platform(void)
-{
-    static char elf_platform[] = "i386";
-    int family = object_property_get_int(OBJECT(thread_cpu), "family", NULL);
-    if (family > 6) {
-        family = 6;
-    }
-    if (family >= 3) {
-        elf_platform[1] = '0' + family;
-    }
-    return elf_platform;
-}
-
-#define ELF_HWCAP get_elf_hwcap()
-
-static uint32_t get_elf_hwcap(void)
-{
-    X86CPU *cpu = X86_CPU(thread_cpu);
-
-    return cpu->env.features[FEAT_1_EDX];
-}
-
 #define ELF_START_MMAP 0x80000000
+#define ELF_ET_DYN_LOAD_ADDR    0x01001000
+#define elf_check_arch(x) ( ((x) == EM_386) || ((x) == EM_486) )
 
-/*
- * This is used to ensure we don't load something for the wrong architecture.
- */
-#define elf_check_arch(x) (((x) == EM_386) || ((x) == EM_486))
-
-/*
- * These are used to set parameters in the core dumps.
- */
 #define ELF_CLASS       ELFCLASS32
 #define ELF_DATA        ELFDATA2LSB
 #define ELF_ARCH        EM_386
 
-static inline void init_thread(struct target_pt_regs *regs,
-                               struct image_info *infop)
-{
-    regs->esp = infop->start_stack;
-    regs->eip = infop->entry;
-
-    /*
-     * SVR4/i386 ABI (pages 3-31, 3-32) says that when the program starts %edx
-     * contains a pointer to a function which might be registered using
-     * `atexit'.  This provides a mean for the dynamic linker to call DT_FINI
-     * functions for shared libraries that have been loaded before the code
-     * runs.
-     *
-     * A value of 0 tells we have no such handler.
-     */
-    regs->edx = 0;
-}
-
 #define USE_ELF_CORE_DUMP
 #define ELF_EXEC_PAGESIZE       4096
 
diff --git a/bsd-user/i386/target_arch_thread.h b/bsd-user/i386/target_arch_thread.h
new file mode 100644
index 0000000000..e65e476f75
--- /dev/null
+++ b/bsd-user/i386/target_arch_thread.h
@@ -0,0 +1,47 @@ 
+/*
+ *  i386 thread support
+ *
+ *  Copyright (c) 2013 Stacey D. Son
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+#ifndef _TARGET_ARCH_THREAD_H_
+#define _TARGET_ARCH_THREAD_H_
+
+/* Compare to vm_machdep.c cpu_set_upcall_kse() */
+static inline void target_thread_set_upcall(CPUX86State *regs, abi_ulong entry,
+    abi_ulong arg, abi_ulong stack_base, abi_ulong stack_size)
+{
+    /* XXX */
+}
+
+static inline void target_thread_init(struct target_pt_regs *regs,
+        struct image_info *infop)
+{
+    regs->esp = infop->start_stack;
+    regs->eip = infop->entry;
+
+    /*
+     * SVR4/i386 ABI (pages 3-31, 3-32) says that when the program starts %edx
+     * contains a pointer to a function which might be registered using
+     * `atexit'.  This provides a mean for the dynamic linker to call DT_FINI
+     * functions for shared libraries that have been loaded before the code
+     * runs.
+     *
+     * A value of 0 tells we have no such handler.
+     */
+    regs->edx = 0;
+}
+
+#endif /* !_TARGET_ARCH_THREAD_H_ */
diff --git a/bsd-user/netbsd/target_os_thread.h b/bsd-user/netbsd/target_os_thread.h
new file mode 100644
index 0000000000..904dd1bf78
--- /dev/null
+++ b/bsd-user/netbsd/target_os_thread.h
@@ -0,0 +1,25 @@ 
+/*
+ *  NetBSD thread dependent code and definitions
+ *
+ *  Copyright (c) 2013 Stacey D. Son
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef _TARGET_OS_THREAD_H_
+#define _TARGET_OS_THREAD_H_
+
+#include "target_arch_thread.h"
+
+#endif /* !_TARGET_OS_THREAD_H_ */
diff --git a/bsd-user/openbsd/target_os_thread.h b/bsd-user/openbsd/target_os_thread.h
new file mode 100644
index 0000000000..01ed0d9fc8
--- /dev/null
+++ b/bsd-user/openbsd/target_os_thread.h
@@ -0,0 +1,25 @@ 
+/*
+ *  OpenBSD thread dependent code and definitions
+ *
+ *  Copyright (c) 2013 Stacey D. Son
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef _TARGET_OS_THREAD_H_
+#define _TARGET_OS_THREAD_H_
+
+#include "target_arch_thread.h"
+
+#endif /* !_TARGET_OS_THREAD_H_ */
diff --git a/bsd-user/x86_64/target_arch_elf.h b/bsd-user/x86_64/target_arch_elf.h
index e7c8aa2755..180ec77a5f 100644
--- a/bsd-user/x86_64/target_arch_elf.h
+++ b/bsd-user/x86_64/target_arch_elf.h
@@ -19,48 +19,14 @@ 
 #ifndef _TARGET_ARCH_ELF_H_
 #define _TARGET_ARCH_ELF_H_
 
-#define ELF_PLATFORM get_elf_platform()
-
-static const char *get_elf_platform(void)
-{
-    static char elf_platform[] = "i386";
-    int family = object_property_get_int(OBJECT(thread_cpu), "family", NULL);
-    if (family > 6) {
-        family = 6;
-    }
-    if (family >= 3) {
-        elf_platform[1] = '0' + family;
-    }
-    return elf_platform;
-}
-
-#define ELF_HWCAP get_elf_hwcap()
-
-static uint32_t get_elf_hwcap(void)
-{
-    X86CPU *cpu = X86_CPU(thread_cpu);
-
-    return cpu->env.features[FEAT_1_EDX];
-}
-
 #define ELF_START_MMAP 0x2aaaaab000ULL
-#define elf_check_arch(x) (((x) == ELF_ARCH))
+#define ELF_ET_DYN_LOAD_ADDR    0x01021000
+#define elf_check_arch(x) ( ((x) == ELF_ARCH) )
 
 #define ELF_CLASS      ELFCLASS64
 #define ELF_DATA       ELFDATA2LSB
 #define ELF_ARCH       EM_X86_64
 
-static inline void init_thread(struct target_pt_regs *regs,
-                               struct image_info *infop)
-{
-    regs->rax = 0;
-    regs->rsp = infop->start_stack;
-    regs->rip = infop->entry;
-    if (bsd_type == target_freebsd) {
-        regs->rdi = infop->start_stack;
-    }
-}
-
 #define USE_ELF_CORE_DUMP
 #define ELF_EXEC_PAGESIZE       4096
 
diff --git a/bsd-user/x86_64/target_arch_thread.h b/bsd-user/x86_64/target_arch_thread.h
new file mode 100644
index 0000000000..d105e43fd3
--- /dev/null
+++ b/bsd-user/x86_64/target_arch_thread.h
@@ -0,0 +1,40 @@ 
+/*
+ *  x86_64 thread support
+ *
+ *  Copyright (c) 2013 Stacey D. Son
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+#ifndef _TARGET_ARCH_THREAD_H_
+#define _TARGET_ARCH_THREAD_H_
+
+/* Compare to vm_machdep.c cpu_set_upcall_kse() */
+static inline void target_thread_set_upcall(CPUX86State *regs, abi_ulong entry,
+    abi_ulong arg, abi_ulong stack_base, abi_ulong stack_size)
+{
+    /* XXX */
+}
+
+static inline void target_thread_init(struct target_pt_regs *regs,
+    struct image_info *infop)
+{
+    regs->rax = 0;
+    regs->rsp = infop->start_stack;
+    regs->rip = infop->entry;
+    if (bsd_type == target_freebsd) {
+        regs->rdi = infop->start_stack;
+    }
+}
+
+#endif /* !_TARGET_ARCH_THREAD_H_ */