Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

X64 boot fixes #115

Closed
wants to merge 21 commits into from
Closed
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions ntoskrnl/fsrtl/stackovf.c
Expand Up @@ -115,7 +115,7 @@ FsRtlWorkerThread(IN PVOID StartContext)
KIRQL Irql;
PLIST_ENTRY Entry;
PWORK_QUEUE_ITEM WorkItem;
ULONG QueueId = (ULONG)StartContext;
ULONG_PTR QueueId = (ULONG_PTR)StartContext;

/* Set our priority according to the queue we're dealing with */
KeSetPriorityThread(&PsGetCurrentThread()->Tcb, LOW_REALTIME_PRIORITY + QueueId);
Expand Down Expand Up @@ -149,7 +149,7 @@ NTAPI
INIT_FUNCTION
FsRtlInitializeWorkerThread(VOID)
{
ULONG i;
ULONG_PTR i;
NTSTATUS Status;
HANDLE ThreadHandle;
OBJECT_ATTRIBUTES ObjectAttributes;
Expand Down
36 changes: 30 additions & 6 deletions ntoskrnl/include/internal/amd64/ke.h
Expand Up @@ -22,18 +22,42 @@
#define X86_CR4_OSFXSR 0x00000200 /* enable FXSAVE/FXRSTOR instructions */
#define X86_CR4_OSXMMEXCPT 0x00000400 /* enable #XF exception */

/* EDX flags */
#define X86_FEATURE_FPU 0x00000001 /* x87 FPU is present */
#define X86_FEATURE_VME 0x00000002 /* Virtual 8086 Extensions are present */
#define X86_FEATURE_DBG 0x00000004 /* Debugging extensions are present */
#define X86_FEATURE_PSE 0x00000008 /* Page Size Extension is present */
#define X86_FEATURE_TSC 0x00000010 /* time stamp counters are present */
#define X86_FEATURE_PAE 0x00000040 /* physical address extension is present */
#define X86_FEATURE_CX8 0x00000100 /* CMPXCHG8B instruction present */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like these are not even used and all the values are hardcoded in KiGetFeatureBits. Feel free to improve that. Or not :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simple enough, I think I will do it then. Readablility of code is important.

#define X86_FEATURE_SYSCALL 0x00000800 /* SYSCALL/SYSRET support present */
#define X86_FEATURE_MTTR 0x00001000 /* Memory type range registers are present */
#define X86_FEATURE_PGE 0x00002000 /* Page Global Enable */
#define X86_FEATURE_CMOV 0x00008000 /* "Conditional move" instruction supported */
#define X86_FEATURE_PAT 0x00010000 /* Page Attribute Table is supported */
#define X86_FEATURE_DS 0x00200000 /* Debug Store is present */
#define X86_FEATURE_MMX 0x00800000 /* MMX extension present */
#define X86_FEATURE_FXSR 0x01000000 /* FXSAVE/FXRSTOR instructions present */
#define X86_FEATURE_SSE 0x02000000 /* SSE extension present */
#define X86_FEATURE_SSE2 0x04000000 /* SSE2 extension present */
#define X86_FEATURE_HT 0x10000000 /* Hyper-Threading present */

/* ECX flags */
#define X86_FEATURE_SSE3 0x00000001 /* SSE3 is supported */
#define X86_FEATURE_MONITOR 0x00000008 /* SSE3 Monitor instructions supported */
#define X86_FEATURE_VMX 0x00000020 /* Virtual Machine eXtensions are available */
#define X86_FEATURE_SSSE3 0x00000200 /* Supplemental SSE3 are available */
#define X86_FEATURE_FMA3 0x00001000 /* Fused multiple-add supported */
#define X86_FEATURE_CX16 0x00002000 /* CMPXCHG16B instruction are available */
#define X86_FEATURE_PCID 0x00020000 /* Process Context IDentifiers are supported */
#define X86_FEATURE_SSE41 0x00080000 /* SSE 4.1 is supported */
#define X86_FEATURE_SSE42 0x00100000 /* SSE 4.2 is supported */
#define X86_FEATURE_POPCNT 0x00800000 /* POPCNT instruction is available */
#define X86_FEATURE_XSAVE 0x04000000 /* XSAVE family are available */

/* EDX extended flags */
#define X86_FEATURE_NX 0x00100000 /* NX support present */

#define X86_EXT_FEATURE_SSE3 0x00000001 /* SSE3 extension present */
#define X86_EXT_FEATURE_3DNOW 0x40000000 /* 3DNOW! extension present */

Expand All @@ -47,12 +71,12 @@
#define X86_MSR_CSTAR 0xC0000083
#define X86_MSR_SFMASK 0xC0000084

#define EFER_SCE 0x01
#define EFER_LME 0x10
#define EFER_LMA 0x40
#define EFER_NXE 0x80
#define EFER_SVME 0x100
#define EFER_FFXSR 0x400
#define EFER_SCE 0x0001
#define EFER_LME 0x0100
#define EFER_LMA 0x0400
#define EFER_NXE 0x0800
#define EFER_SVME 0x1000
#define EFER_FFXSR 0x4000
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch! This commit LGTM.


#define AMD64_TSS 9

Expand Down
4 changes: 2 additions & 2 deletions ntoskrnl/io/pnpmgr/pnpreport.c
Expand Up @@ -425,7 +425,7 @@ IoReportTargetDeviceChange(IN PDEVICE_OBJECT PhysicalDeviceObject,
/* Check for valid PDO */
if (!IopIsValidPhysicalDeviceObject(PhysicalDeviceObject))
{
KeBugCheckEx(PNP_DETECTED_FATAL_ERROR, 0x2, (ULONG)PhysicalDeviceObject, 0, 0);
KeBugCheckEx(PNP_DETECTED_FATAL_ERROR, 0x2, (ULONG_PTR)PhysicalDeviceObject, 0, 0);
}

/* FileObject must be null. PnP will fill in it */
Expand Down Expand Up @@ -476,7 +476,7 @@ IoReportTargetDeviceChangeAsynchronous(IN PDEVICE_OBJECT PhysicalDeviceObject,
/* Check for valid PDO */
if (!IopIsValidPhysicalDeviceObject(PhysicalDeviceObject))
{
KeBugCheckEx(PNP_DETECTED_FATAL_ERROR, 0x2, (ULONG)PhysicalDeviceObject, 0, 0);
KeBugCheckEx(PNP_DETECTED_FATAL_ERROR, 0x2, (ULONG_PTR)PhysicalDeviceObject, 0, 0);
}

/* FileObject must be null. PnP will fill in it */
Expand Down
48 changes: 24 additions & 24 deletions ntoskrnl/ke/amd64/cpu.c
Expand Up @@ -152,31 +152,31 @@ KiGetFeatureBits(VOID)
Prcb->InitialApicId = (UCHAR)(CpuInfo.Ebx >> 24);

/* Convert all CPUID Feature bits into our format */
if (CpuInfo.Edx & 0x00000002) FeatureBits |= KF_V86_VIS | KF_CR4;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style nit: while touching such lines, do we want to enforce the multi-line it () {} pattern?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general I would accept, but not enforce.
In this particular case, I think the function would become unnecessarily verbose and thus less readable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Readability nit: shall we maintain vertical alignment of FeatureBits |= ..., as they were and as defines are?

if (CpuInfo.Edx & 0x00000008) FeatureBits |= KF_LARGE_PAGE | KF_CR4;
if (CpuInfo.Edx & 0x00000010) FeatureBits |= KF_RDTSC;
if (CpuInfo.Edx & 0x00000100) FeatureBits |= KF_CMPXCHG8B;
if (CpuInfo.Edx & 0x00000800) FeatureBits |= KF_FAST_SYSCALL;
if (CpuInfo.Edx & 0x00001000) FeatureBits |= KF_MTRR;
if (CpuInfo.Edx & 0x00002000) FeatureBits |= KF_GLOBAL_PAGE | KF_CR4;
if (CpuInfo.Edx & 0x00008000) FeatureBits |= KF_CMOV;
if (CpuInfo.Edx & 0x00010000) FeatureBits |= KF_PAT;
if (CpuInfo.Edx & 0x00200000) FeatureBits |= KF_DTS;
if (CpuInfo.Edx & 0x00800000) FeatureBits |= KF_MMX;
if (CpuInfo.Edx & 0x01000000) FeatureBits |= KF_FXSR;
if (CpuInfo.Edx & 0x02000000) FeatureBits |= KF_XMMI;
if (CpuInfo.Edx & 0x04000000) FeatureBits |= KF_XMMI64;

if (CpuInfo.Ecx & 0x00000001) FeatureBits |= KF_SSE3;
//if (CpuInfo.Ecx & 0x00000008) FeatureBits |= KF_MONITOR;
//if (CpuInfo.Ecx & 0x00000200) FeatureBits |= KF_SSE3SUP;
if (CpuInfo.Ecx & 0x00002000) FeatureBits |= KF_CMPXCHG16B;
//if (CpuInfo.Ecx & 0x00080000) FeatureBits |= KF_SSE41;
//if (CpuInfo.Ecx & 0x00800000) FeatureBits |= KF_POPCNT;
if (CpuInfo.Ecx & 0x04000000) FeatureBits |= KF_XSTATE;
if (CpuInfo.Edx & X86_FEATURE_VME) FeatureBits |= KF_V86_VIS | KF_CR4;
if (CpuInfo.Edx & X86_FEATURE_PSE) FeatureBits |= KF_LARGE_PAGE | KF_CR4;
if (CpuInfo.Edx & X86_FEATURE_TSC) FeatureBits |= KF_RDTSC;
if (CpuInfo.Edx & X86_FEATURE_CX8) FeatureBits |= KF_CMPXCHG8B;
if (CpuInfo.Edx & X86_FEATURE_SYSCALL) FeatureBits |= KF_FAST_SYSCALL;
if (CpuInfo.Edx & X86_FEATURE_MTTR) FeatureBits |= KF_MTRR;
if (CpuInfo.Edx & X86_FEATURE_PGE) FeatureBits |= KF_GLOBAL_PAGE | KF_CR4;
if (CpuInfo.Edx & X86_FEATURE_CMOV) FeatureBits |= KF_CMOV;
if (CpuInfo.Edx & X86_FEATURE_PAT) FeatureBits |= KF_PAT;
if (CpuInfo.Edx & X86_FEATURE_DS) FeatureBits |= KF_DTS;
if (CpuInfo.Edx & X86_FEATURE_MMX) FeatureBits |= KF_MMX;
if (CpuInfo.Edx & X86_FEATURE_FXSR) FeatureBits |= KF_FXSR;
if (CpuInfo.Edx & X86_FEATURE_SSE) FeatureBits |= KF_XMMI;
if (CpuInfo.Edx & X86_FEATURE_SSE2) FeatureBits |= KF_XMMI64;

if (CpuInfo.Ecx & X86_FEATURE_SSE3) FeatureBits |= KF_SSE3;
//if (CpuInfo.Ecx & X86_FEATURE_MONITOR) FeatureBits |= KF_MONITOR;
//if (CpuInfo.Ecx & X86_FEATURE_SSSE3) FeatureBits |= KF_SSE3SUP;
if (CpuInfo.Ecx & X86_FEATURE_CX16) FeatureBits |= KF_CMPXCHG16B;
//if (CpuInfo.Ecx & X86_FEATURE_SSE41) FeatureBits |= KF_SSE41;
//if (CpuInfo.Ecx & X86_FEATURE_POPCNT) FeatureBits |= KF_POPCNT;
if (CpuInfo.Ecx & X86_FEATURE_XSAVE) FeatureBits |= KF_XSTATE;

/* Check if the CPU has hyper-threading */
if (CpuInfo.Ecx & 0x10000000)
if (CpuInfo.Edx & X86_FEATURE_HT)
{
/* Set the number of logical CPUs */
Prcb->LogicalProcessorsPerPhysicalProcessor = (UCHAR)(CpuInfo.Ebx >> 16);
Expand All @@ -203,7 +203,7 @@ KiGetFeatureBits(VOID)
KiCpuId(&CpuInfo, 0x80000001);

/* Check if NX-bit is supported */
if (CpuInfo.Edx & 0x00100000) FeatureBits |= KF_NX_BIT;
if (CpuInfo.Edx & X86_FEATURE_NX) FeatureBits |= KF_NX_BIT;

/* Now handle each features for each CPU Vendor */
switch (Vendor)
Expand Down
4 changes: 0 additions & 4 deletions ntoskrnl/ke/amd64/kiinit.c
Expand Up @@ -19,8 +19,6 @@

/* GLOBALS *******************************************************************/

extern BOOLEAN RtlpUse16ByteSLists;

/* Function pointer for early debug prints */
ULONG (*FrLdrDbgPrint)(const char *Format, ...);

Expand Down Expand Up @@ -84,8 +82,6 @@ KiInitMachineDependent(VOID)
// KeBugCheckEx(NO_PAGES_AVAILABLE, 2, PAGE_SIZE * 2, 0, 0);
// }

/* Initialize 8/16 bit SList support */
RtlpUse16ByteSLists = (KeFeatureBits & KF_CMPXCHG16B) ? TRUE: FALSE;
}

VOID
Expand Down
5 changes: 5 additions & 0 deletions ntoskrnl/ke/amd64/krnlinit.c
Expand Up @@ -16,6 +16,8 @@
extern ULONG_PTR MainSSDT[];
extern UCHAR MainSSPT[];

extern BOOLEAN RtlpUse16ByteSLists;

/* FUNCTIONS *****************************************************************/

VOID
Expand Down Expand Up @@ -153,6 +155,9 @@ KiInitializeKernel(IN PKPROCESS InitProcess,
/* Set boot-level flags */
KeFeatureBits = Prcb->FeatureBits;

/* Initialize 8/16 bit SList support */
RtlpUse16ByteSLists = (KeFeatureBits & KF_CMPXCHG16B) ? TRUE : FALSE;

/* Set the current MP Master KPRCB to the Boot PRCB */
Prcb->MultiThreadSetMaster = Prcb;

Expand Down
2 changes: 2 additions & 0 deletions ntoskrnl/mm/ARM3/mminit.c
Expand Up @@ -2118,6 +2118,7 @@ MmArmInitSystem(IN ULONG Phase,
TestPte = MiProtoPteToPte(&TempPte);
ASSERT(PointerPte == TestPte);

#ifndef _M_AMD64 // Not working on x64 for obvoius reason
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this work on anything else than x86?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing it might work on PPC/ARM. We seem to be copying the x86 memory layout for those.

/* Try a bunch of random addresses near the end of the address space */
PointerPte = (PMMPTE)0xFFFC8000;
for (j = 0; j < 20; j += 1)
Expand All @@ -2133,6 +2134,7 @@ MmArmInitSystem(IN ULONG Phase,
MI_MAKE_SUBSECTION_PTE(&TempPte, PointerPte);
TestPte = MiSubsectionPteToSubsection(&TempPte);
ASSERT(PointerPte == TestPte);
#endif
#endif

/* Loop all 8 standby lists */
Expand Down
3 changes: 0 additions & 3 deletions ntoskrnl/mm/amd64/init.c
Expand Up @@ -203,9 +203,6 @@ MiInitializePageTable(VOID)
__writecr4(__readcr4() | CR4_PGE);
ASSERT(__readcr4() & CR4_PGE);

/* Enable no execute */
__writemsr(X86_MSR_EFER, __readmsr(X86_MSR_EFER) | EFER_NXE);

/* Loop the user mode PXEs */
for (PointerPxe = MiAddressToPxe(0);
PointerPxe <= MiAddressToPxe(MmHighestUserAddress);
Expand Down
2 changes: 1 addition & 1 deletion ntoskrnl/mm/mminit.c
Expand Up @@ -45,7 +45,7 @@ extern NTSTATUS MiRosTrimCache(ULONG Target, ULONG Priority, PULONG NrFreed);
VOID
INIT_FUNCTION
NTAPI
MiCreateArm3StaticMemoryArea(PVOID BaseAddress, ULONG Size, BOOLEAN Executable)
MiCreateArm3StaticMemoryArea(PVOID BaseAddress, SIZE_T Size, BOOLEAN Executable)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right.
Nit:
MmCreateMemoryArea grep
mm.h has SIZE_T, but marea.c has ULONG_PTR: should the latter be syn'ed too?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First of all it doesn't matter at all, since ULONG_PTR and SIZE_T are technically the same. I also suggest not putting too much "nits" into PR reviews (that's 90% of my PRs at work and I hate it), since we don't want to turn providing an improvement into an obligation to also "fix" the rest of our code. Think about why there is Wine-Staging and why we use it. We don't have Ros-Staging so it could become dev/null at some point. Let's not go there.

{
const ULONG Protection = Executable ? PAGE_EXECUTE_READWRITE : PAGE_READWRITE;
PVOID pBaseAddress = BaseAddress;
Expand Down
8 changes: 4 additions & 4 deletions ntoskrnl/ps/debug.c
Expand Up @@ -60,18 +60,18 @@ PspDumpThreads(BOOLEAN IncludeSystem)
Thread->Tcb.State == Waiting)
{
ULONG i = 0;
PULONG Esp = (PULONG)Thread->Tcb.KernelStack;
PULONG Ebp = (PULONG)Esp[4];
PULONG_PTR Esp = (PULONG_PTR)Thread->Tcb.KernelStack;
PULONG_PTR Ebp = (PULONG_PTR)Esp[4];

/* Print EBP */
DbgPrint("Ebp %p\n", Ebp);

/* Walk it */
while(Ebp != 0 && Ebp >= (PULONG)Thread->Tcb.StackLimit)
while(Ebp != 0 && Ebp >= (PULONG_PTR)Thread->Tcb.StackLimit)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is x86 specific code. A proper fix would be using KeRosDumpStackFrames() or something similar. Alternatively use #ifdef _M_IX86 ... #else DbgPrint("FIXME: Backtrace skipped on non-x86\n") #endif

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, made it conditional. Didn't know that these is x86-specific.

{
/* Print what's on the stack */
DbgPrint("%.8X %.8X%s", Ebp[0], Ebp[1], (i % 8) == 7 ? "\n" : " ");
Ebp = (PULONG)Ebp[0];
Ebp = (PULONG_PTR)Ebp[0];
i++;
}

Expand Down
4 changes: 2 additions & 2 deletions ntoskrnl/ps/process.c
Expand Up @@ -1416,8 +1416,8 @@ NtCreateProcess(OUT PHANDLE ProcessHandle,
"Parent: %p Attributes: %p\n", ParentProcess, ObjectAttributes);

/* Set new-style flags */
if ((ULONG)SectionHandle & 1) Flags |= PROCESS_CREATE_FLAGS_BREAKAWAY;
if ((ULONG)DebugPort & 1) Flags |= PROCESS_CREATE_FLAGS_NO_DEBUG_INHERIT;
if ((ULONG_PTR)SectionHandle & 1) Flags |= PROCESS_CREATE_FLAGS_BREAKAWAY;
if ((ULONG_PTR)DebugPort & 1) Flags |= PROCESS_CREATE_FLAGS_NO_DEBUG_INHERIT;
if (InheritObjectTable) Flags |= PROCESS_CREATE_FLAGS_INHERIT_HANDLES;

/* Call the new API */
Expand Down
2 changes: 1 addition & 1 deletion sdk/include/ndk/ldrfuncs.h
Expand Up @@ -109,7 +109,7 @@ NTAPI
LdrLockLoaderLock(
_In_ ULONG Flags,
_Out_opt_ PULONG Disposition,
_Out_opt_ PULONG Cookie
_Out_opt_ PULONG_PTR Cookie
);

NTSTATUS
Expand Down
5 changes: 4 additions & 1 deletion sdk/lib/rtl/memstream.c
Expand Up @@ -205,6 +205,7 @@ RtlReadOutOfProcessMemoryStream(
ULONG CopyLength;
PRTL_MEMORY_STREAM Stream = IStream_To_RTL_MEMORY_STREAM(This);
SIZE_T Available = (PUCHAR)Stream->End - (PUCHAR)Stream->Current;
SIZE_T nBytesRead = 0;

if (BytesRead)
*BytesRead = 0;
Expand All @@ -218,7 +219,9 @@ RtlReadOutOfProcessMemoryStream(
Stream->Current,
Buffer,
CopyLength,
BytesRead);
&nBytesRead);

*BytesRead = (ULONG)nBytesRead;

if (NT_SUCCESS(Status))
Stream->Current = (PUCHAR)Stream->Current + *BytesRead;
Expand Down
4 changes: 2 additions & 2 deletions sdk/lib/rtl/rtlp.h
Expand Up @@ -27,10 +27,10 @@ extern VOID FASTCALL CHECK_PAGED_CODE_RTL(char *file, int line);
#endif

#define ROUND_DOWN(n, align) \
(((ULONG)(n)) & ~((align) - 1l))
(((ULONG_PTR)(n)) & ~((align) - 1l))

#define ROUND_UP(n, align) \
ROUND_DOWN(((ULONG)(n)) + (align) - 1, (align))
ROUND_DOWN(((ULONG_PTR)(n)) + (align) - 1, (align))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This must be carefully synced with the other ALIGN_MEM_xxx macros etc everywhere else (FreeLdr and elsewhere).
I don't know whether @tkreuzer or @ThFabba have some particular comments about that?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any ALIGN_MEM_...? Can't find them

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, these are called ALIGN_UP_... and ALIGN_DOWN_... :
https://git.reactos.org/?p=reactos.git&a=search&h=HEAD&st=grep&s=ALIGN_UP
https://git.reactos.org/?p=reactos.git&a=search&h=HEAD&st=grep&s=ALIGN_DOWN

Also I think some other ROUND_UP (not necessary in the kernel, but in other places in the code, but related to manipulating (what would be 64 bits) points) may need fixup:
https://git.reactos.org/?p=reactos.git&a=search&h=HEAD&st=grep&s=ROUND_UP
and check also for ROUND_DOWN ...


#define RVA(m, b) ((PVOID)((ULONG_PTR)(b) + (ULONG_PTR)(m)))

Expand Down