[PATCH 0/6] bsd-user upstreaming: read, write and exit

[PATCH 0/6] bsd-user upstreaming: read, write and exit

[Warner Losh]

This series of patches continues the effort to get system calls working
upstream. This series was cleaved off a prior series to give me time to rework
based on the feedback from the first time I posted these.

These still need review based on comments from  Richard Henderson

       o bsd-user/freebsd/os-syscall.c: lock_iovec
I rewored to use g_try_new, as well as fixing bugs in the 'after a FAULT
handling code' Created a common routine to cleanup after errors that can
be used for the unlock_iovec.

       o bsd-user/freebsd/os-syscall.c: unlock_iovec
Fixed the error handling to be consistent with a normal unlock_iovec.

       o bsd-user/freebsd/os-syscall.c: Tracing and error boilerplate
Created the wrapper function as suggested in prior reviews.

The others were reviewed before, and haven't changed in any real way, but more
eyes likely won't hurt.

Warner Losh (6):
  bsd-user/freebsd/os-syscall.c: lock_iovec
  bsd-user/freebsd/os-syscall.c: unlock_iovec
  bsd-user/freebsd/os-syscall.c: Tracing and error boilerplate
  bsd-user/bsd-file.h: Add implementations for read, pread, readv and
    preadv
  bsd-user/bsd-file.h: Meat of the write system calls
  bsd-user/freebsd/os-syscall.c: Implement exit

 bsd-user/bsd-file.h           | 164 ++++++++++++++++++++++++
 bsd-user/bsd-proc.h           |  43 +++++++
 bsd-user/freebsd/os-syscall.c | 231 +++++++++++++++++++++++++++++++++-
 3 files changed, 434 insertions(+), 4 deletions(-)
 create mode 100644 bsd-user/bsd-proc.h

-- 
2.33.1

[PATCH 1/6] bsd-user/freebsd/os-syscall.c: lock_iovec

[Warner Losh]

lock_iovec will lock an I/O vec and the memory to which it refers and
create a iovec in the host space that refers to it, with full error
unwinding. Add helper_iovec_unlock to unlock the partially locked iovec
in case there's an error. The code will be used in iovec_unlock when
that is committed.

Note: memory handling likely could be rewritten to use q_autofree. That
will be explored in the future since what we have now works well enough.

Signed-off-by: Warner Losh <imp@bsdimp.com>
---
 bsd-user/freebsd/os-syscall.c | 113 ++++++++++++++++++++++++++++++++++
 1 file changed, 113 insertions(+)

diff --git a/bsd-user/freebsd/os-syscall.c b/bsd-user/freebsd/os-syscall.c
index d272478e7b0..c41ef0eda40 100644
--- a/bsd-user/freebsd/os-syscall.c
+++ b/bsd-user/freebsd/os-syscall.c
@@ -73,6 +73,119 @@ bool is_error(abi_long ret)
     return (abi_ulong)ret >= (abi_ulong)(-4096);
 }
 
+/*
+ * Unlocks a iovec. Unlike unlock_iovec, it assumes the tvec array itself is
+ * already locked from target_addr. It will be unlocked as well as all the iovec
+ * elements.
+ */
+static void helper_unlock_iovec(struct target_iovec *target_vec,
+                                abi_ulong target_addr, struct iovec *vec,
+                                int count, int copy)
+{
+    for (int i = 0; i < count; i++) {
+        abi_ulong base = tswapal(target_vec[i].iov_base);
+        abi_long len = tswapal(target_vec[i].iov_len);
+        if (len < 0) {
+            /*
+             * Can't really happen: we'll fail to lock if any elements have a
+             * length < 0. Better to fail-safe though.
+             */
+            break;
+        }
+        if (vec[i].iov_base) {
+            unlock_user(vec[i].iov_base, base, copy ? vec[i].iov_len : 0);
+        }
+    }
+    unlock_user(target_vec, target_addr, 0);
+}
+
+struct iovec *lock_iovec(int type, abi_ulong target_addr,
+        int count, int copy)
+{
+    struct target_iovec *target_vec;
+    struct iovec *vec;
+    abi_ulong total_len, max_len;
+    int i;
+    int err = 0;
+    bool bad_address = false;
+
+    if (count == 0) {
+        errno = 0;
+        return NULL;
+    }
+    if (count < 0 || count > IOV_MAX) {
+        errno = EINVAL;
+        return NULL;
+    }
+
+    vec = g_try_new(struct iovec, count);
+    if (vec == NULL) {
+        errno = ENOMEM;
+        return NULL;
+    }
+
+    target_vec = lock_user(VERIFY_READ, target_addr,
+                           count * sizeof(struct target_iovec), 1);
+    if (target_vec == NULL) {
+        err = EFAULT;
+        goto fail2;
+    }
+
+    /*
+     * ??? If host page size > target page size, this will result in a value
+     * larger than what we can actually support.
+     * ??? Should we just assert something for new 16k page size on aarch64?
+     */
+    max_len = 0x7fffffff & TARGET_PAGE_MASK;
+    total_len = 0;
+
+    for (i = 0; i < count; i++) {
+        abi_ulong base = tswapal(target_vec[i].iov_base);
+        abi_long len = tswapal(target_vec[i].iov_len);
+
+        if (len < 0) {
+            err = EINVAL;
+            goto fail;
+        } else if (len == 0 || bad_address) {
+            /* Zero length pointer is ignored. */
+            len = 0;
+            vec[i].iov_base = 0;
+        } else {
+            vec[i].iov_base = lock_user(type, base, len, copy);
+            /*
+             * If the first buffer pointer is bad, this is a fault.  But
+             * subsequent bad buffers will result in a partial write; this is
+             * realized by filling the vector with null pointers and zero
+             * lengths.
+             */
+            if (!vec[i].iov_base) {
+                if (i == 0) {
+                    err = EFAULT;
+                    goto fail;
+                } else {
+                    bad_address = true;
+                    len = 0;
+                }
+            }
+            if (len > max_len - total_len) {
+                len = max_len - total_len;
+            }
+        }
+        vec[i].iov_len = len;
+        total_len += len;
+    }
+
+    unlock_user(target_vec, target_addr, 0);
+    return vec;
+
+fail:
+    helper_unlock_iovec(target_vec, target_addr, vec, i, copy);
+fail2:
+    g_free(vec);
+    errno = err;
+    return NULL;
+}
+
 /*
  * do_syscall() should always have a single exit point at the end so that
  * actions, such as logging of syscall results, can be performed.  All errnos
-- 
2.33.1

Re: [PATCH 1/6] bsd-user/freebsd/os-syscall.c: lock_iovec

[Richard Henderson]

On 6/7/22 13:14, Warner Losh wrote:
> +static void helper_unlock_iovec(struct target_iovec *target_vec,
> +                                abi_ulong target_addr, struct iovec *vec,
> +                                int count, int copy)
> +{
> +    for (int i = 0; i < count; i++) {
> +        abi_ulong base = tswapal(target_vec[i].iov_base);
> +        abi_long len = tswapal(target_vec[i].iov_len);
> +        if (len < 0) {
> +            /*
> +             * Can't really happen: we'll fail to lock if any elements have a
> +             * length < 0. Better to fail-safe though.
> +             */
> +            break;
> +        }

