On 8/7/21 8:03 PM, Warner Losh wrote:
>
>
> On Sat, Aug 7, 2021 at 11:35 PM Richard Henderson <richard.henderson@linaro.org
> <mailto:richard.henderson@linaro.org>> wrote:
>
> On 8/7/21 11:42 AM, Warner Losh wrote:
> > diff --git a/bsd-user/i386/target_arch_cpu.c b/bsd-user/i386/target_arch_cpu.c
> > index 7f2f755a11..71998e5ba5 100644
> > --- a/bsd-user/i386/target_arch_cpu.c
> > +++ b/bsd-user/i386/target_arch_cpu.c
> > @@ -1,6 +1,7 @@
> > /*
> > * i386 cpu related code
> > *
> > + * Copyright (c) 2013 Stacey Son <sson@FreeBSD.org>
> > *
> > * This program is free software; you can redistribute it and/or modify
> > * it under the terms of the GNU General Public License as published by
>
> Should be in previous.
>
>
> Gotcha.
>
> > +static inline void target_cpu_init(CPUX86State *env,
> > + struct target_pt_regs *regs)
> > +{
> > + uint64_t *gdt_table;
> > +
> > + env->cr[0] = CR0_PG_MASK | CR0_WP_MASK | CR0_PE_MASK;
> > + env->hflags |= HF_PE_MASK | HF_CPL_MASK;
> > + if (env->features[FEAT_1_EDX] & CPUID_SSE) {
> > + env->cr[4] |= CR4_OSFXSR_MASK;
> > + env->hflags |= HF_OSFXSR_MASK;
> > + }
> > +
> > + /* flags setup : we activate the IRQs by default as in user mode */
> > + env->eflags |= IF_MASK;
> > +
> > + /* register setup */
> > + env->regs[R_EAX] = regs->eax;
> > + env->regs[R_EBX] = regs->ebx;
> > + env->regs[R_ECX] = regs->ecx;
> > + env->regs[R_EDX] = regs->edx;
> > + env->regs[R_ESI] = regs->esi;
> > + env->regs[R_EDI] = regs->edi;
> > + env->regs[R_EBP] = regs->ebp;
> > + env->regs[R_ESP] = regs->esp;
> > + env->eip = regs->eip;
> > +
> > + /* interrupt setup */
> > + env->idt.limit = 255;
> > +
> > + env->idt.base = target_mmap(0, sizeof(uint64_t) * (env->idt.limit + 1),
> > + PROT_READ|PROT_WRITE, MAP_ANONYMOUS|MAP_PRIVATE, -1, 0);
> > + bsd_i386_set_idt_base(env->idt.base);
> > + bsd_i386_set_idt(0, 0);
> > + bsd_i386_set_idt(1, 0);
> > + bsd_i386_set_idt(2, 0);
> > + bsd_i386_set_idt(3, 3);
> > + bsd_i386_set_idt(4, 3);
> > + bsd_i386_set_idt(5, 0);
> > + bsd_i386_set_idt(6, 0);
> > + bsd_i386_set_idt(7, 0);
> > + bsd_i386_set_idt(8, 0);
> > + bsd_i386_set_idt(9, 0);
> > + bsd_i386_set_idt(10, 0);
> > + bsd_i386_set_idt(11, 0);
> > + bsd_i386_set_idt(12, 0);
> > + bsd_i386_set_idt(13, 0);
> > + bsd_i386_set_idt(14, 0);
> > + bsd_i386_set_idt(15, 0);
> > + bsd_i386_set_idt(16, 0);
> > + bsd_i386_set_idt(17, 0);
> > + bsd_i386_set_idt(18, 0);
> > + bsd_i386_set_idt(19, 0);
> > + bsd_i386_set_idt(0x80, 3);
> > +
> > + /* segment setup */
> > + env->gdt.base = target_mmap(0, sizeof(uint64_t) * TARGET_GDT_ENTRIES,
> > + PROT_READ|PROT_WRITE, MAP_ANONYMOUS|MAP_PRIVATE, -1, 0);
> > + env->gdt.limit = sizeof(uint64_t) * TARGET_GDT_ENTRIES - 1;
> > + gdt_table = g2h_untagged(env->gdt.base);
> > +
> > + bsd_i386_write_dt(&gdt_table[__USER_CS >> 3], 0, 0xfffff,
> > + DESC_G_MASK | DESC_B_MASK | DESC_P_MASK | DESC_S_MASK |
> > + (3 << DESC_DPL_SHIFT) | (0xa << DESC_TYPE_SHIFT));
> > +
> > + bsd_i386_write_dt(&gdt_table[__USER_DS >> 3], 0, 0xfffff,
> > + DESC_G_MASK | DESC_B_MASK | DESC_P_MASK | DESC_S_MASK |
> > + (3 << DESC_DPL_SHIFT) | (0x2 << DESC_TYPE_SHIFT));
> > +
> > + cpu_x86_load_seg(env, R_CS, __USER_CS);
> > + cpu_x86_load_seg(env, R_SS, __USER_DS);
> > + cpu_x86_load_seg(env, R_DS, __USER_DS);
> > + cpu_x86_load_seg(env, R_ES, __USER_DS);
> > + cpu_x86_load_seg(env, R_FS, __USER_DS);
> > + cpu_x86_load_seg(env, R_GS, __USER_DS);
> > + /* This hack makes Wine work... */
> > + env->segs[R_FS].selector = 0;
> > +}
> > +
> > +static inline void target_cpu_loop(CPUX86State *env)
> > +{
> > + CPUState *cs = env_cpu(env);
> > + int trapnr;
> > + abi_ulong pc;
> > + /* target_siginfo_t info; */
> > +
> > + for (;;) {
> > + cpu_exec_start(cs);
> > + trapnr = cpu_exec(cs);
> > + cpu_exec_end(cs);
> > + process_queued_cpu_work(cs);
> > +
> > + switch (trapnr) {
> > + case 0x80:
> > + /* syscall from int $0x80 */
> > + if (bsd_type == target_freebsd) {
> > + abi_ulong params = (abi_ulong) env->regs[R_ESP] +
> > + sizeof(int32_t);
> > + int32_t syscall_nr = env->regs[R_EAX];
> > + int32_t arg1, arg2, arg3, arg4, arg5, arg6, arg7, arg8;
> > +
> > + if (syscall_nr == TARGET_FREEBSD_NR_syscall) {
> > + get_user_s32(syscall_nr, params);
> > + params += sizeof(int32_t);
> > + } else if (syscall_nr == TARGET_FREEBSD_NR___syscall) {
> > + get_user_s32(syscall_nr, params);
> > + params += sizeof(int64_t);
> > + }
> > + get_user_s32(arg1, params);
> > + params += sizeof(int32_t);
> > + get_user_s32(arg2, params);
> > + params += sizeof(int32_t);
> > + get_user_s32(arg3, params);
> > + params += sizeof(int32_t);
> > + get_user_s32(arg4, params);
> > + params += sizeof(int32_t);
> > + get_user_s32(arg5, params);
> > + params += sizeof(int32_t);
> > + get_user_s32(arg6, params);
> > + params += sizeof(int32_t);
> > + get_user_s32(arg7, params);
> > + params += sizeof(int32_t);
> > + get_user_s32(arg8, params);
> > + env->regs[R_EAX] = do_freebsd_syscall(env,
> > + syscall_nr,
> > + arg1,
> > + arg2,
> > + arg3,
> > + arg4,
> > + arg5,
> > + arg6,
> > + arg7,
> > + arg8);
> > + } else { /* if (bsd_type == target_openbsd) */
> > + env->regs[R_EAX] = do_openbsd_syscall(env,
> > + env->regs[R_EAX],
> > + env->regs[R_EBX],
> > + env->regs[R_ECX],
> > + env->regs[R_EDX],
> > + env->regs[R_ESI],
> > + env->regs[R_EDI],
> > + env->regs[R_EBP]);
> > + }
> > + if (((abi_ulong)env->regs[R_EAX]) >= (abi_ulong)(-515)) {
> > + env->regs[R_EAX] = -env->regs[R_EAX];
> > + env->eflags |= CC_C;
> > + } else {
> > + env->eflags &= ~CC_C;
> > + }
> > + break;
> > +
> > +#if 0
> > + case EXCP0B_NOSEG:
> > + case EXCP0C_STACK:
> > + info.si_signo = TARGET_SIGBUS;
> > + info.si_errno = 0;
> > + info.si_code = TARGET_SI_KERNEL;
> > + info._sifields._sigfault._addr = 0;
> > + queue_signal(env, info.si_signo, &info);
> > + break;
> > +
> > + case EXCP0D_GPF:
> > + /* XXX: potential problem if ABI32 */
> > + if (env->eflags & VM_MASK) {
> > + handle_vm86_fault(env);
> > + } else {
> > + info.si_signo = TARGET_SIGSEGV;
> > + info.si_errno = 0;
> > + info.si_code = TARGET_SI_KERNEL;
> > + info._sifields._sigfault._addr = 0;
> > + queue_signal(env, info.si_signo, &info);
> > + }
> > + break;
> > +
> > + case EXCP0E_PAGE:
> > + info.si_signo = TARGET_SIGSEGV;
> > + info.si_errno = 0;
> > + if (!(env->error_code & 1)) {
> > + info.si_code = TARGET_SEGV_MAPERR;
> > + } else {
> > + info.si_code = TARGET_SEGV_ACCERR;
> > + }
> > + info._sifields._sigfault._addr = env->cr[2];
> > + queue_signal(env, info.si_signo, &info);
> > + break;
> > +
> > + case EXCP00_DIVZ:
> > + if (env->eflags & VM_MASK) {
> > + handle_vm86_trap(env, trapnr);
> > + } else {
> > + /* division by zero */
> > + info.si_signo = TARGET_SIGFPE;
> > + info.si_errno = 0;
> > + info.si_code = TARGET_FPE_INTDIV;
> > + info._sifields._sigfault._addr = env->eip;
> > + queue_signal(env, info.si_signo, &info);
> > + }
> > + break;
> > +
> > + case EXCP01_DB:
> > + case EXCP03_INT3:
> > + if (env->eflags & VM_MASK) {
> > + handle_vm86_trap(env, trapnr);
> > + } else {
> > + info.si_signo = TARGET_SIGTRAP;
> > + info.si_errno = 0;
> > + if (trapnr == EXCP01_DB) {
> > + info.si_code = TARGET_TRAP_BRKPT;
> > + info._sifields._sigfault._addr = env->eip;
> > + } else {
> > + info.si_code = TARGET_SI_KERNEL;
> > + info._sifields._sigfault._addr = 0;
> > + }
> > + queue_signal(env, info.si_signo, &info);
> > + }
> > + break;
> > +
> > + case EXCP04_INTO:
> > + case EXCP05_BOUND:
> > + if (env->eflags & VM_MASK) {
> > + handle_vm86_trap(env, trapnr);
> > + } else {
> > + info.si_signo = TARGET_SIGSEGV;
> > + info.si_errno = 0;
> > + info.si_code = TARGET_SI_KERNEL;
> > + info._sifields._sigfault._addr = 0;
> > + queue_signal(env, info.si_signo, &info);
> > + }
> > + break;
> > +
> > + case EXCP06_ILLOP:
> > + info.si_signo = TARGET_SIGILL;
> > + info.si_errno = 0;
> > + info.si_code = TARGET_ILL_ILLOPN;
> > + info._sifields._sigfault._addr = env->eip;
> > + queue_signal(env, info.si_signo, &info);
> > + break;
> > +#endif
> > + case EXCP_INTERRUPT:
> > + /* just indicate that signals should be handled asap */
> > + break;
> > +#if 0
> > + case EXCP_DEBUG:
> > + {
> > +
> > + info.si_signo = TARGET_SIGTRAP;
> > + info.si_errno = 0;
> > + info.si_code = TARGET_TRAP_BRKPT;
> > + queue_signal(env, info.si_signo, &info);
> > + }
> > + break;
> > +#endif
> > +
> > + case EXCP_ATOMIC:
> > + cpu_exec_step_atomic(cs);
> > + break;
> > +
> > + default:
> > + pc = env->segs[R_CS].base + env->eip;
> > + fprintf(stderr, "qemu: 0x%08lx: unhandled CPU exception 0x%x - "
> > + "aborting\n", (long)pc, trapnr);
> > + abort();
> > + }
> > + process_pending_signals(env);
> > + }
> > +}
>
> Those are some really big functions to want to inline. Any reason for that? At first
> glance they could live just fine in target_arch_cpu.c.
>
>
> Mostly because that's how Stacey wrote it and we have half a dozen implementations
> already. I'll see how hard it would be to transition to this structure. I worried about
> this being so different than linux-user, but at the same time I worried about regressions
> that may be introduced in the effort since our testing story isn't what I'd like. I'm also
> a bit worried because we have two branches: the tip that we're trying to release from
> and this effort which needs any changes also merged to that tip branch.
>
> Having said that, I'll see what I can do.
I guess I'm happy to have them in the header for now, but just add something to the to-do
list once you're not juggling multiple branches.
I'll not say that linux-user is ideal either, btw. Just stuff you see doing new patch review.
I think I'll take you up on that offer. It's a pattern that Stacey Son used a lot when writing
this code, and I'd need to do it to many different files. If it were just this one, I'd definitely
do that in a heart-beat. But since there's a lot, I think I'd get bogged down with so
many changes. There's a number of things you see when upstreaming the code too
that suggest themselves as cleanup, and so far I've just been keeping a list for later
on the larger of those items (eg command line parsing). In general, there was a preference
for having lots of inline functions, some quite long, so that they could be swapped out
more easily by including an alternative header (per arch, per bsd flavor, etc). With LTO
here or on the horizon, I'm thinking having the interface in the .h file and the code in a .c file
and having LTO eliminate the extra calls that would create may be acceptable. But
I think the optimal solution needs some time for thought and study as well.
Warner