Commit Message

This patch augments SGX selftest with two new tests.
The first test exercises the newly added callback interface, by marking the
whole enclave range as PROT_READ, then calling mprotect() upon #PFs to add
necessary PTE permissions per PFEC (#PF Error Code) until the enclave finishes.
This test also serves as an example to demonstrate the callback interface.
The second test single-steps through __vdso_sgx_enter_enclave() to make sure
the call stack can be unwound at every instruction within that vDSO API. Its
purpose is to validate the hand-crafted CFI directives in the assembly.
Besides the new tests, this patch also fixes minor problems in the Makefile,
such as:
* appended "--build-id=none" to ld command line to suppress the
".note.gnu.build-id section discarded" linker warning.
* removed "--remove-section=.got.plt" from objcopy command line as that
section would never exist in statically linked (enclave) images.
Signed-off-by: Cedric Xing <cedric.xing@intel.com>
---
tools/testing/selftests/x86/sgx/Makefile | 6 +-
tools/testing/selftests/x86/sgx/main.c | 344 ++++++++++++++++++---
tools/testing/selftests/x86/sgx/sgx_call.S | 40 ++-
3 files changed, 343 insertions(+), 47 deletions(-)

Comments

On Fri, 2019-07-12 at 23:51 -0700, Cedric Xing wrote:
> This patch augments SGX selftest with two new tests.> > The first test exercises the newly added callback interface, by marking the> whole enclave range as PROT_READ, then calling mprotect() upon #PFs to add> necessary PTE permissions per PFEC (#PF Error Code) until the enclave finishes.> This test also serves as an example to demonstrate the callback interface.> > The second test single-steps through __vdso_sgx_enter_enclave() to make sure> the call stack can be unwound at every instruction within that vDSO API. Its> purpose is to validate the hand-crafted CFI directives in the assembly.> > Besides the new tests, this patch also fixes minor problems in the Makefile,> such as:> * appended "--build-id=none" to ld command line to suppress the> ".note.gnu.build-id section discarded" linker warning.> * removed "--remove-section=.got.plt" from objcopy command line as that> section would never exist in statically linked (enclave) images.> > Signed-off-by: Cedric Xing <cedric.xing@intel.com>
This is also in "unreviewable" state. And you ignored the comment about the
function names.
All I asked was patch that just changes the calling convention to send v21
and include the vDSO change. How hard can that be?
Also do not add the main LKML (linux-kernel@vger.kernel.org) to these patches
that are not directly against the mainline. That is only causing unnecessary
saturation and noise.
Thank you.
/Jarkko

On 7/13/2019 8:21 AM, Jarkko Sakkinen wrote:
> On Fri, 2019-07-12 at 23:51 -0700, Cedric Xing wrote:>> This patch augments SGX selftest with two new tests.>>>> The first test exercises the newly added callback interface, by marking the>> whole enclave range as PROT_READ, then calling mprotect() upon #PFs to add>> necessary PTE permissions per PFEC (#PF Error Code) until the enclave finishes.>> This test also serves as an example to demonstrate the callback interface.>>>> The second test single-steps through __vdso_sgx_enter_enclave() to make sure>> the call stack can be unwound at every instruction within that vDSO API. Its>> purpose is to validate the hand-crafted CFI directives in the assembly.>>>> Besides the new tests, this patch also fixes minor problems in the Makefile,>> such as:>> * appended "--build-id=none" to ld command line to suppress the>> ".note.gnu.build-id section discarded" linker warning.>> * removed "--remove-section=.got.plt" from objcopy command line as that>> section would never exist in statically linked (enclave) images.>>>> Signed-off-by: Cedric Xing <cedric.xing@intel.com>> > This is also in "unreviewable" state. And you ignored the comment about the> function names.
Like I said, there are plenty of test### functions in the selftests. And
your original version didn't have even a name - everything was inside
main() without any comments. How was your original version "reviewable"?
> All I asked was patch that just changes the calling convention to send v21> and include the vDSO change. How hard can that be?
Your selftest Makefile was broken. You had to run the test manually. It
broke the build on 32-bit platforms.
> Also do not add the main LKML (linux-kernel@vger.kernel.org) to these patches> that are not directly against the mainline. That is only causing unnecessary> saturation and noise.
My apology for that. I'll take note.
> Thank you.> > /Jarkko>

On Sat, Jul 13, 2019 at 10:20:58AM -0700, Xing, Cedric wrote:
> > All I asked was patch that just changes the calling convention to send v21> > and include the vDSO change. How hard can that be?> > Your selftest Makefile was broken. You had to run the test manually. It> broke the build on 32-bit platforms.
1. We don't support 32-bit.
2. All changes should be in their own commits.
/Jarkko

On Sat, Jul 13, 2019 at 10:20:58AM -0700, Xing, Cedric wrote:
> Like I said, there are plenty of test### functions in the selftests. And> your original version didn't have even a name - everything was inside main()> without any comments. How was your original version "reviewable"?
I don't care. We can do better than that.
If you have a specific thing that needs a comment in my selftest, please
remark and I can add it.
As for your last question, I neither care about metaphysics. Please,
stick to the content.
/Jarkko

On 7/14/2019 7:47 AM, Jarkko Sakkinen wrote:
> On Sat, Jul 13, 2019 at 10:20:58AM -0700, Xing, Cedric wrote:>> Like I said, there are plenty of test### functions in the selftests. And>> your original version didn't have even a name - everything was inside main()>> without any comments. How was your original version "reviewable"?> > I don't care. We can do better than that.
Not that they did a poor job. You probably don't understand the
intention here. Naming tests with numbers helps users correlate test
outputs with test functions.
> If you have a specific thing that needs a comment in my selftest, please> remark and I can add it.
You should have enclosed your test code in a function to be separated
from the initialization code (e.g. finding vDSO in memory, locating API,
etc.). My patch actually did that for you.
> As for your last question, I neither care about metaphysics. Please,> stick to the content.
I tried to point out you are implementing a double standard here. Please
do not misinterpret it.
> /Jarkko>