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

alloc: initial TryBox implementation #196

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

00xc
Copy link
Member

@00xc 00xc commented Dec 27, 2023

Introduce the alloc submodule and the TryBox type, a fork of the upstream alloc crate and Box type respectively. The new type behaves exactly like Box, except it only supports fallible allocations, meaning that users must always handle potential errors.

Users must also explicitly set the allocator to use. Using a specific allocator by default like the standard library does is complicated. The standard library uses whichever allocator is set as global (via the #[global_allocator] macro), which can be accessed via a global instance (alloc::alloc::Global). However, the global instance is gated behind an unstable feature, and the macro cannot be reimplemented because it is a compiler intrinsic. As a workaround, one may add new higher level types in the future which use a specific allocator by default.

The new TryBox does not support unsized types (e.g. [T] pr dyn Trait), since that requires unstable features. On the other hand, it has a few extra methods for convenience like try_default_in(), try_clone() and try_clone_in().

This PR also includes a new type, GlobalBox, which uses the global SVSM allocator (SvsmAllocator), and is meant to be used as a replacement for the regular Box type.

TODO:

  • Implement SvsmAlloc for SvsmAllocator
  • Implement GlobalBox, a wrapper which uses the SvsmAllocator transparently.
  • Replace Box uses with GlobalBox. NOTE: there is a single instance that cannot be replaced, see comment below.

TODO in future PRs:

  • Introduce TryArc
  • Introduce TryRc
  • Introduce TryVec / RawVec
  • Introduce TryString
  • Introduce other collections (not used for now).

@00xc 00xc marked this pull request as draft December 27, 2023 11:24
@00xc
Copy link
Member Author

00xc commented Dec 27, 2023

For review I suggest taking a look at the public API via make doc, and diffing against upstream alloc (library/alloc/ in upstream Rust).

@p4zuu
Copy link
Collaborator

p4zuu commented Jan 3, 2024

Looks good to me at the moment. Just a few questions:

  • Do you have a plan for easy maintenance in the future? Eg. easy update when upstream alloc gets updated
  • Did you pull the boxed unit tests from upstream? Anyway, miri doesn't find issue with them :)

@joergroedel
Copy link
Member

Thanks for working in this, I think it is going into the right direction.

I particularly like that the box only supports fallible allocations, forcing users to handle allocation errors. This might actually become a reason to not switch back to the standard library once these features become stable there.

A few things I noticed:

  • The Allocator trait is defined in mod.rs, please move it to a separate file
  • The name SvsmBox is too much tied to the SVSM. With the planned move to cargo workspaces this code will likely end up in its own crate. Maybe name it TryBox or CheckedBox or something like that? The SVSM can then use it to define its own types with different backend allocators (Suggestion: Maybe defining an allocator-specific type can be done via a macro)

@00xc
Copy link
Member Author

00xc commented Jan 4, 2024

Looks good to me at the moment. Just a few questions:

  • Do you have a plan for easy maintenance in the future? Eg. easy update when upstream alloc gets updated

Not really, other than diffing I don't see much that can be done sadly. On the other hand, this code should not require heavy maintenance as we likely don't want many more features. One exception would be, as I mentioned in the PR description, the support for unsized types - when the required features are stabilized, we will likely want to add those.

  • Did you pull the boxed unit tests from upstream? Anyway, miri doesn't find issue with them :)

No, I wrote those to ensure that everything worked as expected, as there are some slight syntax changes I had to perform to avoid nightly features. Upstream does have tests but most rely on parts of the Box type that we do not support. At some point I'll revisit them to ensure that we use as much tests from upstream as possible.

@00xc
Copy link
Member Author

00xc commented Jan 4, 2024

A few things I noticed:

  • The Allocator trait is defined in mod.rs, please move it to a separate file

I simply placed in mod.rs everything that is not strictly pulled from upstream, which I think its not a bad practice in itself. Anyhow, I'll eventually move everything into a separate file and use mod.rs to replace upstream's lib.rs.

EDIT: technically I pulled the Allocator (i.e. SvsmAlloc) trait from the core library. I cannot import it normally because it is gated behind a nightly feature.

  • The name SvsmBox is too much tied to the SVSM. With the planned move to cargo workspaces this code will likely end up in its own crate. Maybe name it TryBox or CheckedBox or something like that? The SVSM can then use it to define its own types with different backend allocators (Suggestion: Maybe defining an allocator-specific type can be done via a macro)

Yeah the name change makes sense, I'll add it to the next version of the PR. For default allocator types, I think we can even use typedefs (but I have not tested it). Something like:

static ALLOCATOR: SvsmAllocator = SvsmAllocator::new();
// ...
pub type GlobalBox<T> = TryBox<T, A = ALLOCATOR>;

@00xc 00xc changed the title alloc: initial SvsmBox implementation alloc: initial TryBox implementation Jan 4, 2024
@00xc
Copy link
Member Author

00xc commented Jan 4, 2024

  • Renamed SvsmBox to TryBox
  • Renamed SvsmUnique to Unique (private type)
  • Renamed SvsmAlloc to Allocator.

@00xc 00xc force-pushed the alloc/fallible-alloc branch 3 times, most recently from 0335561 to 099c29b Compare January 4, 2024 11:33
@00xc
Copy link
Member Author

00xc commented Jan 4, 2024

static ALLOCATOR: SvsmAllocator = SvsmAllocator::new();
// ...
pub type GlobalBox<T> = TryBox<T, A = ALLOCATOR>;

Just verified that this does not really work. But we can do:

pub struct GlobalBox<T>(TryBox<T, &'static SvsmAllocator>);

impl<T> GlobalBox<T> {
    pub fn try_new(val: T) -> Result<Self, SvsmError> {
        let inner = TryBox::try_new_in(val, &ALLOCATOR)?;
        Ok(Self(inner))
    }
}

I'll add something like this to this PR soon.

@00xc 00xc marked this pull request as ready for review January 24, 2024 11:10
@00xc 00xc marked this pull request as draft January 24, 2024 11:10
@00xc 00xc force-pushed the alloc/fallible-alloc branch 2 times, most recently from 488dcbe to 5aea5c2 Compare January 24, 2024 13:54
@00xc
Copy link
Member Author

00xc commented Jan 25, 2024

There is currently an use of the old Box in the Mapping struct in the mm/vm/mapping code that cannot be replaced:

pub struct Mapping {
    mapping: RWLock<Box<dyn VirtualMapping>>,
}

This is because dyn VirtualMapping is unsized, and TryBox does not work with those, as it requires unstable features to implement that support. There are some potential reworks that could be done to the virtual mapping code to avoid using an unsized item, such as using enums instead of dynamic dispatch, but for the purposes of this PR I'll leave this Box use unchanged

@00xc 00xc force-pushed the alloc/fallible-alloc branch 5 times, most recently from 8404a0b to 427695b Compare January 31, 2024 13:50
@00xc 00xc marked this pull request as ready for review January 31, 2024 13:57
@00xc 00xc force-pushed the alloc/fallible-alloc branch 3 times, most recently from 901c1f1 to 0d1a197 Compare February 1, 2024 14:04
@00xc
Copy link
Member Author

00xc commented Feb 8, 2024

There is currently an use of the old Box in the Mapping struct in the mm/vm/mapping code that cannot be replaced:

pub struct Mapping {
    mapping: RWLock<Box<dyn VirtualMapping>>,
}

This is because dyn VirtualMapping is unsized, and TryBox does not work with those, as it requires unstable features to implement that support.

I've been digging into this and we might actually be able to get it working for unsized types. The main issue to replace the use above (as far as I can tell) is that CoerceUnsized is unstable. This means that TryBox<T, A> cannot be transparently coerced into TryBox<dyn Trait, A>.

To solve this, I've come up with a macro that will do this for us. With regular coercion, we could simply do:

trait MyTrait {}
impl MyTrait for usize {}

let boxed = Box::new(5usize);
let dynamic: Box<dyn MyTrait> = boxed;

But we need to do:

trait MyTrait {}
impl MyTrait for usize {}

let boxed = TryBox::try_new_in(5usize, Allocator)?;
let dynamic = trybox_upcast!(boxed, MyTrait);

@00xc 00xc force-pushed the alloc/fallible-alloc branch 3 times, most recently from cd3b6ee to f7e59cc Compare February 8, 2024 13:40
@00xc
Copy link
Member Author

00xc commented Feb 8, 2024

  • Unsized types now work.
  • Replaced the last remaining use of Box with GlobalBox.
  • Rebased on top of the workspace changes.

@00xc 00xc force-pushed the alloc/fallible-alloc branch 2 times, most recently from 4f6732e to d4f883b Compare February 9, 2024 09:00
@joergroedel joergroedel self-requested a review February 13, 2024 12:45
@joergroedel joergroedel added the wait-for-review PR needs for approval by reviewers label Feb 13, 2024
@joergroedel joergroedel self-assigned this Feb 16, 2024
@00xc
Copy link
Member Author

00xc commented Feb 19, 2024

Added a new test and rebased on main.

@00xc 00xc force-pushed the alloc/fallible-alloc branch 2 times, most recently from e69750a to 6964b6e Compare February 21, 2024 13:27
Introduce the alloc submodule and the TryBox type, a fork of the
upstream alloc crate and Box type respectively. The new type behaves
exactly like Box, except it only supports fallible allocations,
meaning that users must always handle potential errors.

Users must also explicitly set the allocator to use. Using a specific
allocator by default like the standard library does is complicated.
The standard library uses whichever allocator is set as global
(via the #[global_allocator] macro), which can be accessed via a
global instance (alloc::alloc::Global). However, the global instance
is gated behind an unstable feature, and the macro cannot be
reimplemented because it is a compiler intrinsic. As a workaround,
one may add new higher level types in the future which use a specific
allocator by default.

The new type also has a few extra methods for convenience like
try_default_in(), try_clone() and try_clone_in().

Signed-off-by: Carlos López <carlos.lopez@suse.com>
In order to be used along with the new TryBox type, an allocator must
implement the Allocator trait, so implement it for the current global
allocator.

Signed-off-by: Carlos López <carlos.lopez@suse.com>
Add a new variant to hold a TryAllocError from the alloc submodule.
This new error does not implement Copy, so SvsmError cannot implement
it either.

Signed-off-by: Carlos López <carlos.lopez@suse.com>
Add a new type, GlobalBox, which wraps around the TryBox type but uses
the global SvsmAllocator transparently. The new type implements
AsRef<TryBox> and Into<TryBox>, so regular TryBox methods can also be
used on the new type.

Signed-off-by: Carlos López <carlos.lopez@suse.com>
Allocate PageTablePart structs with the new fallible-allocation-aware
GlobalBox type.

Signed-off-by: Carlos López <carlos.lopez@suse.com>
Use the new fallible-allocation-aware GlobalBox type when allocating
SNP request messages.

Signed-off-by: Carlos López <carlos.lopez@suse.com>
Replace uses of Box with the new fallible-allocation-aware GlobalBox
type. Unfortunately, replacing the Box used in the RB tree for
virtual mappings is very involved.

In short, the intrusive_collections crate provides an
intrusive_adapter macro, which generates a new adapter type that
handles the insertion and deletion of any type into an intrusive
collection. Sadly, this macro is only prepared to work with the smart
pointer types in the standard library. This is done by implementing
the PointerOps trait for DefaultPointerOps<T>, where T is one of the
regular smart pointer types (Rc, Arc, Box, etc.). The macro then
generates an implementation of Adapter for the newly generated type,
which uses the selected smart pointer.

We define a substitute for DefaultPointerOps, CustomPointerOps, and
implement PointerOps for CustomPointerOps<GlobalBox>. We then add a
manual implementation of Adapter for VMMAdapter, which uses GlobalBox
under the hood.

Signed-off-by: Carlos López <carlos.lopez@suse.com>
Replace uses of Box in the mapping API with the new
fallible-allocation-aware GlobalBox type.

Signed-off-by: Carlos López <carlos.lopez@suse.com>
Use the new fallible-allocation-aware GlobalBox type when allocating
a new secrets page for the given VPML.

Signed-off-by: Carlos López <carlos.lopez@suse.com>
@00xc
Copy link
Member Author

00xc commented Feb 26, 2024

Rebased on main and fixed conflicts

@00xc 00xc marked this pull request as draft July 4, 2024 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wait-for-review PR needs for approval by reviewers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants