xt-sys/exectos
xt-sys/exectos
3
5
Fork 3
Code Issues 3 Pull Requests 1 Actions Projects 1 Releases Wiki Activity

Add CPU features and fix CPU vendor #2

Closed
Ghost wants to merge 5 commits from (deleted):prcb-cpu-features into master
pull from: (deleted):prcb-cpu-features
merge into: xt-sys:master
Conversation 3 Commits 5 Files Changed 8 +165 -144
2 changed files with 165 additions and 144 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 289b1bdebc - Show all commits

156
sdk/xtdk/amd64/artypes.h
View File

@ -111,7 +111,7 @@
#define X86_EFLAGS_ID_MASK 0x00200000 #define X86_EFLAGS_ID_MASK 0x00200000
/* CPUID vendor names */ /* CPUID vendor names */
#define CPUID_VENDOR_NAME_AMD "AuthenticAMD" #define CPUID_VENDOR_NAME_AMD "AuthenticAMD"
#define CPUID_VENDOR_NAME_INTEL "GenuineIntel" #define CPUID_VENDOR_NAME_INTEL "GenuineIntel"
belliash commented 2023-11-27 19:25:51 +01:00
Outdated
Review
Copy Link

Why do we change the values in CPU_VENDOR enum?

Why do we change the values in CPU_VENDOR enum?
Ghost commented 2023-11-27 20:15:32 +01:00
Outdated
Review
Copy Link

The modifications were made to enhance code readability and maintainability, I specifically targeted removing magic numbers, and, to introduce a specific value (CPU_VENDOR_INVALID) to handle cases where the CPU vendor is in an invalid state.

The modifications were made to enhance code readability and maintainability, I specifically targeted removing magic numbers, and, to introduce a specific value (CPU_VENDOR_INVALID) to handle cases where the CPU vendor is in an invalid state.
belliash commented 2023-11-27 20:19:42 +01:00
Outdated
Review
Copy Link

That magic numbers have some meaning: HEX(68747541) = STR(Auth)

That magic numbers have some meaning: HEX(68747541) = STR(Auth)
Ghost commented 2023-11-27 21:09:25 +01:00
Outdated
Review
Copy Link

I know. Why shouldn't we change the values in CPU_VENDOR enum?

I know. Why shouldn't we change the values in CPU_VENDOR enum?
/* CPU vendor enumeration list */ /* CPU vendor enumeration list */
@ -190,81 +190,89 @@ typedef enum _CPUID_FEATURES
} CPUID_FEATURES, *PCPUID_FEATURES; } CPUID_FEATURES, *PCPUID_FEATURES;
Ghost marked this conversation as resolved Outdated
belliash commented 2023-11-20 14:51:18 +01:00
Outdated
Review
Copy Link

UINT, or maybe something smaller if just one bit used.

UINT, or maybe something smaller if just one bit used.
Ghost commented 2023-11-21 20:43:03 +01:00
Outdated
Review
Copy Link

Using BYTE instead of unsigned int for the bitfield

