* [PATCH v3 1/8] linux-user: Expose do_guest_openat() and do_guest_readlink()
2023-06-06 13:27 [PATCH v3 0/8] gdbstub: Add support for info proc mappings Ilya Leoshkevich
@ 2023-06-06 13:27 ` Ilya Leoshkevich
2023-06-06 16:33 ` Richard Henderson
2023-06-06 13:27 ` [PATCH v3 2/8] linux-user: Add "safe" parameter to do_guest_openat() Ilya Leoshkevich
` (6 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: Ilya Leoshkevich @ 2023-06-06 13:27 UTC (permalink / raw)
To: Alex Bennée, Laurent Vivier, Peter Maydell,
Richard Henderson, David Hildenbrand
Cc: Philippe Mathieu-Daudé, qemu-devel, qemu-arm, qemu-s390x,
Ilya Leoshkevich
These functions will be required by the GDB stub in order to provide
the guest view of /proc to GDB.
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
linux-user/qemu.h | 3 +++
linux-user/syscall.c | 54 ++++++++++++++++++++++++++++----------------
2 files changed, 38 insertions(+), 19 deletions(-)
diff --git a/linux-user/qemu.h b/linux-user/qemu.h
index 92f9f5af41c..a5830ec2396 100644
--- a/linux-user/qemu.h
+++ b/linux-user/qemu.h
@@ -165,6 +165,9 @@ typedef struct TaskState {
} TaskState;
abi_long do_brk(abi_ulong new_brk);
+int do_guest_openat(CPUArchState *cpu_env, int dirfd, const char *pathname,
+ int flags, mode_t mode);
+ssize_t do_guest_readlink(const char *pathname, char *buf, size_t bufsiz);
/* user access */
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 83685f0aa59..2d3070cfd62 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -8448,7 +8448,8 @@ static int open_hardware(CPUArchState *cpu_env, int fd)
}
#endif
-static int do_openat(CPUArchState *cpu_env, int dirfd, const char *pathname, int flags, mode_t mode)
+int do_guest_openat(CPUArchState *cpu_env, int dirfd, const char *pathname,
+ int flags, mode_t mode)
{
struct fake_open {
const char *filename;
@@ -8520,6 +8521,36 @@ static int do_openat(CPUArchState *cpu_env, int dirfd, const char *pathname, int
return safe_openat(dirfd, path(pathname), flags, mode);
}
+ssize_t do_guest_readlink(const char *pathname, char *buf, size_t bufsiz)
+{
+ ssize_t ret;
+
+ if (!pathname || !buf) {
+ errno = EFAULT;
+ return -1;
+ }
+
+ if (!bufsiz) {
+ /* Short circuit this for the magic exe check. */
+ errno = EINVAL;
+ return -1;
+ }
+
+ if (is_proc_myself((const char *)pathname, "exe")) {
+ /*
+ * Don't worry about sign mismatch as earlier mapping
+ * logic would have thrown a bad address error.
+ */
+ ret = MIN(strlen(exec_path), bufsiz);
+ /* We cannot NUL terminate the string. */
+ memcpy(buf, exec_path, ret);
+ } else {
+ ret = readlink(path(pathname), buf, bufsiz);
+ }
+
+ return ret;
+}
+
static int do_execveat(CPUArchState *cpu_env, int dirfd,
abi_long pathname, abi_long guest_argp,
abi_long guest_envp, int flags)
@@ -8994,7 +9025,7 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1,
case TARGET_NR_open:
if (!(p = lock_user_string(arg1)))
return -TARGET_EFAULT;
- ret = get_errno(do_openat(cpu_env, AT_FDCWD, p,
+ ret = get_errno(do_guest_openat(cpu_env, AT_FDCWD, p,
target_to_host_bitmask(arg2, fcntl_flags_tbl),
arg3));
fd_trans_unregister(ret);
@@ -9004,7 +9035,7 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1,
case TARGET_NR_openat:
if (!(p = lock_user_string(arg2)))
return -TARGET_EFAULT;
- ret = get_errno(do_openat(cpu_env, arg1, p,
+ ret = get_errno(do_guest_openat(cpu_env, arg1, p,
target_to_host_bitmask(arg3, fcntl_flags_tbl),
arg4));
fd_trans_unregister(ret);
@@ -10229,22 +10260,7 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1,
void *p2;
p = lock_user_string(arg1);
p2 = lock_user(VERIFY_WRITE, arg2, arg3, 0);
- if (!p || !p2) {
- ret = -TARGET_EFAULT;
- } else if (!arg3) {
- /* Short circuit this for the magic exe check. */
- ret = -TARGET_EINVAL;
- } else if (is_proc_myself((const char *)p, "exe")) {
- /*
- * Don't worry about sign mismatch as earlier mapping
- * logic would have thrown a bad address error.
- */
- ret = MIN(strlen(exec_path), arg3);
- /* We cannot NUL terminate the string. */
- memcpy(p2, exec_path, ret);
- } else {
- ret = get_errno(readlink(path(p), p2, arg3));
- }
+ ret = get_errno(do_guest_readlink(p, p2, arg3));
unlock_user(p2, arg2, ret);
unlock_user(p, arg1, 0);
}
--
2.40.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v3 1/8] linux-user: Expose do_guest_openat() and do_guest_readlink()
2023-06-06 13:27 ` [PATCH v3 1/8] linux-user: Expose do_guest_openat() and do_guest_readlink() Ilya Leoshkevich
@ 2023-06-06 16:33 ` Richard Henderson
0 siblings, 0 replies; 18+ messages in thread
From: Richard Henderson @ 2023-06-06 16:33 UTC (permalink / raw)
To: Ilya Leoshkevich, Alex Bennée, Laurent Vivier, Peter Maydell,
David Hildenbrand
Cc: Philippe Mathieu-Daudé, qemu-devel, qemu-arm, qemu-s390x
On 6/6/23 06:27, Ilya Leoshkevich wrote:
> These functions will be required by the GDB stub in order to provide
> the guest view of /proc to GDB.
>
> Reviewed-by: Alex Bennée<alex.bennee@linaro.org>
> Signed-off-by: Ilya Leoshkevich<iii@linux.ibm.com>
> ---
> linux-user/qemu.h | 3 +++
> linux-user/syscall.c | 54 ++++++++++++++++++++++++++++----------------
> 2 files changed, 38 insertions(+), 19 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v3 2/8] linux-user: Add "safe" parameter to do_guest_openat()
2023-06-06 13:27 [PATCH v3 0/8] gdbstub: Add support for info proc mappings Ilya Leoshkevich
2023-06-06 13:27 ` [PATCH v3 1/8] linux-user: Expose do_guest_openat() and do_guest_readlink() Ilya Leoshkevich
@ 2023-06-06 13:27 ` Ilya Leoshkevich
2023-06-06 18:24 ` Richard Henderson
2023-06-06 13:27 ` [PATCH v3 3/8] linux-user: Emulate /proc/self/smaps Ilya Leoshkevich
` (5 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: Ilya Leoshkevich @ 2023-06-06 13:27 UTC (permalink / raw)
To: Alex Bennée, Laurent Vivier, Peter Maydell,
Richard Henderson, David Hildenbrand
Cc: Philippe Mathieu-Daudé, qemu-devel, qemu-arm, qemu-s390x,
Ilya Leoshkevich
gdbstub cannot meaningfully handle QEMU_ERESTARTSYS, and it doesn't
need to. Add a parameter to do_guest_openat() that makes it use
openat() instead of safe_openat(), so that it becomes usable from
gdbstub.
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
linux-user/qemu.h | 2 +-
linux-user/syscall.c | 18 +++++++++++++-----
2 files changed, 14 insertions(+), 6 deletions(-)
diff --git a/linux-user/qemu.h b/linux-user/qemu.h
index a5830ec2396..9b8e0860d70 100644
--- a/linux-user/qemu.h
+++ b/linux-user/qemu.h
@@ -166,7 +166,7 @@ typedef struct TaskState {
abi_long do_brk(abi_ulong new_brk);
int do_guest_openat(CPUArchState *cpu_env, int dirfd, const char *pathname,
- int flags, mode_t mode);
+ int flags, mode_t mode, bool safe);
ssize_t do_guest_readlink(const char *pathname, char *buf, size_t bufsiz);
/* user access */
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 2d3070cfd62..28a0b1f7882 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -8449,7 +8449,7 @@ static int open_hardware(CPUArchState *cpu_env, int fd)
#endif
int do_guest_openat(CPUArchState *cpu_env, int dirfd, const char *pathname,
- int flags, mode_t mode)
+ int flags, mode_t mode, bool safe)
{
struct fake_open {
const char *filename;
@@ -8476,7 +8476,11 @@ int do_guest_openat(CPUArchState *cpu_env, int dirfd, const char *pathname,
};
if (is_proc_myself(pathname, "exe")) {
- return safe_openat(dirfd, exec_path, flags, mode);
+ if (safe) {
+ return safe_openat(dirfd, exec_path, flags, mode);
+ } else {
+ return openat(dirfd, exec_path, flags, mode);
+ }
}
for (fake_open = fakes; fake_open->filename; fake_open++) {
@@ -8518,7 +8522,11 @@ int do_guest_openat(CPUArchState *cpu_env, int dirfd, const char *pathname,
return fd;
}
- return safe_openat(dirfd, path(pathname), flags, mode);
+ if (safe) {
+ return safe_openat(dirfd, path(pathname), flags, mode);
+ } else {
+ return openat(dirfd, path(pathname), flags, mode);
+ }
}
ssize_t do_guest_readlink(const char *pathname, char *buf, size_t bufsiz)
@@ -9027,7 +9035,7 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1,
return -TARGET_EFAULT;
ret = get_errno(do_guest_openat(cpu_env, AT_FDCWD, p,
target_to_host_bitmask(arg2, fcntl_flags_tbl),
- arg3));
+ arg3, true));
fd_trans_unregister(ret);
unlock_user(p, arg1, 0);
return ret;
@@ -9037,7 +9045,7 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1,
return -TARGET_EFAULT;
ret = get_errno(do_guest_openat(cpu_env, arg1, p,
target_to_host_bitmask(arg3, fcntl_flags_tbl),
- arg4));
+ arg4, true));
fd_trans_unregister(ret);
unlock_user(p, arg2, 0);
return ret;
--
2.40.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v3 2/8] linux-user: Add "safe" parameter to do_guest_openat()
2023-06-06 13:27 ` [PATCH v3 2/8] linux-user: Add "safe" parameter to do_guest_openat() Ilya Leoshkevich
@ 2023-06-06 18:24 ` Richard Henderson
2023-06-06 19:29 ` Ilya Leoshkevich
0 siblings, 1 reply; 18+ messages in thread
From: Richard Henderson @ 2023-06-06 18:24 UTC (permalink / raw)
To: Ilya Leoshkevich, Alex Bennée, Laurent Vivier, Peter Maydell,
David Hildenbrand
Cc: Philippe Mathieu-Daudé, qemu-devel, qemu-arm, qemu-s390x
On 6/6/23 06:27, Ilya Leoshkevich wrote:
> @@ -8518,7 +8522,11 @@ int do_guest_openat(CPUArchState *cpu_env, int dirfd, const char *pathname,
> return fd;
> }
>
> - return safe_openat(dirfd, path(pathname), flags, mode);
> + if (safe) {
> + return safe_openat(dirfd, path(pathname), flags, mode);
> + } else {
> + return openat(dirfd, path(pathname), flags, mode);
> + }
> }
I'm not keen on this, as it seems like the wrong abstraction. But I can't immediately
think of how it could be better structured.
The only concrete objection I have is the change of API, which could be fixed with return
get_errno(openat(...)).
With that,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v3 2/8] linux-user: Add "safe" parameter to do_guest_openat()
2023-06-06 18:24 ` Richard Henderson
@ 2023-06-06 19:29 ` Ilya Leoshkevich
2023-06-06 20:35 ` Richard Henderson
0 siblings, 1 reply; 18+ messages in thread
From: Ilya Leoshkevich @ 2023-06-06 19:29 UTC (permalink / raw)
To: Richard Henderson, Alex Bennée, Laurent Vivier,
Peter Maydell, David Hildenbrand
Cc: Philippe Mathieu-Daudé, qemu-devel, qemu-arm, qemu-s390x
On Tue, 2023-06-06 at 11:24 -0700, Richard Henderson wrote:
> On 6/6/23 06:27, Ilya Leoshkevich wrote:
> > @@ -8518,7 +8522,11 @@ int do_guest_openat(CPUArchState *cpu_env,
> > int dirfd, const char *pathname,
> > return fd;
> > }
> >
> > - return safe_openat(dirfd, path(pathname), flags, mode);
> > + if (safe) {
> > + return safe_openat(dirfd, path(pathname), flags, mode);
> > + } else {
> > + return openat(dirfd, path(pathname), flags, mode);
> > + }
> > }
>
> I'm not keen on this, as it seems like the wrong abstraction. But I
> can't immediately
> think of how it could be better structured.
I also thought about temporarily clearing signal_pending in gdbstub,
but could not convince myself that this were safe.
> The only concrete objection I have is the change of API, which could
> be fixed with return
> get_errno(openat(...)).
I believe both openat() and safe_openat() return -1 on error and set
errno, or am I missing something?
>
> With that,
>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>
>
> r~
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v3 2/8] linux-user: Add "safe" parameter to do_guest_openat()
2023-06-06 19:29 ` Ilya Leoshkevich
@ 2023-06-06 20:35 ` Richard Henderson
0 siblings, 0 replies; 18+ messages in thread
From: Richard Henderson @ 2023-06-06 20:35 UTC (permalink / raw)
To: Ilya Leoshkevich, Alex Bennée, Laurent Vivier, Peter Maydell,
David Hildenbrand
Cc: Philippe Mathieu-Daudé, qemu-devel, qemu-arm, qemu-s390x
On 6/6/23 12:29, Ilya Leoshkevich wrote:
>> The only concrete objection I have is the change of API, which could
>> be fixed with return
>> get_errno(openat(...)).
>
> I believe both openat() and safe_openat() return -1 on error and set
> errno, or am I missing something?
Oops, no, bad memory on my part -- I thought safe_foo returned -errno.
r~
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v3 3/8] linux-user: Emulate /proc/self/smaps
2023-06-06 13:27 [PATCH v3 0/8] gdbstub: Add support for info proc mappings Ilya Leoshkevich
2023-06-06 13:27 ` [PATCH v3 1/8] linux-user: Expose do_guest_openat() and do_guest_readlink() Ilya Leoshkevich
2023-06-06 13:27 ` [PATCH v3 2/8] linux-user: Add "safe" parameter to do_guest_openat() Ilya Leoshkevich
@ 2023-06-06 13:27 ` Ilya Leoshkevich
2023-06-06 18:00 ` Richard Henderson
2023-06-06 13:27 ` [PATCH v3 4/8] gdbstub: Expose gdb_get_process() and gdb_get_first_cpu_in_process() Ilya Leoshkevich
` (4 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: Ilya Leoshkevich @ 2023-06-06 13:27 UTC (permalink / raw)
To: Alex Bennée, Laurent Vivier, Peter Maydell,
Richard Henderson, David Hildenbrand
Cc: Philippe Mathieu-Daudé, qemu-devel, qemu-arm, qemu-s390x,
Ilya Leoshkevich
/proc/self/smaps is an extension of /proc/self/maps: it provides the
same lines, plus additional information about each range.
GDB uses /proc/self/smaps when available, which means that
generate-core-file tries it first before falling back to
/proc/self/maps. This, in turn, causes it to dump the host mappings,
since /proc/self/smaps is not emulated and is just passed through.
Fix by emulating /proc/self/smaps. Provide true values only for
Size, KernelPageSize, MMUPageSize and VmFlags. Leave all other values
at 0, which is a valid conservative estimate.
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
linux-user/syscall.c | 58 +++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 57 insertions(+), 1 deletion(-)
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 28a0b1f7882..c1045ea7925 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -8042,7 +8042,36 @@ static int open_self_cmdline(CPUArchState *cpu_env, int fd)
return 0;
}
-static int open_self_maps(CPUArchState *cpu_env, int fd)
+static void show_smaps(int fd, unsigned long size)
+{
+ unsigned long page_size_kb = TARGET_PAGE_SIZE >> 10;
+ unsigned long size_kb = size >> 10;
+
+ dprintf(fd, "Size: %lu kB\n"
+ "KernelPageSize: %lu kB\n"
+ "MMUPageSize: %lu kB\n"
+ "Rss: 0 kB\n"
+ "Pss: 0 kB\n"
+ "Pss_Dirty: 0 kB\n"
+ "Shared_Clean: 0 kB\n"
+ "Shared_Dirty: 0 kB\n"
+ "Private_Clean: 0 kB\n"
+ "Private_Dirty: 0 kB\n"
+ "Referenced: 0 kB\n"
+ "Anonymous: 0 kB\n"
+ "LazyFree: 0 kB\n"
+ "AnonHugePages: 0 kB\n"
+ "ShmemPmdMapped: 0 kB\n"
+ "FilePmdMapped: 0 kB\n"
+ "Shared_Hugetlb: 0 kB\n"
+ "Private_Hugetlb: 0 kB\n"
+ "Swap: 0 kB\n"
+ "SwapPss: 0 kB\n"
+ "Locked: 0 kB\n"
+ "THPeligible: 0\n", size_kb, page_size_kb, page_size_kb);
+}
+
+static int open_self_maps_1(CPUArchState *cpu_env, int fd, bool smaps)
{
CPUState *cpu = env_cpu(cpu_env);
TaskState *ts = cpu->opaque;
@@ -8089,6 +8118,18 @@ static int open_self_maps(CPUArchState *cpu_env, int fd)
} else {
dprintf(fd, "\n");
}
+ if (smaps) {
+ show_smaps(fd, max - min);
+ dprintf(fd, "VmFlags:%s%s%s%s%s%s%s%s\n",
+ (flags & PAGE_READ) ? " rd" : "",
+ (flags & PAGE_WRITE_ORG) ? " wr" : "",
+ (flags & PAGE_EXEC) ? " ex" : "",
+ e->is_priv ? "" : " sh",
+ (flags & PAGE_READ) ? " mr" : "",
+ (flags & PAGE_WRITE_ORG) ? " mw" : "",
+ (flags & PAGE_EXEC) ? " me" : "",
+ e->is_priv ? "" : " ms");
+ }
}
}
@@ -8103,11 +8144,25 @@ static int open_self_maps(CPUArchState *cpu_env, int fd)
" --xp 00000000 00:00 0",
TARGET_VSYSCALL_PAGE, TARGET_VSYSCALL_PAGE + TARGET_PAGE_SIZE);
dprintf(fd, "%*s%s\n", 73 - count, "", "[vsyscall]");
+ if (smaps) {
+ show_smaps(fd, TARGET_PAGE_SIZE);
+ dprintf(fd, "VmFlags: ex\n");
+ }
#endif
return 0;
}
+static int open_self_maps(CPUArchState *cpu_env, int fd)
+{
+ return open_self_maps_1(cpu_env, fd, false);
+}
+
+static int open_self_smaps(CPUArchState *cpu_env, int fd)
+{
+ return open_self_maps_1(cpu_env, fd, true);
+}
+
static int open_self_stat(CPUArchState *cpu_env, int fd)
{
CPUState *cpu = env_cpu(cpu_env);
@@ -8459,6 +8514,7 @@ int do_guest_openat(CPUArchState *cpu_env, int dirfd, const char *pathname,
const struct fake_open *fake_open;
static const struct fake_open fakes[] = {
{ "maps", open_self_maps, is_proc_myself },
+ { "smaps", open_self_smaps, is_proc_myself },
{ "stat", open_self_stat, is_proc_myself },
{ "auxv", open_self_auxv, is_proc_myself },
{ "cmdline", open_self_cmdline, is_proc_myself },
--
2.40.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v3 3/8] linux-user: Emulate /proc/self/smaps
2023-06-06 13:27 ` [PATCH v3 3/8] linux-user: Emulate /proc/self/smaps Ilya Leoshkevich
@ 2023-06-06 18:00 ` Richard Henderson
0 siblings, 0 replies; 18+ messages in thread
From: Richard Henderson @ 2023-06-06 18:00 UTC (permalink / raw)
To: Ilya Leoshkevich, Alex Bennée, Laurent Vivier, Peter Maydell,
David Hildenbrand
Cc: Philippe Mathieu-Daudé, qemu-devel, qemu-arm, qemu-s390x
On 6/6/23 06:27, Ilya Leoshkevich wrote:
> /proc/self/smaps is an extension of /proc/self/maps: it provides the
> same lines, plus additional information about each range.
>
> GDB uses /proc/self/smaps when available, which means that
> generate-core-file tries it first before falling back to
> /proc/self/maps. This, in turn, causes it to dump the host mappings,
> since /proc/self/smaps is not emulated and is just passed through.
>
> Fix by emulating /proc/self/smaps. Provide true values only for
> Size, KernelPageSize, MMUPageSize and VmFlags. Leave all other values
> at 0, which is a valid conservative estimate.
>
> Signed-off-by: Ilya Leoshkevich<iii@linux.ibm.com>
> ---
> linux-user/syscall.c | 58 +++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 57 insertions(+), 1 deletion(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> + if (smaps) {
> + show_smaps(fd, max - min);
> + dprintf(fd, "VmFlags:%s%s%s%s%s%s%s%s\n",
> + (flags & PAGE_READ) ? " rd" : "",
> + (flags & PAGE_WRITE_ORG) ? " wr" : "",
> + (flags & PAGE_EXEC) ? " ex" : "",
> + e->is_priv ? "" : " sh",
> + (flags & PAGE_READ) ? " mr" : "",
> + (flags & PAGE_WRITE_ORG) ? " mw" : "",
> + (flags & PAGE_EXEC) ? " me" : "",
> + e->is_priv ? "" : " ms");
> + }
There are a couple of AArch64 specific bits here, which we emulate.
But this is good enough for now.
r~
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v3 4/8] gdbstub: Expose gdb_get_process() and gdb_get_first_cpu_in_process()
2023-06-06 13:27 [PATCH v3 0/8] gdbstub: Add support for info proc mappings Ilya Leoshkevich
` (2 preceding siblings ...)
2023-06-06 13:27 ` [PATCH v3 3/8] linux-user: Emulate /proc/self/smaps Ilya Leoshkevich
@ 2023-06-06 13:27 ` Ilya Leoshkevich
2023-06-06 13:27 ` [PATCH v3 5/8] gdbstub: Report the actual qemu-user pid Ilya Leoshkevich
` (3 subsequent siblings)
7 siblings, 0 replies; 18+ messages in thread
From: Ilya Leoshkevich @ 2023-06-06 13:27 UTC (permalink / raw)
To: Alex Bennée, Laurent Vivier, Peter Maydell,
Richard Henderson, David Hildenbrand
Cc: Philippe Mathieu-Daudé, qemu-devel, qemu-arm, qemu-s390x,
Ilya Leoshkevich
These functions will be needed by user-target.c in order to retrieve
the name of the executable.
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
gdbstub/gdbstub.c | 16 ++++++++--------
gdbstub/internals.h | 2 ++
2 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index be18568d0af..9139fec92af 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -211,7 +211,7 @@ static uint32_t gdb_get_cpu_pid(CPUState *cpu)
return cpu->cluster_index + 1;
}
-static GDBProcess *gdb_get_process(uint32_t pid)
+GDBProcess *gdb_get_process(uint32_t pid)
{
int i;
@@ -247,7 +247,7 @@ static CPUState *find_cpu(uint32_t thread_id)
return NULL;
}
-static CPUState *get_first_cpu_in_process(GDBProcess *process)
+CPUState *gdb_get_first_cpu_in_process(GDBProcess *process)
{
CPUState *cpu;
@@ -325,7 +325,7 @@ static CPUState *gdb_get_cpu(uint32_t pid, uint32_t tid)
return NULL;
}
- return get_first_cpu_in_process(process);
+ return gdb_get_first_cpu_in_process(process);
} else {
/* a specific thread */
cpu = find_cpu(tid);
@@ -354,7 +354,7 @@ static const char *get_feature_xml(const char *p, const char **newp,
size_t len;
int i;
const char *name;
- CPUState *cpu = get_first_cpu_in_process(process);
+ CPUState *cpu = gdb_get_first_cpu_in_process(process);
CPUClass *cc = CPU_GET_CLASS(cpu);
len = 0;
@@ -490,7 +490,7 @@ void gdb_register_coprocessor(CPUState *cpu,
static void gdb_process_breakpoint_remove_all(GDBProcess *p)
{
- CPUState *cpu = get_first_cpu_in_process(p);
+ CPUState *cpu = gdb_get_first_cpu_in_process(p);
while (cpu) {
gdb_breakpoint_remove_all(cpu);
@@ -653,7 +653,7 @@ static int gdb_handle_vcont(const char *p)
goto out;
}
- cpu = get_first_cpu_in_process(process);
+ cpu = gdb_get_first_cpu_in_process(process);
while (cpu) {
if (newstates[cpu->cpu_index] == 1) {
newstates[cpu->cpu_index] = cur_action;
@@ -1280,7 +1280,7 @@ static void handle_v_attach(GArray *params, void *user_ctx)
goto cleanup;
}
- cpu = get_first_cpu_in_process(process);
+ cpu = gdb_get_first_cpu_in_process(process);
if (!cpu) {
goto cleanup;
}
@@ -1403,7 +1403,7 @@ static void handle_query_curr_tid(GArray *params, void *user_ctx)
* first thread).
*/
process = gdb_get_cpu_process(gdbserver_state.g_cpu);
- cpu = get_first_cpu_in_process(process);
+ cpu = gdb_get_first_cpu_in_process(process);
g_string_assign(gdbserver_state.str_buf, "QC");
gdb_append_thread_id(cpu, gdbserver_state.str_buf);
gdb_put_strbuf();
diff --git a/gdbstub/internals.h b/gdbstub/internals.h
index 33d21d64886..25e4d5eeaa6 100644
--- a/gdbstub/internals.h
+++ b/gdbstub/internals.h
@@ -129,6 +129,8 @@ void gdb_read_byte(uint8_t ch);
*/
bool gdb_got_immediate_ack(void);
/* utility helpers */
+GDBProcess *gdb_get_process(uint32_t pid);
+CPUState *gdb_get_first_cpu_in_process(GDBProcess *process);
CPUState *gdb_first_attached_cpu(void);
void gdb_append_thread_id(CPUState *cpu, GString *buf);
int gdb_get_cpu_index(CPUState *cpu);
--
2.40.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH v3 5/8] gdbstub: Report the actual qemu-user pid
2023-06-06 13:27 [PATCH v3 0/8] gdbstub: Add support for info proc mappings Ilya Leoshkevich
` (3 preceding siblings ...)
2023-06-06 13:27 ` [PATCH v3 4/8] gdbstub: Expose gdb_get_process() and gdb_get_first_cpu_in_process() Ilya Leoshkevich
@ 2023-06-06 13:27 ` Ilya Leoshkevich
2023-06-06 13:27 ` [PATCH v3 6/8] gdbstub: Add support for info proc mappings Ilya Leoshkevich
` (2 subsequent siblings)
7 siblings, 0 replies; 18+ messages in thread
From: Ilya Leoshkevich @ 2023-06-06 13:27 UTC (permalink / raw)
To: Alex Bennée, Laurent Vivier, Peter Maydell,
Richard Henderson, David Hildenbrand
Cc: Philippe Mathieu-Daudé, qemu-devel, qemu-arm, qemu-s390x,
Ilya Leoshkevich
Currently qemu-user reports pid 1 to GDB. Resolve the TODO and report
the actual PID. Using getpid() relies on the assumption that there is
only one GDBProcess. Add an assertion to make sure that future changes
don't break it.
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
gdbstub/gdbstub.c | 25 +++++++++++++++++--------
1 file changed, 17 insertions(+), 8 deletions(-)
diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index 9139fec92af..c7e3ee71f2f 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -202,13 +202,16 @@ void gdb_memtox(GString *buf, const char *mem, int len)
static uint32_t gdb_get_cpu_pid(CPUState *cpu)
{
- /* TODO: In user mode, we should use the task state PID */
+#ifdef CONFIG_USER_ONLY
+ return getpid();
+#else
if (cpu->cluster_index == UNASSIGNED_CLUSTER_INDEX) {
/* Return the default process' PID */
int index = gdbserver_state.process_num - 1;
return gdbserver_state.processes[index].pid;
}
return cpu->cluster_index + 1;
+#endif
}
GDBProcess *gdb_get_process(uint32_t pid)
@@ -2146,19 +2149,25 @@ void gdb_read_byte(uint8_t ch)
void gdb_create_default_process(GDBState *s)
{
GDBProcess *process;
- int max_pid = 0;
+ int pid;
+#ifdef CONFIG_USER_ONLY
+ assert(gdbserver_state.process_num == 0);
+ pid = getpid();
+#else
if (gdbserver_state.process_num) {
- max_pid = s->processes[s->process_num - 1].pid;
+ pid = s->processes[s->process_num - 1].pid;
+ } else {
+ pid = 0;
}
+ /* We need an available PID slot for this process */
+ assert(pid < UINT32_MAX);
+ pid++;
+#endif
s->processes = g_renew(GDBProcess, s->processes, ++s->process_num);
process = &s->processes[s->process_num - 1];
-
- /* We need an available PID slot for this process */
- assert(max_pid < UINT32_MAX);
-
- process->pid = max_pid + 1;
+ process->pid = pid;
process->attached = false;
process->target_xml[0] = '\0';
}
--
2.40.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH v3 6/8] gdbstub: Add support for info proc mappings
2023-06-06 13:27 [PATCH v3 0/8] gdbstub: Add support for info proc mappings Ilya Leoshkevich
` (4 preceding siblings ...)
2023-06-06 13:27 ` [PATCH v3 5/8] gdbstub: Report the actual qemu-user pid Ilya Leoshkevich
@ 2023-06-06 13:27 ` Ilya Leoshkevich
2023-06-21 10:19 ` Alex Bennée
2023-06-06 13:27 ` [PATCH v3 7/8] docs: Document security implications of debugging Ilya Leoshkevich
2023-06-06 13:27 ` [PATCH v3 8/8] tests/tcg: Add a test for info proc mappings Ilya Leoshkevich
7 siblings, 1 reply; 18+ messages in thread
From: Ilya Leoshkevich @ 2023-06-06 13:27 UTC (permalink / raw)
To: Alex Bennée, Laurent Vivier, Peter Maydell,
Richard Henderson, David Hildenbrand
Cc: Philippe Mathieu-Daudé, qemu-devel, qemu-arm, qemu-s390x,
Ilya Leoshkevich, Dominik 'Disconnect3d' Czarnota
Currently the GDB's generate-core-file command doesn't work well with
qemu-user: the resulting dumps are huge [1] and at the same time
incomplete (argv and envp are missing). The reason is that GDB has no
access to proc mappings and therefore has to fall back to using
heuristics for discovering them. This is, in turn, because qemu-user
does not implement the Host I/O feature of the GDB Remote Serial
Protocol.
Implement vFile:{open,close,pread,readlink} and also
qXfer:exec-file:read+. With that, generate-core-file begins to work on
aarch64 and s390x.
[1] https://sourceware.org/pipermail/gdb-patches/2023-May/199432.html
Co-developed-by: Dominik 'Disconnect3d' Czarnota <dominik.b.czarnota@gmail.com>
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
gdbstub/gdbstub.c | 45 +++++++++++++-
gdbstub/internals.h | 5 ++
gdbstub/user-target.c | 139 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 187 insertions(+), 2 deletions(-)
diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index c7e3ee71f2f..d2efefd3528 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -1337,6 +1337,36 @@ static const GdbCmdParseEntry gdb_v_commands_table[] = {
.cmd = "Kill;",
.cmd_startswith = 1
},
+#ifdef CONFIG_USER_ONLY
+ /*
+ * Host I/O Packets. See [1] for details.
+ * [1] https://sourceware.org/gdb/onlinedocs/gdb/Host-I_002fO-Packets.html
+ */
+ {
+ .handler = gdb_handle_v_file_open,
+ .cmd = "File:open:",
+ .cmd_startswith = 1,
+ .schema = "s,L,L0"
+ },
+ {
+ .handler = gdb_handle_v_file_close,
+ .cmd = "File:close:",
+ .cmd_startswith = 1,
+ .schema = "l0"
+ },
+ {
+ .handler = gdb_handle_v_file_pread,
+ .cmd = "File:pread:",
+ .cmd_startswith = 1,
+ .schema = "l,L,L0"
+ },
+ {
+ .handler = gdb_handle_v_file_readlink,
+ .cmd = "File:readlink:",
+ .cmd_startswith = 1,
+ .schema = "s0"
+ },
+#endif
};
static void handle_v_commands(GArray *params, void *user_ctx)
@@ -1482,11 +1512,14 @@ static void handle_query_supported(GArray *params, void *user_ctx)
";ReverseStep+;ReverseContinue+");
}
-#if defined(CONFIG_USER_ONLY) && defined(CONFIG_LINUX)
+#if defined(CONFIG_USER_ONLY)
+#if defined(CONFIG_LINUX)
if (gdbserver_state.c_cpu->opaque) {
g_string_append(gdbserver_state.str_buf, ";qXfer:auxv:read+");
}
#endif
+ g_string_append(gdbserver_state.str_buf, ";qXfer:exec-file:read+");
+#endif
if (params->len &&
strstr(get_param(params, 0)->data, "multiprocess+")) {
@@ -1625,13 +1658,21 @@ static const GdbCmdParseEntry gdb_gen_query_table[] = {
.cmd_startswith = 1,
.schema = "s:l,l0"
},
-#if defined(CONFIG_USER_ONLY) && defined(CONFIG_LINUX)
+#if defined(CONFIG_USER_ONLY)
+#if defined(CONFIG_LINUX)
{
.handler = gdb_handle_query_xfer_auxv,
.cmd = "Xfer:auxv:read::",
.cmd_startswith = 1,
.schema = "l,l0"
},
+#endif
+ {
+ .handler = gdb_handle_query_xfer_exec_file,
+ .cmd = "Xfer:exec-file:read:",
+ .cmd_startswith = 1,
+ .schema = "l:l,l0"
+ },
#endif
{
.handler = gdb_handle_query_attached,
diff --git a/gdbstub/internals.h b/gdbstub/internals.h
index 25e4d5eeaa6..f2b46cce412 100644
--- a/gdbstub/internals.h
+++ b/gdbstub/internals.h
@@ -189,6 +189,11 @@ typedef union GdbCmdVariant {
void gdb_handle_query_rcmd(GArray *params, void *user_ctx); /* softmmu */
void gdb_handle_query_offsets(GArray *params, void *user_ctx); /* user */
void gdb_handle_query_xfer_auxv(GArray *params, void *user_ctx); /*user */
+void gdb_handle_v_file_open(GArray *params, void *user_ctx); /* user */
+void gdb_handle_v_file_close(GArray *params, void *user_ctx); /* user */
+void gdb_handle_v_file_pread(GArray *params, void *user_ctx); /* user */
+void gdb_handle_v_file_readlink(GArray *params, void *user_ctx); /* user */
+void gdb_handle_query_xfer_exec_file(GArray *params, void *user_ctx); /* user */
void gdb_handle_query_attached(GArray *params, void *user_ctx); /* both */
diff --git a/gdbstub/user-target.c b/gdbstub/user-target.c
index fa0e59ec9a5..aa64a8b9440 100644
--- a/gdbstub/user-target.c
+++ b/gdbstub/user-target.c
@@ -11,6 +11,10 @@
#include "exec/gdbstub.h"
#include "qemu.h"
#include "internals.h"
+#ifdef CONFIG_LINUX
+#include "linux-user/loader.h"
+#include "linux-user/qemu.h"
+#endif
/*
* Map target signal numbers to GDB protocol signal numbers and vice
@@ -281,3 +285,138 @@ void gdb_handle_query_xfer_auxv(GArray *params, void *user_ctx)
gdbserver_state.str_buf->len, true);
}
#endif
+
+static const char *get_filename_param(GArray *params, int i)
+{
+ const char *hex_filename = get_param(params, i)->data;
+ gdb_hextomem(gdbserver_state.mem_buf, hex_filename,
+ strlen(hex_filename) / 2);
+ g_byte_array_append(gdbserver_state.mem_buf, (const guint8 *)"", 1);
+ return (const char *)gdbserver_state.mem_buf->data;
+}
+
+static void hostio_reply_with_data(const void *buf, size_t n)
+{
+ g_string_printf(gdbserver_state.str_buf, "F%lx;", n);
+ gdb_memtox(gdbserver_state.str_buf, buf, n);
+ gdb_put_packet_binary(gdbserver_state.str_buf->str,
+ gdbserver_state.str_buf->len, true);
+}
+
+void gdb_handle_v_file_open(GArray *params, void *user_ctx)
+{
+ const char *filename = get_filename_param(params, 0);
+ uint64_t flags = get_param(params, 1)->val_ull;
+ uint64_t mode = get_param(params, 2)->val_ull;
+
+#ifdef CONFIG_LINUX
+ int fd = do_guest_openat(gdbserver_state.g_cpu->env_ptr, 0, filename,
+ flags, mode, false);
+#else
+ int fd = open(filename, flags, mode);
+#endif
+ if (fd < 0) {
+ g_string_printf(gdbserver_state.str_buf, "F-1,%d", errno);
+ } else {
+ g_string_printf(gdbserver_state.str_buf, "F%d", fd);
+ }
+ gdb_put_strbuf();
+}
+
+void gdb_handle_v_file_close(GArray *params, void *user_ctx)
+{
+ int fd = get_param(params, 0)->val_ul;
+
+ if (close(fd) == -1) {
+ g_string_printf(gdbserver_state.str_buf, "F-1,%d", errno);
+ gdb_put_strbuf();
+ return;
+ }
+
+ gdb_put_packet("F00");
+}
+
+#define BUFSIZ 8192
+
+void gdb_handle_v_file_pread(GArray *params, void *user_ctx)
+{
+ int fd = get_param(params, 0)->val_ul;
+ size_t count = get_param(params, 1)->val_ull;
+ off_t offset = get_param(params, 2)->val_ull;
+
+ size_t bufsiz = MIN(count, BUFSIZ);
+ g_autofree char *buf = g_try_malloc(bufsiz);
+ if (buf == NULL) {
+ gdb_put_packet("E12");
+ return;
+ }
+
+ ssize_t n = pread(fd, buf, bufsiz, offset);
+ if (n < 0) {
+ g_string_printf(gdbserver_state.str_buf, "F-1,%d", errno);
+ gdb_put_strbuf();
+ return;
+ }
+ hostio_reply_with_data(buf, n);
+}
+
+void gdb_handle_v_file_readlink(GArray *params, void *user_ctx)
+{
+ const char *filename = get_filename_param(params, 0);
+
+ g_autofree char *buf = g_try_malloc(BUFSIZ);
+ if (buf == NULL) {
+ gdb_put_packet("E12");
+ return;
+ }
+
+#ifdef CONFIG_LINUX
+ ssize_t n = do_guest_readlink(filename, buf, BUFSIZ);
+#else
+ ssize_t n = readlink(filename, buf, BUFSIZ);
+#endif
+ if (n < 0) {
+ g_string_printf(gdbserver_state.str_buf, "F-1,%d", errno);
+ gdb_put_strbuf();
+ return;
+ }
+ hostio_reply_with_data(buf, n);
+}
+
+void gdb_handle_query_xfer_exec_file(GArray *params, void *user_ctx)
+{
+ uint32_t pid = get_param(params, 0)->val_ul;
+ uint32_t offset = get_param(params, 1)->val_ul;
+ uint32_t length = get_param(params, 2)->val_ul;
+
+ GDBProcess *process = gdb_get_process(pid);
+ if (!process) {
+ gdb_put_packet("E00");
+ return;
+ }
+
+ CPUState *cpu = gdb_get_first_cpu_in_process(process);
+ if (!cpu) {
+ gdb_put_packet("E00");
+ return;
+ }
+
+ TaskState *ts = cpu->opaque;
+ if (!ts || !ts->bprm || !ts->bprm->filename) {
+ gdb_put_packet("E00");
+ return;
+ }
+
+ size_t total_length = strlen(ts->bprm->filename);
+ if (offset > total_length) {
+ gdb_put_packet("E00");
+ return;
+ }
+ if (offset + length > total_length) {
+ length = total_length - offset;
+ }
+
+ g_string_printf(gdbserver_state.str_buf, "l%.*s", length,
+ ts->bprm->filename + offset);
+ gdb_put_strbuf();
+}
--
2.40.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v3 6/8] gdbstub: Add support for info proc mappings
2023-06-06 13:27 ` [PATCH v3 6/8] gdbstub: Add support for info proc mappings Ilya Leoshkevich
@ 2023-06-21 10:19 ` Alex Bennée
0 siblings, 0 replies; 18+ messages in thread
From: Alex Bennée @ 2023-06-21 10:19 UTC (permalink / raw)
To: Ilya Leoshkevich
Cc: Laurent Vivier, Peter Maydell, Richard Henderson,
David Hildenbrand, Philippe Mathieu-Daudé, qemu-devel,
qemu-arm, qemu-s390x, Dominik 'Disconnect3d' Czarnota
Ilya Leoshkevich <iii@linux.ibm.com> writes:
> Currently the GDB's generate-core-file command doesn't work well with
> qemu-user: the resulting dumps are huge [1] and at the same time
> incomplete (argv and envp are missing). The reason is that GDB has no
> access to proc mappings and therefore has to fall back to using
> heuristics for discovering them. This is, in turn, because qemu-user
> does not implement the Host I/O feature of the GDB Remote Serial
> Protocol.
>
> Implement vFile:{open,close,pread,readlink} and also
> qXfer:exec-file:read+. With that, generate-core-file begins to work on
> aarch64 and s390x.
>
> [1] https://sourceware.org/pipermail/gdb-patches/2023-May/199432.html
>
> Co-developed-by: Dominik 'Disconnect3d' Czarnota <dominik.b.czarnota@gmail.com>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
> gdbstub/gdbstub.c | 45 +++++++++++++-
> gdbstub/internals.h | 5 ++
> gdbstub/user-target.c | 139 ++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 187 insertions(+), 2 deletions(-)
>
> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> index c7e3ee71f2f..d2efefd3528 100644
> --- a/gdbstub/gdbstub.c
> +++ b/gdbstub/gdbstub.c
> @@ -1337,6 +1337,36 @@ static const GdbCmdParseEntry gdb_v_commands_table[] = {
> .cmd = "Kill;",
> .cmd_startswith = 1
> },
> +#ifdef CONFIG_USER_ONLY
> + /*
> + * Host I/O Packets. See [1] for details.
> + * [1] https://sourceware.org/gdb/onlinedocs/gdb/Host-I_002fO-Packets.html
> + */
> + {
> + .handler = gdb_handle_v_file_open,
> + .cmd = "File:open:",
> + .cmd_startswith = 1,
> + .schema = "s,L,L0"
> + },
> + {
> + .handler = gdb_handle_v_file_close,
> + .cmd = "File:close:",
> + .cmd_startswith = 1,
> + .schema = "l0"
> + },
> + {
> + .handler = gdb_handle_v_file_pread,
> + .cmd = "File:pread:",
> + .cmd_startswith = 1,
> + .schema = "l,L,L0"
> + },
> + {
> + .handler = gdb_handle_v_file_readlink,
> + .cmd = "File:readlink:",
> + .cmd_startswith = 1,
> + .schema = "s0"
> + },
> +#endif
> };
>
> static void handle_v_commands(GArray *params, void *user_ctx)
> @@ -1482,11 +1512,14 @@ static void handle_query_supported(GArray *params, void *user_ctx)
> ";ReverseStep+;ReverseContinue+");
> }
>
> -#if defined(CONFIG_USER_ONLY) && defined(CONFIG_LINUX)
> +#if defined(CONFIG_USER_ONLY)
> +#if defined(CONFIG_LINUX)
> if (gdbserver_state.c_cpu->opaque) {
> g_string_append(gdbserver_state.str_buf, ";qXfer:auxv:read+");
> }
> #endif
> + g_string_append(gdbserver_state.str_buf, ";qXfer:exec-file:read+");
> +#endif
>
> if (params->len &&
> strstr(get_param(params, 0)->data, "multiprocess+")) {
> @@ -1625,13 +1658,21 @@ static const GdbCmdParseEntry gdb_gen_query_table[] = {
> .cmd_startswith = 1,
> .schema = "s:l,l0"
> },
> -#if defined(CONFIG_USER_ONLY) && defined(CONFIG_LINUX)
> +#if defined(CONFIG_USER_ONLY)
> +#if defined(CONFIG_LINUX)
> {
> .handler = gdb_handle_query_xfer_auxv,
> .cmd = "Xfer:auxv:read::",
> .cmd_startswith = 1,
> .schema = "l,l0"
> },
> +#endif
> + {
> + .handler = gdb_handle_query_xfer_exec_file,
> + .cmd = "Xfer:exec-file:read:",
> + .cmd_startswith = 1,
> + .schema = "l:l,l0"
> + },
> #endif
> {
> .handler = gdb_handle_query_attached,
> diff --git a/gdbstub/internals.h b/gdbstub/internals.h
> index 25e4d5eeaa6..f2b46cce412 100644
> --- a/gdbstub/internals.h
> +++ b/gdbstub/internals.h
> @@ -189,6 +189,11 @@ typedef union GdbCmdVariant {
> void gdb_handle_query_rcmd(GArray *params, void *user_ctx); /* softmmu */
> void gdb_handle_query_offsets(GArray *params, void *user_ctx); /* user */
> void gdb_handle_query_xfer_auxv(GArray *params, void *user_ctx); /*user */
> +void gdb_handle_v_file_open(GArray *params, void *user_ctx); /* user */
> +void gdb_handle_v_file_close(GArray *params, void *user_ctx); /* user */
> +void gdb_handle_v_file_pread(GArray *params, void *user_ctx); /* user */
> +void gdb_handle_v_file_readlink(GArray *params, void *user_ctx); /* user */
> +void gdb_handle_query_xfer_exec_file(GArray *params, void *user_ctx); /* user */
>
> void gdb_handle_query_attached(GArray *params, void *user_ctx); /* both */
>
> diff --git a/gdbstub/user-target.c b/gdbstub/user-target.c
> index fa0e59ec9a5..aa64a8b9440 100644
> --- a/gdbstub/user-target.c
> +++ b/gdbstub/user-target.c
> @@ -11,6 +11,10 @@
> #include "exec/gdbstub.h"
> #include "qemu.h"
> #include "internals.h"
> +#ifdef CONFIG_LINUX
> +#include "linux-user/loader.h"
> +#include "linux-user/qemu.h"
> +#endif
>
> /*
> * Map target signal numbers to GDB protocol signal numbers and vice
> @@ -281,3 +285,138 @@ void gdb_handle_query_xfer_auxv(GArray *params, void *user_ctx)
> gdbserver_state.str_buf->len, true);
> }
> #endif
> +
> +static const char *get_filename_param(GArray *params, int i)
> +{
> + const char *hex_filename = get_param(params, i)->data;
> + gdb_hextomem(gdbserver_state.mem_buf, hex_filename,
> + strlen(hex_filename) / 2);
> + g_byte_array_append(gdbserver_state.mem_buf, (const guint8 *)"", 1);
> + return (const char *)gdbserver_state.mem_buf->data;
> +}
> +
> +static void hostio_reply_with_data(const void *buf, size_t n)
> +{
> + g_string_printf(gdbserver_state.str_buf, "F%lx;", n);
> + gdb_memtox(gdbserver_state.str_buf, buf, n);
> + gdb_put_packet_binary(gdbserver_state.str_buf->str,
> + gdbserver_state.str_buf->len, true);
> +}
This fails on 64/32 builds:
../gdbstub/user-target.c: In function ‘hostio_reply_with_data’:
../gdbstub/user-target.c:300:50: error: format ‘%lx’ expects argument of type ‘long unsigned int’, but argument 3 has type ‘size_t’ {aka ‘unsigned int’} [-Werror=format=]
300 | g_string_printf(gdbserver_state.str_buf, "F%lx;", n);
| ~~^ ~
| | |
| | size_t {aka unsigned int}
| long unsigned int
| %x
cc1: all warnings being treated as errors
I think "%zx" is the portable format string you want.
> +void gdb_handle_v_file_open(GArray *params, void *user_ctx)
> +{
> + const char *filename = get_filename_param(params, 0);
> + uint64_t flags = get_param(params, 1)->val_ull;
> + uint64_t mode = get_param(params, 2)->val_ull;
> +
> +#ifdef CONFIG_LINUX
> + int fd = do_guest_openat(gdbserver_state.g_cpu->env_ptr, 0, filename,
> + flags, mode, false);
> +#else
> + int fd = open(filename, flags, mode);
> +#endif
> + if (fd < 0) {
> + g_string_printf(gdbserver_state.str_buf, "F-1,%d", errno);
> + } else {
> + g_string_printf(gdbserver_state.str_buf, "F%d", fd);
> + }
> + gdb_put_strbuf();
> +}
> +
> +void gdb_handle_v_file_close(GArray *params, void *user_ctx)
> +{
> + int fd = get_param(params, 0)->val_ul;
> +
> + if (close(fd) == -1) {
> + g_string_printf(gdbserver_state.str_buf, "F-1,%d", errno);
> + gdb_put_strbuf();
> + return;
> + }
> +
> + gdb_put_packet("F00");
> +}
> +
> +#define BUFSIZ 8192
> +
> +void gdb_handle_v_file_pread(GArray *params, void *user_ctx)
> +{
> + int fd = get_param(params, 0)->val_ul;
> + size_t count = get_param(params, 1)->val_ull;
> + off_t offset = get_param(params, 2)->val_ull;
> +
> + size_t bufsiz = MIN(count, BUFSIZ);
> + g_autofree char *buf = g_try_malloc(bufsiz);
> + if (buf == NULL) {
> + gdb_put_packet("E12");
> + return;
> + }
> +
> + ssize_t n = pread(fd, buf, bufsiz, offset);
> + if (n < 0) {
> + g_string_printf(gdbserver_state.str_buf, "F-1,%d", errno);
> + gdb_put_strbuf();
> + return;
> + }
> + hostio_reply_with_data(buf, n);
> +}
> +
> +void gdb_handle_v_file_readlink(GArray *params, void *user_ctx)
> +{
> + const char *filename = get_filename_param(params, 0);
> +
> + g_autofree char *buf = g_try_malloc(BUFSIZ);
> + if (buf == NULL) {
> + gdb_put_packet("E12");
> + return;
> + }
> +
> +#ifdef CONFIG_LINUX
> + ssize_t n = do_guest_readlink(filename, buf, BUFSIZ);
> +#else
> + ssize_t n = readlink(filename, buf, BUFSIZ);
> +#endif
> + if (n < 0) {
> + g_string_printf(gdbserver_state.str_buf, "F-1,%d", errno);
> + gdb_put_strbuf();
> + return;
> + }
> + hostio_reply_with_data(buf, n);
> +}
> +
> +void gdb_handle_query_xfer_exec_file(GArray *params, void *user_ctx)
> +{
> + uint32_t pid = get_param(params, 0)->val_ul;
> + uint32_t offset = get_param(params, 1)->val_ul;
> + uint32_t length = get_param(params, 2)->val_ul;
> +
> + GDBProcess *process = gdb_get_process(pid);
> + if (!process) {
> + gdb_put_packet("E00");
> + return;
> + }
> +
> + CPUState *cpu = gdb_get_first_cpu_in_process(process);
> + if (!cpu) {
> + gdb_put_packet("E00");
> + return;
> + }
> +
> + TaskState *ts = cpu->opaque;
> + if (!ts || !ts->bprm || !ts->bprm->filename) {
> + gdb_put_packet("E00");
> + return;
> + }
> +
> + size_t total_length = strlen(ts->bprm->filename);
> + if (offset > total_length) {
> + gdb_put_packet("E00");
> + return;
> + }
> + if (offset + length > total_length) {
> + length = total_length - offset;
> + }
> +
> + g_string_printf(gdbserver_state.str_buf, "l%.*s", length,
> + ts->bprm->filename + offset);
> + gdb_put_strbuf();
> +}
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v3 7/8] docs: Document security implications of debugging
2023-06-06 13:27 [PATCH v3 0/8] gdbstub: Add support for info proc mappings Ilya Leoshkevich
` (5 preceding siblings ...)
2023-06-06 13:27 ` [PATCH v3 6/8] gdbstub: Add support for info proc mappings Ilya Leoshkevich
@ 2023-06-06 13:27 ` Ilya Leoshkevich
2023-06-06 13:27 ` [PATCH v3 8/8] tests/tcg: Add a test for info proc mappings Ilya Leoshkevich
7 siblings, 0 replies; 18+ messages in thread
From: Ilya Leoshkevich @ 2023-06-06 13:27 UTC (permalink / raw)
To: Alex Bennée, Laurent Vivier, Peter Maydell,
Richard Henderson, David Hildenbrand
Cc: Philippe Mathieu-Daudé, qemu-devel, qemu-arm, qemu-s390x,
Ilya Leoshkevich
Now that the GDB stub explicitly implements reading host files (note
that it was already possible by changing the emulated code to open and
read those files), concerns may arise that it undermines security.
Document the status quo, which is that the users are already
responsible for securing the GDB connection themselves.
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
docs/system/gdb.rst | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/docs/system/gdb.rst b/docs/system/gdb.rst
index 7d3718deefb..9906991b841 100644
--- a/docs/system/gdb.rst
+++ b/docs/system/gdb.rst
@@ -214,3 +214,18 @@ The memory mode can be checked by sending the following command:
``maintenance packet Qqemu.PhyMemMode:0``
This will change it back to normal memory mode.
+
+Security considerations
+=======================
+
+Connecting to the GDB socket allows running arbitrary code inside the guest;
+in case of the TCG emulation, which is not considered a security boundary, this
+also means running arbitrary code on the host. Additionally, when debugging
+qemu-user, it allows directly downloading any file readable by QEMU from the
+host.
+
+The GDB socket is not protected by authentication, authorization or encryption.
+It is therefore a responsibility of the user to make sure that only authorized
+clients can connect to it, e.g., by using a unix socket with proper
+permissions, or by opening a TCP socket only on interfaces that are not
+reachable by potential attackers.
--
2.40.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH v3 8/8] tests/tcg: Add a test for info proc mappings
2023-06-06 13:27 [PATCH v3 0/8] gdbstub: Add support for info proc mappings Ilya Leoshkevich
` (6 preceding siblings ...)
2023-06-06 13:27 ` [PATCH v3 7/8] docs: Document security implications of debugging Ilya Leoshkevich
@ 2023-06-06 13:27 ` Ilya Leoshkevich
2023-06-21 10:21 ` Alex Bennée
7 siblings, 1 reply; 18+ messages in thread
From: Ilya Leoshkevich @ 2023-06-06 13:27 UTC (permalink / raw)
To: Alex Bennée, Laurent Vivier, Peter Maydell,
Richard Henderson, David Hildenbrand
Cc: Philippe Mathieu-Daudé, qemu-devel, qemu-arm, qemu-s390x,
Ilya Leoshkevich
Add a small test to prevent regressions.
Since there are issues with how GDB interprets QEMU's target.xml,
enable the test only on aarch64 and s390x for now.
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
tests/tcg/aarch64/Makefile.target | 3 +-
tests/tcg/multiarch/Makefile.target | 7 +++
.../multiarch/gdbstub/test-proc-mappings.py | 55 +++++++++++++++++++
tests/tcg/s390x/Makefile.target | 2 +-
4 files changed, 65 insertions(+), 2 deletions(-)
create mode 100644 tests/tcg/multiarch/gdbstub/test-proc-mappings.py
diff --git a/tests/tcg/aarch64/Makefile.target b/tests/tcg/aarch64/Makefile.target
index 03157954871..38402b0ba1f 100644
--- a/tests/tcg/aarch64/Makefile.target
+++ b/tests/tcg/aarch64/Makefile.target
@@ -97,7 +97,8 @@ run-gdbstub-sve-ioctls: sve-ioctls
--bin $< --test $(AARCH64_SRC)/gdbstub/test-sve-ioctl.py, \
basic gdbstub SVE ZLEN support)
-EXTRA_RUNS += run-gdbstub-sysregs run-gdbstub-sve-ioctls
+EXTRA_RUNS += run-gdbstub-sysregs run-gdbstub-sve-ioctls \
+ run-gdbstub-proc-mappings
endif
endif
diff --git a/tests/tcg/multiarch/Makefile.target b/tests/tcg/multiarch/Makefile.target
index 373db696481..cbc0b75787a 100644
--- a/tests/tcg/multiarch/Makefile.target
+++ b/tests/tcg/multiarch/Makefile.target
@@ -81,6 +81,13 @@ run-gdbstub-qxfer-auxv-read: sha1
--bin $< --test $(MULTIARCH_SRC)/gdbstub/test-qxfer-auxv-read.py, \
basic gdbstub qXfer:auxv:read support)
+run-gdbstub-proc-mappings: sha1
+ $(call run-test, $@, $(GDB_SCRIPT) \
+ --gdb $(HAVE_GDB_BIN) \
+ --qemu $(QEMU) --qargs "$(QEMU_OPTS)" \
+ --bin $< --test $(MULTIARCH_SRC)/gdbstub/test-proc-mappings.py, \
+ proc mappings support)
+
run-gdbstub-thread-breakpoint: testthread
$(call run-test, $@, $(GDB_SCRIPT) \
--gdb $(HAVE_GDB_BIN) \
diff --git a/tests/tcg/multiarch/gdbstub/test-proc-mappings.py b/tests/tcg/multiarch/gdbstub/test-proc-mappings.py
new file mode 100644
index 00000000000..657e36a2fc7
--- /dev/null
+++ b/tests/tcg/multiarch/gdbstub/test-proc-mappings.py
@@ -0,0 +1,55 @@
+"""Test that gdbstub has access to proc mappings.
+
+This runs as a sourced script (via -x, via run-test.py)."""
+from __future__ import print_function
+import gdb
+import sys
+
+
+n_failures = 0
+
+
+def report(cond, msg):
+ """Report success/fail of a test"""
+ if cond:
+ print("PASS: {}".format(msg))
+ else:
+ print("FAIL: {}".format(msg))
+ global n_failures
+ n_failures += 1
+
+
+def run_test():
+ """Run through the tests one by one"""
+ mappings = gdb.execute("info proc mappings", False, True)
+ report(isinstance(mappings, str), "Fetched the mappings from the inferior")
+ report("/sha1" in mappings, "Found the test binary name in the mappings")
+
+
+def main():
+ """Prepare the environment and run through the tests"""
+ try:
+ inferior = gdb.selected_inferior()
+ print("ATTACHED: {}".format(inferior.architecture().name()))
+ except (gdb.error, AttributeError):
+ print("SKIPPING (not connected)")
+ exit(0)
+
+ if gdb.parse_and_eval('$pc') == 0:
+ print("SKIP: PC not set")
+ exit(0)
+
+ try:
+ # These are not very useful in scripts
+ gdb.execute("set pagination off")
+ gdb.execute("set confirm off")
+
+ # Run the actual tests
+ run_test()
+ except gdb.error:
+ report(False, "GDB Exception: {}".format(sys.exc_info()[0]))
+ print("All tests complete: %d failures" % n_failures)
+ exit(n_failures)
+
+
+main()
diff --git a/tests/tcg/s390x/Makefile.target b/tests/tcg/s390x/Makefile.target
index 4899503e1db..d33960caa0a 100644
--- a/tests/tcg/s390x/Makefile.target
+++ b/tests/tcg/s390x/Makefile.target
@@ -78,7 +78,7 @@ run-gdbstub-signals-s390x: signals-s390x
--bin $< --test $(S390X_SRC)/gdbstub/test-signals-s390x.py, \
mixing signals and debugging)
-EXTRA_RUNS += run-gdbstub-signals-s390x
+EXTRA_RUNS += run-gdbstub-signals-s390x run-gdbstub-proc-mappings
endif
# MVX versions of sha512
--
2.40.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v3 8/8] tests/tcg: Add a test for info proc mappings
2023-06-06 13:27 ` [PATCH v3 8/8] tests/tcg: Add a test for info proc mappings Ilya Leoshkevich
@ 2023-06-21 10:21 ` Alex Bennée
2023-06-21 13:48 ` Ilya Leoshkevich
0 siblings, 1 reply; 18+ messages in thread
From: Alex Bennée @ 2023-06-21 10:21 UTC (permalink / raw)
To: Ilya Leoshkevich
Cc: Laurent Vivier, Peter Maydell, Richard Henderson,
David Hildenbrand, Philippe Mathieu-Daudé, qemu-devel,
qemu-arm, qemu-s390x
Ilya Leoshkevich <iii@linux.ibm.com> writes:
> Add a small test to prevent regressions.
> Since there are issues with how GDB interprets QEMU's target.xml,
> enable the test only on aarch64 and s390x for now.
>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
> tests/tcg/aarch64/Makefile.target | 3 +-
> tests/tcg/multiarch/Makefile.target | 7 +++
> .../multiarch/gdbstub/test-proc-mappings.py | 55 +++++++++++++++++++
> tests/tcg/s390x/Makefile.target | 2 +-
> 4 files changed, 65 insertions(+), 2 deletions(-)
> create mode 100644 tests/tcg/multiarch/gdbstub/test-proc-mappings.py
>
> diff --git a/tests/tcg/aarch64/Makefile.target b/tests/tcg/aarch64/Makefile.target
> index 03157954871..38402b0ba1f 100644
> --- a/tests/tcg/aarch64/Makefile.target
> +++ b/tests/tcg/aarch64/Makefile.target
> @@ -97,7 +97,8 @@ run-gdbstub-sve-ioctls: sve-ioctls
> --bin $< --test $(AARCH64_SRC)/gdbstub/test-sve-ioctl.py, \
> basic gdbstub SVE ZLEN support)
>
> -EXTRA_RUNS += run-gdbstub-sysregs run-gdbstub-sve-ioctls
> +EXTRA_RUNS += run-gdbstub-sysregs run-gdbstub-sve-ioctls \
> + run-gdbstub-proc-mappings
> endif
> endif
>
> diff --git a/tests/tcg/multiarch/Makefile.target b/tests/tcg/multiarch/Makefile.target
> index 373db696481..cbc0b75787a 100644
> --- a/tests/tcg/multiarch/Makefile.target
> +++ b/tests/tcg/multiarch/Makefile.target
> @@ -81,6 +81,13 @@ run-gdbstub-qxfer-auxv-read: sha1
> --bin $< --test $(MULTIARCH_SRC)/gdbstub/test-qxfer-auxv-read.py, \
> basic gdbstub qXfer:auxv:read support)
>
> +run-gdbstub-proc-mappings: sha1
> + $(call run-test, $@, $(GDB_SCRIPT) \
> + --gdb $(HAVE_GDB_BIN) \
> + --qemu $(QEMU) --qargs "$(QEMU_OPTS)" \
> + --bin $< --test $(MULTIARCH_SRC)/gdbstub/test-proc-mappings.py, \
> + proc mappings support)
> +
I wondered if it makes more sense to keep the extra test configuration
logic in multiarch:
run-gdbstub-proc-mappings: sha1
$(call run-test, $@, $(GDB_SCRIPT) \
--gdb $(HAVE_GDB_BIN) \
--qemu $(QEMU) --qargs "$(QEMU_OPTS)" \
--bin $< --test $(MULTIARCH_SRC)/gdbstub/test-proc-mappings.py, \
proc mappings support)
# only enable for s390x and aarch64 for now
ifneq (,$(findstring aarch64,$(TARGET_NAME)))
EXTRA_RUNS += run-gdbstub-proc-mappings
else ifneq (,$(findstring s390x,$(TARGET_NAME)))
EXTRA_RUNS += run-gdbstub-proc-mappings
endif
but it still ends up pretty ugly. Is the gdb handling fixed for other
arches in other versions. Maybe we could probe gdb for support and wrap
the whole stanza in something like:
ifeq ($(HOST_GDB_SUPPORTS_PROC_MAPPING),y)
...
endif
?
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v3 8/8] tests/tcg: Add a test for info proc mappings
2023-06-21 10:21 ` Alex Bennée
@ 2023-06-21 13:48 ` Ilya Leoshkevich
2023-06-21 14:43 ` Alex Bennée
0 siblings, 1 reply; 18+ messages in thread
From: Ilya Leoshkevich @ 2023-06-21 13:48 UTC (permalink / raw)
To: Alex Bennée
Cc: Laurent Vivier, Peter Maydell, Richard Henderson,
David Hildenbrand, Philippe Mathieu-Daudé, qemu-devel,
qemu-arm, qemu-s390x
On Wed, 2023-06-21 at 11:21 +0100, Alex Bennée wrote:
>
> Ilya Leoshkevich <iii@linux.ibm.com> writes:
>
> > Add a small test to prevent regressions.
> > Since there are issues with how GDB interprets QEMU's target.xml,
> > enable the test only on aarch64 and s390x for now.
> >
> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > ---
> > tests/tcg/aarch64/Makefile.target | 3 +-
> > tests/tcg/multiarch/Makefile.target | 7 +++
> > .../multiarch/gdbstub/test-proc-mappings.py | 55
> > +++++++++++++++++++
> > tests/tcg/s390x/Makefile.target | 2 +-
> > 4 files changed, 65 insertions(+), 2 deletions(-)
> > create mode 100644 tests/tcg/multiarch/gdbstub/test-proc-
> > mappings.py
> >
> > diff --git a/tests/tcg/aarch64/Makefile.target
> > b/tests/tcg/aarch64/Makefile.target
> > index 03157954871..38402b0ba1f 100644
> > --- a/tests/tcg/aarch64/Makefile.target
> > +++ b/tests/tcg/aarch64/Makefile.target
> > @@ -97,7 +97,8 @@ run-gdbstub-sve-ioctls: sve-ioctls
> > --bin $< --test $(AARCH64_SRC)/gdbstub/test-sve-
> > ioctl.py, \
> > basic gdbstub SVE ZLEN support)
> >
> > -EXTRA_RUNS += run-gdbstub-sysregs run-gdbstub-sve-ioctls
> > +EXTRA_RUNS += run-gdbstub-sysregs run-gdbstub-sve-ioctls \
> > + run-gdbstub-proc-mappings
> > endif
> > endif
> >
> > diff --git a/tests/tcg/multiarch/Makefile.target
> > b/tests/tcg/multiarch/Makefile.target
> > index 373db696481..cbc0b75787a 100644
> > --- a/tests/tcg/multiarch/Makefile.target
> > +++ b/tests/tcg/multiarch/Makefile.target
> > @@ -81,6 +81,13 @@ run-gdbstub-qxfer-auxv-read: sha1
> > --bin $< --test $(MULTIARCH_SRC)/gdbstub/test-
> > qxfer-auxv-read.py, \
> > basic gdbstub qXfer:auxv:read support)
> >
> > +run-gdbstub-proc-mappings: sha1
> > + $(call run-test, $@, $(GDB_SCRIPT) \
> > + --gdb $(HAVE_GDB_BIN) \
> > + --qemu $(QEMU) --qargs "$(QEMU_OPTS)" \
> > + --bin $< --test $(MULTIARCH_SRC)/gdbstub/test-proc-
> > mappings.py, \
> > + proc mappings support)
> > +
>
> I wondered if it makes more sense to keep the extra test
> configuration
> logic in multiarch:
>
> run-gdbstub-proc-mappings: sha1
> $(call run-test, $@, $(GDB_SCRIPT) \
> --gdb $(HAVE_GDB_BIN) \
> --qemu $(QEMU) --qargs "$(QEMU_OPTS)" \
> --bin $< --test $(MULTIARCH_SRC)/gdbstub/test-proc-
> mappings.py, \
> proc mappings support)
>
> # only enable for s390x and aarch64 for now
> ifneq (,$(findstring aarch64,$(TARGET_NAME)))
> EXTRA_RUNS += run-gdbstub-proc-mappings
> else ifneq (,$(findstring s390x,$(TARGET_NAME)))
> EXTRA_RUNS += run-gdbstub-proc-mappings
> endif
>
> but it still ends up pretty ugly. Is the gdb handling fixed for other
> arches in other versions. Maybe we could probe gdb for support and
> wrap
> the whole stanza in something like:
>
> ifeq ($(HOST_GDB_SUPPORTS_PROC_MAPPING),y)
> ...
> endif
>
> ?
I think I better add the check to the test itself, because otherwise we
have to probe GDB against QEMU binary we just built, which sounds
unnecessarily complicated.
The error message on all arches without this series is:
warning: unable to open /proc file '/proc/1/maps'
The error message on x86_64 (expected) with this series is:
Not supported on this target.
So we can simply exit(0) from the test if we see this.
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v3 8/8] tests/tcg: Add a test for info proc mappings
2023-06-21 13:48 ` Ilya Leoshkevich
@ 2023-06-21 14:43 ` Alex Bennée
0 siblings, 0 replies; 18+ messages in thread
From: Alex Bennée @ 2023-06-21 14:43 UTC (permalink / raw)
To: Ilya Leoshkevich
Cc: Laurent Vivier, Peter Maydell, Richard Henderson,
David Hildenbrand, Philippe Mathieu-Daudé, qemu-devel,
qemu-arm, qemu-s390x
Ilya Leoshkevich <iii@linux.ibm.com> writes:
> On Wed, 2023-06-21 at 11:21 +0100, Alex Bennée wrote:
>>
>> Ilya Leoshkevich <iii@linux.ibm.com> writes:
>>
>> > Add a small test to prevent regressions.
>> > Since there are issues with how GDB interprets QEMU's target.xml,
>> > enable the test only on aarch64 and s390x for now.
>> >
>> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
>> > ---
>> > tests/tcg/aarch64/Makefile.target | 3 +-
>> > tests/tcg/multiarch/Makefile.target | 7 +++
>> > .../multiarch/gdbstub/test-proc-mappings.py | 55
>> > +++++++++++++++++++
>> > tests/tcg/s390x/Makefile.target | 2 +-
>> > 4 files changed, 65 insertions(+), 2 deletions(-)
>> > create mode 100644 tests/tcg/multiarch/gdbstub/test-proc-
>> > mappings.py
>> >
>> > diff --git a/tests/tcg/aarch64/Makefile.target
>> > b/tests/tcg/aarch64/Makefile.target
>> > index 03157954871..38402b0ba1f 100644
>> > --- a/tests/tcg/aarch64/Makefile.target
>> > +++ b/tests/tcg/aarch64/Makefile.target
>> > @@ -97,7 +97,8 @@ run-gdbstub-sve-ioctls: sve-ioctls
>> > --bin $< --test $(AARCH64_SRC)/gdbstub/test-sve-
>> > ioctl.py, \
>> > basic gdbstub SVE ZLEN support)
>> >
>> > -EXTRA_RUNS += run-gdbstub-sysregs run-gdbstub-sve-ioctls
>> > +EXTRA_RUNS += run-gdbstub-sysregs run-gdbstub-sve-ioctls \
>> > + run-gdbstub-proc-mappings
>> > endif
>> > endif
>> >
>> > diff --git a/tests/tcg/multiarch/Makefile.target
>> > b/tests/tcg/multiarch/Makefile.target
>> > index 373db696481..cbc0b75787a 100644
>> > --- a/tests/tcg/multiarch/Makefile.target
>> > +++ b/tests/tcg/multiarch/Makefile.target
>> > @@ -81,6 +81,13 @@ run-gdbstub-qxfer-auxv-read: sha1
>> > --bin $< --test $(MULTIARCH_SRC)/gdbstub/test-
>> > qxfer-auxv-read.py, \
>> > basic gdbstub qXfer:auxv:read support)
>> >
>> > +run-gdbstub-proc-mappings: sha1
>> > + $(call run-test, $@, $(GDB_SCRIPT) \
>> > + --gdb $(HAVE_GDB_BIN) \
>> > + --qemu $(QEMU) --qargs "$(QEMU_OPTS)" \
>> > + --bin $< --test $(MULTIARCH_SRC)/gdbstub/test-proc-
>> > mappings.py, \
>> > + proc mappings support)
>> > +
>>
>> I wondered if it makes more sense to keep the extra test
>> configuration
>> logic in multiarch:
>>
>> run-gdbstub-proc-mappings: sha1
>> $(call run-test, $@, $(GDB_SCRIPT) \
>> --gdb $(HAVE_GDB_BIN) \
>> --qemu $(QEMU) --qargs "$(QEMU_OPTS)" \
>> --bin $< --test $(MULTIARCH_SRC)/gdbstub/test-proc-
>> mappings.py, \
>> proc mappings support)
>>
>> # only enable for s390x and aarch64 for now
>> ifneq (,$(findstring aarch64,$(TARGET_NAME)))
>> EXTRA_RUNS += run-gdbstub-proc-mappings
>> else ifneq (,$(findstring s390x,$(TARGET_NAME)))
>> EXTRA_RUNS += run-gdbstub-proc-mappings
>> endif
>>
>> but it still ends up pretty ugly. Is the gdb handling fixed for other
>> arches in other versions. Maybe we could probe gdb for support and
>> wrap
>> the whole stanza in something like:
>>
>> ifeq ($(HOST_GDB_SUPPORTS_PROC_MAPPING),y)
>> ...
>> endif
>>
>> ?
>
> I think I better add the check to the test itself, because otherwise we
> have to probe GDB against QEMU binary we just built, which sounds
> unnecessarily complicated.
>
> The error message on all arches without this series is:
>
> warning: unable to open /proc file '/proc/1/maps'
>
> The error message on x86_64 (expected) with this series is:
>
> Not supported on this target.
>
> So we can simply exit(0) from the test if we see this.
That seems a simpler solution, lets do that.
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
^ permalink raw reply [flat|nested] 18+ messages in thread