Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add pci I/O bar access and use bitfields #825

Draft
wants to merge 1 commit into
base: theseus_main
Choose a base branch
from

Conversation

hecatia-elegua
Copy link
Contributor

  • edition update/clippy as well

I added some TODOs because I'm not done with pci, will do more PRs. I actually stated a question there too: Do we / does the device author always know what kind of address a determine_mem_base call returns?
I can look that up in a pci spec, just asking if anybody knows.

* edition update/clippy as well
Copy link
Member

@kevinaboos kevinaboos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, i like this PR, but am concerned that the representation of the PCI BAR is just way too complex with the multiple enums and bitfields.

Given the fact that the register layout is so dynamic (e.g., address field changes width and type based on bit 0), I think a design like this would be much simpler (pseudocode):

/// A PCI Base Address Register (BAR) ... finish docs
#[repr(transparent)]
pub struct BaseAddressRegister(u32);
impl BaseAddressRegister {
    
    /// TODO: here, add const defs for the various magic number

    /// Docs here
    pub fn get(&self) -> PciBar {
        match self.0 & 0x1 {
            0 => {
                let typ = self.0.get_bits(1..=2);
                if typ != 0x0 {
                    panic!("PCI BAR of type {typ:#X} is is unsupported")
                }
                PciBar::MemorySpace {
                    base_addr: PhysicalAddress::new_canonical((self.0 >> 4) as usize),
                    prefetchable: self.get_bit(3),
                }
            }
            1 => PciBar::IoSpace(self.0 >> 2)
        }
    }
}

/// Docs here
pub enum PciBar {
    /// Docs here
    MemorySpace {
        base_addr: PhysicalAddress,
        /// If `true`, reading from this PCI memory can be cached,
        /// and should thus be mapped as write-through, not uncacheable.
        prefetchable: bool,
    }
    /// Docs here
    IoSpace {
        addr: u32,
    }
}

and then the PciDevice struct would use the BaseAddressRegister type in its bars field.


//TODO: incorporate this somewhere else
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this type can now be deleted since you've replaced it with the two enums

@@ -420,7 +421,7 @@ impl PciDevice {

self.pci_write(bar_offset, 0xFFFF_FFFF); // Step 1
let mut mem_size = self.pci_read_32(bar_offset); // Step 2
mem_size.set_bits(0..4, 0); // Step 3
mem_size &= 0xFFFFFFF0; // Step 3
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this better? I don't necessarily think it's any clearer

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is temporary since I wanted to remove the bitfield crate and this was the only place left

@kevinaboos
Copy link
Member

To answer your question:

Do we / does the device author always know what kind of address a determine_mem_base call returns?

Yes, but we should verify that by actually checking the PCI BAR content. Your approach in the network device drivers of returning an error if it's not a memory space BAR is correct.

@hecatia-elegua hecatia-elegua marked this pull request as draft March 27, 2023 17:56
@hecatia-elegua
Copy link
Contributor Author

Thanks for the comments, I'll think about this some more when I'm stuck on my main tasks.
I do think that your version is more readable, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants