Fix/reuse helpers

[arkivm]

Reuse the helpers that are already exported by the vc module for all vc_terminate conditions. Tweak the return code such that the compiler is aware of the noreturn path and not complain that the Err path does not return the same value as the success path.

Signed-off-by: Vikram Narayanan <vikram186@gmail.com>

[dubek]

(hi again @arkivm !)

Nice, I like the use of the never type ! to indicate non-returning functions.

I wonder if something we can have something like unwrap that instead of panic will call vc_terminate_xxxx. Consider this code:

    let init_frame: PhysFrame = match mem_allocate_frames(init_count) {
        Some(f) => f,
        None => vc_terminate_svsm_enomem(),
    };

can we somehow turn it into:

    let init_frame: PhysFrame = mem_allocate_frames(init_count).unwrap_svsm_enomem();

another way is of course to make more and more functions return Result, and use the ? operator to return early. Then the vc_terminate_xxxx functions will only be called at the very top level. That will take time...

[tlendacky]

Thanks for the patches. Please make sure that any changes don't generate new warnings. So whichever patch results in the warning from src/mem/pgtable.rs, make the necessary changes to eliminate the warning in the same patch.

[arkivm]

hi @dubek :)

can we somehow turn it into:

    let init_frame: PhysFrame = mem_allocate_frames(init_count).unwrap_svsm_enomem();

we can do something like this without much effort.

   let frame: PhysFrame =
       mem_allocate_frames(count as u64).unwrap_or_else(|| vc_terminate_svsm_enomem());

another way is of course to make more and more functions return Result, and use the ? operator to return early. Then the vc_terminate_xxxx functions will only be called at the very top level. That will take time...

That'd be perfect! This way, we can propagate Result all over to the top-level function and liberally use ?. But there are lot of places that needs patching. I have a draft (WiP) in my stash for converting rmp_update to return Result instead of c-style ret and check for non-zero values. I will post that sometime and we can work together on the design of propagating Result.

[Zildj1an]

can we somehow turn it into:

    let init_frame: PhysFrame = mem_allocate_frames(init_count).unwrap_svsm_enomem();

I like this idea but:

we can do something like this without much effort.

   let frame: PhysFrame =
       mem_allocate_frames(count as u64).unwrap_or_else(|| vc_terminate_svsm_enomem());

I don't think this is very readable.

[arkivm]

I don't think this is very readable.

The alternative I could think of is to define a trait that would define a function unwrap_svsm_enomem. Is that what you had in mind @dubek? e.g.,

pub trait CustomUnwrap<T> {
    fn unwrap_svsm_enomem(self) -> T;
}

impl CustomUnwrap<PhysFrame> for Option<PhysFrame> {
    fn unwrap_svsm_enomem(self) -> PhysFrame {
        match self {
            Some(f) => f,
            None => vc_terminate_svsm_enomem(),
        }
    }
}

However, this would cause needless code bloat. otoh, unwrap_or_else is more rustic way of panicking if it is an error. I am open to other ideas @Zildj1an @dubek

[arkivm]

make the necessary changes to eliminate the warning in the same patch.

Squashed. Ready for review @tlendacky @Zildj1an

[tlendacky]

So vc_terminate() is already marked as no-return... why do all the functions where the body only calls vc_terminate() need to be marked that way? Shouldn't rust be able to figure that out? I would say this is more an issue with rust than having to mark all of those other functions as no-return. Thoughts?

And if I apply the first patch and build, I get a warning, so it looks like you squashed the wrong patches.

[dubek]

However, this would cause needless code bloat.

@arkivm is it possible to do this in a generic way, for every Option and Result? Something like (not sure this is valid syntax):

pub trait CustomUnwrap<T> {
    fn unwrap_svsm_enomem(self) -> T;
}

impl CustomUnwrap<T> for Option<T> {
    fn unwrap_svsm_enomem(self) -> T {
        match self {
            Some(value) => value,
            None => vc_terminate_svsm_enomem(),
        }
    }
}

[dubek]

