Modeling a Bus and its components in Rust emulator

fcoury · April 15, 2023, 9:59pm

I am writing an emulator and am having a hard time modeling a solution for this problem. Below is what I have so far. I am sure there's a better way to model this problem, and that's exactly what I am looking for.

I am trying to model the following scenario:

Structs:

Machine: is the high level abstraction of the hardware, talks via the BUS
BUS: the orchestrator, talks to the CPU via stepping to next instruction and to the VDP via writing to a given port
CPU: the CPU, talks to memory via the BUS (not represented here) and to the VDP via the BUS by writing to a given I/O port
VDP: the graphics chip with VRAM and registers, needs to talk to the CPU to enable/disable interrupts

Flow:

Machine asks CPU to step to next instruction via the BUS
CPU reads a given memory address and writes to a given VDP port via the BUS
VDP receives the write and enables interrupts on CPU via the BUS

Problem:

thread 'main' panicked at 'already borrowed: BorrowMutError', src/main.rs:61:18

    fn step(&mut self) {
        println!("CPU step");
        self.bus.borrow_mut().write_port(); // <- here
    }

What I tried so far:

Weak BUS references to CPU/VDP, returning the concrete implementation to the Machine, but the strong reference ended up being dropped
Returning and keeping the Rc<RefCell> to the Machine, and we follow on this reentrant problem
ChatGPT-4 which suggested extracting traits:

trait BusTrait {
    fn write_port(&mut self);
    fn enable_interrupt(&mut self);
}

trait CpuTrait {
    fn step(&mut self);
    fn enable_interrupt(&mut self);
}

trait VdpTrait {
    fn write_port(&mut self);
}

EDIT: adding the link to the ChatGPT proposed solution - Rust Playground

Is this the best way to go about this problem?

Minimal replication code:

use std::{cell::RefCell, rc::Rc};

struct Machine {
    bus: Rc<RefCell<Bus>>,
}

impl Machine {
    fn new() -> Self {
        let bus = Rc::new(RefCell::new(Bus::default()));
        let cpu = Cpu::new(bus.clone());
        let vdp = Vdp::new(bus.clone());

        bus.borrow_mut().cpu = Some(cpu);
        bus.borrow_mut().vdp = Some(vdp);

        Self { bus }
    }

    fn step(&self) {
        self.bus.borrow_mut().step();
    }
}

#[derive(Default)]
struct Bus {
    cpu: Option<Cpu>,
    vdp: Option<Vdp>,
}

impl Bus {
    fn step(&mut self) {
        if let Some(cpu) = &mut self.cpu {
            cpu.step();
        }
    }

    fn write_port(&mut self) {
        if let Some(vdp) = &mut self.vdp {
            vdp.write_port();
        }
    }

    fn enable_interrupt(&mut self) {
        if let Some(cpu) = &mut self.cpu {
            cpu.enable_interrupt();
        }
    }
}

struct Cpu {
    bus: Rc<RefCell<Bus>>,
}

impl Cpu {
    fn new(bus: Rc<RefCell<Bus>>) -> Self {
        Self { bus }
    }

    fn step(&mut self) {
        println!("CPU step");
        self.bus.borrow_mut().write_port();
    }

    fn enable_interrupt(&mut self) {
        println!("CPU interrupt");
    }
}

struct Vdp {
    bus: Rc<RefCell<Bus>>,
}

impl Vdp {
    fn new(bus: Rc<RefCell<Bus>>) -> Self {
        Self { bus }
    }

    fn write_port(&mut self) {
        println!("VDP enable IRQ");
        self.bus.borrow_mut().enable_interrupt();
    }
}

fn main() {
    let machine = Machine::new();
    machine.step();
}

EDIT: IRL the flow is a little more complex, so I wanted to add this here:

Machine.step -> 
  Bus.step [borrows Bus mutably] -> 
    Cpu.step [borrows CPU mutably] -> 
      <Extrenal Cpu implementation>.step -> 
        Io.read_byte [borrows BUS mutably] -> 
          panic

kpreid · April 16, 2023, 12:44am

In general, it is not a good idea to model interconnections of this sort using Rcs, for multiple reasons:

It forces you to use RefCells or similar, which, as you have discovered, doesn't solve all borrowing conflicts, just defers them to run time.
You've constructed a circular reference, which will become a memory leak when the Machine is dropped.
It hides the dependencies that exist in the form of what data is manipulated by different function calls.

Instead, you should own the components directly and pass all of them to the functions that need them.

struct Machine {
    cpu: Cpu,
    vdp: Vdp,
}

