From 017eaf2955331d21f386441a85a71430d556c8f3 Mon Sep 17 00:00:00 2001 From: Jared Burce Date: Wed, 24 Mar 2021 01:12:32 -0700 Subject: [PATCH] emu: Hack in push/pop support for ModR/M. UNSAFE, and maybe UNSOUND. --- src/emu/i8088.rs | 43 +++++++++++++++++++++++++++++- src/emu/operands.rs | 18 +++++++++++++ src/emu/operations.rs | 62 ++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 121 insertions(+), 2 deletions(-) diff --git a/src/emu/i8088.rs b/src/emu/i8088.rs index 642f5e2..b5fa3e3 100644 --- a/src/emu/i8088.rs +++ b/src/emu/i8088.rs @@ -104,6 +104,45 @@ macro_rules! step { step!(@arg $cookie, $bus) }; + // Horrible, Ugly, No Good Hack. + // + // Enqueue a mutable reference to the bus without telling the borrow + // checker, iff the ModR/M byte selects for a register operand. See + // comments on push_modrm/pop_modrm in the operations module. + // + // Safety: Yolo! (Iolo?) + // + // Well, maybe. The reason we can't just enqueue the bus directly is + // because the borrowck can't know statically that the cases where + // we take the bus here (ModR/M within 0xC0..=0xC7) are also the + // cases where there won't be a FarPtr operand taking a conflicting + // bus ref. + // + // It is up to the caller to make sure that there isn't actually + // another reference to the bus, as that would violate aliasing + // rules. Of course, there IS another bus reference, the one we're + // copying from! So we can't touch that at all for the lifetime of + // the reference we're creating here, either. + // + // Is this enough to be safe? Who knows! Rust hasn't yet actually + // defined its aliasing rules or UB and doesn't seem to be in any + // hurry to. LLVM talks in terms of "memory access done through a + // pointer" which as long as we're not doing anything with the + // original pointer while the other one exists sort of suggests that + // this should be okay but also doesn't really sound like a + // guarantee. + (@unsafe_bus_maybe $cookie:tt, $cpu:expr, $bus:expr, $prefix:tt, + ($modrm_val:ident, $modrm:tt, $modrm16:tt, $modrm8:tt)) => { + step!(@arg $cookie, + if (0xC0..=0xC7).contains(&$modrm_val) { + // Sneak a mutable reference past the borrow checker. + unsafe { ($bus as *mut Bus).as_mut() } + } else { + None + } + ) + }; + (@cpu $cookie:tt, $cpu:expr, $bus:expr, $prefix:tt, $modrm:tt) => { step!(@arg $cookie, $cpu) }; @@ -451,6 +490,7 @@ impl i8088 { 0x08 => mov[reg=cs, modrm16] / "2/12+", 0x10 => mov[reg=ss, modrm16] / "2/12+", 0x18 => mov[reg=ds, modrm16] / "2/12+", }, + 0x8F: { 0x00 => pop_modrm[unsafe_bus_maybe, regval=ss, reg=sp, modrm16] / "12/25+" }, // POP r/m16 0x90 => nop[] / 3, 0xA0 => mov[reglo=a, addr] / 14, 0xA1 => mov[reg=a, addr] / 14, @@ -493,7 +533,8 @@ impl i8088 { 0xFE: { 00 => inc[width=byte, flags, modrm8] / "3/23+", // INC r/m8 08 => dec[width=byte, flags, modrm8] / "3/23+", }, // DEC r/m8 0xFF: { 00 => inc[width=word, flags, modrm16] / "3/23+", // INC r/m16 - 08 => dec[width=word, flags, modrm16] / "3/23+", }, // DEC r/m16 + 08 => dec[width=word, flags, modrm16] / "3/23+", // DEC r/m16 + 30 => push_modrm[unsafe_bus_maybe, regval=ss, reg=sp, modrm16] / "15/24+" }, }, modrm: { 0x00 => seg=ds, displace0=(b+si) / M 7, diff --git a/src/emu/operands.rs b/src/emu/operands.rs index 5659f81..22e0e2e 100644 --- a/src/emu/operands.rs +++ b/src/emu/operands.rs @@ -1,4 +1,5 @@ use std::cell::Cell; +use std::convert::From; use emu::num_traits as nt; @@ -148,3 +149,20 @@ impl RValue for RegLo<'_> { read_lo(self.reg) } } + +pub enum DynLValue<'a> { + Reg(Reg<'a>), + FarPtr(FarPtr<'a>), +} + +impl<'a> From> for DynLValue<'a> { + fn from(reg: Reg<'a>) -> DynLValue<'a> { + DynLValue::Reg(reg) + } +} + +impl<'a> From> for DynLValue<'a> { + fn from(farptr: FarPtr<'a>) -> DynLValue<'a> { + DynLValue::FarPtr(farptr) + } +} diff --git a/src/emu/operations.rs b/src/emu/operations.rs index 33f1c2a..6a2a29f 100644 --- a/src/emu/operations.rs +++ b/src/emu/operations.rs @@ -1,9 +1,10 @@ +use std::convert::Into; use std::fmt::Debug; use std::marker::PhantomData; use emu::dos; use emu::i8088::{Flags, RepPrefix, i8088}; -use emu::operands::{Address, FarPtr, LValue, Operand, Reg, RValue}; +use emu::operands::{Address, DynLValue, FarPtr, LValue, Operand, Reg, RValue}; use emu::pc::Bus; macro_rules! string_op { @@ -320,9 +321,57 @@ pub fn nop() {} pub fn pop(bus: &mut Bus, ss: u16, mut sp: Reg, mut dst: Reg) { let ptr = FarPtr { bus: bus, segment: ss, offset: sp.read() }; dst.write(ptr.read()); + // XXX: Not checking for stack faults or anything sp.write(sp.read() + 2); } +// Ugly hack for the ModR/M case. The ModR/M byte may select for a +// memory operand, in which case we are supplied with a FarPtr for our +// operand which contains a mutable pointer to the Bus. Unfortunately, +// we also want a mutable Bus pointer for accessing the stack. In the +// normal pop() function, this is passed in as an argument. Doing this +// here though would mean two mutable references and an angry borrow +// checker. We could just NOT pass in a Bus for stack access and +// instead just use the one from the memory operand, but alas the +// ModR/M byte can also select for a register operand and those do +// not carry a Bus pointer so we'd be left without one in that case. +// This is all the more annoying because there isn't a great reason +// to do this since all the registers that ModR/M can select for have +// their own dedicated single-byte opcodes for push/pop. +// +// The "solution" employed here takes a Bus pointer Option as an +// argument which is filled whenever the supplied operand is a +// register, but None whenever the operand is a FarPtr. We then do a +// manual dynamic dispatch on the operand type. In the Register case, +// we just delegate to normal pop(). In the memory case, we use the +// operand's Bus pointer to access the stack. +// +// There is unsafety getting to this point. See the comments on the +// @unsafe_bus_maybe argument decoder. I'm not sure that this is +// sound. If things start crashing, removal of the modrm variants is a +// resonable option as they do not appear to be used in any code I +// currently care about. +// +// If it turns out that a "real" solution is needed, this can be +// rewritten safely. We could remove the reference to the Bus in +// FarPtr, and just *always* pass in a bus to all operations +// everywhere that might require it. This would also mean that +// [LR]Value read & write would also need to take a bus argument. That +// seems like a lot of ugly for these two ops that I don't even care +// about, so instead of spreading the ugly out evenly and doing it +// "right", I'm concentrating it here. +pub fn pop_modrm<'a>(bus: Option<&mut Bus>, ss: u16, mut sp: Reg, dst: impl Into>) { + match dst.into() { + DynLValue::Reg(reg) => pop(bus.unwrap(), ss, sp, reg), + DynLValue::FarPtr(mut farptr) => { + let val = >::read(&FarPtr { bus: farptr.bus, segment: ss, offset: sp.read() }); + farptr.write(val); + // XXX: Not checking for stack faults or anything + sp.write(sp.read() + 2); + } + } +} + pub fn push(bus: &mut Bus, ss: u16, mut sp: Reg, val: u16) { // XXX: Not checking for stack faults or anything sp.write(sp.read() - 2); @@ -330,6 +379,17 @@ pub fn push(bus: &mut Bus, ss: u16, mut sp: Reg, val: u16) { ptr.write(val); } +// Ugly hack for the ModR/M case. See the comments on pop_modrm. +pub fn push_modrm<'a>(bus: Option<&mut Bus>, ss: u16, sp: Reg, src: impl Into>) { + match src.into() { + DynLValue::Reg(reg) => push(bus.unwrap(), ss, sp, reg.read()), + DynLValue::FarPtr(farptr) => { + let val: u16 = farptr.read(); + push(farptr.bus, ss, sp, val); + } + } +} + pub fn scasb(flags: &mut Flags, bus: &mut Bus, rep: RepPrefix,