Can you split this into a proper series? There are many different things
going on here, reviewing everything at once is difficult.
On Thu, Oct 01, 2020 at 07:16:42PM +0300, Jarkko Sakkinen wrote:
> Reduce the struct size to 64 bytes. It makes sense to have a naturally
> aligning struct size and it leaves 24 bytes of spare space for possible
> future expansion. Having 256 bytes is over the top without any reasonable
> explanation, which does not exist in the commit for the SGX vDSO.
I'm all for dropping the padding, but we should drop reserved fields
altogether and instead reserve space for future features by keeping 'flags',
enforcing it to be zero, and conditioning consumption of future fields on
opt-in flags being set.
> Drop struct sgx_exception. It does not serve any semantic purpose in the
> context of this vDSO. Doing this is also consistent with user_handler and
IMO it does serve a purpose, it helps convey that it's a package deal, i.e.
either all fields are valid or none are valid.
> user_data fields, as neither are they encapsulated in an embedded data
> structure.
>
> Drop 'flags' from sgx_enclave_run as it is an unused value.'vsgx.S'
> literally states this in an inline comment.
It's not unused, it's reserved to zero.
> Pick more reasonable names for the four fields of the exception. For
> example, 'trapnr' should rather be just 'number' because in practice we
> deliver only trap and the rest are faults.
No, in Intel terminology both faults and traps are delivered. E.g. #DBs
are traps (except for code breakpoints and INT1), #PF and #GP are faults.
That being said, I'm all for changing the 'trapnr' name. I grabbed it from
from the kernel's exception fixup, it wasn't deliberately chosen to imply
that only trap-like exceptions are reported to userspace.
> Also 'vector' would be an
> adequate name for this field. Document the fields properly in order to
> better explain the values that they have inherited.
My vote is for 'vector'.
> Remove 'exit_reason' as it is really a redundant field to save some space.
> Instead, have just a field called 'leaf' that tells the last seen ENCLU.
This justification contradicts the removal of flags. If you want to shave
bytes, keep flags and enforce that flags is zero so that you don't need to
have reserved bytes at the end of the struct.
IMO, having a separate exit reason provides saner code for userspace, e.g.
if (run->exit_reason == SGX_EXCEPTION_EXIT)
<do exception stuff>
versus
if (run->leaf == EENTER || run->leaf == ERESUME)
<do exception stuff>
I would prefer to be explicit about why the vDSO is transferring control to
the caller or its handler. It also gives us line of sight to supporting
exit types other than EEXIT and exceptions. Maybe we never end up with
another type, but IMO shaving 4 bytes and a MOV isn't worth the risk of
ending up with a mess of an API if another exit reason comes along.
> The documentation, selftest and the kernel source code use the terms "user
> handler" and "exit handler" in somewhat inconsistent manner. Start using
> "AEX handler" consistently as this is the term that Intel SDM volume D uses
> for these events.
No, AEX is specifically for "asynchronus" exits. In this context, that is
limited to the exception path. The vDSO invokes the user handler for EEXIT,
i.e. the synchrous path, which is not an AEX.
