* [Qemu-devel] [PATCH 01/13] target/arm/arm-semi: Capture errno in softmmu version of set_swi_errno()
2019-09-10 14:44 [Qemu-devel] [PATCH 00/13] target/arm: Implement semihosting v2.0 Peter Maydell
@ 2019-09-10 14:44 ` Peter Maydell
2019-09-12 10:39 ` [Qemu-devel] [Qemu-arm] " Alex Bennée
2019-09-10 14:44 ` [Qemu-devel] [PATCH 02/13] target/arm/arm-semi: Always set some kind of errno for failed calls Peter Maydell
` (11 subsequent siblings)
12 siblings, 1 reply; 33+ messages in thread
From: Peter Maydell @ 2019-09-10 14:44 UTC (permalink / raw)
To: qemu-arm, qemu-devel
The set_swi_errno() function is called to capture the errno
from a host system call, so that we can return -1 from the
semihosting function and later allow the guest to get a more
specific error code with the SYS_ERRNO function. It comes in
two versions, one for user-only and one for softmmu. We forgot
to capture the errno in the softmmu version; fix the error.
(Semihosting calls directed to gdb are unaffected because
they go through a different code path that captures the
error return from the gdbstub call in arm_semi_cb() or
arm_semi_flen_cb().)
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
NB that a later commit will put in some cleanup of TaskState
that will let us reduce the duplication between the two
implementations of this function.
---
target/arm/arm-semi.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
index 90423a35deb..03e60105c05 100644
--- a/target/arm/arm-semi.c
+++ b/target/arm/arm-semi.c
@@ -114,8 +114,13 @@ static inline uint32_t set_swi_errno(TaskState *ts, uint32_t code)
return code;
}
#else
+static target_ulong syscall_err;
+
static inline uint32_t set_swi_errno(CPUARMState *env, uint32_t code)
{
+ if (code == (uint32_t)-1) {
+ syscall_err = errno;
+ }
return code;
}
@@ -124,10 +129,6 @@ static inline uint32_t set_swi_errno(CPUARMState *env, uint32_t code)
static target_ulong arm_semi_syscall_len;
-#if !defined(CONFIG_USER_ONLY)
-static target_ulong syscall_err;
-#endif
-
static void arm_semi_cb(CPUState *cs, target_ulong ret, target_ulong err)
{
ARMCPU *cpu = ARM_CPU(cs);
--
2.20.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [Qemu-arm] [PATCH 01/13] target/arm/arm-semi: Capture errno in softmmu version of set_swi_errno()
2019-09-10 14:44 ` [Qemu-devel] [PATCH 01/13] target/arm/arm-semi: Capture errno in softmmu version of set_swi_errno() Peter Maydell
@ 2019-09-12 10:39 ` Alex Bennée
2019-09-12 10:49 ` Peter Maydell
0 siblings, 1 reply; 33+ messages in thread
From: Alex Bennée @ 2019-09-12 10:39 UTC (permalink / raw)
To: qemu-arm; +Cc: qemu-devel
Peter Maydell <peter.maydell@linaro.org> writes:
> The set_swi_errno() function is called to capture the errno
> from a host system call, so that we can return -1 from the
> semihosting function and later allow the guest to get a more
> specific error code with the SYS_ERRNO function. It comes in
> two versions, one for user-only and one for softmmu. We forgot
> to capture the errno in the softmmu version; fix the error.
>
> (Semihosting calls directed to gdb are unaffected because
> they go through a different code path that captures the
> error return from the gdbstub call in arm_semi_cb() or
> arm_semi_flen_cb().)
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> NB that a later commit will put in some cleanup of TaskState
> that will let us reduce the duplication between the two
> implementations of this function.
> ---
> target/arm/arm-semi.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
> index 90423a35deb..03e60105c05 100644
> --- a/target/arm/arm-semi.c
> +++ b/target/arm/arm-semi.c
> @@ -114,8 +114,13 @@ static inline uint32_t set_swi_errno(TaskState *ts, uint32_t code)
> return code;
> }
> #else
> +static target_ulong syscall_err;
> +
I appreciate that this is just moving things around but this will be
broken for -smp > 1 if two vCPUs make a syscall at the same time. For
linux-user this information is kept in ts->swi_errno which is
per-thread. Either we need a __thread version or find somewhere in
CPUARMState to store it.
> static inline uint32_t set_swi_errno(CPUARMState *env, uint32_t code)
> {
> + if (code == (uint32_t)-1) {
> + syscall_err = errno;
> + }
> return code;
> }
>
> @@ -124,10 +129,6 @@ static inline uint32_t set_swi_errno(CPUARMState *env, uint32_t code)
>
> static target_ulong arm_semi_syscall_len;
>
> -#if !defined(CONFIG_USER_ONLY)
> -static target_ulong syscall_err;
> -#endif
> -
> static void arm_semi_cb(CPUState *cs, target_ulong ret, target_ulong err)
> {
> ARMCPU *cpu = ARM_CPU(cs);
--
Alex Bennée
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [Qemu-arm] [PATCH 01/13] target/arm/arm-semi: Capture errno in softmmu version of set_swi_errno()
2019-09-12 10:39 ` [Qemu-devel] [Qemu-arm] " Alex Bennée
@ 2019-09-12 10:49 ` Peter Maydell
0 siblings, 0 replies; 33+ messages in thread
From: Peter Maydell @ 2019-09-12 10:49 UTC (permalink / raw)
To: Alex Bennée; +Cc: qemu-arm, QEMU Developers
On Thu, 12 Sep 2019 at 11:40, Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > The set_swi_errno() function is called to capture the errno
> > from a host system call, so that we can return -1 from the
> > semihosting function and later allow the guest to get a more
> > specific error code with the SYS_ERRNO function. It comes in
> > two versions, one for user-only and one for softmmu. We forgot
> > to capture the errno in the softmmu version; fix the error.
> >
> > (Semihosting calls directed to gdb are unaffected because
> > they go through a different code path that captures the
> > error return from the gdbstub call in arm_semi_cb() or
> > arm_semi_flen_cb().)
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> > NB that a later commit will put in some cleanup of TaskState
> > that will let us reduce the duplication between the two
> > implementations of this function.
> > ---
> > target/arm/arm-semi.c | 9 +++++----
> > 1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
> > index 90423a35deb..03e60105c05 100644
> > --- a/target/arm/arm-semi.c
> > +++ b/target/arm/arm-semi.c
> > @@ -114,8 +114,13 @@ static inline uint32_t set_swi_errno(TaskState *ts, uint32_t code)
> > return code;
> > }
> > #else
> > +static target_ulong syscall_err;
> > +
>
> I appreciate that this is just moving things around but this will be
> broken for -smp > 1 if two vCPUs make a syscall at the same time. For
> linux-user this information is kept in ts->swi_errno which is
> per-thread. Either we need a __thread version or find somewhere in
> CPUARMState to store it.
The semihosting API has no concept of errno being per-CPU at all.
(It really predates SMP entirely, so nobody was thinking about
that, and in any case it's a debug API, not a serious-work one).
The assumption is that the guest is handling this somehow.
The errno interface is not very useful anyway since it doesn't
actually define any errno values, and it's even worse in our
implementation because we use the host errno values rather
than defining some clean target-specific set of values...
thanks
-- PMM
^ permalink raw reply [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH 02/13] target/arm/arm-semi: Always set some kind of errno for failed calls
2019-09-10 14:44 [Qemu-devel] [PATCH 00/13] target/arm: Implement semihosting v2.0 Peter Maydell
2019-09-10 14:44 ` [Qemu-devel] [PATCH 01/13] target/arm/arm-semi: Capture errno in softmmu version of set_swi_errno() Peter Maydell
@ 2019-09-10 14:44 ` Peter Maydell
2019-09-12 10:42 ` [Qemu-devel] [Qemu-arm] " Alex Bennée
2019-09-10 14:44 ` [Qemu-devel] [PATCH 03/13] target/arm/arm-semi: Make semihosting code hand out its own file descriptors Peter Maydell
` (10 subsequent siblings)
12 siblings, 1 reply; 33+ messages in thread
From: Peter Maydell @ 2019-09-10 14:44 UTC (permalink / raw)
To: qemu-arm, qemu-devel
If we fail a semihosting call we should always set the
semihosting errno to something; we were failing to do
this for some of the "check inputs for sanity" cases.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
target/arm/arm-semi.c | 45 ++++++++++++++++++++++++++-----------------
1 file changed, 27 insertions(+), 18 deletions(-)
diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
index 03e60105c05..51b55816faf 100644
--- a/target/arm/arm-semi.c
+++ b/target/arm/arm-semi.c
@@ -232,11 +232,13 @@ static target_ulong arm_gdb_syscall(ARMCPU *cpu, gdb_syscall_complete_cb cb,
#define GET_ARG(n) do { \
if (is_a64(env)) { \
if (get_user_u64(arg ## n, args + (n) * 8)) { \
- return -1; \
+ errno = EFAULT; \
+ return set_swi_errno(ts, -1); \
} \
} else { \
if (get_user_u32(arg ## n, args + (n) * 4)) { \
- return -1; \
+ errno = EFAULT; \
+ return set_swi_errno(ts, -1); \
} \
} \
} while (0)
@@ -287,12 +289,13 @@ target_ulong do_arm_semihosting(CPUARMState *env)
GET_ARG(2);
s = lock_user_string(arg0);
if (!s) {
- /* FIXME - should this error code be -TARGET_EFAULT ? */
- return (uint32_t)-1;
+ errno = EFAULT;
+ return set_swi_errno(ts, -1);
}
if (arg1 >= 12) {
unlock_user(s, arg0, 0);
- return (uint32_t)-1;
+ errno = EINVAL;
+ return set_swi_errno(ts, -1);
}
if (strcmp(s, ":tt") == 0) {
int result_fileno = arg1 < 4 ? STDIN_FILENO : STDOUT_FILENO;
@@ -413,8 +416,8 @@ target_ulong do_arm_semihosting(CPUARMState *env)
} else {
s = lock_user_string(arg0);
if (!s) {
- /* FIXME - should this error code be -TARGET_EFAULT ? */
- return (uint32_t)-1;
+ errno = EFAULT;
+ return set_swi_errno(ts, -1);
}
ret = set_swi_errno(ts, remove(s));
unlock_user(s, arg0, 0);
@@ -432,11 +435,12 @@ target_ulong do_arm_semihosting(CPUARMState *env)
char *s2;
s = lock_user_string(arg0);
s2 = lock_user_string(arg2);
- if (!s || !s2)
- /* FIXME - should this error code be -TARGET_EFAULT ? */
- ret = (uint32_t)-1;
- else
+ if (!s || !s2) {
+ errno = EFAULT;
+ ret = set_swi_errno(ts, -1);
+ } else {
ret = set_swi_errno(ts, rename(s, s2));
+ }
if (s2)
unlock_user(s2, arg2, 0);
if (s)
@@ -456,8 +460,8 @@ target_ulong do_arm_semihosting(CPUARMState *env)
} else {
s = lock_user_string(arg0);
if (!s) {
- /* FIXME - should this error code be -TARGET_EFAULT ? */
- return (uint32_t)-1;
+ errno = EFAULT;
+ return set_swi_errno(ts, -1);
}
ret = set_swi_errno(ts, system(s));
unlock_user(s, arg0, 0);
@@ -517,19 +521,22 @@ target_ulong do_arm_semihosting(CPUARMState *env)
if (output_size > input_size) {
/* Not enough space to store command-line arguments. */
- return -1;
+ errno = E2BIG;
+ return set_swi_errno(ts, -1);
}
/* Adjust the command-line length. */
if (SET_ARG(1, output_size - 1)) {
/* Couldn't write back to argument block */
- return -1;
+ errno = EFAULT;
+ return set_swi_errno(ts, -1);
}
/* Lock the buffer on the ARM side. */
output_buffer = lock_user(VERIFY_WRITE, arg0, output_size, 0);
if (!output_buffer) {
- return -1;
+ errno = EFAULT;
+ return set_swi_errno(ts, -1);
}
/* Copy the command-line arguments. */
@@ -544,7 +551,8 @@ target_ulong do_arm_semihosting(CPUARMState *env)
if (copy_from_user(output_buffer, ts->info->arg_start,
output_size)) {
- status = -1;
+ errno = EFAULT;
+ status = set_swi_errno(ts, -1);
goto out;
}
@@ -614,7 +622,8 @@ target_ulong do_arm_semihosting(CPUARMState *env)
if (fail) {
/* Couldn't write back to argument block */
- return -1;
+ errno = EFAULT;
+ return set_swi_errno(ts, -1);
}
}
return 0;
--
2.20.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [Qemu-arm] [PATCH 02/13] target/arm/arm-semi: Always set some kind of errno for failed calls
2019-09-10 14:44 ` [Qemu-devel] [PATCH 02/13] target/arm/arm-semi: Always set some kind of errno for failed calls Peter Maydell
@ 2019-09-12 10:42 ` Alex Bennée
2019-09-12 10:50 ` Peter Maydell
0 siblings, 1 reply; 33+ messages in thread
From: Alex Bennée @ 2019-09-12 10:42 UTC (permalink / raw)
To: qemu-arm; +Cc: qemu-devel
Peter Maydell <peter.maydell@linaro.org> writes:
> If we fail a semihosting call we should always set the
> semihosting errno to something; we were failing to do
> this for some of the "check inputs for sanity" cases.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
although:
> ---
> target/arm/arm-semi.c | 45 ++++++++++++++++++++++++++-----------------
> 1 file changed, 27 insertions(+), 18 deletions(-)
>
> diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
> index 03e60105c05..51b55816faf 100644
> --- a/target/arm/arm-semi.c
> +++ b/target/arm/arm-semi.c
> @@ -232,11 +232,13 @@ static target_ulong arm_gdb_syscall(ARMCPU *cpu, gdb_syscall_complete_cb cb,
> #define GET_ARG(n) do { \
> if (is_a64(env)) { \
> if (get_user_u64(arg ## n, args + (n) * 8)) { \
> - return -1; \
> + errno = EFAULT; \
> + return set_swi_errno(ts, -1); \
This looks a little queasy given ts is NULL for the softmmu version. I
wonder (depending on your approach to -smp for 1/13) if we should just
pass the ARMCPU down to the helper?
--
Alex Bennée
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [Qemu-arm] [PATCH 02/13] target/arm/arm-semi: Always set some kind of errno for failed calls
2019-09-12 10:42 ` [Qemu-devel] [Qemu-arm] " Alex Bennée
@ 2019-09-12 10:50 ` Peter Maydell
2019-09-12 11:09 ` Alex Bennée
0 siblings, 1 reply; 33+ messages in thread
From: Peter Maydell @ 2019-09-12 10:50 UTC (permalink / raw)
To: Alex Bennée; +Cc: qemu-arm, QEMU Developers
On Thu, 12 Sep 2019 at 11:42, Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > If we fail a semihosting call we should always set the
> > semihosting errno to something; we were failing to do
> > this for some of the "check inputs for sanity" cases.
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>
> although:
>
> > ---
> > target/arm/arm-semi.c | 45 ++++++++++++++++++++++++++-----------------
> > 1 file changed, 27 insertions(+), 18 deletions(-)
> >
> > diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
> > index 03e60105c05..51b55816faf 100644
> > --- a/target/arm/arm-semi.c
> > +++ b/target/arm/arm-semi.c
> > @@ -232,11 +232,13 @@ static target_ulong arm_gdb_syscall(ARMCPU *cpu, gdb_syscall_complete_cb cb,
> > #define GET_ARG(n) do { \
> > if (is_a64(env)) { \
> > if (get_user_u64(arg ## n, args + (n) * 8)) { \
> > - return -1; \
> > + errno = EFAULT; \
> > + return set_swi_errno(ts, -1); \
>
> This looks a little queasy given ts is NULL for the softmmu version. I
> wonder (depending on your approach to -smp for 1/13) if we should just
> pass the ARMCPU down to the helper?
NULL is fine because the softmmu version of set_swi_errno() doesn't
do anything with it anyway, right?
thanks
-- PMM
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [Qemu-arm] [PATCH 02/13] target/arm/arm-semi: Always set some kind of errno for failed calls
2019-09-12 10:50 ` Peter Maydell
@ 2019-09-12 11:09 ` Alex Bennée
0 siblings, 0 replies; 33+ messages in thread
From: Alex Bennée @ 2019-09-12 11:09 UTC (permalink / raw)
To: Peter Maydell; +Cc: qemu-arm, QEMU Developers
Peter Maydell <peter.maydell@linaro.org> writes:
> On Thu, 12 Sep 2019 at 11:42, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>>
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>> > If we fail a semihosting call we should always set the
>> > semihosting errno to something; we were failing to do
>> > this for some of the "check inputs for sanity" cases.
>> >
>> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>
>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>>
>> although:
>>
>> > ---
>> > target/arm/arm-semi.c | 45 ++++++++++++++++++++++++++-----------------
>> > 1 file changed, 27 insertions(+), 18 deletions(-)
>> >
>> > diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
>> > index 03e60105c05..51b55816faf 100644
>> > --- a/target/arm/arm-semi.c
>> > +++ b/target/arm/arm-semi.c
>> > @@ -232,11 +232,13 @@ static target_ulong arm_gdb_syscall(ARMCPU *cpu, gdb_syscall_complete_cb cb,
>> > #define GET_ARG(n) do { \
>> > if (is_a64(env)) { \
>> > if (get_user_u64(arg ## n, args + (n) * 8)) { \
>> > - return -1; \
>> > + errno = EFAULT; \
>> > + return set_swi_errno(ts, -1); \
>>
>> This looks a little queasy given ts is NULL for the softmmu version. I
>> wonder (depending on your approach to -smp for 1/13) if we should just
>> pass the ARMCPU down to the helper?
>
> NULL is fine because the softmmu version of set_swi_errno() doesn't
> do anything with it anyway, right?
Yes it's fine - it just looks a little odd when you are reading it.
Given TaskState is derived from CPUState which you always have you could
just pass cs to set_swi_errno and hide the final implementation detail
there depending on if you are softmmu or linux-user.
But it's purely a subjective style thing, not a bug hence the r-b ;-)
--
Alex Bennée
^ permalink raw reply [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH 03/13] target/arm/arm-semi: Make semihosting code hand out its own file descriptors
2019-09-10 14:44 [Qemu-devel] [PATCH 00/13] target/arm: Implement semihosting v2.0 Peter Maydell
2019-09-10 14:44 ` [Qemu-devel] [PATCH 01/13] target/arm/arm-semi: Capture errno in softmmu version of set_swi_errno() Peter Maydell
2019-09-10 14:44 ` [Qemu-devel] [PATCH 02/13] target/arm/arm-semi: Always set some kind of errno for failed calls Peter Maydell
@ 2019-09-10 14:44 ` Peter Maydell
2019-09-12 11:02 ` [Qemu-devel] [Qemu-arm] " Alex Bennée
2019-09-10 14:44 ` [Qemu-devel] [PATCH 04/13] target/arm/arm-semi: clean up TaskState* usage in non-user-only code Peter Maydell
` (9 subsequent siblings)
12 siblings, 1 reply; 33+ messages in thread
From: Peter Maydell @ 2019-09-10 14:44 UTC (permalink / raw)
To: qemu-arm, qemu-devel
Currently the Arm semihosting code returns the guest file descriptors
(handles) which are simply the fd values from the host OS or the
remote gdbstub. Part of the semihosting 2.0 specification requires
that we implement special handling of opening a ":semihosting-features"
filename. Guest fds which result from opening the special file
won't correspond to host fds, so to ensure that we don't end up
with duplicate fds we need to have QEMU code control the allocation
of the fd values we give the guest.
Add in an abstraction layer which lets us allocate new guest FD
values, and translate from a guest FD value back to the host one.
This also fixes an odd hole where a semihosting guest could
use the semihosting API to read, write or close file descriptors
that it had never allocated but which were being used by QEMU itself.
(This isn't a security hole, because enabling semihosting permits
the guest to do arbitrary file access to the whole host filesystem,
and so should only be done if the guest is completely trusted.)
Currently the only kind of guest fd is one which maps to a
host fd, but in a following commit we will add one which maps
to the :semihosting-features magic data.
If the guest is migrated with an open semihosting file descriptor
then subsequent attempts to use the fd will all fail; this is
not a change from the previous situation (where the host fd
being used on the source end would not be re-opened on the
destination end).
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
target/arm/arm-semi.c | 201 ++++++++++++++++++++++++++++++++++++++----
1 file changed, 186 insertions(+), 15 deletions(-)
diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
index 51b55816faf..05491bf5248 100644
--- a/target/arm/arm-semi.c
+++ b/target/arm/arm-semi.c
@@ -106,6 +106,113 @@ static int open_modeflags[12] = {
O_RDWR | O_CREAT | O_APPEND | O_BINARY
};
+typedef enum GuestFDType {
+ GuestFDUnused = 0,
+ GuestFDHost = 1,
+} GuestFDType;
+
+/*
+ * Guest file descriptors are integer indexes into an array of
+ * these structures (we will dynamically resize as necessary).
+ */
+typedef struct GuestFD {
+ GuestFDType type;
+ int hostfd;
+} GuestFD;
+
+static GArray *guestfd_array;
+
+/*
+ * Allocate a new guest file descriptor and return it; if we
+ * couldn't allocate a new fd then return -1.
+ * This is a fairly simplistic implementation because we don't
+ * expect that most semihosting guest programs will make very
+ * heavy use of opening and closing fds.
+ */
+static int alloc_guestfd(void)
+{
+ guint i;
+
+ if (!guestfd_array) {
+ /* New entries zero-initialized, i.e. type GuestFDUnused */
+ guestfd_array = g_array_new(FALSE, TRUE, sizeof(GuestFD));
+ }
+
+ for (i = 0; i < guestfd_array->len; i++) {
+ GuestFD *gf = &g_array_index(guestfd_array, GuestFD, i);
+
+ if (gf->type == GuestFDUnused) {
+ return i;
+ }
+ }
+
+ /* All elements already in use: expand the array */
+ g_array_set_size(guestfd_array, i + 1);
+ return i;
+}
+
+/*
+ * Look up the guestfd in the data structure; return NULL
+ * for out of bounds, but don't check whether the slot is unused.
+ * This is used internally by the other guestfd functions.
+ */
+static GuestFD *do_get_guestfd(int guestfd)
+{
+ if (!guestfd_array) {
+ return NULL;
+ }
+
+ if (guestfd < 0 || guestfd >= guestfd_array->len) {
+ return NULL;
+ }
+
+ return &g_array_index(guestfd_array, GuestFD, guestfd);
+}
+
+/*
+ * Associate the specified guest fd (which must have been
+ * allocated via alloc_fd() and not previously used) with
+ * the specified host fd.
+ */
+static void associate_guestfd(int guestfd, int hostfd)
+{
+ GuestFD *gf = do_get_guestfd(guestfd);
+
+ assert(gf);
+ gf->type = GuestFDHost;
+ gf->hostfd = hostfd;
+}
+
+/*
+ * Deallocate the specified guest file descriptor. This doesn't
+ * close the host fd, it merely undoes the work of alloc_fd().
+ */
+static void dealloc_guestfd(int guestfd)
+{
+ GuestFD *gf = do_get_guestfd(guestfd);
+
+ assert(gf);
+ gf->type = GuestFDUnused;
+}
+
+/*
+ * Given a guest file descriptor, get the associated struct.
+ * If the fd is not valid, return NULL. This is the function
+ * used by the various semihosting calls to validate a handle
+ * from the guest.
+ * Note: calling alloc_guestfd() or dealloc_guestfd() will
+ * invalidate any GuestFD* obtained by calling this function.
+ */
+static GuestFD *get_guestfd(int guestfd)
+{
+ GuestFD *gf = do_get_guestfd(guestfd);
+
+ if (!gf || gf->type == GuestFDUnused) {
+ return NULL;
+ }
+ return gf;
+}
+
#ifdef CONFIG_USER_ONLY
static inline uint32_t set_swi_errno(TaskState *ts, uint32_t code)
{
@@ -272,6 +379,7 @@ target_ulong do_arm_semihosting(CPUARMState *env)
#else
CPUARMState *ts = env;
#endif
+ GuestFD *gf;
if (is_a64(env)) {
/* Note that the syscall number is in W0, not X0 */
@@ -284,6 +392,9 @@ target_ulong do_arm_semihosting(CPUARMState *env)
switch (nr) {
case TARGET_SYS_OPEN:
+ {
+ int guestfd;
+
GET_ARG(0);
GET_ARG(1);
GET_ARG(2);
@@ -297,10 +408,19 @@ target_ulong do_arm_semihosting(CPUARMState *env)
errno = EINVAL;
return set_swi_errno(ts, -1);
}
+
+ guestfd = alloc_guestfd();
+ if (guestfd < 0) {
+ unlock_user(s, arg0, 0);
+ errno = EMFILE;
+ return set_swi_errno(ts, -1);
+ }
+
if (strcmp(s, ":tt") == 0) {
int result_fileno = arg1 < 4 ? STDIN_FILENO : STDOUT_FILENO;
+ associate_guestfd(guestfd, result_fileno);
unlock_user(s, arg0, 0);
- return result_fileno;
+ return guestfd;
}
if (use_gdb_syscalls()) {
ret = arm_gdb_syscall(cpu, arm_semi_cb, "open,%s,%x,1a4", arg0,
@@ -308,15 +428,31 @@ target_ulong do_arm_semihosting(CPUARMState *env)
} else {
ret = set_swi_errno(ts, open(s, open_modeflags[arg1], 0644));
}
+ if (ret == (uint32_t)-1) {
+ dealloc_guestfd(guestfd);
+ } else {
+ associate_guestfd(guestfd, ret);
+ ret = guestfd;
+ }
unlock_user(s, arg0, 0);
return ret;
+ }
case TARGET_SYS_CLOSE:
GET_ARG(0);
- if (use_gdb_syscalls()) {
- return arm_gdb_syscall(cpu, arm_semi_cb, "close,%x", arg0);
- } else {
- return set_swi_errno(ts, close(arg0));
+
+ gf = get_guestfd(arg0);
+ if (!gf) {
+ errno = EBADF;
+ return set_swi_errno(ts, -1);
}
+
+ if (use_gdb_syscalls()) {
+ ret = arm_gdb_syscall(cpu, arm_semi_cb, "close,%x", gf->hostfd);
+ } else {
+ ret = set_swi_errno(ts, close(gf->hostfd));
+ }
+ dealloc_guestfd(arg0);
+ return ret;
case TARGET_SYS_WRITEC:
qemu_semihosting_console_outc(env, args);
return 0xdeadbeef;
@@ -327,17 +463,24 @@ target_ulong do_arm_semihosting(CPUARMState *env)
GET_ARG(1);
GET_ARG(2);
len = arg2;
+
+ gf = get_guestfd(arg0);
+ if (!gf) {
+ errno = EBADF;
+ return set_swi_errno(ts, -1);
+ }
+
if (use_gdb_syscalls()) {
arm_semi_syscall_len = len;
return arm_gdb_syscall(cpu, arm_semi_cb, "write,%x,%x,%x",
- arg0, arg1, len);
+ gf->hostfd, arg1, len);
} else {
s = lock_user(VERIFY_READ, arg1, len, 1);
if (!s) {
/* Return bytes not written on error */
return len;
}
- ret = set_swi_errno(ts, write(arg0, s, len));
+ ret = set_swi_errno(ts, write(gf->hostfd, s, len));
unlock_user(s, arg1, 0);
if (ret == (uint32_t)-1) {
ret = 0;
@@ -350,10 +493,17 @@ target_ulong do_arm_semihosting(CPUARMState *env)
GET_ARG(1);
GET_ARG(2);
len = arg2;
+
+ gf = get_guestfd(arg0);
+ if (!gf) {
+ errno = EBADF;
+ return set_swi_errno(ts, -1);
+ }
+
if (use_gdb_syscalls()) {
arm_semi_syscall_len = len;
return arm_gdb_syscall(cpu, arm_semi_cb, "read,%x,%x,%x",
- arg0, arg1, len);
+ gf->hostfd, arg1, len);
} else {
s = lock_user(VERIFY_WRITE, arg1, len, 0);
if (!s) {
@@ -361,7 +511,7 @@ target_ulong do_arm_semihosting(CPUARMState *env)
return len;
}
do {
- ret = set_swi_errno(ts, read(arg0, s, len));
+ ret = set_swi_errno(ts, read(gf->hostfd, s, len));
} while (ret == -1 && errno == EINTR);
unlock_user(s, arg1, len);
if (ret == (uint32_t)-1) {
@@ -375,31 +525,52 @@ target_ulong do_arm_semihosting(CPUARMState *env)
return 0;
case TARGET_SYS_ISTTY:
GET_ARG(0);
+
+ gf = get_guestfd(arg0);
+ if (!gf) {
+ errno = EBADF;
+ return set_swi_errno(ts, -1);
+ }
+
if (use_gdb_syscalls()) {
- return arm_gdb_syscall(cpu, arm_semi_cb, "isatty,%x", arg0);
+ return arm_gdb_syscall(cpu, arm_semi_cb, "isatty,%x", gf->hostfd);
} else {
- return isatty(arg0);
+ return isatty(gf->hostfd);
}
case TARGET_SYS_SEEK:
GET_ARG(0);
GET_ARG(1);
+
+ gf = get_guestfd(arg0);
+ if (!gf) {
+ errno = EBADF;
+ return set_swi_errno(ts, -1);
+ }
+
if (use_gdb_syscalls()) {
return arm_gdb_syscall(cpu, arm_semi_cb, "lseek,%x,%x,0",
- arg0, arg1);
+ gf->hostfd, arg1);
} else {
- ret = set_swi_errno(ts, lseek(arg0, arg1, SEEK_SET));
+ ret = set_swi_errno(ts, lseek(gf->hostfd, arg1, SEEK_SET));
if (ret == (uint32_t)-1)
return -1;
return 0;
}
case TARGET_SYS_FLEN:
GET_ARG(0);
+
+ gf = get_guestfd(arg0);
+ if (!gf) {
+ errno = EBADF;
+ return set_swi_errno(ts, -1);
+ }
+
if (use_gdb_syscalls()) {
return arm_gdb_syscall(cpu, arm_semi_flen_cb, "fstat,%x,%x",
- arg0, arm_flen_buf(cpu));
+ gf->hostfd, arm_flen_buf(cpu));
} else {
struct stat buf;
- ret = set_swi_errno(ts, fstat(arg0, &buf));
+ ret = set_swi_errno(ts, fstat(gf->hostfd, &buf));
if (ret == (uint32_t)-1)
return -1;
return buf.st_size;
--
2.20.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [Qemu-arm] [PATCH 03/13] target/arm/arm-semi: Make semihosting code hand out its own file descriptors
2019-09-10 14:44 ` [Qemu-devel] [PATCH 03/13] target/arm/arm-semi: Make semihosting code hand out its own file descriptors Peter Maydell
@ 2019-09-12 11:02 ` Alex Bennée
0 siblings, 0 replies; 33+ messages in thread
From: Alex Bennée @ 2019-09-12 11:02 UTC (permalink / raw)
To: qemu-arm; +Cc: qemu-devel
Peter Maydell <peter.maydell@linaro.org> writes:
> Currently the Arm semihosting code returns the guest file descriptors
> (handles) which are simply the fd values from the host OS or the
> remote gdbstub. Part of the semihosting 2.0 specification requires
> that we implement special handling of opening a ":semihosting-features"
> filename. Guest fds which result from opening the special file
> won't correspond to host fds, so to ensure that we don't end up
> with duplicate fds we need to have QEMU code control the allocation
> of the fd values we give the guest.
>
> Add in an abstraction layer which lets us allocate new guest FD
> values, and translate from a guest FD value back to the host one.
> This also fixes an odd hole where a semihosting guest could
> use the semihosting API to read, write or close file descriptors
> that it had never allocated but which were being used by QEMU itself.
I'm kind surprised QEMU doesn't already need this sort of facility for
something else. I guess virtfs and 9p have their own thing?
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Anyway:
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
--
Alex Bennée
^ permalink raw reply [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH 04/13] target/arm/arm-semi: clean up TaskState* usage in non-user-only code
2019-09-10 14:44 [Qemu-devel] [PATCH 00/13] target/arm: Implement semihosting v2.0 Peter Maydell
` (2 preceding siblings ...)
2019-09-10 14:44 ` [Qemu-devel] [PATCH 03/13] target/arm/arm-semi: Make semihosting code hand out its own file descriptors Peter Maydell
@ 2019-09-10 14:44 ` Peter Maydell
2019-09-12 11:42 ` [Qemu-devel] [Qemu-arm] " Alex Bennée
2019-09-10 14:44 ` [Qemu-devel] [PATCH 05/13] target/arm/arm-semi: Factor out implementation of SYS_CLOSE Peter Maydell
` (8 subsequent siblings)
12 siblings, 1 reply; 33+ messages in thread
From: Peter Maydell @ 2019-09-10 14:44 UTC (permalink / raw)
To: qemu-arm, qemu-devel
The semihosting code has to build for both user-only and softmmu;
for user-only it needs access to the TaskState struct that holds
per-thread information. For softmmu we don't need it.
Currently the softmmu set_swi_errno() takes a CPUARMState *,
which it doesn't use, and the 'ts' variable in do_arm_semihosting()
is set to either be a TaskState* or a CPUARMState* depending on
whether CONFIG_USER_ONLY is set, so that the callsite always
passes 'ts'. Since we don't actually need the CPUARMState *,
we can instead make set_swi_errno() always take a TaskState*,
by providing a local-to-this-file dummy typedef for the softmmu
case and setting ts to NULL for softmmu.
This will make it easier to have other functions which pass
through the TaskState*, because now they can have the same
prototype regardless of whether they're CONFIG_USER_ONLY or not.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
target/arm/arm-semi.c | 48 ++++++++++++++++++++++++-------------------
1 file changed, 27 insertions(+), 21 deletions(-)
diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
index 05491bf5248..ce3ba554bef 100644
--- a/target/arm/arm-semi.c
+++ b/target/arm/arm-semi.c
@@ -36,6 +36,13 @@
#else
#include "exec/gdbstub.h"
#include "qemu/cutils.h"
+
+/*
+ * Dummy typedef so that we can have functions that take
+ * a TaskState* even if we're building for softmmu; in that
+ * case the argument will always be NULL.
+ */
+typedef void TaskState;
#endif
#define TARGET_SYS_OPEN 0x01
@@ -213,27 +220,24 @@ static GuestFD *get_guestfd(int guestfd)
return gf;
}
-#ifdef CONFIG_USER_ONLY
-static inline uint32_t set_swi_errno(TaskState *ts, uint32_t code)
-{
- if (code == (uint32_t)-1)
- ts->swi_errno = errno;
- return code;
-}
-#else
+#ifndef CONFIG_USER_ONLY
static target_ulong syscall_err;
-static inline uint32_t set_swi_errno(CPUARMState *env, uint32_t code)
-{
- if (code == (uint32_t)-1) {
- syscall_err = errno;
- }
- return code;
-}
-
#include "exec/softmmu-semi.h"
#endif
+static inline uint32_t set_swi_errno(TaskState *ts, uint32_t code)
+{
+ if (code == (uint32_t)-1) {
+#ifdef CONFIG_USER_ONLY
+ ts->swi_errno = errno;
+#else
+ syscall_err = errno;
+#endif
+ }
+ return code;
+}
+
static target_ulong arm_semi_syscall_len;
static void arm_semi_cb(CPUState *cs, target_ulong ret, target_ulong err)
@@ -374,13 +378,15 @@ target_ulong do_arm_semihosting(CPUARMState *env)
int nr;
uint32_t ret;
uint32_t len;
-#ifdef CONFIG_USER_ONLY
- TaskState *ts = cs->opaque;
-#else
- CPUARMState *ts = env;
-#endif
+ TaskState *ts;
GuestFD *gf;
+#ifdef CONFIG_USER_ONLY
+ ts = cs->opaque;
+#else
+ ts = NULL;
+#endif
+
if (is_a64(env)) {
/* Note that the syscall number is in W0, not X0 */
nr = env->xregs[0] & 0xffffffffU;
--
2.20.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [Qemu-arm] [PATCH 04/13] target/arm/arm-semi: clean up TaskState* usage in non-user-only code
2019-09-10 14:44 ` [Qemu-devel] [PATCH 04/13] target/arm/arm-semi: clean up TaskState* usage in non-user-only code Peter Maydell
@ 2019-09-12 11:42 ` Alex Bennée
2019-09-12 11:49 ` Peter Maydell
0 siblings, 1 reply; 33+ messages in thread
From: Alex Bennée @ 2019-09-12 11:42 UTC (permalink / raw)
To: qemu-arm; +Cc: qemu-devel
Peter Maydell <peter.maydell@linaro.org> writes:
> The semihosting code has to build for both user-only and softmmu;
> for user-only it needs access to the TaskState struct that holds
> per-thread information. For softmmu we don't need it.
>
> Currently the softmmu set_swi_errno() takes a CPUARMState *,
> which it doesn't use, and the 'ts' variable in do_arm_semihosting()
> is set to either be a TaskState* or a CPUARMState* depending on
> whether CONFIG_USER_ONLY is set, so that the callsite always
> passes 'ts'. Since we don't actually need the CPUARMState *,
> we can instead make set_swi_errno() always take a TaskState*,
> by providing a local-to-this-file dummy typedef for the softmmu
> case and setting ts to NULL for softmmu.
>
> This will make it easier to have other functions which pass
> through the TaskState*, because now they can have the same
> prototype regardless of whether they're CONFIG_USER_ONLY or not.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> target/arm/arm-semi.c | 48 ++++++++++++++++++++++++-------------------
> 1 file changed, 27 insertions(+), 21 deletions(-)
>
> diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
> index 05491bf5248..ce3ba554bef 100644
> --- a/target/arm/arm-semi.c
> +++ b/target/arm/arm-semi.c
> @@ -36,6 +36,13 @@
> #else
> #include "exec/gdbstub.h"
> #include "qemu/cutils.h"
> +
> +/*
> + * Dummy typedef so that we can have functions that take
> + * a TaskState* even if we're building for softmmu; in that
> + * case the argument will always be NULL.
> + */
> +typedef void TaskState;
> #endif
>
> #define TARGET_SYS_OPEN 0x01
> @@ -213,27 +220,24 @@ static GuestFD *get_guestfd(int guestfd)
> return gf;
> }
>
> -#ifdef CONFIG_USER_ONLY
> -static inline uint32_t set_swi_errno(TaskState *ts, uint32_t code)
> -{
> - if (code == (uint32_t)-1)
> - ts->swi_errno = errno;
> - return code;
> -}
> -#else
> +#ifndef CONFIG_USER_ONLY
> static target_ulong syscall_err;
>
> -static inline uint32_t set_swi_errno(CPUARMState *env, uint32_t code)
> -{
> - if (code == (uint32_t)-1) {
> - syscall_err = errno;
> - }
> - return code;
> -}
> -
> #include "exec/softmmu-semi.h"
> #endif
>
> +static inline uint32_t set_swi_errno(TaskState *ts, uint32_t code)
> +{
> + if (code == (uint32_t)-1) {
> +#ifdef CONFIG_USER_ONLY
> + ts->swi_errno = errno;
> +#else
> + syscall_err = errno;
> +#endif
> + }
> + return code;
> +}
> +
> static target_ulong arm_semi_syscall_len;
>
> static void arm_semi_cb(CPUState *cs, target_ulong ret, target_ulong err)
> @@ -374,13 +378,15 @@ target_ulong do_arm_semihosting(CPUARMState *env)
> int nr;
> uint32_t ret;
> uint32_t len;
> -#ifdef CONFIG_USER_ONLY
> - TaskState *ts = cs->opaque;
> -#else
> - CPUARMState *ts = env;
> -#endif
> + TaskState *ts;
> GuestFD *gf;
>
> +#ifdef CONFIG_USER_ONLY
> + ts = cs->opaque;
> +#else
> + ts = NULL;
> +#endif
Why not pass cs to set_swi_errno and deal with all the differences in
the helper?
> +
> if (is_a64(env)) {
> /* Note that the syscall number is in W0, not X0 */
> nr = env->xregs[0] & 0xffffffffU;
--
Alex Bennée
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [Qemu-arm] [PATCH 04/13] target/arm/arm-semi: clean up TaskState* usage in non-user-only code
2019-09-12 11:42 ` [Qemu-devel] [Qemu-arm] " Alex Bennée
@ 2019-09-12 11:49 ` Peter Maydell
0 siblings, 0 replies; 33+ messages in thread
From: Peter Maydell @ 2019-09-12 11:49 UTC (permalink / raw)
To: Alex Bennée; +Cc: qemu-arm, QEMU Developers
On Thu, 12 Sep 2019 at 12:44, Alex Bennée <alex.bennee@linaro.org> wrote:
> Why not pass cs to set_swi_errno and deal with all the differences in
> the helper?
Mmm, that might be better. I think I was going for
not changing the existing use of TaskState in the
code paths that use it.
thanks
-- PMM
^ permalink raw reply [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH 05/13] target/arm/arm-semi: Factor out implementation of SYS_CLOSE
2019-09-10 14:44 [Qemu-devel] [PATCH 00/13] target/arm: Implement semihosting v2.0 Peter Maydell
` (3 preceding siblings ...)
2019-09-10 14:44 ` [Qemu-devel] [PATCH 04/13] target/arm/arm-semi: clean up TaskState* usage in non-user-only code Peter Maydell
@ 2019-09-10 14:44 ` Peter Maydell
2019-09-12 11:13 ` [Qemu-devel] [Qemu-arm] " Alex Bennée
2019-09-10 14:44 ` [Qemu-devel] [PATCH 06/13] target/arm/arm-semi: Factor out implementation of SYS_WRITE Peter Maydell
` (7 subsequent siblings)
12 siblings, 1 reply; 33+ messages in thread
From: Peter Maydell @ 2019-09-10 14:44 UTC (permalink / raw)
To: qemu-arm, qemu-devel
Currently for the semihosting calls which take a file descriptor
(SYS_CLOSE, SYS_WRITE, SYS_READ, SYS_ISTTY, SYS_SEEK, SYS_FLEN)
we have effectively two implementations, one for real host files
and one for when we indirect via the gdbstub. We want to add a
third one to deal with the magic :semihosting-features file.
Instead of having a three-way if statement in each of these
cases, factor out the implementation of the calls to separate
functions which we dispatch to via function pointers selected
via the GuestFDType for the guest fd.
In this commit, we set up the framework for the dispatch,
and convert the SYS_CLOSE call to use it.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
target/arm/arm-semi.c | 42 +++++++++++++++++++++++++++++++++++-------
1 file changed, 35 insertions(+), 7 deletions(-)
diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
index ce3ba554bef..f3e0bf77cd3 100644
--- a/target/arm/arm-semi.c
+++ b/target/arm/arm-semi.c
@@ -116,6 +116,7 @@ static int open_modeflags[12] = {
typedef enum GuestFDType {
GuestFDUnused = 0,
GuestFDHost = 1,
+ GuestFDGDB = 2,
} GuestFDType;
/*
@@ -179,14 +180,14 @@ static GuestFD *do_get_guestfd(int guestfd)
/*
* Associate the specified guest fd (which must have been
* allocated via alloc_fd() and not previously used) with
- * the specified host fd.
+ * the specified host/gdb fd.
*/
static void associate_guestfd(int guestfd, int hostfd)
{
GuestFD *gf = do_get_guestfd(guestfd);
assert(gf);
- gf->type = GuestFDHost;
+ gf->type = use_gdb_syscalls() ? GuestFDGDB : GuestFDHost;
gf->hostfd = hostfd;
}
@@ -337,6 +338,37 @@ static target_ulong arm_gdb_syscall(ARMCPU *cpu, gdb_syscall_complete_cb cb,
return is_a64(env) ? env->xregs[0] : env->regs[0];
}
+/*
+ * Types for functions implementing various semihosting calls
+ * for specific types of guest file descriptor. These must all
+ * do the work and return the required return value for the guest,
+ * setting the guest errno if appropriate.
+ */
+typedef uint32_t sys_closefn(TaskState *ts, ARMCPU *cpu, GuestFD *gf);
+
+static uint32_t host_closefn(TaskState *ts, ARMCPU *cpu, GuestFD *gf)
+{
+ return set_swi_errno(ts, close(gf->hostfd));
+}
+
+static uint32_t gdb_closefn(TaskState *ts, ARMCPU *cpu, GuestFD *gf)
+{
+ return arm_gdb_syscall(cpu, arm_semi_cb, "close,%x", gf->hostfd);
+}
+
+typedef struct GuestFDFunctions {
+ sys_closefn *closefn;
+} GuestFDFunctions;
+
+static const GuestFDFunctions guestfd_fns[] = {
+ [GuestFDHost] = {
+ .closefn = host_closefn,
+ },
+ [GuestFDGDB] = {
+ .closefn = gdb_closefn,
+ },
+};
+
/* Read the input value from the argument block; fail the semihosting
* call if the memory read fails.
*/
@@ -452,11 +484,7 @@ target_ulong do_arm_semihosting(CPUARMState *env)
return set_swi_errno(ts, -1);
}
- if (use_gdb_syscalls()) {
- ret = arm_gdb_syscall(cpu, arm_semi_cb, "close,%x", gf->hostfd);
- } else {
- ret = set_swi_errno(ts, close(gf->hostfd));
- }
+ ret = guestfd_fns[gf->type].closefn(ts, cpu, gf);
dealloc_guestfd(arg0);
return ret;
case TARGET_SYS_WRITEC:
--
2.20.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [Qemu-arm] [PATCH 05/13] target/arm/arm-semi: Factor out implementation of SYS_CLOSE
2019-09-10 14:44 ` [Qemu-devel] [PATCH 05/13] target/arm/arm-semi: Factor out implementation of SYS_CLOSE Peter Maydell
@ 2019-09-12 11:13 ` Alex Bennée
0 siblings, 0 replies; 33+ messages in thread
From: Alex Bennée @ 2019-09-12 11:13 UTC (permalink / raw)
To: qemu-arm; +Cc: qemu-devel
Peter Maydell <peter.maydell@linaro.org> writes:
> Currently for the semihosting calls which take a file descriptor
> (SYS_CLOSE, SYS_WRITE, SYS_READ, SYS_ISTTY, SYS_SEEK, SYS_FLEN)
> we have effectively two implementations, one for real host files
> and one for when we indirect via the gdbstub. We want to add a
> third one to deal with the magic :semihosting-features file.
>
> Instead of having a three-way if statement in each of these
> cases, factor out the implementation of the calls to separate
> functions which we dispatch to via function pointers selected
> via the GuestFDType for the guest fd.
>
> In this commit, we set up the framework for the dispatch,
> and convert the SYS_CLOSE call to use it.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> ---
> target/arm/arm-semi.c | 42 +++++++++++++++++++++++++++++++++++-------
> 1 file changed, 35 insertions(+), 7 deletions(-)
>
> diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
> index ce3ba554bef..f3e0bf77cd3 100644
> --- a/target/arm/arm-semi.c
> +++ b/target/arm/arm-semi.c
> @@ -116,6 +116,7 @@ static int open_modeflags[12] = {
> typedef enum GuestFDType {
> GuestFDUnused = 0,
> GuestFDHost = 1,
> + GuestFDGDB = 2,
> } GuestFDType;
>
> /*
> @@ -179,14 +180,14 @@ static GuestFD *do_get_guestfd(int guestfd)
> /*
> * Associate the specified guest fd (which must have been
> * allocated via alloc_fd() and not previously used) with
> - * the specified host fd.
> + * the specified host/gdb fd.
> */
> static void associate_guestfd(int guestfd, int hostfd)
> {
> GuestFD *gf = do_get_guestfd(guestfd);
>
> assert(gf);
> - gf->type = GuestFDHost;
> + gf->type = use_gdb_syscalls() ? GuestFDGDB : GuestFDHost;
> gf->hostfd = hostfd;
> }
>
> @@ -337,6 +338,37 @@ static target_ulong arm_gdb_syscall(ARMCPU *cpu, gdb_syscall_complete_cb cb,
> return is_a64(env) ? env->xregs[0] : env->regs[0];
> }
>
> +/*
> + * Types for functions implementing various semihosting calls
> + * for specific types of guest file descriptor. These must all
> + * do the work and return the required return value for the guest,
> + * setting the guest errno if appropriate.
> + */
> +typedef uint32_t sys_closefn(TaskState *ts, ARMCPU *cpu, GuestFD *gf);
> +
> +static uint32_t host_closefn(TaskState *ts, ARMCPU *cpu, GuestFD *gf)
> +{
> + return set_swi_errno(ts, close(gf->hostfd));
> +}
> +
> +static uint32_t gdb_closefn(TaskState *ts, ARMCPU *cpu, GuestFD *gf)
> +{
> + return arm_gdb_syscall(cpu, arm_semi_cb, "close,%x", gf->hostfd);
> +}
> +
> +typedef struct GuestFDFunctions {
> + sys_closefn *closefn;
> +} GuestFDFunctions;
> +
> +static const GuestFDFunctions guestfd_fns[] = {
> + [GuestFDHost] = {
> + .closefn = host_closefn,
> + },
> + [GuestFDGDB] = {
> + .closefn = gdb_closefn,
> + },
> +};
> +
> /* Read the input value from the argument block; fail the semihosting
> * call if the memory read fails.
> */
> @@ -452,11 +484,7 @@ target_ulong do_arm_semihosting(CPUARMState *env)
> return set_swi_errno(ts, -1);
> }
>
> - if (use_gdb_syscalls()) {
> - ret = arm_gdb_syscall(cpu, arm_semi_cb, "close,%x", gf->hostfd);
> - } else {
> - ret = set_swi_errno(ts, close(gf->hostfd));
> - }
> + ret = guestfd_fns[gf->type].closefn(ts, cpu, gf);
> dealloc_guestfd(arg0);
> return ret;
> case TARGET_SYS_WRITEC:
--
Alex Bennée
^ permalink raw reply [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH 06/13] target/arm/arm-semi: Factor out implementation of SYS_WRITE
2019-09-10 14:44 [Qemu-devel] [PATCH 00/13] target/arm: Implement semihosting v2.0 Peter Maydell
` (4 preceding siblings ...)
2019-09-10 14:44 ` [Qemu-devel] [PATCH 05/13] target/arm/arm-semi: Factor out implementation of SYS_CLOSE Peter Maydell
@ 2019-09-10 14:44 ` Peter Maydell
2019-09-12 11:18 ` Alex Bennée
2019-09-10 14:44 ` [Qemu-devel] [PATCH 07/13] target/arm/arm-semi: Factor out implementation of SYS_READ Peter Maydell
` (6 subsequent siblings)
12 siblings, 1 reply; 33+ messages in thread
From: Peter Maydell @ 2019-09-10 14:44 UTC (permalink / raw)
To: qemu-arm, qemu-devel
Factor out the implementation of SYS_WRITE via the
new function tables.
The #ifdef around the declaration/initialization of the
local 'env' variable is unfortunate but necessary, because
the softmmu-semi.h version of lock_user implicitly uses 'env',
but the user-only version doesn't need it. Without the ifdefs
we'd get a warning about the unused variable for the user-only
compilation.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
target/arm/arm-semi.c | 53 ++++++++++++++++++++++++++++---------------
1 file changed, 35 insertions(+), 18 deletions(-)
diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
index f3e0bf77cd3..0dec4c04e2f 100644
--- a/target/arm/arm-semi.c
+++ b/target/arm/arm-semi.c
@@ -345,27 +345,61 @@ static target_ulong arm_gdb_syscall(ARMCPU *cpu, gdb_syscall_complete_cb cb,
* setting the guest errno if appropriate.
*/
typedef uint32_t sys_closefn(TaskState *ts, ARMCPU *cpu, GuestFD *gf);
+typedef uint32_t sys_writefn(TaskState *ts, ARMCPU *cpu, GuestFD *gf,
+ target_ulong buf, uint32_t len);
static uint32_t host_closefn(TaskState *ts, ARMCPU *cpu, GuestFD *gf)
{
return set_swi_errno(ts, close(gf->hostfd));
}
+static uint32_t host_writefn(TaskState *ts, ARMCPU *cpu, GuestFD *gf,
+ target_ulong buf, uint32_t len)
+{
+ uint32_t ret;
+#ifndef CONFIG_USER_ONLY
+ CPUARMState *env = &cpu->env;
+#endif
+ char *s = lock_user(VERIFY_READ, buf, len, 1);
+ if (!s) {
+ /* Return bytes not written on error */
+ return len;
+ }
+ ret = set_swi_errno(ts, write(gf->hostfd, s, len));
+ unlock_user(s, buf, 0);
+ if (ret == (uint32_t)-1) {
+ ret = 0;
+ }
+ /* Return bytes not written */
+ return len - ret;
+}
+
static uint32_t gdb_closefn(TaskState *ts, ARMCPU *cpu, GuestFD *gf)
{
return arm_gdb_syscall(cpu, arm_semi_cb, "close,%x", gf->hostfd);
}
+static uint32_t gdb_writefn(TaskState *ts, ARMCPU *cpu, GuestFD *gf,
+ target_ulong buf, uint32_t len)
+{
+ arm_semi_syscall_len = len;
+ return arm_gdb_syscall(cpu, arm_semi_cb, "write,%x,%x,%x",
+ gf->hostfd, buf, len);
+}
+
typedef struct GuestFDFunctions {
sys_closefn *closefn;
+ sys_writefn *writefn;
} GuestFDFunctions;
static const GuestFDFunctions guestfd_fns[] = {
[GuestFDHost] = {
.closefn = host_closefn,
+ .writefn = host_writefn,
},
[GuestFDGDB] = {
.closefn = gdb_closefn,
+ .writefn = gdb_writefn,
},
};
@@ -504,24 +538,7 @@ target_ulong do_arm_semihosting(CPUARMState *env)
return set_swi_errno(ts, -1);
}
- if (use_gdb_syscalls()) {
- arm_semi_syscall_len = len;
- return arm_gdb_syscall(cpu, arm_semi_cb, "write,%x,%x,%x",
- gf->hostfd, arg1, len);
- } else {
- s = lock_user(VERIFY_READ, arg1, len, 1);
- if (!s) {
- /* Return bytes not written on error */
- return len;
- }
- ret = set_swi_errno(ts, write(gf->hostfd, s, len));
- unlock_user(s, arg1, 0);
- if (ret == (uint32_t)-1) {
- ret = 0;
- }
- /* Return bytes not written */
- return len - ret;
- }
+ return guestfd_fns[gf->type].writefn(ts, cpu, gf, arg1, len);
case TARGET_SYS_READ:
GET_ARG(0);
GET_ARG(1);
--
2.20.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [PATCH 06/13] target/arm/arm-semi: Factor out implementation of SYS_WRITE
2019-09-10 14:44 ` [Qemu-devel] [PATCH 06/13] target/arm/arm-semi: Factor out implementation of SYS_WRITE Peter Maydell
@ 2019-09-12 11:18 ` Alex Bennée
0 siblings, 0 replies; 33+ messages in thread
From: Alex Bennée @ 2019-09-12 11:18 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-arm
Peter Maydell <peter.maydell@linaro.org> writes:
> Factor out the implementation of SYS_WRITE via the
> new function tables.
>
> The #ifdef around the declaration/initialization of the
> local 'env' variable is unfortunate but necessary, because
> the softmmu-semi.h version of lock_user implicitly uses 'env',
> but the user-only version doesn't need it.
heh - one reason I rewrote a local lock_user_string for semihost.c
although of course it only has to worry about the softmmu case as you
don't have re-direct-able char devices in linux-user.
> Without the ifdefs
> we'd get a warning about the unused variable for the user-only
> compilation.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> ---
> target/arm/arm-semi.c | 53 ++++++++++++++++++++++++++++---------------
> 1 file changed, 35 insertions(+), 18 deletions(-)
>
> diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
> index f3e0bf77cd3..0dec4c04e2f 100644
> --- a/target/arm/arm-semi.c
> +++ b/target/arm/arm-semi.c
> @@ -345,27 +345,61 @@ static target_ulong arm_gdb_syscall(ARMCPU *cpu, gdb_syscall_complete_cb cb,
> * setting the guest errno if appropriate.
> */
> typedef uint32_t sys_closefn(TaskState *ts, ARMCPU *cpu, GuestFD *gf);
> +typedef uint32_t sys_writefn(TaskState *ts, ARMCPU *cpu, GuestFD *gf,
> + target_ulong buf, uint32_t len);
>
> static uint32_t host_closefn(TaskState *ts, ARMCPU *cpu, GuestFD *gf)
> {
> return set_swi_errno(ts, close(gf->hostfd));
> }
>
> +static uint32_t host_writefn(TaskState *ts, ARMCPU *cpu, GuestFD *gf,
> + target_ulong buf, uint32_t len)
> +{
> + uint32_t ret;
> +#ifndef CONFIG_USER_ONLY
> + CPUARMState *env = &cpu->env;
> +#endif
> + char *s = lock_user(VERIFY_READ, buf, len, 1);
> + if (!s) {
> + /* Return bytes not written on error */
> + return len;
> + }
> + ret = set_swi_errno(ts, write(gf->hostfd, s, len));
> + unlock_user(s, buf, 0);
> + if (ret == (uint32_t)-1) {
> + ret = 0;
> + }
> + /* Return bytes not written */
> + return len - ret;
> +}
> +
> static uint32_t gdb_closefn(TaskState *ts, ARMCPU *cpu, GuestFD *gf)
> {
> return arm_gdb_syscall(cpu, arm_semi_cb, "close,%x", gf->hostfd);
> }
>
> +static uint32_t gdb_writefn(TaskState *ts, ARMCPU *cpu, GuestFD *gf,
> + target_ulong buf, uint32_t len)
> +{
> + arm_semi_syscall_len = len;
> + return arm_gdb_syscall(cpu, arm_semi_cb, "write,%x,%x,%x",
> + gf->hostfd, buf, len);
> +}
> +
> typedef struct GuestFDFunctions {
> sys_closefn *closefn;
> + sys_writefn *writefn;
> } GuestFDFunctions;
>
> static const GuestFDFunctions guestfd_fns[] = {
> [GuestFDHost] = {
> .closefn = host_closefn,
> + .writefn = host_writefn,
> },
> [GuestFDGDB] = {
> .closefn = gdb_closefn,
> + .writefn = gdb_writefn,
> },
> };
>
> @@ -504,24 +538,7 @@ target_ulong do_arm_semihosting(CPUARMState *env)
> return set_swi_errno(ts, -1);
> }
>
> - if (use_gdb_syscalls()) {
> - arm_semi_syscall_len = len;
> - return arm_gdb_syscall(cpu, arm_semi_cb, "write,%x,%x,%x",
> - gf->hostfd, arg1, len);
> - } else {
> - s = lock_user(VERIFY_READ, arg1, len, 1);
> - if (!s) {
> - /* Return bytes not written on error */
> - return len;
> - }
> - ret = set_swi_errno(ts, write(gf->hostfd, s, len));
> - unlock_user(s, arg1, 0);
> - if (ret == (uint32_t)-1) {
> - ret = 0;
> - }
> - /* Return bytes not written */
> - return len - ret;
> - }
> + return guestfd_fns[gf->type].writefn(ts, cpu, gf, arg1, len);
> case TARGET_SYS_READ:
> GET_ARG(0);
> GET_ARG(1);
--
Alex Bennée
^ permalink raw reply [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH 07/13] target/arm/arm-semi: Factor out implementation of SYS_READ
2019-09-10 14:44 [Qemu-devel] [PATCH 00/13] target/arm: Implement semihosting v2.0 Peter Maydell
` (5 preceding siblings ...)
2019-09-10 14:44 ` [Qemu-devel] [PATCH 06/13] target/arm/arm-semi: Factor out implementation of SYS_WRITE Peter Maydell
@ 2019-09-10 14:44 ` Peter Maydell
2019-09-12 11:19 ` [Qemu-devel] [Qemu-arm] " Alex Bennée
2019-09-10 14:44 ` [Qemu-devel] [PATCH 08/13] target/arm/arm-semi: Factor out implementation of SYS_ISTTY Peter Maydell
` (5 subsequent siblings)
12 siblings, 1 reply; 33+ messages in thread
From: Peter Maydell @ 2019-09-10 14:44 UTC (permalink / raw)
To: qemu-arm, qemu-devel
Factor out the implementation of SYS_READ via the
new function tables.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
target/arm/arm-semi.c | 57 ++++++++++++++++++++++++++++---------------
1 file changed, 37 insertions(+), 20 deletions(-)
diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
index 0dec4c04e2f..48a10dd3c3a 100644
--- a/target/arm/arm-semi.c
+++ b/target/arm/arm-semi.c
@@ -347,6 +347,8 @@ static target_ulong arm_gdb_syscall(ARMCPU *cpu, gdb_syscall_complete_cb cb,
typedef uint32_t sys_closefn(TaskState *ts, ARMCPU *cpu, GuestFD *gf);
typedef uint32_t sys_writefn(TaskState *ts, ARMCPU *cpu, GuestFD *gf,
target_ulong buf, uint32_t len);
+typedef uint32_t sys_readfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf,
+ target_ulong buf, uint32_t len);
static uint32_t host_closefn(TaskState *ts, ARMCPU *cpu, GuestFD *gf)
{
@@ -374,6 +376,29 @@ static uint32_t host_writefn(TaskState *ts, ARMCPU *cpu, GuestFD *gf,
return len - ret;
}
+static uint32_t host_readfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf,
+ target_ulong buf, uint32_t len)
+{
+ uint32_t ret;
+#ifndef CONFIG_USER_ONLY
+ CPUARMState *env = &cpu->env;
+#endif
+ char *s = lock_user(VERIFY_WRITE, buf, len, 0);
+ if (!s) {
+ /* return bytes not read */
+ return len;
+ }
+ do {
+ ret = set_swi_errno(ts, read(gf->hostfd, s, len));
+ } while (ret == -1 && errno == EINTR);
+ unlock_user(s, buf, len);
+ if (ret == (uint32_t)-1) {
+ ret = 0;
+ }
+ /* Return bytes not read */
+ return len - ret;
+}
+
static uint32_t gdb_closefn(TaskState *ts, ARMCPU *cpu, GuestFD *gf)
{
return arm_gdb_syscall(cpu, arm_semi_cb, "close,%x", gf->hostfd);
@@ -387,19 +412,30 @@ static uint32_t gdb_writefn(TaskState *ts, ARMCPU *cpu, GuestFD *gf,
gf->hostfd, buf, len);
}
+static uint32_t gdb_readfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf,
+ target_ulong buf, uint32_t len)
+{
+ arm_semi_syscall_len = len;
+ return arm_gdb_syscall(cpu, arm_semi_cb, "read,%x,%x,%x",
+ gf->hostfd, buf, len);
+}
+
typedef struct GuestFDFunctions {
sys_closefn *closefn;
sys_writefn *writefn;
+ sys_readfn *readfn;
} GuestFDFunctions;
static const GuestFDFunctions guestfd_fns[] = {
[GuestFDHost] = {
.closefn = host_closefn,
.writefn = host_writefn,
+ .readfn = host_readfn,
},
[GuestFDGDB] = {
.closefn = gdb_closefn,
.writefn = gdb_writefn,
+ .readfn = gdb_readfn,
},
};
@@ -551,26 +587,7 @@ target_ulong do_arm_semihosting(CPUARMState *env)
return set_swi_errno(ts, -1);
}
- if (use_gdb_syscalls()) {
- arm_semi_syscall_len = len;
- return arm_gdb_syscall(cpu, arm_semi_cb, "read,%x,%x,%x",
- gf->hostfd, arg1, len);
- } else {
- s = lock_user(VERIFY_WRITE, arg1, len, 0);
- if (!s) {
- /* return bytes not read */
- return len;
- }
- do {
- ret = set_swi_errno(ts, read(gf->hostfd, s, len));
- } while (ret == -1 && errno == EINTR);
- unlock_user(s, arg1, len);
- if (ret == (uint32_t)-1) {
- ret = 0;
- }
- /* Return bytes not read */
- return len - ret;
- }
+ return guestfd_fns[gf->type].readfn(ts, cpu, gf, arg1, len);
case TARGET_SYS_READC:
qemu_log_mask(LOG_UNIMP, "%s: SYS_READC not implemented", __func__);
return 0;
--
2.20.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [Qemu-arm] [PATCH 07/13] target/arm/arm-semi: Factor out implementation of SYS_READ
2019-09-10 14:44 ` [Qemu-devel] [PATCH 07/13] target/arm/arm-semi: Factor out implementation of SYS_READ Peter Maydell
@ 2019-09-12 11:19 ` Alex Bennée
0 siblings, 0 replies; 33+ messages in thread
From: Alex Bennée @ 2019-09-12 11:19 UTC (permalink / raw)
To: qemu-arm; +Cc: qemu-devel
Peter Maydell <peter.maydell@linaro.org> writes:
> Factor out the implementation of SYS_READ via the
> new function tables.
"As for SYS_WRITE we need env for lock_user in system emulation."?
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Either way:
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> ---
> target/arm/arm-semi.c | 57 ++++++++++++++++++++++++++++---------------
> 1 file changed, 37 insertions(+), 20 deletions(-)
>
> diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
> index 0dec4c04e2f..48a10dd3c3a 100644
> --- a/target/arm/arm-semi.c
> +++ b/target/arm/arm-semi.c
> @@ -347,6 +347,8 @@ static target_ulong arm_gdb_syscall(ARMCPU *cpu, gdb_syscall_complete_cb cb,
> typedef uint32_t sys_closefn(TaskState *ts, ARMCPU *cpu, GuestFD *gf);
> typedef uint32_t sys_writefn(TaskState *ts, ARMCPU *cpu, GuestFD *gf,
> target_ulong buf, uint32_t len);
> +typedef uint32_t sys_readfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf,
> + target_ulong buf, uint32_t len);
>
> static uint32_t host_closefn(TaskState *ts, ARMCPU *cpu, GuestFD *gf)
> {
> @@ -374,6 +376,29 @@ static uint32_t host_writefn(TaskState *ts, ARMCPU *cpu, GuestFD *gf,
> return len - ret;
> }
>
> +static uint32_t host_readfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf,
> + target_ulong buf, uint32_t len)
> +{
> + uint32_t ret;
> +#ifndef CONFIG_USER_ONLY
> + CPUARMState *env = &cpu->env;
> +#endif
> + char *s = lock_user(VERIFY_WRITE, buf, len, 0);
> + if (!s) {
> + /* return bytes not read */
> + return len;
> + }
> + do {
> + ret = set_swi_errno(ts, read(gf->hostfd, s, len));
> + } while (ret == -1 && errno == EINTR);
> + unlock_user(s, buf, len);
> + if (ret == (uint32_t)-1) {
> + ret = 0;
> + }
> + /* Return bytes not read */
> + return len - ret;
> +}
> +
> static uint32_t gdb_closefn(TaskState *ts, ARMCPU *cpu, GuestFD *gf)
> {
> return arm_gdb_syscall(cpu, arm_semi_cb, "close,%x", gf->hostfd);
> @@ -387,19 +412,30 @@ static uint32_t gdb_writefn(TaskState *ts, ARMCPU *cpu, GuestFD *gf,
> gf->hostfd, buf, len);
> }
>
> +static uint32_t gdb_readfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf,
> + target_ulong buf, uint32_t len)
> +{
> + arm_semi_syscall_len = len;
> + return arm_gdb_syscall(cpu, arm_semi_cb, "read,%x,%x,%x",
> + gf->hostfd, buf, len);
> +}
> +
> typedef struct GuestFDFunctions {
> sys_closefn *closefn;
> sys_writefn *writefn;
> + sys_readfn *readfn;
> } GuestFDFunctions;
>
> static const GuestFDFunctions guestfd_fns[] = {
> [GuestFDHost] = {
> .closefn = host_closefn,
> .writefn = host_writefn,
> + .readfn = host_readfn,
> },
> [GuestFDGDB] = {
> .closefn = gdb_closefn,
> .writefn = gdb_writefn,
> + .readfn = gdb_readfn,
> },
> };
>
> @@ -551,26 +587,7 @@ target_ulong do_arm_semihosting(CPUARMState *env)
> return set_swi_errno(ts, -1);
> }
>
> - if (use_gdb_syscalls()) {
> - arm_semi_syscall_len = len;
> - return arm_gdb_syscall(cpu, arm_semi_cb, "read,%x,%x,%x",
> - gf->hostfd, arg1, len);
> - } else {
> - s = lock_user(VERIFY_WRITE, arg1, len, 0);
> - if (!s) {
> - /* return bytes not read */
> - return len;
> - }
> - do {
> - ret = set_swi_errno(ts, read(gf->hostfd, s, len));
> - } while (ret == -1 && errno == EINTR);
> - unlock_user(s, arg1, len);
> - if (ret == (uint32_t)-1) {
> - ret = 0;
> - }
> - /* Return bytes not read */
> - return len - ret;
> - }
> + return guestfd_fns[gf->type].readfn(ts, cpu, gf, arg1, len);
> case TARGET_SYS_READC:
> qemu_log_mask(LOG_UNIMP, "%s: SYS_READC not implemented", __func__);
> return 0;
--
Alex Bennée
^ permalink raw reply [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH 08/13] target/arm/arm-semi: Factor out implementation of SYS_ISTTY
2019-09-10 14:44 [Qemu-devel] [PATCH 00/13] target/arm: Implement semihosting v2.0 Peter Maydell
` (6 preceding siblings ...)
2019-09-10 14:44 ` [Qemu-devel] [PATCH 07/13] target/arm/arm-semi: Factor out implementation of SYS_READ Peter Maydell
@ 2019-09-10 14:44 ` Peter Maydell
2019-09-12 11:20 ` [Qemu-devel] [Qemu-arm] " Alex Bennée
2019-09-10 14:44 ` [Qemu-devel] [PATCH 09/13] target/arm/arm-semi: Factor out implementation of SYS_SEEK Peter Maydell
` (4 subsequent siblings)
12 siblings, 1 reply; 33+ messages in thread
From: Peter Maydell @ 2019-09-10 14:44 UTC (permalink / raw)
To: qemu-arm, qemu-devel
Factor out the implementation of SYS_ISTTY via the new function
tables.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
target/arm/arm-semi.c | 20 +++++++++++++++-----
1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
index 48a10dd3c3a..64ed39fc075 100644
--- a/target/arm/arm-semi.c
+++ b/target/arm/arm-semi.c
@@ -349,6 +349,7 @@ typedef uint32_t sys_writefn(TaskState *ts, ARMCPU *cpu, GuestFD *gf,
target_ulong buf, uint32_t len);
typedef uint32_t sys_readfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf,
target_ulong buf, uint32_t len);
+typedef uint32_t sys_isattyfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf);
static uint32_t host_closefn(TaskState *ts, ARMCPU *cpu, GuestFD *gf)
{
@@ -399,6 +400,11 @@ static uint32_t host_readfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf,
return len - ret;
}
+static uint32_t host_isattyfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf)
+{
+ return isatty(gf->hostfd);
+}
+
static uint32_t gdb_closefn(TaskState *ts, ARMCPU *cpu, GuestFD *gf)
{
return arm_gdb_syscall(cpu, arm_semi_cb, "close,%x", gf->hostfd);
@@ -420,10 +426,16 @@ static uint32_t gdb_readfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf,
gf->hostfd, buf, len);
}
+static uint32_t gdb_isattyfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf)
+{
+ return arm_gdb_syscall(cpu, arm_semi_cb, "isatty,%x", gf->hostfd);
+}
+
typedef struct GuestFDFunctions {
sys_closefn *closefn;
sys_writefn *writefn;
sys_readfn *readfn;
+ sys_isattyfn *isattyfn;
} GuestFDFunctions;
static const GuestFDFunctions guestfd_fns[] = {
@@ -431,11 +443,13 @@ static const GuestFDFunctions guestfd_fns[] = {
.closefn = host_closefn,
.writefn = host_writefn,
.readfn = host_readfn,
+ .isattyfn = host_isattyfn,
},
[GuestFDGDB] = {
.closefn = gdb_closefn,
.writefn = gdb_writefn,
.readfn = gdb_readfn,
+ .isattyfn = gdb_isattyfn,
},
};
@@ -600,11 +614,7 @@ target_ulong do_arm_semihosting(CPUARMState *env)
return set_swi_errno(ts, -1);
}
- if (use_gdb_syscalls()) {
- return arm_gdb_syscall(cpu, arm_semi_cb, "isatty,%x", gf->hostfd);
- } else {
- return isatty(gf->hostfd);
- }
+ return guestfd_fns[gf->type].isattyfn(ts, cpu, gf);
case TARGET_SYS_SEEK:
GET_ARG(0);
GET_ARG(1);
--
2.20.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [Qemu-arm] [PATCH 08/13] target/arm/arm-semi: Factor out implementation of SYS_ISTTY
2019-09-10 14:44 ` [Qemu-devel] [PATCH 08/13] target/arm/arm-semi: Factor out implementation of SYS_ISTTY Peter Maydell
@ 2019-09-12 11:20 ` Alex Bennée
0 siblings, 0 replies; 33+ messages in thread
From: Alex Bennée @ 2019-09-12 11:20 UTC (permalink / raw)
To: qemu-arm; +Cc: qemu-devel
Peter Maydell <peter.maydell@linaro.org> writes:
> Factor out the implementation of SYS_ISTTY via the new function
> tables.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> ---
> target/arm/arm-semi.c | 20 +++++++++++++++-----
> 1 file changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
> index 48a10dd3c3a..64ed39fc075 100644
> --- a/target/arm/arm-semi.c
> +++ b/target/arm/arm-semi.c
> @@ -349,6 +349,7 @@ typedef uint32_t sys_writefn(TaskState *ts, ARMCPU *cpu, GuestFD *gf,
> target_ulong buf, uint32_t len);
> typedef uint32_t sys_readfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf,
> target_ulong buf, uint32_t len);
> +typedef uint32_t sys_isattyfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf);
>
> static uint32_t host_closefn(TaskState *ts, ARMCPU *cpu, GuestFD *gf)
> {
> @@ -399,6 +400,11 @@ static uint32_t host_readfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf,
> return len - ret;
> }
>
> +static uint32_t host_isattyfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf)
> +{
> + return isatty(gf->hostfd);
> +}
> +
> static uint32_t gdb_closefn(TaskState *ts, ARMCPU *cpu, GuestFD *gf)
> {
> return arm_gdb_syscall(cpu, arm_semi_cb, "close,%x", gf->hostfd);
> @@ -420,10 +426,16 @@ static uint32_t gdb_readfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf,
> gf->hostfd, buf, len);
> }
>
> +static uint32_t gdb_isattyfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf)
> +{
> + return arm_gdb_syscall(cpu, arm_semi_cb, "isatty,%x", gf->hostfd);
> +}
> +
> typedef struct GuestFDFunctions {
> sys_closefn *closefn;
> sys_writefn *writefn;
> sys_readfn *readfn;
> + sys_isattyfn *isattyfn;
> } GuestFDFunctions;
>
> static const GuestFDFunctions guestfd_fns[] = {
> @@ -431,11 +443,13 @@ static const GuestFDFunctions guestfd_fns[] = {
> .closefn = host_closefn,
> .writefn = host_writefn,
> .readfn = host_readfn,
> + .isattyfn = host_isattyfn,
> },
> [GuestFDGDB] = {
> .closefn = gdb_closefn,
> .writefn = gdb_writefn,
> .readfn = gdb_readfn,
> + .isattyfn = gdb_isattyfn,
> },
> };
>
> @@ -600,11 +614,7 @@ target_ulong do_arm_semihosting(CPUARMState *env)
> return set_swi_errno(ts, -1);
> }
>
> - if (use_gdb_syscalls()) {
> - return arm_gdb_syscall(cpu, arm_semi_cb, "isatty,%x", gf->hostfd);
> - } else {
> - return isatty(gf->hostfd);
> - }
> + return guestfd_fns[gf->type].isattyfn(ts, cpu, gf);
> case TARGET_SYS_SEEK:
> GET_ARG(0);
> GET_ARG(1);
--
Alex Bennée
^ permalink raw reply [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH 09/13] target/arm/arm-semi: Factor out implementation of SYS_SEEK
2019-09-10 14:44 [Qemu-devel] [PATCH 00/13] target/arm: Implement semihosting v2.0 Peter Maydell
` (7 preceding siblings ...)
2019-09-10 14:44 ` [Qemu-devel] [PATCH 08/13] target/arm/arm-semi: Factor out implementation of SYS_ISTTY Peter Maydell
@ 2019-09-10 14:44 ` Peter Maydell
2019-09-12 11:43 ` Alex Bennée
2019-09-10 14:44 ` [Qemu-devel] [PATCH 10/13] target/arm/arm-semi: Factor out implementation of SYS_FLEN Peter Maydell
` (3 subsequent siblings)
12 siblings, 1 reply; 33+ messages in thread
From: Peter Maydell @ 2019-09-10 14:44 UTC (permalink / raw)
To: qemu-arm, qemu-devel
Factor out the implementation of SYS_SEEK via the new function
tables.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
target/arm/arm-semi.c | 32 +++++++++++++++++++++++---------
1 file changed, 23 insertions(+), 9 deletions(-)
diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
index 64ed39fc075..c548ce849e7 100644
--- a/target/arm/arm-semi.c
+++ b/target/arm/arm-semi.c
@@ -350,6 +350,8 @@ typedef uint32_t sys_writefn(TaskState *ts, ARMCPU *cpu, GuestFD *gf,
typedef uint32_t sys_readfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf,
target_ulong buf, uint32_t len);
typedef uint32_t sys_isattyfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf);
+typedef uint32_t sys_seekfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf,
+ target_ulong offset);
static uint32_t host_closefn(TaskState *ts, ARMCPU *cpu, GuestFD *gf)
{
@@ -405,6 +407,16 @@ static uint32_t host_isattyfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf)
return isatty(gf->hostfd);
}
+static uint32_t host_seekfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf,
+ target_ulong offset)
+{
+ uint32_t ret = set_swi_errno(ts, lseek(gf->hostfd, offset, SEEK_SET));
+ if (ret == (uint32_t)-1) {
+ return -1;
+ }
+ return 0;
+}
+
static uint32_t gdb_closefn(TaskState *ts, ARMCPU *cpu, GuestFD *gf)
{
return arm_gdb_syscall(cpu, arm_semi_cb, "close,%x", gf->hostfd);
@@ -431,11 +443,19 @@ static uint32_t gdb_isattyfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf)
return arm_gdb_syscall(cpu, arm_semi_cb, "isatty,%x", gf->hostfd);
}
+static uint32_t gdb_seekfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf,
+ target_ulong offset)
+{
+ return arm_gdb_syscall(cpu, arm_semi_cb, "lseek,%x,%x,0",
+ gf->hostfd, offset);
+}
+
typedef struct GuestFDFunctions {
sys_closefn *closefn;
sys_writefn *writefn;
sys_readfn *readfn;
sys_isattyfn *isattyfn;
+ sys_seekfn *seekfn;
} GuestFDFunctions;
static const GuestFDFunctions guestfd_fns[] = {
@@ -444,12 +464,14 @@ static const GuestFDFunctions guestfd_fns[] = {
.writefn = host_writefn,
.readfn = host_readfn,
.isattyfn = host_isattyfn,
+ .seekfn = host_seekfn,
},
[GuestFDGDB] = {
.closefn = gdb_closefn,
.writefn = gdb_writefn,
.readfn = gdb_readfn,
.isattyfn = gdb_isattyfn,
+ .seekfn = gdb_seekfn,
},
};
@@ -625,15 +647,7 @@ target_ulong do_arm_semihosting(CPUARMState *env)
return set_swi_errno(ts, -1);
}
- if (use_gdb_syscalls()) {
- return arm_gdb_syscall(cpu, arm_semi_cb, "lseek,%x,%x,0",
- gf->hostfd, arg1);
- } else {
- ret = set_swi_errno(ts, lseek(gf->hostfd, arg1, SEEK_SET));
- if (ret == (uint32_t)-1)
- return -1;
- return 0;
- }
+ return guestfd_fns[gf->type].seekfn(ts, cpu, gf, arg1);
case TARGET_SYS_FLEN:
GET_ARG(0);
--
2.20.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [PATCH 09/13] target/arm/arm-semi: Factor out implementation of SYS_SEEK
2019-09-10 14:44 ` [Qemu-devel] [PATCH 09/13] target/arm/arm-semi: Factor out implementation of SYS_SEEK Peter Maydell
@ 2019-09-12 11:43 ` Alex Bennée
0 siblings, 0 replies; 33+ messages in thread
From: Alex Bennée @ 2019-09-12 11:43 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-arm
Peter Maydell <peter.maydell@linaro.org> writes:
> Factor out the implementation of SYS_SEEK via the new function
> tables.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> ---
> target/arm/arm-semi.c | 32 +++++++++++++++++++++++---------
> 1 file changed, 23 insertions(+), 9 deletions(-)
>
> diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
> index 64ed39fc075..c548ce849e7 100644
> --- a/target/arm/arm-semi.c
> +++ b/target/arm/arm-semi.c
> @@ -350,6 +350,8 @@ typedef uint32_t sys_writefn(TaskState *ts, ARMCPU *cpu, GuestFD *gf,
> typedef uint32_t sys_readfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf,
> target_ulong buf, uint32_t len);
> typedef uint32_t sys_isattyfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf);
> +typedef uint32_t sys_seekfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf,
> + target_ulong offset);
>
> static uint32_t host_closefn(TaskState *ts, ARMCPU *cpu, GuestFD *gf)
> {
> @@ -405,6 +407,16 @@ static uint32_t host_isattyfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf)
> return isatty(gf->hostfd);
> }
>
> +static uint32_t host_seekfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf,
> + target_ulong offset)
> +{
> + uint32_t ret = set_swi_errno(ts, lseek(gf->hostfd, offset, SEEK_SET));
> + if (ret == (uint32_t)-1) {
> + return -1;
> + }
> + return 0;
> +}
> +
> static uint32_t gdb_closefn(TaskState *ts, ARMCPU *cpu, GuestFD *gf)
> {
> return arm_gdb_syscall(cpu, arm_semi_cb, "close,%x", gf->hostfd);
> @@ -431,11 +443,19 @@ static uint32_t gdb_isattyfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf)
> return arm_gdb_syscall(cpu, arm_semi_cb, "isatty,%x", gf->hostfd);
> }
>
> +static uint32_t gdb_seekfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf,
> + target_ulong offset)
> +{
> + return arm_gdb_syscall(cpu, arm_semi_cb, "lseek,%x,%x,0",
> + gf->hostfd, offset);
> +}
> +
> typedef struct GuestFDFunctions {
> sys_closefn *closefn;
> sys_writefn *writefn;
> sys_readfn *readfn;
> sys_isattyfn *isattyfn;
> + sys_seekfn *seekfn;
> } GuestFDFunctions;
>
> static const GuestFDFunctions guestfd_fns[] = {
> @@ -444,12 +464,14 @@ static const GuestFDFunctions guestfd_fns[] = {
> .writefn = host_writefn,
> .readfn = host_readfn,
> .isattyfn = host_isattyfn,
> + .seekfn = host_seekfn,
> },
> [GuestFDGDB] = {
> .closefn = gdb_closefn,
> .writefn = gdb_writefn,
> .readfn = gdb_readfn,
> .isattyfn = gdb_isattyfn,
> + .seekfn = gdb_seekfn,
> },
> };
>
> @@ -625,15 +647,7 @@ target_ulong do_arm_semihosting(CPUARMState *env)
> return set_swi_errno(ts, -1);
> }
>
> - if (use_gdb_syscalls()) {
> - return arm_gdb_syscall(cpu, arm_semi_cb, "lseek,%x,%x,0",
> - gf->hostfd, arg1);
> - } else {
> - ret = set_swi_errno(ts, lseek(gf->hostfd, arg1, SEEK_SET));
> - if (ret == (uint32_t)-1)
> - return -1;
> - return 0;
> - }
> + return guestfd_fns[gf->type].seekfn(ts, cpu, gf, arg1);
> case TARGET_SYS_FLEN:
> GET_ARG(0);
--
Alex Bennée
^ permalink raw reply [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH 10/13] target/arm/arm-semi: Factor out implementation of SYS_FLEN
2019-09-10 14:44 [Qemu-devel] [PATCH 00/13] target/arm: Implement semihosting v2.0 Peter Maydell
` (8 preceding siblings ...)
2019-09-10 14:44 ` [Qemu-devel] [PATCH 09/13] target/arm/arm-semi: Factor out implementation of SYS_SEEK Peter Maydell
@ 2019-09-10 14:44 ` Peter Maydell
2019-09-12 11:43 ` [Qemu-devel] [Qemu-arm] " Alex Bennée
2019-09-10 14:44 ` [Qemu-devel] [PATCH 11/13] target/arm/arm-semi: Implement support for semihosting feature detection Peter Maydell
` (2 subsequent siblings)
12 siblings, 1 reply; 33+ messages in thread
From: Peter Maydell @ 2019-09-10 14:44 UTC (permalink / raw)
To: qemu-arm, qemu-devel
Factor out the implementation of SYS_FLEN via the new
function tables.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
target/arm/arm-semi.c | 31 +++++++++++++++++++++----------
1 file changed, 21 insertions(+), 10 deletions(-)
diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
index c548ce849e7..f9019b00b8d 100644
--- a/target/arm/arm-semi.c
+++ b/target/arm/arm-semi.c
@@ -352,6 +352,7 @@ typedef uint32_t sys_readfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf,
typedef uint32_t sys_isattyfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf);
typedef uint32_t sys_seekfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf,
target_ulong offset);
+typedef uint32_t sys_flenfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf);
static uint32_t host_closefn(TaskState *ts, ARMCPU *cpu, GuestFD *gf)
{
@@ -417,6 +418,16 @@ static uint32_t host_seekfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf,
return 0;
}
+static uint32_t host_flenfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf)
+{
+ struct stat buf;
+ uint32_t ret = set_swi_errno(ts, fstat(gf->hostfd, &buf));
+ if (ret == (uint32_t)-1) {
+ return -1;
+ }
+ return buf.st_size;
+}
+
static uint32_t gdb_closefn(TaskState *ts, ARMCPU *cpu, GuestFD *gf)
{
return arm_gdb_syscall(cpu, arm_semi_cb, "close,%x", gf->hostfd);
@@ -450,12 +461,19 @@ static uint32_t gdb_seekfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf,
gf->hostfd, offset);
}
+static uint32_t gdb_flenfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf)
+{
+ return arm_gdb_syscall(cpu, arm_semi_flen_cb, "fstat,%x,%x",
+ gf->hostfd, arm_flen_buf(cpu));
+}
+
typedef struct GuestFDFunctions {
sys_closefn *closefn;
sys_writefn *writefn;
sys_readfn *readfn;
sys_isattyfn *isattyfn;
sys_seekfn *seekfn;
+ sys_flenfn *flenfn;
} GuestFDFunctions;
static const GuestFDFunctions guestfd_fns[] = {
@@ -465,6 +483,7 @@ static const GuestFDFunctions guestfd_fns[] = {
.readfn = host_readfn,
.isattyfn = host_isattyfn,
.seekfn = host_seekfn,
+ .flenfn = host_flenfn,
},
[GuestFDGDB] = {
.closefn = gdb_closefn,
@@ -472,6 +491,7 @@ static const GuestFDFunctions guestfd_fns[] = {
.readfn = gdb_readfn,
.isattyfn = gdb_isattyfn,
.seekfn = gdb_seekfn,
+ .flenfn = gdb_flenfn,
},
};
@@ -657,16 +677,7 @@ target_ulong do_arm_semihosting(CPUARMState *env)
return set_swi_errno(ts, -1);
}
- if (use_gdb_syscalls()) {
- return arm_gdb_syscall(cpu, arm_semi_flen_cb, "fstat,%x,%x",
- gf->hostfd, arm_flen_buf(cpu));
- } else {
- struct stat buf;
- ret = set_swi_errno(ts, fstat(gf->hostfd, &buf));
- if (ret == (uint32_t)-1)
- return -1;
- return buf.st_size;
- }
+ return guestfd_fns[gf->type].flenfn(ts, cpu, gf);
case TARGET_SYS_TMPNAM:
qemu_log_mask(LOG_UNIMP, "%s: SYS_TMPNAM not implemented", __func__);
return -1;
--
2.20.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [Qemu-arm] [PATCH 10/13] target/arm/arm-semi: Factor out implementation of SYS_FLEN
2019-09-10 14:44 ` [Qemu-devel] [PATCH 10/13] target/arm/arm-semi: Factor out implementation of SYS_FLEN Peter Maydell
@ 2019-09-12 11:43 ` Alex Bennée
0 siblings, 0 replies; 33+ messages in thread
From: Alex Bennée @ 2019-09-12 11:43 UTC (permalink / raw)
To: qemu-arm; +Cc: qemu-devel
Peter Maydell <peter.maydell@linaro.org> writes:
> Factor out the implementation of SYS_FLEN via the new
> function tables.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> ---
> target/arm/arm-semi.c | 31 +++++++++++++++++++++----------
> 1 file changed, 21 insertions(+), 10 deletions(-)
>
> diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
> index c548ce849e7..f9019b00b8d 100644
> --- a/target/arm/arm-semi.c
> +++ b/target/arm/arm-semi.c
> @@ -352,6 +352,7 @@ typedef uint32_t sys_readfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf,
> typedef uint32_t sys_isattyfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf);
> typedef uint32_t sys_seekfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf,
> target_ulong offset);
> +typedef uint32_t sys_flenfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf);
>
> static uint32_t host_closefn(TaskState *ts, ARMCPU *cpu, GuestFD *gf)
> {
> @@ -417,6 +418,16 @@ static uint32_t host_seekfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf,
> return 0;
> }
>
> +static uint32_t host_flenfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf)
> +{
> + struct stat buf;
> + uint32_t ret = set_swi_errno(ts, fstat(gf->hostfd, &buf));
> + if (ret == (uint32_t)-1) {
> + return -1;
> + }
> + return buf.st_size;
> +}
> +
> static uint32_t gdb_closefn(TaskState *ts, ARMCPU *cpu, GuestFD *gf)
> {
> return arm_gdb_syscall(cpu, arm_semi_cb, "close,%x", gf->hostfd);
> @@ -450,12 +461,19 @@ static uint32_t gdb_seekfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf,
> gf->hostfd, offset);
> }
>
> +static uint32_t gdb_flenfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf)
> +{
> + return arm_gdb_syscall(cpu, arm_semi_flen_cb, "fstat,%x,%x",
> + gf->hostfd, arm_flen_buf(cpu));
> +}
> +
> typedef struct GuestFDFunctions {
> sys_closefn *closefn;
> sys_writefn *writefn;
> sys_readfn *readfn;
> sys_isattyfn *isattyfn;
> sys_seekfn *seekfn;
> + sys_flenfn *flenfn;
> } GuestFDFunctions;
>
> static const GuestFDFunctions guestfd_fns[] = {
> @@ -465,6 +483,7 @@ static const GuestFDFunctions guestfd_fns[] = {
> .readfn = host_readfn,
> .isattyfn = host_isattyfn,
> .seekfn = host_seekfn,
> + .flenfn = host_flenfn,
> },
> [GuestFDGDB] = {
> .closefn = gdb_closefn,
> @@ -472,6 +491,7 @@ static const GuestFDFunctions guestfd_fns[] = {
> .readfn = gdb_readfn,
> .isattyfn = gdb_isattyfn,
> .seekfn = gdb_seekfn,
> + .flenfn = gdb_flenfn,
> },
> };
>
> @@ -657,16 +677,7 @@ target_ulong do_arm_semihosting(CPUARMState *env)
> return set_swi_errno(ts, -1);
> }
>
> - if (use_gdb_syscalls()) {
> - return arm_gdb_syscall(cpu, arm_semi_flen_cb, "fstat,%x,%x",
> - gf->hostfd, arm_flen_buf(cpu));
> - } else {
> - struct stat buf;
> - ret = set_swi_errno(ts, fstat(gf->hostfd, &buf));
> - if (ret == (uint32_t)-1)
> - return -1;
> - return buf.st_size;
> - }
> + return guestfd_fns[gf->type].flenfn(ts, cpu, gf);
> case TARGET_SYS_TMPNAM:
> qemu_log_mask(LOG_UNIMP, "%s: SYS_TMPNAM not implemented", __func__);
> return -1;
--
Alex Bennée
^ permalink raw reply [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH 11/13] target/arm/arm-semi: Implement support for semihosting feature detection
2019-09-10 14:44 [Qemu-devel] [PATCH 00/13] target/arm: Implement semihosting v2.0 Peter Maydell
` (9 preceding siblings ...)
2019-09-10 14:44 ` [Qemu-devel] [PATCH 10/13] target/arm/arm-semi: Factor out implementation of SYS_FLEN Peter Maydell
@ 2019-09-10 14:44 ` Peter Maydell
2019-09-10 17:00 ` Peter Maydell
2019-09-12 11:56 ` [Qemu-devel] [Qemu-arm] " Alex Bennée
2019-09-10 14:44 ` [Qemu-devel] [PATCH 12/13] target/arm/arm-semi: Implement SH_EXT_STDOUT_STDERR extension Peter Maydell
2019-09-10 14:44 ` [Qemu-devel] [PATCH 13/13] target/arm/arm-semi: Implement SH_EXT_EXIT_EXTENDED extension Peter Maydell
12 siblings, 2 replies; 33+ messages in thread
From: Peter Maydell @ 2019-09-10 14:44 UTC (permalink / raw)
To: qemu-arm, qemu-devel
Version 2.0 of the semihosting specification added support for
allowing a guest to detect whether the implementation supported
particular features. This works by the guest opening a magic
file ":semihosting-features", which contains a fixed set of
data with some magic numbers followed by a sequence of bytes
with feature flags. The file is expected to behave sensibly
for the various semihosting calls which operate on files
(SYS_FLEN, SYS_SEEK, etc).
Implement this as another kind of guest FD using our function
table dispatch mechanism. Initially we report no extended
features, so we have just one feature flag byte which is zero.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
target/arm/arm-semi.c | 107 +++++++++++++++++++++++++++++++++++++++++-
1 file changed, 106 insertions(+), 1 deletion(-)
diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
index f9019b00b8d..531084b7799 100644
--- a/target/arm/arm-semi.c
+++ b/target/arm/arm-semi.c
@@ -117,6 +117,7 @@ typedef enum GuestFDType {
GuestFDUnused = 0,
GuestFDHost = 1,
GuestFDGDB = 2,
+ GuestFDFeatureFile = 3,
} GuestFDType;
/*
@@ -125,7 +126,10 @@ typedef enum GuestFDType {
*/
typedef struct GuestFD {
GuestFDType type;
- int hostfd;
+ union {
+ int hostfd;
+ target_ulong featurefile_offset;
+ };
} GuestFD;
static GArray *guestfd_array;
@@ -467,6 +471,87 @@ static uint32_t gdb_flenfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf)
gf->hostfd, arm_flen_buf(cpu));
}
+#define SHFB_MAGIC_0 0x53
+#define SHFB_MAGIC_1 0x48
+#define SHFB_MAGIC_2 0x46
+#define SHFB_MAGIC_3 0x42
+
+static const uint8_t featurefile_data[] = {
+ SHFB_MAGIC_0,
+ SHFB_MAGIC_1,
+ SHFB_MAGIC_2,
+ SHFB_MAGIC_3,
+ 0, /* Feature byte 0 */
+};
+
+static void init_featurefile_guestfd(int guestfd)
+{
+ GuestFD *gf = do_get_guestfd(guestfd);
+
+ assert(gf);
+ gf->type = GuestFDFeatureFile;
+ gf->featurefile_offset = 0;
+}
+
+static uint32_t featurefile_closefn(TaskState *ts, ARMCPU *cpu, GuestFD *gf)
+{
+ /* Nothing to do */
+ return 0;
+}
+
+static uint32_t featurefile_writefn(TaskState *ts, ARMCPU *cpu, GuestFD *gf,
+ target_ulong buf, uint32_t len)
+{
+ /* This fd can never be open for writing */
+ errno = EBADF;
+ return set_swi_errno(ts, -1);
+}
+
+static uint32_t featurefile_readfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf,
+ target_ulong buf, uint32_t len)
+{
+ uint32_t i;
+#ifndef CONFIG_USER_ONLY
+ CPUARMState *env = &cpu->env;
+#endif
+ char *s;
+
+ s = lock_user(VERIFY_WRITE, buf, len, 0);
+ if (!s) {
+ return len;
+ }
+
+ for (i = 0; i < len; i++) {
+ if (gf->featurefile_offset >= sizeof(featurefile_data)) {
+ break;
+ }
+ s[i] = featurefile_data[gf->featurefile_offset];
+ gf->featurefile_offset++;
+ }
+
+ unlock_user(s, buf, len);
+
+ /* Return number of bytes not read */
+ return len - i;
+}
+
+static uint32_t featurefile_isattyfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf)
+{
+ return 0;
+}
+
+static uint32_t featurefile_seekfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf,
+ target_ulong offset)
+{
+ gf->featurefile_offset = offset;
+ return 0;
+}
+
+static uint32_t featurefile_flenfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf)
+{
+ return sizeof(featurefile_data);
+}
+
typedef struct GuestFDFunctions {
sys_closefn *closefn;
sys_writefn *writefn;
@@ -493,6 +578,14 @@ static const GuestFDFunctions guestfd_fns[] = {
.seekfn = gdb_seekfn,
.flenfn = gdb_flenfn,
},
+ [GuestFDFeatureFile] = {
+ .closefn = featurefile_closefn,
+ .writefn = featurefile_writefn,
+ .readfn = featurefile_readfn,
+ .isattyfn = featurefile_isattyfn,
+ .seekfn = featurefile_seekfn,
+ .flenfn = featurefile_flenfn,
+ },
};
/* Read the input value from the argument block; fail the semihosting
@@ -586,6 +679,18 @@ target_ulong do_arm_semihosting(CPUARMState *env)
unlock_user(s, arg0, 0);
return guestfd;
}
+ if (strcmp(s, ":semihosting-features") == 0) {
+ unlock_user(s, arg0, 0);
+ /* We must fail opens for modes other than 0 ('r') or 1 ('rb') */
+ if (arg1 != 0 && arg1 != 1) {
+ dealloc_guestfd(guestfd);
+ errno = EINVAL;
+ return set_swi_errno(ts, -1);
+ }
+ init_featurefile_guestfd(guestfd);
+ return guestfd;
+ }
+
if (use_gdb_syscalls()) {
ret = arm_gdb_syscall(cpu, arm_semi_cb, "open,%s,%x,1a4", arg0,
(int)arg2+1, gdb_open_modeflags[arg1]);
--
2.20.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [PATCH 11/13] target/arm/arm-semi: Implement support for semihosting feature detection
2019-09-10 14:44 ` [Qemu-devel] [PATCH 11/13] target/arm/arm-semi: Implement support for semihosting feature detection Peter Maydell
@ 2019-09-10 17:00 ` Peter Maydell
2019-09-12 11:56 ` [Qemu-devel] [Qemu-arm] " Alex Bennée
1 sibling, 0 replies; 33+ messages in thread
From: Peter Maydell @ 2019-09-10 17:00 UTC (permalink / raw)
To: qemu-arm, QEMU Developers
On Tue, 10 Sep 2019 at 15:44, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> Version 2.0 of the semihosting specification added support for
> allowing a guest to detect whether the implementation supported
> particular features. This works by the guest opening a magic
> file ":semihosting-features", which contains a fixed set of
> data with some magic numbers followed by a sequence of bytes
> with feature flags. The file is expected to behave sensibly
> for the various semihosting calls which operate on files
> (SYS_FLEN, SYS_SEEK, etc).
> @@ -586,6 +679,18 @@ target_ulong do_arm_semihosting(CPUARMState *env)
> unlock_user(s, arg0, 0);
> return guestfd;
> }
> + if (strcmp(s, ":semihosting-features") == 0) {
> + unlock_user(s, arg0, 0);
> + /* We must fail opens for modes other than 0 ('r') or 1 ('rb') */
> + if (arg1 != 0 && arg1 != 1) {
> + dealloc_guestfd(guestfd);
> + errno = EINVAL;
The spec doesn't mandate any particular errno here, but
EACCES would probably be better, since that's the usual error
for trying to open a read-only file for writing.
thanks
-- PMM
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [Qemu-arm] [PATCH 11/13] target/arm/arm-semi: Implement support for semihosting feature detection
2019-09-10 14:44 ` [Qemu-devel] [PATCH 11/13] target/arm/arm-semi: Implement support for semihosting feature detection Peter Maydell
2019-09-10 17:00 ` Peter Maydell
@ 2019-09-12 11:56 ` Alex Bennée
1 sibling, 0 replies; 33+ messages in thread
From: Alex Bennée @ 2019-09-12 11:56 UTC (permalink / raw)
To: qemu-arm; +Cc: qemu-devel
Peter Maydell <peter.maydell@linaro.org> writes:
> Version 2.0 of the semihosting specification added support for
> allowing a guest to detect whether the implementation supported
> particular features. This works by the guest opening a magic
> file ":semihosting-features", which contains a fixed set of
> data with some magic numbers followed by a sequence of bytes
> with feature flags. The file is expected to behave sensibly
> for the various semihosting calls which operate on files
> (SYS_FLEN, SYS_SEEK, etc).
>
> Implement this as another kind of guest FD using our function
> table dispatch mechanism. Initially we report no extended
> features, so we have just one feature flag byte which is zero.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
with your EACCESS suggestion:
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> ---
> target/arm/arm-semi.c | 107 +++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 106 insertions(+), 1 deletion(-)
>
> diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
> index f9019b00b8d..531084b7799 100644
> --- a/target/arm/arm-semi.c
> +++ b/target/arm/arm-semi.c
> @@ -117,6 +117,7 @@ typedef enum GuestFDType {
> GuestFDUnused = 0,
> GuestFDHost = 1,
> GuestFDGDB = 2,
> + GuestFDFeatureFile = 3,
> } GuestFDType;
>
> /*
> @@ -125,7 +126,10 @@ typedef enum GuestFDType {
> */
> typedef struct GuestFD {
> GuestFDType type;
> - int hostfd;
> + union {
> + int hostfd;
> + target_ulong featurefile_offset;
> + };
> } GuestFD;
>
> static GArray *guestfd_array;
> @@ -467,6 +471,87 @@ static uint32_t gdb_flenfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf)
> gf->hostfd, arm_flen_buf(cpu));
> }
>
> +#define SHFB_MAGIC_0 0x53
> +#define SHFB_MAGIC_1 0x48
> +#define SHFB_MAGIC_2 0x46
> +#define SHFB_MAGIC_3 0x42
> +
> +static const uint8_t featurefile_data[] = {
> + SHFB_MAGIC_0,
> + SHFB_MAGIC_1,
> + SHFB_MAGIC_2,
> + SHFB_MAGIC_3,
> + 0, /* Feature byte 0 */
> +};
> +
> +static void init_featurefile_guestfd(int guestfd)
> +{
> + GuestFD *gf = do_get_guestfd(guestfd);
> +
> + assert(gf);
> + gf->type = GuestFDFeatureFile;
> + gf->featurefile_offset = 0;
> +}
> +
> +static uint32_t featurefile_closefn(TaskState *ts, ARMCPU *cpu, GuestFD *gf)
> +{
> + /* Nothing to do */
> + return 0;
> +}
> +
> +static uint32_t featurefile_writefn(TaskState *ts, ARMCPU *cpu, GuestFD *gf,
> + target_ulong buf, uint32_t len)
> +{
> + /* This fd can never be open for writing */
> + errno = EBADF;
> + return set_swi_errno(ts, -1);
> +}
> +
> +static uint32_t featurefile_readfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf,
> + target_ulong buf, uint32_t len)
> +{
> + uint32_t i;
> +#ifndef CONFIG_USER_ONLY
> + CPUARMState *env = &cpu->env;
> +#endif
> + char *s;
> +
> + s = lock_user(VERIFY_WRITE, buf, len, 0);
> + if (!s) {
> + return len;
> + }
> +
> + for (i = 0; i < len; i++) {
> + if (gf->featurefile_offset >= sizeof(featurefile_data)) {
> + break;
> + }
> + s[i] = featurefile_data[gf->featurefile_offset];
> + gf->featurefile_offset++;
> + }
> +
> + unlock_user(s, buf, len);
> +
> + /* Return number of bytes not read */
> + return len - i;
> +}
> +
> +static uint32_t featurefile_isattyfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf)
> +{
> + return 0;
> +}
> +
> +static uint32_t featurefile_seekfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf,
> + target_ulong offset)
> +{
> + gf->featurefile_offset = offset;
> + return 0;
> +}
> +
> +static uint32_t featurefile_flenfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf)
> +{
> + return sizeof(featurefile_data);
> +}
> +
> typedef struct GuestFDFunctions {
> sys_closefn *closefn;
> sys_writefn *writefn;
> @@ -493,6 +578,14 @@ static const GuestFDFunctions guestfd_fns[] = {
> .seekfn = gdb_seekfn,
> .flenfn = gdb_flenfn,
> },
> + [GuestFDFeatureFile] = {
> + .closefn = featurefile_closefn,
> + .writefn = featurefile_writefn,
> + .readfn = featurefile_readfn,
> + .isattyfn = featurefile_isattyfn,
> + .seekfn = featurefile_seekfn,
> + .flenfn = featurefile_flenfn,
> + },
> };
>
> /* Read the input value from the argument block; fail the semihosting
> @@ -586,6 +679,18 @@ target_ulong do_arm_semihosting(CPUARMState *env)
> unlock_user(s, arg0, 0);
> return guestfd;
> }
> + if (strcmp(s, ":semihosting-features") == 0) {
> + unlock_user(s, arg0, 0);
> + /* We must fail opens for modes other than 0 ('r') or 1 ('rb') */
> + if (arg1 != 0 && arg1 != 1) {
> + dealloc_guestfd(guestfd);
> + errno = EINVAL;
> + return set_swi_errno(ts, -1);
> + }
> + init_featurefile_guestfd(guestfd);
> + return guestfd;
> + }
> +
> if (use_gdb_syscalls()) {
> ret = arm_gdb_syscall(cpu, arm_semi_cb, "open,%s,%x,1a4", arg0,
> (int)arg2+1, gdb_open_modeflags[arg1]);
--
Alex Bennée
^ permalink raw reply [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH 12/13] target/arm/arm-semi: Implement SH_EXT_STDOUT_STDERR extension
2019-09-10 14:44 [Qemu-devel] [PATCH 00/13] target/arm: Implement semihosting v2.0 Peter Maydell
` (10 preceding siblings ...)
2019-09-10 14:44 ` [Qemu-devel] [PATCH 11/13] target/arm/arm-semi: Implement support for semihosting feature detection Peter Maydell
@ 2019-09-10 14:44 ` Peter Maydell
2019-09-12 12:05 ` Alex Bennée
2019-09-10 14:44 ` [Qemu-devel] [PATCH 13/13] target/arm/arm-semi: Implement SH_EXT_EXIT_EXTENDED extension Peter Maydell
12 siblings, 1 reply; 33+ messages in thread
From: Peter Maydell @ 2019-09-10 14:44 UTC (permalink / raw)
To: qemu-arm, qemu-devel
SH_EXT_STDOUT_STDERR is a v2.0 semihosting extension: the guest
can open ":tt" with a file mode requesting append access in
order to open stderr, in addition to the existing "open for
read for stdin or write for stdout". Implement this and
report it via the :semihosting-features data.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
target/arm/arm-semi.c | 22 ++++++++++++++++++++--
1 file changed, 20 insertions(+), 2 deletions(-)
diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
index 531084b7799..0df8d4d69d6 100644
--- a/target/arm/arm-semi.c
+++ b/target/arm/arm-semi.c
@@ -476,12 +476,16 @@ static uint32_t gdb_flenfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf)
#define SHFB_MAGIC_2 0x46
#define SHFB_MAGIC_3 0x42
+/* Feature bits reportable in feature byte 0 */
+#define SH_EXT_EXIT_EXTENDED (1 << 0)
+#define SH_EXT_STDOUT_STDERR (1 << 1)
+
static const uint8_t featurefile_data[] = {
SHFB_MAGIC_0,
SHFB_MAGIC_1,
SHFB_MAGIC_2,
SHFB_MAGIC_3,
- 0, /* Feature byte 0 */
+ SH_EXT_STDOUT_STDERR, /* Feature byte 0 */
};
static void init_featurefile_guestfd(int guestfd)
@@ -674,7 +678,21 @@ target_ulong do_arm_semihosting(CPUARMState *env)
}
if (strcmp(s, ":tt") == 0) {
- int result_fileno = arg1 < 4 ? STDIN_FILENO : STDOUT_FILENO;
+ int result_fileno;
+
+ /*
+ * We implement SH_EXT_STDOUT_STDERR, so:
+ * open for read == stdin
+ * open for write == stdout
+ * open for append == stderr
+ */
+ if (arg1 < 4) {
+ result_fileno = STDIN_FILENO;
+ } else if (arg1 < 8) {
+ result_fileno = STDOUT_FILENO;
+ } else {
+ result_fileno = STDERR_FILENO;
+ }
associate_guestfd(guestfd, result_fileno);
unlock_user(s, arg0, 0);
return guestfd;
--
2.20.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [PATCH 12/13] target/arm/arm-semi: Implement SH_EXT_STDOUT_STDERR extension
2019-09-10 14:44 ` [Qemu-devel] [PATCH 12/13] target/arm/arm-semi: Implement SH_EXT_STDOUT_STDERR extension Peter Maydell
@ 2019-09-12 12:05 ` Alex Bennée
2019-09-12 12:09 ` [Qemu-devel] [Qemu-arm] " Peter Maydell
0 siblings, 1 reply; 33+ messages in thread
From: Alex Bennée @ 2019-09-12 12:05 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-arm
Peter Maydell <peter.maydell@linaro.org> writes:
> SH_EXT_STDOUT_STDERR is a v2.0 semihosting extension: the guest
> can open ":tt" with a file mode requesting append access in
> order to open stderr, in addition to the existing "open for
> read for stdin or write for stdout". Implement this and
> report it via the :semihosting-features data.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> target/arm/arm-semi.c | 22 ++++++++++++++++++++--
> 1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
> index 531084b7799..0df8d4d69d6 100644
> --- a/target/arm/arm-semi.c
> +++ b/target/arm/arm-semi.c
> @@ -476,12 +476,16 @@ static uint32_t gdb_flenfn(TaskState *ts, ARMCPU *cpu, GuestFD *gf)
> #define SHFB_MAGIC_2 0x46
> #define SHFB_MAGIC_3 0x42
>
> +/* Feature bits reportable in feature byte 0 */
> +#define SH_EXT_EXIT_EXTENDED (1 << 0)
If you swap 12/13 this could be kept with the related feature. I don't
think one implies the other right?
> +#define SH_EXT_STDOUT_STDERR (1 << 1)
> +
> static const uint8_t featurefile_data[] = {
> SHFB_MAGIC_0,
> SHFB_MAGIC_1,
> SHFB_MAGIC_2,
> SHFB_MAGIC_3,
> - 0, /* Feature byte 0 */
> + SH_EXT_STDOUT_STDERR, /* Feature byte 0 */
> };
>
> static void init_featurefile_guestfd(int guestfd)
> @@ -674,7 +678,21 @@ target_ulong do_arm_semihosting(CPUARMState *env)
> }
>
> if (strcmp(s, ":tt") == 0) {
> - int result_fileno = arg1 < 4 ? STDIN_FILENO : STDOUT_FILENO;
> + int result_fileno;
> +
> + /*
> + * We implement SH_EXT_STDOUT_STDERR, so:
> + * open for read == stdin
> + * open for write == stdout
> + * open for append == stderr
> + */
I love the way the spec documents field2 as an ISO C fopen() mode and
then an extension literally subverts the meaning to be something else.
Where the designers worried about adding a SYS_OPEN_TTY function to the
interface?
Anyway it meets the spec however weird it might be:
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> + if (arg1 < 4) {
> + result_fileno = STDIN_FILENO;
> + } else if (arg1 < 8) {
> + result_fileno = STDOUT_FILENO;
> + } else {
> + result_fileno = STDERR_FILENO;
> + }
> associate_guestfd(guestfd, result_fileno);
> unlock_user(s, arg0, 0);
> return guestfd;
--
Alex Bennée
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [Qemu-arm] [PATCH 12/13] target/arm/arm-semi: Implement SH_EXT_STDOUT_STDERR extension
2019-09-12 12:05 ` Alex Bennée
@ 2019-09-12 12:09 ` Peter Maydell
0 siblings, 0 replies; 33+ messages in thread
From: Peter Maydell @ 2019-09-12 12:09 UTC (permalink / raw)
To: Alex Bennée; +Cc: qemu-arm, QEMU Developers
On Thu, 12 Sep 2019 at 13:05, Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Peter Maydell <peter.maydell@linaro.org> writes:
> > + /*
> > + * We implement SH_EXT_STDOUT_STDERR, so:
> > + * open for read == stdin
> > + * open for write == stdout
> > + * open for append == stderr
> > + */
>
> I love the way the spec documents field2 as an ISO C fopen() mode and
> then an extension literally subverts the meaning to be something else.
> Where the designers worried about adding a SYS_OPEN_TTY function to the
> interface?
It's just extending the existing convention of "open ::tt as
read for stdin and write for stdout" a bit. IIRC some existing
implementations actually did this as a sort of undocumented
extra.
thanks
-- PMM
^ permalink raw reply [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH 13/13] target/arm/arm-semi: Implement SH_EXT_EXIT_EXTENDED extension
2019-09-10 14:44 [Qemu-devel] [PATCH 00/13] target/arm: Implement semihosting v2.0 Peter Maydell
` (11 preceding siblings ...)
2019-09-10 14:44 ` [Qemu-devel] [PATCH 12/13] target/arm/arm-semi: Implement SH_EXT_STDOUT_STDERR extension Peter Maydell
@ 2019-09-10 14:44 ` Peter Maydell
2019-09-12 12:07 ` [Qemu-devel] [Qemu-arm] " Alex Bennée
12 siblings, 1 reply; 33+ messages in thread
From: Peter Maydell @ 2019-09-10 14:44 UTC (permalink / raw)
To: qemu-arm, qemu-devel
SH_EXT_EXIT_EXTENDED is a v2.0 semihosting extension: it
indicates that the implementation supports the SYS_EXIT_EXTENDED
function. This function allows both A64 and A32/T32 guests to
exit with a specified exit status, unlike the older SYS_EXIT
function which only allowed this for A64 guests. Implement
this extension.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
target/arm/arm-semi.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
index 0df8d4d69d6..3900bd4e1e6 100644
--- a/target/arm/arm-semi.c
+++ b/target/arm/arm-semi.c
@@ -66,6 +66,7 @@ typedef void TaskState;
#define TARGET_SYS_HEAPINFO 0x16
#define TARGET_SYS_EXIT 0x18
#define TARGET_SYS_SYNCCACHE 0x19
+#define TARGET_SYS_EXIT_EXTENDED 0x20
/* ADP_Stopped_ApplicationExit is used for exit(0),
* anything else is implemented as exit(1) */
@@ -485,7 +486,7 @@ static const uint8_t featurefile_data[] = {
SHFB_MAGIC_1,
SHFB_MAGIC_2,
SHFB_MAGIC_3,
- SH_EXT_STDOUT_STDERR, /* Feature byte 0 */
+ SH_EXT_EXIT_EXTENDED | SH_EXT_STDOUT_STDERR, /* Feature byte 0 */
};
static void init_featurefile_guestfd(int guestfd)
@@ -1026,11 +1027,14 @@ target_ulong do_arm_semihosting(CPUARMState *env)
return 0;
}
case TARGET_SYS_EXIT:
- if (is_a64(env)) {
+ case TARGET_SYS_EXIT_EXTENDED:
+ if (nr == TARGET_SYS_EXIT_EXTENDED || is_a64(env)) {
/*
- * The A64 version of this call takes a parameter block,
+ * The A64 version of SYS_EXIT takes a parameter block,
* so the application-exit type can return a subcode which
* is the exit status code from the application.
+ * SYS_EXIT_EXTENDED is an a new-in-v2.0 optional function
+ * which allows A32/T32 guests to also provide a status code.
*/
GET_ARG(0);
GET_ARG(1);
@@ -1042,8 +1046,10 @@ target_ulong do_arm_semihosting(CPUARMState *env)
}
} else {
/*
- * ARM specifies only Stopped_ApplicationExit as normal
- * exit, everything else is considered an error
+ * The A32/T32 version of SYS_EXIT specifies only
+ * Stopped_ApplicationExit as normal exit, but does not
+ * allow the guest to specify the exit status code.
+ * Everything else is considered an error.
*/
ret = (args == ADP_Stopped_ApplicationExit) ? 0 : 1;
}
--
2.20.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [Qemu-arm] [PATCH 13/13] target/arm/arm-semi: Implement SH_EXT_EXIT_EXTENDED extension
2019-09-10 14:44 ` [Qemu-devel] [PATCH 13/13] target/arm/arm-semi: Implement SH_EXT_EXIT_EXTENDED extension Peter Maydell
@ 2019-09-12 12:07 ` Alex Bennée
0 siblings, 0 replies; 33+ messages in thread
From: Alex Bennée @ 2019-09-12 12:07 UTC (permalink / raw)
To: qemu-arm; +Cc: qemu-devel
Peter Maydell <peter.maydell@linaro.org> writes:
> SH_EXT_EXIT_EXTENDED is a v2.0 semihosting extension: it
> indicates that the implementation supports the SYS_EXIT_EXTENDED
> function. This function allows both A64 and A32/T32 guests to
> exit with a specified exit status, unlike the older SYS_EXIT
> function which only allowed this for A64 guests. Implement
> this extension.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Aside from the ordering nit mentioned in the previous commit:
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> ---
> target/arm/arm-semi.c | 16 +++++++++++-----
> 1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
> index 0df8d4d69d6..3900bd4e1e6 100644
> --- a/target/arm/arm-semi.c
> +++ b/target/arm/arm-semi.c
> @@ -66,6 +66,7 @@ typedef void TaskState;
> #define TARGET_SYS_HEAPINFO 0x16
> #define TARGET_SYS_EXIT 0x18
> #define TARGET_SYS_SYNCCACHE 0x19
> +#define TARGET_SYS_EXIT_EXTENDED 0x20
>
> /* ADP_Stopped_ApplicationExit is used for exit(0),
> * anything else is implemented as exit(1) */
> @@ -485,7 +486,7 @@ static const uint8_t featurefile_data[] = {
> SHFB_MAGIC_1,
> SHFB_MAGIC_2,
> SHFB_MAGIC_3,
> - SH_EXT_STDOUT_STDERR, /* Feature byte 0 */
> + SH_EXT_EXIT_EXTENDED | SH_EXT_STDOUT_STDERR, /* Feature byte 0 */
> };
>
> static void init_featurefile_guestfd(int guestfd)
> @@ -1026,11 +1027,14 @@ target_ulong do_arm_semihosting(CPUARMState *env)
> return 0;
> }
> case TARGET_SYS_EXIT:
> - if (is_a64(env)) {
> + case TARGET_SYS_EXIT_EXTENDED:
> + if (nr == TARGET_SYS_EXIT_EXTENDED || is_a64(env)) {
> /*
> - * The A64 version of this call takes a parameter block,
> + * The A64 version of SYS_EXIT takes a parameter block,
> * so the application-exit type can return a subcode which
> * is the exit status code from the application.
> + * SYS_EXIT_EXTENDED is an a new-in-v2.0 optional function
> + * which allows A32/T32 guests to also provide a status code.
> */
> GET_ARG(0);
> GET_ARG(1);
> @@ -1042,8 +1046,10 @@ target_ulong do_arm_semihosting(CPUARMState *env)
> }
> } else {
> /*
> - * ARM specifies only Stopped_ApplicationExit as normal
> - * exit, everything else is considered an error
> + * The A32/T32 version of SYS_EXIT specifies only
> + * Stopped_ApplicationExit as normal exit, but does not
> + * allow the guest to specify the exit status code.
> + * Everything else is considered an error.
> */
> ret = (args == ADP_Stopped_ApplicationExit) ? 0 : 1;
> }
--
Alex Bennée
^ permalink raw reply [flat|nested] 33+ messages in thread