'sevctl ok' reports wrong C-bit position?

[dubek]

I ran sevctl ok on an AMD Rome machine:

...
[ PASS ]     - Physical address bit reduction: 47
[ PASS ]     - C-bit location: 15
[ PASS ]     - Number of encrypted guests supported simultaneously: 509
...

As far as I understand it, the C-bit location is 47, and the physical address bit reduction is 1 (instead of 48-bit addresses we can only have 47-bit).

The code in sevctl (ok.rs) is:

                        Test {
                            name: "Physical address bit reduction",
                            gen_mask: SEV_MASK,
                            run: Box::new(|| {
                                let res = unsafe { x86_64::__cpuid(0x8000_001f) };
                                let field = res.ebx & 0b1111_1100_0000 >> 6;

                                TestResult {
                                    name: "Physical address bit reduction",
                                    stat: TestState::Pass,
                                    mesg: Some(format!("{}", field)),
                                }
                            }),
                            sub: vec![],
                        },
                        Test {
                            name: "C-bit location",
                            gen_mask: SEV_MASK,
                            run: Box::new(|| {
                                let res = unsafe { x86_64::__cpuid(0x8000_001f) };
                                let field = res.ebx & 0b01_1111;

The "equivalent" code in QEMU (target/i386/sev.c) is:

host_cpuid(0x8000001F, 0, NULL, &ebx, NULL, NULL);
cap->cbitpos = ebx & 0x3f;

/*
 * When SEV feature is enabled, we loose one bit in guest physical
 * addressing.
 */
cap->reduced_phys_bits = 1;

[haraldh]

For the C-bit location 32 should be added or it should be renamed to C-bit location in page table entry.

And Physical address bit reduction might be renamed to Physical address bits

[haraldh]

Physical address bit reduction is taken from the AMD64 Programmer's Manual Volume 3 - E.4.18 - page 635

on Milan this value is 51

[dubek]

For the C-bit location 32 should be added or it should be renamed to C-bit location in page table entry.

I think that the C-bit location ebx mask is wrong:

                                let res = unsafe { x86_64::__cpuid(0x8000_001f) };
                                let field = res.ebx & 0b01_1111;

the mask should be res.ebx & 0b11_1111; (equivalent to qemu's ebx & 0x3f).

[haraldh]

For the C-bit location 32 should be added or it should be renamed to C-bit location in page table entry.

I think that the C-bit location ebx mask is wrong:

                                let res = unsafe { x86_64::__cpuid(0x8000_001f) };
                                let field = res.ebx & 0b01_1111;

the mask should be res.ebx & 0b11_1111; (equivalent to qemu's ebx & 0x3f).

Manual says "5:0 C-bit location in page table entry" .. you are right

[haraldh]

Luckily our enarx sev kernel does the right thing: https://github.com/enarx/enarx/blob/d2d2f60fb6453051a68c5388f1da17cfde6f38dd/internal/shim-sev/src/main.rs#L315-L316

        // mask the other bits
        "and    edx,    0x3f",

[haraldh]

Do you want to create the PR?

[dubek]

Suggested fix for the C-bit location in PR #85 .