[Pal/Linux-SGX] Get rid of sgx driver submodule

[woju]

Description of the changes

• link-intel-driver.py is imported from oscarlab/graphene-sgx-driver and the submodule is removed.
• Interactive input() is removed, because it is unwieldy in build system.
• There are some changes in includes and include dirs, to include only the new file.

How to test this PR?

Jenkins, as usual.

---

This change is 

[mkow]

Reviewed 13 of 15 files at r1.
Reviewable status: 13 of 15 files reviewed, 8 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @woju)

---

Documentation/building.rst, line 175 at r1 (raw file):

   git submodule update --init -- Pal/src/host/Linux-SGX/sgx-driver
   cd Pal/src/host/Linux-SGX/sgx-driver

What about this?

---

Documentation/cloud-deployment.rst, line 43 at r1 (raw file):

       git clone https://github.com/oscarlab/graphene.git
       cd graphene
       git submodule update --init -- Pal/src/host/Linux-SGX/sgx-driver/

I think this whole line is not needed now.

---

Documentation/quickstart.rst, line 58 at r1 (raw file):

      git clone https://github.com/oscarlab/graphene.git
      cd graphene
      git submodule update --init -- Pal/src/host/Linux-SGX/sgx-driver/

ditto

---

Pal/src/host/Linux-SGX/gsgx.h.in, line 5 at r1 (raw file):

 *                    Dmitrii Kuvaiskii <[email protected]>
 */

Isn't this a 1-1 duplication from the gsgx repo? Seems quite dangerous...

---

Pal/src/host/Linux-SGX/link-intel-driver.py, line 11 at r1 (raw file):

        'include/uapi/asm/sgx_oot.h': '/dev/sgx/enclave',
        'include/uapi/asm/sgx.h':     '/dev/sgx/enclave',
        'sgx_in_kernel.h':            '/dev/sgx/enclave',

ditto (question to @dimakuv)

---

Pal/src/host/Linux-SGX/link-intel-driver.py, line 21 at r1 (raw file):

      - include/uapi/asm/sgx_oot.h for DCAP 1.6+ version of the driver
        (https://github.com/intel/SGXDataCenterAttestationPrimitives)
      - include/uapi/asm/sgx.h for in-kernel 20+ version of the driver

IMO no reason to support this archaic version

---

Pal/src/host/Linux-SGX/link-intel-driver.py, line 71 at r1 (raw file):

if __name__ == "__main__":

Please use single quotes for consistency

---

Pal/src/host/Linux-SGX/sgx_in_kernel.h, line 10 at r1 (raw file):

 *       with driver versions 32 and 36, it *may* be out of sync with newer
 *       versions. If possible, use the header found on the system instead of
 *       this one. */

@dimakuv Do we actually need this header? There was some reason I think, but I don't remember it now. I myself just used the headers from /usr/src.

[dimakuv]

Reviewed 15 of 15 files at r1.
Reviewable status: all files reviewed, 12 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @mkow and @woju)

---

LICENSE.addendum.txt, line 4 at r1 (raw file):

Graphene on an SGX host requires a kernel driver, which is separately licensed under
the GPL.

Actually, I recommend to remove the whole para. The "kernel driver" here means the GSGX kernel module -- which only does FSGSBASE enabling (insecurely). This GSGX module is deprecated and is in fact not needed in Linux 5.9+ (and we ship patches to Linux 5.4 to remove the need for this GSGX module).

TLDR: The less we mention GSGX kernel module, the better.

---

Documentation/building.rst, line 175 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

What about this?

+1, needs to be fixed to git clone ....

---

Documentation/cloud-deployment.rst, line 43 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

I think this whole line is not needed now.

+1, we are left with only one submodule now (LTP tests), and this is not needed for a casual user.

---

Documentation/quickstart.rst, line 58 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

ditto

+1

---

Pal/src/host/Linux-SGX/gsgx.h.in, line 5 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Isn't this a 1-1 duplication from the gsgx repo? Seems quite dangerous...

