Introduce safe page allocation abstractions

[00xc]

• Introduce a PageBox<T> type. It works like Box<T>, releasing memory on drop, but it only has a subset of the functionality and uses memory obtained directly from the page allocator. This is useful for some types that need to reside on a page of their own.
• Introduce a VmsaPage abstraction based on the PageBox type.
• Introduce a HVDoorbellPage abstraction based on the PageBox type.
• Introduce a GhcbPage abstraction based on the PageBox type.
• Update the PerCpu code to use the PageBox type.
• Update the pagetable code to use the PageBox type.

[msft-jlange]

Is your change compatible with gdbstub? The gdb support requires borrowing the GHCB in order to step in the debugger, but the debugger may be stepping through code that already has the GHCB borrowed, which would cause debugger actions to panic. I'm also concerned about the possibility that somebody tries to add logging to code that has an active GHCB borrow, which would cause a similar panic because the console code will attempt to use the GHCB in order to communicate with the console. While this is safe, it is not exactly desirable to make logging impossible in certain code paths.

I started (but did not complete) a nested GHCB borrow design in which GHCB borrowing had a "level" concept, in which each borrow of the GHCB declared an intended nesting level. Three levels were defined: normal, console, and debugger. The premise was that recursive borrows were illegal at a level that was the same or lower than any current borrow, but a recursive borrow was legal at a higher level. The reference tracker (equivalent of Ref or RefMut) would contain a separate copy of the GHCB, populated at the time of the recursive borrow, and copied back to the GHCB page at the time the nested borrow is dropped. This creates a safe recursive borrow for the console and debugger cases described above.

As I said, I never finished that work because it seemed like a lower priority than other core logic, but I could pick it up and try to finish it if necessary. But I would be really concerned about any intermediate change that breaks debugger/console functionality in the pursuit of borrow safety.

[00xc]

Is your change compatible with gdbstub? The gdb support requires borrowing the GHCB in order to step in the debugger, but the debugger may be stepping through code that already has the GHCB borrowed, which would cause debugger actions to panic. I'm also concerned about the possibility that somebody tries to add logging to code that has an active GHCB borrow, which would cause a similar panic because the console code will attempt to use the GHCB in order to communicate with the console. While this is safe, it is not exactly desirable to make logging impossible in certain code paths.

I have not tested this, but it looks like it would interfere, yes.

I started (but did not complete) a nested GHCB borrow design in which GHCB borrowing had a "level" concept, in which each borrow of the GHCB declared an intended nesting level. Three levels were defined: normal, console, and debugger. The premise was that recursive borrows were illegal at a level that was the same or lower than any current borrow, but a recursive borrow was legal at a higher level. The reference tracker (equivalent of Ref or RefMut) would contain a separate copy of the GHCB, populated at the time of the recursive borrow, and copied back to the GHCB page at the time the nested borrow is dropped. This creates a safe recursive borrow for the console and debugger cases described above.

I'm not sure I understand how this would work. If the contents are only copied on drop, then what will we pass to the GHCB method that performs the VMGEXIT? All of these methods (e.g. GHCB::wrmsr()) take a &mut self, but that cannot be done if the reference is dropped. Hope I'm not completely missing the point.

I would have assumed that the new RefMut-like type would need to register its GHCB copy via the GHCB MSR on acquisition. Then the VMGEXIT would work on the new copy, and the MSR could be restored on drop (avoiding the need to copy the GHCB page contents back).

As I said, I never finished that work because it seemed like a lower priority than other core logic, but I could pick it up and try to finish it if necessary. But I would be really concerned about any intermediate change that breaks debugger/console functionality in the pursuit of borrow safety.

I can try to implement something like that if you want.

[00xc]

It just dawned on me that you perhaps meant that the new RefMut-like type would still operate on the original GHCB page, but it would use its copy as a snapshot. I think that could work, but if I understand correctly, this opens a window for the following:

ref1 = ghcb_mut(Level::Normal);    // Takes snapshot 0
ref2 = ghcb_mut(Level::Debugger);  // Takes snapshot 1

ref1.some_field = some_value;      // Some modification to the GHCB

drop(ref2);                        // Restores to snapshot 1

ref1.some_call()                   // GHCB has unexpected state, ref1 modification is lost

I'm not claiming this is necessarily a very common pattern, but one that may happen, making the abstraction somewhat unsafe.

[00xc]

It just dawned on me that you perhaps meant that the new RefMut-like type would still operate on the original GHCB page, but it would use its copy as a snapshot. I think that could work, but if I understand correctly, this opens a window for the following
...

It would also have some usability issues. If the new RefMut-like type implements DerefMut<GHCB>, nothing is stopping us from doing the following, which is unsound:

let ghcb = ghcb_mut(Level::Normal)?;
let some_ref: &mut GHCB = ghcb.deref_mut();
log::info() {
    // call some GHCB method that takes `&mut self`
    ghcb_mut(Level::Console)?.some_call();
}
some_ref.some_call()?;

