feat: prepare for enarx

[jarkkojs]

No description provided.

[npmccallum]

@jarkkojs Don't request a review until tests pass.

[jarkkojs]

@npmccallum: it's the same 1.56.0 tests as always. Should I never request a review?

[npmccallum]

@jarkkojs No. You should either fix your code or fix the test. In this case, you introduced panic!() in a const fn. This wasn't stabilized until Rust 1.57. So you need to bump the MSRV again. Tracking the MSRV is important for many reasons and you shouldn't just skip this test or ignore it.

[jarkkojs]

@npmccallum: it's the same 1.56.0 tests as always. Should I never request a review?

I can demonstrate this with a nil commit, if required (as I did for SGX crate).

[jarkkojs]

@jarkkojs No. You should either fix your code or fix the test. In this case, you introduced panic!() in a const fn. This wasn't stabilized until Rust 1.57. So you need to bump the MSRV again. Tracking the MSRV is important for many reasons and you shouldn't just skip this test or ignore it.

OK, I could not comprehend this from the commit message. I'll start with the code if I could render the panic out altogether, thanks.

[npmccallum]

@jarkkojs As for reviewing the code, I think making Record generic is a bad idea. This data type is designed to be naturally aligned and with a size that divides evenly into a Page with 128 records per page. Making this generic breaks all these properties. And we don't have any use case for storing anything else in a record yet.

[npmccallum]

OK, I could not comprehend this from the commit message. I'll start with the code if I could render the panic out altogether, thanks.

I doubt you'll learn anything about the compiler failure from a commit message. The test failure log, however, says this:

error[E0658]: panicking in constant functions is unstable
Error:    --> /home/runner/.cargo/git/checkouts/primordial-7bcd0b5590a177f6/b606b9e/src/address.rs:106:13
    |
106 |             panic!("unaligned address value");
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: see issue #51999 <https://github.com/rust-lang/rust/issues/51999> for more information
    = note: this error originates in the macro `$crate::panic::panic_20[15](https://github.com/enarx/mmledger/runs/5680296183?check_suite_focus=true#step:4:15)` (in Nightly builds, run with -Z macro-backtrace for more info)

For more information about this error, try `rustc --explain E0658`.
error: could not compile `primordial` due to previous error

Notice that it tells you exactly what the problem is: panicking in constant functions is unstable. It even gives you a link to the issue for this feature: rust-lang/rust#51999. When you click this link it says that the "tracking issue" is closed. This means that the feature was stabilized.

You can also look at the tests. We test this code against four Rust releases:

• nightly
• beta
• stable
• MSRV (currently: 1.56)

Since the failure occurs only in the MSRV test but not in stable or beta, it means you're using a feature that wasn't stabilized until a later release. From there you just need to figure out which release contains your feature. You can do this manually by bumping the MSRV and running the tests again. Or you can look at the Rust changelog.

[jarkkojs]

OK, I could not comprehend this from the commit message. I'll start with the code if I could render the panic out altogether, thanks.

I doubt you'll learn anything about the compiler failure from a commit message. The test failure log, however, says this:

error[E0658]: panicking in constant functions is unstable
Error:    --> /home/runner/.cargo/git/checkouts/primordial-7bcd0b5590a177f6/b606b9e/src/address.rs:106:13
    |
106 |             panic!("unaligned address value");
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: see issue #51999 <https://github.com/rust-lang/rust/issues/51999> for more information
    = note: this error originates in the macro `$crate::panic::panic_20[15](https://github.com/enarx/mmledger/runs/5680296183?check_suite_focus=true#step:4:15)` (in Nightly builds, run with -Z macro-backtrace for more info)

For more information about this error, try `rustc --explain E0658`.
error: could not compile `primordial` due to previous error

Notice that it tells you exactly what the problem is: panicking in constant functions is unstable. It even gives you a link to the issue for this feature: rust-lang/rust#51999. When you click this link it says that the "tracking issue" is closed. This means that the feature was stabilized.

You can also look at the tests. We test this code against four Rust releases:

* nightly

* beta

* stable

* MSRV (currently: 1.56)

Since the failure occurs only in the MSRV test but not in stable or beta, it means you're using a feature that wasn't stabilized until a later release. From there you just need to figure out which release contains your feature. You can do this manually by bumping the MSRV and running the tests again. Or you can look at the Rust changelog.

Thanks for briefing!

[jarkkojs]

I'll do enarx side changes because it's not a huge stretch to help with the review process. I'll put a draft pull request to enarx once it's done. I'll take draft away once this pull request has been merged.

[jarkkojs]