Using BYTE instead of unsigned int for the bitfield
/* CPU features, as reported by CPUID instruction */ /* CPU features, as reported by CPUID instruction */
typedef struct _CPU_FEATURES { typedef union _CPU_FEATURES
union { {
struct { UINT64 Value;
BOOLEAN SSE3 : 1; union
belliash commented 2023-11-27 19:28:52 +01:00
Outdated
Review
Copy Link

It is not guaranteed that enum has unsigned underlying type.

It is not guaranteed that enum has unsigned underlying type.
Ghost commented 2023-11-27 20:18:18 +01:00
Outdated
Review
Copy Link

You pointed the struct in this case, was this a mis-comment?

You pointed the struct in this case, was this a mis-comment?
belliash commented 2023-11-27 20:21:25 +01:00
Outdated
Review
Copy Link

No, it's not a mistake. BOOLEAN is defined as enum.

No, it's not a mistake. BOOLEAN is defined as enum.
Ghost commented 2023-11-27 20:58:13 +01:00
Outdated
Review
Copy Link

May we define BOOLEAN as enum : BYTE?
https://learn.microsoft.com/en-us/windows/win32/winprog/windows-data-types

May we define `BOOLEAN as enum : BYTE`? https://learn.microsoft.com/en-us/windows/win32/winprog/windows-data-types
Ghost commented 2023-11-27 22:08:08 +01:00
Outdated
Review
Copy Link

Swapped all BOOLEAN on the structure with BYTE

Swapped all BOOLEAN on the structure with BYTE
BOOLEAN PCLMUL : 1; {
BOOLEAN DTES64 : 1; UINT32 Ecx;
BOOLEAN MONITOR : 1; UINT32 ExtendedFeatures;
BOOLEAN DS_CPL : 1; struct
BOOLEAN VMX : 1; {
BOOLEAN SMX : 1; BYTE SSE3 : 1;
BOOLEAN EST : 1; BYTE PCLMUL : 1;
BOOLEAN TM2 : 1; BYTE DTES64 : 1;
BOOLEAN SSSE3 : 1; BYTE MONITOR : 1;
BOOLEAN CID : 1; BYTE DS_CPL : 1;
BOOLEAN SDBG : 1; BYTE VMX : 1;
BOOLEAN FMA : 1; BYTE SMX : 1;
BOOLEAN CX16 : 1; BYTE EST : 1;
BOOLEAN XTPR : 1; BYTE TM2 : 1;
BOOLEAN PDCM : 1; BYTE SSSE3 : 1;
BOOLEAN Reserved1 : 1; // Bit 16 is reserved BYTE CID : 1;
BOOLEAN PCID : 1; BYTE SDBG : 1;
BOOLEAN DCA : 1; BYTE FMA : 1;
BOOLEAN SSE4_1 : 1; BYTE CX16 : 1;
BOOLEAN SSE4_2 : 1; BYTE XTPR : 1;
BOOLEAN X2APIC : 1; BYTE PDCM : 1;
BOOLEAN MOVBE : 1; BYTE Reserved4 : 1; // Bit 16 is reserved
BOOLEAN POPCNT : 1; BYTE PCID : 1;
BOOLEAN TSC_DEADLINE : 1; BYTE DCA : 1;
BOOLEAN AES : 1; BYTE SSE4_1 : 1;
BOOLEAN XSAVE : 1; BYTE SSE4_2 : 1;
BOOLEAN OSXSAVE : 1; BYTE X2APIC : 1;
BOOLEAN AVX : 1; BYTE MOVBE : 1;
BOOLEAN F16C : 1; BYTE POPCNT : 1;
BOOLEAN RDRAND : 1; BYTE TSC_DEADLINE : 1;
BOOLEAN HYPERVISOR : 1; BYTE AES : 1;
BYTE XSAVE : 1;
BYTE OSXSAVE : 1;
BYTE AVX : 1;
BYTE F16C : 1;
BYTE RDRAND : 1;
BYTE HYPERVISOR : 1;
};
}; };
UINT32 ExtendedFeatures; union
}; {
union { UINT32 Edx;
struct { UINT32 Features;
BOOLEAN FPU : 1; struct
BOOLEAN VME : 1; {
BOOLEAN DE : 1; BYTE FPU : 1;
BOOLEAN PSE : 1; BYTE VME : 1;
BOOLEAN TSC : 1; BYTE DE : 1;
BOOLEAN MSR : 1; BYTE PSE : 1;
BOOLEAN PAE : 1; BYTE TSC : 1;
BOOLEAN MCE : 1; BYTE MSR : 1;
BOOLEAN CX8 : 1; BYTE PAE : 1;
BOOLEAN APIC : 1; BYTE MCE : 1;
BOOLEAN Reserved2 : 1; // Bit 10 is reserved BYTE CX8 : 1;
BOOLEAN SEP : 1; BYTE APIC : 1;
BOOLEAN MTRR : 1; BYTE Reserved1 : 1; // Bit 10 is reserved
BOOLEAN PGE : 1; BYTE SEP : 1;
BOOLEAN MCA : 1; BYTE MTRR : 1;
BOOLEAN CMOV : 1; BYTE PGE : 1;
BOOLEAN PAT : 1; BYTE MCA : 1;
BOOLEAN PSE36 : 1; BYTE CMOV : 1;
BOOLEAN PSN : 1; BYTE PAT : 1;
BOOLEAN CLFLUSH : 1; BYTE PSE36 : 1;
BOOLEAN Reserved3 : 1; // Bit 20 is reserved BYTE PSN : 1;
BOOLEAN DS : 1; BYTE CLFLUSH : 1;
BOOLEAN ACPI : 1; BYTE Reserved2 : 1; // Bit 20 is reserved
BOOLEAN MMX : 1; BYTE DS : 1;
BOOLEAN FXSR : 1; BYTE ACPI : 1;
BOOLEAN SSE : 1; BYTE MMX : 1;
BOOLEAN SSE2 : 1; BYTE FXSR : 1;
belliash commented 2023-11-27 19:30:00 +01:00
Outdated
Review
Copy Link

Any reason for using UINT64 here?

Any reason for using UINT64 here?
Ghost commented 2023-11-27 20:21:05 +01:00
Outdated
Review
Copy Link

I've specifically added that to access the whole value AsUINT64, maybe it's unnecesary right now, but added it as a plus. I took some inspiration from this code, that does the same:
xtchain/binaries/x86_64-w64-mingw32/include/winhvplatformdefs.h:29
(look for typedef union WHV_CAPABILITY_FEATURES)

I've specifically added that to access the whole value AsUINT64, maybe it's unnecesary right now, but added it as a plus. I took some inspiration from this code, that does the same: xtchain/binaries/x86_64-w64-mingw32/include/winhvplatformdefs.h:29 (look for typedef union WHV_CAPABILITY_FEATURES)
Ghost commented 2023-11-27 22:08:21 +01:00
Outdated
Review
Copy Link

Swapped all BOOLEAN on the structure with BYTE

Swapped all BOOLEAN on the structure with BYTE
BOOLEAN SS : 1; BYTE SSE : 1;
BOOLEAN HTT : 1; BYTE SSE2 : 1;
BOOLEAN TM : 1; BYTE SS : 1;
BOOLEAN Reserved4 : 1; // Bit 30 is reserved BYTE HTT : 1;
BOOLEAN PBE : 1; BYTE TM : 1;
BYTE Reserved3 : 1; // Bit 30 is reserved
BYTE PBE : 1;
};
}; };
UINT64 Features;
};
} CPU_FEATURES, *PCPU_FEATURES; } CPU_FEATURES, *PCPU_FEATURES;
/* CPUID requests */ /* CPUID requests */