Why dangerous? Woju's idea is to deprecate and remove the graphene-sgx-driver repo altogether (some day).

---

Pal/src/host/Linux-SGX/gsgx.h.in, line 19 at r1 (raw file):

#define GSGX_FILE  "/dev/gsgx"
#define GSGX_MINOR MISC_DYNAMIC_MINOR

These two lines are not needed, remove them.

---

Pal/src/host/Linux-SGX/gsgx.h.in, line 57 at r1 (raw file):

#ifdef SGX_DCAP
# define SGX_DCAP_16_OR_LATER 1

So SGX_DCAP_16_OR_LATER simply becomes an alias to SGX_DCAP. It would be better to remove all Graphene code under the code path of ifndef SGX_DCAP_16_OR_LATER (and replace the macro with SGX_DCAP where needed). Could you add this change, @woju ?

---

Pal/src/host/Linux-SGX/link-intel-driver.py, line 11 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

ditto (question to @dimakuv)

What is the question? Do we need this header file shipped with Graphene? Yes, we need it because of usability reasons (people want a one-click build).

---

Pal/src/host/Linux-SGX/link-intel-driver.py, line 21 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

IMO no reason to support this archaic version

Yes, agreed. Let's remove this comment and the corresponding entry in DRIVER_VERSIONS.

---

Pal/src/host/Linux-SGX/link-intel-driver.py, line 55 at r1 (raw file):

        sys.exit(1)

    if not isgx_driver_path:

But in the above code you exit with failure. You'll never reach this code. Please move this under except:.

---

Pal/src/host/Linux-SGX/sgx_in_kernel.h, line 10 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

@dimakuv Do we actually need this header? There was some reason I think, but I don't remember it now. I myself just used the headers from /usr/src.

We need this header, at least for some time more. It was an ask from other people: they don't want to bother typing in the path to the SGX driver; and the interface to the SGX driver became quite stable. So casual users can just use this header and expect it to work in 95% of the cases.

[woju]

Reviewable status: 8 of 17 files reviewed, 13 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @dimakuv and @mkow)

---

LICENSE.addendum.txt, line 4 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Actually, I recommend to remove the whole para. The "kernel driver" here means the GSGX kernel module -- which only does FSGSBASE enabling (insecurely). This GSGX module is deprecated and is in fact not needed in Linux 5.9+ (and we ship patches to Linux 5.4 to remove the need for this GSGX module).

TLDR: The less we mention GSGX kernel module, the better.

Done.

---

Documentation/building.rst, line 175 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

+1, needs to be fixed to git clone ....

Done.

---

Documentation/cloud-deployment.rst, line 43 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

+1, we are left with only one submodule now (LTP tests), and this is not needed for a casual user.

Done.

---

Documentation/quickstart.rst, line 58 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

+1

Done.

---

Pal/src/host/Linux-SGX/gsgx.h.in, line 1 at r1 (raw file):

/* SPDX-License-Identifier: GPL-2.0-only */

@dimakuv Would you kindly relicence this to LGPL-3.0-or-later?

---

Pal/src/host/Linux-SGX/gsgx.h.in, line 5 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Why dangerous? Woju's idea is to deprecate and remove the graphene-sgx-driver repo altogether (some day).

About the only dangerous thing is GSGX_FILE if that would diverge. GSGX_MINOR is probably unneeded, as are #include <linux/*> (x2), but I wasn't sure, so I didn't mess with it. There were two files that imported it like this and I didn't read them to check if this is really needed.

---

Pal/src/host/Linux-SGX/gsgx.h.in, line 19 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

These two lines are not needed, remove them.

GSGX_FILE is certainly needed: https://github.com/oscarlab/graphene/blob/8a381fab5c087cf8388a4c9311c888a5562ad3de/Pal/src/host/Linux-SGX/sgx_framework.c#L19.

I removed GSGX_MINOR.

---

Pal/src/host/Linux-SGX/gsgx.h.in, line 57 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

