[Qemu-devel] [PATCH 0/4] make busybox udhcpc happy

[Qemu-devel] [PATCH 0/4] make busybox udhcpc happy

[Joakim Tjernlund]

Fix a endian/socket bugs so that busybox udhcpc for
qemu-user(host amd64, target ppc) works.

Note, the "[PATCH] SIOCGIFINDEX: fix typo" patch sent yesterday
is also needed for udhcpc to work.

BTW, busybox still generates a
 Unsupported setsockopt level=263 optname=8 (AUXDATA)
but that seems harmless.

Joakim Tjernlund (4):
  qemu-user: Impl. setsockopt(SO_BINDTODEVICE)
  linux-user: impl. sockaddr_ll
  alloca one extra byte sockets
  ppc: remove excessive logging

 linux-user/main.c         |  1 -
 linux-user/syscall.c      | 26 +++++++++++++++++++++++---
 linux-user/syscall_defs.h | 10 ++++++++++
 3 files changed, 33 insertions(+), 4 deletions(-)

-- 
1.8.5.5

[Qemu-devel] [PATCH 1/4] qemu-user: Impl. setsockopt(SO_BINDTODEVICE)

[Joakim Tjernlund]

Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
---
 linux-user/syscall.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 5a272d3..1380f4e 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -1497,6 +1497,18 @@ set_timeout:
                 unlock_user_struct(tfprog, optval_addr, 1);
                 return ret;
         }
+	case TARGET_SO_BINDTODEVICE:
+	{
+		char *dev_ifname;
+
+		if (optlen > IFNAMSIZ)
+			return -TARGET_EINVAL;
+
+		dev_ifname = lock_user(VERIFY_READ, optval_addr, optlen, 1);
+		ret = get_errno(setsockopt(sockfd, level, optname, dev_ifname, optlen));
+		unlock_user (dev_ifname, optval_addr, 0);
+		return ret;
+	}
             /* Options with 'int' argument.  */
         case TARGET_SO_DEBUG:
 		optname = SO_DEBUG;
-- 
1.8.5.5

[Qemu-devel] [PATCH 2/4] linux-user: impl. sockaddr_ll

[Joakim Tjernlund]

