* [Qemu-devel] [PATCH 0/3] target-m68k/m68k-semi: don't ignore get/put_user failure
@ 2012-10-29 12:05 Peter Maydell
2012-10-29 12:05 ` [Qemu-devel] [PATCH 1/3] m68k: Return semihosting errno values correctly Peter Maydell
` (3 more replies)
0 siblings, 4 replies; 5+ messages in thread
From: Peter Maydell @ 2012-10-29 12:05 UTC (permalink / raw)
To: qemu-devel; +Cc: Blue Swirl, Meador Inge, Paul Brook, patches
This patch series cleans up the m68k semihosting support to not
ignore failure of get_user and put_user when reading semihosting
arguments and writing return values (compare f296c0d1 which did
something similar for ARM semihosting). The main motivation for
this patch is to shut up clang's complaints about 'expression
result unused'. Tested with a simple m68k hello world semihosting
binary created with the CodeSourcery coldfire toolchain:
~/freescale-coldfire-2011.09/bin/m68k-elf-gcc -T m5206ec3-ram-hosted.ld -o /tmp/hello /tmp/hello.c -lc -lcs3hosted
./m68k-softmmu/qemu-system-m68k -semihosting -M dummy -display none -kernel /tmp/hello
Meador's patch is already in qemu-trivial but I include it
here as it is a dependency.
Meador Inge (1):
m68k: Return semihosting errno values correctly
Peter Maydell (2):
target-m68k/m68k-semi: Handle get_user failure
target-m68k/m68k-semi.c: Log when put_user for returning values fails
target-m68k/m68k-semi.c | 191 +++++++++++++++++++++++++++++++-----------------
1 file changed, 124 insertions(+), 67 deletions(-)
--
1.7.11.4
^ permalink raw reply [flat|nested] 5+ messages in thread
* [Qemu-devel] [PATCH 1/3] m68k: Return semihosting errno values correctly
2012-10-29 12:05 [Qemu-devel] [PATCH 0/3] target-m68k/m68k-semi: don't ignore get/put_user failure Peter Maydell
@ 2012-10-29 12:05 ` Peter Maydell
2012-10-29 12:05 ` [Qemu-devel] [PATCH 2/3] target-m68k/m68k-semi: Handle get_user failure Peter Maydell
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Peter Maydell @ 2012-10-29 12:05 UTC (permalink / raw)
To: qemu-devel; +Cc: Blue Swirl, Meador Inge, Paul Brook, patches
From: Meador Inge <meadori@codesourcery.com>
Fixing a simple typo, s/errno/err/, that caused
the error status from GDB semihosted system calls
to be returned incorrectly.
Signed-off-by: Meador Inge <meadori@codesourcery.com>
Reviewed-by: Andreas Färber <afaerber@suse.de>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
target-m68k/m68k-semi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target-m68k/m68k-semi.c b/target-m68k/m68k-semi.c
index 3bb30cd..fed44ea 100644
--- a/target-m68k/m68k-semi.c
+++ b/target-m68k/m68k-semi.c
@@ -150,7 +150,7 @@ static void m68k_semi_cb(CPUM68KState *env, target_ulong ret, target_ulong err)
}
/* FIXME - handle put_user() failure */
put_user_u32(ret, args);
- put_user_u32(errno, args + 4);
+ put_user_u32(err, args + 4);
}
#define ARG(n) \
--
1.7.11.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [Qemu-devel] [PATCH 2/3] target-m68k/m68k-semi: Handle get_user failure
2012-10-29 12:05 [Qemu-devel] [PATCH 0/3] target-m68k/m68k-semi: don't ignore get/put_user failure Peter Maydell
2012-10-29 12:05 ` [Qemu-devel] [PATCH 1/3] m68k: Return semihosting errno values correctly Peter Maydell
@ 2012-10-29 12:05 ` Peter Maydell
2012-10-29 12:05 ` [Qemu-devel] [PATCH 3/3] target-m68k/m68k-semi.c: Log when put_user for returning values fails Peter Maydell
2012-11-03 12:51 ` [Qemu-devel] [PATCH 0/3] target-m68k/m68k-semi: don't ignore get/put_user failure Blue Swirl
3 siblings, 0 replies; 5+ messages in thread
From: Peter Maydell @ 2012-10-29 12:05 UTC (permalink / raw)
To: qemu-devel; +Cc: Blue Swirl, Meador Inge, Paul Brook, patches
Handle failure of get_user accessing the semihosting
argument block, rather than simply ignoring the failures.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
target-m68k/m68k-semi.c | 144 +++++++++++++++++++++++++++++++-----------------
1 file changed, 93 insertions(+), 51 deletions(-)
diff --git a/target-m68k/m68k-semi.c b/target-m68k/m68k-semi.c
index fed44ea..d569bf1 100644
--- a/target-m68k/m68k-semi.c
+++ b/target-m68k/m68k-semi.c
@@ -153,17 +153,21 @@ static void m68k_semi_cb(CPUM68KState *env, target_ulong ret, target_ulong err)
put_user_u32(err, args + 4);
}
-#define ARG(n) \
-({ \
- target_ulong __arg; \
- /* FIXME - handle get_user() failure */ \
- get_user_ual(__arg, args + (n) * 4); \
- __arg; \
-})
-#define PARG(x) ((unsigned long)ARG(x))
+/* Read the input value from the argument block; fail the semihosting
+ * call if the memory read fails.
+ */
+#define GET_ARG(n) do { \
+ if (get_user_ual(arg ## n, args + (n) * 4)) { \
+ result = -1; \
+ errno = EFAULT; \
+ goto failed; \
+ } \
+} while (0)
+
void do_m68k_semihosting(CPUM68KState *env, int nr)
{
uint32_t args;
+ target_ulong arg0, arg1, arg2, arg3;
void *p;
void *q;
uint32_t len;
@@ -175,27 +179,33 @@ void do_m68k_semihosting(CPUM68KState *env, int nr)
gdb_exit(env, env->dregs[0]);
exit(env->dregs[0]);
case HOSTED_OPEN:
+ GET_ARG(0);
+ GET_ARG(1);
+ GET_ARG(2);
+ GET_ARG(3);
if (use_gdb_syscalls()) {
- gdb_do_syscall(m68k_semi_cb, "open,%s,%x,%x", ARG(0), (int)ARG(1),
- ARG(2), ARG(3));
+ gdb_do_syscall(m68k_semi_cb, "open,%s,%x,%x", arg0, (int)arg1,
+ arg2, arg3);
return;
} else {
- if (!(p = lock_user_string(ARG(0)))) {
+ p = lock_user_string(arg0);
+ if (!p) {
/* FIXME - check error code? */
result = -1;
} else {
- result = open(p, translate_openflags(ARG(2)), ARG(3));
- unlock_user(p, ARG(0), 0);
+ result = open(p, translate_openflags(arg2), arg3);
+ unlock_user(p, arg0, 0);
}
}
break;
case HOSTED_CLOSE:
{
/* Ignore attempts to close stdin/out/err. */
- int fd = ARG(0);
+ GET_ARG(0);
+ int fd = arg0;
if (fd > 2) {
if (use_gdb_syscalls()) {
- gdb_do_syscall(m68k_semi_cb, "close,%x", ARG(0));
+ gdb_do_syscall(m68k_semi_cb, "close,%x", arg0);
return;
} else {
result = close(fd);
@@ -206,47 +216,59 @@ void do_m68k_semihosting(CPUM68KState *env, int nr)
break;
}
case HOSTED_READ:
- len = ARG(2);
+ GET_ARG(0);
+ GET_ARG(1);
+ GET_ARG(2);
+ len = arg2;
if (use_gdb_syscalls()) {
gdb_do_syscall(m68k_semi_cb, "read,%x,%x,%x",
- ARG(0), ARG(1), len);
+ arg0, arg1, len);
return;
} else {
- if (!(p = lock_user(VERIFY_WRITE, ARG(1), len, 0))) {
+ p = lock_user(VERIFY_WRITE, arg1, len, 0);
+ if (!p) {
/* FIXME - check error code? */
result = -1;
} else {
- result = read(ARG(0), p, len);
- unlock_user(p, ARG(1), len);
+ result = read(arg0, p, len);
+ unlock_user(p, arg1, len);
}
}
break;
case HOSTED_WRITE:
- len = ARG(2);
+ GET_ARG(0);
+ GET_ARG(1);
+ GET_ARG(2);
+ len = arg2;
if (use_gdb_syscalls()) {
gdb_do_syscall(m68k_semi_cb, "write,%x,%x,%x",
- ARG(0), ARG(1), len);
+ arg0, arg1, len);
return;
} else {
- if (!(p = lock_user(VERIFY_READ, ARG(1), len, 1))) {
+ p = lock_user(VERIFY_READ, arg1, len, 1);
+ if (!p) {
/* FIXME - check error code? */
result = -1;
} else {
- result = write(ARG(0), p, len);
- unlock_user(p, ARG(0), 0);
+ result = write(arg0, p, len);
+ unlock_user(p, arg0, 0);
}
}
break;
case HOSTED_LSEEK:
{
uint64_t off;
- off = (uint32_t)ARG(2) | ((uint64_t)ARG(1) << 32);
+ GET_ARG(0);
+ GET_ARG(1);
+ GET_ARG(2);
+ GET_ARG(3);
+ off = (uint32_t)arg2 | ((uint64_t)arg1 << 32);
if (use_gdb_syscalls()) {
m68k_semi_is_fseek = 1;
gdb_do_syscall(m68k_semi_cb, "fseek,%x,%lx,%x",
- ARG(0), off, ARG(3));
+ arg0, off, arg3);
} else {
- off = lseek(ARG(0), off, ARG(3));
+ off = lseek(arg0, off, arg3);
/* FIXME - handle put_user() failure */
put_user_u32(off >> 32, args);
put_user_u32(off, args + 4);
@@ -255,74 +277,89 @@ void do_m68k_semihosting(CPUM68KState *env, int nr)
return;
}
case HOSTED_RENAME:
+ GET_ARG(0);
+ GET_ARG(1);
+ GET_ARG(2);
+ GET_ARG(3);
if (use_gdb_syscalls()) {
gdb_do_syscall(m68k_semi_cb, "rename,%s,%s",
- ARG(0), (int)ARG(1), ARG(2), (int)ARG(3));
+ arg0, (int)arg1, arg2, (int)arg3);
return;
} else {
- p = lock_user_string(ARG(0));
- q = lock_user_string(ARG(2));
+ p = lock_user_string(arg0);
+ q = lock_user_string(arg2);
if (!p || !q) {
/* FIXME - check error code? */
result = -1;
} else {
result = rename(p, q);
}
- unlock_user(p, ARG(0), 0);
- unlock_user(q, ARG(2), 0);
+ unlock_user(p, arg0, 0);
+ unlock_user(q, arg2, 0);
}
break;
case HOSTED_UNLINK:
+ GET_ARG(0);
+ GET_ARG(1);
if (use_gdb_syscalls()) {
gdb_do_syscall(m68k_semi_cb, "unlink,%s",
- ARG(0), (int)ARG(1));
+ arg0, (int)arg1);
return;
} else {
- if (!(p = lock_user_string(ARG(0)))) {
+ p = lock_user_string(arg0);
+ if (!p) {
/* FIXME - check error code? */
result = -1;
} else {
result = unlink(p);
- unlock_user(p, ARG(0), 0);
+ unlock_user(p, arg0, 0);
}
}
break;
case HOSTED_STAT:
+ GET_ARG(0);
+ GET_ARG(1);
+ GET_ARG(2);
if (use_gdb_syscalls()) {
gdb_do_syscall(m68k_semi_cb, "stat,%s,%x",
- ARG(0), (int)ARG(1), ARG(2));
+ arg0, (int)arg1, arg2);
return;
} else {
struct stat s;
- if (!(p = lock_user_string(ARG(0)))) {
+ p = lock_user_string(arg0);
+ if (!p) {
/* FIXME - check error code? */
result = -1;
} else {
result = stat(p, &s);
- unlock_user(p, ARG(0), 0);
+ unlock_user(p, arg0, 0);
}
if (result == 0) {
- translate_stat(env, ARG(2), &s);
+ translate_stat(env, arg2, &s);
}
}
break;
case HOSTED_FSTAT:
+ GET_ARG(0);
+ GET_ARG(1);
if (use_gdb_syscalls()) {
gdb_do_syscall(m68k_semi_cb, "fstat,%x,%x",
- ARG(0), ARG(1));
+ arg0, arg1);
return;
} else {
struct stat s;
- result = fstat(ARG(0), &s);
+ result = fstat(arg0, &s);
if (result == 0) {
- translate_stat(env, ARG(1), &s);
+ translate_stat(env, arg1, &s);
}
}
break;
case HOSTED_GETTIMEOFDAY:
+ GET_ARG(0);
+ GET_ARG(1);
if (use_gdb_syscalls()) {
gdb_do_syscall(m68k_semi_cb, "gettimeofday,%x,%x",
- ARG(0), ARG(1));
+ arg0, arg1);
return;
} else {
qemu_timeval tv;
@@ -330,37 +367,41 @@ void do_m68k_semihosting(CPUM68KState *env, int nr)
result = qemu_gettimeofday(&tv);
if (result != 0) {
if (!(p = lock_user(VERIFY_WRITE,
- ARG(0), sizeof(struct gdb_timeval), 0))) {
+ arg0, sizeof(struct gdb_timeval), 0))) {
/* FIXME - check error code? */
result = -1;
} else {
p->tv_sec = cpu_to_be32(tv.tv_sec);
p->tv_usec = cpu_to_be64(tv.tv_usec);
- unlock_user(p, ARG(0), sizeof(struct gdb_timeval));
+ unlock_user(p, arg0, sizeof(struct gdb_timeval));
}
}
}
break;
case HOSTED_ISATTY:
+ GET_ARG(0);
if (use_gdb_syscalls()) {
- gdb_do_syscall(m68k_semi_cb, "isatty,%x", ARG(0));
+ gdb_do_syscall(m68k_semi_cb, "isatty,%x", arg0);
return;
} else {
- result = isatty(ARG(0));
+ result = isatty(arg0);
}
break;
case HOSTED_SYSTEM:
+ GET_ARG(0);
+ GET_ARG(1);
if (use_gdb_syscalls()) {
gdb_do_syscall(m68k_semi_cb, "system,%s",
- ARG(0), (int)ARG(1));
+ arg0, (int)arg1);
return;
} else {
- if (!(p = lock_user_string(ARG(0)))) {
+ p = lock_user_string(arg0);
+ if (!p) {
/* FIXME - check error code? */
result = -1;
} else {
result = system(p);
- unlock_user(p, ARG(0), 0);
+ unlock_user(p, arg0, 0);
}
}
break;
@@ -402,6 +443,7 @@ void do_m68k_semihosting(CPUM68KState *env, int nr)
cpu_abort(env, "Unsupported semihosting syscall %d\n", nr);
result = 0;
}
+failed:
/* FIXME - handle put_user() failure */
put_user_u32(result, args);
put_user_u32(errno, args + 4);
--
1.7.11.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [Qemu-devel] [PATCH 3/3] target-m68k/m68k-semi.c: Log when put_user for returning values fails
2012-10-29 12:05 [Qemu-devel] [PATCH 0/3] target-m68k/m68k-semi: don't ignore get/put_user failure Peter Maydell
2012-10-29 12:05 ` [Qemu-devel] [PATCH 1/3] m68k: Return semihosting errno values correctly Peter Maydell
2012-10-29 12:05 ` [Qemu-devel] [PATCH 2/3] target-m68k/m68k-semi: Handle get_user failure Peter Maydell
@ 2012-10-29 12:05 ` Peter Maydell
2012-11-03 12:51 ` [Qemu-devel] [PATCH 0/3] target-m68k/m68k-semi: don't ignore get/put_user failure Blue Swirl
3 siblings, 0 replies; 5+ messages in thread
From: Peter Maydell @ 2012-10-29 12:05 UTC (permalink / raw)
To: qemu-devel; +Cc: Blue Swirl, Meador Inge, Paul Brook, patches
Abstract out the use of put_user for returning semihosting call results,
so that we can log when a guest erroneously attempts a semihosting call
with an unwritable argument block.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
target-m68k/m68k-semi.c | 47 +++++++++++++++++++++++++++++++----------------
1 file changed, 31 insertions(+), 16 deletions(-)
diff --git a/target-m68k/m68k-semi.c b/target-m68k/m68k-semi.c
index d569bf1..9f7a24c 100644
--- a/target-m68k/m68k-semi.c
+++ b/target-m68k/m68k-semi.c
@@ -133,24 +133,44 @@ static void translate_stat(CPUM68KState *env, target_ulong addr, struct stat *s)
unlock_user(p, addr, sizeof(struct m68k_gdb_stat));
}
+static void m68k_semi_return_u32(CPUM68KState *env, uint32_t ret, uint32_t err)
+{
+ target_ulong args = env->dregs[1];
+ if (put_user_u32(ret, args) ||
+ put_user_u32(err, args + 4)) {
+ /* The m68k semihosting ABI does not provide any way to report this
+ * error to the guest, so the best we can do is log it in qemu.
+ * It is always a guest error not to pass us a valid argument block.
+ */
+ qemu_log_mask(LOG_GUEST_ERROR, "m68k-semihosting: return value "
+ "discarded because argument block not writable\n");
+ }
+}
+
+static void m68k_semi_return_u64(CPUM68KState *env, uint64_t ret, uint32_t err)
+{
+ target_ulong args = env->dregs[1];
+ if (put_user_u32(ret >> 32, args) ||
+ put_user_u32(ret, args + 4) ||
+ put_user_u32(err, args + 8)) {
+ /* No way to report this via m68k semihosting ABI; just log it */
+ qemu_log_mask(LOG_GUEST_ERROR, "m68k-semihosting: return value "
+ "discarded because argument block not writable\n");
+ }
+}
+
static int m68k_semi_is_fseek;
static void m68k_semi_cb(CPUM68KState *env, target_ulong ret, target_ulong err)
{
- target_ulong args;
-
- args = env->dregs[1];
if (m68k_semi_is_fseek) {
/* FIXME: We've already lost the high bits of the fseek
return value. */
- /* FIXME - handle put_user() failure */
- put_user_u32(0, args);
- args += 4;
+ m68k_semi_return_u64(env, ret, err);
m68k_semi_is_fseek = 0;
+ } else {
+ m68k_semi_return_u32(env, ret, err);
}
- /* FIXME - handle put_user() failure */
- put_user_u32(ret, args);
- put_user_u32(err, args + 4);
}
/* Read the input value from the argument block; fail the semihosting
@@ -269,10 +289,7 @@ void do_m68k_semihosting(CPUM68KState *env, int nr)
arg0, off, arg3);
} else {
off = lseek(arg0, off, arg3);
- /* FIXME - handle put_user() failure */
- put_user_u32(off >> 32, args);
- put_user_u32(off, args + 4);
- put_user_u32(errno, args + 8);
+ m68k_semi_return_u64(env, off, errno);
}
return;
}
@@ -444,7 +461,5 @@ void do_m68k_semihosting(CPUM68KState *env, int nr)
result = 0;
}
failed:
- /* FIXME - handle put_user() failure */
- put_user_u32(result, args);
- put_user_u32(errno, args + 4);
+ m68k_semi_return_u32(env, result, errno);
}
--
1.7.11.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH 0/3] target-m68k/m68k-semi: don't ignore get/put_user failure
2012-10-29 12:05 [Qemu-devel] [PATCH 0/3] target-m68k/m68k-semi: don't ignore get/put_user failure Peter Maydell
` (2 preceding siblings ...)
2012-10-29 12:05 ` [Qemu-devel] [PATCH 3/3] target-m68k/m68k-semi.c: Log when put_user for returning values fails Peter Maydell
@ 2012-11-03 12:51 ` Blue Swirl
3 siblings, 0 replies; 5+ messages in thread
From: Blue Swirl @ 2012-11-03 12:51 UTC (permalink / raw)
To: Peter Maydell; +Cc: Paul Brook, Meador Inge, qemu-devel, patches
On Mon, Oct 29, 2012 at 12:05 PM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> This patch series cleans up the m68k semihosting support to not
> ignore failure of get_user and put_user when reading semihosting
> arguments and writing return values (compare f296c0d1 which did
> something similar for ARM semihosting). The main motivation for
> this patch is to shut up clang's complaints about 'expression
> result unused'. Tested with a simple m68k hello world semihosting
> binary created with the CodeSourcery coldfire toolchain:
> ~/freescale-coldfire-2011.09/bin/m68k-elf-gcc -T m5206ec3-ram-hosted.ld -o /tmp/hello /tmp/hello.c -lc -lcs3hosted
> ./m68k-softmmu/qemu-system-m68k -semihosting -M dummy -display none -kernel /tmp/hello
>
> Meador's patch is already in qemu-trivial but I include it
> here as it is a dependency.
Thanks, applied all.
>
>
> Meador Inge (1):
> m68k: Return semihosting errno values correctly
>
> Peter Maydell (2):
> target-m68k/m68k-semi: Handle get_user failure
> target-m68k/m68k-semi.c: Log when put_user for returning values fails
>
> target-m68k/m68k-semi.c | 191 +++++++++++++++++++++++++++++++-----------------
> 1 file changed, 124 insertions(+), 67 deletions(-)
>
> --
> 1.7.11.4
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-11-03 12:51 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-10-29 12:05 [Qemu-devel] [PATCH 0/3] target-m68k/m68k-semi: don't ignore get/put_user failure Peter Maydell
2012-10-29 12:05 ` [Qemu-devel] [PATCH 1/3] m68k: Return semihosting errno values correctly Peter Maydell
2012-10-29 12:05 ` [Qemu-devel] [PATCH 2/3] target-m68k/m68k-semi: Handle get_user failure Peter Maydell
2012-10-29 12:05 ` [Qemu-devel] [PATCH 3/3] target-m68k/m68k-semi.c: Log when put_user for returning values fails Peter Maydell
2012-11-03 12:51 ` [Qemu-devel] [PATCH 0/3] target-m68k/m68k-semi: don't ignore get/put_user failure Blue Swirl
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).