So SGX_DCAP_16_OR_LATER simply becomes an alias to SGX_DCAP. It would be better to remove all Graphene code under the code path of ifndef SGX_DCAP_16_OR_LATER (and replace the macro with SGX_DCAP where needed). Could you add this change, @woju ?

I'm not sure if there isn't anything that relies on this macro being defined, so I'm doing this as separate commit, should we need to revert this.

---

Pal/src/host/Linux-SGX/link-intel-driver.py, line 21 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Yes, agreed. Let's remove this comment and the corresponding entry in DRIVER_VERSIONS.

Done.

---

Pal/src/host/Linux-SGX/link-intel-driver.py, line 55 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

But in the above code you exit with failure. You'll never reach this code. Please move this under except:.

Nope, this is testing for "defined but empty", which is a correct, well-defined state. See for yourself:

ISGX_DRIVER_PATH= ./link-intel-driver.py < gsgx.h.in > gsgx.h

---

Pal/src/host/Linux-SGX/link-intel-driver.py, line 71 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Please use single quotes for consistency

Done.

---

Pal/src/host/Linux-SGX/sgx_in_kernel.h, line 10 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

We need this header, at least for some time more. It was an ask from other people: they don't want to bother typing in the path to the SGX driver; and the interface to the SGX driver became quite stable. So casual users can just use this header and expect it to work in 95% of the cases.

Let's keep it for now.

[dimakuv]

Reviewed 9 of 9 files at r2.
Reviewable status: all files reviewed, 10 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @mkow and @woju)

---

Pal/src/host/Linux-SGX/gsgx.h.in, line 1 at r1 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

@dimakuv Would you kindly relicence this to LGPL-3.0-or-later?

Yes, now that it is not used in the Linux kernel module, we should relicense to LGPL 3.0. Feel free to modify this line.

---

Pal/src/host/Linux-SGX/gsgx.h.in, line 19 at r1 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

GSGX_FILE is certainly needed: https://github.com/oscarlab/graphene/blob/8a381fab5c087cf8388a4c9311c888a5562ad3de/Pal/src/host/Linux-SGX/sgx_framework.c#L19.

I removed GSGX_MINOR.

OK, true.

---

Pal/src/host/Linux-SGX/link-intel-driver.py, line 55 at r1 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

Nope, this is testing for "defined but empty", which is a correct, well-defined state. See for yourself:

ISGX_DRIVER_PATH= ./link-intel-driver.py < gsgx.h.in > gsgx.h

Ah, so the user must specify ISGX_DRIVER_PATH, even an empty one (if empty, then the default header is used). Can we add this to the error message above (You can define an empty ISGX_DRIVER_PATH="" to use the default in-kernel driver's C header), something like this. Also, doesn't it mean that our documentation (Quick Start, Building) is not really correct?

[woju]

Reviewable status: 15 of 19 files reviewed, 9 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @dimakuv and @mkow)

---