Used by AF_PACKET sockets

Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
---
 linux-user/syscall.c      |  8 ++++++++
 linux-user/syscall_defs.h | 10 ++++++++++
 2 files changed, 18 insertions(+)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 1380f4e..a0e1ccc 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -1140,6 +1140,14 @@ static inline abi_long target_to_host_sockaddr(struct sockaddr *addr,
 
     memcpy(addr, target_saddr, len);
     addr->sa_family = sa_family;
+    if (sa_family == AF_PACKET) {
+	    struct target_sockaddr_ll *target_lladdr, *lladdr;
+	    target_lladdr = (void *)target_saddr;
+	    lladdr = (void *)addr;
+
+	    lladdr->sll_ifindex = tswap32(target_lladdr->sll_ifindex);
+	    lladdr->sll_hatype = tswap16(target_lladdr->sll_hatype);
+    }
     unlock_user(target_saddr, target_addr, 0);
 
     return 0;
diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
index 8563027..329c9c5 100644
--- a/linux-user/syscall_defs.h
+++ b/linux-user/syscall_defs.h
@@ -121,6 +121,16 @@ struct target_sockaddr {
     uint8_t sa_data[14];
 };
 
+struct target_sockaddr_ll {
+	uint16_t sll_family;   /* Always AF_PACKET */
+	uint16_t sll_protocol; /* Physical layer protocol */
+	int      sll_ifindex;  /* Interface number */
+	uint16_t sll_hatype;   /* ARP hardware type */
+	uint8_t  sll_pkttype;  /* Packet type */
+	uint8_t  sll_halen;    /* Length of address */
+	uint8_t  sll_addr[8];  /* Physical layer address */
+};
+
 struct target_sock_filter {
     abi_ushort code;
     uint8_t jt;
-- 
1.8.5.5

[Qemu-devel] [PATCH 3/4] alloca one extra byte sockets

[Joakim Tjernlund]

target_to_host_sockaddr() may increase the lenth with 1 byte
for AF_UNIX sockets so allocate 1 extra byte.

Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
---
 linux-user/syscall.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index a0e1ccc..8853c4e 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -1978,7 +1978,7 @@ static abi_long do_connect(int sockfd, abi_ulong target_addr,
         return -TARGET_EINVAL;
     }
 
-    addr = alloca(addrlen);
+    addr = alloca(addrlen+1);
 
     ret = target_to_host_sockaddr(addr, target_addr, addrlen);
     if (ret)
@@ -1999,7 +1999,7 @@ static abi_long do_sendrecvmsg_locked(int fd, struct target_msghdr *msgp,
 
     if (msgp->msg_name) {
         msg.msg_namelen = tswap32(msgp->msg_namelen);
-        msg.msg_name = alloca(msg.msg_namelen);
+        msg.msg_name = alloca(msg.msg_namelen+1);
         ret = target_to_host_sockaddr(msg.msg_name, tswapal(msgp->msg_name),
                                 msg.msg_namelen);
         if (ret) {
@@ -2262,7 +2262,7 @@ static abi_long do_sendto(int fd, abi_ulong msg, size_t len, int flags,
     if (!host_msg)
         return -TARGET_EFAULT;
     if (target_addr) {
-        addr = alloca(addrlen);
+        addr = alloca(addrlen+1);
         ret = target_to_host_sockaddr(addr, target_addr, addrlen);
         if (ret) {
             unlock_user(host_msg, msg, 0);
-- 
1.8.5.5

[Qemu-devel] [PATCH 4/4] ppc: remove excessive logging

[Joakim Tjernlund]

ppc logs every type of Invalid instruction. This generates a lot
of garbage on console when sshd/ssh_keygen executes as
they try various insn to optimize its performance.
The invalid operation log is still there so an unknown insn
will still be logged.

Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
---
 linux-user/main.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/linux-user/main.c b/linux-user/main.c
index b453a39..71a33c7 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -1698,7 +1698,6 @@ void cpu_loop(CPUPPCState *env)
                 }
                 break;
             case POWERPC_EXCP_INVAL:
-                EXCP_DUMP(env, "Invalid instruction\n");
                 info.si_signo = TARGET_SIGILL;
                 info.si_errno = 0;
                 switch (env->error_code & 0xF) {
-- 
1.8.5.5

Re: [Qemu-devel] [PATCH 1/4] qemu-user: Impl. setsockopt(SO_BINDTODEVICE)

[Peter Maydell]

On 11 July 2014 16:18, Joakim Tjernlund <Joakim.Tjernlund@transmode.se> wrote:
> Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> ---
>  linux-user/syscall.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 5a272d3..1380f4e 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -1497,6 +1497,18 @@ set_timeout:
>                  unlock_user_struct(tfprog, optval_addr, 1);
>                  return ret;
>          }
> +       case TARGET_SO_BINDTODEVICE:
> +       {
> +               char *dev_ifname;
> +
> +               if (optlen > IFNAMSIZ)
> +                       return -TARGET_EINVAL;

This needs braces for our coding style; you might like
to run your patches through scripts/checkpatch.pl, which
will warn about this kind of thing.

Also, the kernel implementation handles overlong
lengths via
     if (optlen > IFNAMSIZ - 1) {
         optlen = IFNAMSIZ - 1;
     }

which effectively truncates overlong strings rather
than rejecting them. We should do the same (there will
be complication required to ensure we pass the kernel
a NUL-terminated string even if we don't get one from
the guest; may be easiest to use a local array, given
IFNAMSIZ is only 16).

> +
> +               dev_ifname = lock_user(VERIFY_READ, optval_addr, optlen, 1);
> +               ret = get_errno(setsockopt(sockfd, level, optname, dev_ifname, optlen));

This is passing the target optname through to the
host kernel; you need to set 'optname = SO_BINDTODEVICE;'
(look at the other cases in this switch).

> +               unlock_user (dev_ifname, optval_addr, 0);
> +               return ret;
> +       }
>              /* Options with 'int' argument.  */
>          case TARGET_SO_DEBUG:
>                 optname = SO_DEBUG;
> --
> 1.8.5.5

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/4] linux-user: impl. sockaddr_ll

[Peter Maydell]

On 11 July 2014 16:18, Joakim Tjernlund <Joakim.Tjernlund@transmode.se> wrote:
> Used by AF_PACKET sockets
>
> Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> ---
>  linux-user/syscall.c      |  8 ++++++++
>  linux-user/syscall_defs.h | 10 ++++++++++
>  2 files changed, 18 insertions(+)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 1380f4e..a0e1ccc 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -1140,6 +1140,14 @@ static inline abi_long target_to_host_sockaddr(struct sockaddr *addr,
>
>      memcpy(addr, target_saddr, len);
>      addr->sa_family = sa_family;
> +    if (sa_family == AF_PACKET) {
> +           struct target_sockaddr_ll *target_lladdr, *lladdr;
> +           target_lladdr = (void *)target_saddr;

Casting to (struct target_sockaddr_ll *) would be nicer
than the void* cast.

> +           lladdr = (void *)addr;

This looks fishy -- shouldn't lladdr be  a struct sockaddr_ll, not
a struct target_sockaddr? Alternatively, if (as we seem to)
we rely on host and target layouts being the same, you might
as well use struct sockaddr_ll in both cases and add a comment
about our assumption.

> +
> +           lladdr->sll_ifindex = tswap32(target_lladdr->sll_ifindex);
> +           lladdr->sll_hatype = tswap16(target_lladdr->sll_hatype);

In fact we rely on memcpy() above to do most of the
data copying, so you might as well just say
   /* data already copied, just swap the fields which might
    * be different endianness in host and guest
    */
   lladdr->sll_ifindex = tswap32(lladdr->sll_ifindex);
   lladdr->sll_hatype = tswap16(lladdr->sll_hatype);

and drop the target_lladdr entirely.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/4] qemu-user: Impl. setsockopt(SO_BINDTODEVICE)

[Joakim Tjernlund]

Peter Maydell <peter.maydell@linaro.org> wrote on 2014/07/11 17:46:28:

> From: Peter Maydell <peter.maydell@linaro.org>
> To: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>, 
> Cc: QEMU Developers <qemu-devel@nongnu.org>
> Date: 2014/07/11 17:46
> Subject: Re: [Qemu-devel] [PATCH 1/4] qemu-user: Impl. 
setsockopt(SO_BINDTODEVICE)
> 
> On 11 July 2014 16:18, Joakim Tjernlund <Joakim.Tjernlund@transmode.se> 
wrote:
> > Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> > ---
> >  linux-user/syscall.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> > index 5a272d3..1380f4e 100644
> > --- a/linux-user/syscall.c
> > +++ b/linux-user/syscall.c
> > @@ -1497,6 +1497,18 @@ set_timeout:
> >                  unlock_user_struct(tfprog, optval_addr, 1);
> >                  return ret;
> >          }
> > +       case TARGET_SO_BINDTODEVICE:
> > +       {
> > +               char *dev_ifname;
> > +
> > +               if (optlen > IFNAMSIZ)
> > +                       return -TARGET_EINVAL;
> 
> This needs braces for our coding style; you might like
> to run your patches through scripts/checkpatch.pl, which
> will warn about this kind of thing.

Ahh, I figured you had kernel style.

> 
> Also, the kernel implementation handles overlong
> lengths via
>      if (optlen > IFNAMSIZ - 1) {
>          optlen = IFNAMSIZ - 1;
>      }
> 
> which effectively truncates overlong strings rather
> than rejecting them. We should do the same (there will
> be complication required to ensure we pass the kernel
> a NUL-terminated string even if we don't get one from
> the guest; may be easiest to use a local array, given
> IFNAMSIZ is only 16).

hmm, should we not pass through as is to the kernel?
Since we don't copy anything we could just remove this
check and let the kernel decide policy?

> 
> > +
> > +               dev_ifname = lock_user(VERIFY_READ, optval_addr, 
optlen, 1);
> > +               ret = get_errno(setsockopt(sockfd, level, optname, 
dev_ifname, optlen));
> 
> This is passing the target optname through to the
> host kernel; you need to set 'optname = SO_BINDTODEVICE;'
> (look at the other cases in this switch).

Yes, I see. Thanks

Re: [Qemu-devel] [PATCH 1/4] qemu-user: Impl. setsockopt(SO_BINDTODEVICE)

[Peter Maydell]

On 11 July 2014 17:07, Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote:
> Peter Maydell <peter.maydell@linaro.org> wrote on 2014/07/11 17:46:28:
>> This needs braces for our coding style; you might like
>> to run your patches through scripts/checkpatch.pl, which
>> will warn about this kind of thing.
>
> Ahh, I figured you had kernel style.

No, we're a bit different (and we don't always follow
our own style in existing code, though we try to
maintain it on new changes).

>>
>> Also, the kernel implementation handles overlong
>> lengths via
>>      if (optlen > IFNAMSIZ - 1) {
>>          optlen = IFNAMSIZ - 1;
>>      }
>>
>> which effectively truncates overlong strings rather
>> than rejecting them. We should do the same (there will
>> be complication required to ensure we pass the kernel
>> a NUL-terminated string even if we don't get one from
>> the guest; may be easiest to use a local array, given
>> IFNAMSIZ is only 16).
>
> hmm, should we not pass through as is to the kernel?
> Since we don't copy anything we could just remove this
> check and let the kernel decide policy?

I thought about that, but there's a corner case:
the kernel does the clamping of the optlen before the
copy_from_user(), which means if you have:
 [interface name] [unreadable memory]
and optlen is long enough that optval_addr + optlen
reaches into the unreadable memory, then QEMU will return
EFAULT (whereas the native kernel implementation will
succeed) unless we do the clamping of the optlen ourselves.

Which reminds me that your patch forgot the check
    if (!dev_ifname) {
        return -TARGET_EFAULT;
    }
(lock_user can fail).

thanks
-- PMM

Re: [Qemu-devel] [PATCH 3/4] alloca one extra byte sockets

[Peter Maydell]

On 11 July 2014 16:18, Joakim Tjernlund <Joakim.Tjernlund@transmode.se> wrote:
> target_to_host_sockaddr() may increase the lenth with 1 byte
> for AF_UNIX sockets so allocate 1 extra byte.
>
> Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> ---
>  linux-user/syscall.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index a0e1ccc..8853c4e 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -1978,7 +1978,7 @@ static abi_long do_connect(int sockfd, abi_ulong target_addr,
>          return -TARGET_EINVAL;
>      }
>
> -    addr = alloca(addrlen);
> +    addr = alloca(addrlen+1);
>
>      ret = target_to_host_sockaddr(addr, target_addr, addrlen);
>      if (ret)
> @@ -1999,7 +1999,7 @@ static abi_long do_sendrecvmsg_locked(int fd, struct target_msghdr *msgp,
>
>      if (msgp->msg_name) {
>          msg.msg_namelen = tswap32(msgp->msg_namelen);
> -        msg.msg_name = alloca(msg.msg_namelen);
> +        msg.msg_name = alloca(msg.msg_namelen+1);
>          ret = target_to_host_sockaddr(msg.msg_name, tswapal(msgp->msg_name),
>                                  msg.msg_namelen);
>          if (ret) {
> @@ -2262,7 +2262,7 @@ static abi_long do_sendto(int fd, abi_ulong msg, size_t len, int flags,
>      if (!host_msg)
>          return -TARGET_EFAULT;
>      if (target_addr) {
> -        addr = alloca(addrlen);
> +        addr = alloca(addrlen+1);
>          ret = target_to_host_sockaddr(addr, target_addr, addrlen);
>          if (ret) {
>              unlock_user(host_msg, msg, 0);

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

I see we already got this right for do_bind() when the code to fiddle
with the terminator for AF_UNIX sockaddrs went in.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 4/4] ppc: remove excessive logging

[Peter Maydell]

On 11 July 2014 16:18, Joakim Tjernlund <Joakim.Tjernlund@transmode.se> wrote:
> ppc logs every type of Invalid instruction. This generates a lot
> of garbage on console when sshd/ssh_keygen executes as
> they try various insn to optimize its performance.
> The invalid operation log is still there so an unknown insn
> will still be logged.
>
> Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> ---
>  linux-user/main.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/linux-user/main.c b/linux-user/main.c
> index b453a39..71a33c7 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -1698,7 +1698,6 @@ void cpu_loop(CPUPPCState *env)
>                  }
>                  break;
>              case POWERPC_EXCP_INVAL:
> -                EXCP_DUMP(env, "Invalid instruction\n");
>                  info.si_signo = TARGET_SIGILL;
>                  info.si_errno = 0;
>                  switch (env->error_code & 0xF) {

Rather than just deleting this EXCP_DUMP, I would suggest
changing the EXCP_DUMP macro so  it only does anything
if the user has passed the "-d int" debug logging flag:

#define EXCP_DUMP(env, fmt, ...)                                        \
do {                                                                    \
    CPUState *cs = ENV_GET_CPU(env);                                    \
    qemu_log_mask(CPU_LOG_INT, fmt, ## __VA_ARGS__);                    \
    log_cpu_state_mask(CPU_LOG_INT, cs, 0);                             \
} while (0)

(untested code!). Some applications (like QEMU itself!)
use SIGSEGV as well as SIGILL intentionally, for instance.

(cc'd qemu-ppc to see if they have an opinion.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/4] linux-user: impl. sockaddr_ll

[Joakim Tjernlund]

Peter Maydell <peter.maydell@linaro.org> wrote on 2014/07/11 18:00:25:
> On 11 July 2014 16:18, Joakim Tjernlund <Joakim.Tjernlund@transmode.se> 
wrote:
> > Used by AF_PACKET sockets
> >
> > Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> > ---
> >  linux-user/syscall.c      |  8 ++++++++
> >  linux-user/syscall_defs.h | 10 ++++++++++
> >  2 files changed, 18 insertions(+)
> >
> > diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> > index 1380f4e..a0e1ccc 100644
> > --- a/linux-user/syscall.c
> > +++ b/linux-user/syscall.c
> > @@ -1140,6 +1140,14 @@ static inline abi_long 
target_to_host_sockaddr(struct sockaddr *addr,
> >
> >      memcpy(addr, target_saddr, len);
> >      addr->sa_family = sa_family;
> > +    if (sa_family == AF_PACKET) {
> > +           struct target_sockaddr_ll *target_lladdr, *lladdr;
> > +           target_lladdr = (void *)target_saddr;
> 
> Casting to (struct target_sockaddr_ll *) would be nicer
> than the void* cast.
> 
> > +           lladdr = (void *)addr;
> 
> This looks fishy -- shouldn't lladdr be  a struct sockaddr_ll, not
> a struct target_sockaddr? Alternatively, if (as we seem to)
> we rely on host and target layouts being the same, you might
> as well use struct sockaddr_ll in both cases and add a comment
> about our assumption.
> 
> > +
> > +           lladdr->sll_ifindex = tswap32(target_lladdr->sll_ifindex);
> > +           lladdr->sll_hatype = tswap16(target_lladdr->sll_hatype);
> 
> In fact we rely on memcpy() above to do most of the
> data copying, so you might as well just say
>    /* data already copied, just swap the fields which might
>     * be different endianness in host and guest
>     */
>    lladdr->sll_ifindex = tswap32(lladdr->sll_ifindex);
>    lladdr->sll_hatype = tswap16(lladdr->sll_hatype);
> 
> and drop the target_lladdr entirely.

Yes of course! I had messed with the code so much back and forth to
make it work so I completely lost track.

 Jocke

Re: [Qemu-devel] [PATCH 4/4] ppc: remove excessive logging

[Joakim Tjernlund]

Peter Maydell <peter.maydell@linaro.org> wrote on 2014/07/11 19:14:25:

> 
> On 11 July 2014 16:18, Joakim Tjernlund <Joakim.Tjernlund@transmode.se> 
wrote:
> > ppc logs every type of Invalid instruction. This generates a lot
> > of garbage on console when sshd/ssh_keygen executes as
> > they try various insn to optimize its performance.
> > The invalid operation log is still there so an unknown insn
> > will still be logged.
> >
> > Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> > ---
> >  linux-user/main.c | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/linux-user/main.c b/linux-user/main.c
> > index b453a39..71a33c7 100644
> > --- a/linux-user/main.c
> > +++ b/linux-user/main.c
> > @@ -1698,7 +1698,6 @@ void cpu_loop(CPUPPCState *env)
> >                  }
> >                  break;
> >              case POWERPC_EXCP_INVAL:
> > -                EXCP_DUMP(env, "Invalid instruction\n");
> >                  info.si_signo = TARGET_SIGILL;
> >                  info.si_errno = 0;
> >                  switch (env->error_code & 0xF) {
> 
> Rather than just deleting this EXCP_DUMP, I would suggest
> changing the EXCP_DUMP macro so  it only does anything
> if the user has passed the "-d int" debug logging flag:

I don't think ppc wants that. They want unconditionally
debug on to get relevant bug reports. This one is getting in the
way of normal operations so I think it should be deleted.

 Jocke 

Re: [Qemu-devel] [PATCH 4/4] ppc: remove excessive logging

[Peter Maydell]

On 11 July 2014 19:15, Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote:
> Peter Maydell <peter.maydell@linaro.org> wrote on 2014/07/11 19:14:25:
>> On 11 July 2014 16:18, Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> wrote:
>> > ppc logs every type of Invalid instruction. This generates a lot
>> Rather than just deleting this EXCP_DUMP, I would suggest
>> changing the EXCP_DUMP macro so  it only does anything
>> if the user has passed the "-d int" debug logging flag:
>
> I don't think ppc wants that. They want unconditionally
> debug on to get relevant bug reports. This one is getting in the
> way of normal operations so I think it should be deleted.

If the PPC maintainers want that behaviour then they need
to defend it. No other architecture's linux-user code spews
junk to stderr for exceptions, and PPC shouldn't either.
The debug log switches are exactly for allowing us to
say "please turn on debug logging" when bugs are reported,
and those are what we should use.

thanks
-- PMM

Re: [Qemu-devel] [Qemu-ppc] [PATCH 4/4] ppc: remove excessive logging

[Alexander Graf]

On 11.07.14 20:22, Peter Maydell wrote:
> On 11 July 2014 19:15, Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote:
>> Peter Maydell <peter.maydell@linaro.org> wrote on 2014/07/11 19:14:25:
>>> On 11 July 2014 16:18, Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
>> wrote:
>>>> ppc logs every type of Invalid instruction. This generates a lot
>>> Rather than just deleting this EXCP_DUMP, I would suggest
>>> changing the EXCP_DUMP macro so  it only does anything
>>> if the user has passed the "-d int" debug logging flag:
>> I don't think ppc wants that. They want unconditionally
>> debug on to get relevant bug reports. This one is getting in the
>> way of normal operations so I think it should be deleted.
> If the PPC maintainers want that behaviour then they need
> to defend it. No other architecture's linux-user code spews
> junk to stderr for exceptions, and PPC shouldn't either.
> The debug log switches are exactly for allowing us to
> say "please turn on debug logging" when bugs are reported,
> and those are what we should use.

I agree - and it's how we behave in system emulation mode already :).

What do the other platforms do on illegal instructions during user mode? 
Any way we can get consistency across the board?


Alex

Re: [Qemu-devel] [Qemu-ppc] [PATCH 4/4] ppc: remove excessive logging

[Joakim Tjernlund]

Alexander Graf <agraf@suse.de> wrote on 2014/07/12 02:39:21:
> 
> 
> On 11.07.14 20:22, Peter Maydell wrote:
> > On 11 July 2014 19:15, Joakim Tjernlund 
<joakim.tjernlund@transmode.se> wrote:
> >> Peter Maydell <peter.maydell@linaro.org> wrote on 2014/07/11 
19:14:25:
> >>> On 11 July 2014 16:18, Joakim Tjernlund 
<Joakim.Tjernlund@transmode.se>
> >> wrote:
> >>>> ppc logs every type of Invalid instruction. This generates a lot
> >>> Rather than just deleting this EXCP_DUMP, I would suggest
> >>> changing the EXCP_DUMP macro so  it only does anything
> >>> if the user has passed the "-d int" debug logging flag:
> >> I don't think ppc wants that. They want unconditionally
> >> debug on to get relevant bug reports. This one is getting in the
> >> way of normal operations so I think it should be deleted.
> > If the PPC maintainers want that behaviour then they need
> > to defend it. No other architecture's linux-user code spews
> > junk to stderr for exceptions, and PPC shouldn't either.
> > The debug log switches are exactly for allowing us to
> > say "please turn on debug logging" when bugs are reported,
> > and those are what we should use.
> 
> I agree - and it's how we behave in system emulation mode already :).
> 
> What do the other platforms do on illegal instructions during user mode? 

> Any way we can get consistency across the board?

In this case it is not an illegal insn, it is a valid insn but not just
for the QEMU emulated CPU.

Re: [Qemu-devel] [PATCH 1/4] qemu-user: Impl. setsockopt(SO_BINDTODEVICE)

[Joakim Tjernlund]

Peter Maydell <peter.maydell@linaro.org> wrote on 2014/07/11 19:02:30:

> From: Peter Maydell <peter.maydell@linaro.org>
> To: Joakim Tjernlund <joakim.tjernlund@transmode.se>, 
> Cc: QEMU Developers <qemu-devel@nongnu.org>
> Date: 2014/07/11 19:02
> Subject: Re: [Qemu-devel] [PATCH 1/4] qemu-user: Impl. 
setsockopt(SO_BINDTODEVICE)
> 
Snip
> 
> >>
> >> Also, the kernel implementation handles overlong
> >> lengths via
> >>      if (optlen > IFNAMSIZ - 1) {
> >>          optlen = IFNAMSIZ - 1;
> >>      }
> >>
> >> which effectively truncates overlong strings rather
> >> than rejecting them. We should do the same (there will
> >> be complication required to ensure we pass the kernel
> >> a NUL-terminated string even if we don't get one from
> >> the guest; may be easiest to use a local array, given
> >> IFNAMSIZ is only 16).
> >
> > hmm, should we not pass through as is to the kernel?
> > Since we don't copy anything we could just remove this
> > check and let the kernel decide policy?
> 
> I thought about that, but there's a corner case:
> the kernel does the clamping of the optlen before the
> copy_from_user(), which means if you have:
>  [interface name] [unreadable memory]
> and optlen is long enough that optval_addr + optlen
> reaches into the unreadable memory, then QEMU will return
> EFAULT (whereas the native kernel implementation will
> succeed) unless we do the clamping of the optlen ourselves.

I can live with that IMHO very minor flaw that I dont think is
going to matter in practice for simplicity and speed of QEMU.
It is your call though, do we go for exact emulation or can we
cut some corners?

> 
> Which reminds me that your patch forgot the check
>     if (!dev_ifname) {
>         return -TARGET_EFAULT;
>     }
> (lock_user can fail).
> 
> thanks
> -- PMM

Re: [Qemu-devel] [Qemu-ppc] [PATCH 4/4] ppc: remove excessive logging

[Peter Maydell]

On 12 July 2014 01:39, Alexander Graf <agraf@suse.de> wrote:
> What do the other platforms do on illegal instructions during user mode?
> Any way we can get consistency across the board?

Mostly it looks like they just silently generate the SIGILL.
Consistency has never been our strong point :-)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/4] qemu-user: Impl. setsockopt(SO_BINDTODEVICE)

[Peter Maydell]

On 12 July 2014 09:31, Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote:
> Peter Maydell <peter.maydell@linaro.org> wrote on 2014/07/11 19:02:30:
>> I thought about that, but there's a corner case:
>> the kernel does the clamping of the optlen before the
>> copy_from_user(), which means if you have:
>>  [interface name] [unreadable memory]
>> and optlen is long enough that optval_addr + optlen
>> reaches into the unreadable memory, then QEMU will return
>> EFAULT (whereas the native kernel implementation will
>> succeed) unless we do the clamping of the optlen ourselves.
>
> I can live with that IMHO very minor flaw that I dont think is
> going to matter in practice for simplicity and speed of QEMU.
> It is your call though, do we go for exact emulation or can we
> cut some corners?

In this case I would prefer to get it right:
 * it's purely localised to this function
 * it's not all that hard to get right
 * we've already done the hard work of looking at the
   kernel and determining the correct behaviour

SO_BINDTODEVICE is not going to be on any guest's
speed-critical fastpath anyway...

thanks
-- PMM

Re: [Qemu-devel] [Qemu-ppc] [PATCH 4/4] ppc: remove excessive logging

[Alexander Graf]

On 12.07.14 10:58, Peter Maydell wrote:
> On 12 July 2014 01:39, Alexander Graf <agraf@suse.de> wrote:
>> What do the other platforms do on illegal instructions during user mode?
>> Any way we can get consistency across the board?
> Mostly it looks like they just silently generate the SIGILL.
> Consistency has never been our strong point :-)

That means this patch brings things towards consistency? It's certainly 
good for me then :)


Alex

Re: [Qemu-devel] [PATCH 1/4] qemu-user: Impl. setsockopt(SO_BINDTODEVICE)

[Joakim Tjernlund]

Peter Maydell <peter.maydell@linaro.org> wrote on 2014/07/12 11:01:55:
> 
> On 12 July 2014 09:31, Joakim Tjernlund <joakim.tjernlund@transmode.se> 
wrote:
> > Peter Maydell <peter.maydell@linaro.org> wrote on 2014/07/11 19:02:30:
> >> I thought about that, but there's a corner case:
> >> the kernel does the clamping of the optlen before the
> >> copy_from_user(), which means if you have:
> >>  [interface name] [unreadable memory]
> >> and optlen is long enough that optval_addr + optlen
> >> reaches into the unreadable memory, then QEMU will return
> >> EFAULT (whereas the native kernel implementation will
> >> succeed) unless we do the clamping of the optlen ourselves.
> >
> > I can live with that IMHO very minor flaw that I dont think is
> > going to matter in practice for simplicity and speed of QEMU.
> > It is your call though, do we go for exact emulation or can we
> > cut some corners?
> 
> In this case I would prefer to get it right:
>  * it's purely localised to this function
>  * it's not all that hard to get right
>  * we've already done the hard work of looking at the
>    kernel and determining the correct behaviour
> 
> SO_BINDTODEVICE is not going to be on any guest's
> speed-critical fastpath anyway...

OK, 2 new patches sent

I hope these will all make it to the impending release?

Re: [Qemu-devel] [Qemu-ppc] [PATCH 4/4] ppc: remove excessive logging

[Peter Maydell]

On 12 July 2014 10:39, Alexander Graf <agraf@suse.de> wrote:
>
> On 12.07.14 10:58, Peter Maydell wrote:
>>
>> On 12 July 2014 01:39, Alexander Graf <agraf@suse.de> wrote:
>>>
>>> What do the other platforms do on illegal instructions during user mode?
>>> Any way we can get consistency across the board?
>>
>> Mostly it looks like they just silently generate the SIGILL.
>> Consistency has never been our strong point :-)
>
>
> That means this patch brings things towards consistency? It's certainly good
> for me then :)

No, this just removes one use of this logging. If you
wanted consistency we should remove all of them...

-- PMM

Re: [Qemu-devel] [Qemu-ppc] [PATCH 4/4] ppc: remove excessive logging

[Alexander Graf]

On 12.07.14 12:40, Peter Maydell wrote:
> On 12 July 2014 10:39, Alexander Graf <agraf@suse.de> wrote:
>> On 12.07.14 10:58, Peter Maydell wrote:
>>> On 12 July 2014 01:39, Alexander Graf <agraf@suse.de> wrote:
>>>> What do the other platforms do on illegal instructions during user mode?
>>>> Any way we can get consistency across the board?
>>> Mostly it looks like they just silently generate the SIGILL.
>>> Consistency has never been our strong point :-)
>>
>> That means this patch brings things towards consistency? It's certainly good
>> for me then :)
> No, this just removes one use of this logging. If you
> wanted consistency we should remove all of them...

Agreed :)


Alex

Re: [Qemu-devel] [PATCH 1/4] qemu-user: Impl. setsockopt(SO_BINDTODEVICE)

[Peter Maydell]

On 12 July 2014 10:48, Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote:
> Peter Maydell <peter.maydell@linaro.org> wrote on 2014/07/12 11:01:55:
> OK, 2 new patches sent

Thanks.

> I hope these will all make it to the impending release?

I'm not sure at this point; they are bug fixes, and reasonably
small ones, but we're in freeze and only a few weeks from
release, and these aren't regressions, so you could argue
either way. Ultimately that's up to the linux-user submaintainer.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/4] qemu-user: Impl. setsockopt(SO_BINDTODEVICE)

[Joakim Tjernlund]

Peter Maydell <peter.maydell@linaro.org> wrote on 2014/07/12 12:42:48:
> 
> On 12 July 2014 10:48, Joakim Tjernlund <joakim.tjernlund@transmode.se> 
wrote:
> > Peter Maydell <peter.maydell@linaro.org> wrote on 2014/07/12 11:01:55:
> > OK, 2 new patches sent
> 
> Thanks.
> 
> > I hope these will all make it to the impending release?
> 
> I'm not sure at this point; they are bug fixes, and reasonably
> small ones, but we're in freeze and only a few weeks from
> release, and these aren't regressions, so you could argue
> either way. Ultimately that's up to the linux-user submaintainer.

Fingers crossed the :) I would be great to have DHCP functioning
though.

I just sent v3 of the 2 patches addressing your comments.

Could you look at too?
http://lists.nongnu.org/archive/html/qemu-devel/2014-07/msg01925.html

That one really simple and also needed for dhcp 

 Jocke

Re: [Qemu-devel] [Qemu-ppc] [PATCH 4/4] ppc: remove excessive logging

[Joakim Tjernlund]

Alexander Graf <agraf@suse.de> wrote on 2014/07/12 12:41:05:
> 
> On 12.07.14 12:40, Peter Maydell wrote:
> > On 12 July 2014 10:39, Alexander Graf <agraf@suse.de> wrote:
> >> On 12.07.14 10:58, Peter Maydell wrote:
> >>> On 12 July 2014 01:39, Alexander Graf <agraf@suse.de> wrote:
> >>>> What do the other platforms do on illegal instructions during user 
mode?
> >>>> Any way we can get consistency across the board?
> >>> Mostly it looks like they just silently generate the SIGILL.
> >>> Consistency has never been our strong point :-)
> >>
> >> That means this patch brings things towards consistency? It's 
certainly good
> >> for me then :)
> > No, this just removes one use of this logging. If you
> > wanted consistency we should remove all of them...
> 
> Agreed :)

So can I infer from this discussion that you will apply the patch?

Re: [Qemu-devel] [PATCH 1/4] qemu-user: Impl. setsockopt(SO_BINDTODEVICE)

[Joakim Tjernlund]

Peter Maydell <peter.maydell@linaro.org> wrote on 2014/07/11 19:02:30:
> >
> > hmm, should we not pass through as is to the kernel?
> > Since we don't copy anything we could just remove this
> > check and let the kernel decide policy?
> 
> I thought about that, but there's a corner case:
> the kernel does the clamping of the optlen before the
> copy_from_user(), which means if you have:
>  [interface name] [unreadable memory]
> and optlen is long enough that optval_addr + optlen
> reaches into the unreadable memory, then QEMU will return
> EFAULT (whereas the native kernel implementation will
> succeed) unless we do the clamping of the optlen ourselves.

hmm, this kernel code: 
        if (optlen > IFNAMSIZ - 1)
                optlen = IFNAMSIZ - 1;
        memset(devname, 0, sizeof(devname));

        ret = -EFAULT;
        if (copy_from_user(devname, optval, optlen))
                goto out;

suggests to me that the qemu code could be written:

        case TARGET_SO_BINDTODEVICE:
        {
                char *dev_ifname;

                if (optlen > IFNAMSIZ - 1) {
                    optlen = IFNAMSIZ - 1;
                }
                dev_ifname = lock_user(VERIFY_READ, optval_addr, optlen, 
1);
                if (!dev_ifname) {
                    return -TARGET_EFAULT;
                }
                optname = SO_BINDTODEVICE;
                ret = get_errno(setsockopt(sockfd, level, optname, 
dev_ifname, optlen));
                unlock_user (dev_ifname, optval_addr, 0);
                return ret;
        }

Not a big deal, I just wanted to improve my understanding of the 
kernel/qemu code.

 Jocke

Re: [Qemu-devel] [PATCH 1/4] qemu-user: Impl. setsockopt(SO_BINDTODEVICE)

[Peter Maydell]

On 12 July 2014 16:13, Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote:
> Peter Maydell <peter.maydell@linaro.org> wrote on 2014/07/11 19:02:30:
>> >
>> > hmm, should we not pass through as is to the kernel?
>> > Since we don't copy anything we could just remove this
>> > check and let the kernel decide policy?
>>
>> I thought about that, but there's a corner case:
>> the kernel does the clamping of the optlen before the
>> copy_from_user(), which means if you have:
>>  [interface name] [unreadable memory]
>> and optlen is long enough that optval_addr + optlen
>> reaches into the unreadable memory, then QEMU will return
>> EFAULT (whereas the native kernel implementation will
>> succeed) unless we do the clamping of the optlen ourselves.
>
> hmm, this kernel code:
>         if (optlen > IFNAMSIZ - 1)
>                 optlen = IFNAMSIZ - 1;
>         memset(devname, 0, sizeof(devname));
>
>         ret = -EFAULT;
>         if (copy_from_user(devname, optval, optlen))
>                 goto out;
>
> suggests to me that the qemu code could be written:
>
>         case TARGET_SO_BINDTODEVICE:
>         {
>                 char *dev_ifname;
>
>                 if (optlen > IFNAMSIZ - 1) {
>                     optlen = IFNAMSIZ - 1;
>                 }
>                 dev_ifname = lock_user(VERIFY_READ, optval_addr, optlen,
> 1);
>                 if (!dev_ifname) {
>                     return -TARGET_EFAULT;
>                 }
>                 optname = SO_BINDTODEVICE;
>                 ret = get_errno(setsockopt(sockfd, level, optname,
> dev_ifname, optlen));
>                 unlock_user (dev_ifname, optval_addr, 0);
>                 return ret;
>         }
>
> Not a big deal, I just wanted to improve my understanding of the
> kernel/qemu code.

That would work with the current kernel implementation, but
the API definition (as described in the socket(7) manpage)
says we should pass a NUL-terminated string to the kernel,
so it's more robust for us to make sure we do so.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/4] qemu-user: Impl. setsockopt(SO_BINDTODEVICE)

[Joakim Tjernlund]

Peter Maydell <peter.maydell@linaro.org> wrote on 2014/07/12 17:47:56:

> From: Peter Maydell <peter.maydell@linaro.org>
> To: Joakim Tjernlund <joakim.tjernlund@transmode.se>, 
> Cc: QEMU Developers <qemu-devel@nongnu.org>
> Date: 2014/07/12 17:48
> Subject: Re: [Qemu-devel] [PATCH 1/4] qemu-user: Impl. 
setsockopt(SO_BINDTODEVICE)
> 
> On 12 July 2014 16:13, Joakim Tjernlund <joakim.tjernlund@transmode.se> 
wrote:
> > Peter Maydell <peter.maydell@linaro.org> wrote on 2014/07/11 19:02:30:
> >> >
> >> > hmm, should we not pass through as is to the kernel?
> >> > Since we don't copy anything we could just remove this
> >> > check and let the kernel decide policy?
> >>
> >> I thought about that, but there's a corner case:
> >> the kernel does the clamping of the optlen before the
> >> copy_from_user(), which means if you have:
> >>  [interface name] [unreadable memory]
> >> and optlen is long enough that optval_addr + optlen
> >> reaches into the unreadable memory, then QEMU will return
> >> EFAULT (whereas the native kernel implementation will
> >> succeed) unless we do the clamping of the optlen ourselves.
> >
> > hmm, this kernel code:
> >         if (optlen > IFNAMSIZ - 1)
> >                 optlen = IFNAMSIZ - 1;
> >         memset(devname, 0, sizeof(devname));
> >
> >         ret = -EFAULT;
> >         if (copy_from_user(devname, optval, optlen))
> >                 goto out;
> >
> > suggests to me that the qemu code could be written:
> >
> >         case TARGET_SO_BINDTODEVICE:
> >         {
> >                 char *dev_ifname;
> >
> >                 if (optlen > IFNAMSIZ - 1) {
> >                     optlen = IFNAMSIZ - 1;
> >                 }
> >                 dev_ifname = lock_user(VERIFY_READ, optval_addr, 
optlen,
> > 1);
> >                 if (!dev_ifname) {
> >                     return -TARGET_EFAULT;
> >                 }
> >                 optname = SO_BINDTODEVICE;
> >                 ret = get_errno(setsockopt(sockfd, level, optname,
> > dev_ifname, optlen));
> >                 unlock_user (dev_ifname, optval_addr, 0);
> >                 return ret;
> >         }
> >
> > Not a big deal, I just wanted to improve my understanding of the
> > kernel/qemu code.
> 
> That would work with the current kernel implementation, but
> the API definition (as described in the socket(7) manpage)
> says we should pass a NUL-terminated string to the kernel,
> so it's more robust for us to make sure we do so.

But we emulate user space and we should not "improve" stuff on the way 
should we?
If we do then one could end up with progs that work with qemu but not with
the real kernel.

 Jocke

Re: [Qemu-devel] [PATCH 1/4] qemu-user: Impl. setsockopt(SO_BINDTODEVICE)

[Peter Maydell]

On 12 July 2014 18:30, Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote:
> Peter Maydell <peter.maydell@linaro.org> wrote on 2014/07/12 17:47:56:
>> That would work with the current kernel implementation, but
>> the API definition (as described in the socket(7) manpage)
>> says we should pass a NUL-terminated string to the kernel,
>> so it's more robust for us to make sure we do so.
>
> But we emulate user space and we should not "improve" stuff on the way
> should we?

No, but in this case we don't improve anything. The unterminated
string from the guest is still truncated as a native kernel would do;
it's just that our implementation of that behaviour doesn't rely
on the specific behaviour of the host kernel in this undocumented
corner case. It's not a big deal either way, but given that the code
is pretty similar either way I prefer the approach I suggest.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/4] qemu-user: Impl. setsockopt(SO_BINDTODEVICE)

[Joakim Tjernlund]

Peter Maydell <peter.maydell@linaro.org> wrote on 2014/07/12 19:38:26:
> 
> On 12 July 2014 18:30, Joakim Tjernlund <joakim.tjernlund@transmode.se> 
wrote:
> > Peter Maydell <peter.maydell@linaro.org> wrote on 2014/07/12 17:47:56:
> >> That would work with the current kernel implementation, but
> >> the API definition (as described in the socket(7) manpage)
> >> says we should pass a NUL-terminated string to the kernel,
> >> so it's more robust for us to make sure we do so.
> >
> > But we emulate user space and we should not "improve" stuff on the way
> > should we?
> 
> No, but in this case we don't improve anything. The unterminated
> string from the guest is still truncated as a native kernel would do;
> it's just that our implementation of that behaviour doesn't rely
> on the specific behaviour of the host kernel in this undocumented
> corner case. It's not a big deal either way, but given that the code

Right, thank you for your patience. Now I got a little better 
understanding.

 Jocke

Re: [Qemu-devel] [PATCH 3/4] alloca one extra byte sockets

[Riku Voipio]

On Fri, Jul 11, 2014 at 05:18:03PM +0200, Joakim Tjernlund wrote:
> target_to_host_sockaddr() may increase the lenth with 1 byte
> for AF_UNIX sockets so allocate 1 extra byte.

Thanks, applied to linux-user tree

> Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> ---
>  linux-user/syscall.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index a0e1ccc..8853c4e 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -1978,7 +1978,7 @@ static abi_long do_connect(int sockfd, abi_ulong target_addr,
>          return -TARGET_EINVAL;
>      }
>  
> -    addr = alloca(addrlen);
> +    addr = alloca(addrlen+1);
>  
>      ret = target_to_host_sockaddr(addr, target_addr, addrlen);
>      if (ret)
> @@ -1999,7 +1999,7 @@ static abi_long do_sendrecvmsg_locked(int fd, struct target_msghdr *msgp,
>  
>      if (msgp->msg_name) {
>          msg.msg_namelen = tswap32(msgp->msg_namelen);
> -        msg.msg_name = alloca(msg.msg_namelen);
> +        msg.msg_name = alloca(msg.msg_namelen+1);
>          ret = target_to_host_sockaddr(msg.msg_name, tswapal(msgp->msg_name),
>                                  msg.msg_namelen);
>          if (ret) {
> @@ -2262,7 +2262,7 @@ static abi_long do_sendto(int fd, abi_ulong msg, size_t len, int flags,
>      if (!host_msg)
>          return -TARGET_EFAULT;
>      if (target_addr) {
> -        addr = alloca(addrlen);
> +        addr = alloca(addrlen+1);
>          ret = target_to_host_sockaddr(addr, target_addr, addrlen);
>          if (ret) {
>              unlock_user(host_msg, msg, 0);
> -- 
> 1.8.5.5
> 

Re: [Qemu-devel] [Qemu-ppc] [PATCH 4/4] ppc: remove excessive logging

[Riku Voipio]

On Sat, Jul 12, 2014 at 04:06:09PM +0200, Joakim Tjernlund wrote:
> Alexander Graf <agraf@suse.de> wrote on 2014/07/12 12:41:05:
> > 
> > On 12.07.14 12:40, Peter Maydell wrote:
> > > On 12 July 2014 10:39, Alexander Graf <agraf@suse.de> wrote:
> > >> On 12.07.14 10:58, Peter Maydell wrote:
> > >>> On 12 July 2014 01:39, Alexander Graf <agraf@suse.de> wrote:
> > >>>> What do the other platforms do on illegal instructions during user 
> mode?
> > >>>> Any way we can get consistency across the board?
> > >>> Mostly it looks like they just silently generate the SIGILL.
> > >>> Consistency has never been our strong point :-)
> > >>
> > >> That means this patch brings things towards consistency? It's 
> certainly good
> > >> for me then :)
> > > No, this just removes one use of this logging. If you
> > > wanted consistency we should remove all of them...
 
> > Agreed :)
 
> So can I infer from this discussion that you will apply the patch?

I think Peter and Alex suggest that the EXCP_DUMP() loggings should be
removed from all cases in PPC code where TARGET_SIGILL is risen. Your
patch removes just once case, and that would make PPC code become
internally inconsistent where some SIGILLs are logged and others aren't.

Even more dramatically, we remove the whole EXCP_DUMP and users since none
of the other archs output anything for SIGFPE/SIGSEGV either. After all,
the Linux kernel (ppc or others) doesn't output anything either. And
it is the behaviour of linux we try to match.

Riku

Re: [Qemu-devel] [Qemu-ppc] [PATCH 4/4] ppc: remove excessive logging

[Joakim Tjernlund]

Riku Voipio <riku.voipio@iki.fi> wrote on 2014/07/16 09:55:50:

> From: Riku Voipio <riku.voipio@iki.fi>
> To: Joakim Tjernlund <joakim.tjernlund@transmode.se>, 
> Cc: Alexander Graf <agraf@suse.de>, Peter Maydell 
<peter.maydell@linaro.org>, "qemu-ppc@nongnu.org" <qemu-ppc@nongnu.org>, 
QEMU Developers <qemu-devel@nongnu.org>
> Date: 2014/07/16 09:55
> Subject: Re: [Qemu-devel] [Qemu-ppc] [PATCH 4/4] ppc: remove excessive 
logging
> 
> On Sat, Jul 12, 2014 at 04:06:09PM +0200, Joakim Tjernlund wrote:
> > Alexander Graf <agraf@suse.de> wrote on 2014/07/12 12:41:05:
> > > 
> > > On 12.07.14 12:40, Peter Maydell wrote:
> > > > On 12 July 2014 10:39, Alexander Graf <agraf@suse.de> wrote:
> > > >> On 12.07.14 10:58, Peter Maydell wrote:
> > > >>> On 12 July 2014 01:39, Alexander Graf <agraf@suse.de> wrote:
> > > >>>> What do the other platforms do on illegal instructions during 
user 
> > mode?
> > > >>>> Any way we can get consistency across the board?
> > > >>> Mostly it looks like they just silently generate the SIGILL.
> > > >>> Consistency has never been our strong point :-)
> > > >>
> > > >> That means this patch brings things towards consistency? It's 
> > certainly good
> > > >> for me then :)
> > > > No, this just removes one use of this logging. If you
> > > > wanted consistency we should remove all of them...
> 
> > > Agreed :)
> 
> > So can I infer from this discussion that you will apply the patch?
> 
> I think Peter and Alex suggest that the EXCP_DUMP() loggings should be
> removed from all cases in PPC code where TARGET_SIGILL is risen. Your
> patch removes just once case, and that would make PPC code become
> internally inconsistent where some SIGILLs are logged and others aren't.

Something like that. This is one step in that direction. We(or I cannot) 
do
the consistency for all cases/arches at once. With the patch we become
one step closer to the Linux kernel so I don't see why not apply it.

> 
> Even more dramatically, we remove the whole EXCP_DUMP and users since 
none
> of the other archs output anything for SIGFPE/SIGSEGV either. After all,
> the Linux kernel (ppc or others) doesn't output anything either. And
> it is the behaviour of linux we try to match.

hmm, not clear to me what you mean here

 Jocke

Re: [Qemu-devel] [Qemu-ppc] [PATCH 4/4] ppc: remove excessive logging

[Alexander Graf]

> Am 16.07.2014 um 10:32 schrieb Joakim Tjernlund <joakim.tjernlund@transmode.se>:
> 
> Riku Voipio <riku.voipio@iki.fi> wrote on 2014/07/16 09:55:50:
> 
>> From: Riku Voipio <riku.voipio@iki.fi>
>> To: Joakim Tjernlund <joakim.tjernlund@transmode.se>,
>> Cc: Alexander Graf <agraf@suse.de>, Peter Maydell
> <peter.maydell@linaro.org>, "qemu-ppc@nongnu.org" <qemu-ppc@nongnu.org>, 
> QEMU Developers <qemu-devel@nongnu.org>
>> Date: 2014/07/16 09:55
>> Subject: Re: [Qemu-devel] [Qemu-ppc] [PATCH 4/4] ppc: remove excessive
> logging
>> 
>>> On Sat, Jul 12, 2014 at 04:06:09PM +0200, Joakim Tjernlund wrote:
>>> Alexander Graf <agraf@suse.de> wrote on 2014/07/12 12:41:05:
>>>> 
>>>>> On 12.07.14 12:40, Peter Maydell wrote:
>>>>>> On 12 July 2014 10:39, Alexander Graf <agraf@suse.de> wrote:
>>>>>>> On 12.07.14 10:58, Peter Maydell wrote:
>>>>>>>> On 12 July 2014 01:39, Alexander Graf <agraf@suse.de> wrote:
>>>>>>>> What do the other platforms do on illegal instructions during
> user 
>>> mode?
>>>>>>>> Any way we can get consistency across the board?
>>>>>>> Mostly it looks like they just silently generate the SIGILL.
>>>>>>> Consistency has never been our strong point :-)
>>>>>> 
>>>>>> That means this patch brings things towards consistency? It's
>>> certainly good
>>>>>> for me then :)
>>>>> No, this just removes one use of this logging. If you
>>>>> wanted consistency we should remove all of them...
>> 
>>>> Agreed :)
>> 
>>> So can I infer from this discussion that you will apply the patch?
>> 
>> I think Peter and Alex suggest that the EXCP_DUMP() loggings should be
>> removed from all cases in PPC code where TARGET_SIGILL is risen. Your
>> patch removes just once case, and that would make PPC code become
>> internally inconsistent where some SIGILLs are logged and others aren't.
> 
> Something like that. This is one step in that direction. We(or I cannot) 
> do
> the consistency for all cases/arches at once. With the patch we become
> one step closer to the Linux kernel so I don't see why not apply it.

