On Sat, Feb 11, 2023 at 3:56 PM Richard Henderson <
[email protected]> 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~
>
>