Pal/src/host/Linux-SGX/link-intel-driver.py, line 55 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Ah, so the user must specify ISGX_DRIVER_PATH, even an empty one (if empty, then the default header is used). Can we add this to the error message above (You can define an empty ISGX_DRIVER_PATH="" to use the default in-kernel driver's C header), something like this. Also, doesn't it mean that our documentation (Quick Start, Building) is not really correct?

Done.

[dimakuv]

Reviewed 4 of 4 files at r3.
Reviewable status: all files reviewed, 8 unresolved discussions, not enough approvals from maintainers (1 more required) (waiting on @mkow)

[jurobystricky]

Reviewable status: all files reviewed, 9 unresolved discussions, not enough approvals from maintainers (1 more required) (waiting on @mkow and @woju)

---

Pal/src/host/Linux-SGX/link-intel-driver.py, line 10 at r3 (raw file):

        'sgx_user.h':                 '/dev/isgx',
        'include/uapi/asm/sgx_oot.h': '/dev/sgx/enclave',
        'sgx_in_kernel.h':            '/dev/sgx/enclave',

The latest kernel device (in-tree) node name is /dev/sgx_enclave. There will be most likely a symlink to maintain backwards compatibility, however DRIVER_VERSIONS should contain an additional entry "'sgx_in_kernel..h :' /dev/sgx_enclave '""

[dimakuv]

Reviewable status: all files reviewed, 9 unresolved discussions, not enough approvals from maintainers (1 more required) (waiting on @jurobystricky and @mkow)

---

Pal/src/host/Linux-SGX/link-intel-driver.py, line 10 at r3 (raw file):

Previously, jurobystricky wrote…

The latest kernel device (in-tree) node name is /dev/sgx_enclave. There will be most likely a symlink to maintain backwards compatibility, however DRIVER_VERSIONS should contain an additional entry "'sgx_in_kernel..h :' /dev/sgx_enclave '""

This won't work. There can be no two entries here with the same key. What we can do is to probe explicitly in this Python code whether dev/sgx/enclave exists, and if not, set dev_path to /dev/sgx_enclave. Or we can just postpone it to another PR.

[woju]

Reviewable status: all files reviewed, 9 unresolved discussions, not enough approvals from maintainers (1 more required) (waiting on @jurobystricky and @mkow)

---

Pal/src/host/Linux-SGX/link-intel-driver.py, line 11 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

What is the question? Do we need this header file shipped with Graphene? Yes, we need it because of usability reasons (people want a one-click build).

@mkow ?

---

Pal/src/host/Linux-SGX/link-intel-driver.py, line 10 at r3 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This won't work. There can be no two entries here with the same key. What we can do is to probe explicitly in this Python code whether dev/sgx/enclave exists, and if not, set dev_path to /dev/sgx_enclave. Or we can just postpone it to another PR.

Definitely postpone. The proper place for such thing would be a config option to meson.

[jurobystricky]

I believe this file should be also updated accordingly:
https://github.com/oscarlab/graphene/blob/woju/get-rid-of-sgx-driver-submodule/Tools/gsc/test/Makefile

[woju]

I believe this file should be also updated accordingly:
https://github.com/oscarlab/graphene/blob/woju/get-rid-of-sgx-driver-submodule/Tools/gsc/test/Makefile

What's wrong with this file?

[jurobystricky]

I believe this file should be also updated accordingly:
https://github.com/oscarlab/graphene/blob/woju/get-rid-of-sgx-driver-submodule/Tools/gsc/test/Makefile

What's wrong with this file?

If I am reading this patch correctly, the gsc containers are built with in-tree kernel now. So the line:

DEVICES_VOLUMES = --device=/dev/gsgx --device=/dev/${SGX_DEVICE} -v /var/run/aesmd/aesm.socket:/var/run/aesmd/aesm.socket

could/should be changed to:

DEVICES_VOLUMES = --device=/dev/sgx/enclave 

aesmd should not be needed for DCAP/in-tree drivers.
In any case, the proof is in the pudding, you can try to run:

$ cd graphene/Tools/gsc/test
$ make
$ make test

If the tests don't fail, I stand corrected and you can disregard my comments.

[dimakuv]

@vahldiek Could you chime in on the GSC discussion?

Reviewed 1 of 1 files at r4.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @jurobystricky and @mkow)

---

Pal/src/host/Linux-SGX/link-intel-driver.py, line 10 at r3 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

Definitely postpone. The proper place for such thing would be a config option to meson.

OK.

[vahldiek]

GSC is build based on what drivers are specified in the configuration file. For the tests we set the values to Jenkins settings. This has not changed in the PR. What may have tricked you into thinking that it changed is the file in images/ which compiles Graphene for a specific Azure Kubernetes Services (they use DCAP + FSGS patches and hence, need other DEVICES_VOLUMES parameters).

So in my opinion, everything should stay as is for DEVICES_VOLUMES...

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @jurobystricky and @mkow)

[mkow]

Reviewed 1 of 15 files at r1, 7 of 9 files at r2, 4 of 4 files at r3, 1 of 1 files at r4.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @dimakuv and @woju)