impl Machine {
    fn new() -> Self {
        let cpu = Cpu::new();
        let vdp = Vdp::new();

        Self { cpu, vdp }
    }

    fn step(&mut self) {
        self.cpu.step(&mut self.vdp);
    }
}

struct Cpu {}

impl Cpu {
    fn new() -> Self {
        Self {}
    }

    fn step(&mut self, vdp: &mut Vdp) {
        println!("CPU step");
        vdp.write_port(self);
    }

    fn enable_interrupt(&mut self) {
        println!("CPU interrupt");
    }
}

struct Vdp {}

impl Vdp {
    fn new() -> Self {
        Self {}
    }

    fn write_port(&mut self, cpu: &mut Cpu) {
        println!("VDP enable IRQ");
        cpu.enable_interrupt();
    }
}

fn main() {
    let mut machine = Machine::new();
    machine.step();
}

(In this program I deleted the Bus because it seemed to be redundant with Machine, but it is possible that it should still exist in some form.)

semicoleon · April 16, 2023, 12:46am

Well one simple option that doesn't really fix your bigger design question is to just not borrow Bus mutably. It's entire purpose is to be a shared communication channel, so it would likely make more sense to have the Cpu and Vdp in the Bus be inside RefCells instead of the Bus. You're much less likely to have a legitimate reason to borrow Cpu/Vdp mutably twice in the same call stack.

One alternate way to model a system like this is via message passing. Instead of making calls directly you send messages between components. In some ways that's closer to how the actual hardware functions, though it can be quite annoying to write code that way. Here's a sketch of how that might work based on that sample code

use std::{cell::RefCell, rc::Rc};

enum Message {
    EnableInterrupt,
    WritePort,
    CpuStep,
}

struct Machine {
    bus: Bus,
}

impl Machine {
    fn new() -> Self {
        let queue = Rc::new(RefCell::new(Vec::new()));
        let bus = Bus {
            queue: queue.clone(),
            cpu: Cpu {
                queue: queue.clone(),
            },
            vdp: Vdp { queue },
        };

        Self { bus }
    }

    fn step(&mut self) {
        self.bus.step();
    }
}

struct Bus {
    queue: Rc<RefCell<Vec<Message>>>,
    cpu: Cpu,
    vdp: Vdp,
}

impl Bus {
    fn step(&mut self) {
        self.queue.borrow_mut().push(Message::CpuStep);

        loop {
            let Some(message) = self.queue.borrow_mut().pop() else {
                break;
            };

            match message {
                Message::EnableInterrupt => self.cpu.enable_interrupt(),
                Message::WritePort => self.vdp.write_port(),
                Message::CpuStep => self.cpu.step(),
            }
        }
    }
}

struct Cpu {
    queue: Rc<RefCell<Vec<Message>>>,
}

impl Cpu {
    fn step(&mut self) {
        println!("CPU step");
        self.queue.borrow_mut().push(Message::WritePort);
    }

    fn enable_interrupt(&mut self) {
        println!("CPU interrupt");
    }
}

struct Vdp {
    queue: Rc<RefCell<Vec<Message>>>,
}

impl Vdp {
    fn write_port(&mut self) {
        println!("VDP enable IRQ");
        self.queue.borrow_mut().push(Message::EnableInterrupt);
    }
}

fn main() {
    let mut machine = Machine::new();
    machine.step();
}

fcoury · April 16, 2023, 12:49am

This makes a lot of sense. I'll try to go down this path first, even before thinking about message passing. I'll see how far I get.

fcoury · April 16, 2023, 12:55am

Now that I tried, it's almost impossible to do one without the other, but I think I'm down on a good path. Will report back

quinedot · April 16, 2023, 1:03am

If there's a maximum of one message in the queue ^[1], that can be easily modeled with a state machine instead of an actual queue.

okay any finite number, but who wants to deal with that ↩︎

fcoury · April 16, 2023, 1:08am

OK let me bug you a bit more if I may. Would it be possible to have messages that require a response in this same design, like Message::ReadByte(Address) that would, somehow, return a u8? I thought about mpsc channels, but not sure how it would fit here.

Thank you.

semicoleon · April 16, 2023, 1:20am

Responses would be most easily modeled by sending a new message back to the original component.

You could use something like a response channel, but unless each component has a thread and is waiting on new messages, that doesn't really help you much. You still need to get back to the point where the component checks the channel.

fcoury · April 16, 2023, 1:23am

That is definitely food for thought. Let me see if I can wrap my head around it. Thanks!

fcoury · April 16, 2023, 2:11am