I'll do enarx side changes because it's not a huge stretch to help with the review process. I'll put a draft pull request to enarx once it's done. I'll take draft away once this pull request has been merged.

enarx/enarx#1588

[npmccallum]

This is not the right place to change. You have to change it in the GitHub Actions configuration. See: https://github.com/enarx/mmledger/blob/main/.github/workflows/test.yml#L24

[npmccallum]

Please restore the previous record type.

[npmccallum]

@jarkkojs I might have misunderstand your intent here. Is your intent to say that we don't need to worry about the alignment/size of the ledger because it will be coalesced with all the other RW variables in the data section?

[jarkkojs]

I agree that generic type was not a great idea but otherwise this is much simpler now. I'll fix that part.

[jarkkojs]

Please restore the previous record type.

I don't see any sense in it. I've cleaned up the whole thing. This is now convenient to use. Undo all the work and start over because of what? Alignment issues are a valid point, this other part makes no sense.

You could see it from the examples already that you had to manually set off-by-one length to the other. Using generic type was agreed a bad choice but also trivial to fix. I'll add also reserved field to ledger. That way it can have its personal field in its own struct and that reserved data can be used for something if needed in the regions.

I'm not sure if this about the name but address regions is very common language, more common than addresses records at least when we talk about ranges of addresses, and not e.g. postal addresses.

[jarkkojs]

I.e. as soon I push the fix for Ledger, you just create ledger like:

const LEDGER: Ledger<127> = Ledger::new(Line::<Address<usize, Page>(start, end));

And that data, which previously reserved for a ledger specific use, is now reserved for future use. It's objectively better to do it this way.

[jarkkojs]

Region

/// A range of tagged address space in a ledger.
#[derive(Copy, Clone, Debug, Default, PartialEq, Eq)]
#[repr(C, align(32))]
pub struct Region {
    /// Address range
    pub addresses: Line<Address<usize, Page>>,
    /// Access token
    pub token: u64,
    reserved: u64,
}

impl Region {
    #[inline]
    /// Create a new instance.
    pub const fn new(addresses: Line<Address<usize, Page>>, token: u64) -> Self {
        Self {
            addresses,
            token,
            reserved: 0,
        }
    }

    #[inline]
    /// Create an empty instance.
    pub const fn empty() -> Self {
        Self::new(Line::new(Address::NULL, Address::NULL), 0)
    }
}

Ledger

#[derive(Clone, Debug)]
#[repr(C, align(32))]
pub struct Ledger<const N: usize> {
    addresses: Line<Address<usize, Page>>,
    len: usize,
    reserved: u64,
    regions: [Region; N],
}

(and all constructors are const as mentioned)

[jarkkojs]

I understand the tendency to put them into same array but they are different structure. Length field does not mean anything to a region. This way the area can be marked in region as reserved and used for something else. This is as memory efficient as earlier and a lot simpler to use at the same time. And handles overlaps.

Can I move to enarx so that we get some experience how this will work out, or should I instead revert to record and start this over?

[jarkkojs]

Your alignment argument was an appropriate and I have now data organized that way and also alignments in place. It helps to drop-in-replace stuff in heap.

[jarkkojs]

I also added a clear heading comment how to properly use the Ledger:

/// Maintains a log of reserved address regions. The header of the ledger and
/// each region are 32 bytes, and they are also aligned to 32 bytes. To get a
/// ledger of which total size is a power of two, pick N = M - 1, where M is a
/// power of two.
#[derive(Clone, Debug)]
#[repr(C, align(32))]
pub struct Ledger<const N: usize> {
    addresses: Line<Address<usize, Page>>,
    len: usize,
    reserved: u64,
    regions: [Region; N],
}

This is just evolution. I fix issue when they are reported, it's just life. It's way better idea to test this with enarx than get stuck. It's evolution from what you did, not a total turn-over in the same way as it evolved from your work. In my opinion it is just always a bad idea to interleave data like with that length field. It's just asking for bugs... Other than the thing is based on same principles that I PoC'd, you evolved, and now this just maturizes the whole thing.

[jarkkojs]

@npmccallum I think I got an idea how to find a great middle-roads, which deals with your concerns and also my concern of interleaved data. I'll use enum for that field. Then the same field can be shared instead of both having each others field lying around.

This way there is a single Record structure, Rust's type system takes care of my concerns of mixing up data, and does not have the limitations of my approach. E.g. you can implement From trait. Perhaps this was the quantitative reason for this single structure in the first place?

[jarkkojs]

I started to implement From trait for easier test writing and realized this quantitative argument. Still I think it'd be wrong to prevent fully what it was.