Add CPU features and fix CPU vendor #2
2 Katılımcı
Bildirimler
Bitiş Tarihi
Bitiş tarihi atanmadı.
Bağımlılıklar
Bağımlılık yok.
Referans: xt-sys/exectos#2
Yeni konuda referans
Bir kullanıcı engelle
"(silindi): prcb-cpu-features" Dalını Sil
Bir dalı silmek kalıcıdır. Her ne kadar silinen dal tamamen kaldırılana kadar çok kısa bir süre yaşamını sürdürse de, çoğu durumda bu işlem GERİ ALINAMAZ. Devam edilsin mi?
Fixed CPU vendor name code which was bugged, and
Extracted CPU vendor name code to
ArpSetCpuVendorAdded CPU features field onto
CPU_IDENTIFICATIONstructAdded few debugging utilities
@@ -187,0 +188,4 @@typedef struct _CPU_FEATURES {union {struct {unsigned int SSE3 : 1;UINT, or maybe something smaller if just one bit used.
Using BYTE instead of unsigned int for the bitfield
@@ -138,0 +139,4 @@typedef struct _CPU_FEATURES {union {struct {unsigned int SSE3 : 1;UINT, or maybe something smaller if just one bit used.
Using BYTE instead of unsigned int for the bitfield
@@ -152,6 +225,7 @@ typedef struct _CPU_IDENTIFICATIONUSHORT Stepping;CPU_VENDOR Vendor;UCHAR VendorName[13];CPU_FEATURES Features;IMHO this should go to PROCESSOR_CONTROL_BLOCK.
Moved to PROCESSOR_CONTROL_BLOCK
@@ -21,9 +21,12 @@#ifdef DBG#define DEBUG 1#define DebugPrint(Format, ...) if(KeDbgPrint) KeDbgPrint(Format, __VA_ARGS__);#define DebugPrintSeparator() DebugPrint(L"===============================================================================\n");Why need this? What would you like to separate?
Only debug messages are printed to serial console.
Just being fancy, but pretty much unnecessary.
Being removed now.
@@ -12,6 +12,7 @@ list(APPEND XTOSKRNL_SOURCE${XTOSKRNL_SOURCE_DIR}/ar/${ARCH}/globals.c${XTOSKRNL_SOURCE_DIR}/ar/${ARCH}/procsup.c${XTOSKRNL_SOURCE_DIR}/ar/${ARCH}/traps.c${XTOSKRNL_SOURCE_DIR}/ar/arcommon.cAR is architecture specific library, so I believe it should not contain any common points. Especially, there are architectures different than x86.
Removed
@@ -8,6 +8,11 @@xtoskrnl/ar/i686/procsup.c missing in this PR
Implemented on i686
@@ -9,2 +9,4 @@#include <xtos.h>#ifdef DEBUGWCHAR VendorName[13];All global variables should be placed in globals.c and globals.h
Removed debug stuff
@@ -11,0 +12,4 @@WCHAR VendorName[13];#endifextern const WCHAR ArCpuFeatureNames[64][16];All global variables should be placed in globals.c and globals.h
Removed debug stuff
@@ -110,0 +127,4 @@*/XTAPIVOIDArpSetCpuVendor(IN PKPROCESSOR_CONTROL_BLOCK Prcb,Why this needs to be so over-complicated? It should be possible to simply store data read from EBX, EDX and ECX registers.
Simplified
@@ -123,8 +168,14 @@ ArpIdentifyProcessor(VOID)CPUID_REGISTERS CpuRegisters;CPUID_SIGNATURE CpuSignature;#ifdef DEBUGAll these
ifdefstatements are completely useless, as DebugPrint on non-debug build is just a NOP.@@ -0,0 +1,25 @@/**AR is architecture specific library, so I believe it should not contain any common points. Especially, there are architectures different than x86 that might get supported in the future.
@@ -0,0 +9,4 @@#include <xtos.h>/* CPU feature names */const WCHAR ArCpuFeatureNames[64][16] = {What is this needed for?
This is a kernel and it needs information about CPU supported features, for example to check minimal requirements and panic if they are not met. They should be stored somewhere in PROCESSOR_CONTROL_BLOCK probably.
Why WCHAR (wide character) needs to be used here?
These are actually debug messages. Stored there temporarily. Removed.
Add PRCB CPU features and fixesiken WIP: Add PRCB CPU features and fixes olarakWIP: Add PRCB CPU features and fixesiken Add PRCB CPU features and fixes olarakAdd PRCB CPU features and fixesiken Add CPU features and fix CPU vendor olarakAdd CPU features and fix CPU vendoriken WIP: Add CPU features and fix CPU vendor olarak9ae270c241hedefindena270c08dcfhedefine zorla gönderildiReopening a clean PR
WIP: Add CPU features and fix CPU vendoriken Add CPU features and fix CPU vendor olarakCould you update your source branch to match the destination?
Updated
@@ -113,4 +117,3 @@/* CPU vendor enumeration list */typedef enum _CPU_VENDOR{CPU_VENDOR_AMD = 0x68747541,Why do we change the values in CPU_VENDOR enum?
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.
That magic numbers have some meaning: HEX(68747541) = STR(Auth)
I know. Why shouldn't we change the values in CPU_VENDOR enum?
@@ -187,0 +193,4 @@typedef struct _CPU_FEATURES {union {struct {BOOLEAN SSE3 : 1;It is not guaranteed that enum has unsigned underlying type.
You pointed the struct in this case, was this a mis-comment?
No, it's not a mistake. BOOLEAN is defined as enum.
May we define
BOOLEAN as enum : BYTE?https://learn.microsoft.com/en-us/windows/win32/winprog/windows-data-types
Swapped all BOOLEAN on the structure with BYTE
@@ -187,0 +263,4 @@BOOLEAN Reserved4 : 1; // Bit 30 is reservedBOOLEAN PBE : 1;};UINT64 Features;Any reason for using UINT64 here?
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)
Swapped all BOOLEAN on the structure with BYTE
@@ -64,4 +68,3 @@/* CPU vendor enumeration list */typedef enum _CPU_VENDOR{CPU_VENDOR_AMD = 0x68747541,Why do we change the values in CPU_VENDOR enum?
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.
That magic numbers have some meaning, eg: HEX(68747541) = STR(Auth)
I know. Why shouldn't we change the values in CPU_VENDOR enum?
@@ -138,0 +144,4 @@typedef struct _CPU_FEATURES {union {struct {BOOLEAN SSE3 : 1;It is not guaranteed that enum has unsigned underlying type.
You pointed the struct in this case, was this a mis-comment?
@@ -138,0 +209,4 @@BOOLEAN Reserved4 : 1; // Bit 30 is reservedBOOLEAN PBE : 1;};UINT64 AsUINT64;Any reason for using UINT64 here?
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 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?
This is a small regression on local commits, I'll push the final structure.
Fixed on last commit.
@@ -154,3 +186,3 @@/* CPU vendor specific quirks */if(Prcb->CpuId.Vendor == CPU_VENDOR_AMD)if (Prcb->CpuId.Vendor == CPU_VENDOR_AMD)Please don't mix code changes with formatting.
Do you expect to just copy and paste the code from one side to another without giving it proper format? Notice it's placed on another function (Diff does not help here)
It does not look like copied from one place into another. There is additional space between if and parenthesis. If it gets removed, git won't see any change in this place.
It is placed in another function, because ArpIdentifyProcessor() got divided.
In the process of moving the code from
ArpIdentifyProcessortoArpSetCpuVendorI formatted the whole function, as it makes sense to tidy up and reformat the function I've touched.@@ -163,3 +199,3 @@{/* Intel CPU */if(Prcb->CpuId.Family == 0xF)if (Prcb->CpuId.Family == 0xF)Please don't mix code changes with formatting.
Do you expect to just copy and paste the code from one side to another without giving it proper format? Notice it's placed on another function (Diff does not help here)
It does not look like copied from one place into another. There is additional space between if and parenthesis. If it gets removed, git won't see any change in this place.
It is placed in another function, because ArpIdentifyProcessor() got divided.
In the process of moving the code from
ArpIdentifyProcessortoArpSetCpuVendorI formatted the whole function, as it makes sense to tidy up and reformat the function I've touched.Değişiklik isteği kapatıldı