igvm_params: avoid creating misaligned references and other small improvements

[00xc]

In Rust, references to T that are not aligned to that T's requirements are undefined behavior. Check that the addresses obtained from the IGVM configuration are properly aligned before attempting to read the appropriate structures from them.

While we are at it, perform some minor code improvements.

[msft-jlange]

I wonder whether it would be more effective to resolve the alignment issues by removing (packed) from the structure definitions. They are repr(C) because they have to be shared between tools and runtime, but the structures as defined are naturally aligned anyway so (packed) is not providing any benefit. This certainly seems like it could save a whole lot of extra code (particularly code that requires specialized Rust knowledge to maintain effectively).

[00xc]

I wonder whether it would be more effective to resolve the alignment issues by removing (packed) from the structure definitions. They are repr(C) because they have to be shared between tools and runtime, but the structures as defined are naturally aligned anyway so (packed) is not providing any benefit. This certainly seems like it could save a whole lot of extra code (particularly code that requires specialized Rust knowledge to maintain effectively).

This would remove one source of problems for sure, but my concern here is that we get a misaligned pointer because of some mistake up the call stack. We could get misaligned pointers for all sorts of reasons, not just packed structs, so we must always check alignment first.

[msft-jlange]

I wonder whether it would be more effective to resolve the alignment issues by removing (packed) from the structure definitions. They are repr(C) because they have to be shared between tools and runtime, but the structures as defined are naturally aligned anyway so (packed) is not providing any benefit. This certainly seems like it could save a whole lot of extra code (particularly code that requires specialized Rust knowledge to maintain effectively).

This would remove one source of problems for sure, but my concern here is that we get a misaligned pointer because of some mistake up the call stack. We could get misaligned pointers for all sorts of reasons, not just packed structs, so we must always check alignment first.

Would it not be more effective to verify the alignment of the pointer when it is first captured so that it is not necessary to verify alignment on every field access? It seems that would simplify the code (by eliminating all of the extra logic for alignment checks) as well as the runtime behavior (by eliminating the actual alignment checks) while still guaranteeing safety (by rejecting structure pointers that are not correctly aligned).

[00xc]

Would it not be more effective to verify the alignment of the pointer when it is first captured so that it is not necessary to verify alignment on every field access? It seems that would simplify the code (by eliminating all of the extra logic for alignment checks) as well as the runtime behavior (by eliminating the actual alignment checks) while still guaranteeing safety (by rejecting structure pointers that are not correctly aligned).

Isn't that what the current PR is doing? Alignment is only verified in IgvmParams::new()

[msft-jlange]

Would it not be more effective to verify the alignment of the pointer when it is first captured so that it is not necessary to verify alignment on every field access? It seems that would simplify the code (by eliminating all of the extra logic for alignment checks) as well as the runtime behavior (by eliminating the actual alignment checks) while still guaranteeing safety (by rejecting structure pointers that are not correctly aligned).

Isn't that what the current PR is doing? Alignment is only verified in IgvmParams::new()

Yes, I guess I lost that intent among all of the other changes proposed.

Given that pointer alignment validation is such a useful concept, should try_aligned_ref be moved out of IgvmParams into a utility function (e.g. src/utils/utills.rs) so it can be usable in other places as well (e.g. guest memory accesses)? Also, in practice, an unaligned IgvmParams pointer is fatal so a panic would be just as appropriate as a Result, though if this becomes a general utility function, a Result is likely more appropriate because other usage environments may not want to panic as a result of alignment failure.

[00xc]

Given that pointer alignment validation is such a useful concept, should try_aligned_ref be moved out of IgvmParams into a utility function (e.g. src/utils/utills.rs) so it can be usable in other places as well (e.g. guest memory accesses)?

I think we can do that in a separate PR. GuestPtr at the moment does not really create references, it simply uses rep movsb to copy data from an arbitrary address into a well aligned location (e.g. a MaybeUninit<T>, which has the same layout as T). As far as I know rep movsb does not have any alignment requirements beyond performance, but perhaps I'm wrong.

Also, in practice, an unaligned IgvmParams pointer is fatal so a panic would be just as appropriate as a Result, though if this becomes a general utility function, a Result is likely more appropriate because other usage environments may not want to panic as a result of alignment failure.

Yes, returning a Result makes it easier to refactor IgvmParams::try_aligned_ref() as general purpose function in the future. At the moment we are already panicking in all the callers of IgvmParams::new() (stage2_main, svsm_main() and svsm_start() via init_page_table()).

[msft-jlange]

Given that pointer alignment validation is such a useful concept, should try_aligned_ref be moved out of IgvmParams into a utility function (e.g. src/utils/utills.rs) so it can be usable in other places as well (e.g. guest memory accesses)? I think we can do that in a separate PR. GuestPtr at the moment does not really create references, it simply uses rep movsb to copy data from an arbitrary address into a well aligned location (e.g. a MaybeUninit<T>, which has the same layout as T). As far as I know rep movsb does not have any alignment requirements beyond performance, but perhaps I'm wrong.

I agree that we can do this in a separate PR. You are correct that movsb has no alignment requirements, but it is also slow. Long-term, we would be much better off just using pointers or references directly instead of doing slow copies. In general, all of the guest memory accesses are defined to require aligned pointers anyway, and those few that don't will know they are special and should probably be going through a different slow path instead of slowing down the common path.

[joergroedel]

Compiling with latest lints gives these errors:

error: unnecessary qualification
  --> kernel/src/igvm_params.rs:65:24
   |
65 |             return Err(SvsmError::Firmware);
   |                        ^^^^^^^^^^^^^^^^^^^
   |
   = note: requested on the command line with `-D unused-qualifications`
help: remove the unnecessary path segments
   |
65 -             return Err(SvsmError::Firmware);
65 +             return Err(Firmware);
   |

Please update the rebase and update the PR.