Add a builder to multiboot2

[YtvwlD]

Hey, I'm currently working on adding Multiboot 2 support to a bootloader (towboot) and while using the multiboot2 and multiboot2-header crates, I encountered some pieces that were missing.

I particular:

• I added more getters to multiboot2-header (mostly inspired by the ones in multiboot2).
• I added a builder to multiboot2 (inspired by multiboot2-header).
• I added the SMBIOS tag.
• I added a function to find a header in a given (file) slice.

This is mostly just boilerplate, but there were two bigger problems I had to work around:

1. The memory map is only really accurate after exiting Boot Services, but then the bootloader can't allocate anymore.

I've solved this by prefilling the memory map tags with empty entries and overwriting them later on. It seems to work, but it's not pretty.

2. Most tags are dynamically sized. The previous implementation had them fixed-size in Rust and let the content dangle behind them. I think this is a bit weird and it doesn't work when you try to allocate new structs.

I made them dynamically sized – which is worse to handle in Rust (you can't put them on the stack anymore).
In addition, since the ptr_metadata feature is currently unstable, I used the slice methods instead. (You can probably revert 800567f once it's stable).

I tried to adjust the tests and add some where appropriate. If that's not enough, I'll add more. :)

There's one test that fails for me on stable Rust (rustc 1.68.0 (2c8cc3432 2023-03-06)), though it works on nightly (rustc 1.70.0-nightly (900c35403 2023-03-08)).

I tried to keep the changes small. I think eg. that it would be nicer if Multiboot2Header::from_addr returned a Result, but I didn't change it

[phip1611]

Hi @YtvwlD
Thanks very much for working on this. I'm in big favor of the new functionality. Can you please rebase your changes onto the latest main branch? I refrain from merging any other breaking stuff, until we get this thing merged - to keep things simple.

I will have a more detailed look in the upcoming days.

[YtvwlD]

That's great to hear. :)

I've tried rebasing this on Monday, but it didn't compile afterwards, sadly. I hope to be able to do it this weekend.

[YtvwlD]

So, this took a bit longer than expected. :)
The one test sadly still only works with some compilers (though it seems to work on stable and fail on nightly in the CI, interesting).

I've removed cast_tag after the rebase because rustc doesn't seem to support generic casts over dynamically sized types? Here's what I was getting:

error[E0606]: casting `*const Tag` as `*const T` is invalid
   --> multiboot2/src/tag_type.rs:307:20
    |
307 |         unsafe { &*(self as *const Tag as *const T) }
    |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: vtable kinds may not match

(I've tried to put a minimal example in the playground.)

[phip1611]

I've removed cast_tag after the rebase because rustc doesn't seem to support generic casts over dynamically sized types?

Yes, because DSTs uses fat pointers. This is also an existing problem in the multiboot2 crate. I plan to refactor this eventually - If I find some time.. One can fix this with https://crates.io/crates/ptr_meta for example.

[phip1611]

But I', not sure how you can encounter this issue after you rebased your code. I think I didn't change anything in that regard

[YtvwlD]

cast_tag didn't exist before the rebase. Each getter had its own cast.

[phip1611]

Ah yes, totally right. I broke the API with that change... the right solution would be to remove cast_tag() and properly support DSTs... a quick fix for you might be to add something like find_tag next to get_tag that has the right semantics. This way, you could make your code compile.

[YtvwlD]

ah, I've just reverted the two commits that added it (the last two commits in this PR).

[YtvwlD]

The msrv fail is because alloc::ffi is only available since Rust 1.64. Any ideas on what to do about that?

[phip1611]

I started working on DSTs which should fix your

error[E0606]: casting `*const Tag` as `*const T` is invalid
   --> multiboot2/src/tag_type.rs:307:20
    |
307 |         unsafe { &*(self as *const Tag as *const T) }
    |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: vtable kinds may not match

problem. What do you think about my changes in #132 ? We could also merge your PR first and I adapt it then to my changes. But I'd like to hear if the new API would work in your case. I still hadn't much time to look into your code, unfortunately

The msrv fail is because alloc::ffi is only available since Rust 1.64. Any ideas on what to do about that?

I don't have anything against a MSVR bump, but I do not see a benefit in using this abstraction here. Multiboot2 strings and Rust strings are guaranteed to be valid UTF-8, CStrings are not. So I think it would be better to use regular Rust strings and push a final \0 byte onto the rust string. When the data is serialized (with .as_bytes()), the terminating null will be included.

PS: I don't quite understand at this point why your DST additions compile and why you do not have the fat pointer cast problem.

[YtvwlD]

I don't have any problems with fat pointers (anymore), because here, Tag is unsized, too. Casting Tag to eg. BootLoaderNameTag works out of the box. (Unless that cast is generic, then the vtable compiler error occurs.)

Casting raw bytes to a struct still requires explicit fat pointers, though (as can be seen in the tests for BootLoaderNameTag).

Having a from_base_tag method in a trait might be better than me just removing cast_tag, though.

I'd rebase this PR to keep using [u8] for strings then, but it'll probably make more sense for you to look into it, first?

[phip1611]

I'm on vacation right now, so I won't have much capacity for more review in the next 7 days.

But overall, the changes look good and I'm looking forward to merging them soon!

[YtvwlD]

Have fun. :)

[phip1611]

I've had time to review everything. I converted this MR as draft, as there are several points to address before getting this merged. I hope I do not make the contribution process for you too frustrating or so, but this PR is in its current state to big to pass the review process.

I'd prefer to have dedicated PRs for the following things:

• add missing functionality in multiboot2-header
  • find the header
  • add getter for the tags
  • that module align thingy
• add missing tags in multiboot2
  • add smbios tag
  • add basic memory info tag
• add multiboot2 MBI builder
• mutable references to the mbi (this one is controversial to me, so a dedicated MR where we can discuss)

I think, and as you already said, that the DST approach I took in #132 is more mature. Maybe you can have a look there and tell me if it would work with your contribution. Then we merge it and you can rebase your code onto those changes.

I'd highly appreciate if you can address (at least some of) my concerns and improvement suggestions. Otherwise, I'd assist you and do (some) of the work myself.

Don't get me wrong, U'm are very happy of merging the changes. But this PR is just to big.

PS: You live/are from germany? Wir können auch gerne in nem Video Call oder so bequatschen, wie wir hier am besten weiter machen :D

[YtvwlD]

Do we want a EFIMemoryDesc::new? I'm currently just transmuting the ones from the firmware into it (well, and checking the size), but I'm not sure if that's the right way.

[phip1611]

LGTM. Thanks! Please address that one single remark regarding the sections(n) method.

[phip1611]

Thanks for working on this. I'm glad we finally can merge it.

I address some open issues and then going to release it in a couple of days.

[YtvwlD]

Thanks, but I'd still like to talk about modifying the memory map. 🙃

[phip1611]

Via git bisecting, I could find that you might have broke something with commit d198dce

Since then, the debug print of the MBI doesn't work anymore and operates on wrong memory. I guess, some pointer magic is now wrong. I try to trace it down and create a unit test for it :)