On the other hand, if the new type does not implement DerefMut there is no straightforward way to call GHCB methods. We would have to do something like the following to scope the lifetime of the mutable reference:

let ghcb = ghcb_mut(Level::Normal)?;
ghcb.with_mut(|ghcb| ghcb.some_call())?;

Which would still not allow using the GHCB while GHCB::some_call() is running, as it takes &mut self, and creating another reference is undefined behavior.

[00xc]

For now I'm tempted to drop the last commit on this PR so this can be merged straight away.

[msft-jlange]

It just dawned on me that you perhaps meant that the new RefMut-like type would still operate on the original GHCB page, but it would use its copy as a snapshot. I think that could work, but if I understand correctly, this opens a window for the following:
...
I'm not claiming this is necessarily a very common pattern, but one that may happen, making the abstraction somewhat unsafe.

I take your point, and I don't have any alternative suggestions. I suspect this is a case where consumers of the GHCB will almost certainly do the right thing and we can accept the risk, because the pattern you are concerned about is one that looks like it is deliberately trying to do something very strange, and therefore is not one we should ever expect to see. Yes, it's unfortunate if the compiler can't stop this pattern from occurring, but I think we can be OK with that.

[msft-jlange]

It would also have some usability issues. If the new RefMut-like type implements DerefMut<GHCB>, nothing is stopping us from doing the following, which is unsound:

let ghcb = ghcb_mut(Level::Normal)?;
let some_ref: &mut GHCB = ghcb.deref_mut();
log::info() {
    // call some GHCB method that takes `&mut self`
    ghcb_mut(Level::Console)?.some_call();
}
some_ref.some_call()?;

But that's exactly the pattern we need to support: some outer block acquires a mutable reference to the GHCB and some inner block temporarily acquires a nested reference. In your example, the inner block will drop the nested reference before the outer block attempts to reuse its reference, so the nesting should be accomplished successfully - meaning that the inner block will make a temporary copy in its local RefMut, then the drop will put that data back into the GHCB page, and the outer block will continue based on the data that it holds. What is about this that is unsound? It should certainly be the case that any contents of the GHCB page will be identical both before and after the inner block so the outer block won't be able to observe that any temporary copies were made.

[msft-jlange]

For now I'm tempted to drop the last commit on this PR so this can be merged straight away.

That's very reasonable, but now I want to raise a separate concern. The HV doorbell page contains no mutable data, and thus no mutable reference can ever be acquired (we don't enforce that today, but we should). If no mutable reference can ever be acquired, then there are no constraints on the number of shared references that can be acquired. Since there is no constraint, it seems like we would not want to implement any dynamic logic to enforce borrow checking because it can never fail (by construction) and the borrow checking would be wasted effort. Wouldn't it be better if we just introduced a function that returns &'static HVDoorbell and avoid the need for borrow checking?

[00xc]

I take your point, and I don't have any alternative suggestions. I suspect this is a case where consumers of the GHCB will almost certainly do the right thing and we can accept the risk, because the pattern you are concerned about is one that looks like it is deliberately trying to do something very strange

Not necessarily. This could happen by attaching and detaching with the debugger IIUC.

and therefore is not one we should ever expect to see. Yes, it's unfortunate if the compiler can't stop this pattern from occurring, but I think we can be OK with that.

I think we can enforce this at the compiler level by making any subsequent reference's lifetime depend on the previous one (e.g. something like let r2 = r1.borrow()). I have implemented a small PoC of this but I have to test it properly, plus it's hard to add into the SVSM because the PerCpu cannot store the GHCB and something that references the GHCB (the RefMut-like type), since it makes the PerCpu self-referential (I think). Perhaps this can be done with the Pin type, but I haven't gotten around to trying it.

It would also have some usability issues. If the new RefMut-like type implements DerefMut<GHCB>, nothing is stopping us from doing the following, which is unsound:

let ghcb = ghcb_mut(Level::Normal)?;
let some_ref: &mut GHCB = ghcb.deref_mut();
log::info() {
    // call some GHCB method that takes `&mut self`
    ghcb_mut(Level::Console)?.some_call();
}
some_ref.some_call()?;

But that's exactly the pattern we need to support: some outer block acquires a mutable reference to the GHCB and some inner block temporarily acquires a nested reference. In your example, the inner block will drop the nested reference before the outer block attempts to reuse its reference, so the nesting should be accomplished successfully - meaning that the inner block will make a temporary copy in its local RefMut, then the drop will put that data back into the GHCB page, and the outer block will continue based on the data that it holds. What is about this that is unsound? It should certainly be the case that any contents of the GHCB page will be identical both before and after the inner block so the outer block won't be able to observe that any temporary copies were made.

It is still technically undefined behavior as far as I understand. I don't think in practice it would cause much of a problem in the current state of the compiler, but then again I'm not a compiler expert and it's still not allowed. The following snippet won't build for example:

fn main() {
    let mut v = 5u32;

    let r1 = &mut v;
    *r1 += 1;
    {
        let r2 = &mut v;
    }
    println!("{}", *r1);
}

