Reviewed 2 of 2 files at r1.
Reviewable status: all 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: Intel) (waiting on @dimakuv)

LibOS/shim/test/regression/cpuid.c, line 75 at r1 (raw file):

0x00

I'd either write 0 or 0x0, the current version looks like it's suggesting that this is a one-byte argument.

LibOS/shim/test/regression/cpuid.c, line 78 at r1 (raw file):

    if (r.eax || r.ebx || r.ecx || r.edx)
        abort();
    memset(&r, 0, sizeof(r));

Could you set this to something non-zero, so we could detect a bug if some of the registers are not cleared by Graphene? Same in other similar places in this file.

LibOS/shim/test/regression/cpuid.c, line 86 at r1 (raw file):

static void test_cpuid_leaf_not_recognized(void) {
    /* in case of unrecognized leaves, Graphene returns info for highest basic information leaf */

Uhm, really? Is this what x86 does?

LibOS/shim/test/regression/cpuid.c, line 92 at r1 (raw file):

    /* we don't care about return values, just check that they are not all-zeros */
    if (!r.eax && !r.ebx && !r.ecx && !r.edx)
        abort();

My Intel CPU says: "Program received signal SIGABRT, Aborted." :)

Pal/src/host/Linux-SGX/db_misc.c, line 323 at r1 (raw file):

    {.leaf = 0x0F, .zero_subleaf = false, .cache = true},  /* Intel RDT Monitoring */
    {.leaf = 0x10, .zero_subleaf = false, .cache = true},  /* RDT/L2/L3 Cache Allocation Tech */
    /* NOTE: 0x11 leaf is not recognized, see code below */

What's the difference between unrecognized and reserved? I can't find any info about these leaves in SDM, but maybe I'm looking at a wrong place.

Pal/src/host/Linux-SGX/db_misc.c, line 369 at r1 (raw file):

    }

    /* a few basic leaves are considered reserved and always return zeros */

[citation needed]

Pal/src/host/Linux-SGX/db_misc.c, line 388 at r1 (raw file):

    if (!known_leaf) {
        /* leaf is not recognized (EAX value is outside of recongized range for CPUID), return info
         * for highest basic information leaf (see cpuid_known_leaves table; currently 0x1F) */

[citation needed]

Pal/src/host/Linux-SGX/db_misc.c, line 390 at r1 (raw file):

         * for highest basic information leaf (see cpuid_known_leaves table; currently 0x1F) */
        leaf = 0x1F;
        for (unsigned int i = 0; i < ARRAY_SIZE(cpuid_known_leaves); i++) {

should be size_t

Reviewable status: 0 of 2 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: Intel), "fixup! " found in commit messages' one-liners (waiting on @mkow)

LibOS/shim/test/regression/cpuid.c, line 75 at r1 (raw file):

Done.

LibOS/shim/test/regression/cpuid.c, line 78 at r1 (raw file):

Done.

LibOS/shim/test/regression/cpuid.c, line 86 at r1 (raw file):

Yes. See Intel SDM or here: https://www.felixcloutier.com/x86/cpuid (the very end of the pseudo-code, the DEFAULT case).

LibOS/shim/test/regression/cpuid.c, line 92 at r1 (raw file):

Done. I changed this check, but I'm actually curious what "highest basic" CPUID leaf returns all-zeros. Well, I guess this is possible, e.g. some kind of keylocker CPUID leaf is the last one on your CPU and it is set to all zeros.

Pal/src/host/Linux-SGX/db_misc.c, line 323 at r1 (raw file):

You should be looking at the "Operation" section (pseudo-code) of the CPUID instruction. In my Intel SDM, this is Vol. 2A, Chapter 3.2, pages 3-234. Or check pseudo-code here: https://www.felixcloutier.com/x86/cpuid (e.g., search for EAX = 8H:).

Reserved always return all-zeros. Unrecognized return the same as the "highest basic supported CPUID leaf".

Pal/src/host/Linux-SGX/db_misc.c, line 369 at r1 (raw file):

Done.

Pal/src/host/Linux-SGX/db_misc.c, line 388 at r1 (raw file):

Done.

Pal/src/host/Linux-SGX/db_misc.c, line 390 at r1 (raw file):

Done.

Reviewed 2 of 2 files at r2.
Reviewable status: all files reviewed, 9 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)

LibOS/shim/test/regression/cpuid.c, line 92 at r1 (raw file):