153
sdk/xtdk/i686/artypes.h
View File

@ -62,7 +62,7 @@
#define SEGMENT_GS 0x65 #define SEGMENT_GS 0x65
/* CPUID vendor names */ /* CPUID vendor names */
#define CPUID_VENDOR_NAME_AMD "AuthenticAMD" #define CPUID_VENDOR_NAME_AMD "AuthenticAMD"
#define CPUID_VENDOR_NAME_INTEL "GenuineIntel" #define CPUID_VENDOR_NAME_INTEL "GenuineIntel"
belliash commented 2023-11-27 19:33:43 +01:00
Outdated
Review
Copy Link

Why do we change the values in CPU_VENDOR enum?

Why do we change the values in CPU_VENDOR enum?
Ghost commented 2023-11-27 20:16:14 +01:00
Outdated
Review
Copy Link

The modifications were made to enhance code readability and maintainability, I specifically targeted removing magic numbers, and, to introduce a specific value (CPU_VENDOR_INVALID) to handle cases where the CPU vendor is in an invalid state.

The modifications were made to enhance code readability and maintainability, I specifically targeted removing magic numbers, and, to introduce a specific value (CPU_VENDOR_INVALID) to handle cases where the CPU vendor is in an invalid state.
belliash commented 2023-11-27 20:23:02 +01:00
Outdated
Review
Copy Link

That magic numbers have some meaning, eg: HEX(68747541) = STR(Auth)

That magic numbers have some meaning, eg: HEX(68747541) = STR(Auth)
Ghost commented 2023-11-27 21:09:51 +01:00
Outdated
Review
Copy Link

I know. Why shouldn't we change the values in CPU_VENDOR enum?

I know. Why shouldn't we change the values in CPU_VENDOR enum?
/* CPU vendor enumeration list */ /* CPU vendor enumeration list */
@ -141,76 +141,89 @@ typedef enum _CPUID_FEATURES
} CPUID_FEATURES, *PCPUID_FEATURES; } CPUID_FEATURES, *PCPUID_FEATURES;
Ghost marked this conversation as resolved Outdated
belliash commented 2023-11-20 14:51:27 +01:00
Outdated
Review
Copy Link

