On Wed, Aug 26, 2020 at 04:26:11PM -0700, Xing, Cedric wrote: > After countless discussions along this upstream journey, I thought we had > agreed that simplicity was the priority. And for simplicity we were even > willing to give up compliance to x86_64 ABI. Then how do you justify > treating rust better than other programming languages? This looks a hard > sell to me. If you view using a struct instead of a multiple params as a separate cost, adding a pass-through param to throw a bone to Rust adds zero complexity. The flags field has nothing to do with Rust whatsoever. As for the struct itself, I suppose you could argue it's gratuitous, but I don't buy that it adds complexity. The impact on the vDSO is something like four extra instructions, two of which are a CMP+Jcc to enforce the flags, and the others are the additional moves need to chase pointers through the struct. Unless someone really dislikes structs, there is no added complexity to the caller, it's just slightly different code, e.g. the selftest update for the struct variant actually removed code. > > > > Moving the params into a struct will also make it less painful to use > > > > dedicated exit reasons, and to support exiting on interrupts in future > > > > patches. > > > > > > > My apology for not following discussion lately. What did you mean by > > > "dedicated exit reasons" and why was it "painful"? According to another > > > thread, I guess we wouldn't have "exiting on interrupts". > > > > Currently the vDSO returns -EFAULT if the enclave encounters an exception, > > which is kludgy for two reasons: > > > > 1. EFAULT traditionally means "bad address", whereas an exception could be > > a #UD in the enclave. > > > > 2. Returning -EFAULT provides weird semantics as it implies the vDSO call > > failed, which is not the case. The vDSO itself was successful, i.e. it > > did the requested EENTER/ERESUME operation, and should really return '0'. > > > EFAULT is descriptive enough for me. And more importantly detailed/specific > info is always available in exinfo. > > IMO, the purpose of return code is for the caller/handler to branch on. If 2 > cases share the same execution path, then there's no need to distinguish > them. In the case of SGX, most (if not all) exceptions are handled in the > same way - nested EENTER, then what's point of distinguishing them? Please > note that detailed info is always available in exinfo to cover corner cases. The primary motiviation is to adhere to the general rules of the kernel, specifically that -EFAULT isn't intended as a catchall for exceptions. > Moreover, even the vDSO API itself cannot tell between faults at > EENTER/ERESUME (the vDSO itself failes) vs. faults inside the enclave (vDSO > succeeded but the enclave fails). I don't think you can be accurate without > complicating the code significantly. And I didn't even try. The proposed documentation explicitly states that "success" is defined as attempting ENCLU. > > The proposed solution for #1 is to define arbitrary return values to > > differentiate between synchronous (EEXIT) exits and asynchronous exits due > > to exceptions. But that doesn't address #2, and gets especially awkward when > > dealing with the call back return, which is also overloaded to use positive > > return values for ENCLU leafs. > > > > Passing a "run" struct makes it trivially easy to different the return value > > of the vDSO function from the enclave exit reason, and to avoid collisions > > with the return value from the userspace callback. > > > When a callback is configured, the vDSO NEVER returns to caller directly - > i.e. all return codes must be from the callback but NOT the vDSO. I don't > see any collisions here. Unless the user provides a bad leaf, in which case -EINVAL is returned. But that wasn't what I was talking about. If we want to use arbitrary values instead of hijacking -EFAULT (and possibly -EINTR), then we run into problems with choosing an arbitrary value that won't collide with the value returned from the callback to the _vDSO_. The callback essentially gets dibs on all negative values and 0 is off limits. That leaves positive values, but those are interpreted as the new ENCLU leaf by the vDSO. That means the vDSO can "return" a value to the callback that the callback can't safely reflect back to the vDSO. Using an exit reason side steps that by virtue of it not being the return value in either direction.