On my CPU 0x13 returns all-zeros but 0x1b returns highest supported info.

LibOS/shim/test/regression/cpuid.c, line 116 at r2 (raw file):

static void test_cpuid_leaf_invalid(void) {
    /* Graphene returns all zeros for CPUID leaves 0x40000000 - 0x4FFFFFFF ("no virtualization") */

We should return highest supported leaf info here - technically that could be all zeroes, but most likely it's not. It's also most likely not important, so not blocking.

Pal/src/host/Linux-SGX/db_misc.c, line 323 at r1 (raw file):

On my CPU this leaf is reserved.

Pal/src/host/Linux-SGX/db_misc.c, line 390 at r1 (raw file):

Not done.

Pal/src/host/Linux-SGX/db_misc.c, line 325 at r2 (raw file):

    /* NOTE: 0x11 leaf is not recognized, see code below */
    {.leaf = 0x12, .zero_subleaf = false, .cache = true},  /* Intel SGX Capability */
    /* NOTE: 0x13 leaf is not recognized, see code below */

On my CPU this leaf is reserved.

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

a discussion (no related file):

make our CPUID logic exactly the same as in the latest Intel SDM documentation

This sentence in my commit message is now not entirely correct. I will replace it with something like make our CPUID logic conform to the actual CPU behavior and to the latest Intel SDM documentation.

LibOS/shim/test/regression/cpuid.c, line 92 at r1 (raw file):

Done. Changed to 0x1b here. See also my comments below.

LibOS/shim/test/regression/cpuid.c, line 116 at r2 (raw file):

I also checked that on my local machine, these leafs return the highest supported leaf info. However, this looks to directly contradict to what the Intel SDM says: No existing or future CPU will return processor identification or feature information (clearly by returning the highest supported leaf info, we do return feature information). So what the hack is happening here, I don't understand. I'll keep our Graphene logic as-is for now, but adding a comment.

Pal/src/host/Linux-SGX/db_misc.c, line 323 at r1 (raw file):

Done. On my CPU as well. So it looks like 0x11 and 0x13 are reserved after all. So I'm adding them as such and adding a comment (because the Intel SDM says otherwise...).

Pal/src/host/Linux-SGX/db_misc.c, line 390 at r1 (raw file):

Done.

Pal/src/host/Linux-SGX/db_misc.c, line 325 at r2 (raw file):

Done.

Reviewed 2 of 2 files at r3.
Reviewable status: all files reviewed, 10 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)

LibOS/shim/test/regression/cpuid.c, line 116 at r2 (raw file):

On the other hand it states that these leafs are invalid and it also states that for invalid leafs it returns highest basic info...
I guess it's just a messy documentation :)

Pal/src/host/Linux-SGX/db_misc.c, line 360 at r3 (raw file):

     * they return all zeros on bare metal; runtimes like JVM query these leaves to learn about
     * the underlying virtualization software */
    if (leaf >= 0x40000000 && leaf <= 0x4FFFFFFF) {

Why don't we remove this special case? It would be consistent with SDM and actual CPUs then...

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

LibOS/shim/test/regression/cpuid.c, line 116 at r2 (raw file):

Done. You're right, it's probably just messy documentation.

Pal/src/host/Linux-SGX/db_misc.c, line 360 at r3 (raw file):

Done.

Reviewed 2 of 2 files at r4.
Reviewable status: all files reviewed, 9 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)

Reviewed 2 of 2 files at r4.
Reviewable status: all files reviewed, 1 unresolved discussion, "fixup! " found in commit messages' one-liners (waiting on @dimakuv)

a discussion (no related file):

heh, typical x86

Pal/src/host/Linux-SGX/db_misc.c, line 323 at r1 (raw file):

e.g., search for EAX = 8H:

What I meant is that there's completely zero information about 0x11 and 0x13 leaves (under which I posted my original comment), so how do we know they are "reserved" and not "unrecognized"? From manual testing on a random x86 CPU I guess?

Update: I see from the comment below that I guessed correctly :)

Reviewed 1 of 1 files at r5.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dimakuv)

Reviewed 1 of 1 files at r5.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dimakuv)

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

Pal/src/host/Linux-SGX/db_misc.c, line 323 at r1 (raw file):

there's completely zero information about 0x11 and 0x13 leaves (under which I posted my original comment), so how do we know they are "reserved" and not "unrecognized"?

I assumed that "zero information" in the Intel SDM means that these leaves are "unrecognized". But I was wrong.