---

Documentation/building.rst, line 83 at r4 (raw file):

it is available as separate patches.

Could you point out where they can be found?

---

Documentation/building.rst, line 85 at r4 (raw file):

patches.

Enabling FSGSBASE support requires building and installing a custom kernel with

The new version of the previous sentence doesn't work well with this paragraph.

---

Documentation/building.rst, line 141 at r4 (raw file):

.. warning::

   This module is a |~| quick-and-dirty hack with gaping security hole.

gaping -> dangerous

• please add parenthesis: "(allows unauthorized local privilege escalation)"

---

Documentation/building.rst, line 143 at r4 (raw file):

   This module is a |~| quick-and-dirty hack with gaping security hole.
   "Do not use for production" is not a |~| joke. We use it only for testing
   on very old kernels when the patchset does not apply cleanly.

when -> where

---

Pal/src/host/Linux-SGX/gsgx.h.in, line 5 at r1 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

About the only dangerous thing is GSGX_FILE if that would diverge. GSGX_MINOR is probably unneeded, as are #include <linux/*> (x2), but I wasn't sure, so I didn't mess with it. There were two files that imported it like this and I didn't read them to check if this is really needed.

Ok, if we are going to remove it soon, then please add a comment that this soon will be removed.

---

Pal/src/host/Linux-SGX/link-intel-driver.py, line 52 at r4 (raw file):

    except KeyError:
        print('ISGX_DRIVER_PATH environment variable is undefined. You can define an\n'
            'empty ISGX_DRIVER_PATH="" to use the default in-kernel driver\'s C header.',

an empty ISGX_DRIVER_PATH="" - either an empty or ISGX_DRIVER_PATH=""

---

Pal/src/host/Linux-SGX/link-intel-driver.py, line 52 at r4 (raw file):

    except KeyError:
        print('ISGX_DRIVER_PATH environment variable is undefined. You can define an\n'
            'empty ISGX_DRIVER_PATH="" to use the default in-kernel driver\'s C header.',

this line should be wrapped to the parenthesis (2 more spaces)

[woju]

Reviewable status: 17 of 19 files reviewed, 7 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)

---

Documentation/building.rst, line 83 at r4 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

it is available as separate patches.

Could you point out where they can be found?

Done.

---

Documentation/building.rst, line 85 at r4 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

The new version of the previous sentence doesn't work well with this paragraph.

Done.

---

Documentation/building.rst, line 141 at r4 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

gaping -> dangerous

• please add parenthesis: "(allows unauthorized local privilege escalation)"

Done.

---

Documentation/building.rst, line 143 at r4 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

when -> where

Done.

---

Pal/src/host/Linux-SGX/gsgx.h.in, line 5 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Ok, if we are going to remove it soon, then please add a comment that this soon will be removed.

We're going to keep this file (in some form or another) as long as we support different SGX drivers and it will be actively maintained. For example, as part of going full meson, I'll have #define GSGX_FILE "@SGX_DRIVER_DEV@" (or whatever) substituted here to adjust between different drivers' ideas about /dev.

If you're talking about removing sgx.h from oscarlab/graphene-sgx-driver, I don't think a comment is needed here. I just removed all references to it everywhere and substituted for #includeing this file, so there would be no actionable info.

---

Pal/src/host/Linux-SGX/link-intel-driver.py, line 52 at r4 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

this line should be wrapped to the parenthesis (2 more spaces)

Nope, wrapping to next multiple of 4 spaces is also OK according to PEP-8, and consistent with print() in find_intel_sgx_driver() above. I broke also the first line, so this aligns nicely.

Also, this file is going to be remove as part of meson change.

---

Pal/src/host/Linux-SGX/link-intel-driver.py, line 52 at r4 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

an empty ISGX_DRIVER_PATH="" - either an empty or ISGX_DRIVER_PATH=""

Done.

[mkow]

Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @woju)

---

Pal/src/host/Linux-SGX/gsgx.h.in, line 53 at r6 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

