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 +358 -45
Ghost commented 2023-11-20 00:29:38 +01:00
First-time contributor
Copy Link

Fixed CPU vendor name code which was bugged, and
Extracted CPU vendor name code to ArpSetCpuVendor

Added CPU features field onto CPU_IDENTIFICATION struct

Added few debugging utilities

Fixed CPU vendor name code which was bugged, and Extracted CPU vendor name code to `ArpSetCpuVendor` Added CPU features field onto `CPU_IDENTIFICATION` struct Added few debugging utilities
belliash requested changes 2023-11-21 20:09:39 +01:00
sdk/xtdk/amd64/artypes.h Outdated
@ -187,0 +188,4 @@
typedef struct _CPU_FEATURES {
union {
struct {
unsigned int SSE3 : 1;
belliash commented 2023-11-20 14:51:18 +01:00
Owner
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
Author
First-time contributor
Copy Link

Using BYTE instead of unsigned int for the bitfield

Using BYTE instead of unsigned int for the bitfield
Ghost marked this conversation as resolved
sdk/xtdk/i686/artypes.h Outdated
@ -138,0 +139,4 @@
typedef struct _CPU_FEATURES {
union {
struct {
unsigned int SSE3 : 1;
belliash commented 2023-11-20 14:51:27 +01:00
Owner
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
Author
First-time contributor
Copy Link

Using BYTE instead of unsigned int for the bitfield

Using BYTE instead of unsigned int for the bitfield
Ghost marked this conversation as resolved
sdk/xtdk/i686/artypes.h Outdated
@ -152,6 +225,7 @@ typedef struct _CPU_IDENTIFICATION
USHORT Stepping;
CPU_VENDOR Vendor;
UCHAR VendorName[13];
CPU_FEATURES Features;
belliash commented 2023-11-20 14:51:45 +01:00
Owner
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
Author
First-time contributor
Copy Link

Moved to PROCESSOR_CONTROL_BLOCK

Moved to PROCESSOR_CONTROL_BLOCK
Ghost marked this conversation as resolved
sdk/xtdk/xtdebug.h Outdated
@ -21,9 +21,12 @@
#ifdef DBG
#define DEBUG 1
#define DebugPrint(Format, ...) if(KeDbgPrint) KeDbgPrint(Format, __VA_ARGS__);
#define DebugPrintSeparator() DebugPrint(L"===============================================================================\n");
belliash commented 2023-11-20 14:51:52 +01:00
Owner
Copy Link

Why need this? What would you like to separate?
Only debug messages are printed to serial console.

Why need this? What would you like to separate? Only debug messages are printed to serial console.
Ghost commented 2023-11-21 20:49:54 +01:00
Author
First-time contributor
Copy Link

Just being fancy, but pretty much unnecessary.
Being removed now.

Just being fancy, but pretty much unnecessary. Being removed now.
Ghost marked this conversation as resolved
xtoskrnl/CMakeLists.txt Outdated
@ -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.c
belliash commented 2023-11-20 14:51:55 +01:00
Owner
Copy Link

AR is architecture specific library, so I believe it should not contain any common points. Especially, there are architectures different than x86.

AR is architecture specific library, so I believe it should not contain any common points. Especially, there are architectures different than x86.
Ghost commented 2023-11-21 22:45:42 +01:00
Author
First-time contributor
Copy Link

Removed

Removed
Ghost marked this conversation as resolved
xtoskrnl/ar/amd64/procsup.c
@ -8,6 +8,11 @@
belliash commented 2023-11-20 14:51:59 +01:00
Owner
Copy Link

xtoskrnl/ar/i686/procsup.c missing in this PR

xtoskrnl/ar/i686/procsup.c missing in this PR
Ghost commented 2023-11-21 22:45:57 +01:00
Author
First-time contributor
Copy Link

Implemented on i686

Implemented on i686
Ghost marked this conversation as resolved
xtoskrnl/ar/amd64/procsup.c Outdated
@ -9,2 +9,4 @@
#include <xtos.h>
#ifdef DEBUG
WCHAR VendorName[13];
belliash commented 2023-11-20 14:52:01 +01:00
Owner
Copy Link

All global variables should be placed in globals.c and globals.h

All global variables should be placed in globals.c and globals.h
Ghost commented 2023-11-21 22:46:24 +01:00
Author
First-time contributor
Copy Link

Removed debug stuff

Removed debug stuff
Ghost marked this conversation as resolved
xtoskrnl/ar/amd64/procsup.c Outdated
@ -11,0 +12,4 @@
WCHAR VendorName[13];
#endif
extern const WCHAR ArCpuFeatureNames[64][16];
belliash commented 2023-11-20 14:52:03 +01:00
Owner
Copy Link

All global variables should be placed in globals.c and globals.h

All global variables should be placed in globals.c and globals.h
Ghost commented 2023-11-21 22:46:51 +01:00
Author
First-time contributor
Copy Link

Removed debug stuff

Removed debug stuff
Ghost marked this conversation as resolved
xtoskrnl/ar/amd64/procsup.c Outdated
@ -110,0 +127,4 @@
*/
XTAPI
VOID
ArpSetCpuVendor(IN PKPROCESSOR_CONTROL_BLOCK Prcb,
belliash commented 2023-11-20 14:52:09 +01:00
Owner
Copy Link

Why this needs to be so over-complicated? It should be possible to simply store data read from EBX, EDX and ECX registers.

Why this needs to be so over-complicated? It should be possible to simply store data read from EBX, EDX and ECX registers.
Ghost commented 2023-11-21 22:08:23 +01:00
Author
First-time contributor
Copy Link

Simplified

XTAPI
VOID ArpSetCpuVendor(IN PKPROCESSOR_CONTROL_BLOCK Prcb,
                     IN CPUID_REGISTERS CpuRegisters) 
{
    *((UINT32 *)(Prcb->CpuId.VendorName))     = CpuRegisters.Ebx;
    *((UINT32 *)(Prcb->CpuId.VendorName + 4)) = CpuRegisters.Edx;
    *((UINT32 *)(Prcb->CpuId.VendorName + 8)) = CpuRegisters.Ecx;
    Prcb->CpuId.VendorName[12]                = '\0';
}
Simplified ```c XTAPI VOID ArpSetCpuVendor(IN PKPROCESSOR_CONTROL_BLOCK Prcb, IN CPUID_REGISTERS CpuRegisters) { *((UINT32 *)(Prcb->CpuId.VendorName)) = CpuRegisters.Ebx; *((UINT32 *)(Prcb->CpuId.VendorName + 4)) = CpuRegisters.Edx; *((UINT32 *)(Prcb->CpuId.VendorName + 8)) = CpuRegisters.Ecx; Prcb->CpuId.VendorName[12] = '\0'; } ```
Ghost marked this conversation as resolved
xtoskrnl/ar/amd64/procsup.c Outdated
@ -123,8 +168,14 @@ ArpIdentifyProcessor(VOID)
CPUID_REGISTERS CpuRegisters;
CPUID_SIGNATURE CpuSignature;
#ifdef DEBUG
belliash commented 2023-11-20 14:52:12 +01:00
Owner
Copy Link

All these ifdef statements are completely useless, as DebugPrint on non-debug build is just a NOP.

All these `ifdef ` statements are completely useless, as DebugPrint on non-debug build is just a NOP.
Ghost marked this conversation as resolved
xtoskrnl/ar/arcommon.c Outdated
@ -0,0 +1,25 @@
/**
belliash commented 2023-11-20 14:52:22 +01:00
Owner
Copy Link

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.

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.
Ghost marked this conversation as resolved
xtoskrnl/ar/arcommon.c Outdated
@ -0,0 +9,4 @@
#include <xtos.h>
/* CPU feature names */
const WCHAR ArCpuFeatureNames[64][16] = {
belliash commented 2023-11-20 14:52:26 +01:00
Owner
Copy Link

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?

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?
Ghost commented 2023-11-21 20:38:16 +01:00
Author
First-time contributor
Copy Link

These are actually debug messages. Stored there temporarily. Removed.

These are actually debug messages. Stored there temporarily. Removed.
Ghost marked this conversation as resolved
Ghost changed title from Add PRCB CPU features and fixes to WIP: Add PRCB CPU features and fixes 2023-11-21 22:37:38 +01:00
Ghost changed title from WIP: Add PRCB CPU features and fixes to Add PRCB CPU features and fixes 2023-11-22 01:13:28 +01:00
Ghost changed title from Add PRCB CPU features and fixes to Add CPU features and fix CPU vendor 2023-11-22 01:14:03 +01:00
Ghost requested review from belliash 2023-11-22 17:11:16 +01:00
Ghost changed title from Add CPU features and fix CPU vendor to WIP: Add CPU features and fix CPU vendor 2023-11-23 16:34:48 +01:00
Ghost force-pushed prcb-cpu-features from 9ae270c241 to a270c08dcf 2023-11-23 23:28:07 +01:00 Compare
Ghost commented 2023-11-23 23:28:45 +01:00
Author
First-time contributor
Copy Link

Reopening a clean PR

Reopening a clean PR
Ghost closed this pull request 2023-11-23 23:28:46 +01:00
Ghost reopened this pull request 2023-11-24 00:36:18 +01:00
Ghost changed title from WIP: Add CPU features and fix CPU vendor to Add CPU features and fix CPU vendor 2023-11-24 00:36:23 +01:00
belliash commented 2023-11-26 16:19:41 +01:00
Owner
Copy Link

Could you update your source branch to match the destination?

Could you update your source branch to match the destination?
Ghost added 1 commit 2023-11-26 18:53:30 +01:00
Ghost added 1 commit 2023-11-27 19:12:11 +01:00
Ghost commented 2023-11-27 19:12:17 +01:00
Author
First-time contributor
Copy Link

Updated

Updated
belliash requested changes 2023-11-27 19:34:56 +01:00
sdk/xtdk/amd64/artypes.h Outdated
@ -113,4 +117,3 @@
/* CPU vendor enumeration list */
typedef enum _CPU_VENDOR
{
CPU_VENDOR_AMD = 0x68747541,
belliash commented 2023-11-27 19:25:51 +01:00
Owner
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
Author
First-time contributor
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
Owner
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
Author
First-time contributor
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?
sdk/xtdk/amd64/artypes.h Outdated
@ -187,0 +193,4 @@
typedef struct _CPU_FEATURES {
union {
struct {
BOOLEAN SSE3 : 1;
belliash commented 2023-11-27 19:28:52 +01:00
Owner
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
Author
First-time contributor
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
Owner
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
Author
First-time contributor
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
Author
First-time contributor
Copy Link

Swapped all BOOLEAN on the structure with BYTE

Swapped all BOOLEAN on the structure with BYTE
sdk/xtdk/amd64/artypes.h Outdated
@ -187,0 +263,4 @@
BOOLEAN Reserved4 : 1; // Bit 30 is reserved
BOOLEAN PBE : 1;
};
UINT64 Features;
belliash commented 2023-11-27 19:30:00 +01:00
Owner
Copy Link

Any reason for using UINT64 here?

Any reason for using UINT64 here?
Ghost commented 2023-11-27 20:21:05 +01:00
Author
First-time contributor
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
Author
First-time contributor
Copy Link

Swapped all BOOLEAN on the structure with BYTE

Swapped all BOOLEAN on the structure with BYTE
sdk/xtdk/i686/artypes.h Outdated
@ -64,4 +68,3 @@
/* CPU vendor enumeration list */
typedef enum _CPU_VENDOR
{
CPU_VENDOR_AMD = 0x68747541,
belliash commented 2023-11-27 19:33:43 +01:00
Owner
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
Author
First-time contributor
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
Owner
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
Author
First-time contributor
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?
sdk/xtdk/i686/artypes.h Outdated
@ -138,0 +144,4 @@
typedef struct _CPU_FEATURES {
union {
struct {
BOOLEAN SSE3 : 1;
belliash commented 2023-11-27 19:32:49 +01:00
Owner
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
Author
First-time contributor
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?
sdk/xtdk/i686/artypes.h Outdated
@ -138,0 +209,4 @@
BOOLEAN Reserved4 : 1; // Bit 30 is reserved
BOOLEAN PBE : 1;
};
UINT64 AsUINT64;
belliash commented 2023-11-27 19:33:24 +01:00
Owner
Copy Link

Any reason for using UINT64 here?

Any reason for using UINT64 here?
Ghost commented 2023-11-27 20:04:15 +01:00
Author
First-time contributor
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
Owner
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
Author
First-time contributor
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
Author
First-time contributor
Copy Link

Fixed on last commit.

Fixed on last commit.
xtoskrnl/ar/amd64/procsup.c
@ -154,3 +186,3 @@
/* CPU vendor specific quirks */
if(Prcb->CpuId.Vendor == CPU_VENDOR_AMD)
if (Prcb->CpuId.Vendor == CPU_VENDOR_AMD)
belliash commented 2023-11-27 19:18:56 +01:00
Owner
Copy Link

Please don't mix code changes with formatting.

Please don't mix code changes with formatting.
Ghost commented 2023-11-27 19:56:24 +01:00
Author
First-time contributor
Copy Link

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)

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)
belliash commented 2023-11-27 19:59:11 +01:00
Owner
Copy Link

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.

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.
Ghost commented 2023-11-27 20:20:28 +01:00
Author
First-time contributor
Copy Link

In the process of moving the code from ArpIdentifyProcessor to ArpSetCpuVendor I formatted the whole function, as it makes sense to tidy up and reformat the function I've touched.

In the process of moving the code from `ArpIdentifyProcessor` to `ArpSetCpuVendor` I formatted the whole function, as it makes sense to tidy up and reformat the function I've touched.
xtoskrnl/ar/i686/procsup.c
@ -163,3 +199,3 @@
{
/* Intel CPU */
if(Prcb->CpuId.Family == 0xF)
if (Prcb->CpuId.Family == 0xF)
belliash commented 2023-11-27 19:34:44 +01:00
Owner
Copy Link

Please don't mix code changes with formatting.

Please don't mix code changes with formatting.
Ghost commented 2023-11-27 19:56:39 +01:00
Author
First-time contributor
Copy Link

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)

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)
belliash commented 2023-11-27 19:59:02 +01:00
Owner
Copy Link

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.

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.
Ghost commented 2023-11-27 20:20:36 +01:00
Author
First-time contributor
Copy Link

In the process of moving the code from ArpIdentifyProcessor to ArpSetCpuVendor I formatted the whole function, as it makes sense to tidy up and reformat the function I've touched.

In the process of moving the code from `ArpIdentifyProcessor` to `ArpSetCpuVendor` I formatted the whole function, as it makes sense to tidy up and reformat the function I've touched.
Ghost added 1 commit 2023-11-27 22:06:49 +01:00
Ghost closed this pull request 2023-11-27 22:51:01 +01:00

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No reviewers
belliash
Labels
Clear labels
  
API CHANGE

Requires or requests API change

  
BUG

Something is not working

  
DUPLICATE

This issue or pull request already exists

  
ENHANCEMENT

New feature request or implementation

  
HELP WANTED

Needs some help

  
IDEA

Ideas and proposals

  
INVALID

Something is wrong

  
MODDING

Visual design modifications

  
QUESTION

More information is needed

  
REFACTORING

Code refactoring

  
SYNC

Synchronization with other project

  
TRANSLATION

Translation and localization issue or change

  
UPSTREAM

The patch needs to be sent upstream

  
WONTFIX

This won't be fixed

No Label
API CHANGE
BUG
DUPLICATE
ENHANCEMENT
HELP WANTED
IDEA
INVALID
MODDING
QUESTION
REFACTORING
SYNC
TRANSLATION
UPSTREAM
WONTFIX
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: xt-sys/exectos#2
No description provided.
Delete Branch "(deleted):prcb-cpu-features"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?