Re: git: 4b500174dd2d - main - arm64: emulate swp/swpb instructions

On Mon, May 15, 2023 at 10:49 AM Jessica Clarke <jrtc27@freebsd.org> wrote:
>
> On 15 May 2023, at 16:42, Kyle Evans <kevans@FreeBSD.org> wrote:
> >
> > The branch main has been updated by kevans:
> >
> > URL: https://cgit.FreeBSD.org/src/commit/?id=4b500174dd2d1e16dcb87c22364f724472c82edc
> >
> > commit 4b500174dd2d1e16dcb87c22364f724472c82edc
> > Author:     Kyle Evans <kevans@FreeBSD.org>
> > AuthorDate: 2023-05-15 15:42:10 +0000
> > Commit:     Kyle Evans <kevans@FreeBSD.org>
> > CommitDate: 2023-05-15 15:42:10 +0000
> >
> >    arm64: emulate swp/swpb instructions
> >
> >    Add another undefined instruction handler for compat32 and watch out for
> >    SWP/SWPB instructions.
> >
> >    SWP/SWPB were deprecated in ARMv6 and declared obsolete in ARMv7, but
> >    this implementation is motivated by some proprietary software that still
> >    uses SWP/SWPB. Because it's deprecated, emulation is pushed back behind
> >    a sysctl that defaults to OFF in GENERIC so that it doesn't potentially
> >    adversely affect package builds; it's unknown whether software may test
> >    for a functional swp/swpb instruction with the desire of using it later,
> >    so we err on the side of caution to ensure we don't end up with swp/swpb
> >    use in freebsd/arm packages (which are built on aarch64).
> >
> >    The EMUL_SWP config option may be used to enable emulation by default in
> >    environments where emulation is desired and won't really be turned off.
> >
> >    Reviewed by:    andrew, mmel (both earlier version)
> >    Sponsored by:   Stormshield
> >    Sponsored by:   Klara, Inc.
> >    Differential Revision:  https://reviews.freebsd.org/D39667
> > ---
> > sys/arm64/arm64/freebsd32_machdep.c |   4 +
> > sys/arm64/arm64/undefined.c         | 162 ++++++++++++++++++++++++++++++++++++
> > sys/conf/options.arm64              |   2 +
> > 3 files changed, 168 insertions(+)
> >
> > diff --git a/sys/arm64/arm64/freebsd32_machdep.c b/sys/arm64/arm64/freebsd32_machdep.c
> > index 5fadef74df87..44f35f1b2abf 100644
> > --- a/sys/arm64/arm64/freebsd32_machdep.c
> > +++ b/sys/arm64/arm64/freebsd32_machdep.c
> > @@ -34,6 +34,7 @@ __FBSDID("$FreeBSD$");
> > #include <sys/mutex.h>
> > #include <sys/syscallsubr.h>
> > #include <sys/ktr.h>
> > +#include <sys/sysctl.h>
> > #include <sys/sysent.h>
> > #include <sys/sysproto.h>
> > #include <machine/armreg.h>
> > @@ -55,6 +56,9 @@ _Static_assert(sizeof(struct siginfo32) == 64, "struct siginfo32 size incorrect"
> >
> > extern void freebsd32_sendsig(sig_t catcher, ksiginfo_t *ksi, sigset_t *mask);
> >
> > +SYSCTL_NODE(_compat, OID_AUTO, arm, CTLFLAG_RW | CTLFLAG_MPSAFE, 0,
> > +    "32-bit mode");
> > +
> > /*
> >  * The first two fields of a ucontext_t are the signal mask and the machine
> >  * context.  The next field is uc_link; we want to avoid destroying the link
> > diff --git a/sys/arm64/arm64/undefined.c b/sys/arm64/arm64/undefined.c
> > index 1feb242db060..7f436aaef6e5 100644
> > --- a/sys/arm64/arm64/undefined.c
> > +++ b/sys/arm64/arm64/undefined.c
> > @@ -39,14 +39,47 @@ __FBSDID("$FreeBSD$");
> > #include <sys/queue.h>
> > #include <sys/signal.h>
> > #include <sys/signalvar.h>
> > +#include <sys/sysctl.h>
> > #include <sys/sysent.h>
> >
> > +#include <machine/atomic.h>
> > #include <machine/frame.h>
> > +#define _MD_WANT_SWAPWORD
> > +#include <machine/md_var.h>
> > +#include <machine/pcb.h>
> > #include <machine/undefined.h>
> > #include <machine/vmparam.h>
> >
> > +#include <vm/vm.h>
> > +#include <vm/vm_extern.h>
> > +
> > +/* Low bit masked off */
> > +#define INSN_COND(insn) ((insn >> 28) & ~0x1)
> > +#define INSN_COND_INVERTED(insn) ((insn >> 28) & 0x1)
> > +#define INSN_COND_EQ 0x00 /* NE */
> > +#define INSN_COND_CS 0x02 /* CC */
> > +#define INSN_COND_MI 0x04 /* PL */
> > +#define INSN_COND_VS 0x06 /* VC */
> > +#define INSN_COND_HI 0x08 /* LS */
> > +#define INSN_COND_GE 0x0a /* LT */
> > +#define INSN_COND_GT 0x0c /* LE */
> > +#define INSN_COND_AL 0x0e /* Always */
> > +
> > MALLOC_DEFINE(M_UNDEF, "undefhandler", "Undefined instruction handler data");
> >
> > +#ifdef COMPAT_FREEBSD32
> > +#ifndef EMUL_SWP
> > +#define EMUL_SWP 0
> > +#endif
> > +
> > +SYSCTL_DECL(_compat_arm);
> > +
> > +static bool compat32_emul_swp = EMUL_SWP;
> > +SYSCTL_BOOL(_compat_arm, OID_AUTO, emul_swp,
> > +    CTLFLAG_RWTUN | CTLFLAG_MPSAFE, &compat32_emul_swp, 0,
> > +    "Enable SWP/SWPB emulation");
> > +#endif
> > +
> > struct undef_handler {
> > LIST_ENTRY(undef_handler) uh_link;
> > undef_handler_t uh_handler;
> > @@ -88,6 +121,54 @@ id_aa64mmfr2_handler(vm_offset_t va, uint32_t insn, struct trapframe *frame,
> > return (0);
> > }
> >
> > +static bool
> > +arm_cond_match(uint32_t insn, struct trapframe *frame)
> > +{
> > + uint64_t spsr;
> > + uint32_t cond;
> > + bool invert;
> > + bool match;
> > +
> > + /*
> > + * Generally based on the function of the same name in NetBSD, though
> > + * condition bits left in their original position rather than shifting
> > + * over the low bit that indicates inversion for quicker sanity checking
> > + * against spec.
> > + */
> > + spsr = frame->tf_spsr;
> > + cond = INSN_COND(insn);
> > + invert = INSN_COND_INVERTED(insn);
> > +
> > + switch (cond) {
> > + case INSN_COND_EQ:
> > + match = (spsr & PSR_Z) != 0;
> > + break;
> > + case INSN_COND_CS:
> > + match = (spsr & PSR_C) != 0;
> > + break;
> > + case INSN_COND_MI:
> > + match = (spsr & PSR_N) != 0;
> > + break;
> > + case INSN_COND_VS:
> > + match = (spsr & PSR_V) != 0;
> > + break;
> > + case INSN_COND_HI:
> > + match = (spsr & (PSR_C | PSR_Z)) == PSR_C;
> > + break;
> > + case INSN_COND_GE:
> > + match = (!(spsr & PSR_N) == !(spsr & PSR_V));
> > + break;
> > + case INSN_COND_GT:
> > + match = !(spsr & PSR_Z) && (!(spsr & PSR_N) == !(spsr & PSR_V));
> > + break;
> > + case INSN_COND_AL:
> > + match = true;
> > + break;
> > + }
> > +
> > + return (!match != !invert);
>
> This is equivalent to match != invert?
>

Good shout, fixed in b68588618b43, thanks. Clear lack of consideration
on my part, and I'm guessing they were ints in an earlier version of
NetBSD's implementation.

> Jess