Because our makefiles stink and I don't want to touch them even with a stick.

In this particular case, you'd have to add it everywhere, including this place: https://github.com/oscarlab/graphene/blob/da14ac0297e4747afbc0ce46363fdcab96624b91/python/graphenelibos/sgx_get_token.py#L84-L86

This is out of scope by a great margin.

But aren't you partially getting rid of Makefiles here? Is the problem that in the current construction you can't change the parameters of the Make build from Meson?
If so, can you add a TODO here that once we stop building with Make and fully switch to Meson, then we should get rid of this template and just define the macro in compiler's cmdline invocation in Meson?

Ad. changing sgx_get_token.py: I don't think so, AFAIR the attribute it's checking is generated conditionally on the C macro, but I may be wrong.

[woju]

Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @mkow)

---

Pal/src/host/Linux-SGX/gsgx.h.in, line 53 at r6 (raw file):

attribute it's checking is generated conditionally on the C macro

Yes, this is from generated_offsets. But you get the picture.

in the current construction you can't change the parameters of the Make build from Meson?

Yes, that's the case, because we call them sequentially: first make, then meson, then ninja. That's intended and agreed as part of transition plan. As long as part of the build is done by make, you have to run all of them in this particular order and pass correct arguments to their respective receivers. I don't see a problem here. Also there is no point of adding TODO, because it cannot be done in full meson without changing this, so it's not possible to forget about this.

I don't want to spend any more time on this. All of this will be removed shortly. If you don't want to merge as-is, I'll abandon this MR and will just put this on the bottom of the next MR in the meson queue. Eventually it'll be reduced to empty diff, because, as I wrote, this will be removed. Now is a chance to review this piece by piece, not as 20kloc diff dump.

[mkow]

Reviewable status: all files reviewed, all discussions resolved, not enough approvals from different teams (1 more required, approved so far: ITL)

---

Pal/src/host/Linux-SGX/gsgx.h.in, line 53 at r6 (raw file):

Also there is no point of adding TODO, because it cannot be done in full meson without changing this, so it's not possible to forget about this.

Ok, I hope you're right here :)

I don't want to spend any more time on this. All of this will be removed shortly.

I just want to ensure that something can't be accidentally left in such "hacked around" state.

[dimakuv]

Reviewed 1 of 2 files at r5, 1 of 1 files at r6.
Reviewable status: complete! all files reviewed, all discussions resolved

---

Pal/src/host/Linux-SGX/gsgx.h.in, line 5 at r1 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

I suspect this was needed for older version. I don't think there's much to it.

@mkow These constants (SGX_INVALID_*) are redefined here because of legacy/differing SGX driver versions from https://github.com/intel/linux-sgx-driver/tags. The main (real) reason was that our Jenkins used super-old version 1.8 until very recently, and that version didn't have SGX_INVALID_EINITTOKEN (see comment in this file). So to make it uniform across different OOT driver versions, we added all these constants here.

I shudder at the memory of OOT SGX drivers... This all can be purged now, since we moved to recent-ish SGX drivers with stable constants. Not in this PR of course. But some future PR should remove this gsgx.h alltogether.

[mkow]

Reviewable status: complete! all files reviewed, all discussions resolved

---

Pal/src/host/Linux-SGX/gsgx.h.in, line 5 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

@mkow These constants (SGX_INVALID_*) are redefined here because of legacy/differing SGX driver versions from https://github.com/intel/linux-sgx-driver/tags. The main (real) reason was that our Jenkins used super-old version 1.8 until very recently, and that version didn't have SGX_INVALID_EINITTOKEN (see comment in this file). So to make it uniform across different OOT driver versions, we added all these constants here.

I shudder at the memory of OOT SGX drivers... This all can be purged now, since we moved to recent-ish SGX drivers with stable constants. Not in this PR of course. But some future PR should remove this gsgx.h alltogether.

👍

[dimakuv]

Reviewed 4 of 4 files at r7.
Reviewable status: complete! all files reviewed, all discussions resolved