That's not how it will end up though. If we apply this one patch now, it will stay that way forever and be even more confusing and inconsistent than today.

I think what we really want is proper -d int logging on all archs for linux-user. This patch is getting us nowhere close to it.


Alex

Re: [Qemu-devel] [Qemu-ppc] [PATCH 4/4] ppc: remove excessive logging

[Alex Bennée]

Joakim Tjernlund writes:

> Alexander Graf <agraf@suse.de> wrote on 2014/07/12 02:39:21:
>> 
>> 
>> On 11.07.14 20:22, Peter Maydell wrote:
>> > On 11 July 2014 19:15, Joakim Tjernlund 
> <joakim.tjernlund@transmode.se> wrote:
>> >> Peter Maydell <peter.maydell@linaro.org> wrote on 2014/07/11 
> 19:14:25:
>> >>> On 11 July 2014 16:18, Joakim Tjernlund 
> <Joakim.Tjernlund@transmode.se>
>> >> wrote:
>> >>>> ppc logs every type of Invalid instruction. This generates a lot
>> >>> Rather than just deleting this EXCP_DUMP, I would suggest
>> >>> changing the EXCP_DUMP macro so  it only does anything
>> >>> if the user has passed the "-d int" debug logging flag:
>> >> I don't think ppc wants that. They want unconditionally
>> >> debug on to get relevant bug reports. This one is getting in the
>> >> way of normal operations so I think it should be deleted.
>> > If the PPC maintainers want that behaviour then they need
>> > to defend it. No other architecture's linux-user code spews
>> > junk to stderr for exceptions, and PPC shouldn't either.
>> > The debug log switches are exactly for allowing us to
>> > say "please turn on debug logging" when bugs are reported,
>> > and those are what we should use.
>> 
>> I agree - and it's how we behave in system emulation mode already :).
>> 
>> What do the other platforms do on illegal instructions during user mode? 
>
>> Any way we can get consistency across the board?
>
> In this case it is not an illegal insn, it is a valid insn but not just
> for the QEMU emulated CPU.

