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~