I think this is over-complicated, could be fixed by...

> +    vec = g_try_new(struct iovec, count);

... using g_try_new0.

> +    /*
> +     * ??? If host page size > target page size, this will result in a value
> +     * larger than what we can actually support.
> +     * ??? Should we just assert something for new 16k page size on aarch64?
> +     */
> +    max_len = 0x7fffffff & TARGET_PAGE_MASK;

Use minimum value, I think.


r~

Re: [PATCH 1/6] bsd-user/freebsd/os-syscall.c: lock_iovec

[Warner Losh]

[-- Attachment #1: Type: text/plain, Size: 1726 bytes --]

On Tue, Jun 7, 2022 at 2:01 PM Richard Henderson <
richard.henderson@linaro.org> wrote:

> On 6/7/22 13:14, Warner Losh wrote:
> > +static void helper_unlock_iovec(struct target_iovec *target_vec,
> > +                                abi_ulong target_addr, struct iovec
> *vec,
> > +                                int count, int copy)
> > +{
> > +    for (int i = 0; i < count; i++) {
> > +        abi_ulong base = tswapal(target_vec[i].iov_base);
> > +        abi_long len = tswapal(target_vec[i].iov_len);
> > +        if (len < 0) {
> > +            /*
> > +             * Can't really happen: we'll fail to lock if any elements
> have a
> > +             * length < 0. Better to fail-safe though.
> > +             */
> > +            break;
> > +        }
>
> I think this is over-complicated, could be fixed by...
>
> > +    vec = g_try_new(struct iovec, count);
>
> ... using g_try_new0.
>

I agree. Once I fixed the 'bad_address' issue, I can make this code simpler.
linux-user/syscall.c likely can be simplified as well since this code is a
fairly
straight forward copy of that code way back when, it seems...


> > +    /*
> > +     * ??? If host page size > target page size, this will result in a
> value
> > +     * larger than what we can actually support.
> > +     * ??? Should we just assert something for new 16k page size on
> aarch64?
> > +     */
> > +    max_len = 0x7fffffff & TARGET_PAGE_MASK;
>
> Use minimum value, I think.
>

OK. Will update this, and the other suggestions and use the fact that once
we
hit an error, we basically don't do anything to the zero'd frame, which
makes it
even simpler (and still, I think) correct. I'll send V2 kinda quickly as a
result after
my smoke test...

Warner

[-- Attachment #2: Type: text/html, Size: 2592 bytes --]

[PATCH 2/6] bsd-user/freebsd/os-syscall.c: unlock_iovec

[Warner Losh]

Releases the references to the iovec created by lock_iovec.

Signed-off-by: Warner Losh <imp@bsdimp.com>
---
 bsd-user/freebsd/os-syscall.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/bsd-user/freebsd/os-syscall.c b/bsd-user/freebsd/os-syscall.c
index c41ef0eda40..510307f29d9 100644
--- a/bsd-user/freebsd/os-syscall.c
+++ b/bsd-user/freebsd/os-syscall.c
@@ -186,6 +186,20 @@ fail2:
     return NULL;
 }
 
+void unlock_iovec(struct iovec *vec, abi_ulong target_addr,
+        int count, int copy)
+{
+    struct target_iovec *target_vec;
+
+    target_vec = lock_user(VERIFY_READ, target_addr,
+                           count * sizeof(struct target_iovec), 1);
+    if (target_vec) {
+        helper_unlock_iovec(target_vec, target_addr, vec, count, copy);
+    }
+
+    g_free(vec);
+}
+
 /*
  * do_syscall() should always have a single exit point at the end so that
  * actions, such as logging of syscall results, can be performed.  All errnos
-- 
2.33.1

Re: [PATCH 2/6] bsd-user/freebsd/os-syscall.c: unlock_iovec

[Richard Henderson]

On 6/7/22 13:14, Warner Losh wrote:
> +void unlock_iovec(struct iovec *vec, abi_ulong target_addr,
> +        int count, int copy)
> +{
> +    struct target_iovec *target_vec;
> +
> +    target_vec = lock_user(VERIFY_READ, target_addr,
> +                           count * sizeof(struct target_iovec), 1);
> +    if (target_vec) {

Locking the same region twice seems like a bad idea.

How about something like

typedef struct {
     struct target_iovec *target;
     abi_ulong target_addr;
     int count;
     struct iovec host[];
} IOVecMap;

IOVecMap *lock_iovec(abi_ulong target_addr, int count, bool copy_in)
{
     IOVecMap *map;

     if (count == 0) ...
     if (count < 0) ...

     map = g_try_malloc0(sizeof(IOVecNew) + sizeof(struct iovec) * count);
     if (!map) ...

     map->target = lock_user(...);
     if (!map->target) {
         g_free(map);
         errno = EFAULT;
         return NULL;
     }
     map->target_addr = target_addr;
     map->count = count;

     // lock loop

  fail:
     unlock_iovec(vec, false);
     errno = err;
     return NULL;
}

void unlock_iovec(IOVecMap *map, bool copy_out)
{
     for (int i = 0, count = map->count; i < count; ++i) {
         if (map->host[i].iov_base) {
             abi_ulong target_base = tswapal(map->target[i].iov_base);
             unlock_user(map->host[i].iov_base, target_base,
                         copy_out ? map->host[i].iov_len : 0);
         }
     }
     unlock_user(map->target, map->target_addr, 0);
     g_free(map);
}


r~

Re: [PATCH 2/6] bsd-user/freebsd/os-syscall.c: unlock_iovec

[Warner Losh]

[-- Attachment #1: Type: text/plain, Size: 2047 bytes --]

On Tue, Jun 7, 2022 at 2:28 PM Richard Henderson <
richard.henderson@linaro.org> wrote:

> On 6/7/22 13:14, Warner Losh wrote:
> > +void unlock_iovec(struct iovec *vec, abi_ulong target_addr,
> > +        int count, int copy)
> > +{
> > +    struct target_iovec *target_vec;
> > +
> > +    target_vec = lock_user(VERIFY_READ, target_addr,
> > +                           count * sizeof(struct target_iovec), 1);
> > +    if (target_vec) {
>
> Locking the same region twice seems like a bad idea.
>

We unlock the iovec memory in the lock_iovec()


> How about something like
>
> typedef struct {
>      struct target_iovec *target;
>      abi_ulong target_addr;
>      int count;
>      struct iovec host[];
> } IOVecMap;
>
> IOVecMap *lock_iovec(abi_ulong target_addr, int count, bool copy_in)
> {
>      IOVecMap *map;
>
>      if (count == 0) ...
>      if (count < 0) ...
>
>      map = g_try_malloc0(sizeof(IOVecNew) + sizeof(struct iovec) * count);
>      if (!map) ...
>
>      map->target = lock_user(...);
>      if (!map->target) {
>          g_free(map);
>          errno = EFAULT;
>          return NULL;
>      }
>      map->target_addr = target_addr;
>      map->count = count;
>
>      // lock loop
>
>   fail:
>      unlock_iovec(vec, false);
>      errno = err;
>      return NULL;
> }
>
> void unlock_iovec(IOVecMap *map, bool copy_out)
> {
>      for (int i = 0, count = map->count; i < count; ++i) {
>          if (map->host[i].iov_base) {
>              abi_ulong target_base = tswapal(map->target[i].iov_base);
>              unlock_user(map->host[i].iov_base, target_base,
>                          copy_out ? map->host[i].iov_len : 0);
>          }
>

And wouldn't we want to filter out the iov_base that == 0 since
we may terminate the loop before we get to the count. When the
I/O is done, we'll call it not with the number we mapped, but with
the original number...  Or am I not understanding something here...

Warner


>      }
>      unlock_user(map->target, map->target_addr, 0);
>      g_free(map);
> }
>
>
> r~
>

[-- Attachment #2: Type: text/html, Size: 3159 bytes --]

Re: [PATCH 2/6] bsd-user/freebsd/os-syscall.c: unlock_iovec

[Richard Henderson]

On 6/7/22 14:51, Warner Losh wrote:
>     void unlock_iovec(IOVecMap *map, bool copy_out)
>     {
>           for (int i = 0, count = map->count; i < count; ++i) {
>               if (map->host[i].iov_base) {
>                   abi_ulong target_base = tswapal(map->target[i].iov_base);
>                   unlock_user(map->host[i].iov_base, target_base,
>                               copy_out ? map->host[i].iov_len : 0);
>               }
> 
> 
> And wouldn't we want to filter out the iov_base that == 0 since
> we may terminate the loop before we get to the count. When the
> I/O is done, we'll call it not with the number we mapped, but with
> the original number...  Or am I not understanding something here...

I'm not following -- when and why are you adjusting count?


r~

Re: [PATCH 2/6] bsd-user/freebsd/os-syscall.c: unlock_iovec

[Warner Losh]

[-- Attachment #1: Type: text/plain, Size: 1397 bytes --]



> On Jun 7, 2022, at 3:23 PM, Richard Henderson <richard.henderson@linaro.org> wrote:
> 
> On 6/7/22 14:51, Warner Losh wrote:
>>    void unlock_iovec(IOVecMap *map, bool copy_out)
>>    {
>>          for (int i = 0, count = map->count; i < count; ++i) {
>>              if (map->host[i].iov_base) {
>>                  abi_ulong target_base = tswapal(map->target[i].iov_base);
>>                  unlock_user(map->host[i].iov_base, target_base,
>>                              copy_out ? map->host[i].iov_len : 0);
>>              }
>> And wouldn't we want to filter out the iov_base that == 0 since
>> we may terminate the loop before we get to the count. When the
>> I/O is done, we'll call it not with the number we mapped, but with
>> the original number...  Or am I not understanding something here...
> 
> I'm not following -- when and why are you adjusting count?

When we hit a memory range we can’t map after the first one,
we effectively stop mapping in (in the current linux code we
do map after, but then destroy the length). So that means
we’ll have entries in the iovec that are zero, and this code
doesn’t account for that. We’re not changing the count, per
se, but have a scenario where they might wind up NULL.

I’ll add “if I understand all this right” because I a little shaky
still on these aspects of qemu’s soft mmu.

Warner

[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 874 bytes --]

Re: [PATCH 2/6] bsd-user/freebsd/os-syscall.c: unlock_iovec

[Richard Henderson]

On 6/7/22 16:35, Warner Losh wrote:
> 
> 
>> On Jun 7, 2022, at 3:23 PM, Richard Henderson <richard.henderson@linaro.org> wrote:
>>
>> On 6/7/22 14:51, Warner Losh wrote:
>>>     void unlock_iovec(IOVecMap *map, bool copy_out)
>>>     {
>>>           for (int i = 0, count = map->count; i < count; ++i) {
>>>               if (map->host[i].iov_base) {
>>>                   abi_ulong target_base = tswapal(map->target[i].iov_base);
>>>                   unlock_user(map->host[i].iov_base, target_base,
>>>                               copy_out ? map->host[i].iov_len : 0);
>>>               }
>>> And wouldn't we want to filter out the iov_base that == 0 since
>>> we may terminate the loop before we get to the count. When the
>>> I/O is done, we'll call it not with the number we mapped, but with
>>> the original number...  Or am I not understanding something here...
>>
>> I'm not following -- when and why are you adjusting count?
> 
> When we hit a memory range we can’t map after the first one,
> we effectively stop mapping in (in the current linux code we
> do map after, but then destroy the length). So that means
> we’ll have entries in the iovec that are zero, and this code
> doesn’t account for that. We’re not changing the count, per
> se, but have a scenario where they might wind up NULL.

... and so skip them with the if.

I mean, I suppose you could set map->count on error, as you say, so that we don't iterate 
so far, but... duh, error case.  So long as you don't actively fail, there's no point in 
optimizing for it.


r~

Re: [PATCH 2/6] bsd-user/freebsd/os-syscall.c: unlock_iovec

[Warner Losh]

[-- Attachment #1: Type: text/plain, Size: 2363 bytes --]

On Tue, Jun 7, 2022 at 7:02 PM Richard Henderson <
richard.henderson@linaro.org> wrote:

> On 6/7/22 16:35, Warner Losh wrote:
> >
> >
> >> On Jun 7, 2022, at 3:23 PM, Richard Henderson <
> richard.henderson@linaro.org> wrote:
> >>
> >> On 6/7/22 14:51, Warner Losh wrote:
> >>>     void unlock_iovec(IOVecMap *map, bool copy_out)
> >>>     {
> >>>           for (int i = 0, count = map->count; i < count; ++i) {
> >>>               if (map->host[i].iov_base) {
> >>>                   abi_ulong target_base =
> tswapal(map->target[i].iov_base);
> >>>                   unlock_user(map->host[i].iov_base, target_base,
> >>>                               copy_out ? map->host[i].iov_len : 0);
> >>>               }
> >>> And wouldn't we want to filter out the iov_base that == 0 since
> >>> we may terminate the loop before we get to the count. When the
> >>> I/O is done, we'll call it not with the number we mapped, but with
> >>> the original number...  Or am I not understanding something here...
> >>
> >> I'm not following -- when and why are you adjusting count?
> >
> > When we hit a memory range we can’t map after the first one,
> > we effectively stop mapping in (in the current linux code we
> > do map after, but then destroy the length). So that means
> > we’ll have entries in the iovec that are zero, and this code
> > doesn’t account for that. We’re not changing the count, per
> > se, but have a scenario where they might wind up NULL.
>
> ... and so skip them with the if.
>
> I mean, I suppose you could set map->count on error, as you say, so that
> we don't iterate
> so far, but... duh, error case.  So long as you don't actively fail,
> there's no point in
> optimizing for it.
>

Setting the count would be hard because we'd have to allocate and free
state that we're not currently doing. Better to just skip it with an if. We
allocate
a vector that's used in a number of places, and we'd have to change that
code if we did things differently. While I'm open to suggestions here, I
think
that just accounting for the possible error with an if is our best bet for
now.
I have a lot of code to get in, and am hoping to not rewrite things unless
there's
some clear benefit over the existing structure (like fixing bugs, matching
linux-user,
or increasing performance).

Warner

[-- Attachment #2: Type: text/html, Size: 3205 bytes --]

[PATCH 3/6] bsd-user/freebsd/os-syscall.c: Tracing and error boilerplate

[Warner Losh]

Add in the tracing and this system call not implemented boilerplate. Do
this by moving the guts of do_freebsd_syscall to freebsd_syscall. Put
the tracing in the wrapper function. Since freebsd_syscall is a
singleton static function, it will almost certainly be inlined. Fix
comments that referred to do_syscall since that was renamed some tie
ago.

Signed-off-by: Warner Losh <imp@bsdimp.com>
---
 bsd-user/freebsd/os-syscall.c | 50 ++++++++++++++++++++++++++++++++---
 1 file changed, 46 insertions(+), 4 deletions(-)

diff --git a/bsd-user/freebsd/os-syscall.c b/bsd-user/freebsd/os-syscall.c
index 510307f29d9..334c573739b 100644
--- a/bsd-user/freebsd/os-syscall.c
+++ b/bsd-user/freebsd/os-syscall.c
@@ -201,16 +201,58 @@ void unlock_iovec(struct iovec *vec, abi_ulong target_addr,
 }
 
 /*
- * do_syscall() should always have a single exit point at the end so that
- * actions, such as logging of syscall results, can be performed.  All errnos
- * that do_syscall() returns must be -TARGET_<errcode>.
+ * All errnos that freebsd_syscall() returns must be -TARGET_<errcode>.
+ */
+static abi_long freebsd_syscall(void *cpu_env, int num, abi_long arg1,
+                                abi_long arg2, abi_long arg3, abi_long arg4,
+                                abi_long arg5, abi_long arg6, abi_long arg7,
+                                abi_long arg8)
+{
+    abi_long ret;
+
+    switch (num) {
+    default:
+        gemu_log("qemu: unsupported syscall: %d\n", num);
+        ret = -TARGET_ENOSYS;
+        break;
+    }
+
+    return ret;
+}
+
+/*
+ * do_freebsd_syscall() should always have a single exit point at the end so
+ * that actions, such as logging of syscall results, can be performed. This
+ * as a wrapper around freebsd_syscall() so that actually happens. Since
+ * that is a singleton, modern compilers will inline it anyway...
  */
 abi_long do_freebsd_syscall(void *cpu_env, int num, abi_long arg1,
                             abi_long arg2, abi_long arg3, abi_long arg4,
                             abi_long arg5, abi_long arg6, abi_long arg7,
                             abi_long arg8)
 {
-    return 0;
+    CPUState *cpu = env_cpu(cpu_env);
+    int ret;
+
+#ifdef DEBUG
+    gemu_log("freebsd syscall %d\n", num);
+#endif
+    trace_guest_user_syscall(cpu, num, arg1, arg2, arg3, arg4, arg5, arg6, arg7, arg8);
+    if (do_strace) {
+        print_freebsd_syscall(num, arg1, arg2, arg3, arg4, arg5, arg6);
+    }
+
+    ret = freebsd_syscall(cpu_env, num, arg1, arg2, arg3, arg4, arg5, arg6,
+                          arg7, arg8);
+#ifdef DEBUG
+    gemu_log(" = %ld\n", ret);
+#endif
+    if (do_strace) {
+        print_freebsd_syscall_ret(num, ret);
+    }
+    trace_guest_user_syscall_ret(cpu, num, ret);
+
+    return ret;
 }
 
 void syscall_init(void)
-- 
2.33.1

Re: [PATCH 3/6] bsd-user/freebsd/os-syscall.c: Tracing and error boilerplate

[Richard Henderson]

On 6/7/22 13:14, Warner Losh wrote:
> +static abi_long freebsd_syscall(void *cpu_env, int num, abi_long arg1,
> +                                abi_long arg2, abi_long arg3, abi_long arg4,
> +                                abi_long arg5, abi_long arg6, abi_long arg7,
> +                                abi_long arg8)
> +{
> +    abi_long ret;
> +
> +    switch (num) {
> +    default:
> +        gemu_log("qemu: unsupported syscall: %d\n", num);

qemu_log_mask(LOG_UNIMP, "Unsupported syscall: %d\n", num);


> +#ifdef DEBUG
> +    gemu_log("freebsd syscall %d\n", num);
> +#endif

Drop this.  It's redundant with strace.

Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH 3/6] bsd-user/freebsd/os-syscall.c: Tracing and error boilerplate

[Warner Losh]

[-- Attachment #1: Type: text/plain, Size: 949 bytes --]

On Tue, Jun 7, 2022 at 2:34 PM Richard Henderson <
richard.henderson@linaro.org> wrote:

> On 6/7/22 13:14, Warner Losh wrote:
> > +static abi_long freebsd_syscall(void *cpu_env, int num, abi_long arg1,
> > +                                abi_long arg2, abi_long arg3, abi_long
> arg4,
> > +                                abi_long arg5, abi_long arg6, abi_long
> arg7,
> > +                                abi_long arg8)
> > +{
> > +    abi_long ret;
> > +
> > +    switch (num) {
> > +    default:
> > +        gemu_log("qemu: unsupported syscall: %d\n", num);
>
> qemu_log_mask(LOG_UNIMP, "Unsupported syscall: %d\n", num);
>

Agreed.


>
> > +#ifdef DEBUG
> > +    gemu_log("freebsd syscall %d\n", num);
> > +#endif
>
> Drop this.  It's redundant with strace.
>

Yea, it was a quick hack in the past that the wrapper function highlighted
nicely..


> Otherwise,
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


Thanks!

Warner

[-- Attachment #2: Type: text/html, Size: 1910 bytes --]

[PATCH 4/6] bsd-user/bsd-file.h: Add implementations for read, pread, readv and preadv

[Warner Losh]

Implement do_bsd_{read,pread,readv,preadv}. Connect them to the system
call table.

Signed-off-by: Stacey Son <sson@FreeBSD.org>
Signed-off-by: Kyle Evans <kevans@FreeBSD.org>
Signed-off-by: Warner Losh <imp@bsdimp.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 bsd-user/bsd-file.h           | 79 +++++++++++++++++++++++++++++++++++
 bsd-user/freebsd/os-syscall.c | 24 +++++++++++
 2 files changed, 103 insertions(+)

diff --git a/bsd-user/bsd-file.h b/bsd-user/bsd-file.h
index a6bff3b8c26..ed305439e1a 100644
--- a/bsd-user/bsd-file.h
+++ b/bsd-user/bsd-file.h
@@ -27,4 +27,83 @@ extern struct iovec *lock_iovec(int type, abi_ulong target_addr, int count,
 extern void unlock_iovec(struct iovec *vec, abi_ulong target_addr, int count,
         int copy);
 
+ssize_t safe_read(int fd, void *buf, size_t nbytes);
+ssize_t safe_pread(int fd, void *buf, size_t nbytes, off_t offset);
+ssize_t safe_readv(int fd, const struct iovec *iov, int iovcnt);
+ssize_t safe_preadv(int fd, const struct iovec *iov, int iovcnt, off_t offset);
+
+/* read(2) */
+static inline abi_long do_bsd_read(abi_long arg1, abi_long arg2, abi_long arg3)
+{
+    abi_long ret;
+    void *p;
+
+    p = lock_user(VERIFY_WRITE, arg2, arg3, 0);
+    if (p == NULL) {
+        return -TARGET_EFAULT;
+    }
+    ret = get_errno(safe_read(arg1, p, arg3));
+    unlock_user(p, arg2, ret);
+
+    return ret;
+}
+
+/* pread(2) */
+static inline abi_long do_bsd_pread(void *cpu_env, abi_long arg1,
+    abi_long arg2, abi_long arg3, abi_long arg4, abi_long arg5, abi_long arg6)
+{
+    abi_long ret;
+    void *p;
+
+    p = lock_user(VERIFY_WRITE, arg2, arg3, 0);
+    if (p == NULL) {
+        return -TARGET_EFAULT;
+    }
+    if (regpairs_aligned(cpu_env) != 0) {
+        arg4 = arg5;
+        arg5 = arg6;
+    }
+    ret = get_errno(safe_pread(arg1, p, arg3, target_arg64(arg4, arg5)));
+    unlock_user(p, arg2, ret);
+
+    return ret;
+}
+
+/* readv(2) */
+static inline abi_long do_bsd_readv(abi_long arg1, abi_long arg2, abi_long arg3)
+{
+    abi_long ret;
+    struct iovec *vec = lock_iovec(VERIFY_WRITE, arg2, arg3, 0);
+
+    if (vec != NULL) {
+        ret = get_errno(safe_readv(arg1, vec, arg3));
+        unlock_iovec(vec, arg2, arg3, 1);
+    } else {
+        ret = -host_to_target_errno(errno);
+    }
+
+    return ret;
+}
+
+/* preadv(2) */
+static inline abi_long do_bsd_preadv(void *cpu_env, abi_long arg1,
+    abi_long arg2, abi_long arg3, abi_long arg4, abi_long arg5, abi_long arg6)
+{
+    abi_long ret;
+    struct iovec *vec = lock_iovec(VERIFY_WRITE, arg2, arg3, 1);
+
+    if (vec != NULL) {
+        if (regpairs_aligned(cpu_env) != 0) {
+            arg4 = arg5;
+            arg5 = arg6;
+        }
+        ret = get_errno(safe_preadv(arg1, vec, arg3, target_arg64(arg4, arg5)));
+        unlock_iovec(vec, arg2, arg3, 0);
+    } else {
+        ret = -host_to_target_errno(errno);
+    }
+
+    return ret;
+}
+
 #endif /* BSD_FILE_H */
diff --git a/bsd-user/freebsd/os-syscall.c b/bsd-user/freebsd/os-syscall.c
index 334c573739b..79fb2cb69f8 100644
--- a/bsd-user/freebsd/os-syscall.c
+++ b/bsd-user/freebsd/os-syscall.c
@@ -42,6 +42,14 @@
 
 #include "bsd-file.h"
 
+/* I/O */
+safe_syscall3(ssize_t, read, int, fd, void *, buf, size_t, nbytes);
+safe_syscall4(ssize_t, pread, int, fd, void *, buf, size_t, nbytes, off_t,
+    offset);
+safe_syscall3(ssize_t, readv, int, fd, const struct iovec *, iov, int, iovcnt);
+safe_syscall4(ssize_t, preadv, int, fd, const struct iovec *, iov, int, iovcnt,
+    off_t, offset);
+
 void target_set_brk(abi_ulong new_brk)
 {
 }
@@ -211,6 +219,22 @@ static abi_long freebsd_syscall(void *cpu_env, int num, abi_long arg1,
     abi_long ret;
 
     switch (num) {
+
+        /*
+         * File system calls.
+         */
+    case TARGET_FREEBSD_NR_read: /* read(2) */
+        ret = do_bsd_read(arg1, arg2, arg3);
+        break;
+
+    case TARGET_FREEBSD_NR_pread: /* pread(2) */
+        ret = do_bsd_pread(cpu_env, arg1, arg2, arg3, arg4, arg5, arg6);
+        break;
+
+    case TARGET_FREEBSD_NR_readv: /* readv(2) */
+        ret = do_bsd_readv(arg1, arg2, arg3);
+        break;
+
     default:
         gemu_log("qemu: unsupported syscall: %d\n", num);
         ret = -TARGET_ENOSYS;
-- 
2.33.1

Re: [PATCH 4/6] bsd-user/bsd-file.h: Add implementations for read, pread, readv and preadv

[Richard Henderson]

On 6/7/22 13:14, Warner Losh wrote:
> +/* read(2) */
> +static inline abi_long do_bsd_read(abi_long arg1, abi_long arg2, abi_long arg3)

Why the inline markers?  Best to drop them.

> +        /*
> +         * File system calls.
> +         */
> +    case TARGET_FREEBSD_NR_read: /* read(2) */
> +        ret = do_bsd_read(arg1, arg2, arg3);
> +        break;
> +
> +    case TARGET_FREEBSD_NR_pread: /* pread(2) */
> +        ret = do_bsd_pread(cpu_env, arg1, arg2, arg3, arg4, arg5, arg6);
> +        break;
> +
> +    case TARGET_FREEBSD_NR_readv: /* readv(2) */
> +        ret = do_bsd_readv(arg1, arg2, arg3);
> +        break;

Missing preadv, which you added above.

r~

Re: [PATCH 4/6] bsd-user/bsd-file.h: Add implementations for read, pread, readv and preadv

[Warner Losh]

[-- Attachment #1: Type: text/plain, Size: 1042 bytes --]

On Tue, Jun 7, 2022 at 2:45 PM Richard Henderson <
richard.henderson@linaro.org> wrote:

> On 6/7/22 13:14, Warner Losh wrote:
> > +/* read(2) */
> > +static inline abi_long do_bsd_read(abi_long arg1, abi_long arg2,
> abi_long arg3)
>
> Why the inline markers?  Best to drop them.
>

static inline ensures that we don't get a warning if bsd-file.h is included
in multiple places and these routines aren't used.

Though it turns out....


> > +        /*
> > +         * File system calls.
> > +         */
> > +    case TARGET_FREEBSD_NR_read: /* read(2) */
> > +        ret = do_bsd_read(arg1, arg2, arg3);
> > +        break;
> > +
> > +    case TARGET_FREEBSD_NR_pread: /* pread(2) */
> > +        ret = do_bsd_pread(cpu_env, arg1, arg2, arg3, arg4, arg5, arg6);
> > +        break;
> > +
> > +    case TARGET_FREEBSD_NR_readv: /* readv(2) */
> > +        ret = do_bsd_readv(arg1, arg2, arg3);
> > +        break;
>
> Missing preadv, which you added above.


It would have caught this :)  I'll update this and the write changes...

Warner

[-- Attachment #2: Type: text/html, Size: 1759 bytes --]

[PATCH 5/6] bsd-user/bsd-file.h: Meat of the write system calls

[Warner Losh]

Implement write, writev, pwrite and pwritev and connect them to the
system call dispatch routine.

Signed-off-by: Stacey Son <sson@FreeBSD.org>
Signed-off-by: Kyle Evans <kevans@FreeBSD.org>
Signed-off-by: Warner Losh <imp@bsdimp.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 bsd-user/bsd-file.h           | 85 +++++++++++++++++++++++++++++++++++
 bsd-user/freebsd/os-syscall.c | 23 ++++++++++
 2 files changed, 108 insertions(+)

diff --git a/bsd-user/bsd-file.h b/bsd-user/bsd-file.h
index ed305439e1a..9c3dcb9ef3f 100644
--- a/bsd-user/bsd-file.h
+++ b/bsd-user/bsd-file.h
@@ -32,6 +32,11 @@ ssize_t safe_pread(int fd, void *buf, size_t nbytes, off_t offset);
 ssize_t safe_readv(int fd, const struct iovec *iov, int iovcnt);
 ssize_t safe_preadv(int fd, const struct iovec *iov, int iovcnt, off_t offset);
 
+ssize_t safe_write(int fd, void *buf, size_t nbytes);
+ssize_t safe_pwrite(int fd, void *buf, size_t nbytes, off_t offset);
+ssize_t safe_writev(int fd, const struct iovec *iov, int iovcnt);
+ssize_t safe_pwritev(int fd, const struct iovec *iov, int iovcnt, off_t offset);
+
 /* read(2) */
 static inline abi_long do_bsd_read(abi_long arg1, abi_long arg2, abi_long arg3)
 {
@@ -106,4 +111,84 @@ static inline abi_long do_bsd_preadv(void *cpu_env, abi_long arg1,
     return ret;
 }
 
+/* write(2) */
+static inline abi_long do_bsd_write(abi_long arg1, abi_long arg2, abi_long arg3)
+{
+    abi_long nbytes, ret;
+    void *p;
+
+    /* nbytes < 0 implies that it was larger than SIZE_MAX. */
+    nbytes = arg3;
+    if (nbytes < 0) {
+        return -TARGET_EINVAL;
+    }
+    p = lock_user(VERIFY_READ, arg2, nbytes, 1);
+    if (p == NULL) {
+        return -TARGET_EFAULT;
+    }
+    ret = get_errno(safe_write(arg1, p, arg3));
+    unlock_user(p, arg2, 0);
+
+    return ret;
+}
+
+/* pwrite(2) */
+static inline abi_long do_bsd_pwrite(void *cpu_env, abi_long arg1,
+    abi_long arg2, abi_long arg3, abi_long arg4, abi_long arg5, abi_long arg6)
+{
+    abi_long ret;
+    void *p;
+
+    p = lock_user(VERIFY_READ, arg2, arg3, 1);
+    if (p == NULL) {
+        return -TARGET_EFAULT;
+    }
+    if (regpairs_aligned(cpu_env) != 0) {
+        arg4 = arg5;
+        arg5 = arg6;
+    }
+    ret = get_errno(safe_pwrite(arg1, p, arg3, target_arg64(arg4, arg5)));
+    unlock_user(p, arg2, 0);
+
+    return ret;
+}
+
+/* writev(2) */
+static inline abi_long do_bsd_writev(abi_long arg1, abi_long arg2,
+        abi_long arg3)
+{
+    abi_long ret;
+    struct iovec *vec = lock_iovec(VERIFY_READ, arg2, arg3, 1);
+
+    if (vec != NULL) {
+        ret = get_errno(safe_writev(arg1, vec, arg3));
+        unlock_iovec(vec, arg2, arg3, 0);
+    } else {
+        ret = -host_to_target_errno(errno);
+    }
+
+    return ret;
+}
+
+/* pwritev(2) */
+static inline abi_long do_bsd_pwritev(void *cpu_env, abi_long arg1,
+    abi_long arg2, abi_long arg3, abi_long arg4, abi_long arg5, abi_long arg6)
+{
+    abi_long ret;
+    struct iovec *vec = lock_iovec(VERIFY_READ, arg2, arg3, 1);
+
+    if (vec != NULL) {
+        if (regpairs_aligned(cpu_env) != 0) {
+            arg4 = arg5;
+            arg5 = arg6;
+        }
+        ret = get_errno(safe_pwritev(arg1, vec, arg3, target_arg64(arg4, arg5)));
+        unlock_iovec(vec, arg2, arg3, 0);
+    } else {
+        ret = -host_to_target_errno(errno);
+    }
+
+    return ret;
+}
+
 #endif /* BSD_FILE_H */
diff --git a/bsd-user/freebsd/os-syscall.c b/bsd-user/freebsd/os-syscall.c
index 79fb2cb69f8..4c7c32daa56 100644
--- a/bsd-user/freebsd/os-syscall.c
+++ b/bsd-user/freebsd/os-syscall.c
@@ -50,6 +50,13 @@ safe_syscall3(ssize_t, readv, int, fd, const struct iovec *, iov, int, iovcnt);
 safe_syscall4(ssize_t, preadv, int, fd, const struct iovec *, iov, int, iovcnt,
     off_t, offset);
 
+safe_syscall3(ssize_t, write, int, fd, void *, buf, size_t, nbytes);
+safe_syscall4(ssize_t, pwrite, int, fd, void *, buf, size_t, nbytes, off_t,
+    offset);
+safe_syscall3(ssize_t, writev, int, fd, const struct iovec *, iov, int, iovcnt);
+safe_syscall4(ssize_t, pwritev, int, fd, const struct iovec *, iov, int, iovcnt,
+    off_t, offset);
+
 void target_set_brk(abi_ulong new_brk)
 {
 }
@@ -235,6 +242,22 @@ static abi_long freebsd_syscall(void *cpu_env, int num, abi_long arg1,
         ret = do_bsd_readv(arg1, arg2, arg3);
         break;
 
+    case TARGET_FREEBSD_NR_write: /* write(2) */
+        ret = do_bsd_write(arg1, arg2, arg3);
+        break;
+
+    case TARGET_FREEBSD_NR_pwrite: /* pwrite(2) */
+        ret = do_bsd_pwrite(cpu_env, arg1, arg2, arg3, arg4, arg5, arg6);
+        break;
+
+    case TARGET_FREEBSD_NR_writev: /* writev(2) */
+        ret = do_bsd_writev(arg1, arg2, arg3);
+        break;
+
+    case TARGET_FREEBSD_NR_pwritev: /* pwritev(2) */
+        ret = do_bsd_pwritev(cpu_env, arg1, arg2, arg3, arg4, arg5, arg6);
+        break;
+
     default:
         gemu_log("qemu: unsupported syscall: %d\n", num);
         ret = -TARGET_ENOSYS;
-- 
2.33.1

Re: [PATCH 5/6] bsd-user/bsd-file.h: Meat of the write system calls

[Richard Henderson]

On 6/7/22 13:14, Warner Losh wrote:
> +/* write(2) */
> +static inline abi_long do_bsd_write(abi_long arg1, abi_long arg2, abi_long arg3)

Likewise drop the inline markers.


r~

[PATCH 6/6] bsd-user/freebsd/os-syscall.c: Implement exit

[Warner Losh]

Implement the exit system call. Bring in bsd-proc.h to contain all the
process system call implementation and helper routines.

Signed-off-by: Stacey Son <sson@FreeBSD.org>
Signed-off-by: Warner Losh <imp@bsdimp.com>
Reviewed-by: Kyle Evans <kevans@FreeBSD.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 bsd-user/bsd-proc.h           | 43 +++++++++++++++++++++++++++++++++++
 bsd-user/freebsd/os-syscall.c |  7 ++++++
 2 files changed, 50 insertions(+)
 create mode 100644 bsd-user/bsd-proc.h

diff --git a/bsd-user/bsd-proc.h b/bsd-user/bsd-proc.h
new file mode 100644
index 00000000000..8f0b6990d14
--- /dev/null
+++ b/bsd-user/bsd-proc.h
@@ -0,0 +1,43 @@
+/*
+ *  process related system call shims and definitions
+ *
+ *  Copyright (c) 2013-2014 Stacey D. Son
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef BSD_PROC_H_
+#define BSD_PROC_H_
+
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <sys/time.h>
+#include <sys/resource.h>
+#include <unistd.h>
+
+/* exit(2) */
+static inline abi_long do_bsd_exit(void *cpu_env, abi_long arg1)
+{
+#ifdef TARGET_GPROF
+    _mcleanup();
+#endif
+    gdb_exit(arg1);
+    qemu_plugin_user_exit();
+    /* XXX: should free thread stack and CPU env here  */
+    _exit(arg1);
+
+    return 0;
+}
+
+#endif /* !BSD_PROC_H_ */
diff --git a/bsd-user/freebsd/os-syscall.c b/bsd-user/freebsd/os-syscall.c
index 4c7c32daa56..2daba0e623c 100644
--- a/bsd-user/freebsd/os-syscall.c
+++ b/bsd-user/freebsd/os-syscall.c
@@ -41,6 +41,7 @@
 #include "user/syscall-trace.h"
 
 #include "bsd-file.h"
+#include "bsd-proc.h"
 
 /* I/O */
 safe_syscall3(ssize_t, read, int, fd, void *, buf, size_t, nbytes);
@@ -226,6 +227,12 @@ static abi_long freebsd_syscall(void *cpu_env, int num, abi_long arg1,
     abi_long ret;
 
     switch (num) {
+        /*
+         * process system calls
+         */
+    case TARGET_FREEBSD_NR_exit: /* exit(2) */
+        ret = do_bsd_exit(cpu_env, arg1);
+        break;
 
         /*
          * File system calls.
-- 
2.33.1