| Message ID | 20170914195418.16672-1-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Sep 15, 2017 at 01:41:37PM -0700, Jarkko Sakkinen wrote: > On Fri, Sep 15, 2017 at 08:22:48PM +0000, Christopherson, Sean J wrote: > > On Fri, Sep 15, 2017 at 01:11:19PM -0700, Jarkko Sakkinen wrote: > > > On Fri, Sep 15, 2017 at 07:42:52PM +0000, Christopherson, Sean J wrote: > > > > On Thu, Sep 14, 2017 at 03:13:33PM -0700, Jarkko Sakkinen wrote: > > > > > On Thu, Sep 14, 2017 at 12:54:18PM -0700, Sean Christopherson wrote: > > > > > > When trapping EINIT, KVM needs the vector if EINIT faults in order > > > > > > to inject the correct fault back into the guest. Add "_raw" suffixed > > > > > > ENCLS macros that utilize _ASM_EXTABLE_FAULT to return the raw fault > > > > > > vector to the caller. The fault vector is shifted into bits 31:16 > > > > > > so that the caller can distinguish between faults and SGX error codes. > > > > > > > > > > > > Modify the existing __encls_ret and __encls macros to return -EFAULT > > > > > > if a fault occurs on ENCLS. This provides userspace with an API that > > > > > > is more consistent with other IOCTLs, and obviates the need for the > > > > > > caller to adjust return values, e.g. sgx_ioc_enclave_create no longer > > > > > > needs to manually return -EFAULT on any __ecreate failure. > > > > > > > > > > > > Coincidentally, these changes also fix a bug where __encls_ret did > > > > > > not update EAX after a fault, e.g. __einit would always return '2' > > > > > > if it faulted. > > > > > > > > > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > > > > > > > > > Makes sense and in this case the code change could be taken. > > > > > > > > > > It would help if you reviewed the actual code you see the issue because > > > > > now I have to project this back to my patches. And if you have the patch > > > > > available you can propose sending it. Now it is very unclear where and > > > > > when you want it. In this case I would have agreed to take your code > > > > > change. > > > > > > > > > > /Jarkko > > > > > > > > I submitted patches based on your master branch. If master is no longer > > > > valid than please delete the branch or make it explicitly clear that it's > > > > a stale branch and should not be used. > > > > > > > > The patches I submitted have no relation to your LE branch. The two bug > > > > fix patches addressed issues that recently bit me but have existed in the > > > > SGX code since time immemorial. The EPC bank patch is a v2 revision for > > > > a months-old patch that I belatedly realized hadn't been accepted. > > > > > > The master branch is needed to support out-of-tree driver up > > > until le branch is upstreamed > > > > > > /Jarkko > > > > Is master being maintained, i.e. accepting bug patches, or has it been put out > > to pasture for all intents and purposes? In other words, should all future > > comments and/or bug fixes be directed towards the LE patches/branch? > > I think the reasonable thing to do is to put a marker in the diffstat > section of patch if it is a bug fix. I'll apply them to the master > branch and backport them to the 'le' branch. > > For the most part I won't accept new features to the master branch but I > think that for example these encls changes make sense enough to be put > there. I'll apply that patch as soon as possible. I'm sorry but I must > missed the first version somehow. You didn't miss anything, the EPC bank patch was a v2, not the ENCLS patch. > The le branch will become the new master after upstreaming. I'll apply > your encls changes right after upstreaming there so you will get credit > and the blame for them. It's cutting hairs but with le I'm even more > conservative on adding new features. That's fine, though if you take that approach then __encls_ret needs to be modified in your LE branch to update EAX on a fault. Returning the leaf number instead of -1 is a bug that is userspace visible. > With upstreaming the main blocker is GCC7 compilation issue that I've > explained earlier. After that is fixed I'm ready to send the patch > set to LKML. > > Does this make sense to you? Yep. Hopefully there are no more bugs and this doesn't come up again :)
On Fri, Sep 15, 2017 at 09:07:57PM +0000, Christopherson, Sean J wrote: > > For the most part I won't accept new features to the master branch but I > > think that for example these encls changes make sense enough to be put > > there. I'll apply that patch as soon as possible. I'm sorry but I must > > missed the first version somehow. > > You didn't miss anything, the EPC bank patch was a v2, not the ENCLS patch. Everything is now anyway in the master branch. > > The le branch will become the new master after upstreaming. I'll apply > > your encls changes right after upstreaming there so you will get credit > > and the blame for them. It's cutting hairs but with le I'm even more > > conservative on adding new features. > > That's fine, though if you take that approach then __encls_ret needs to > be modified in your LE branch to update EAX on a fault. Returning the > leaf number instead of -1 is a bug that is userspace visible. You are probably talking about __encls() (in guest __encls_ret() can fault, which your patch addesses)? > > With upstreaming the main blocker is GCC7 compilation issue that I've > > explained earlier. After that is fixed I'm ready to send the patch > > set to LKML. > > > > Does this make sense to you? > > Yep. Hopefully there are no more bugs and this doesn't come up again :) If you put a note just before diffstat after the dashes that add this to the master, I will do it. I can then do deduction whether it is mandatory for the le branch. /Jarkko
On Fri, Sep 15, 2017 at 02:30:30PM -0700, Jarkko Sakkinen wrote: > On Fri, Sep 15, 2017 at 09:07:57PM +0000, Christopherson, Sean J wrote: > > > For the most part I won't accept new features to the master branch but I > > > think that for example these encls changes make sense enough to be put > > > there. I'll apply that patch as soon as possible. I'm sorry but I must > > > missed the first version somehow. > > > > You didn't miss anything, the EPC bank patch was a v2, not the ENCLS patch. > > Everything is now anyway in the master branch. > > > > The le branch will become the new master after upstreaming. I'll apply > > > your encls changes right after upstreaming there so you will get credit > > > and the blame for them. It's cutting hairs but with le I'm even more > > > conservative on adding new features. > > > > That's fine, though if you take that approach then __encls_ret needs to > > be modified in your LE branch to update EAX on a fault. Returning the > > leaf number instead of -1 is a bug that is userspace visible. > > You are probably talking about __encls() (in guest __encls_ret() can > fault, which your patch addesses)? No, I'm talking about __encs_ret. In your current LE branch, __encls() sets EAX/RAX to -1 in its .fixup section. __encls_ret() does not do this, which results in EAX/RAX retaining its previous value, i.e. the ENCLS leaf, if ENCLS faults. For example, if ENCLS[EINIT] faults for whatever reason, then __einit will ultimately return '2' because __encls_ret() doesn't update EAX/RAX. This is extremely confusing for userspace because '2' is also the error code for SGX_INVALID_ATTRIBUTE, which happens to be a valid error code for EINIT. > > > With upstreaming the main blocker is GCC7 compilation issue that I've > > > explained earlier. After that is fixed I'm ready to send the patch > > > set to LKML. > > > > > > Does this make sense to you? > > > > Yep. Hopefully there are no more bugs and this doesn't come up again :) > > If you put a note just before diffstat after the dashes that add this > to the master, I will do it. > > I can then do deduction whether it is mandatory for the le branch.
On Fri, Sep 15, 2017 at 09:42:31PM +0000, Christopherson, Sean J wrote: > On Fri, Sep 15, 2017 at 02:30:30PM -0700, Jarkko Sakkinen wrote: > > On Fri, Sep 15, 2017 at 09:07:57PM +0000, Christopherson, Sean J wrote: > > > > For the most part I won't accept new features to the master branch but I > > > > think that for example these encls changes make sense enough to be put > > > > there. I'll apply that patch as soon as possible. I'm sorry but I must > > > > missed the first version somehow. > > > > > > You didn't miss anything, the EPC bank patch was a v2, not the ENCLS patch. > > > > Everything is now anyway in the master branch. > > > > > > The le branch will become the new master after upstreaming. I'll apply > > > > your encls changes right after upstreaming there so you will get credit > > > > and the blame for them. It's cutting hairs but with le I'm even more > > > > conservative on adding new features. > > > > > > That's fine, though if you take that approach then __encls_ret needs to > > > be modified in your LE branch to update EAX on a fault. Returning the > > > leaf number instead of -1 is a bug that is userspace visible. > > > > You are probably talking about __encls() (in guest __encls_ret() can > > fault, which your patch addesses)? > > No, I'm talking about __encs_ret. In your current LE branch, __encls() sets > EAX/RAX to -1 in its .fixup section. __encls_ret() does not do this, which > results in EAX/RAX retaining its previous value, i.e. the ENCLS leaf, if ENCLS > faults. For example, if ENCLS[EINIT] faults for whatever reason, then __einit > will ultimately return '2' because __encls_ret() doesn't update EAX/RAX. This > is extremely confusing for userspace because '2' is also the error code for > SGX_INVALID_ATTRIBUTE, which happens to be a valid error code for EINIT. What is the reason ENCLS[EINIT] when it could fault in a legit case (not a driver bug)? It would make sense set it to some sane value in the case of a driver bug and report a critical error (pr_crit) to the user. I'm just trying to decipher "whatever case" that's all. /Jarkko
On Fri, Sep 15, 2017 at 03:27:19PM -0700, Jarkko Sakkinen wrote: > On Fri, Sep 15, 2017 at 09:42:31PM +0000, Christopherson, Sean J wrote: > > On Fri, Sep 15, 2017 at 02:30:30PM -0700, Jarkko Sakkinen wrote: > > > On Fri, Sep 15, 2017 at 09:07:57PM +0000, Christopherson, Sean J wrote: > > > > > For the most part I won't accept new features to the master branch but I > > > > > think that for example these encls changes make sense enough to be put > > > > > there. I'll apply that patch as soon as possible. I'm sorry but I must > > > > > missed the first version somehow. > > > > > > > > You didn't miss anything, the EPC bank patch was a v2, not the ENCLS patch. > > > > > > Everything is now anyway in the master branch. > > > > > > > > The le branch will become the new master after upstreaming. I'll apply > > > > > your encls changes right after upstreaming there so you will get credit > > > > > and the blame for them. It's cutting hairs but with le I'm even more > > > > > conservative on adding new features. > > > > > > > > That's fine, though if you take that approach then __encls_ret needs to > > > > be modified in your LE branch to update EAX on a fault. Returning the > > > > leaf number instead of -1 is a bug that is userspace visible. > > > > > > You are probably talking about __encls() (in guest __encls_ret() can > > > fault, which your patch addesses)? > > > > No, I'm talking about __encs_ret. In your current LE branch, __encls() sets > > EAX/RAX to -1 in its .fixup section. __encls_ret() does not do this, which > > results in EAX/RAX retaining its previous value, i.e. the ENCLS leaf, if ENCLS > > faults. For example, if ENCLS[EINIT] faults for whatever reason, then __einit > > will ultimately return '2' because __encls_ret() doesn't update EAX/RAX. This > > is extremely confusing for userspace because '2' is also the error code for > > SGX_INVALID_ATTRIBUTE, which happens to be a valid error code for EINIT. > > What is the reason ENCLS[EINIT] when it could fault in a legit case (not > a driver bug)? > > It would make sense set it to some sane value in the case of a driver > bug and report a critical error (pr_crit) to the user. I'm just trying > to decipher "whatever case" that's all. > > /Jarkko This can be postponed up until the driver is upstreamed unless there is a legit case where it could fault. /Jarkko
On Fri, Sep 15, 2017 at 06:40:05AM -0700, Jarkko Sakkinen wrote: > > > > > Shall we add "cc" to clobber list as RFLAGS may be updated by > > > > > ENCLS? Hey, I think I was too rushy while responsing to these emails in the middle of the conference. Calling convention does not require saving rflags so do you have any specific reason why they should be in the clobbered list? It's a macro but still I doubt GCC would do anything wrong here. Please correct me if I'm missing something here. However, I still think that volatile can be safely dropped. I have to test this at home though. /Jarkko
On Fri, Sep 15, 2017 at 02:30:30PM -0700, Jarkko Sakkinen wrote: > On Fri, Sep 15, 2017 at 09:07:57PM +0000, Christopherson, Sean J wrote: > > > For the most part I won't accept new features to the master branch but I > > > think that for example these encls changes make sense enough to be put > > > there. I'll apply that patch as soon as possible. I'm sorry but I must > > > missed the first version somehow. > > > > You didn't miss anything, the EPC bank patch was a v2, not the ENCLS patch. > > Everything is now anyway in the master branch. I took another look at this (was too rushy in the middle of the con). No one is consuming fault vector. Why is it there? /Jarkko
On Sat, Sep 16, 2017 at 09:43:12AM -0700, Jarkko Sakkinen wrote: > On Fri, Sep 15, 2017 at 03:27:19PM -0700, Jarkko Sakkinen wrote: > > On Fri, Sep 15, 2017 at 09:42:31PM +0000, Christopherson, Sean J wrote: > > > On Fri, Sep 15, 2017 at 02:30:30PM -0700, Jarkko Sakkinen wrote: > > > > On Fri, Sep 15, 2017 at 09:07:57PM +0000, Christopherson, Sean J wrote: > > > > > > For the most part I won't accept new features to the master branch but I > > > > > > think that for example these encls changes make sense enough to be put > > > > > > there. I'll apply that patch as soon as possible. I'm sorry but I must > > > > > > missed the first version somehow. > > > > > > > > > > You didn't miss anything, the EPC bank patch was a v2, not the ENCLS patch. > > > > > > > > Everything is now anyway in the master branch. > > > > > > > > > > The le branch will become the new master after upstreaming. I'll apply > > > > > > your encls changes right after upstreaming there so you will get credit > > > > > > and the blame for them. It's cutting hairs but with le I'm even more > > > > > > conservative on adding new features. > > > > > > > > > > That's fine, though if you take that approach then __encls_ret needs to > > > > > be modified in your LE branch to update EAX on a fault. Returning the > > > > > leaf number instead of -1 is a bug that is userspace visible. > > > > > > > > You are probably talking about __encls() (in guest __encls_ret() can > > > > fault, which your patch addesses)? > > > > > > No, I'm talking about __encs_ret. In your current LE branch, __encls() sets > > > EAX/RAX to -1 in its .fixup section. __encls_ret() does not do this, which > > > results in EAX/RAX retaining its previous value, i.e. the ENCLS leaf, if ENCLS > > > faults. For example, if ENCLS[EINIT] faults for whatever reason, then __einit > > > will ultimately return '2' because __encls_ret() doesn't update EAX/RAX. This > > > is extremely confusing for userspace because '2' is also the error code for > > > SGX_INVALID_ATTRIBUTE, which happens to be a valid error code for EINIT. > > > > What is the reason ENCLS[EINIT] when it could fault in a legit case (not > > a driver bug)? > > > > It would make sense set it to some sane value in the case of a driver > > bug and report a critical error (pr_crit) to the user. I'm just trying > > to decipher "whatever case" that's all. > > > > /Jarkko > > This can be postponed up until the driver is upstreamed unless there > is a legit case where it could fault. Any ENCLS instruction will #GP if SGX is not enabled in the feature control MSR, and the driver does not guarantee that it's enabled for all CPUs.
On Mon, Sep 18, 2017 at 01:09:58PM +0000, Christopherson, Sean J wrote: > On Sat, Sep 16, 2017 at 09:43:12AM -0700, Jarkko Sakkinen wrote: > > On Fri, Sep 15, 2017 at 03:27:19PM -0700, Jarkko Sakkinen wrote: > > > On Fri, Sep 15, 2017 at 09:42:31PM +0000, Christopherson, Sean J wrote: > > > > On Fri, Sep 15, 2017 at 02:30:30PM -0700, Jarkko Sakkinen wrote: > > > > > On Fri, Sep 15, 2017 at 09:07:57PM +0000, Christopherson, Sean J wrote: > > > > > > > For the most part I won't accept new features to the master branch but I > > > > > > > think that for example these encls changes make sense enough to be put > > > > > > > there. I'll apply that patch as soon as possible. I'm sorry but I must > > > > > > > missed the first version somehow. > > > > > > > > > > > > You didn't miss anything, the EPC bank patch was a v2, not the ENCLS patch. > > > > > > > > > > Everything is now anyway in the master branch. > > > > > > > > > > > > The le branch will become the new master after upstreaming. I'll apply > > > > > > > your encls changes right after upstreaming there so you will get credit > > > > > > > and the blame for them. It's cutting hairs but with le I'm even more > > > > > > > conservative on adding new features. > > > > > > > > > > > > That's fine, though if you take that approach then __encls_ret needs to > > > > > > be modified in your LE branch to update EAX on a fault. Returning the > > > > > > leaf number instead of -1 is a bug that is userspace visible. > > > > > > > > > > You are probably talking about __encls() (in guest __encls_ret() can > > > > > fault, which your patch addesses)? > > > > > > > > No, I'm talking about __encs_ret. In your current LE branch, __encls() sets > > > > EAX/RAX to -1 in its .fixup section. __encls_ret() does not do this, which > > > > results in EAX/RAX retaining its previous value, i.e. the ENCLS leaf, if ENCLS > > > > faults. For example, if ENCLS[EINIT] faults for whatever reason, then __einit > > > > will ultimately return '2' because __encls_ret() doesn't update EAX/RAX. This > > > > is extremely confusing for userspace because '2' is also the error code for > > > > SGX_INVALID_ATTRIBUTE, which happens to be a valid error code for EINIT. > > > > > > What is the reason ENCLS[EINIT] when it could fault in a legit case (not > > > a driver bug)? > > > > > > It would make sense set it to some sane value in the case of a driver > > > bug and report a critical error (pr_crit) to the user. I'm just trying > > > to decipher "whatever case" that's all. > > > > > > /Jarkko > > > > This can be postponed up until the driver is upstreamed unless there > > is a legit case where it could fault. > > Any ENCLS instruction will #GP if SGX is not enabled in the feature control > MSR, and the driver does not guarantee that it's enabled for all CPUs. That is a valid point. Here's a small suggestion for the patch. I think it would cleaner to have it as follows 1. Add the stuff that you added to '_raw' macros. 2. Have a macro ENCLS_TO_ERR(x) that converts result of the macro to a system error code. This would make it less polluted although call sites have to be converted to use ENCLS_TO_ERR() . Can you implement it this way? This is maybe cutting hairs but ENCLS_TO_ERR could be a separate commit whereas encls changes will merged to main driver commit. For ENCLS_TO_ERR you should add "Suggested-by:" tag. /Jarkko
On Mon, Sep 18, 2017 at 08:53:37PM +0300, Jarkko Sakkinen wrote: > On Mon, Sep 18, 2017 at 01:09:58PM +0000, Christopherson, Sean J wrote: > > On Sat, Sep 16, 2017 at 09:43:12AM -0700, Jarkko Sakkinen wrote: > > > On Fri, Sep 15, 2017 at 03:27:19PM -0700, Jarkko Sakkinen wrote: > > > > On Fri, Sep 15, 2017 at 09:42:31PM +0000, Christopherson, Sean J wrote: > > > > > On Fri, Sep 15, 2017 at 02:30:30PM -0700, Jarkko Sakkinen wrote: > > > > > > On Fri, Sep 15, 2017 at 09:07:57PM +0000, Christopherson, Sean J wrote: > > > > > > > > For the most part I won't accept new features to the master branch but I > > > > > > > > think that for example these encls changes make sense enough to be put > > > > > > > > there. I'll apply that patch as soon as possible. I'm sorry but I must > > > > > > > > missed the first version somehow. > > > > > > > > > > > > > > You didn't miss anything, the EPC bank patch was a v2, not the ENCLS patch. > > > > > > > > > > > > Everything is now anyway in the master branch. > > > > > > > > > > > > > > The le branch will become the new master after upstreaming. I'll apply > > > > > > > > your encls changes right after upstreaming there so you will get credit > > > > > > > > and the blame for them. It's cutting hairs but with le I'm even more > > > > > > > > conservative on adding new features. > > > > > > > > > > > > > > That's fine, though if you take that approach then __encls_ret needs to > > > > > > > be modified in your LE branch to update EAX on a fault. Returning the > > > > > > > leaf number instead of -1 is a bug that is userspace visible. > > > > > > > > > > > > You are probably talking about __encls() (in guest __encls_ret() can > > > > > > fault, which your patch addesses)? > > > > > > > > > > No, I'm talking about __encs_ret. In your current LE branch, __encls() sets > > > > > EAX/RAX to -1 in its .fixup section. __encls_ret() does not do this, which > > > > > results in EAX/RAX retaining its previous value, i.e. the ENCLS leaf, if ENCLS > > > > > faults. For example, if ENCLS[EINIT] faults for whatever reason, then __einit > > > > > will ultimately return '2' because __encls_ret() doesn't update EAX/RAX. This > > > > > is extremely confusing for userspace because '2' is also the error code for > > > > > SGX_INVALID_ATTRIBUTE, which happens to be a valid error code for EINIT. > > > > > > > > What is the reason ENCLS[EINIT] when it could fault in a legit case (not > > > > a driver bug)? > > > > > > > > It would make sense set it to some sane value in the case of a driver > > > > bug and report a critical error (pr_crit) to the user. I'm just trying > > > > to decipher "whatever case" that's all. > > > > > > > > /Jarkko > > > > > > This can be postponed up until the driver is upstreamed unless there > > > is a legit case where it could fault. > > > > Any ENCLS instruction will #GP if SGX is not enabled in the feature control > > MSR, and the driver does not guarantee that it's enabled for all CPUs. > > That is a valid point. > > Here's a small suggestion for the patch. I think it would cleaner to > have it as follows > > 1. Add the stuff that you added to '_raw' macros. > 2. Have a macro ENCLS_TO_ERR(x) that converts result of the macro > to a system error code. > > This would make it less polluted although call sites have to be > converted to use ENCLS_TO_ERR() . > > Can you implement it this way? This is maybe cutting hairs but > ENCLS_TO_ERR could be a separate commit whereas encls changes will > merged to main driver commit. For ENCLS_TO_ERR you should add > "Suggested-by:" tag. > > /Jarkko You can just send one commit and I'll chop it in the way I see proper :-) /jarkko
On Mon, Sep 18, 2017 at 09:05:13PM +0300, Jarkko Sakkinen wrote: > On Mon, Sep 18, 2017 at 08:53:37PM +0300, Jarkko Sakkinen wrote: > > On Mon, Sep 18, 2017 at 01:09:58PM +0000, Christopherson, Sean J wrote: > > > On Sat, Sep 16, 2017 at 09:43:12AM -0700, Jarkko Sakkinen wrote: > > > > On Fri, Sep 15, 2017 at 03:27:19PM -0700, Jarkko Sakkinen wrote: > > > > > On Fri, Sep 15, 2017 at 09:42:31PM +0000, Christopherson, Sean J wrote: > > > > > > On Fri, Sep 15, 2017 at 02:30:30PM -0700, Jarkko Sakkinen wrote: > > > > > > > On Fri, Sep 15, 2017 at 09:07:57PM +0000, Christopherson, Sean J wrote: > > > > > > > > > For the most part I won't accept new features to the master branch but I > > > > > > > > > think that for example these encls changes make sense enough to be put > > > > > > > > > there. I'll apply that patch as soon as possible. I'm sorry but I must > > > > > > > > > missed the first version somehow. > > > > > > > > > > > > > > > > You didn't miss anything, the EPC bank patch was a v2, not the ENCLS patch. > > > > > > > > > > > > > > Everything is now anyway in the master branch. > > > > > > > > > > > > > > > > The le branch will become the new master after upstreaming. I'll apply > > > > > > > > > your encls changes right after upstreaming there so you will get credit > > > > > > > > > and the blame for them. It's cutting hairs but with le I'm even more > > > > > > > > > conservative on adding new features. > > > > > > > > > > > > > > > > That's fine, though if you take that approach then __encls_ret needs to > > > > > > > > be modified in your LE branch to update EAX on a fault. Returning the > > > > > > > > leaf number instead of -1 is a bug that is userspace visible. > > > > > > > > > > > > > > You are probably talking about __encls() (in guest __encls_ret() can > > > > > > > fault, which your patch addesses)? > > > > > > > > > > > > No, I'm talking about __encs_ret. In your current LE branch, __encls() sets > > > > > > EAX/RAX to -1 in its .fixup section. __encls_ret() does not do this, which > > > > > > results in EAX/RAX retaining its previous value, i.e. the ENCLS leaf, if ENCLS > > > > > > faults. For example, if ENCLS[EINIT] faults for whatever reason, then __einit > > > > > > will ultimately return '2' because __encls_ret() doesn't update EAX/RAX. This > > > > > > is extremely confusing for userspace because '2' is also the error code for > > > > > > SGX_INVALID_ATTRIBUTE, which happens to be a valid error code for EINIT. > > > > > > > > > > What is the reason ENCLS[EINIT] when it could fault in a legit case (not > > > > > a driver bug)? > > > > > > > > > > It would make sense set it to some sane value in the case of a driver > > > > > bug and report a critical error (pr_crit) to the user. I'm just trying > > > > > to decipher "whatever case" that's all. > > > > > > > > > > /Jarkko > > > > > > > > This can be postponed up until the driver is upstreamed unless there > > > > is a legit case where it could fault. > > > > > > Any ENCLS instruction will #GP if SGX is not enabled in the feature control > > > MSR, and the driver does not guarantee that it's enabled for all CPUs. > > > > That is a valid point. > > > > Here's a small suggestion for the patch. I think it would cleaner to > > have it as follows > > > > 1. Add the stuff that you added to '_raw' macros. > > 2. Have a macro ENCLS_TO_ERR(x) that converts result of the macro > > to a system error code. > > > > This would make it less polluted although call sites have to be > > converted to use ENCLS_TO_ERR() . > > > > Can you implement it this way? This is maybe cutting hairs but > > ENCLS_TO_ERR could be a separate commit whereas encls changes will > > merged to main driver commit. For ENCLS_TO_ERR you should add > > "Suggested-by:" tag. > > > > /Jarkko > > You can just send one commit and I'll chop it in the way I see proper :-) > > /jarkko Please also add yourself to the authors list of sgx.h as this is so essential change how the macros work. After the driver is upstreamed those lists will never be updated since commit log will take care of that but for the initial versions they do make sense. [1] [1] I will not update them for minor fixes etc, only for "architecture defining" changes like this. /Jarkko
On 2017-09-15 13:11, Jarkko Sakkinen wrote: > The master branch is needed to support out-of-tree driver up > until le branch is upstreamed Jarkko, are you still maintaining the master branch? The current version doesn't work because there is no way to supply EINITTOKEN. > /Jarkko Jethro
On Fri, Sep 22, 2017 at 01:04:13AM -0700, Jethro Beekman wrote: > On 2017-09-15 13:11, Jarkko Sakkinen wrote: > > The master branch is needed to support out-of-tree driver up > > until le branch is upstreamed > > Jarkko, are you still maintaining the master branch? The current version doesn't > work because there is no way to supply EINITTOKEN. > > > /Jarkko > > Jethro My understanding is that out-of-tree maintainers will patch that. It includes subset of commits of le branch that they need. /Jarkko
diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h index 541abddab6c7..38a088c28d69 100644 --- a/arch/x86/include/asm/sgx.h +++ b/arch/x86/include/asm/sgx.h @@ -84,24 +84,27 @@ enum sgx_commands { EMODT = 0xF, }; -#define __encls_ret(rax, rbx, rcx, rdx) \ +#define IS_ENCLS_FAULT(r) ((r) & 0xffff0000) +#define ENCLS_FAULT_VECTOR(r) ((r) >> 16) + +#define __encls_ret_raw(rax, rbx, rcx, rdx) \ ({ \ int ret; \ asm volatile( \ "1: .byte 0x0f, 0x01, 0xcf;\n\t" \ "2:\n" \ ".section .fixup,\"ax\"\n" \ - "3: jmp 2b\n" \ + "3: shll $16,%%eax\n" \ + " jmp 2b\n" \ ".previous\n" \ - _ASM_EXTABLE(1b, 3b) \ + _ASM_EXTABLE_FAULT(1b, 3b) \ : "=a"(ret) \ : "a"(rax), "b"(rbx), "c"(rcx), "d"(rdx) \ : "memory"); \ ret; \ }) -#ifdef CONFIG_X86_64 -#define __encls(rax, rbx, rcx, rdx...) \ +#define __encls_raw(rax, rbx, rcx, rdx...) \ ({ \ int ret; \ asm volatile( \ @@ -109,36 +112,31 @@ enum sgx_commands { " xor %%eax,%%eax;\n" \ "2:\n" \ ".section .fixup,\"ax\"\n" \ - "3: movq $-1,%%rax\n" \ + "3: shll $16,%%eax\n" \ " jmp 2b\n" \ ".previous\n" \ - _ASM_EXTABLE(1b, 3b) \ + _ASM_EXTABLE_FAULT(1b, 3b) \ : "=a"(ret), "=b"(rbx), "=c"(rcx) \ : "a"(rax), "b"(rbx), "c"(rcx), rdx \ : "memory"); \ ret; \ }) -#else + +#define __encls_ret(rax, rbx, rcx, rdx) \ + ({ \ + int ret = __encls_ret_raw(rax, rbx, rcx, rdx); \ + ret = IS_ENCLS_FAULT(ret) ? -EFAULT : ret; \ + ret; \ + }) + #define __encls(rax, rbx, rcx, rdx...) \ ({ \ - int ret; \ - asm volatile( \ - "1: .byte 0x0f, 0x01, 0xcf;\n\t" \ - " xor %%eax,%%eax;\n" \ - "2:\n" \ - ".section .fixup,\"ax\"\n" \ - "3: mov $-1,%%eax\n" \ - " jmp 2b\n" \ - ".previous\n" \ - _ASM_EXTABLE(1b, 3b) \ - : "=a"(ret), "=b"(rbx), "=c"(rcx) \ - : "a"(rax), "b"(rbx), "c"(rcx), rdx \ - : "memory"); \ + int ret = __encls_raw(rax, rbx, rcx, rdx); \ + ret = IS_ENCLS_FAULT(ret) ? -EFAULT : ret; \ ret; \ }) -#endif -static inline unsigned long __ecreate(struct sgx_pageinfo *pginfo, void *secs) +static inline int __ecreate(struct sgx_pageinfo *pginfo, void *secs) { return __encls(ECREATE, pginfo, secs, "d"(0)); } diff --git a/drivers/platform/x86/intel_sgx/sgx_encl.c b/drivers/platform/x86/intel_sgx/sgx_encl.c index cde732bb6e83..9451369ae43a 100644 --- a/drivers/platform/x86/intel_sgx/sgx_encl.c +++ b/drivers/platform/x86/intel_sgx/sgx_encl.c @@ -524,7 +524,6 @@ int sgx_encl_create(struct sgx_secs *secs) if (ret) { sgx_dbg(encl, "ECREATE returned %ld\n", ret); - ret = -EFAULT; goto out; }
When trapping EINIT, KVM needs the vector if EINIT faults in order to inject the correct fault back into the guest. Add "_raw" suffixed ENCLS macros that utilize _ASM_EXTABLE_FAULT to return the raw fault vector to the caller. The fault vector is shifted into bits 31:16 so that the caller can distinguish between faults and SGX error codes. Modify the existing __encls_ret and __encls macros to return -EFAULT if a fault occurs on ENCLS. This provides userspace with an API that is more consistent with other IOCTLs, and obviates the need for the caller to adjust return values, e.g. sgx_ioc_enclave_create no longer needs to manually return -EFAULT on any __ecreate failure. Coincidentally, these changes also fix a bug where __encls_ret did not update EAX after a fault, e.g. __einit would always return '2' if it faulted. Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- arch/x86/include/asm/sgx.h | 44 +++++++++++++++---------------- drivers/platform/x86/intel_sgx/sgx_encl.c | 1 - 2 files changed, 21 insertions(+), 24 deletions(-)