I have created a new example that reflects my current scenario with more fidelity. In the code below, I have no control over ExtCpu, which belongs to an external crate. In theory, I could fork the project and tweak it, but for now I would like to avoid doing it, I don't think that's what's keeping me from moving forward.

Here's the playground link and the code:

use std::{cell::RefCell, rc::Rc};

enum Message {
    CpuStep,
    ReadByte(u16),
    ReadByteResponse(u8),
    VdpEnableLineInterrupt,
    CpuEnableInterrupt,
}

struct Machine {
    bus: Bus,
}

impl Machine {
    fn new() -> Self {
        let queue = Rc::new(RefCell::new(Vec::new()));
        let cpu = Cpu::new(queue.clone());
        let vdp = Vdp::new(queue.clone());
        let ppi = Ppi {};
        let bus = Bus::new(cpu, vdp, ppi, queue);
        Self { bus }
    }

    fn step(&mut self) {
        self.bus.step();
    }
}

struct Bus {
    cpu: Cpu,
    vdp: Vdp,
    ppi: Ppi,
    queue: Rc<RefCell<Vec<Message>>>,
}

impl Bus {
    fn new(cpu: Cpu, vdp: Vdp, ppi: Ppi, queue: Rc<RefCell<Vec<Message>>>) -> Self {
        Self {
            cpu,
            vdp,
            ppi,
            queue,
        }
    }

    fn step(&mut self) {
        println!("Bus step");
        self.queue.borrow_mut().push(Message::CpuStep);

        loop {
            let Some(message) = self.queue.borrow_mut().pop() else {
                break;
            };

            match message {
                Message::CpuStep => {
                    self.cpu.step();
                }
                Message::ReadByte(addr) => {
                    let data = self.ppi.read_byte(addr);
                    self.queue
                        .borrow_mut()
                        .push(Message::ReadByteResponse(data));
                }
                Message::ReadByteResponse(data) => {
                    // How do I get the data to the External CPU?
                }
                Message::VdpEnableLineInterrupt => {
                    self.vdp.enable_line_interrupt();
                }
                Message::CpuEnableInterrupt => {
                    self.cpu.enable_interrupt();
                }
            }
        }
    }
}

struct Cpu {
    ext_cpu: ExtCpu<Io>,
}

impl Cpu {
    fn new(queue: Rc<RefCell<Vec<Message>>>) -> Self {
        let io = Io::new(queue);
        let ext_cpu = ExtCpu { io };
        Self { ext_cpu }
    }

    fn step(&mut self) {
        println!("Cpu step");
        self.ext_cpu.step();
    }

    fn enable_interrupt(&self) {
        println!("Cpu enable_interrupt");
        self.ext_cpu.enable_interrupt();
    }
}

struct Ppi;

impl Ppi {
    fn read_byte(&self, addr: u16) -> u8 {
        println!("Ppi read_byte {:x}", addr);
        0xfe
    }
}

struct Vdp {
    queue: Rc<RefCell<Vec<Message>>>,
}

impl Vdp {
    fn new(queue: Rc<RefCell<Vec<Message>>>) -> Self {
        Self { queue }
    }

    fn enable_line_interrupt(&self) {
        println!("Vdp enable_line_interrupt");
        self.queue.borrow_mut().push(Message::CpuEnableInterrupt);
    }
}

trait ExtCpuIo {
    fn read(&self, addr: u16) -> u8;
    fn write(&self, addr: u16, data: u8);
    fn read_port(&self, port: u8) -> u8;
    fn write_port(&self, port: u8, data: u8);
}

struct ExtCpu<ExtCpuIo> {
    io: ExtCpuIo,
}

impl<T: ExtCpuIo> ExtCpu<T> {
    fn step(&mut self) {
        println!("ExtCpu step");
        if self.io.read(0) == 0 {
            self.io.write_port(0, 1);
        }
    }

    fn enable_interrupt(&self) {
        println!("ExtCpu enable_interrupt");
        self.io.write_port(1, 1);
    }
}

struct Io {
    queue: Rc<RefCell<Vec<Message>>>,
}

impl Io {
    fn new(queue: Rc<RefCell<Vec<Message>>>) -> Self {
        Self { queue }
    }
}

impl ExtCpuIo for Io {
    fn read(&self, addr: u16) -> u8 {
        println!("ExtCpuIo read: {:x}", addr);
        self.queue.borrow_mut().push(Message::ReadByte(0x0042));
        todo!("How do I get the u8 to respond the External CPU?")
    }

    fn write(&self, addr: u16, data: u8) {
        println!("write: {:x} {:x}", addr, data);
    }

