[PATCH 0/9] 2023 Q1 bsd-user upstreaming: bugfixes and sysctl

[PATCH 0/9] 2023 Q1 bsd-user upstreaming: bugfixes and sysctl

[Warner Losh]

This group of patches gets the basic framework for sysctl upstreamed. There's a
lot more to translate far too many binary blobs the kernel publishes via
sysctls, but I'm leaving those out in the name of simplicity.

There's also a bug fix from Doug Rabson that fixes a long int confusion leading
to a trunctation of addresses (oops)

I've added a new command line arg -strict for debugging when you want to stop
dead in the tracks when there's something unimplemented (like system calls)
rather than trying one's best to cope. It will also let whoever is working on
upstreaming to get something working to run it this way and find the missing
bits more easily. sysctl happens to be the first unimplemented system call for a
dynamically linked hello world.

There's a fix for the -static option, since clang hates -no-pie and needs only
-fno-pie.

Finally, I'm changing how I'm upstreaming a little. I'm doing a little deeper
dives into our rather chaotic repo to find a couple of authors I might have
missed. From here on out, I'll be using the original author's name as the git
author. I'll also tag the co-authors better as well when there's multiple people
that did something (other than reformat and/or move code around). I've
discovered more code moved about than I'd previously known. This seems more in
line with standard practice. Also, I've reviewed all these changes, but I don't
know if I need to add Reviewed-by: or not, so I've done it for one or two and
will make it consistent before the pull request. git log suggests maintainers
are inconsistent about this (or I've not discovered the rules they follow).

check-patch gives some line lenght warnings, but should otherwise be OK. There's
several static functions that aren't used until the end of the patch
series... Not sure the best way to suppress the build warnings there (but since
they are just warnings...).

Doug Rabson (1):
  bsd-user: Don't truncate the return value from freebsd_syscall

Juergen Lock (2):
  bsd-user: sysctl helper funtions: sysctl_name2oid and sysctl_oidfmt
  bsd-user: common routine do_freebsd_sysctl_oid for all sysctl variants

Kyle Evans (2):
  bsd-user: do_freebsd_sysctl helper for sysctl(2)
  bsd-user: implement sysctlbyname(2)

Stacey Son (2):
  bsd-user: Add sysarch syscall
  bsd-user: Two helper routines oidfmt and sysctl_oldcvt

Warner Losh (2):
  build: Don't specify -no-pie for --static user-mode programs
  bsd-user: Add -strict

 bsd-user/freebsd/os-sys.c     | 432 ++++++++++++++++++++++++++++++++++
 bsd-user/freebsd/os-syscall.c |  21 +-
 bsd-user/main.c               |   5 +-
 bsd-user/qemu.h               |   6 +
 configure                     |   2 +-
 5 files changed, 463 insertions(+), 3 deletions(-)

-- 
2.39.1

[PATCH 1/9] bsd-user: Don't truncate the return value from freebsd_syscall

[Warner Losh]

From: Doug Rabson <dfr@rabson.org>

System call return values on FreeBSD are in a register (which is spelled
api_long in qemu). This was being assigned into an int variable which
causes problems for 64bit targets.

Resolves: https://github.com/qemu-bsd-user/qemu-bsd-user/issues/40
Signed-off-by: Doug Rabson <dfr@rabson.org>
Reviewed-by: Warner Losh <imp@bsdimp.com>
[ Edited commit message for upstreaming into qemu-project ]
Signed-off-by: Warner Losh <imp@bsdimp.com>
---
 bsd-user/freebsd/os-syscall.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/bsd-user/freebsd/os-syscall.c b/bsd-user/freebsd/os-syscall.c
index 57996cad8ae..b4a663fc021 100644
--- a/bsd-user/freebsd/os-syscall.c
+++ b/bsd-user/freebsd/os-syscall.c
@@ -512,7 +512,7 @@ abi_long do_freebsd_syscall(void *cpu_env, int num, abi_long arg1,
                             abi_long arg8)
 {
     CPUState *cpu = env_cpu(cpu_env);
-    int ret;
+    abi_long ret;
 
     trace_guest_user_syscall(cpu, num, arg1, arg2, arg3, arg4, arg5, arg6, arg7, arg8);
     if (do_strace) {
-- 
2.39.1

[PATCH 2/9] build: Don't specify -no-pie for --static user-mode programs

[Warner Losh]

When building with clang, -no-pie gives a warning on every single build,
so remove it.

Signed-off-by: Warner Losh <imp@bsdimp.com>
---
 configure | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/configure b/configure
index 64960c6000f..eb284ccf308 100755
--- a/configure
+++ b/configure
@@ -1313,7 +1313,7 @@ if test "$static" = "yes"; then
     error_exit "-static-pie not available due to missing toolchain support"
   else
     pie="no"
-    QEMU_CFLAGS="-fno-pie -no-pie $QEMU_CFLAGS"
+    QEMU_CFLAGS="-fno-pie $QEMU_CFLAGS"
   fi
 elif test "$pie" = "no"; then
   if compile_prog "-Werror -fno-pie" "-no-pie"; then
-- 
2.39.1

[PATCH 3/9] bsd-user: Add sysarch syscall

[Warner Losh]

From: Stacey Son <sson@FreeBSD.org>

Connect up the sysarch system call.

Signed-off-by: Juergen Lock <nox@jelal.kn-bremen.de>
Co-authored-by: Juergen Lock <nox@jelal.kn-bremen.de>
Signed-off-by: Stacey Son <sson@FreeBSD.org>
Reviewed-by: Warner Losh <imp@bsdimp.com>
Signed-off-by: Warner Losh <imp@bsdimp.com>
---
 bsd-user/freebsd/os-syscall.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/bsd-user/freebsd/os-syscall.c b/bsd-user/freebsd/os-syscall.c
index b4a663fc021..e00997a818c 100644
--- a/bsd-user/freebsd/os-syscall.c
+++ b/bsd-user/freebsd/os-syscall.c
@@ -491,6 +491,13 @@ static abi_long freebsd_syscall(void *cpu_env, int num, abi_long arg1,
         ret = do_bsd_undelete(arg1);
         break;
 
+        /*
+         * sys{ctl, arch, call}
+         */
+    case TARGET_FREEBSD_NR_sysarch: /* sysarch(2) */
+        ret = do_freebsd_sysarch(cpu_env, arg1, arg2);
+        break;
+
     default:
         qemu_log_mask(LOG_UNIMP, "Unsupported syscall: %d\n", num);
         ret = -TARGET_ENOSYS;
-- 
2.39.1

[PATCH 4/9] bsd-user: Two helper routines oidfmt and sysctl_oldcvt

[Warner Losh]

From: Stacey Son <sson@FreeBSD.org>

oidfmt uses undocumented system call to get the type of the sysctl.
sysctl_oldcvt does the byte swapping in the data to return it to the
target.

Co-Authored-by: Sean Bruno <sbruno@FreeBSD.org>
Signed-off-by: Sean Bruno <sbruno@FreeBSD.org>
Co-Authored-by: Juergen Lock <nox@jelal.kn-bremen.de>
Signed-off-by: Juergen Lock <nox@jelal.kn-bremen.de>
Co-Authored-by: Raphael Kubo da Costa <rakuco@FreeBSD.org>
Signed-off-by: Raphael Kubo da Costa <rakuco@FreeBSD.org>
Signed-off-by: Stacey Son <sson@FreeBSD.org>
Signed-off-by: Warner Losh <imp@bsdimp.com>
---
 bsd-user/freebsd/os-sys.c | 94 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 94 insertions(+)

diff --git a/bsd-user/freebsd/os-sys.c b/bsd-user/freebsd/os-sys.c
index 1676ec10f83..e3b9f168a2b 100644
--- a/bsd-user/freebsd/os-sys.c
+++ b/bsd-user/freebsd/os-sys.c
@@ -21,6 +21,100 @@
 #include "qemu.h"
 #include "target_arch_sysarch.h"
 
+#include <sys/sysctl.h>
+
+/*
+ * This uses the undocumented oidfmt interface to find the kind of a requested
+ * sysctl, see /sys/kern/kern_sysctl.c:sysctl_sysctl_oidfmt() (compare to
+ * src/sbin/sysctl/sysctl.c)
+ */
+static int oidfmt(int *oid, int len, char *fmt, uint32_t *kind)
+{
+    int qoid[CTL_MAXNAME + 2];
+    uint8_t buf[BUFSIZ];
+    int i;
+    size_t j;
+
+    qoid[0] = 0;
+    qoid[1] = 4;
+    memcpy(qoid + 2, oid, len * sizeof(int));
+
+    j = sizeof(buf);
+    i = sysctl(qoid, len + 2, buf, &j, 0, 0);
+    if (i) {
+        return i;
+    }
+
+    if (kind) {
+        *kind = *(uint32_t *)buf;
+    }
+
+    if (fmt) {
+        strcpy(fmt, (char *)(buf + sizeof(uint32_t)));
+    }
+    return 0;
+}
+
+/*
+ * try and convert sysctl return data for the target.
+ * Note: doesn't handle CTLTYPE_OPAQUE and CTLTYPE_STRUCT.
+ */
+static int sysctl_oldcvt(void *holdp, size_t *holdlen, uint32_t kind)
+{
+    switch (kind & CTLTYPE) {
+    case CTLTYPE_INT:
+    case CTLTYPE_UINT:
+        *(uint32_t *)holdp = tswap32(*(uint32_t *)holdp);
+        break;
+
+#ifdef TARGET_ABI32
+    case CTLTYPE_LONG:
+    case CTLTYPE_ULONG:
+        /*
+         * If the sysctl has a type of long/ulong but seems to be bigger than
+         * these data types, its probably an array.  Double check that its
+         * evenly divisible by the size of long and convert holdp to a series of
+         * 32bit elements instead, adjusting holdlen to the new size.
+         */
+        if ((*holdlen > sizeof(abi_ulong)) &&
+            ((*holdlen % sizeof(abi_ulong)) == 0)) {
+            int array_size = *holdlen / sizeof(long);
+            int i;
+            if (holdp) {
+                for (i = 0; i < array_size; i++) {
+                    ((uint32_t *)holdp)[i] = tswap32(((long *)holdp)[i]);
+                }
+                *holdlen = array_size * sizeof(abi_ulong);
+            } else {
+                *holdlen = sizeof(abi_ulong);
+            }
+        } else {
+            *(uint32_t *)holdp = tswap32(*(long *)holdp);
+            *holdlen = sizeof(uint32_t);
+        }
+        break;
+#else
+    case CTLTYPE_LONG:
+        *(uint64_t *)holdp = tswap64(*(long *)holdp);
+        break;
+    case CTLTYPE_ULONG:
+        *(uint64_t *)holdp = tswap64(*(unsigned long *)holdp);
+        break;
+#endif
+    case CTLTYPE_U64:
+    case CTLTYPE_S64:
+        *(uint64_t *)holdp = tswap64(*(uint64_t *)holdp);
+        break;
+
+    case CTLTYPE_STRING:
+        break;
+
+    default:
+        return -1;
+    }
+    return 0;
+}
+
 /* sysarch() is architecture dependent. */
 abi_long do_freebsd_sysarch(void *cpu_env, abi_long arg1, abi_long arg2)
 {
-- 
2.39.1

[PATCH 5/9] bsd-user: sysctl helper funtions: sysctl_name2oid and sysctl_oidfmt

[Warner Losh]

From: Juergen Lock <nox@jelal.kn-bremen.de>

Helper functions for sysctl implementations. sysctl_name2oid and
sysctl_oidfmt convert oids between host and targets

Signed-off-by: Juergen Lock <nox@jelal.kn-bremen.de>
Signed-off-by: Warner Losh <imp@bsdimp.com>
---
 bsd-user/freebsd/os-sys.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/bsd-user/freebsd/os-sys.c b/bsd-user/freebsd/os-sys.c
index e3b9f168a2b..ac5ab9b17bc 100644
--- a/bsd-user/freebsd/os-sys.c
+++ b/bsd-user/freebsd/os-sys.c
@@ -115,6 +115,24 @@ static int sysctl_oldcvt(void *holdp, size_t *holdlen, uint32_t kind)
     return 0;
 }
 
+/*
+ * Convert the undocmented name2oid sysctl data for the target.
+ */
+static inline void sysctl_name2oid(uint32_t *holdp, size_t holdlen)
+{
+    size_t i, num = holdlen / sizeof(uint32_t);
+
+    for (i = 0; i < num; i++) {
+        holdp[i] = tswap32(holdp[i]);
+    }
+}
+
+static inline void sysctl_oidfmt(uint32_t *holdp)
+{
+    /* byte swap the kind */
+    holdp[0] = tswap32(holdp[0]);
+}
+
 /* sysarch() is architecture dependent. */
 abi_long do_freebsd_sysarch(void *cpu_env, abi_long arg1, abi_long arg2)
 {
-- 
2.39.1

[PATCH 6/9] bsd-user: common routine do_freebsd_sysctl_oid for all sysctl variants

[Warner Losh]

From: Juergen Lock <nox@jelal.kn-bremen.de>

do_freebsd_sysctl_oid filters out some of the binary and special sysctls
where host != target. This commit focuses on the simple sysctls that can
be done in a few lines.

Signed-off-by: Juergen Lock <nox@jelal.kn-bremen.de>
Co-Authored-by: Stacey Son <sson@FreeBSD.org>
Signed-off-by: Stacey Son <sson@FreeBSD.org>
Signed-off-by: Warner Losh <imp@bsdimp.com>
---
 bsd-user/freebsd/os-sys.c | 212 ++++++++++++++++++++++++++++++++++++++
 bsd-user/qemu.h           |   5 +
 2 files changed, 217 insertions(+)

diff --git a/bsd-user/freebsd/os-sys.c b/bsd-user/freebsd/os-sys.c
index ac5ab9b17bc..a8fb29f36b7 100644
--- a/bsd-user/freebsd/os-sys.c
+++ b/bsd-user/freebsd/os-sys.c
@@ -133,6 +133,218 @@ static inline void sysctl_oidfmt(uint32_t *holdp)
     holdp[0] = tswap32(holdp[0]);
 }
 
+#define bsd_get_ncpu() 1 /* Placeholder */
+
+static abi_long do_freebsd_sysctl_oid(CPUArchState *env, int32_t *snamep,
+        int32_t namelen, void *holdp, size_t *holdlenp, void *hnewp,
+        size_t newlen)
+{
+    uint32_t kind = 0;
+#if TARGET_ABI_BITS != HOST_LONG_BITS
+    const abi_ulong maxmem = -0x100c000;
+#endif
+    abi_long ret;
+    size_t holdlen, oldlen;
+
+    holdlen = oldlen = *holdlenp;
+    oidfmt(snamep, namelen, NULL, &kind);
+
+    /* Handle some arch/emulator dependent sysctl()'s here. */
+    switch (snamep[0]) {
+#if defined(TARGET_PPC) || defined(TARGET_PPC64)
+    case CTL_MACHDEP:
+        switch (snamep[1]) {
+        case 1:    /* CPU_CACHELINE */
+            holdlen = sizeof(uint32_t);
+            (*(uint32_t *)holdp) = tswap32(env->dcache_line_size);
+            ret = 0;
+            goto out;
+        }
+        break;
+#endif
+    case CTL_KERN:
+        switch (snamep[1]) {
+        case KERN_USRSTACK:
+            if (oldlen) {
+                (*(abi_ulong *)holdp) = tswapal(TARGET_USRSTACK);
+            }
+            holdlen = sizeof(abi_ulong);
+            ret = 0;
+            goto out;
+
+        case KERN_PS_STRINGS:
+            if (oldlen) {
+                (*(abi_ulong *)holdp) = tswapal(TARGET_PS_STRINGS);
+            }
+            holdlen = sizeof(abi_ulong);
+            ret = 0;
+            goto out;
+
+        default:
+            break;
+        }
+        break;
+
+    case CTL_HW:
+        switch (snamep[1]) {
+        case HW_MACHINE:
+            holdlen = sizeof(TARGET_HW_MACHINE);
+            if (holdp) {
+                strlcpy(holdp, TARGET_HW_MACHINE, oldlen);
+            }
+            ret = 0;
+            goto out;
+
+        case HW_MACHINE_ARCH:
+        {
+            holdlen = sizeof(TARGET_HW_MACHINE_ARCH);
+            if (holdp) {
+                strlcpy(holdp, TARGET_HW_MACHINE_ARCH, oldlen);
+            }
+            ret = 0;
+            goto out;
+        }
+        case HW_NCPU:
+            if (oldlen) {
+                (*(int32_t *)holdp) = tswap32(bsd_get_ncpu());
+            }
+            holdlen = sizeof(int32_t);
+            ret = 0;
+            goto out;
+#if defined(TARGET_ARM)
+        case HW_FLOATINGPT:
+            if (oldlen) {
+#ifdef ARM_FEATURE_VFP /* XXX FIXME XXX */
+                if (env->features & ((1ULL << ARM_FEATURE_VFP)|
+                                     (1ULL << ARM_FEATURE_VFP3)|
+                                     (1ULL << ARM_FEATURE_VFP4)))
+                    *(int32_t *)holdp = 1;
+                else
+                    *(int32_t *)holdp = 0;
+#else
+                *(int32_t *)holdp = 1;
+#endif
+            }
+            holdlen = sizeof(int32_t);
+            ret = 0;
+            goto out;
+#endif
+
+
+#if TARGET_ABI_BITS != HOST_LONG_BITS
+        case HW_PHYSMEM:
+        case HW_USERMEM:
+        case HW_REALMEM:
+            holdlen = sizeof(abi_ulong);
+            ret = 0;
+
+            if (oldlen) {
+                int mib[2] = {snamep[0], snamep[1]};
+                unsigned long lvalue;
+                size_t len = sizeof(lvalue);
+
+                if (sysctl(mib, 2, &lvalue, &len, NULL, 0) == -1) {
+                    ret = -1;
+                } else {
+                    if (((unsigned long)maxmem) < lvalue) {
+                        lvalue = maxmem;
+                    }
+                    (*(abi_ulong *)holdp) = tswapal((abi_ulong)lvalue);
+                }
+            }
+            goto out;
+#endif
+
+        default:
+        {
+            static int oid_hw_availpages;
+            static int oid_hw_pagesizes;
+
+            if (!oid_hw_availpages) {
+                int real_oid[CTL_MAXNAME + 2];
+                size_t len = sizeof(real_oid) / sizeof(int);
+
+                if (sysctlnametomib("hw.availpages", real_oid, &len) >= 0) {
+                    oid_hw_availpages = real_oid[1];
+                }
+            }
+            if (!oid_hw_pagesizes) {
+                int real_oid[CTL_MAXNAME + 2];
+                size_t len = sizeof(real_oid) / sizeof(int);
+
+                if (sysctlnametomib("hw.pagesizes", real_oid, &len) >= 0) {
+                    oid_hw_pagesizes = real_oid[1];
+                }
+            }
+
+            if (oid_hw_availpages && snamep[1] == oid_hw_availpages) {
+                long lvalue;
+                size_t len = sizeof(lvalue);
+
+                if (sysctlbyname("hw.availpages", &lvalue, &len, NULL, 0) == -1) {
+                    ret = -1;
+                } else {
+                    if (oldlen) {
+#if TARGET_ABI_BITS != HOST_LONG_BITS
+                        abi_ulong maxpages = maxmem / (abi_ulong)getpagesize();
+                        if (((unsigned long)maxpages) < lvalue) {
+                            lvalue = maxpages;
+                        }
+#endif
+                        (*(abi_ulong *)holdp) = tswapal((abi_ulong)lvalue);
+                    }
+                    holdlen = sizeof(abi_ulong);
+                    ret = 0;
+                }
+                goto out;
+            }
+
+            if (oid_hw_pagesizes && snamep[1] == oid_hw_pagesizes) {
+                if (oldlen) {
+                    (*(abi_ulong *)holdp) = tswapal((abi_ulong)getpagesize());
+                    ((abi_ulong *)holdp)[1] = 0;
+                }
+                holdlen = sizeof(abi_ulong) * 2;
+                ret = 0;
+                goto out;
+            }
+            break;
+        }
+        }
+        break;
+
+    default:
+        break;
+    }
+
+    ret = get_errno(sysctl(snamep, namelen, holdp, &holdlen, hnewp, newlen));
+    if (!ret && (holdp != 0)) {
+
+        if (0 == snamep[0] &&
+            (2 == snamep[1] || 3 == snamep[1] || 4 == snamep[1])) {
+            switch (snamep[1]) {
+            case 2:
+            case 3:
+                /* Handle the undocumented name2oid special case. */
+                sysctl_name2oid(holdp, holdlen);
+                break;
+
+            case 4:
+            default:
+                /* Handle oidfmt */
+                sysctl_oidfmt(holdp);
+                break;
+            }
+        } else {
+            sysctl_oldcvt(holdp, &holdlen, kind);
+        }
+    }
+
+out:
+    *holdlenp = holdlen;
+    return ret;
+}
+
 /* sysarch() is architecture dependent. */
 abi_long do_freebsd_sysarch(void *cpu_env, abi_long arg1, abi_long arg2)
 {
diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h
index 0ceecfb6dfa..e24a8cfcfb1 100644
--- a/bsd-user/qemu.h
+++ b/bsd-user/qemu.h
@@ -252,6 +252,11 @@ bool is_error(abi_long ret);
 int host_to_target_errno(int err);
 
 /* os-sys.c */
+abi_long do_freebsd_sysctl(CPUArchState *env, abi_ulong namep, int32_t namelen,
+        abi_ulong oldp, abi_ulong oldlenp, abi_ulong newp, abi_ulong newlen);
+abi_long do_freebsd_sysctlbyname(CPUArchState *env, abi_ulong namep,
+        int32_t namelen, abi_ulong oldp, abi_ulong oldlenp, abi_ulong newp,
+        abi_ulong newlen);
 abi_long do_freebsd_sysarch(void *cpu_env, abi_long arg1, abi_long arg2);
 
 /* user access */
-- 
2.39.1

[PATCH 7/9] bsd-user: do_freebsd_sysctl helper for sysctl(2)

[Warner Losh]

From: Kyle Evans <kevans@FreeBSD.org>

Implement the wrapper function for sysctl(2). This puts the oid
arguments into a standard form and calls the common
do_freebsd_sysctl_oid.

Signed-off-by: Kyle Evans <kevans@FreeBSD.org>
Co-Authored-by: Juergen Lock <nox@jelal.kn-bremen.de>
Signed-off-by: Juergen Lock <nox@jelal.kn-bremen.de>
Co-Authored-by: Stacey Son <sson@FreeBSD.org>
Signed-off-by: Stacey Son <sson@FreeBSD.org>
Signed-off-by: Warner Losh <imp@bsdimp.com>
---
 bsd-user/freebsd/os-sys.c     | 50 +++++++++++++++++++++++++++++++++++
 bsd-user/freebsd/os-syscall.c |  4 +++
 2 files changed, 54 insertions(+)

diff --git a/bsd-user/freebsd/os-sys.c b/bsd-user/freebsd/os-sys.c
index a8fb29f36b7..13736936e5f 100644
--- a/bsd-user/freebsd/os-sys.c
+++ b/bsd-user/freebsd/os-sys.c
@@ -345,6 +345,56 @@ out:
     return ret;
 }
 
+abi_long do_freebsd_sysctl(CPUArchState *env, abi_ulong namep, int32_t namelen,
+        abi_ulong oldp, abi_ulong oldlenp, abi_ulong newp, abi_ulong newlen)
+{
+    abi_long ret;
+    void *hnamep, *holdp = NULL, *hnewp = NULL;
+    size_t holdlen;
+    abi_ulong oldlen = 0;
+    int32_t *snamep = g_malloc(sizeof(int32_t) * namelen), *p, *q, i;
+
+    if (oldlenp) {
+        if (get_user_ual(oldlen, oldlenp)) {
+            return -TARGET_EFAULT;
+        }
+    }
+    hnamep = lock_user(VERIFY_READ, namep, namelen, 1);
+    if (hnamep == NULL) {
+        return -TARGET_EFAULT;
+    }
+    if (newp) {
+        hnewp = lock_user(VERIFY_READ, newp, newlen, 1);
+        if (hnewp == NULL) {
+            return -TARGET_EFAULT;
+        }
+    }
+    if (oldp) {
+        holdp = lock_user(VERIFY_WRITE, oldp, oldlen, 0);
+        if (holdp == NULL) {
+            return -TARGET_EFAULT;
+        }
+    }
+    holdlen = oldlen;
+    for (p = hnamep, q = snamep, i = 0; i < namelen; p++, i++) {
+        *q++ = tswap32(*p);
+    }
+
+    ret = do_freebsd_sysctl_oid(env, snamep, namelen, holdp, &holdlen, hnewp,
+        newlen);
+
+    if (oldlenp) {
+        put_user_ual(holdlen, oldlenp);
+    }
+    unlock_user(hnamep, namep, 0);
+    unlock_user(holdp, oldp, holdlen);
+    if (hnewp) {
+        unlock_user(hnewp, newp, 0);
+    }
+    g_free(snamep);
+    return ret;
+}
+
 /* sysarch() is architecture dependent. */
 abi_long do_freebsd_sysarch(void *cpu_env, abi_long arg1, abi_long arg2)
 {
diff --git a/bsd-user/freebsd/os-syscall.c b/bsd-user/freebsd/os-syscall.c
index e00997a818c..20ab3d4d9a1 100644
--- a/bsd-user/freebsd/os-syscall.c
+++ b/bsd-user/freebsd/os-syscall.c
@@ -494,6 +494,10 @@ static abi_long freebsd_syscall(void *cpu_env, int num, abi_long arg1,
         /*
          * sys{ctl, arch, call}
          */
+    case TARGET_FREEBSD_NR___sysctl: /* sysctl(3) */
+        ret = do_freebsd_sysctl(cpu_env, arg1, arg2, arg3, arg4, arg5, arg6);
+        break;
+
     case TARGET_FREEBSD_NR_sysarch: /* sysarch(2) */
         ret = do_freebsd_sysarch(cpu_env, arg1, arg2);
         break;
-- 
2.39.1

[PATCH 8/9] bsd-user: implement sysctlbyname(2)

[Warner Losh]

From: Kyle Evans <kevans@FreeBSD.org>

do_freebsd_sysctlbyname needs to translate the 'name' back down to a OID
so we can intercept the special ones. Do that and call the common wrapper
do_freebsd_sysctl_oid.

Signed-off-by: Kyle Evans <kevans@FreeBSD.org>
Signed-off-by: Warner Losh <imp@bsdimp.com>
---
 bsd-user/freebsd/os-sys.c     | 58 +++++++++++++++++++++++++++++++++++
 bsd-user/freebsd/os-syscall.c |  4 +++
 2 files changed, 62 insertions(+)

diff --git a/bsd-user/freebsd/os-sys.c b/bsd-user/freebsd/os-sys.c
index 13736936e5f..62c729dfe47 100644
--- a/bsd-user/freebsd/os-sys.c
+++ b/bsd-user/freebsd/os-sys.c
@@ -345,6 +345,64 @@ out:
     return ret;
 }
 
+/*
+ * This syscall was created to make sysctlbyname(3) more efficient.
+ * Unfortunately, because we have to fake some sysctls, we can't do that.
+ */
+abi_long do_freebsd_sysctlbyname(CPUArchState *env, abi_ulong namep,
+        int32_t namelen, abi_ulong oldp, abi_ulong oldlenp, abi_ulong newp,
+        abi_ulong newlen)
+{
+    abi_long ret;
+    void *holdp = NULL, *hnewp = NULL;
+    char *snamep;
+    int oid[CTL_MAXNAME + 2];
+    size_t holdlen, oidplen;
+    abi_ulong oldlen = 0;
+
+    if (oldlenp) {
+        if (get_user_ual(oldlen, oldlenp)) {
+            return -TARGET_EFAULT;
+        }
+    }
+    snamep = lock_user_string(namep);
+    if (snamep == NULL) {
+        return -TARGET_EFAULT;
+    }
+    if (newp) {
+        hnewp = lock_user(VERIFY_READ, newp, newlen, 1);
+        if (hnewp == NULL) {
+            return -TARGET_EFAULT;
+        }
+    }
+    if (oldp) {
+        holdp = lock_user(VERIFY_WRITE, oldp, oldlen, 0);
+        if (holdp == NULL) {
+            return -TARGET_EFAULT;
+        }
+    }
+    holdlen = oldlen;
+
+    oidplen = sizeof(oid) / sizeof(int);
+    if (sysctlnametomib(snamep, oid, &oidplen) != 0) {
+        return -TARGET_EINVAL;
+    }
+
+    ret = do_freebsd_sysctl_oid(env, oid, oidplen, holdp, &holdlen, hnewp,
+        newlen);
+
+    if (oldlenp) {
+        put_user_ual(holdlen, oldlenp);
+    }
+    unlock_user(snamep, namep, 0);
+    unlock_user(holdp, oldp, holdlen);
+    if (hnewp) {
+        unlock_user(hnewp, newp, 0);
+    }
+
+    return ret;
+}
+
 abi_long do_freebsd_sysctl(CPUArchState *env, abi_ulong namep, int32_t namelen,
         abi_ulong oldp, abi_ulong oldlenp, abi_ulong newp, abi_ulong newlen)
 {
diff --git a/bsd-user/freebsd/os-syscall.c b/bsd-user/freebsd/os-syscall.c
index 20ab3d4d9a1..179a20c304b 100644
--- a/bsd-user/freebsd/os-syscall.c
+++ b/bsd-user/freebsd/os-syscall.c
@@ -498,6 +498,10 @@ static abi_long freebsd_syscall(void *cpu_env, int num, abi_long arg1,
         ret = do_freebsd_sysctl(cpu_env, arg1, arg2, arg3, arg4, arg5, arg6);
         break;
 
+    case TARGET_FREEBSD_NR___sysctlbyname: /* sysctlbyname(2) */
+        ret = do_freebsd_sysctlbyname(cpu_env, arg1, arg2, arg3, arg4, arg5, arg6);
+        break;
+
     case TARGET_FREEBSD_NR_sysarch: /* sysarch(2) */
         ret = do_freebsd_sysarch(cpu_env, arg1, arg2);
         break;
-- 
2.39.1

[PATCH 9/9] bsd-user: Add -strict

[Warner Losh]

Most of the time, it's useful to make our best effort, but sometimes we
want to know right away when we don't implement something. First place
we use it is for unknown syscalls.

Signed-off-by: Warner Losh <imp@bsdimp.com>
---
 bsd-user/freebsd/os-syscall.c | 4 ++++
 bsd-user/main.c               | 5 ++++-
 bsd-user/qemu.h               | 1 +
 3 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/bsd-user/freebsd/os-syscall.c b/bsd-user/freebsd/os-syscall.c
index 179a20c304b..e2b26ecb8dd 100644
--- a/bsd-user/freebsd/os-syscall.c
+++ b/bsd-user/freebsd/os-syscall.c
@@ -508,6 +508,10 @@ static abi_long freebsd_syscall(void *cpu_env, int num, abi_long arg1,
 
     default:
         qemu_log_mask(LOG_UNIMP, "Unsupported syscall: %d\n", num);
+        if (bsd_user_strict) {
+            printf("Unimplemented system call %d\n", num);
+            abort();
+        }
         ret = -TARGET_ENOSYS;
         break;
     }
diff --git a/bsd-user/main.c b/bsd-user/main.c
index 41290e16f98..ba0ad86ad28 100644
--- a/bsd-user/main.c
+++ b/bsd-user/main.c
@@ -91,9 +91,10 @@ unsigned long reserved_va = MAX_RESERVED_VA;
 unsigned long reserved_va;
 #endif
 
-static const char *interp_prefix = CONFIG_QEMU_INTERP_PREFIX;
+const char *interp_prefix = CONFIG_QEMU_INTERP_PREFIX;
 const char *qemu_uname_release;
 char qemu_proc_pathname[PATH_MAX];  /* full path to exeutable */
+bool bsd_user_strict = false;	/* Abort for unimplemned things */
 
 unsigned long target_maxtsiz = TARGET_MAXTSIZ;   /* max text size */
 unsigned long target_dfldsiz = TARGET_DFLDSIZ;   /* initial data size limit */
@@ -396,6 +397,8 @@ int main(int argc, char **argv)
             trace_opt_parse(optarg);
         } else if (!strcmp(r, "0")) {
             argv0 = argv[optind++];
+        } else if (!strcmp(r, "strict")) {
+            bsd_user_strict = true;
         } else {
             usage();
         }
diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h
index e24a8cfcfb1..22bd5a3df42 100644
--- a/bsd-user/qemu.h
+++ b/bsd-user/qemu.h
@@ -113,6 +113,7 @@ typedef struct TaskState {
 
 void stop_all_tasks(void);
 extern const char *qemu_uname_release;
+extern bool bsd_user_strict;
 
 /*
  * TARGET_ARG_MAX defines the number of bytes allocated for arguments
-- 
2.39.1

Re: [PATCH 1/9] bsd-user: Don't truncate the return value from freebsd_syscall

[Richard Henderson]

On 2/10/23 13:18, Warner Losh wrote:
> From: Doug Rabson <dfr@rabson.org>
> 
> System call return values on FreeBSD are in a register (which is spelled
> api_long in qemu). This was being assigned into an int variable which

s/api/abi/

> causes problems for 64bit targets.
> 
> Resolves: https://github.com/qemu-bsd-user/qemu-bsd-user/issues/40
> Signed-off-by: Doug Rabson <dfr@rabson.org>
> Reviewed-by: Warner Losh <imp@bsdimp.com>
> [ Edited commit message for upstreaming into qemu-project ]
> Signed-off-by: Warner Losh <imp@bsdimp.com>
> ---
>   bsd-user/freebsd/os-syscall.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

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

r~

> 
> diff --git a/bsd-user/freebsd/os-syscall.c b/bsd-user/freebsd/os-syscall.c
> index 57996cad8ae..b4a663fc021 100644
> --- a/bsd-user/freebsd/os-syscall.c
> +++ b/bsd-user/freebsd/os-syscall.c
> @@ -512,7 +512,7 @@ abi_long do_freebsd_syscall(void *cpu_env, int num, abi_long arg1,
>                               abi_long arg8)
>   {
>       CPUState *cpu = env_cpu(cpu_env);
> -    int ret;
> +    abi_long ret;
>   
>       trace_guest_user_syscall(cpu, num, arg1, arg2, arg3, arg4, arg5, arg6, arg7, arg8);
>       if (do_strace) {

Re: [PATCH 3/9] bsd-user: Add sysarch syscall

[Richard Henderson]

On 2/10/23 13:18, Warner Losh wrote:
> From: Stacey Son<sson@FreeBSD.org>
> 
> Connect up the sysarch system call.
> 
> Signed-off-by: Juergen Lock<nox@jelal.kn-bremen.de>
> Co-authored-by: Juergen Lock<nox@jelal.kn-bremen.de>
> Signed-off-by: Stacey Son<sson@FreeBSD.org>
> Reviewed-by: Warner Losh<imp@bsdimp.com>
> Signed-off-by: Warner Losh<imp@bsdimp.com>
> ---
>   bsd-user/freebsd/os-syscall.c | 7 +++++++
>   1 file changed, 7 insertions(+)

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

r~

Re: [PATCH 0/9] 2023 Q1 bsd-user upstreaming: bugfixes and sysctl

[Richard Henderson]

On 2/10/23 13:18, Warner Losh wrote:
> There's
> several static functions that aren't used until the end of the patch
> series... Not sure the best way to suppress the build warnings there (but since
> they are just warnings...).

Are they just warnings?  --enable-werror is default...

Anyway, I've used

static type G_GNUC_UNUSED function(args...)

in the past to ensure bisection, removing the UNUSED marker when they become used.


r~

Re: [PATCH 4/9] bsd-user: Two helper routines oidfmt and sysctl_oldcvt

[Richard Henderson]

On 2/10/23 13:18, Warner Losh wrote:
> +static int sysctl_oldcvt(void *holdp, size_t *holdlen, uint32_t kind)
> +{
> +    switch (kind & CTLTYPE) {
> +    case CTLTYPE_INT:
> +    case CTLTYPE_UINT:
> +        *(uint32_t *)holdp = tswap32(*(uint32_t *)holdp);
> +        break;
> +
> +#ifdef TARGET_ABI32
> +    case CTLTYPE_LONG:
> +    case CTLTYPE_ULONG:
> +        /*
> +         * If the sysctl has a type of long/ulong but seems to be bigger than
> +         * these data types, its probably an array.  Double check that its
> +         * evenly divisible by the size of long and convert holdp to a series of
> +         * 32bit elements instead, adjusting holdlen to the new size.
> +         */
> +        if ((*holdlen > sizeof(abi_ulong)) &&
> +            ((*holdlen % sizeof(abi_ulong)) == 0)) {
> +            int array_size = *holdlen / sizeof(long);
> +            int i;
> +            if (holdp) {
> +                for (i = 0; i < array_size; i++) {
> +                    ((uint32_t *)holdp)[i] = tswap32(((long *)holdp)[i]);
> +                }
> +                *holdlen = array_size * sizeof(abi_ulong);
> +            } else {
> +                *holdlen = sizeof(abi_ulong);
> +            }
> +        } else {
> +            *(uint32_t *)holdp = tswap32(*(long *)holdp);
> +            *holdlen = sizeof(uint32_t);

This is totally confusing.  Why would it ever be an array?
Why is this section the only place we ever assign back into holdlen?

Can you point to anything similar in the freebsd source?  The whole thing is pretty hard 
to track, starting from sys/kern/kern_sysctl.c.


r~

Re: [PATCH 0/9] 2023 Q1 bsd-user upstreaming: bugfixes and sysctl

[Warner Losh]

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

On Sat, Feb 11, 2023 at 12:30 PM Richard Henderson <
richard.henderson@linaro.org> wrote:

> On 2/10/23 13:18, Warner Losh wrote:
> > There's
> > several static functions that aren't used until the end of the patch
> > series... Not sure the best way to suppress the build warnings there
> (but since
> > they are just warnings...).
>
> Are they just warnings?  --enable-werror is default...
>
> Anyway, I've used
>
> static type G_GNUC_UNUSED function(args...)
>
> in the past to ensure bisection, removing the UNUSED marker when they
> become used.
>

I like that suggestion, and it's easy to implement. Thanks for this and the
reviews so far. I've
updated things with your comments, but will give it another day or two
before I send v2 for
others to comment.

Warner

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

Re: [PATCH 6/9] bsd-user: common routine do_freebsd_sysctl_oid for all sysctl variants

[Richard Henderson]

On 2/10/23 13:18, Warner Losh wrote:
> +    /* Handle some arch/emulator dependent sysctl()'s here. */
> +    switch (snamep[0]) {
> +#if defined(TARGET_PPC) || defined(TARGET_PPC64)
> +    case CTL_MACHDEP:
> +        switch (snamep[1]) {
> +        case 1:    /* CPU_CACHELINE */
> +            holdlen = sizeof(uint32_t);
> +            (*(uint32_t *)holdp) = tswap32(env->dcache_line_size);
> +            ret = 0;
> +            goto out;
> +        }
> +        break;
> +#endif

abi_int instead of uint32_t.


> +    case CTL_HW:
> +        switch (snamep[1]) {
> +        case HW_MACHINE:
> +            holdlen = sizeof(TARGET_HW_MACHINE);
> +            if (holdp) {
> +                strlcpy(holdp, TARGET_HW_MACHINE, oldlen);
> +            }

What's the semantics here when oldlen < sizeof(literal)?
I was expecting something like sysctl_old_kernel.
It would probably be good to create a number of small helper functions per type.

> +#ifdef ARM_FEATURE_VFP /* XXX FIXME XXX */

This define has been removed, so this part is dead,

> +                if (env->features & ((1ULL << ARM_FEATURE_VFP)|
> +                                     (1ULL << ARM_FEATURE_VFP3)|
> +                                     (1ULL << ARM_FEATURE_VFP4)))
> +                    *(int32_t *)holdp = 1;
> +                else
> +                    *(int32_t *)holdp = 0;
> +#else
> +                *(int32_t *)holdp = 1;

and this is not right.

You're looking for

     ARMCPU *cpu = env_archcpu(env);
     *(abi_int *)holdp = cpu_isar_feature(aa32_vfp, cpu);

> +#if TARGET_ABI_BITS != HOST_LONG_BITS
> +        case HW_PHYSMEM:
> +        case HW_USERMEM:
> +        case HW_REALMEM:
> +            holdlen = sizeof(abi_ulong);
> +            ret = 0;
> +
> +            if (oldlen) {
> +                int mib[2] = {snamep[0], snamep[1]};
> +                unsigned long lvalue;
> +                size_t len = sizeof(lvalue);
> +
> +                if (sysctl(mib, 2, &lvalue, &len, NULL, 0) == -1) {
> +                    ret = -1;
> +                } else {
> +                    if (((unsigned long)maxmem) < lvalue) {


Where is maxmem defined?
Why are these numbers only special-cased for TARGET_ABI_BITS != HOST_LONG_BITS?

> +            static int oid_hw_pagesizes;
> +
> +            if (!oid_hw_availpages) {
> +                int real_oid[CTL_MAXNAME + 2];
> +                size_t len = sizeof(real_oid) / sizeof(int);
> +
> +                if (sysctlnametomib("hw.availpages", real_oid, &len) >= 0) {
> +                    oid_hw_availpages = real_oid[1];
> +                }
> +            }
> +            if (!oid_hw_pagesizes) {
> +                int real_oid[CTL_MAXNAME + 2];
> +                size_t len = sizeof(real_oid) / sizeof(int);
> +
> +                if (sysctlnametomib("hw.pagesizes", real_oid, &len) >= 0) {
> +                    oid_hw_pagesizes = real_oid[1];
> +                }
> +            }

Host pagesizes are not relevant to the guest.

> +
> +            if (oid_hw_availpages && snamep[1] == oid_hw_availpages) {
> +                long lvalue;
> +                size_t len = sizeof(lvalue);
> +
> +                if (sysctlbyname("hw.availpages", &lvalue, &len, NULL, 0) == -1) {
> +                    ret = -1;
> +                } else {
> +                    if (oldlen) {
> +#if TARGET_ABI_BITS != HOST_LONG_BITS
> +                        abi_ulong maxpages = maxmem / (abi_ulong)getpagesize();

Again with maxmem...

> +                        if (((unsigned long)maxpages) < lvalue) {
> +                            lvalue = maxpages;
> +                        }
> +#endif
> +                        (*(abi_ulong *)holdp) = tswapal((abi_ulong)lvalue);

I would expect a 64-bit guest to rescale the result for TARGET_PAGE_SIZE != getpagesize().

> +                    }
> +                    holdlen = sizeof(abi_ulong);
> +                    ret = 0;
> +                }
> +                goto out;
> +            }
> +
> +            if (oid_hw_pagesizes && snamep[1] == oid_hw_pagesizes) {
> +                if (oldlen) {
> +                    (*(abi_ulong *)holdp) = tswapal((abi_ulong)getpagesize());

Indeed, this needs TARGET_PAGE_SIZE.

> diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h
> index 0ceecfb6dfa..e24a8cfcfb1 100644
> --- a/bsd-user/qemu.h
> +++ b/bsd-user/qemu.h
> @@ -252,6 +252,11 @@ bool is_error(abi_long ret);
>   int host_to_target_errno(int err);
>   
>   /* os-sys.c */
> +abi_long do_freebsd_sysctl(CPUArchState *env, abi_ulong namep, int32_t namelen,
> +        abi_ulong oldp, abi_ulong oldlenp, abi_ulong newp, abi_ulong newlen);
> +abi_long do_freebsd_sysctlbyname(CPUArchState *env, abi_ulong namep,
> +        int32_t namelen, abi_ulong oldp, abi_ulong oldlenp, abi_ulong newp,
> +        abi_ulong newlen);

These belong to different patches.


r~

Re: [PATCH 7/9] bsd-user: do_freebsd_sysctl helper for sysctl(2)

[Richard Henderson]

On 2/10/23 13:18, Warner Losh wrote:
> +abi_long do_freebsd_sysctl(CPUArchState *env, abi_ulong namep, int32_t namelen,
> +        abi_ulong oldp, abi_ulong oldlenp, abi_ulong newp, abi_ulong newlen)
> +{
> +    abi_long ret;
> +    void *hnamep, *holdp = NULL, *hnewp = NULL;
> +    size_t holdlen;
> +    abi_ulong oldlen = 0;
> +    int32_t *snamep = g_malloc(sizeof(int32_t) * namelen), *p, *q, i;
> +
> +    if (oldlenp) {
> +        if (get_user_ual(oldlen, oldlenp)) {
> +            return -TARGET_EFAULT;
> +        }
> +    }

You need to check for write early.  Either access_ok, or lock_user.

> +    for (p = hnamep, q = snamep, i = 0; i < namelen; p++, i++) {
> +        *q++ = tswap32(*p);
> +    }

Why the inconsistent increments?

> +    unlock_user(holdp, oldp, holdlen);

Usually we don't want writeback on error.


r~

Re: [PATCH 8/9] bsd-user: implement sysctlbyname(2)

[Richard Henderson]

On 2/10/23 13:18, Warner Losh wrote:
> From: Kyle Evans <kevans@FreeBSD.org>
> 
> do_freebsd_sysctlbyname needs to translate the 'name' back down to a OID
> so we can intercept the special ones. Do that and call the common wrapper
> do_freebsd_sysctl_oid.
> 
> Signed-off-by: Kyle Evans <kevans@FreeBSD.org>
> Signed-off-by: Warner Losh <imp@bsdimp.com>
> ---
>   bsd-user/freebsd/os-sys.c     | 58 +++++++++++++++++++++++++++++++++++
>   bsd-user/freebsd/os-syscall.c |  4 +++
>   2 files changed, 62 insertions(+)
> 
> diff --git a/bsd-user/freebsd/os-sys.c b/bsd-user/freebsd/os-sys.c
> index 13736936e5f..62c729dfe47 100644
> --- a/bsd-user/freebsd/os-sys.c
> +++ b/bsd-user/freebsd/os-sys.c
> @@ -345,6 +345,64 @@ out:
>       return ret;
>   }
>   
> +/*
> + * This syscall was created to make sysctlbyname(3) more efficient.
> + * Unfortunately, because we have to fake some sysctls, we can't do that.

Can't do what?  Directly use sysctlbyname?

> +    if (oldlenp) {
> +        if (get_user_ual(oldlen, oldlenp)) {
> +            return -TARGET_EFAULT;
> +        }

Same comment about verifying write early.

> +    unlock_user(holdp, oldp, holdlen);

And writeback vs error.


r~

Re: [PATCH 9/9] bsd-user: Add -strict

[Richard Henderson]

On 2/10/23 13:18, Warner Losh wrote:
> Most of the time, it's useful to make our best effort, but sometimes we
> want to know right away when we don't implement something. First place
> we use it is for unknown syscalls.
> 
> Signed-off-by: Warner Losh <imp@bsdimp.com>
> ---
>   bsd-user/freebsd/os-syscall.c | 4 ++++
>   bsd-user/main.c               | 5 ++++-
>   bsd-user/qemu.h               | 1 +
>   3 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/bsd-user/freebsd/os-syscall.c b/bsd-user/freebsd/os-syscall.c
> index 179a20c304b..e2b26ecb8dd 100644
> --- a/bsd-user/freebsd/os-syscall.c
> +++ b/bsd-user/freebsd/os-syscall.c
> @@ -508,6 +508,10 @@ static abi_long freebsd_syscall(void *cpu_env, int num, abi_long arg1,
>   
>       default:
>           qemu_log_mask(LOG_UNIMP, "Unsupported syscall: %d\n", num);
> +        if (bsd_user_strict) {
> +            printf("Unimplemented system call %d\n", num);
> +            abort();
> +        }

I don't like the raw printf, even if you did write to stderr.
Perhaps just the abort, letting the error message be handled by qemu_log?

> @@ -396,6 +397,8 @@ int main(int argc, char **argv)
>               trace_opt_parse(optarg);
>           } else if (!strcmp(r, "0")) {
>               argv0 = argv[optind++];
> +        } else if (!strcmp(r, "strict")) {
> +            bsd_user_strict = true;

Perhaps force LOG_UNIMP?  Without -D, you'll get the qemu_log above to stderr.


r~

Re: [PATCH 6/9] bsd-user: common routine do_freebsd_sysctl_oid for all sysctl variants

[Warner Losh]

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

On Sat, Feb 11, 2023 at 3:56 PM Richard Henderson <
richard.henderson@linaro.org> wrote:

> On 2/10/23 13:18, Warner Losh wrote:
> > +    /* Handle some arch/emulator dependent sysctl()'s here. */
> > +    switch (snamep[0]) {
> > +#if defined(TARGET_PPC) || defined(TARGET_PPC64)
> > +    case CTL_MACHDEP:
> > +        switch (snamep[1]) {
> > +        case 1:    /* CPU_CACHELINE */
> > +            holdlen = sizeof(uint32_t);
> > +            (*(uint32_t *)holdp) = tswap32(env->dcache_line_size);
> > +            ret = 0;
> > +            goto out;
> > +        }
> > +        break;
> > +#endif
>
> abi_int instead of uint32_t.
>

Indeed. Thanks! Turns out, though, there's no upstream support for PPC for
bsd-user, so I'll drop this hunk of the patch... I thought I'd done it
already when
preparing things...


> > +    case CTL_HW:
> > +        switch (snamep[1]) {
> > +        case HW_MACHINE:
> > +            holdlen = sizeof(TARGET_HW_MACHINE);
> > +            if (holdp) {
> > +                strlcpy(holdp, TARGET_HW_MACHINE, oldlen);
> > +            }
>
> What's the semantics here when oldlen < sizeof(literal)?
> I was expecting something like sysctl_old_kernel.
> It would probably be good to create a number of small helper functions per
> type.
>
> > +#ifdef ARM_FEATURE_VFP /* XXX FIXME XXX */
>
> This define has been removed, so this part is dead,
>

Yup. I added it as a hack... I kept this in because I knew I'd find the
right way to
do this :)


> > +                if (env->features & ((1ULL << ARM_FEATURE_VFP)|
> > +                                     (1ULL << ARM_FEATURE_VFP3)|
> > +                                     (1ULL << ARM_FEATURE_VFP4)))
> > +                    *(int32_t *)holdp = 1;
> > +                else
> > +                    *(int32_t *)holdp = 0;
> > +#else
> > +                *(int32_t *)holdp = 1;
>
> and this is not right.
>
> You're looking for
>
>      ARMCPU *cpu = env_archcpu(env);
>      *(abi_int *)holdp = cpu_isar_feature(aa32_vfp, cpu);
>

Yes. That looks right to me... I was having trouble finding it and the
merge it came
in on was bigger than normal, and I put the above kludge in to get through
it and
then never followed up...


> > +#if TARGET_ABI_BITS != HOST_LONG_BITS
> > +        case HW_PHYSMEM:
> > +        case HW_USERMEM:
> > +        case HW_REALMEM:
> > +            holdlen = sizeof(abi_ulong);
> > +            ret = 0;
> > +
> > +            if (oldlen) {
> > +                int mib[2] = {snamep[0], snamep[1]};
> > +                unsigned long lvalue;
> > +                size_t len = sizeof(lvalue);
> > +
> > +                if (sysctl(mib, 2, &lvalue, &len, NULL, 0) == -1) {
> > +                    ret = -1;
> > +                } else {
> > +                    if (((unsigned long)maxmem) < lvalue) {
>
>
> Where is maxmem defined?
> Why are these numbers only special-cased for TARGET_ABI_BITS !=
> HOST_LONG_BITS?
>

maxmem is defined earlier in this patch:

+#if TARGET_ABI_BITS != HOST_LONG_BITS
+    const abi_ulong maxmem = -0x100c000;

but I'm not at all sure how that number was arrived at...
It's a little less than ULONG_MAX is all I can say for
sure.

As to why it's a special case only sometimes, I believe that it's there for
32-bit
targets running on 64-bit hosts so that we return a sane amount of memory
because
64-bit hosts can have > 4GB of ram... I'm not 100% sure of this, and it
would
likely be wrong for 32-bit host and 64-bit target, but that case isn't
supported at
all by the bsd-user project (though in the past it may have been, we no
longer
built even 32 on 32 target/host emulation).


> > +            static int oid_hw_pagesizes;
> > +
> > +            if (!oid_hw_availpages) {
> > +                int real_oid[CTL_MAXNAME + 2];
> > +                size_t len = sizeof(real_oid) / sizeof(int);
> > +
> > +                if (sysctlnametomib("hw.availpages", real_oid, &len) >=
> 0) {
> > +                    oid_hw_availpages = real_oid[1];
> > +                }
> > +            }
> > +            if (!oid_hw_pagesizes) {
> > +                int real_oid[CTL_MAXNAME + 2];
> > +                size_t len = sizeof(real_oid) / sizeof(int);
> > +
> > +                if (sysctlnametomib("hw.pagesizes", real_oid, &len) >=
> 0) {
> > +                    oid_hw_pagesizes = real_oid[1];
> > +                }
> > +            }
>
> Host pagesizes are not relevant to the guest.
>

Yes. I noticed after I submitted this that I wondered if I should be using
the
host's notion, or the softmmu's notion of page size... But it's clear from
the
other comments below, that it should be TARGET_PAGE_SIZE for all of
these.

> +
> > +            if (oid_hw_availpages && snamep[1] == oid_hw_availpages) {
> > +                long lvalue;
> > +                size_t len = sizeof(lvalue);
> > +
> > +                if (sysctlbyname("hw.availpages", &lvalue, &len, NULL,
> 0) == -1) {
> > +                    ret = -1;
> > +                } else {
> > +                    if (oldlen) {
> > +#if TARGET_ABI_BITS != HOST_LONG_BITS
> > +                        abi_ulong maxpages = maxmem /
> (abi_ulong)getpagesize();
>
> Again with maxmem...
>
> > +                        if (((unsigned long)maxpages) < lvalue) {
> > +                            lvalue = maxpages;
> > +                        }
> > +#endif
> > +                        (*(abi_ulong *)holdp) =
> tswapal((abi_ulong)lvalue);
>
> I would expect a 64-bit guest to rescale the result for TARGET_PAGE_SIZE
> != getpagesize().
>

I would too. I suspect that the reason this is here like this is that an
attempt
was being made to handle it, but since TARGET_PAGE_SIZE == getpagesize() on
all hosts / target pairs until very recently (with the 16k arm64 kernels),
this was
a latent bug in the code and I should fix it before my next submission. And
aarch64
hosts for this are quite rare (most people use bsd-user on amd64 hosts to
build for
all the other architectures).


> > +                    }
> > +                    holdlen = sizeof(abi_ulong);
> > +                    ret = 0;
> > +                }
> > +                goto out;
> > +            }
> > +
> > +            if (oid_hw_pagesizes && snamep[1] == oid_hw_pagesizes) {
> > +                if (oldlen) {
> > +                    (*(abi_ulong *)holdp) =
> tswapal((abi_ulong)getpagesize());
>
> Indeed, this needs TARGET_PAGE_SIZE.
>

That makes things somewhat simpler for rearranging here...


> > diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h
> > index 0ceecfb6dfa..e24a8cfcfb1 100644
> > --- a/bsd-user/qemu.h
> > +++ b/bsd-user/qemu.h
> > @@ -252,6 +252,11 @@ bool is_error(abi_long ret);
> >   int host_to_target_errno(int err);
> >
> >   /* os-sys.c */
> > +abi_long do_freebsd_sysctl(CPUArchState *env, abi_ulong namep, int32_t
> namelen,
> > +        abi_ulong oldp, abi_ulong oldlenp, abi_ulong newp, abi_ulong
> newlen);
> > +abi_long do_freebsd_sysctlbyname(CPUArchState *env, abi_ulong namep,
> > +        int32_t namelen, abi_ulong oldp, abi_ulong oldlenp, abi_ulong
> newp,
> > +        abi_ulong newlen);
>
> These belong to different patches.
>

Oh yes. I'll take care of that... They were, but then they weren't and then
i thought I'd
fixed that (a bit of a rebase misadventure when re-ordering patches
occurred and
I thought I'd fixed it entirely...)

Thanks for helping me clear a few things up in the code that my
understanding was
hazy, but I wasn't sure where it was hazy and it turns out these comments
clear the haze
for me.

Warner


> r~
>
>

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

Re: [PATCH 6/9] bsd-user: common routine do_freebsd_sysctl_oid for all sysctl variants

[Richard Henderson]

On 2/11/23 13:40, Warner Losh wrote:
> maxmem is defined earlier in this patch:
> 
> +#if TARGET_ABI_BITS != HOST_LONG_BITS
> +    const abi_ulong maxmem = -0x100c000;
> 
> but I'm not at all sure how that number was arrived at...
> It's a little less than ULONG_MAX is all I can say for
> sure.
> 
> As to why it's a special case only sometimes, I believe that it's there for 32-bit
> targets running on 64-bit hosts so that we return a sane amount of memory because
> 64-bit hosts can have > 4GB of ram... I'm not 100% sure of this, and it would
> likely be wrong for 32-bit host and 64-bit target, but that case isn't supported at
> all by the bsd-user project (though in the past it may have been, we no longer
> built even 32 on 32 target/host emulation).

Perhaps you're looking for reserved_va?  I.e. the max va the guest is limited to?

Or, given this is a system-wide number of pages, not per-process, and given the types 
involved, cap at UINT32_MAX?

>     I would expect a 64-bit guest to rescale the result for TARGET_PAGE_SIZE != getpagesize().
> 
> 
> I would too. I suspect that the reason this is here like this is that an attempt
> was being made to handle it, but since TARGET_PAGE_SIZE == getpagesize() on
> all hosts / target pairs until very recently (with the 16k arm64 kernels), this was
> a latent bug in the code and I should fix it before my next submission. And aarch64
> hosts for this are quite rare (most people use bsd-user on amd64 hosts to build for
> all the other architectures).

Ok.  When you do this, remember muldiv64.


r~

Re: [PATCH 6/9] bsd-user: common routine do_freebsd_sysctl_oid for all sysctl variants

[Warner Losh]

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

On Sat, Feb 11, 2023 at 4:59 PM Richard Henderson <
richard.henderson@linaro.org> wrote:

> On 2/11/23 13:40, Warner Losh wrote:
> > maxmem is defined earlier in this patch:
> >
> > +#if TARGET_ABI_BITS != HOST_LONG_BITS
> > +    const abi_ulong maxmem = -0x100c000;
> >
> > but I'm not at all sure how that number was arrived at...
> > It's a little less than ULONG_MAX is all I can say for
> > sure.
> >
> > As to why it's a special case only sometimes, I believe that it's there
> for 32-bit
> > targets running on 64-bit hosts so that we return a sane amount of
> memory because
> > 64-bit hosts can have > 4GB of ram... I'm not 100% sure of this, and it
> would
> > likely be wrong for 32-bit host and 64-bit target, but that case isn't
> supported at
> > all by the bsd-user project (though in the past it may have been, we no
> longer
> > built even 32 on 32 target/host emulation).
>
> Perhaps you're looking for reserved_va?  I.e. the max va the guest is
> limited to?
>
> Or, given this is a system-wide number of pages, not per-process, and
> given the types
> involved, cap at UINT32_MAX?
>

I think that I'll use UINT32_MAX - <magic number> + 1 here. I'll explain
that <magic number>
was empirically determined. I'm looking at all repos to see if there's a
better explanation there.


> >     I would expect a 64-bit guest to rescale the result for
> TARGET_PAGE_SIZE != getpagesize().
> >
> >
> > I would too. I suspect that the reason this is here like this is that an
> attempt
> > was being made to handle it, but since TARGET_PAGE_SIZE == getpagesize()
> on
> > all hosts / target pairs until very recently (with the 16k arm64
> kernels), this was
> > a latent bug in the code and I should fix it before my next submission.
> And aarch64
> > hosts for this are quite rare (most people use bsd-user on amd64 hosts
> to build for
> > all the other architectures).
>
> Ok.  When you do this, remember muldiv64.
>

I was going to do something like:

+    if (host_page_size != TARGET_PAGE_SIZE) {
+        if (host_page_size > TARGET_PAGE_SIZE) {
+            /* Scale up */
+            pages *= host_page_size / TARGET_PAGE_SIZE;
+        } else {
+            /* Scale down with truncation */
+            pages /= TARGET_PAGE_SIZE / host_page_size;
+        }
+    }

in a helper function. Does multdiv64 replace that? It's currently unused in
both linux-user
and bsd-user. The above does things in a known-good order (or at least
that's my belief,
even after 30 years C surprises me).

Warner

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

Re: [PATCH 6/9] bsd-user: common routine do_freebsd_sysctl_oid for all sysctl variants

[Richard Henderson]

On 2/11/23 14:40, Warner Losh wrote:
> I was going to do something like:
> 
> +    if (host_page_size != TARGET_PAGE_SIZE) {
> +        if (host_page_size > TARGET_PAGE_SIZE) {
> +            /* Scale up */
> +            pages *= host_page_size / TARGET_PAGE_SIZE;
> +        } else {
> +            /* Scale down with truncation */
> +            pages /= TARGET_PAGE_SIZE / host_page_size;
> +        }
> +    }
> in a helper function. Does multdiv64 replace that?

Yes, it uses a 128-bit intermediate result, so no overflow.
Obviously the above works as well, but perhaps premature optimization.


r~

Re: [PATCH 4/9] bsd-user: Two helper routines oidfmt and sysctl_oldcvt

[Warner Losh]

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

On Sat, Feb 11, 2023 at 3:17 PM Richard Henderson <
richard.henderson@linaro.org> wrote:

> On 2/10/23 13:18, Warner Losh wrote:
> > +static int sysctl_oldcvt(void *holdp, size_t *holdlen, uint32_t kind)
> > +{
> > +    switch (kind & CTLTYPE) {
> > +    case CTLTYPE_INT:
> > +    case CTLTYPE_UINT:
> > +        *(uint32_t *)holdp = tswap32(*(uint32_t *)holdp);
> > +        break;
> > +
> > +#ifdef TARGET_ABI32
> > +    case CTLTYPE_LONG:
> > +    case CTLTYPE_ULONG:
> > +        /*
> > +         * If the sysctl has a type of long/ulong but seems to be
> bigger than
> > +         * these data types, its probably an array.  Double check that
> its
> > +         * evenly divisible by the size of long and convert holdp to a
> series of
> > +         * 32bit elements instead, adjusting holdlen to the new size.
> > +         */
> > +        if ((*holdlen > sizeof(abi_ulong)) &&
> > +            ((*holdlen % sizeof(abi_ulong)) == 0)) {
> > +            int array_size = *holdlen / sizeof(long);
> > +            int i;
> > +            if (holdp) {
> > +                for (i = 0; i < array_size; i++) {
> > +                    ((uint32_t *)holdp)[i] = tswap32(((long
> *)holdp)[i]);
> > +                }
> > +                *holdlen = array_size * sizeof(abi_ulong);
> > +            } else {
> > +                *holdlen = sizeof(abi_ulong);
> > +            }
> > +        } else {
> > +            *(uint32_t *)holdp = tswap32(*(long *)holdp);
> > +            *holdlen = sizeof(uint32_t);
>
> This is totally confusing.  Why would it ever be an array?
> Why is this section the only place we ever assign back into holdlen?
>
> Can you point to anything similar in the freebsd source?  The whole thing
> is pretty hard
> to track, starting from sys/kern/kern_sysctl.c.
>

I need to understand this... I've been looking for where we export an
array, and we just don't.

I've asked the original author who said it had something to do with
different size longs. I'll
look into that a bit and get back to this.

I think we assign back into holdlen in a weird attempt adjust for the
difference of LONG between
the two. But I'm not sure that that's where we should assign.

Warner

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

Re: [PATCH 8/9] bsd-user: implement sysctlbyname(2)

[Kyle Evans]

On Sat, Feb 11, 2023 at 5:13 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 2/10/23 13:18, Warner Losh wrote:
> > From: Kyle Evans <kevans@FreeBSD.org>
> >
> > do_freebsd_sysctlbyname needs to translate the 'name' back down to a OID
> > so we can intercept the special ones. Do that and call the common wrapper
> > do_freebsd_sysctl_oid.
> >
> > Signed-off-by: Kyle Evans <kevans@FreeBSD.org>
> > Signed-off-by: Warner Losh <imp@bsdimp.com>
> > ---
> >   bsd-user/freebsd/os-sys.c     | 58 +++++++++++++++++++++++++++++++++++
> >   bsd-user/freebsd/os-syscall.c |  4 +++
> >   2 files changed, 62 insertions(+)
> >
> > diff --git a/bsd-user/freebsd/os-sys.c b/bsd-user/freebsd/os-sys.c
> > index 13736936e5f..62c729dfe47 100644
> > --- a/bsd-user/freebsd/os-sys.c
> > +++ b/bsd-user/freebsd/os-sys.c
> > @@ -345,6 +345,64 @@ out:
> >       return ret;
> >   }
> >
> > +/*
> > + * This syscall was created to make sysctlbyname(3) more efficient.
> > + * Unfortunately, because we have to fake some sysctls, we can't do that.
>
> Can't do what?  Directly use sysctlbyname?
>

How about:

/*
 * This syscall was created to make sysctlbyname(3) more efficient, but
 * we can't really provide it in bsd-user.  Notably, we must always translate
 * the names independently since some sysctl values have to be faked
 * for the target environment, so it still has to break down to two syscalls
 * for the underlying implementation.
 */

> > +    if (oldlenp) {
> > +        if (get_user_ual(oldlen, oldlenp)) {
> > +            return -TARGET_EFAULT;
> > +        }
>
> Same comment about verifying write early.
>
> > +    unlock_user(holdp, oldp, holdlen);
>
> And writeback vs error.
>
>
> r~

Re: [PATCH 8/9] bsd-user: implement sysctlbyname(2)

[Richard Henderson]

On 2/11/23 18:23, Kyle Evans wrote:
>>> +/*
>>> + * This syscall was created to make sysctlbyname(3) more efficient.
>>> + * Unfortunately, because we have to fake some sysctls, we can't do that.
>>
>> Can't do what?  Directly use sysctlbyname?
>>
> 
> How about:
> 
> /*
>   * This syscall was created to make sysctlbyname(3) more efficient, but
>   * we can't really provide it in bsd-user.  Notably, we must always translate
>   * the names independently since some sysctl values have to be faked
>   * for the target environment, so it still has to break down to two syscalls
>   * for the underlying implementation.
>   */

Better, thanks.


r~

Re: [PATCH 4/9] bsd-user: Two helper routines oidfmt and sysctl_oldcvt

[Warner Losh]

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

Hey Richard

Thanks for the very interesting question... This kept me up...

Kyle,

Please double check what I've written below to make sure I've not missed
anything.
This might well be the source of some of the weird errors we're seeing on
some
ports, but sysctl is rare enough I'm guessing that any of the overflows are
in the
end benign.

On Sat, Feb 11, 2023 at 9:11 PM Warner Losh <imp@bsdimp.com> wrote:

>
>
> On Sat, Feb 11, 2023 at 3:17 PM Richard Henderson <
> richard.henderson@linaro.org> wrote:
>
>> On 2/10/23 13:18, Warner Losh wrote:
>> > +static int sysctl_oldcvt(void *holdp, size_t *holdlen, uint32_t kind)
>> > +{
>> > +    switch (kind & CTLTYPE) {
>> > +    case CTLTYPE_INT:
>> > +    case CTLTYPE_UINT:
>> > +        *(uint32_t *)holdp = tswap32(*(uint32_t *)holdp);
>> > +        break;
>> > +
>> > +#ifdef TARGET_ABI32
>> > +    case CTLTYPE_LONG:
>> > +    case CTLTYPE_ULONG:
>> > +        /*
>> > +         * If the sysctl has a type of long/ulong but seems to be
>> bigger than
>> > +         * these data types, its probably an array.  Double check that
>> its
>> > +         * evenly divisible by the size of long and convert holdp to a
>> series of
>> > +         * 32bit elements instead, adjusting holdlen to the new size.
>> > +         */
>> > +        if ((*holdlen > sizeof(abi_ulong)) &&
>> > +            ((*holdlen % sizeof(abi_ulong)) == 0)) {
>> > +            int array_size = *holdlen / sizeof(long);
>> > +            int i;
>> > +            if (holdp) {
>> > +                for (i = 0; i < array_size; i++) {
>> > +                    ((uint32_t *)holdp)[i] = tswap32(((long
>> *)holdp)[i]);
>> > +                }
>> > +                *holdlen = array_size * sizeof(abi_ulong);
>> > +            } else {
>> > +                *holdlen = sizeof(abi_ulong);
>> > +            }
>> > +        } else {
>> > +            *(uint32_t *)holdp = tswap32(*(long *)holdp);
>> > +            *holdlen = sizeof(uint32_t);
>>
>> This is totally confusing.  Why would it ever be an array?
>> Why is this section the only place we ever assign back into holdlen?
>>
>> Can you point to anything similar in the freebsd source?  The whole thing
>> is pretty hard
>> to track, starting from sys/kern/kern_sysctl.c.
>>
>
> I need to understand this... I've been looking for where we export an
> array, and we just don't.
>
> I've asked the original author who said it had something to do with
> different size longs. I'll
> look into that a bit and get back to this.
>
> I think we assign back into holdlen in a weird attempt adjust for the
> difference of LONG between
> the two. But I'm not sure that that's where we should assign.
>

OK. I understand what's going on. If you look at kern_sysctl.c
sysctl_old_ddb or
sbin/sysctl/sysctl.c show_var, you'll see that these values canbe arrays.
This code
only implements the array part for long and ulong, most likely because
that's
all that was encountered in the field.

 So the code is right, as far as it goes.... But if the value is bigger
than a long, it
will be truncated, which strikes me as a rather weird thing to do since
most longs
are for sizes of things, so I'd think it would be better to saturate.

We also adjust the length here because the host's memory requirements
are larger than tha targets. This also means that we're likely returning an
error for long/ulong fetches since the target would pass in 4 and the host
would want 8, and would return ENOMEM. There's no code to cope with
this at all, but I think there needs to be a temporary host buffer that's
then copied to the target buffer once it's converted. So I need to write
that code.

Also, this code doesn't handle the newer types that FreeBSD has grown
in the last few years: _{S,U}{8,16,32,64}. At least those are fixed between
the two different ABIs that freebsd supports (ILP32 and LP64).

Also, there's a size issue. *holdlen is a size_t, so we need to do a similar
brokering for ABI32 targets. The interface is such that we need to
read/write
this variable because that's what the kernel is doing (reading it to make
sure
it's big enough, and then writing it to the actual size).

Also (not relevant to this patch), we must not set sysctls very often. newp
needs similar treatment tooldp (except the reverse direction), but isn't
getting any of the tswaptreatment, so it's broken for long/ulong types as
well
as on powerpc which we have out-of-tree now and is the only big-endian
port we have left.

tl;dr: I think I'm going to have to do a bit of a rewrite here...

Warner

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

Re: [PATCH 4/9] bsd-user: Two helper routines oidfmt and sysctl_oldcvt

[Warner Losh]

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

On Sun, Feb 12, 2023 at 10:01 AM Warner Losh <imp@bsdimp.com> wrote:

> Hey Richard
>
> Thanks for the very interesting question... This kept me up...
>
> Kyle,
>
> Please double check what I've written below to make sure I've not missed
> anything.
> This might well be the source of some of the weird errors we're seeing on
> some
> ports, but sysctl is rare enough I'm guessing that any of the overflows
> are in the
> end benign.
>
> On Sat, Feb 11, 2023 at 9:11 PM Warner Losh <imp@bsdimp.com> wrote:
>
>>
>>
>> On Sat, Feb 11, 2023 at 3:17 PM Richard Henderson <
>> richard.henderson@linaro.org> wrote:
>>
>>> On 2/10/23 13:18, Warner Losh wrote:
>>> > +static int sysctl_oldcvt(void *holdp, size_t *holdlen, uint32_t kind)
>>> > +{
>>> > +    switch (kind & CTLTYPE) {
>>> > +    case CTLTYPE_INT:
>>> > +    case CTLTYPE_UINT:
>>> > +        *(uint32_t *)holdp = tswap32(*(uint32_t *)holdp);
>>> > +        break;
>>> > +
>>> > +#ifdef TARGET_ABI32
>>> > +    case CTLTYPE_LONG:
>>> > +    case CTLTYPE_ULONG:
>>> > +        /*
>>> > +         * If the sysctl has a type of long/ulong but seems to be
>>> bigger than
>>> > +         * these data types, its probably an array.  Double check
>>> that its
>>> > +         * evenly divisible by the size of long and convert holdp to
>>> a series of
>>> > +         * 32bit elements instead, adjusting holdlen to the new size.
>>> > +         */
>>> > +        if ((*holdlen > sizeof(abi_ulong)) &&
>>> > +            ((*holdlen % sizeof(abi_ulong)) == 0)) {
>>> > +            int array_size = *holdlen / sizeof(long);
>>> > +            int i;
>>> > +            if (holdp) {
>>> > +                for (i = 0; i < array_size; i++) {
>>> > +                    ((uint32_t *)holdp)[i] = tswap32(((long
>>> *)holdp)[i]);
>>> > +                }
>>> > +                *holdlen = array_size * sizeof(abi_ulong);
>>> > +            } else {
>>> > +                *holdlen = sizeof(abi_ulong);
>>> > +            }
>>> > +        } else {
>>> > +            *(uint32_t *)holdp = tswap32(*(long *)holdp);
>>> > +            *holdlen = sizeof(uint32_t);
>>>
>>> This is totally confusing.  Why would it ever be an array?
>>> Why is this section the only place we ever assign back into holdlen?
>>>
>>> Can you point to anything similar in the freebsd source?  The whole
>>> thing is pretty hard
>>> to track, starting from sys/kern/kern_sysctl.c.
>>>
>>
>> I need to understand this... I've been looking for where we export an
>> array, and we just don't.
>>
>> I've asked the original author who said it had something to do with
>> different size longs. I'll
>> look into that a bit and get back to this.
>>
>> I think we assign back into holdlen in a weird attempt adjust for the
>> difference of LONG between
>> the two. But I'm not sure that that's where we should assign.
>>
>
> OK. I understand what's going on. If you look at kern_sysctl.c
> sysctl_old_ddb or
> sbin/sysctl/sysctl.c show_var, you'll see that these values canbe arrays.
> This code
> only implements the array part for long and ulong, most likely because
> that's
> all that was encountered in the field.
>
>  So the code is right, as far as it goes.... But if the value is bigger
> than a long, it
> will be truncated, which strikes me as a rather weird thing to do since
> most longs
> are for sizes of things, so I'd think it would be better to saturate.
>
> We also adjust the length here because the host's memory requirements
> are larger than tha targets. This also means that we're likely returning an
> error for long/ulong fetches since the target would pass in 4 and the host
> would want 8, and would return ENOMEM. There's no code to cope with
> this at all, but I think there needs to be a temporary host buffer that's
> then copied to the target buffer once it's converted. So I need to write
> that code.
>
> Also, this code doesn't handle the newer types that FreeBSD has grown
> in the last few years: _{S,U}{8,16,32,64}. At least those are fixed between
> the two different ABIs that freebsd supports (ILP32 and LP64).
>
> Also, there's a size issue. *holdlen is a size_t, so we need to do a
> similar
> brokering for ABI32 targets. The interface is such that we need to
> read/write
> this variable because that's what the kernel is doing (reading it to make
> sure
> it's big enough, and then writing it to the actual size).
>

Actually, this issue isn't an issue because, modulo bugs, the callers of
sysctl_freebsd_oid() handle it.


> Also (not relevant to this patch), we must not set sysctls very often. newp
> needs similar treatment tooldp (except the reverse direction), but isn't
> getting any of the tswaptreatment, so it's broken for long/ulong types as
> well
> as on powerpc which we have out-of-tree now and is the only big-endian
> port we have left.
>
> tl;dr: I think I'm going to have to do a bit of a rewrite here...
>
> Warner
>

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

Re: [PATCH 7/9] bsd-user: do_freebsd_sysctl helper for sysctl(2)

[Warner Losh]

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

On Sat, Feb 11, 2023 at 4:09 PM Richard Henderson <
richard.henderson@linaro.org> wrote:

> On 2/10/23 13:18, Warner Losh wrote:
> > +abi_long do_freebsd_sysctl(CPUArchState *env, abi_ulong namep, int32_t
> namelen,
> > +        abi_ulong oldp, abi_ulong oldlenp, abi_ulong newp, abi_ulong
> newlen)
> > +{
> > +    abi_long ret;
> > +    void *hnamep, *holdp = NULL, *hnewp = NULL;
> > +    size_t holdlen;
> > +    abi_ulong oldlen = 0;
> > +    int32_t *snamep = g_malloc(sizeof(int32_t) * namelen), *p, *q, i;
> > +
> > +    if (oldlenp) {
> > +        if (get_user_ual(oldlen, oldlenp)) {
> > +            return -TARGET_EFAULT;
> > +        }
> > +    }
>
> You need to check for write early.  Either access_ok, or lock_user.
>
> > +    for (p = hnamep, q = snamep, i = 0; i < namelen; p++, i++) {
> > +        *q++ = tswap32(*p);
> > +    }
>
> Why the inconsistent increments?
>

no reason... Fixed.


> > +    unlock_user(holdp, oldp, holdlen);
>
> Usually we don't want writeback on error.
>

Indeed. Fixed as well.. in the other fixes for error handling.

Warner


>
> r~
>

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

Re: [PATCH 9/9] bsd-user: Add -strict

[Warner Losh]

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

On Sat, Feb 11, 2023 at 4:19 PM Richard Henderson <
richard.henderson@linaro.org> wrote:

> On 2/10/23 13:18, Warner Losh wrote:
> > Most of the time, it's useful to make our best effort, but sometimes we
> > want to know right away when we don't implement something. First place
> > we use it is for unknown syscalls.
> >
> > Signed-off-by: Warner Losh <imp@bsdimp.com>
> > ---
> >   bsd-user/freebsd/os-syscall.c | 4 ++++
> >   bsd-user/main.c               | 5 ++++-
> >   bsd-user/qemu.h               | 1 +
> >   3 files changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/bsd-user/freebsd/os-syscall.c
> b/bsd-user/freebsd/os-syscall.c
> > index 179a20c304b..e2b26ecb8dd 100644
> > --- a/bsd-user/freebsd/os-syscall.c
> > +++ b/bsd-user/freebsd/os-syscall.c
> > @@ -508,6 +508,10 @@ static abi_long freebsd_syscall(void *cpu_env, int
> num, abi_long arg1,
> >
> >       default:
> >           qemu_log_mask(LOG_UNIMP, "Unsupported syscall: %d\n", num);
> > +        if (bsd_user_strict) {
> > +            printf("Unimplemented system call %d\n", num);
> > +            abort();
> > +        }
>
> I don't like the raw printf, even if you did write to stderr.
> Perhaps just the abort, letting the error message be handled by qemu_log?
>
> > @@ -396,6 +397,8 @@ int main(int argc, char **argv)
> >               trace_opt_parse(optarg);
> >           } else if (!strcmp(r, "0")) {
> >               argv0 = argv[optind++];
> > +        } else if (!strcmp(r, "strict")) {
> > +            bsd_user_strict = true;
>
> Perhaps force LOG_UNIMP?  Without -D, you'll get the qemu_log above to
> stderr.
>

Given the number of other changes in the other bits, I'm going to defer
this feedback until round 3, since
the number of changes I've made for round 2 means there will  almost
certainly result in a round 3...

Warner

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