Trying to integrate network stack, failing to get interrupts#1
Trying to integrate network stack, failing to get interrupts#1EthanLavi wants to merge 38 commits into
Conversation
jacob-greenfield
left a comment
jacob-greenfield
left a comment
There was a problem hiding this comment.
This is a massive amount of code to review all at once. As the first order of business could you run cargo fmt and try to go through some of the warnings? I'm seeing some actual logic bugs that were caught by the warnings.
| impl Bytefield16 { | ||
| // Create a bytefield and swap endian-ness | ||
| pub fn new(val: u16) -> Self { | ||
| Self { data: [(val >> 1 * 8 & 0xFF) as u8, (val >> 0 * 8 & 0xFF) as u8] } |
There was a problem hiding this comment.
This gives an error for me locally, I think the order of operations is wrong. I really think it would make more sense to use an established library for things like this whenever possible rather than reinventing the wheel (which is error-prone and can sometimes lead to code duplication). In fact, Rust already has built-in methods to swap the endianness of a number, and to get the individual bytes. See e.g. u32::from_be_bytes, u32::to_be_bytes, u32::swap_bytes, etc.
I do realize there are similar structs for widths that don't exist in Rust (e.g. 48-bit) but could you potentially try to find a library that does this?
There was a problem hiding this comment.
No... it's pretty trivial to test this. It also is super concise in parsing a byte vector, since I can just chain together read_inc calls, which I doubt I will find in a library. It's also already built, why replace something that is already working?
There was a problem hiding this comment.
How about this instead?:
use core::ops::{Index, IndexMut};
// N.B.: Bytefields STORE IN BIG ENDIAN (as per network requirements)
#[derive(Debug, Clone, Copy)]
pub struct Bytefield<const N: usize> {
pub data: [u8; N],
}
impl<const N: usize> Bytefield<N> {
pub fn swapped_endianness(self) -> Self {
let mut data = self.data;
data.reverse();
Self { data }
}
pub fn read_inc(bytevec: &[u8], i: &mut usize) -> Self {
let mut data = [0u8; N];
for i in 0..N {
data[i] = bytevec[N - 1 - i];
}
*i += N;
Self { data }
}
pub const fn size() -> usize {
N
}
}
impl<const N: usize> Index<usize> for Bytefield<N> {
type Output = u8;
fn index(&self, i: usize) -> &u8 {
&self.data[i]
}
}
impl<const N: usize> IndexMut<usize> for Bytefield<N> {
fn index_mut(&mut self, i: usize) -> &mut u8 {
&mut self.data[i]
}
}
macro_rules! bytefield_int {
($t:ident, $int:ident, $size:literal) => {
pub type $t = Bytefield<$size>;
impl $t {
pub fn new(val: $int) -> Self {
Self {
data: val.to_be_bytes(),
}
}
pub fn val(&self) -> $int {
$int::from_be_bytes(self.data)
}
}
};
}
bytefield_int!(Bytefield8, u8, 1);
bytefield_int!(Bytefield16, u16, 2);
bytefield_int!(Bytefield32, u32, 4);
bytefield_int!(Bytefield64, u64, 8);
bytefield_int!(Bytefield128, u128, 16);
pub type Bytefield48 = Bytefield<6>;
impl Bytefield48 {
pub fn new(val: u64) -> Self {
assert!(val < (1 << 48));
let bytes64 = val.to_be_bytes();
Self {
data: [
bytes64[2], bytes64[3], bytes64[4], bytes64[5], bytes64[6], bytes64[7],
],
}
}
pub fn val(&self) -> u64 {
u64::from_be_bytes([
0,
0,
self.data[0],
self.data[1],
self.data[2],
self.data[3],
self.data[4],
self.data[5],
])
}
}| pub static PICS: spin::Mutex<ChainedPics> = | ||
| spin::Mutex::new(unsafe { ChainedPics::new(PIC_1_OFFSET, PIC_2_OFFSET) }); | ||
| pub static PICS: spin::Mutex<ChainedPics> = spin::Mutex::new({ | ||
| unsafe { ChainedPics::new(PIC_1_OFFSET, PIC_2_OFFSET) } |
There was a problem hiding this comment.
IDK why you are commenting here, this just seems like something that got auto-formatted?
| if irq_num < 8 { | ||
| unsafe { PICS.lock().write_masks(data[0] & !(1 << irq_num), data[1]) }; | ||
| } else { | ||
| unsafe { PICS.lock().write_masks(data[0], data[1] & !(1 << irq_num - 8)) }; |
There was a problem hiding this comment.
Why do you lock more than once? Seems unnecessary and could introduce a race condition once we're multi-core. Perhaps these methods should take &mut self, and the caller is responsible for locking?
| let phys_mem_offset = VirtAddr::new(boot_info.physical_memory_offset.into_option().unwrap()); | ||
| let mut mapper = unsafe { memory::init(phys_mem_offset) }; | ||
| let phys_mem_offset = boot_info.physical_memory_offset.into_option().unwrap(); | ||
| let mut mapper = unsafe { memory::init(VirtAddr::new(phys_mem_offset)) }; |
There was a problem hiding this comment.
I'm confused about the rationale behind some of these changes?
There was a problem hiding this comment.
Calling a VirtAddr a phys_mem_offset seemed wrong. In addition, I needed to use the physical memory offset later, so I decided it would be cleaner to construct the virt addr inline with memory::init so I can reuse phys_mem_offset in the NET_INFO.init
| volatile uint8_t status; | ||
| volatile uint8_t errors; | ||
| volatile uint16_t special; | ||
| } __attribute__((packed)); |
There was a problem hiding this comment.
#[repr(C, packed)]???? I don't think this will build
There was a problem hiding this comment.
It won't... I didn't mean to commit this file. It's a driver for a different device that I no longer want to include.
| } | ||
| } | ||
|
|
||
| struct WrappedU16 { |
There was a problem hiding this comment.
What is the purpose of this
| let mut res = !sum as u16; | ||
| // Swap the bytes because we did our sum in big endian | ||
| // (and the bytefield will try to convert to big endian) | ||
| res = ((res >> 8) & 0xFF) | ((res & 0xFF) << 8); |
There was a problem hiding this comment.
See my other comment about the built-in endianness functions
|
Saw some of your new changes, looks great! I truly truly hate to be this person but could you potentially try to begin refactoring on top of |
2 participants
Instead of pushing everything I have, thought it might be easier to make a pull request...
Also, I made slight changes to interrupts.rs that you might want to look over (it was working before switching over to the bootloader, but I'm wondering if there are some changes in the GDT that affects what I changed the IDT code to be)
So once you can ensure interrupts are working, we'll have to do some more integration testing!