VGA console output implementation. #65
Conversation
mythril_core/src/lib.rs
Outdated
| @@ -29,6 +29,7 @@ mod registers; | |||
| pub mod time; | |||
| pub mod tsc; | |||
| pub mod vcpu; | |||
| pub mod vga_logger; | |||
There was a problem hiding this comment.
Hmm, I don't think I want another top level module for this. We should probably ultimately make a 'logger' module that has a 'serial_logger' and 'vga_logger' under it. But that may be too much for this PR. For now, just put it in the existing 'logger' file, I think.
mythril_core/src/logger.rs
Outdated
|
|
||
| pub fn write_console(s: impl AsRef<str>) { | ||
| pub fn write_console(s: &str) { |
There was a problem hiding this comment.
Why change this from AsRef?
There was a problem hiding this comment.
accidentally left this in here, meant to change it back to impl AsRef
mythril_core/src/logger.rs
Outdated
| pub unsafe fn raw_write_console(s: impl AsRef<str>) { | ||
| pub unsafe fn raw_write_console(s: &str) { | ||
| // mirror console output to VGA | ||
| let mut vga_writer = VGA_LOCK.lock(); |
There was a problem hiding this comment.
I think the issue here is that if we panic while we hold the VGA lock, then the panic handler will try to acquire the lock again and we will deadlock
mythril_core/src/vga_logger.rs
Outdated
| } | ||
|
|
||
| pub unsafe fn raw_write_vga(s: &str, x: usize, y: usize) -> (usize, usize) { | ||
| let mut x = x; |
There was a problem hiding this comment.
I think you could just put the mut in the signature as mut x: usize
mythril_core/src/vga_logger.rs
Outdated
| } | ||
|
|
||
| fn write_fmt(&mut self, args: fmt::Arguments) -> Result<(), fmt::Error> { | ||
| let ret = fmt::write(self, args); |
There was a problem hiding this comment.
This can just be fmt::write(self, args) (rather than storing then returning ret)
mythril_core/src/vga_logger.rs
Outdated
| let mut y = y; | ||
|
|
||
| for byte in s.bytes() { | ||
| // move cursor on newlines (0x0A) and carriage-returns (0x0D) |
There was a problem hiding this comment.
Is this the expected behavior? That newline does a newline and carriage return, while carriage return just goes to the start of the line? Is this from an existing implementation?
There was a problem hiding this comment.
Since the rust toolchain uses linux-style line returns for writeln!, we need to both do a newline and a carriage return so that the lines don't start going off diagonally on the screen. I've added in the check for just carriage returns as well just in case this was built on windows or if a log statement used a carriage return for something like a loading bar.
mythril_core/src/vga_logger.rs
Outdated
| const VGA_HEIGHT: usize = 25; | ||
| const VGA_ATTRIB: u16 = 0x0F00; // black background, white text | ||
|
|
||
| fn get_vga_addr(x: usize, y: usize) -> usize { |
There was a problem hiding this comment.
Rather than doing this, I think we should probably have the writer store a &[[u16; VGA_WIDTH]; VGA_HEIGHT] (I may have that backwards). And index in to that. Then we just need to do one unsafe conversion from 0xB8000 to that array ref, rather than using raw pointers everywhere.
There was a problem hiding this comment.
I wasn't able to create a direct reference in the VgaWriter struct itself due to this issue: rust-lang/rust#57349
Instead, i've made it so that the only raw pointer deref occurs in the write method which then passes the reference to the vga functions.
barkera
force-pushed
the
vga_output
branch
2 times, most recently
from
6f0b037
to
93aa009
Compare
- Add VgaWriter struct to handle text output to the VGA text buffer. - Add raw_write_vga function and support functions for writing directly to the VGA text buffer. - Add VGA_WRITER static variable to logger module for handling outputting logs to the vGA text buffer.
|
Sorry for the slow review of this. Looks good to me, thanks! |
fixes #43