| Message ID | 20230210231829.39476-7-imp@bsdimp.com |
|---|---|
| State | New |
| Headers | show |
| Series | 2023 Q1 bsd-user upstreaming: bugfixes and sysctl | expand |
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~
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~ > >
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~
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
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~
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 */