[RFC PATCH v3 1/2] ntdll/tests: Test updating TickCount in user_shared_data.
Original patch from: Andrew Wesie <awesie(a)gmail.com> Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- v3: Server-side implementation of USD timestamp updates. The system time check still has some large diffs compared to native after patch #2, probably because of some different clock granularity between server and client. I didn't include an InterruptTime test because depending on the Windows versions, RtlQueryUnbiasedInterruptTime sometimes doesn't match the USD clock. I'm sending it as RFC as it's still a little bit rough on the edges, and I only tested on Linux so far. dlls/ntdll/tests/time.c | 52 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/dlls/ntdll/tests/time.c b/dlls/ntdll/tests/time.c index 5382a952d8d8..cc6e9f226e63 100644 --- a/dlls/ntdll/tests/time.c +++ b/dlls/ntdll/tests/time.c @@ -18,7 +18,9 @@ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA */ +#define NONAMELESSUNION #include "ntdll_test.h" +#include "ddk/wdm.h" #define TICKSPERSEC 10000000 #define TICKSPERMSEC 10000 @@ -29,6 +31,7 @@ static VOID (WINAPI *pRtlTimeFieldsToTime)( PTIME_FIELDS TimeFields, PLARGE_IN static NTSTATUS (WINAPI *pNtQueryPerformanceCounter)( LARGE_INTEGER *counter, LARGE_INTEGER *frequency ); static NTSTATUS (WINAPI *pRtlQueryTimeZoneInformation)( RTL_TIME_ZONE_INFORMATION *); static NTSTATUS (WINAPI *pRtlQueryDynamicTimeZoneInformation)( RTL_DYNAMIC_TIME_ZONE_INFORMATION *); +static ULONG (WINAPI *pNtGetTickCount)(void); static const int MonthLengths[2][12] = { @@ -153,12 +156,60 @@ static void test_RtlQueryTimeZoneInformation(void) wine_dbgstr_w(tzinfo.DaylightName)); } +static ULONG64 read_ksystem_time(volatile KSYSTEM_TIME *time) +{ + ULONG64 high, low; + + do + { + high = time->High1Time; + low = time->LowPart; + } + while (high != time->High2Time); + + return high << 32 | low; +} + +static void test_NtGetTickCount(void) +{ + KSHARED_USER_DATA *user_shared_data = (void *)0x7ffe0000; + ULONG64 usd_time; + ULONG diff; + int i; + + for (i = 0; i < 5; ++i) + { + LARGE_INTEGER system_time; + + if (user_shared_data->NtMajorVersion <= 5 && user_shared_data->NtMinorVersion <= 1) + usd_time = ((ULONG64)user_shared_data->TickCountLowDeprecated * user_shared_data->TickCountMultiplier) >> 24; + else + usd_time = (read_ksystem_time(&user_shared_data->u.TickCount) * user_shared_data->TickCountMultiplier) >> 24; + + if (!pNtGetTickCount) + diff = GetTickCount() - usd_time; + else + diff = pNtGetTickCount() - usd_time; + todo_wine + ok(diff < 100, "NtGetTickCount - USD->TickCount too high, expected < 100 got %d\n", diff); + + usd_time = read_ksystem_time(&user_shared_data->SystemTime); + NtQuerySystemTime(&system_time); + diff = system_time.QuadPart - usd_time; + todo_wine + ok(diff < 100, "NtQuerySystemTime - USD->SystemTime too high, expected < 100 got %d\n", diff); + + Sleep(50); + } +} + START_TEST(time) { HMODULE mod = GetModuleHandleA("ntdll.dll"); pRtlTimeToTimeFields = (void *)GetProcAddress(mod,"RtlTimeToTimeFields"); pRtlTimeFieldsToTime = (void *)GetProcAddress(mod,"RtlTimeFieldsToTime"); pNtQueryPerformanceCounter = (void *)GetProcAddress(mod, "NtQueryPerformanceCounter"); + pNtGetTickCount = (void *)GetProcAddress(mod,"NtGetTickCount"); pRtlQueryTimeZoneInformation = (void *)GetProcAddress(mod, "RtlQueryTimeZoneInformation"); pRtlQueryDynamicTimeZoneInformation = @@ -169,5 +220,6 @@ START_TEST(time) else win_skip("Required time conversion functions are not available\n"); test_NtQueryPerformanceCounter(); + test_NtGetTickCount(); test_RtlQueryTimeZoneInformation(); } -- 2.26.1
Pages are created on demand, depending on the initial values the process requests. These values usually include NT version information, processor features and number of physical pages. The server maps the pages write-only and the clients map them read-only, then the server updates the timestamps on every page using a 16 ms timeout. Note that it is hard to split the timestamp updates to a separate patch as it would require to map the pages RW on the client side to keep the ntoskrnl updates working, and potentially have some issues with RW pages being shared across clients. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- Note that we could also do one page per process if simpler is better, but that would also increase the work to do on every update. We could also count the number of users on each page and remove the page once no process uses it anymore. But as the number of unique pages should stay low, no cleanup is currently done and memory will be freed on server exit. dlls/ntdll/server.c | 29 ++++++++++++++- dlls/ntdll/tests/time.c | 3 +- dlls/ntoskrnl.exe/instr.c | 12 ------ server/file.h | 2 + server/mapping.c | 77 +++++++++++++++++++++++++++++++++++++++ server/process.c | 2 + server/protocol.def | 12 ++++++ server/trace.c | 22 +++++++++++ 8 files changed, 144 insertions(+), 15 deletions(-) diff --git a/dlls/ntdll/server.c b/dlls/ntdll/server.c index c0dd0f35fc3d..d0483c855e4a 100644 --- a/dlls/ntdll/server.c +++ b/dlls/ntdll/server.c @@ -81,6 +81,7 @@ #include "wine/server.h" #include "wine/debug.h" #include "ntdll_misc.h" +#include "ddk/wdm.h" WINE_DEFAULT_DEBUG_CHANNEL(server); @@ -1517,8 +1518,18 @@ void server_init_process_done(void) PEB *peb = NtCurrentTeb()->Peb; IMAGE_NT_HEADERS *nt = RtlImageNtHeader( peb->ImageBaseAddress ); void *entry = (char *)peb->ImageBaseAddress + nt->OptionalHeader.AddressOfEntryPoint; + obj_handle_t usd_handle; + usd_init_t usd_init; NTSTATUS status; - int suspend; + int suspend, fd, i; + unsigned int usd_size; + + usd_init.number_of_physical_pages = user_shared_data->NumberOfPhysicalPages; + usd_init.nt_major_version = user_shared_data->NtMajorVersion; + usd_init.nt_minor_version = user_shared_data->NtMinorVersion; + usd_init.nt_product_type = user_shared_data->NtProductType; + for (i = 0; i < ARRAY_SIZE(usd_init.processor_features); ++i) + usd_init.processor_features[i] = user_shared_data->ProcessorFeatures[i]; #ifdef __APPLE__ send_server_task_port(); @@ -1541,11 +1552,27 @@ void server_init_process_done(void) #endif req->entry = wine_server_client_ptr( entry ); req->gui = (nt->OptionalHeader.Subsystem != IMAGE_SUBSYSTEM_WINDOWS_CUI); + wine_server_add_data( req, &usd_init, sizeof(usd_init) ); status = wine_server_call( req ); suspend = reply->suspend; + usd_size = reply->usd_size; } SERVER_END_REQ; + if ((fd = receive_fd( &usd_handle )) != -1) + { + void *addr = user_shared_data; + SIZE_T size = usd_size; + ULONG old_prot; + NtProtectVirtualMemory( NtCurrentProcess(), &addr, &size, PAGE_READONLY, &old_prot ); + + munmap( user_shared_data, usd_size ); + if (user_shared_data != mmap( user_shared_data, usd_size, PROT_READ, MAP_SHARED | MAP_FIXED, fd, 0 )) + fatal_error( "failed to remap the process user shared data\n" ); + + close( fd ); + } + assert( !status ); signal_start_process( entry, suspend ); } diff --git a/dlls/ntdll/tests/time.c b/dlls/ntdll/tests/time.c index cc6e9f226e63..6e5d599cb62f 100644 --- a/dlls/ntdll/tests/time.c +++ b/dlls/ntdll/tests/time.c @@ -190,13 +190,12 @@ static void test_NtGetTickCount(void) diff = GetTickCount() - usd_time; else diff = pNtGetTickCount() - usd_time; - todo_wine ok(diff < 100, "NtGetTickCount - USD->TickCount too high, expected < 100 got %d\n", diff); usd_time = read_ksystem_time(&user_shared_data->SystemTime); NtQuerySystemTime(&system_time); diff = system_time.QuadPart - usd_time; - todo_wine + todo_wine_if(diff >= 100) ok(diff < 100, "NtQuerySystemTime - USD->SystemTime too high, expected < 100 got %d\n", diff); Sleep(50); diff --git a/dlls/ntoskrnl.exe/instr.c b/dlls/ntoskrnl.exe/instr.c index 77803f07d725..0973b3a80a07 100644 --- a/dlls/ntoskrnl.exe/instr.c +++ b/dlls/ntoskrnl.exe/instr.c @@ -593,15 +593,6 @@ static void fake_syscall_function(void) } -static void update_shared_data(void) -{ - struct _KUSER_SHARED_DATA *shared_data = (struct _KUSER_SHARED_DATA *)wine_user_shared_data; - - shared_data->u.TickCountQuad = GetTickCount64(); - shared_data->u.TickCount.High2Time = shared_data->u.TickCount.High1Time; -} - - /*********************************************************************** * emulate_instruction * @@ -802,7 +793,6 @@ static DWORD emulate_instruction( EXCEPTION_RECORD *rec, CONTEXT *context ) if (offset <= sizeof(KSHARED_USER_DATA) - data_size) { ULONGLONG temp = 0; - update_shared_data(); memcpy( &temp, wine_user_shared_data + offset, data_size ); store_reg_word( context, instr[2], (BYTE *)&temp, long_op, rex ); context->Rip += prefixlen + len + 2; @@ -823,7 +813,6 @@ static DWORD emulate_instruction( EXCEPTION_RECORD *rec, CONTEXT *context ) if (offset <= sizeof(KSHARED_USER_DATA) - data_size) { - update_shared_data(); switch (*instr) { case 0x8a: store_reg_byte( context, instr[1], wine_user_shared_data + offset, rex ); break; @@ -845,7 +834,6 @@ static DWORD emulate_instruction( EXCEPTION_RECORD *rec, CONTEXT *context ) if (offset <= sizeof(KSHARED_USER_DATA) - data_size) { - update_shared_data(); memcpy( &context->Rax, wine_user_shared_data + offset, data_size ); context->Rip += prefixlen + len + 1; return ExceptionContinueExecution; diff --git a/server/file.h b/server/file.h index 7395814dadd2..96c552bab197 100644 --- a/server/file.h +++ b/server/file.h @@ -173,6 +173,8 @@ extern struct file *get_mapping_file( struct process *process, client_ptr_t base extern void free_mapped_views( struct process *process ); extern int get_page_size(void); +int get_user_shared_data_fd( const usd_init_t *init, unsigned int *size ); + /* device functions */ extern struct object *create_named_pipe_device( struct object *root, const struct unicode_str *name ); diff --git a/server/mapping.c b/server/mapping.c index 6990a1913d76..aae3d0cda972 100644 --- a/server/mapping.c +++ b/server/mapping.c @@ -35,6 +35,7 @@ #define WIN32_NO_STATUS #include "windef.h" #include "winternl.h" +#include "ddk/wdm.h" #include "file.h" #include "handle.h" @@ -943,6 +944,82 @@ int get_page_size(void) return page_mask + 1; } +struct kusd_mapping +{ + struct list entry; + int fd; + usd_init_t init; + KSHARED_USER_DATA *data; +}; + +static const timeout_t kusd_timeout = 16 * -TICKS_PER_SEC / 1000; +static struct list kusd_mappings = LIST_INIT(kusd_mappings); + +static void kusd_mapping_set_current_time( struct kusd_mapping *kusd ) +{ + kusd->data->SystemTime.High2Time = (current_time >> 32); + kusd->data->SystemTime.LowPart = (current_time & 0xffffffff); + kusd->data->SystemTime.High1Time = (current_time >> 32); + + kusd->data->InterruptTime.High2Time = (monotonic_time >> 32); + kusd->data->InterruptTime.LowPart = (monotonic_time & 0xffffffff); + kusd->data->InterruptTime.High1Time = (monotonic_time >> 32); + + kusd->data->TickCount.High2Time = ((monotonic_time / 10000) >> 32); + kusd->data->TickCount.LowPart = ((monotonic_time / 10000) & 0xffffffff); + kusd->data->TickCount.High1Time = ((monotonic_time / 10000) >> 32); + kusd->data->TickCountLowDeprecated = ((monotonic_time / 10000) & 0xffffffff); +} + +static void update_kusd_mappings( void *private ) +{ + struct kusd_mapping *kusd; + + add_timeout_user( kusd_timeout, update_kusd_mappings, NULL ); + + LIST_FOR_EACH_ENTRY( kusd, &kusd_mappings, struct kusd_mapping, entry ) + kusd_mapping_set_current_time( kusd ); +} + +int get_user_shared_data_fd( const usd_init_t *init, unsigned int *size ) +{ + static const WCHAR default_windirW[] = {'C',':','\\','w','i','n','d','o','w','s',0}; + struct kusd_mapping *kusd; + int i; + + LIST_FOR_EACH_ENTRY(kusd, &kusd_mappings, struct kusd_mapping, entry) + { + if (!memcmp(init, &kusd->init, sizeof(usd_init_t))) + goto done; + } + + if (!(kusd = mem_alloc( sizeof(struct kusd_mapping) ))) + return -1; + + kusd->fd = create_temp_file( sizeof(*kusd->data) ); + kusd->data = mmap( NULL, sizeof(*kusd->data), PROT_WRITE, MAP_SHARED, kusd->fd, 0 ); + kusd->init = *init; + + kusd->data->NumberOfPhysicalPages = init->number_of_physical_pages; + kusd->data->NtMajorVersion = init->nt_major_version; + kusd->data->NtMinorVersion = init->nt_minor_version; + kusd->data->NtProductType = init->nt_product_type; + for (i = 0; i < 64; ++i) + kusd->data->ProcessorFeatures[i] = init->processor_features[i]; + + memcpy( kusd->data->NtSystemRoot, default_windirW, sizeof(default_windirW) ); + kusd->data->TickCountMultiplier = 1 << 24; + + kusd_mapping_set_current_time( kusd ); + + if (list_empty( &kusd_mappings )) add_timeout_user( kusd_timeout, update_kusd_mappings, NULL ); + list_add_tail( &kusd_mappings, &kusd->entry ); + +done: + *size = sizeof(*kusd->data); + return kusd->fd; +} + /* create a file mapping */ DECL_HANDLER(create_mapping) { diff --git a/server/process.c b/server/process.c index 73984f363f59..6bb18ec6617a 100644 --- a/server/process.c +++ b/server/process.c @@ -1385,6 +1385,8 @@ DECL_HANDLER(init_process_done) if (req->gui) process->idle_event = create_event( NULL, NULL, 0, 1, 0, NULL ); if (process->debugger) set_process_debug_flag( process, 1 ); reply->suspend = (current->suspend || process->suspend); + + send_client_fd( process, get_user_shared_data_fd( get_req_data(), &reply->usd_size ), -1 ); } /* open a handle to a process */ diff --git a/server/protocol.def b/server/protocol.def index 06a29b153ea0..4a935fd7efcf 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -787,6 +787,16 @@ struct rawinput_device user_handle_t target; }; +/* Initial values for user shared data */ +typedef struct +{ + unsigned int number_of_physical_pages; + unsigned int nt_major_version; + unsigned int nt_minor_version; + unsigned short nt_product_type; + unsigned char processor_features[64]; +} usd_init_t; + /****************************************************************/ /* Request declarations */ @@ -854,8 +864,10 @@ struct rawinput_device mod_handle_t module; /* main module base address */ client_ptr_t ldt_copy; /* address of LDT copy (in thread address space) */ client_ptr_t entry; /* process entry point */ + VARARG(usd,usd_init); /* initial USD values */ @REPLY int suspend; /* is process suspended? */ + unsigned int usd_size; /* size of USD mapping */ @END diff --git a/server/trace.c b/server/trace.c index 2b58ed9fd2c0..31560eaad827 100644 --- a/server/trace.c +++ b/server/trace.c @@ -860,6 +860,28 @@ static void dump_varargs_startup_info( const char *prefix, data_size_t size ) remove_data( size ); } +static void dump_varargs_usd_init( const char *prefix, data_size_t size ) +{ + usd_init_t init; + int i; + + memset( &init, 0, sizeof(init) ); + memcpy( &init, cur_data, min( size, sizeof(init) )); + + fprintf( stderr, + "%s{number_of_physical_pages=%u,nt_major_version=%04x,nt_minor_version=%04x," + "nt_product_type=%02x,processor_features={", + prefix, init.number_of_physical_pages, init.nt_major_version, init.nt_minor_version, + init.nt_product_type ); + for (i = 0; i < ARRAY_SIZE(init.processor_features); ++i) + { + if (i > 0) fputc( ',', stderr ); + fprintf( stderr, "%02x", init.processor_features[i] ); + } + fprintf( stderr, "}}" ); + remove_data( size ); +} + static void dump_varargs_input_records( const char *prefix, data_size_t size ) { const INPUT_RECORD *rec = cur_data; -- 2.26.1
On 4/28/20 10:11 AM, Rémi Bernon wrote:
Pages are created on demand, depending on the initial values the process requests. These values usually include NT version information, processor features and number of physical pages.
The server maps the pages write-only and the clients map them read-only, then the server updates the timestamps on every page using a 16 ms timeout.
Note that it is hard to split the timestamp updates to a separate patch as it would require to map the pages RW on the client side to keep the ntoskrnl updates working, and potentially have some issues with RW pages being shared across clients.
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> ---
Note that we could also do one page per process if simpler is better, but that would also increase the work to do on every update.
One thing that occurs to me as being potentially desirable, if only potentially, is to actually use these counters to implement our tick count APIs, in which case we'd absolutely want to have one page per process. I know we've already had applications that want GetTickCount() to be extremely fast, and that'd allow us to get a little more Unix code out of kernel32. Maybe there's something I'm missing, though.
We could also count the number of users on each page and remove the page once no process uses it anymore. But as the number of unique pages should stay low, no cleanup is currently done and memory will be freed on server exit.
dlls/ntdll/server.c | 29 ++++++++++++++- dlls/ntdll/tests/time.c | 3 +- dlls/ntoskrnl.exe/instr.c | 12 ------ server/file.h | 2 + server/mapping.c | 77 +++++++++++++++++++++++++++++++++++++++ server/process.c | 2 + server/protocol.def | 12 ++++++ server/trace.c | 22 +++++++++++ 8 files changed, 144 insertions(+), 15 deletions(-)
diff --git a/dlls/ntdll/server.c b/dlls/ntdll/server.c index c0dd0f35fc3d..d0483c855e4a 100644 --- a/dlls/ntdll/server.c +++ b/dlls/ntdll/server.c @@ -81,6 +81,7 @@ #include "wine/server.h" #include "wine/debug.h" #include "ntdll_misc.h" +#include "ddk/wdm.h"
WINE_DEFAULT_DEBUG_CHANNEL(server);
@@ -1517,8 +1518,18 @@ void server_init_process_done(void) PEB *peb = NtCurrentTeb()->Peb; IMAGE_NT_HEADERS *nt = RtlImageNtHeader( peb->ImageBaseAddress ); void *entry = (char *)peb->ImageBaseAddress + nt->OptionalHeader.AddressOfEntryPoint; + obj_handle_t usd_handle; + usd_init_t usd_init; NTSTATUS status; - int suspend; + int suspend, fd, i; + unsigned int usd_size; + + usd_init.number_of_physical_pages = user_shared_data->NumberOfPhysicalPages; + usd_init.nt_major_version = user_shared_data->NtMajorVersion; + usd_init.nt_minor_version = user_shared_data->NtMinorVersion; + usd_init.nt_product_type = user_shared_data->NtProductType; + for (i = 0; i < ARRAY_SIZE(usd_init.processor_features); ++i) + usd_init.processor_features[i] = user_shared_data->ProcessorFeatures[i];
#ifdef __APPLE__ send_server_task_port(); @@ -1541,11 +1552,27 @@ void server_init_process_done(void) #endif req->entry = wine_server_client_ptr( entry ); req->gui = (nt->OptionalHeader.Subsystem != IMAGE_SUBSYSTEM_WINDOWS_CUI); + wine_server_add_data( req, &usd_init, sizeof(usd_init) ); status = wine_server_call( req ); suspend = reply->suspend; + usd_size = reply->usd_size; } SERVER_END_REQ;
+ if ((fd = receive_fd( &usd_handle )) != -1) + { + void *addr = user_shared_data; + SIZE_T size = usd_size; + ULONG old_prot; + NtProtectVirtualMemory( NtCurrentProcess(), &addr, &size, PAGE_READONLY, &old_prot ); + + munmap( user_shared_data, usd_size ); + if (user_shared_data != mmap( user_shared_data, usd_size, PROT_READ, MAP_SHARED | MAP_FIXED, fd, 0 )) + fatal_error( "failed to remap the process user shared data\n" ); + + close( fd ); + } + assert( !status ); signal_start_process( entry, suspend ); } diff --git a/dlls/ntdll/tests/time.c b/dlls/ntdll/tests/time.c index cc6e9f226e63..6e5d599cb62f 100644 --- a/dlls/ntdll/tests/time.c +++ b/dlls/ntdll/tests/time.c @@ -190,13 +190,12 @@ static void test_NtGetTickCount(void) diff = GetTickCount() - usd_time; else diff = pNtGetTickCount() - usd_time; - todo_wine ok(diff < 100, "NtGetTickCount - USD->TickCount too high, expected < 100 got %d\n", diff);
usd_time = read_ksystem_time(&user_shared_data->SystemTime); NtQuerySystemTime(&system_time); diff = system_time.QuadPart - usd_time; - todo_wine + todo_wine_if(diff >= 100) ok(diff < 100, "NtQuerySystemTime - USD->SystemTime too high, expected < 100 got %d\n", diff);
Sleep(50); diff --git a/dlls/ntoskrnl.exe/instr.c b/dlls/ntoskrnl.exe/instr.c index 77803f07d725..0973b3a80a07 100644 --- a/dlls/ntoskrnl.exe/instr.c +++ b/dlls/ntoskrnl.exe/instr.c @@ -593,15 +593,6 @@ static void fake_syscall_function(void) }
-static void update_shared_data(void) -{ - struct _KUSER_SHARED_DATA *shared_data = (struct _KUSER_SHARED_DATA *)wine_user_shared_data; - - shared_data->u.TickCountQuad = GetTickCount64(); - shared_data->u.TickCount.High2Time = shared_data->u.TickCount.High1Time; -} - - /*********************************************************************** * emulate_instruction * @@ -802,7 +793,6 @@ static DWORD emulate_instruction( EXCEPTION_RECORD *rec, CONTEXT *context ) if (offset <= sizeof(KSHARED_USER_DATA) - data_size) { ULONGLONG temp = 0; - update_shared_data(); memcpy( &temp, wine_user_shared_data + offset, data_size ); store_reg_word( context, instr[2], (BYTE *)&temp, long_op, rex ); context->Rip += prefixlen + len + 2; @@ -823,7 +813,6 @@ static DWORD emulate_instruction( EXCEPTION_RECORD *rec, CONTEXT *context )
if (offset <= sizeof(KSHARED_USER_DATA) - data_size) { - update_shared_data(); switch (*instr) { case 0x8a: store_reg_byte( context, instr[1], wine_user_shared_data + offset, rex ); break; @@ -845,7 +834,6 @@ static DWORD emulate_instruction( EXCEPTION_RECORD *rec, CONTEXT *context )
if (offset <= sizeof(KSHARED_USER_DATA) - data_size) { - update_shared_data(); memcpy( &context->Rax, wine_user_shared_data + offset, data_size ); context->Rip += prefixlen + len + 1; return ExceptionContinueExecution; diff --git a/server/file.h b/server/file.h index 7395814dadd2..96c552bab197 100644 --- a/server/file.h +++ b/server/file.h @@ -173,6 +173,8 @@ extern struct file *get_mapping_file( struct process *process, client_ptr_t base extern void free_mapped_views( struct process *process ); extern int get_page_size(void);
+int get_user_shared_data_fd( const usd_init_t *init, unsigned int *size ); + /* device functions */
extern struct object *create_named_pipe_device( struct object *root, const struct unicode_str *name ); diff --git a/server/mapping.c b/server/mapping.c index 6990a1913d76..aae3d0cda972 100644 --- a/server/mapping.c +++ b/server/mapping.c @@ -35,6 +35,7 @@ #define WIN32_NO_STATUS #include "windef.h" #include "winternl.h" +#include "ddk/wdm.h"
#include "file.h" #include "handle.h" @@ -943,6 +944,82 @@ int get_page_size(void) return page_mask + 1; }
+struct kusd_mapping +{ + struct list entry; + int fd; + usd_init_t init; + KSHARED_USER_DATA *data; +}; + +static const timeout_t kusd_timeout = 16 * -TICKS_PER_SEC / 1000; +static struct list kusd_mappings = LIST_INIT(kusd_mappings); + +static void kusd_mapping_set_current_time( struct kusd_mapping *kusd ) +{ + kusd->data->SystemTime.High2Time = (current_time >> 32); + kusd->data->SystemTime.LowPart = (current_time & 0xffffffff); + kusd->data->SystemTime.High1Time = (current_time >> 32); + + kusd->data->InterruptTime.High2Time = (monotonic_time >> 32); + kusd->data->InterruptTime.LowPart = (monotonic_time & 0xffffffff); + kusd->data->InterruptTime.High1Time = (monotonic_time >> 32); + + kusd->data->TickCount.High2Time = ((monotonic_time / 10000) >> 32); + kusd->data->TickCount.LowPart = ((monotonic_time / 10000) & 0xffffffff); + kusd->data->TickCount.High1Time = ((monotonic_time / 10000) >> 32); + kusd->data->TickCountLowDeprecated = ((monotonic_time / 10000) & 0xffffffff); +}
Do we want to use atomic writes here, to make sure the compiler does the right thing?
+ +static void update_kusd_mappings( void *private ) +{ + struct kusd_mapping *kusd; + + add_timeout_user( kusd_timeout, update_kusd_mappings, NULL ); + + LIST_FOR_EACH_ENTRY( kusd, &kusd_mappings, struct kusd_mapping, entry ) + kusd_mapping_set_current_time( kusd ); +} + +int get_user_shared_data_fd( const usd_init_t *init, unsigned int *size ) +{ + static const WCHAR default_windirW[] = {'C',':','\\','w','i','n','d','o','w','s',0}; + struct kusd_mapping *kusd; + int i; + + LIST_FOR_EACH_ENTRY(kusd, &kusd_mappings, struct kusd_mapping, entry) + { + if (!memcmp(init, &kusd->init, sizeof(usd_init_t))) + goto done; + } + + if (!(kusd = mem_alloc( sizeof(struct kusd_mapping) ))) + return -1; + + kusd->fd = create_temp_file( sizeof(*kusd->data) ); + kusd->data = mmap( NULL, sizeof(*kusd->data), PROT_WRITE, MAP_SHARED, kusd->fd, 0 ); + kusd->init = *init; + + kusd->data->NumberOfPhysicalPages = init->number_of_physical_pages; + kusd->data->NtMajorVersion = init->nt_major_version; + kusd->data->NtMinorVersion = init->nt_minor_version; + kusd->data->NtProductType = init->nt_product_type; + for (i = 0; i < 64; ++i) + kusd->data->ProcessorFeatures[i] = init->processor_features[i]; + + memcpy( kusd->data->NtSystemRoot, default_windirW, sizeof(default_windirW) ); + kusd->data->TickCountMultiplier = 1 << 24; + + kusd_mapping_set_current_time( kusd ); + + if (list_empty( &kusd_mappings )) add_timeout_user( kusd_timeout, update_kusd_mappings, NULL ); + list_add_tail( &kusd_mappings, &kusd->entry ); + +done: + *size = sizeof(*kusd->data); + return kusd->fd; +} + /* create a file mapping */ DECL_HANDLER(create_mapping) { diff --git a/server/process.c b/server/process.c index 73984f363f59..6bb18ec6617a 100644 --- a/server/process.c +++ b/server/process.c @@ -1385,6 +1385,8 @@ DECL_HANDLER(init_process_done) if (req->gui) process->idle_event = create_event( NULL, NULL, 0, 1, 0, NULL ); if (process->debugger) set_process_debug_flag( process, 1 ); reply->suspend = (current->suspend || process->suspend); + + send_client_fd( process, get_user_shared_data_fd( get_req_data(), &reply->usd_size ), -1 ); }
/* open a handle to a process */ diff --git a/server/protocol.def b/server/protocol.def index 06a29b153ea0..4a935fd7efcf 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -787,6 +787,16 @@ struct rawinput_device user_handle_t target; };
+/* Initial values for user shared data */ +typedef struct +{ + unsigned int number_of_physical_pages; + unsigned int nt_major_version; + unsigned int nt_minor_version; + unsigned short nt_product_type; + unsigned char processor_features[64]; +} usd_init_t; + /****************************************************************/ /* Request declarations */
@@ -854,8 +864,10 @@ struct rawinput_device mod_handle_t module; /* main module base address */ client_ptr_t ldt_copy; /* address of LDT copy (in thread address space) */ client_ptr_t entry; /* process entry point */ + VARARG(usd,usd_init); /* initial USD values */ @REPLY int suspend; /* is process suspended? */ + unsigned int usd_size; /* size of USD mapping */ @END
Maybe it would make more sense to instead pass the whole initial KUSER_SHARED_DATA structure as a VARARG to the server. That lets the server be entirely agnostic as to what's in it aside from timestamps, and makes it even easier to set new fields later.
diff --git a/server/trace.c b/server/trace.c index 2b58ed9fd2c0..31560eaad827 100644 --- a/server/trace.c +++ b/server/trace.c @@ -860,6 +860,28 @@ static void dump_varargs_startup_info( const char *prefix, data_size_t size ) remove_data( size ); }
+static void dump_varargs_usd_init( const char *prefix, data_size_t size ) +{ + usd_init_t init; + int i; + + memset( &init, 0, sizeof(init) ); + memcpy( &init, cur_data, min( size, sizeof(init) )); + + fprintf( stderr, + "%s{number_of_physical_pages=%u,nt_major_version=%04x,nt_minor_version=%04x," + "nt_product_type=%02x,processor_features={", + prefix, init.number_of_physical_pages, init.nt_major_version, init.nt_minor_version, + init.nt_product_type ); + for (i = 0; i < ARRAY_SIZE(init.processor_features); ++i) + { + if (i > 0) fputc( ',', stderr ); + fprintf( stderr, "%02x", init.processor_features[i] ); + } + fprintf( stderr, "}}" ); + remove_data( size ); +} +
In which case you'd probably just get rid of this.
static void dump_varargs_input_records( const char *prefix, data_size_t size ) { const INPUT_RECORD *rec = cur_data;
On 4/28/20 6:40 PM, Zebediah Figura wrote:
On 4/28/20 10:11 AM, Rémi Bernon wrote:
Pages are created on demand, depending on the initial values the process requests. These values usually include NT version information, processor features and number of physical pages.
The server maps the pages write-only and the clients map them read-only, then the server updates the timestamps on every page using a 16 ms timeout.
Note that it is hard to split the timestamp updates to a separate patch as it would require to map the pages RW on the client side to keep the ntoskrnl updates working, and potentially have some issues with RW pages being shared across clients.
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> ---
Note that we could also do one page per process if simpler is better, but that would also increase the work to do on every update.
One thing that occurs to me as being potentially desirable, if only potentially, is to actually use these counters to implement our tick count APIs, in which case we'd absolutely want to have one page per process. I know we've already had applications that want GetTickCount() to be extremely fast, and that'd allow us to get a little more Unix code out of kernel32. Maybe there's something I'm missing, though.
Not sure to see why that would make one-page per process absolutely necessary? Is there any timer here that is process-specific? But yeah I had the same thing in mind for later, and it seems that on Windows the GetTickCount and so on are just reading from the USD, or at least the values perfectly match in the test.
+static void kusd_mapping_set_current_time( struct kusd_mapping *kusd ) +{ + kusd->data->SystemTime.High2Time = (current_time >> 32); + kusd->data->SystemTime.LowPart = (current_time & 0xffffffff); + kusd->data->SystemTime.High1Time = (current_time >> 32); + + kusd->data->InterruptTime.High2Time = (monotonic_time >> 32); + kusd->data->InterruptTime.LowPart = (monotonic_time & 0xffffffff); + kusd->data->InterruptTime.High1Time = (monotonic_time >> 32); + + kusd->data->TickCount.High2Time = ((monotonic_time / 10000) >> 32); + kusd->data->TickCount.LowPart = ((monotonic_time / 10000) & 0xffffffff); + kusd->data->TickCount.High1Time = ((monotonic_time / 10000) >> 32); + kusd->data->TickCountLowDeprecated = ((monotonic_time / 10000) & 0xffffffff); +}
Do we want to use atomic writes here, to make sure the compiler does the right thing?
Possibly and it's probably not a bad thing to add to be safe, indeed. The members are already volatile, so there should be no reordering between each write/read, and the correct way to read the data from what I could see is to compare High1Time with High2Time --but I guess there's some application not doing it right out there.
+ +static void update_kusd_mappings( void *private ) +{ + struct kusd_mapping *kusd; + + add_timeout_user( kusd_timeout, update_kusd_mappings, NULL ); + + LIST_FOR_EACH_ENTRY( kusd, &kusd_mappings, struct kusd_mapping, entry ) + kusd_mapping_set_current_time( kusd ); +} + +int get_user_shared_data_fd( const usd_init_t *init, unsigned int *size ) +{ + static const WCHAR default_windirW[] = {'C',':','\\','w','i','n','d','o','w','s',0}; + struct kusd_mapping *kusd; + int i; + + LIST_FOR_EACH_ENTRY(kusd, &kusd_mappings, struct kusd_mapping, entry) + { + if (!memcmp(init, &kusd->init, sizeof(usd_init_t))) + goto done; + } + + if (!(kusd = mem_alloc( sizeof(struct kusd_mapping) ))) + return -1; + + kusd->fd = create_temp_file( sizeof(*kusd->data) ); + kusd->data = mmap( NULL, sizeof(*kusd->data), PROT_WRITE, MAP_SHARED, kusd->fd, 0 ); + kusd->init = *init; + + kusd->data->NumberOfPhysicalPages = init->number_of_physical_pages; + kusd->data->NtMajorVersion = init->nt_major_version; + kusd->data->NtMinorVersion = init->nt_minor_version; + kusd->data->NtProductType = init->nt_product_type; + for (i = 0; i < 64; ++i) + kusd->data->ProcessorFeatures[i] = init->processor_features[i]; + + memcpy( kusd->data->NtSystemRoot, default_windirW, sizeof(default_windirW) ); + kusd->data->TickCountMultiplier = 1 << 24; + + kusd_mapping_set_current_time( kusd ); + + if (list_empty( &kusd_mappings )) add_timeout_user( kusd_timeout, update_kusd_mappings, NULL ); + list_add_tail( &kusd_mappings, &kusd->entry ); + +done: + *size = sizeof(*kusd->data); + return kusd->fd; +} + /* create a file mapping */ DECL_HANDLER(create_mapping) { diff --git a/server/process.c b/server/process.c index 73984f363f59..6bb18ec6617a 100644 --- a/server/process.c +++ b/server/process.c @@ -1385,6 +1385,8 @@ DECL_HANDLER(init_process_done) if (req->gui) process->idle_event = create_event( NULL, NULL, 0, 1, 0, NULL ); if (process->debugger) set_process_debug_flag( process, 1 ); reply->suspend = (current->suspend || process->suspend); + + send_client_fd( process, get_user_shared_data_fd( get_req_data(), &reply->usd_size ), -1 ); } /* open a handle to a process */ diff --git a/server/protocol.def b/server/protocol.def index 06a29b153ea0..4a935fd7efcf 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -787,6 +787,16 @@ struct rawinput_device user_handle_t target; }; +/* Initial values for user shared data */ +typedef struct +{ + unsigned int number_of_physical_pages; + unsigned int nt_major_version; + unsigned int nt_minor_version; + unsigned short nt_product_type; + unsigned char processor_features[64]; +} usd_init_t; + /****************************************************************/ /* Request declarations */ @@ -854,8 +864,10 @@ struct rawinput_device mod_handle_t module; /* main module base address */ client_ptr_t ldt_copy; /* address of LDT copy (in thread address space) */ client_ptr_t entry; /* process entry point */ + VARARG(usd,usd_init); /* initial USD values */ @REPLY int suspend; /* is process suspended? */ + unsigned int usd_size; /* size of USD mapping */ @END
Maybe it would make more sense to instead pass the whole initial KUSER_SHARED_DATA structure as a VARARG to the server. That lets the server be entirely agnostic as to what's in it aside from timestamps, and makes it even easier to set new fields later.
Yeah, it seemed a little bit overkill at first. An I didn't want to bother including wdm.h here, but why not. As I was doing page de-duplication, it felt nicer to know which information was actually used.
diff --git a/server/trace.c b/server/trace.c index 2b58ed9fd2c0..31560eaad827 100644 --- a/server/trace.c +++ b/server/trace.c @@ -860,6 +860,28 @@ static void dump_varargs_startup_info( const char *prefix, data_size_t size ) remove_data( size ); } +static void dump_varargs_usd_init( const char *prefix, data_size_t size ) +{ + usd_init_t init; + int i; + + memset( &init, 0, sizeof(init) ); + memcpy( &init, cur_data, min( size, sizeof(init) )); + + fprintf( stderr, + "%s{number_of_physical_pages=%u,nt_major_version=%04x,nt_minor_version=%04x,"
+ "nt_product_type=%02x,processor_features={", + prefix, init.number_of_physical_pages, init.nt_major_version, init.nt_minor_version, + init.nt_product_type ); + for (i = 0; i < ARRAY_SIZE(init.processor_features); ++i) + { + if (i > 0) fputc( ',', stderr ); + fprintf( stderr, "%02x", init.processor_features[i] ); + } + fprintf( stderr, "}}" ); + remove_data( size ); +} +
In which case you'd probably just get rid of this.
static void dump_varargs_input_records( const char *prefix, data_size_t size ) { const INPUT_RECORD *rec = cur_data;
If one page per process is preferable, then it can actually be passed to the client earlier, with possibly the version lookup done on the server side, and maybe the cpuinfo and page count as well if the values are identical for 32bit and 64bit processes? So if it's possible, there would be no need to pass anything from the client to the server. If not, then it's just a matter of passing cpu features and page count, which I feel is better than passing the whole initial USD. -- Rémi Bernon <rbernon(a)codeweavers.com>
On 4/28/20 12:11 PM, Rémi Bernon wrote:
On 4/28/20 6:40 PM, Zebediah Figura wrote:
On 4/28/20 10:11 AM, Rémi Bernon wrote:
Pages are created on demand, depending on the initial values the process requests. These values usually include NT version information, processor features and number of physical pages.
The server maps the pages write-only and the clients map them read-only, then the server updates the timestamps on every page using a 16 ms timeout.
Note that it is hard to split the timestamp updates to a separate patch as it would require to map the pages RW on the client side to keep the ntoskrnl updates working, and potentially have some issues with RW pages being shared across clients.
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> ---
Note that we could also do one page per process if simpler is better, but that would also increase the work to do on every update.
One thing that occurs to me as being potentially desirable, if only potentially, is to actually use these counters to implement our tick count APIs, in which case we'd absolutely want to have one page per process. I know we've already had applications that want GetTickCount() to be extremely fast, and that'd allow us to get a little more Unix code out of kernel32. Maybe there's something I'm missing, though.
Not sure to see why that would make one-page per process absolutely necessary? Is there any timer here that is process-specific? But yeah I had the same thing in mind for later, and it seems that on Windows the GetTickCount and so on are just reading from the USD, or at least the values perfectly match in the test.
Oh, I see, I was assuming you were trying to avoid creating pages for processes that didn't need them, rather than trying to reuse pages where they're identical. In retrospect that was dumb of me; we already depend on KUSER_SHARED_DATA for some things.
+static void kusd_mapping_set_current_time( struct kusd_mapping *kusd ) +{ + kusd->data->SystemTime.High2Time = (current_time >> 32); + kusd->data->SystemTime.LowPart = (current_time & 0xffffffff); + kusd->data->SystemTime.High1Time = (current_time >> 32); + + kusd->data->InterruptTime.High2Time = (monotonic_time >> 32); + kusd->data->InterruptTime.LowPart = (monotonic_time & 0xffffffff); + kusd->data->InterruptTime.High1Time = (monotonic_time >> 32); + + kusd->data->TickCount.High2Time = ((monotonic_time / 10000)
32); + kusd->data->TickCount.LowPart = ((monotonic_time / 10000) & 0xffffffff); + kusd->data->TickCount.High1Time = ((monotonic_time / 10000) 32); + kusd->data->TickCountLowDeprecated = ((monotonic_time / 10000) & 0xffffffff); +}
Do we want to use atomic writes here, to make sure the compiler does the right thing?
Possibly and it's probably not a bad thing to add to be safe, indeed. The members are already volatile, so there should be no reordering between each write/read, and the correct way to read the data from what I could see is to compare High1Time with High2Time --but I guess there's some application not doing it right out there.
The problem is that as far as I understand, "volatile" doesn't actually specify that operations are atomic or prevent reordering. $COMPILER might do that in practice, but it's probably better to make explicit what you actually want.
+ +static void update_kusd_mappings( void *private ) +{ + struct kusd_mapping *kusd; + + add_timeout_user( kusd_timeout, update_kusd_mappings, NULL ); + + LIST_FOR_EACH_ENTRY( kusd, &kusd_mappings, struct kusd_mapping, entry ) + kusd_mapping_set_current_time( kusd ); +} + +int get_user_shared_data_fd( const usd_init_t *init, unsigned int *size ) +{ + static const WCHAR default_windirW[] = {'C',':','\\','w','i','n','d','o','w','s',0}; + struct kusd_mapping *kusd; + int i; + + LIST_FOR_EACH_ENTRY(kusd, &kusd_mappings, struct kusd_mapping, entry) + { + if (!memcmp(init, &kusd->init, sizeof(usd_init_t))) + goto done; + } + + if (!(kusd = mem_alloc( sizeof(struct kusd_mapping) ))) + return -1; + + kusd->fd = create_temp_file( sizeof(*kusd->data) ); + kusd->data = mmap( NULL, sizeof(*kusd->data), PROT_WRITE, MAP_SHARED, kusd->fd, 0 ); + kusd->init = *init; + + kusd->data->NumberOfPhysicalPages = init->number_of_physical_pages; + kusd->data->NtMajorVersion = init->nt_major_version; + kusd->data->NtMinorVersion = init->nt_minor_version; + kusd->data->NtProductType = init->nt_product_type; + for (i = 0; i < 64; ++i) + kusd->data->ProcessorFeatures[i] = init->processor_features[i]; + + memcpy( kusd->data->NtSystemRoot, default_windirW, sizeof(default_windirW) ); + kusd->data->TickCountMultiplier = 1 << 24; + + kusd_mapping_set_current_time( kusd ); + + if (list_empty( &kusd_mappings )) add_timeout_user( kusd_timeout, update_kusd_mappings, NULL ); + list_add_tail( &kusd_mappings, &kusd->entry ); + +done: + *size = sizeof(*kusd->data); + return kusd->fd; +} + /* create a file mapping */ DECL_HANDLER(create_mapping) { diff --git a/server/process.c b/server/process.c index 73984f363f59..6bb18ec6617a 100644 --- a/server/process.c +++ b/server/process.c @@ -1385,6 +1385,8 @@ DECL_HANDLER(init_process_done) if (req->gui) process->idle_event = create_event( NULL, NULL, 0, 1, 0, NULL ); if (process->debugger) set_process_debug_flag( process, 1 ); reply->suspend = (current->suspend || process->suspend); + + send_client_fd( process, get_user_shared_data_fd( get_req_data(), &reply->usd_size ), -1 ); } /* open a handle to a process */ diff --git a/server/protocol.def b/server/protocol.def index 06a29b153ea0..4a935fd7efcf 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -787,6 +787,16 @@ struct rawinput_device user_handle_t target; }; +/* Initial values for user shared data */ +typedef struct +{ + unsigned int number_of_physical_pages; + unsigned int nt_major_version; + unsigned int nt_minor_version; + unsigned short nt_product_type; + unsigned char processor_features[64]; +} usd_init_t; + /****************************************************************/ /* Request declarations */ @@ -854,8 +864,10 @@ struct rawinput_device mod_handle_t module; /* main module base address */ client_ptr_t ldt_copy; /* address of LDT copy (in thread address space) */ client_ptr_t entry; /* process entry point */ + VARARG(usd,usd_init); /* initial USD values */ @REPLY int suspend; /* is process suspended? */ + unsigned int usd_size; /* size of USD mapping */ @END
Maybe it would make more sense to instead pass the whole initial KUSER_SHARED_DATA structure as a VARARG to the server. That lets the server be entirely agnostic as to what's in it aside from timestamps, and makes it even easier to set new fields later.
Yeah, it seemed a little bit overkill at first. An I didn't want to bother including wdm.h here, but why not. As I was doing page de-duplication, it felt nicer to know which information was actually used.
For that matter I don't think you need to include wdm.h here; you could just specify it as "bytes" instead.
diff --git a/server/trace.c b/server/trace.c index 2b58ed9fd2c0..31560eaad827 100644 --- a/server/trace.c +++ b/server/trace.c @@ -860,6 +860,28 @@ static void dump_varargs_startup_info( const char *prefix, data_size_t size ) remove_data( size ); } +static void dump_varargs_usd_init( const char *prefix, data_size_t size ) +{ + usd_init_t init; + int i; + + memset( &init, 0, sizeof(init) ); + memcpy( &init, cur_data, min( size, sizeof(init) )); + + fprintf( stderr, + "%s{number_of_physical_pages=%u,nt_major_version=%04x,nt_minor_version=%04x,"
+ "nt_product_type=%02x,processor_features={", + prefix, init.number_of_physical_pages, init.nt_major_version, init.nt_minor_version, + init.nt_product_type ); + for (i = 0; i < ARRAY_SIZE(init.processor_features); ++i) + { + if (i > 0) fputc( ',', stderr ); + fprintf( stderr, "%02x", init.processor_features[i] ); + } + fprintf( stderr, "}}" ); + remove_data( size ); +} +
In which case you'd probably just get rid of this.
static void dump_varargs_input_records( const char *prefix, data_size_t size ) { const INPUT_RECORD *rec = cur_data;
If one page per process is preferable, then it can actually be passed to the client earlier, with possibly the version lookup done on the server side, and maybe the cpuinfo and page count as well if the values are identical for 32bit and 64bit processes?
So if it's possible, there would be no need to pass anything from the client to the server. If not, then it's just a matter of passing cpu features and page count, which I feel is better than passing the whole initial USD.
Maybe. I get the (perhaps faulty) impression from working on the server that a goal is to keep as much out of the server as possible, so it's desirable to keep all of that information (version, processor features, etc.) computed by the client. In that case we'd certainly want to pass information from client to server, and passing the whole page makes things simpler. (It's not clear to me why just passing more data would be a problem, either. I'm not sure there's necessarily overhead to begin with, but even if there is, it's only once, at process initialization.)
Rémi Bernon <rbernon(a)codeweavers.com> writes:
If one page per process is preferable, then it can actually be passed to the client earlier, with possibly the version lookup done on the server side, and maybe the cpuinfo and page count as well if the values are identical for 32bit and 64bit processes?
I think you want one single page for the entire system. I have seen no evidence that the version number not matching the per-process reported version is a major issue in practice. Besides, the versions already don't match in many other places like the registry and the version resources. Also, initializing the page contents could probably be handled by wineboot. -- Alexandre Julliard julliard(a)winehq.org
On 4/28/20 10:11 AM, Rémi Bernon wrote:
Original patch from: Andrew Wesie <awesie(a)gmail.com>
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> ---
v3: Server-side implementation of USD timestamp updates.
The system time check still has some large diffs compared to native after patch #2, probably because of some different clock granularity between server and client.
I didn't include an InterruptTime test because depending on the Windows versions, RtlQueryUnbiasedInterruptTime sometimes doesn't match the USD clock.
I'm sending it as RFC as it's still a little bit rough on the edges, and I only tested on Linux so far.
dlls/ntdll/tests/time.c | 52 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+)
diff --git a/dlls/ntdll/tests/time.c b/dlls/ntdll/tests/time.c index 5382a952d8d8..cc6e9f226e63 100644 --- a/dlls/ntdll/tests/time.c +++ b/dlls/ntdll/tests/time.c @@ -18,7 +18,9 @@ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA */
+#define NONAMELESSUNION #include "ntdll_test.h" +#include "ddk/wdm.h"
#define TICKSPERSEC 10000000 #define TICKSPERMSEC 10000 @@ -29,6 +31,7 @@ static VOID (WINAPI *pRtlTimeFieldsToTime)( PTIME_FIELDS TimeFields, PLARGE_IN static NTSTATUS (WINAPI *pNtQueryPerformanceCounter)( LARGE_INTEGER *counter, LARGE_INTEGER *frequency ); static NTSTATUS (WINAPI *pRtlQueryTimeZoneInformation)( RTL_TIME_ZONE_INFORMATION *); static NTSTATUS (WINAPI *pRtlQueryDynamicTimeZoneInformation)( RTL_DYNAMIC_TIME_ZONE_INFORMATION *); +static ULONG (WINAPI *pNtGetTickCount)(void);
static const int MonthLengths[2][12] = { @@ -153,12 +156,60 @@ static void test_RtlQueryTimeZoneInformation(void) wine_dbgstr_w(tzinfo.DaylightName)); }
+static ULONG64 read_ksystem_time(volatile KSYSTEM_TIME *time) +{ + ULONG64 high, low; + + do + { + high = time->High1Time; + low = time->LowPart; + } + while (high != time->High2Time); + + return high << 32 | low; +} + +static void test_NtGetTickCount(void) +{ + KSHARED_USER_DATA *user_shared_data = (void *)0x7ffe0000; + ULONG64 usd_time; + ULONG diff; + int i; + + for (i = 0; i < 5; ++i) + { + LARGE_INTEGER system_time; + + if (user_shared_data->NtMajorVersion <= 5 && user_shared_data->NtMinorVersion <= 1) + usd_time = ((ULONG64)user_shared_data->TickCountLowDeprecated * user_shared_data->TickCountMultiplier) >> 24; + else + usd_time = (read_ksystem_time(&user_shared_data->u.TickCount) * user_shared_data->TickCountMultiplier) >> 24; + + if (!pNtGetTickCount) + diff = GetTickCount() - usd_time; + else + diff = pNtGetTickCount() - usd_time;
We already have tests in kernel32 that GetTickCount() and NtGetTickCount() are close, so it seems probably easiest to just test GetTickCount() here.
+ todo_wine + ok(diff < 100, "NtGetTickCount - USD->TickCount too high, expected < 100 got %d\n", diff); + + usd_time = read_ksystem_time(&user_shared_data->SystemTime); + NtQuerySystemTime(&system_time); + diff = system_time.QuadPart - usd_time; + todo_wine + ok(diff < 100, "NtQuerySystemTime - USD->SystemTime too high, expected < 100 got %d\n", diff); +
The way it's done in kernel32() is to read from sources A, B, A, then check that A < B < A. Assuming that source A is sane, that lets you avoid any failures due to spurious lag (as is unfortunately often the case with the testbot.)
+ Sleep(50); + } +} + START_TEST(time) { HMODULE mod = GetModuleHandleA("ntdll.dll"); pRtlTimeToTimeFields = (void *)GetProcAddress(mod,"RtlTimeToTimeFields"); pRtlTimeFieldsToTime = (void *)GetProcAddress(mod,"RtlTimeFieldsToTime"); pNtQueryPerformanceCounter = (void *)GetProcAddress(mod, "NtQueryPerformanceCounter"); + pNtGetTickCount = (void *)GetProcAddress(mod,"NtGetTickCount"); pRtlQueryTimeZoneInformation = (void *)GetProcAddress(mod, "RtlQueryTimeZoneInformation"); pRtlQueryDynamicTimeZoneInformation = @@ -169,5 +220,6 @@ START_TEST(time) else win_skip("Required time conversion functions are not available\n"); test_NtQueryPerformanceCounter(); + test_NtGetTickCount(); test_RtlQueryTimeZoneInformation(); }
participants (3)
-
Alexandre Julliard -
Rémi Bernon -
Zebediah Figura