So vc_terminate() is already marked as no-return... why do all the functions where the body only calls vc_terminate() need to be marked that way? Shouldn't rust be able to figure that out? I would say this is more an issue with rust than having to mark all of those other functions as no-return. Thoughts?

@tlendacky I think that we need to drop the semicolon in:

/// Terminate SVSM with generic SVSM reason
#[inline]
pub fn vc_terminate_svsm_general() {
    vc_terminate(SVSM_REASON_CODE_SET, SVSM_TERM_GENERAL);
}

and then the compiler should complain that the return type of vc_terminate_svsm_general doesn't match the actual "value" returned from the function (which is !).
(currently vc_terminate_svsm_general returns type () which is omitted in the declaration)

[Zildj1an]

Looks like there is two separate conversations going in parallel here (unwrap options/this PR). @arkivm @dubek I am very interested in the possibility of improving error unwrapping but let us move the conversation to a separate issue.

[tlendacky]

@tlendacky I think that we need to drop the semicolon in:

/// Terminate SVSM with generic SVSM reason
#[inline]
pub fn vc_terminate_svsm_general() {
    vc_terminate(SVSM_REASON_CODE_SET, SVSM_TERM_GENERAL);
}

and then the compiler should complain that the return type of vc_terminate_svsm_general doesn't match the actual "value" returned from the function (which is !). (currently vc_terminate_svsm_general returns type () which is omitted in the declaration)

I removed the semi-colon(s) and there is no complaining from the compiler.

[dubek]

I removed the semi-colon(s) and there is no complaining from the compiler.

hmm, you're right. The following compiles and runs without warnings, even though never_returns() doesn't return any i32:
https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=b2599abd75a14669da6faf1a85d948c6

non-intuitive to me.

[arkivm]

@tlendacky @dubek Valid concern. There are issues in the past, where having a panic! in the match arm did not work (rust-lang/rust#24157). Thankfully, those were solved. I have added a comment to the same issue with our usecase (calling it from a wrapper). Let's hold this PR until we get a response from Rust folks.

[workingjubilee]

You are witnessing the principle of explosion: "From contradiction, anything follows". Thus, from !, any type T can be arrived at. Rust has various coercion sites, and the return from a function is one¹.

So vc_terminate() is already marked as no-return... why do all the functions where the body only calls vc_terminate() need to be marked that way? Shouldn't rust be able to figure that out? I would say this is more an issue with rust than having to mark all of those other functions as no-return. Thoughts?

From !, Rust can coerce (), so the function,

fn from_contradiction_an_absolute_unit_follows() {
    std::process::exit(random())
}

does indeed, as you have already noted, read more to Rust as

fn from_contradiction_an_absolute_unit_follows() -> () {

and because of the signature for std::process::exit of () -> ! it will, in the somewhat simplistic explanation, evaluate exit to return ! but then find itself at a coercion site as it is also the return expression of fn from_contradiction_an_absolute_unit_follows, and thus follow coercion rules to return () instead.

The same thing happens with the example given of fn never_returns() -> i32, except Rust decides, in that case, ! actually means i32.

For stability reasons, Rust's design has tended to prioritize choices that will lead to the contents of functions not mattering, so that changes inside a function generally won't break someone else's code in the complex software packaging ecosystems of the 21st Century. Global type inference allows every new module you add to potentially break type inference if you have a type system that is complex enough to sometimes require annotations, and my understanding is that this would remain necessarily true even if rustc made its type inference much stronger, because Rust has a type system too complex to be described by a system with fully decidable type inference according to the current understanding of programming language theory.

Footnotes

1. https://doc.rust-lang.org/reference/type-coercions.html ↩

[dubek]

Thanks @workingjubilee for the thorough explanation.

---

@tlendacky @arkivm As I understand it, we have to make sure we fully specify the declarations of functions: arguments and return types. So we have to state that pub fn vc_terminate_svsm_general() (and its friends) return ! (otherwise the compiler fills in the default () return type which is not correct).

[arkivm]

Thank you @workingjubilee for the detailed explanation. @dubek I will revisit this PR to see if some of them can be squashed.

[arkivm]

Ready for review!

[Zildj1an]

This looks good to go :)