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~