UINT, or maybe something smaller if just one bit used.

UINT, or maybe something smaller if just one bit used.
Ghost commented 2023-11-21 20:43:29 +01:00
Outdated
Review
Copy Link

Using BYTE instead of unsigned int for the bitfield

Using BYTE instead of unsigned int for the bitfield
/* CPU features, as reported by CPUID instruction */ /* CPU features, as reported by CPUID instruction */
typedef struct _CPU_FEATURES { typedef union _CPU_FEATURES
union { {
struct { UINT64 Value;
BOOLEAN SSE3 : 1; union
belliash commented 2023-11-27 19:32:49 +01:00
Outdated
Review
Copy Link

It is not guaranteed that enum has unsigned underlying type.

It is not guaranteed that enum has unsigned underlying type.
Ghost commented 2023-11-27 20:18:39 +01:00
Outdated
Review
Copy Link

You pointed the struct in this case, was this a mis-comment?

You pointed the struct in this case, was this a mis-comment?
BOOLEAN PCLMUL : 1; {
BOOLEAN DTES64 : 1; UINT32 Ecx;
BOOLEAN MONITOR : 1; UINT32 ExtendedFeatures;
BOOLEAN DS_CPL : 1; struct
BOOLEAN VMX : 1; {
BOOLEAN SMX : 1; BYTE SSE3 : 1;
BOOLEAN EST : 1; BYTE PCLMUL : 1;
BOOLEAN TM2 : 1; BYTE DTES64 : 1;
BOOLEAN SSSE3 : 1; BYTE MONITOR : 1;
BOOLEAN CID : 1; BYTE DS_CPL : 1;
BOOLEAN SDBG : 1; BYTE VMX : 1;
BOOLEAN FMA : 1; BYTE SMX : 1;
BOOLEAN CX16 : 1; BYTE EST : 1;
BOOLEAN XTPR : 1; BYTE TM2 : 1;
BOOLEAN PDCM : 1; BYTE SSSE3 : 1;
BOOLEAN Reserved1 : 1; // Bit 16 is reserved BYTE CID : 1;
BOOLEAN PCID : 1; BYTE SDBG : 1;
BOOLEAN DCA : 1; BYTE FMA : 1;
BOOLEAN SSE4_1 : 1; BYTE CX16 : 1;
BOOLEAN SSE4_2 : 1; BYTE XTPR : 1;
BOOLEAN X2APIC : 1; BYTE PDCM : 1;
BOOLEAN MOVBE : 1; BYTE Reserved1 : 1; // Bit 16 is reserved
BOOLEAN POPCNT : 1; BYTE PCID : 1;
BOOLEAN TSC_DEADLINE : 1; BYTE DCA : 1;
BOOLEAN AES : 1; BYTE SSE4_1 : 1;
BOOLEAN XSAVE : 1; BYTE SSE4_2 : 1;
BOOLEAN OSXSAVE : 1; BYTE X2APIC : 1;
BOOLEAN AVX : 1; BYTE MOVBE : 1;
BOOLEAN F16C : 1; BYTE POPCNT : 1;
BOOLEAN RDRAND : 1; BYTE TSC_DEADLINE : 1;
BOOLEAN HYPERVISOR : 1; BYTE AES : 1;
BOOLEAN FPU : 1; BYTE XSAVE : 1;
BOOLEAN VME : 1; BYTE OSXSAVE : 1;
BOOLEAN DE : 1; BYTE AVX : 1;
BOOLEAN PSE : 1; BYTE F16C : 1;
BOOLEAN TSC : 1; BYTE RDRAND : 1;
BOOLEAN MSR : 1; BYTE HYPERVISOR : 1;
BOOLEAN PAE : 1; };
BOOLEAN MCE : 1; };
BOOLEAN CX8 : 1; union
BOOLEAN APIC : 1; {
BOOLEAN Reserved2 : 1; // Bit 10 is reserved UINT32 Edx;
BOOLEAN SEP : 1; UINT32 Features;
BOOLEAN MTRR : 1; struct
BOOLEAN PGE : 1; {
BOOLEAN MCA : 1; BYTE FPU : 1;
BOOLEAN CMOV : 1; BYTE VME : 1;
BOOLEAN PAT : 1; BYTE DE : 1;
BOOLEAN PSE36 : 1; BYTE PSE : 1;
BOOLEAN PSN : 1; BYTE TSC : 1;
BOOLEAN CLFLUSH : 1; BYTE MSR : 1;
BOOLEAN Reserved3 : 1; // Bit 20 is reserved BYTE PAE : 1;
BOOLEAN DS : 1; BYTE MCE : 1;
BOOLEAN ACPI : 1; BYTE CX8 : 1;
BOOLEAN MMX : 1; BYTE APIC : 1;
BOOLEAN FXSR : 1; BYTE Reserved2 : 1; // Bit 10 is reserved
BOOLEAN SSE : 1; BYTE SEP : 1;
BOOLEAN SSE2 : 1; BYTE MTRR : 1;
BOOLEAN SS : 1; BYTE PGE : 1;
BOOLEAN HTT : 1; BYTE MCA : 1;
BOOLEAN TM : 1; BYTE CMOV : 1;
BOOLEAN Reserved4 : 1; // Bit 30 is reserved BYTE PAT : 1;
BOOLEAN PBE : 1; BYTE PSE36 : 1;
BYTE PSN : 1;
BYTE CLFLUSH : 1;
belliash commented 2023-11-27 19:33:24 +01:00
Outdated
Review
Copy Link

Any reason for using UINT64 here?

Any reason for using UINT64 here?
Ghost commented 2023-11-27 20:04:15 +01:00
Outdated
Review
Copy Link

I've specifically added that to access the whole value AsUINT64, maybe it's unnecesary right now, but added it as a plus. I took some inspiration from this code, that does the same:
xtchain/binaries/x86_64-w64-mingw32/include/winhvplatformdefs.h:29
(look for typedef union WHV_CAPABILITY_FEATURES)

I've specifically added that to access the whole value `AsUINT64`, maybe it's unnecesary right now, but added it as a plus. I took some inspiration from this code, that does the same: xtchain/binaries/x86_64-w64-mingw32/include/winhvplatformdefs.h:29 (look for `typedef union WHV_CAPABILITY_FEATURES`)
belliash commented 2023-11-27 20:25:07 +01:00
Outdated
Review
Copy Link

I didn't notice the difference between i686 and amd64. In one case it is a structure containing 2 unions. Here we got 1 bit structure. Does it mean, that CPUID returns different data across archs?

I didn't notice the difference between i686 and amd64. In one case it is a structure containing 2 unions. Here we got 1 bit structure. Does it mean, that CPUID returns different data across archs?
Ghost commented 2023-11-27 21:10:45 +01:00
Outdated
Review
Copy Link

This is a small regression on local commits, I'll push the final structure.

This is a small regression on local commits, I'll push the final structure.
Ghost commented 2023-11-27 22:07:31 +01:00
Outdated
Review
Copy Link

Fixed on last commit.

Fixed on last commit.
BYTE Reserved3 : 1; // Bit 20 is reserved
BYTE DS : 1;
BYTE ACPI : 1;
BYTE MMX : 1;
BYTE FXSR : 1;
BYTE SSE : 1;
BYTE SSE2 : 1;
BYTE SS : 1;
BYTE HTT : 1;
BYTE TM : 1;
BYTE Reserved4 : 1; // Bit 30 is reserved
BYTE PBE : 1;
};
}; };
UINT64 AsUINT64;
};
} CPU_FEATURES, *PCPU_FEATURES; } CPU_FEATURES, *PCPU_FEATURES;
Ghost marked this conversation as resolved Outdated
belliash commented 2023-11-20 14:51:45 +01:00
Outdated
Review
Copy Link

IMHO this should go to PROCESSOR_CONTROL_BLOCK.

IMHO this should go to PROCESSOR_CONTROL_BLOCK.
Ghost commented 2023-11-21 20:48:59 +01:00
Outdated
Review
Copy Link

Moved to PROCESSOR_CONTROL_BLOCK

Moved to PROCESSOR_CONTROL_BLOCK
/* CPUID requests */ /* CPUID requests */