Re: [Qemu-devel] [RFC PATCH 06/11] target/arm: use the common interface for WRITE0/WRITEC in arm-semi

[Miroslav Rezanina]

----- Original Message -----
> From: "Alex Bennée" <address@hidden>
> To: "Miroslav Rezanina" <address@hidden>
> Cc: address@hidden, "Peter Maydell" <address@hidden>, "Riku Voipio" 
> <address@hidden>,
> address@hidden, "Laurent Vivier" <address@hidden>
> Sent: Friday, May 31, 2019 4:28:02 PM
> Subject: Re: [Qemu-devel] [RFC PATCH 06/11] target/arm: use the common 
> interface for WRITE0/WRITEC in arm-semi
> 
> 
> Miroslav Rezanina <address@hidden> writes:
> 
> > ----- Original Message -----
> >> From: "Alex Bennée" <address@hidden>
> >> To: "Miroslav Rezanina" <address@hidden>
> >> Cc: address@hidden, "Peter Maydell" <address@hidden>,
> >> "Riku Voipio" <address@hidden>,
> >> address@hidden, "Laurent Vivier" <address@hidden>
> >> Sent: Friday, May 31, 2019 3:16:38 PM
> >> Subject: Re: [Qemu-devel] [RFC PATCH 06/11] target/arm: use the common
> >> interface for WRITE0/WRITEC in arm-semi
> >>
> >>
> >> Miroslav Rezanina <address@hidden> writes:
> >>
> >> > ----- Original Message -----
> >> >> From: "Alex Bennée" <address@hidden>
> >> >> To: "Miroslav Rezanina" <address@hidden>
> >> >> Cc: address@hidden, "Peter Maydell" <address@hidden>,
> >> >> "Riku Voipio" <address@hidden>,
> >> >> address@hidden, "Laurent Vivier" <address@hidden>
> >> >> Sent: Friday, May 31, 2019 1:08:06 PM
> >> >> Subject: Re: [Qemu-devel] [RFC PATCH 06/11] target/arm: use the common
> >> >> interface for WRITE0/WRITEC in arm-semi
> >> >>
> >> >>
> >> >> Miroslav Rezanina <address@hidden> writes:
> >> >>
> >> >> > On Tue, May 14, 2019 at 04:52:56PM +0100, Alex Bennée wrote:
> >> >> >> Now we have a common semihosting console interface use that for our
> >> >> >> string output. However ARM is currently unique in also supporting
> >> >> >> semihosting for linux-user so we need to replicate the API in
> >> >> >> linux-user. If other architectures gain this support we can move the
> >> >> >> file later.
> >> >> >>
> >> >> >> Signed-off-by: Alex Bennée <address@hidden>
> >> >> >> ---
> >> >> >>  linux-user/Makefile.objs  |  2 ++
> >> >> >>  linux-user/arm/semihost.c | 24 ++++++++++++++++++++++++
> >> >> >>  target/arm/arm-semi.c     | 31 ++++++-------------------------
> >> >> >>  3 files changed, 32 insertions(+), 25 deletions(-)
> >> >> >>  create mode 100644 linux-user/arm/semihost.c
> >> >> >>
> >> >> >> diff --git a/linux-user/Makefile.objs b/linux-user/Makefile.objs
> >> >> >> index 769b8d83362..285c5dfa17a 100644
> >> >> >> --- a/linux-user/Makefile.objs
> >> >> >> +++ b/linux-user/Makefile.objs
> >> >> >> @@ -6,4 +6,6 @@ obj-y = main.o syscall.o strace.o mmap.o signal.o \
> >> >> >>  obj-$(TARGET_HAS_BFLT) += flatload.o
> >> >> >>  obj-$(TARGET_I386) += vm86.o
> >> >> >>  obj-$(TARGET_ARM) += arm/nwfpe/
> >> >> >> +obj-$(TARGET_ARM) += arm/semihost.o
> >> >> >> +obj-$(TARGET_AARCH64) += arm/semihost.o
> >> >> >>  obj-$(TARGET_M68K) += m68k-sim.o
> >> >> >> diff --git a/linux-user/arm/semihost.c b/linux-user/arm/semihost.c
> >> >> >> new file mode 100644
> >> >> >> index 00000000000..9554102a855
> >> >> >> --- /dev/null
> >> >> >> +++ b/linux-user/arm/semihost.c
> >> >> >> @@ -0,0 +1,24 @@
> >> >> >> +/*
> >> >> >> + * ARM Semihosting Console Support
> >> >> >> + *
> >> >> >> + * Copyright (c) 2019 Linaro Ltd
> >> >> >> + *
> >> >> >> + * Currently ARM is unique in having support for semihosting
> >> >> >> support
> >> >> >> + * in linux-user. So for now we implement the common console API
> >> >> >> but
> >> >> >> + * just for arm linux-user.
> >> >> >> + *
> >> >> >> + * SPDX-License-Identifier: GPL-2.0-or-later
> >> >> >> + */
> >> >> >> +
> >> >> >> +#include "qemu/osdep.h"
> >> >> >> +#include "cpu.h"
> >> >> >> +#include "hw/semihosting/console.h"
> >> >> >> +#include "qemu.h"
> >> >> >> +
> >> >> >> +int qemu_semihosting_console_out(CPUArchState *env, target_ulong
> >> >> >> addr,
> >> >> >> int len)
> >> >> >> +{
> >> >> >> +    void *s = lock_user_string(addr);
> >> >> >> +    len = write(STDERR_FILENO, s, len ? len : strlen(s));
> >> >> >> +    unlock_user(s, addr, 0);
> >> >> >> +    return len;
> >> >> >> +}
> >> >> >> diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
> >> >> >> index 9e5a414dd89..253c66b172a 100644
> >> >> >> --- a/target/arm/arm-semi.c
> >> >> >> +++ b/target/arm/arm-semi.c
> >> >> >> @@ -27,6 +27,7 @@
> >> >> >>
> >> >> >>  #include "cpu.h"
> >> >> >>  #include "hw/semihosting/semihost.h"
> >> >> >> +#include "hw/semihosting/console.h"
> >> >> >>  #ifdef CONFIG_USER_ONLY
> >> >> >>  #include "qemu.h"
> >> >> >>
> >> >> >> @@ -314,32 +315,12 @@ target_ulong do_arm_semihosting(CPUARMState
> >> >> >> *env)
> >> >> >>              return set_swi_errno(ts, close(arg0));
> >> >> >>          }
> >> >> >>      case TARGET_SYS_WRITEC:
> >> >> >> -        {
> >> >> >> -          char c;
> >> >> >> -
> >> >> >> -          if (get_user_u8(c, args))
> >> >> >> -              /* FIXME - should this error code be -TARGET_EFAULT ?
> >> >> >> */
> >> >> >> -              return (uint32_t)-1;
> >> >> >> -          /* Write to debug console.  stderr is near enough.  */
> >> >> >> -          if (use_gdb_syscalls()) {
> >> >> >> -                return arm_gdb_syscall(cpu, arm_semi_cb,
> >> >> >> "write,2,%x,1",
> >> >> >> args);
> >> >> >> -          } else {
> >> >> >> -                return write(STDERR_FILENO, &c, 1);
> >> >> >> -          }
> >> >> >> -        }
> >> >> >> +    {
> >> >> >> +        qemu_semihosting_console_out(env, args, 1);
> >> >> >> +        return 0xdeadbeef;
> >> >> >> +    }
> >> >> >>      case TARGET_SYS_WRITE0:
> >> >> >> -        if (!(s = lock_user_string(args)))
> >> >> >> -            /* FIXME - should this error code be -TARGET_EFAULT ?
> >> >> >> */
> >> >> >> -            return (uint32_t)-1;
> >> >> >> -        len = strlen(s);
> >> >> >> -        if (use_gdb_syscalls()) {
> >> >> >> -            return arm_gdb_syscall(cpu, arm_semi_cb,
> >> >> >> "write,2,%x,%x",
> >> >> >> -                                   args, len);
> >> >> >> -        } else {
> >> >> >> -            ret = write(STDERR_FILENO, s, len);
> >> >> >> -        }
> >> >> >> -        unlock_user(s, args, 0);
> >> >> >> -        return ret;
> >> >> >> +        return qemu_semihosting_console_out(env, args, 0);
> >> >> >>      case TARGET_SYS_WRITE:
> >> >> >>          GET_ARG(0);
> >> >> >>          GET_ARG(1);
> >> >> >> --
> >> >> >> 2.20.1
> >> >> >>
> >> >> >>
> >> >> >
> >> >> > Hi Alex,
> >> >> >
> >> >> > this patch breaks build for softmmu target when CONFIG_SEMIHOSTING is
> >> >> > not
> >> >> > enabled as qemu_semihosting_console_out
> >> >> > is not defined in such case - neither linux-user/arm/semihost.c nor
> >> >> > hw/semihosting/console.c compiled and function
> >> >> > is not in stubs/semihost.c
> >> >>
> >> >> How do you do that? I tried ../../configure --without-default-devices
> >> >> and that still builds for me.
> >> >
> >> > It's usual RHEL way - use --without-default-devices and use specific
> >> > list of enabled devices (this mean disable CONFIG_SEMIHOSTING in
> >> > default_config/* file).
> >>
> >> OK - so from the upstream source tree CONFIG_SEMIHOSTING is still =y
> >> (but I can see most of them are now =n). Isn't the simplest solution to
> >> fix-up your version of the default_config file to include SEMIHOSTING?
> >>
> >
> > It's fix but it goes against our policy of handling CONFIG options so we
> > would prefer to have this fixed - otherwise there's no meaning in having
> > config option if you can't disable it.
> 
> Is that what it means? For my part it means we don't build in
> CONFIG_SEMIHOSTING for the arches that don't need it (which we were
> before). Granted it only really simplified the vl.c parsing and dropped
> support for semihosting for the linux-user targets (except ARM).
> 
> >
> >> Is this an out-of-tree RHEL addition?
> >>
> >
> > No, it's RHEL device config handling.
> 
> I mean how do I replicate this failure on the upstream source tree?

Easiest way should be probably commenting out the CONFIG_SEMIHOSTING=y
line in default_config/arm-softmmu.mak. It is not auto-include so the
build will be run without it and fail.

Mirek

> 
> >
> > Mirek
> >
> >> >>
> >> >> But I suspect what's needed is to change:
> >> >>
> >> >> #ifndef CONFIG_USER_ONLY
> >> >>
> >> >> to
> >> >>
> >> >> #ifdef CONFIG_SEMIHOSTING
> >> >>
> >> >> to the relevant headers and helper bits.
> >> >
> >> > Yeah, have to find out what are relevant pieces.
> >> >
> >> > Mirek
> >> >
> >> >>
> >> >> >
> >> >> > Mirek
> >> >>
> >> >>
> >> >> --
> >> >> Alex Bennée
> >> >>
> >>
> >>
> >> --
> >> Alex Bennée
> >>
> 
> 
> --
> Alex Bennée
> 

-- 
Miroslav Rezanina
Software Engineer - Virtualization Team Maintainer