On aarch64 we do:

#define unsupported_encoding(s, insn)                                    \
    do {                                                                 \
        qemu_log_mask(LOG_UNIMP,                                         \
                      "%s:%d: unsupported instruction encoding 0x%08x "  \
                      "at pc=%016" PRIx64 "\n",                          \
                      __FILE__, __LINE__, insn, s->pc - 4);              \
        unallocated_encoding(s);                                         \
    } while (0);

So we signal it's unimplemented before falling through to the signal
generating code.

-- 
Alex Bennée

Re: [Qemu-devel] [Qemu-ppc] [PATCH 4/4] ppc: remove excessive logging

[Peter Maydell]

On 16 July 2014 10:17, Alex Bennée <alex.bennee@linaro.org> wrote:
> On aarch64 we do:
>
> #define unsupported_encoding(s, insn)                                    \
>     do {                                                                 \
>         qemu_log_mask(LOG_UNIMP,                                         \
>                       "%s:%d: unsupported instruction encoding 0x%08x "  \
>                       "at pc=%016" PRIx64 "\n",                          \
>                       __FILE__, __LINE__, insn, s->pc - 4);              \
>         unallocated_encoding(s);                                         \
>     } while (0);
>
> So we signal it's unimplemented before falling through to the signal
> generating code.

Yes, but that macro is only for instructions which exist but which
QEMU doesn't implement, not for instructions which exist and
which we do have an implementation of but which we generate an
exception for because they aren't present on this particular CPU.
That's a separate issue from whether you might want to optionally
log every SIGILL a process generates.

thanks
-- PMM