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

mem: introduce traits MemoryMap and MemoryMapMut #1234

Merged
merged 7 commits into from
Jul 15, 2024

Conversation

phip1611
Copy link
Contributor

@phip1611 phip1611 commented Jul 8, 2024

In #1175 we lost the flexibility to use the memory map on any buffer. However, if you write a kernel and get the raw memory map from your bootloader, one has to parse the memory map from this raw memory. To solve this (leveraging the uefi crate), I introduced:

  • the traits MemoryMap and MemoryMapMut, where MemoryMapMut extends MemoryMap
  • the types MemoryMapRef, MemoryMapRefMut, and MemoryMapOwned, where the latter two implement MemoryMapMut

This helps to have a common API for all these use-cases. However, there are the
following open questions:

  • There is still code repetition. Especially for the implementations of Index and IndexMut which I have to provide 3 times. Any idea for improvement? Just use a macro?
  • I didn't manage to make the traits directly dependent on Index and IndexMut - I gave up after many compiler warnings.
  • What do you think about this in general?

Checklist

  • Sensible git history (for example, squash "typo" or "fix" commits). See the Rewriting History guide for help.
  • Update the changelog (if necessary)

@nicholasbishop
Copy link
Contributor

However, if you write a kernel and get the raw memory map from your bootloader, one has to parse the memory map from this raw memory.

Could the kernel take ownership of this memory and use the existing struct MemoryMap instead of a reference type?

@phip1611
Copy link
Contributor Author

phip1611 commented Jul 8, 2024

Don't think so (from a library standpoint) . Most abstraction for boot loader information provide you only &[u8] access, including but not limited to Limine (w/ Limine boot protocol) and multiboot2

@nicholasbishop
Copy link
Contributor

Only &[u8], or &mut [u8] too? (Wondering about MemoryMapRefMut)

@phip1611
Copy link
Contributor Author

phip1611 commented Jul 9, 2024

Only &[u8], or &mut [u8] too? (Wondering about MemoryMapRefMut)

Yes, there were actually requests to be able to modify structures, such as the memory map. For example in the multiboot2 repository. Limine's boot protocol, or to be more specific, the limine crate, also provides you with a slice/a buffer - this is however not mutable yet - but it could be extended in the future.

Note that we need mutable access so that one can call the .sort() function.

I'm not sure if the proposed solution is 100% perfect - but it enables all use cases users will have - giving developers maximum freedom.

What do you think about this? What about the other questions mentioned above?

@nicholasbishop
Copy link
Contributor

Makes sense. I haven't looked through the details of the PR yet (doing so now), I just wanted to make sure I understood how it would be used at a high level first.

@nicholasbishop
Copy link
Contributor

I didn't manage to make the traits directly dependent on Index and IndexMut - I gave up after many compiler warnings.

This seems to work:

pub trait MemoryMap: Debug + Index<usize, Output = MemoryDescriptor> {

(At least, it compiles, I didn't test it thoroughly.)

@nicholasbishop
Copy link
Contributor

There is still code repetition. Especially for the implementations of Index and IndexMut which I have to provide 3 times. Any idea for improvement? Just use a macro?

I think it's OK as-is, there is some code repetition but it's not too bad.

This provides API users all the flexibility they need. The main motivation for
the addition is that one can use a chunk of memory in a kernel (provided by a
bootloader) and parse it as EFI memory map. The main motivation for the specific
implementation (via traits) is code reduction.
@phip1611 phip1611 mentioned this pull request Jul 14, 2024
3 tasks
@phip1611
Copy link
Contributor Author

phip1611 commented Jul 14, 2024

I didn't manage to make the traits directly dependent on Index and IndexMut - I gave up after many compiler warnings.

This seems to work:

pub trait MemoryMap: Debug + Index<usize, Output = MemoryDescriptor> {

(At least, it compiles, I didn't test it thoroughly.)

I'll do/try this in a follow-up (#1240 ).

So, @nicholasbishop , what do you think? I know, I did a major overhaul of the MemoryType API in the past weeks. But I think it is worth it and that we now reached it a point where it is mature and sustainable. However, I'm also biased 😁 What do you think?

@nicholasbishop
Copy link
Contributor

I know, I did a major overhaul of the MemoryType API in the past weeks. But I think it is worth it and that we now reached it a point where it is mature and sustainable. However, I'm also biased 😁 What do you think?

I think it's a reasonable change, looks good to me. FWIW I don't think there's anything wrong with making more API changes after a recent API change if we realize that more improvements are needed. For me the important thing is that it's easy for users of the API to update their code when updating to a new version of uefi-rs. And certainly for these changes I don't think there's any problem there, it's very straightforward to change MemoryMap to MemoryMapOwned, and then potentially use the other new traits as needed.

@nicholasbishop nicholasbishop added this pull request to the merge queue Jul 15, 2024
Merged via the queue into rust-osdev:main with commit 380f98f Jul 15, 2024
12 checks passed
@phip1611 phip1611 deleted the mem branch July 15, 2024 05:35
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