    fn read_port(&self, port: u8) -> u8 {
        println!("ExtCpuIo read_port: {:x}", port);
        todo!()
    }

    fn write_port(&self, port: u8, data: u8) {
        println!("ExtCpuIo write_port: {:x} {:x}", port, data);
        if port == 0x00 {
            self.queue
                .borrow_mut()
                .push(Message::VdpEnableLineInterrupt);
        }
    }
}

fn main() {
    let mut machine = Machine::new();
    machine.step();
}

fcoury · April 16, 2023, 2:15am

Thank you for the advice, I have tried this before, but let me try once again :-).

In any way, I have posted a followup code that better reflects the state the code is at right now. There is one component on the execution path that I don't own, but I guess that wouldn't prevent me from passing it around. I'll look at the code again with this perspective in mind.

fcoury · April 16, 2023, 2:16am

Yeah, I really think if I can wrap my head around how to make the message passing/state machine have one method that is synchronous to the caller and still go through the queue/state machine execution, I will be set.

I updated the code to a more faithful representation of what I'm really working with.

Thank you!

semicoleon · April 16, 2023, 4:17am

There isn't a clean way to switch between the styles when you don't control the component like that. You can wrap the component the CPU needs in an Rc<RefCell<>> as long as it doesn't create a double borrow issue like before.

One possible fix is structuring things so you can run the message loop inside the Io's calls, but setting that up correctly is tricky.

fcoury · April 16, 2023, 4:03pm

I'll attempt using Rc<RefCell<>> to see where it gets me. I'll post a new reply below with the full sequence diagram to see if it helps as well.

fcoury · April 16, 2023, 4:04pm

To add to the whole conversation, here's a full sequence diagram of how the flow works:

fcoury · April 16, 2023, 8:32pm

Just for some closure, I think I solved the issue. I went with a mix of what @semicoleon and @kpreid suggested, specially this:

Instead, you should own the components directly and pass all of them to the functions that need them

I ended up forking the library and removing the generic IO type. Instead, when I call the step() function there, I am now passing an &mut impl IO.

This was not without its challenges, as this method on the Bus struct was having an issue:

    pub fn step(&mut self) {
        self.cpu.step(self);
    }

The issue was this one below, and I think it's the true root cause of all the masked errors I was having before:

error[E0499]: cannot borrow `self.cpu` as mutable more than once at a time
   --> src/bus.rs:217:9
    |
217 |         self.cpu.step(self);
    |         ^^^^^^^^^----^----^
    |         |        |    |
    |         |        |    first mutable borrow occurs here
    |         |        first borrow later used by call
    |         second mutable borrow occurs here

Which is the distilled symptom of what @kpreid mentioned before:

You've constructed a circular reference, which will become a memory leak when the Machine is dropped.

It hides the dependencies that exist in the form of what data is manipulated by different function calls.

I ended up creating a wrapping struct:

pub struct BusWrapper<'a> {
    bus: &'a mut Bus,
}

impl<'a> Z80_io for BusWrapper<'a> {
    fn read_byte(&self, addr: u16) -> u8 {
        self.bus.read_byte(addr)
    }

    fn write_byte(&mut self, addr: u16, data: u8) {
        self.bus.write_byte(addr, data);
    }
}

Making the Z80 on the Bus into an Option<Z80>:

pub struct Bus {
    pub cpu: Option<Z80>,
    pub vdp: TMS9918,
    pub ppi: Ppi,
    pub psg: AY38910,

    slots: [SlotType; 4],
}

And taking the Z80 from the Bus when I need to mutate it:

    pub fn set_irq(&mut self, irq: bool) {
        if let Some(mut cpu) = self.cpu.take() {
            if irq {
                cpu.assert_irq(0);
            } else {
                cpu.clr_irq();
            }
            self.cpu = Some(cpu);
        } else {
            panic!("CPU not initialized");
        }
    }

Not sure if that's the most elegant approach, but it seems to be working. Granted, I spent so much time running after this that I ran out of time to start a fully integrated test, but will report back if I find any holes in this implementation.

Thank you once again @semicoleon, @quinedot and @kpreid for taking the time and your precious input.

system · July 15, 2023, 8:32pm

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.

Topic		Replies	Views
CPU emulator organization	6	293	February 19, 2025
How to borrow mutable reference of owner to method of owned object help	8	2340	January 12, 2023
Modeling embedded hardware in Rust and how to have multiple mutable references cleanly? help	16	2434	January 12, 2023
Best practices for a CPU emulation library? help	5	546	May 26, 2021
Rustifying a real world embedded example?	2	743	February 9, 2023

Modeling a Bus and its components in Rust emulator

Related topics