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

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

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