error[E0499]: cannot borrow `v` as mutable more than once at a time
 --> src/main.rs:7:18
  |
4 |     let r1 = &mut v;
  |              ------ first mutable borrow occurs here
...
7 |         let r2 = &mut v;
  |                  ^^^^^^ second mutable borrow occurs here
8 |     }
9 |     println!("{}", *r1);
  |                    --- first borrow later used here

If you try going around the compiler with something like an UnsafeCell and run the program with Miri you'll see it spills an error as well.

For now I'm tempted to drop the last commit on this PR so this can be merged straight away.

That's very reasonable, but now I want to raise a separate concern. The HV doorbell page contains no mutable data, and thus no mutable reference can ever be acquired (we don't enforce that today, but we should). If no mutable reference can ever be acquired, then there are no constraints on the number of shared references that can be acquired. Since there is no constraint, it seems like we would not want to implement any dynamic logic to enforce borrow checking because it can never fail (by construction) and the borrow checking would be wasted effort. Wouldn't it be better if we just introduced a function that returns &'static HVDoorbell and avoid the need for borrow checking?

Yes, that makes sense, I will change hv_doorbell_unsafe() so that it returns an immutable reference and I'll update the function name.

[msft-jlange]

But that's exactly the pattern we need to support: some outer block acquires a mutable reference to the GHCB and some inner block temporarily acquires a nested reference. In your example, the inner block will drop the nested reference before the outer block attempts to reuse its reference, so the nesting should be accomplished successfully - meaning that the inner block will make a temporary copy in its local RefMut, then the drop will put that data back into the GHCB page, and the outer block will continue based on the data that it holds. What is about this that is unsound? It should certainly be the case that any contents of the GHCB page will be identical both before and after the inner block so the outer block won't be able to observe that any temporary copies were made.

It is still technically undefined behavior as far as I understand. I don't think in practice it would cause much of a problem in the current state of the compiler, but then again I'm not a compiler expert and it's still not allowed. The following snippet won't build for example:

Thinking through this further, I cannot prove that the compiler cannot introduce badly timed spills which upset the nesting design. This makes me wonder whether a better approach would be to reimplement the GHCB object so that all attempts to operate on it use shared references, and the reference to the underlying data is managed internally as an unsafe mutable pointer. This would eliminate any possibility that a GHCB might be borrowed while any mutable reference is outstanding. It would mean that the GHCB implementation would have to track its own nesting state (which likely would require that all calls specify a nesting level to ensure that the requested operation is safe to attempt). Internal state could be stored as atomics for ergonomics, and access to the mutable pointers could be gated on whether the atomic accesses that push/pop nesting state can permit the operation to proceed. This seems to me like it would require fewer contortions.

[00xc]

But that's exactly the pattern we need to support: some outer block acquires a mutable reference to the GHCB and some inner block temporarily acquires a nested reference. In your example, the inner block will drop the nested reference before the outer block attempts to reuse its reference, so the nesting should be accomplished successfully - meaning that the inner block will make a temporary copy in its local RefMut, then the drop will put that data back into the GHCB page, and the outer block will continue based on the data that it holds. What is about this that is unsound? It should certainly be the case that any contents of the GHCB page will be identical both before and after the inner block so the outer block won't be able to observe that any temporary copies were made.

It is still technically undefined behavior as far as I understand. I don't think in practice it would cause much of a problem in the current state of the compiler, but then again I'm not a compiler expert and it's still not allowed. The following snippet won't build for example:

Thinking through this further, I cannot prove that the compiler cannot introduce badly timed spills which upset the nesting design. This makes me wonder whether a better approach would be to reimplement the GHCB object so that all attempts to operate on it use shared references, and the reference to the underlying data is managed internally as an unsafe mutable pointer.

This could also be done by replacing GHCB fields with Cells, since they all implement Copy (I think) and Cell<T> is guaranteed to have the same layout as T.

This would eliminate any possibility that a GHCB might be borrowed while any mutable reference is outstanding. It would mean that the GHCB implementation would have to track its own nesting state (which likely would require that all calls specify a nesting level to ensure that the requested operation is safe to attempt). Internal state could be stored as atomics for ergonomics, and access to the mutable pointers could be gated on whether the atomic accesses that push/pop nesting state can permit the operation to proceed. This seems to me like it would require fewer contortions.

Yes, or Cell could be used too instead of atomics. It's what I used in my PoC.

[00xc]

We continued the nested reference discussion via email. I think all review comments are addressed, so this should be good to go.

[00xc]

This seems to fail booting in debug mode, I'll take a look. Please don't merge yet.

[00xc]

So this is very messy. As-is, the SVSM works with this PR. The thing is, once I introduce the nested protection the SVSM panics early. This is because the new NestedRef type keeps a reference on the PerCpu, which itself is also reentrant and uses a RefCell, which causes it to panic when a mutable borrow is attempted. So this will require a greater rework.

[00xc]

Added some extra documentation in the latest version of the PR. I will merge this by the end of the day if nobody has any objections.