[PATCH v5 0/5] MR3103: server: Create and use a desktop shared mapping for GetCursorPos.
First part of Proton shared memory series. The full branch can be seen at https://gitlab.winehq.org/rbernon/wine/-/commits/mr/shared-memories. -- v5: user32: Use the desktop shared data for GetCursorPos(). server: Add a sequence number to the shared data. server: Move the cursor position and last change time to the shared data. server: Use the helper to update the cursor last change time. server: Create a desktop shared mapping. https://gitlab.winehq.org/wine/wine/-/merge_requests/3103
From: Rémi Bernon <rbernon(a)codeweavers.com> Based on a patch by Huw Davies <huw(a)codeweavers.com>. --- server/directory.c | 17 +++++++++++++++++ server/file.h | 6 ++++++ server/mapping.c | 29 +++++++++++++++++++++++++++++ server/protocol.def | 6 ++++++ server/user.h | 2 ++ server/winstation.c | 24 ++++++++++++++++++++++++ 6 files changed, 84 insertions(+) diff --git a/server/directory.c b/server/directory.c index 23d7eb0a2b7..5c8671d6146 100644 --- a/server/directory.c +++ b/server/directory.c @@ -37,6 +37,7 @@ #include "process.h" #include "file.h" #include "unicode.h" +#include "user.h" #define HASH_SIZE 7 /* default hash size */ @@ -277,6 +278,22 @@ struct object *get_directory_obj( struct process *process, obj_handle_t handle ) return get_handle_obj( process, handle, 0, &directory_ops ); } +struct object *create_desktop_map_directory( struct winstation *winstation ) +{ + static const WCHAR dir_desktop_mapsW[] = {'_','_','w','i','n','e','_','d','e','s','k','t','o','p','_','m','a','p','p','i','n','g','s'}; + static const struct unicode_str dir_desktop_maps_str = {dir_desktop_mapsW, sizeof(dir_desktop_mapsW)}; + struct object *root; + struct directory *mapping_root, *ret; + const struct unicode_str winsta_name = {winstation->obj.name->name, winstation->obj.name->len}; + + root = winstation->obj.name->parent; + mapping_root = create_directory( root, &dir_desktop_maps_str, OBJ_OPENIF, HASH_SIZE, NULL ); + ret = create_directory( &mapping_root->obj, &winsta_name, OBJ_OPENIF, HASH_SIZE, NULL ); + release_object( &mapping_root->obj ); + + return &ret->obj; +} + /* Global initialization */ static void create_session( unsigned int id ) diff --git a/server/file.h b/server/file.h index a43b4afad08..85aba366847 100644 --- a/server/file.h +++ b/server/file.h @@ -156,6 +156,10 @@ extern struct timeout_user *add_timeout_user( timeout_t when, timeout_callback f extern void remove_timeout_user( struct timeout_user *user ); extern const char *get_timeout_str( timeout_t timeout ); +/* directory functions */ + +extern struct object *create_desktop_map_directory( struct winstation *winstation ); + /* file functions */ extern struct file *get_file_obj( struct process *process, obj_handle_t handle, @@ -182,6 +186,8 @@ extern void free_mapped_views( struct process *process ); extern int get_page_size(void); extern struct mapping *create_fd_mapping( struct object *root, const struct unicode_str *name, struct fd *fd, unsigned int attr, const struct security_descriptor *sd ); +extern struct object *create_shared_mapping( struct object *root, const struct unicode_str *name, mem_size_t size, + unsigned int attr, const struct security_descriptor *sd, void **ptr ); extern struct object *create_user_data_mapping( struct object *root, const struct unicode_str *name, unsigned int attr, const struct security_descriptor *sd ); diff --git a/server/mapping.c b/server/mapping.c index 5d4df2050e4..e01b9b781d7 100644 --- a/server/mapping.c +++ b/server/mapping.c @@ -29,6 +29,7 @@ #include <sys/stat.h> #include <sys/mman.h> #include <unistd.h> +#include <errno.h> #include "ntstatus.h" #define WIN32_NO_STATUS @@ -161,6 +162,7 @@ struct mapping pe_image_info_t image; /* image info (for PE image mapping) */ struct ranges *committed; /* list of committed ranges in this mapping */ struct shared_map *shared; /* temp file for shared PE mapping */ + void *shared_ptr; /* mmaped pointer for shared mappings */ }; static void mapping_dump( struct object *obj, int verbose ); @@ -896,6 +898,7 @@ static struct mapping *create_mapping( struct object *root, const struct unicode mapping->fd = NULL; mapping->shared = NULL; mapping->committed = NULL; + mapping->shared_ptr = MAP_FAILED; if (!(mapping->flags = get_mapping_flags( handle, flags ))) goto error; @@ -1096,6 +1099,7 @@ static void mapping_destroy( struct object *obj ) if (mapping->fd) release_object( mapping->fd ); if (mapping->committed) release_object( mapping->committed ); if (mapping->shared) release_object( mapping->shared ); + if (mapping->shared_ptr != MAP_FAILED) munmap( mapping->shared_ptr, mapping->size ); } static enum server_fd_type mapping_get_fd_type( struct fd *fd ) @@ -1109,6 +1113,31 @@ int get_page_size(void) return page_mask + 1; } +struct object *create_shared_mapping( struct object *root, const struct unicode_str *name, mem_size_t size, + unsigned int attr, const struct security_descriptor *sd, void **ptr ) +{ + static unsigned int access = FILE_READ_DATA | FILE_WRITE_DATA; + struct mapping *mapping; + + if (!(mapping = create_mapping( root, name, attr, size, SEC_COMMIT, 0, access, sd ))) return NULL; + + if (mapping->shared_ptr == MAP_FAILED) + { + int fd = get_unix_fd( mapping->fd ); + + mapping->shared_ptr = mmap( NULL, mapping->size, PROT_WRITE, MAP_SHARED, fd, 0 ); + if (mapping->shared_ptr == MAP_FAILED) + { + fprintf( stderr, "wine: Failed to map shared memory: %u %m\n", errno ); + release_object( &mapping->obj ); + return NULL; + } + } + + *ptr = mapping->shared_ptr; + return &mapping->obj; +} + struct object *create_user_data_mapping( struct object *root, const struct unicode_str *name, unsigned int attr, const struct security_descriptor *sd ) { diff --git a/server/protocol.def b/server/protocol.def index 919297c818c..d6cfd8e9f72 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -890,6 +890,12 @@ typedef struct lparam_t info; } cursor_pos_t; +struct desktop_shared_memory +{ + int placeholder; +}; +typedef volatile struct desktop_shared_memory desktop_shm_t; + /****************************************************************/ /* Request declarations */ diff --git a/server/user.h b/server/user.h index b6f47bcac1c..20b7acb6fdc 100644 --- a/server/user.h +++ b/server/user.h @@ -77,6 +77,8 @@ struct desktop unsigned int users; /* processes and threads using this desktop */ struct global_cursor cursor; /* global cursor information */ unsigned char keystate[256]; /* asynchronous key state */ + struct object *shared_mapping; /* desktop shared memory mapping */ + const desktop_shm_t *shared; /* desktop shared memory */ }; /* user handles functions */ diff --git a/server/winstation.c b/server/winstation.c index 5903497d61e..524ee753f90 100644 --- a/server/winstation.c +++ b/server/winstation.c @@ -217,6 +217,22 @@ struct desktop *get_desktop_obj( struct process *process, obj_handle_t handle, u return (struct desktop *)get_handle_obj( process, handle, access, &desktop_ops ); } +static int init_desktop_mapping( struct desktop *desktop, const struct unicode_str *name ) +{ + struct object *dir; + + desktop->shared = NULL; + desktop->shared_mapping = NULL; + + if (!(dir = create_desktop_map_directory( desktop->winstation ))) return 0; + if ((desktop->shared_mapping = create_shared_mapping( dir, name, sizeof(struct desktop_shared_memory), + 0, NULL, (void **)&desktop->shared ))) + memset( (void *)desktop->shared, 0, sizeof(*desktop->shared) ); + release_object( dir ); + + return !!desktop->shared; +} + /* create a desktop object */ static struct desktop *create_desktop( const struct unicode_str *name, unsigned int attr, unsigned int flags, struct winstation *winstation ) @@ -240,6 +256,11 @@ static struct desktop *create_desktop( const struct unicode_str *name, unsigned memset( desktop->keystate, 0, sizeof(desktop->keystate) ); list_add_tail( &winstation->desktops, &desktop->entry ); list_init( &desktop->hotkeys ); + if (!init_desktop_mapping( desktop, name )) + { + release_object( desktop ); + return NULL; + } } else { @@ -297,6 +318,8 @@ static void desktop_destroy( struct object *obj ) if (desktop->global_hooks) release_object( desktop->global_hooks ); if (desktop->close_timeout) remove_timeout_user( desktop->close_timeout ); list_remove( &desktop->entry ); + if (desktop->shared_mapping) release_object( desktop->shared_mapping ); + desktop->shared_mapping = NULL; release_object( desktop->winstation ); } @@ -312,6 +335,7 @@ static void close_desktop_timeout( void *private ) desktop->close_timeout = NULL; unlink_named_object( &desktop->obj ); /* make sure no other process can open it */ + unlink_named_object( desktop->shared_mapping ); post_desktop_message( desktop, WM_CLOSE, 0, 0 ); /* and signal the owner to quit */ } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3103
From: Rémi Bernon <rbernon(a)codeweavers.com> Based on a patch by Huw Davies <huw(a)codeweavers.com>. --- server/queue.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/server/queue.c b/server/queue.c index fcc946ff0cb..51e01a471bf 100644 --- a/server/queue.c +++ b/server/queue.c @@ -1873,7 +1873,9 @@ static int queue_mouse_message( struct desktop *desktop, user_handle_t win, cons WM_MOUSEHWHEEL /* 0x1000 = MOUSEEVENTF_HWHEEL */ }; - desktop->cursor.last_change = get_tick_count(); + /* update last desktop cursor change time */ + update_desktop_cursor_pos( desktop, desktop->cursor.win, desktop->cursor.x, desktop->cursor.y ); + flags = input->mouse.flags; time = input->mouse.time; if (!time) time = desktop->cursor.last_change; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3103
From: Rémi Bernon <rbernon(a)codeweavers.com> Based on a patch by Huw Davies <huw(a)codeweavers.com>. --- server/protocol.def | 9 ++++- server/queue.c | 81 +++++++++++++++++++++++++++------------------ server/user.h | 3 -- 3 files changed, 56 insertions(+), 37 deletions(-) diff --git a/server/protocol.def b/server/protocol.def index d6cfd8e9f72..f8216d57e25 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -890,9 +890,16 @@ typedef struct lparam_t info; } cursor_pos_t; +struct shared_cursor +{ + int x; /* cursor position */ + int y; + unsigned int last_change; /* time of last position change */ +}; + struct desktop_shared_memory { - int placeholder; + struct shared_cursor cursor; /* global cursor information */ }; typedef volatile struct desktop_shared_memory desktop_shm_t; diff --git a/server/queue.c b/server/queue.c index 51e01a471bf..960683c7417 100644 --- a/server/queue.c +++ b/server/queue.c @@ -232,6 +232,15 @@ static unsigned int last_input_time; static cursor_pos_t cursor_history[64]; static unsigned int cursor_history_latest; +#define SHARED_WRITE_BEGIN( object, type ) \ + do { \ + type *shared = (type *)(object)->shared; \ + do + +#define SHARED_WRITE_END \ + while(0); \ + } while(0); + static void queue_hardware_message( struct desktop *desktop, struct message *msg, int always_queue ); static void free_message( struct message *msg ); @@ -423,8 +432,8 @@ static void queue_cursor_message( struct desktop *desktop, user_handle_t win, un msg->msg = message; msg->wparam = wparam; msg->lparam = lparam; - msg->x = desktop->cursor.x; - msg->y = desktop->cursor.y; + msg->x = desktop->shared->cursor.x; + msg->y = desktop->shared->cursor.y; if (!(msg->win = win) && (input = desktop->foreground_input)) msg->win = input->active; queue_hardware_message( desktop, msg, 1 ); } @@ -447,13 +456,19 @@ static int update_desktop_cursor_pos( struct desktop *desktop, user_handle_t win { struct thread_input *input; int updated; + unsigned int time = get_tick_count(); x = max( min( x, desktop->cursor.clip.right - 1 ), desktop->cursor.clip.left ); y = max( min( y, desktop->cursor.clip.bottom - 1 ), desktop->cursor.clip.top ); - updated = (desktop->cursor.x != x || desktop->cursor.y != y); - desktop->cursor.x = x; - desktop->cursor.y = y; - desktop->cursor.last_change = get_tick_count(); + updated = (desktop->shared->cursor.x != x || desktop->shared->cursor.y != y); + + SHARED_WRITE_BEGIN( desktop, desktop_shm_t ) + { + shared->cursor.x = x; + shared->cursor.y = y; + shared->cursor.last_change = time; + } + SHARED_WRITE_END if (!win && (input = desktop->foreground_input)) win = input->capture; if (!win || !is_window_visible( win ) || is_window_transparent( win )) @@ -502,8 +517,8 @@ static void get_message_defaults( struct msg_queue *queue, int *x, int *y, unsig { struct desktop *desktop = queue->input->desktop; - *x = desktop->cursor.x; - *y = desktop->cursor.y; + *x = desktop->shared->cursor.x; + *y = desktop->shared->cursor.y; *time = get_tick_count(); } @@ -527,9 +542,9 @@ void set_clip_rectangle( struct desktop *desktop, const rectangle_t *rect, unsig else desktop->cursor.clip = top_rect; /* warp the mouse to be inside the clip rect */ - x = max( min( desktop->cursor.x, desktop->cursor.clip.right - 1 ), desktop->cursor.clip.left ); - y = max( min( desktop->cursor.y, desktop->cursor.clip.bottom - 1 ), desktop->cursor.clip.top ); - if (x != desktop->cursor.x || y != desktop->cursor.y) set_cursor_pos( desktop, x, y ); + x = max( min( desktop->shared->cursor.x, desktop->cursor.clip.right - 1 ), desktop->cursor.clip.left ); + y = max( min( desktop->shared->cursor.y, desktop->cursor.clip.bottom - 1 ), desktop->cursor.clip.top ); + if (x != desktop->shared->cursor.x || y != desktop->shared->cursor.y) set_cursor_pos( desktop, x, y ); /* request clip cursor rectangle reset to the desktop thread */ if (reset) post_desktop_message( desktop, WM_WINE_CLIPCURSOR, flags, FALSE ); @@ -1682,8 +1697,8 @@ static void queue_hardware_message( struct desktop *desktop, struct message *msg if (desktop->keystate[VK_XBUTTON2] & 0x80) msg->wparam |= MK_XBUTTON2; break; } - msg->x = desktop->cursor.x; - msg->y = desktop->cursor.y; + msg->x = desktop->shared->cursor.x; + msg->y = desktop->shared->cursor.y; if (msg->win && (thread = get_window_thread( msg->win ))) { @@ -1874,11 +1889,11 @@ static int queue_mouse_message( struct desktop *desktop, user_handle_t win, cons }; /* update last desktop cursor change time */ - update_desktop_cursor_pos( desktop, desktop->cursor.win, desktop->cursor.x, desktop->cursor.y ); + update_desktop_cursor_pos( desktop, desktop->cursor.win, desktop->shared->cursor.x, desktop->shared->cursor.y ); flags = input->mouse.flags; time = input->mouse.time; - if (!time) time = desktop->cursor.last_change; + if (!time) time = desktop->shared->cursor.last_change; if (flags & MOUSEEVENTF_MOVE) { @@ -1887,19 +1902,19 @@ static int queue_mouse_message( struct desktop *desktop, user_handle_t win, cons x = input->mouse.x; y = input->mouse.y; if (flags & ~(MOUSEEVENTF_MOVE | MOUSEEVENTF_ABSOLUTE) && - x == desktop->cursor.x && y == desktop->cursor.y) + x == desktop->shared->cursor.x && y == desktop->shared->cursor.y) flags &= ~MOUSEEVENTF_MOVE; } else { - x = desktop->cursor.x + input->mouse.x; - y = desktop->cursor.y + input->mouse.y; + x = desktop->shared->cursor.x + input->mouse.x; + y = desktop->shared->cursor.y + input->mouse.y; } } else { - x = desktop->cursor.x; - y = desktop->cursor.y; + x = desktop->shared->cursor.x; + y = desktop->shared->cursor.y; } if ((foreground = get_foreground_thread( desktop, win ))) @@ -1916,8 +1931,8 @@ static int queue_mouse_message( struct desktop *desktop, user_handle_t win, cons msg_data->size = sizeof(*msg_data); msg_data->flags = flags; msg_data->rawinput.type = RIM_TYPEMOUSE; - msg_data->rawinput.mouse.x = x - desktop->cursor.x; - msg_data->rawinput.mouse.y = y - desktop->cursor.y; + msg_data->rawinput.mouse.x = x - desktop->shared->cursor.x; + msg_data->rawinput.mouse.y = y - desktop->shared->cursor.y; msg_data->rawinput.mouse.data = input->mouse.data; enum_processes( queue_rawinput_message, &raw_msg ); @@ -2144,8 +2159,8 @@ static void queue_custom_hardware_message( struct desktop *desktop, user_handle_ msg->msg = input->hw.msg; msg->wparam = 0; msg->lparam = input->hw.lparam; - msg->x = desktop->cursor.x; - msg->y = desktop->cursor.y; + msg->x = desktop->shared->cursor.x; + msg->y = desktop->shared->cursor.y; queue_hardware_message( desktop, msg, 1 ); } @@ -2660,8 +2675,8 @@ DECL_HANDLER(send_hardware_message) } } - reply->prev_x = desktop->cursor.x; - reply->prev_y = desktop->cursor.y; + reply->prev_x = desktop->shared->cursor.x; + reply->prev_y = desktop->shared->cursor.y; switch (req->input.type) { @@ -2679,8 +2694,8 @@ DECL_HANDLER(send_hardware_message) } if (thread) release_object( thread ); - reply->new_x = desktop->cursor.x; - reply->new_y = desktop->cursor.y; + reply->new_x = desktop->shared->cursor.x; + reply->new_y = desktop->shared->cursor.y; release_object( desktop ); } @@ -3367,8 +3382,8 @@ DECL_HANDLER(set_cursor) reply->prev_handle = input->cursor; reply->prev_count = input->cursor_count; - reply->prev_x = desktop->cursor.x; - reply->prev_y = desktop->cursor.y; + reply->prev_x = desktop->shared->cursor.x; + reply->prev_y = desktop->shared->cursor.y; if (req->flags & SET_CURSOR_HANDLE) { @@ -3394,10 +3409,10 @@ DECL_HANDLER(set_cursor) else update_desktop_cursor_handle( desktop, input->cursor ); } - reply->new_x = desktop->cursor.x; - reply->new_y = desktop->cursor.y; + reply->new_x = desktop->shared->cursor.x; + reply->new_y = desktop->shared->cursor.y; reply->new_clip = desktop->cursor.clip; - reply->last_change = desktop->cursor.last_change; + reply->last_change = desktop->shared->cursor.last_change; } /* Get the history of the 64 last cursor positions */ diff --git a/server/user.h b/server/user.h index 20b7acb6fdc..9c3e2b1381c 100644 --- a/server/user.h +++ b/server/user.h @@ -54,10 +54,7 @@ struct winstation struct global_cursor { - int x; /* cursor position */ - int y; rectangle_t clip; /* cursor clip rectangle */ - unsigned int last_change; /* time of last position change */ user_handle_t win; /* window that contains the cursor */ user_handle_t handle; /* last set cursor handle */ }; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3103
From: Rémi Bernon <rbernon(a)codeweavers.com> The client should check that the lower SEQUENCE_MASK_BITS are zero before reading the data and confirm that the number is unchanged when it's finished. Based on a patch by Huw Davies <huw(a)codeweavers.com>. --- server/protocol.def | 1 + server/queue.c | 19 +++++++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/server/protocol.def b/server/protocol.def index f8216d57e25..e7c267b4148 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -899,6 +899,7 @@ struct shared_cursor struct desktop_shared_memory { + unsigned int seq; /* sequence number - server updating if (seq & 1) != 0 */ struct shared_cursor cursor; /* global cursor information */ }; typedef volatile struct desktop_shared_memory desktop_shm_t; diff --git a/server/queue.c b/server/queue.c index 960683c7417..f033d1a754e 100644 --- a/server/queue.c +++ b/server/queue.c @@ -232,15 +232,34 @@ static unsigned int last_input_time; static cursor_pos_t cursor_history[64]; static unsigned int cursor_history_latest; +#if defined(__i386__) || defined(__x86_64__) + #define SHARED_WRITE_BEGIN( object, type ) \ do { \ type *shared = (type *)(object)->shared; \ + shared->seq++; \ do #define SHARED_WRITE_END \ while(0); \ + shared->seq++; \ } while(0); +#else + +#define SHARED_WRITE_BEGIN( object, type ) \ + do { \ + type *shared = (type *)(object)->shared; \ + __atomic_add_fetch( &shared->seq, 1, __ATOMIC_RELEASE ); \ + } while(0) + +#define SHARED_WRITE_END \ + while(0); \ + __atomic_add_fetch( &shared->seq, 1, __ATOMIC_RELEASE ); \ + } while(0) + +#endif + static void queue_hardware_message( struct desktop *desktop, struct message *msg, int always_queue ); static void free_message( struct message *msg ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3103
From: Rémi Bernon <rbernon(a)codeweavers.com> Based on a patch by Huw Davies <huw(a)codeweavers.com>. --- dlls/win32u/input.c | 20 +++++------ dlls/win32u/ntuser_private.h | 24 +++++++++++++ dlls/win32u/sysparams.c | 6 ++++ dlls/win32u/winstation.c | 65 ++++++++++++++++++++++++++++++++++++ 4 files changed, 104 insertions(+), 11 deletions(-) diff --git a/dlls/win32u/input.c b/dlls/win32u/input.c index 1f9e48423fc..96244e9ed64 100644 --- a/dlls/win32u/input.c +++ b/dlls/win32u/input.c @@ -740,25 +740,23 @@ BOOL WINAPI NtUserSetCursorPos( INT x, INT y ) */ BOOL get_cursor_pos( POINT *pt ) { - BOOL ret; + const desktop_shm_t *shared = get_desktop_shared_memory(); DWORD last_change; + BOOL ret = TRUE; UINT dpi; - if (!pt) return FALSE; + if (!pt || !shared) return FALSE; - SERVER_START_REQ( set_cursor ) + SHARED_READ_BEGIN( &shared->seq ) { - if ((ret = !wine_server_call( req ))) - { - pt->x = reply->new_x; - pt->y = reply->new_y; - last_change = reply->last_change; - } + pt->x = shared->cursor.x; + pt->y = shared->cursor.y; + last_change = shared->cursor.last_change; } - SERVER_END_REQ; + SHARED_READ_END( &shared->seq ); /* query new position from graphics driver if we haven't updated recently */ - if (ret && NtGetTickCount() - last_change > 100) ret = user_driver->pGetCursorPos( pt ); + if (NtGetTickCount() - last_change > 100) ret = user_driver->pGetCursorPos( pt ); if (ret && (dpi = get_thread_dpi())) { HMONITOR monitor = monitor_from_point( *pt, MONITOR_DEFAULTTOPRIMARY, 0 ); diff --git a/dlls/win32u/ntuser_private.h b/dlls/win32u/ntuser_private.h index b39e38db5d6..84a5c49420c 100644 --- a/dlls/win32u/ntuser_private.h +++ b/dlls/win32u/ntuser_private.h @@ -132,6 +132,7 @@ struct user_thread_info UINT spy_indent; /* Current spy indent */ BOOL clipping_cursor; /* thread is currently clipping */ DWORD clipping_reset; /* time when clipping was last reset */ + const desktop_shm_t *desktop_shm; /* Ptr to server's desktop shared memory */ }; C_ASSERT( sizeof(struct user_thread_info) <= sizeof(((TEB *)0)->Win32ClientInfo) ); @@ -250,6 +251,9 @@ void release_user_handle_ptr( void *ptr ) DECLSPEC_HIDDEN; void *next_process_user_handle_ptr( HANDLE *handle, unsigned int type ) DECLSPEC_HIDDEN; UINT win_set_flags( HWND hwnd, UINT set_mask, UINT clear_mask ) DECLSPEC_HIDDEN; +/* winstation.c */ +extern const desktop_shm_t *get_desktop_shared_memory(void) DECLSPEC_HIDDEN; + static inline UINT win_get_flags( HWND hwnd ) { return win_set_flags( hwnd, 0, 0 ); @@ -259,4 +263,24 @@ WND *get_win_ptr( HWND hwnd ) DECLSPEC_HIDDEN; BOOL is_child( HWND parent, HWND child ) DECLSPEC_HIDDEN; BOOL is_window( HWND hwnd ) DECLSPEC_HIDDEN; +#if defined(__i386__) || defined(__x86_64__) +#define __SHARED_READ_SEQ( x ) (*(x)) +#define __SHARED_READ_FENCE do {} while(0) +#else +#define __SHARED_READ_SEQ( x ) __atomic_load_n( x, __ATOMIC_RELAXED ) +#define __SHARED_READ_FENCE __atomic_thread_fence( __ATOMIC_ACQUIRE ) +#endif + +#define SHARED_READ_BEGIN( x ) \ + do { \ + unsigned int __seq; \ + do { \ + while ((__seq = __SHARED_READ_SEQ( x )) & 1) NtYieldExecution(); \ + __SHARED_READ_FENCE; + +#define SHARED_READ_END( x ) \ + __SHARED_READ_FENCE; \ + } while (__SHARED_READ_SEQ( x ) != __seq); \ + } while(0) + #endif /* __WINE_NTUSER_PRIVATE_H */ diff --git a/dlls/win32u/sysparams.c b/dlls/win32u/sysparams.c index fed309ff3d8..0319be584f8 100644 --- a/dlls/win32u/sysparams.c +++ b/dlls/win32u/sysparams.c @@ -6185,6 +6185,12 @@ static void thread_detach(void) cleanup_imm_thread(); NtClose( thread_info->server_queue ); + if (thread_info->desktop_shm) + { + NtUnmapViewOfSection( GetCurrentProcess(), (void *)thread_info->desktop_shm ); + thread_info->desktop_shm = NULL; + } + exiting_thread_id = 0; } diff --git a/dlls/win32u/winstation.c b/dlls/win32u/winstation.c index 5d1d5254ae1..e7aa8ce0d68 100644 --- a/dlls/win32u/winstation.c +++ b/dlls/win32u/winstation.c @@ -265,6 +265,11 @@ BOOL WINAPI NtUserSetThreadDesktop( HDESK handle ) thread_info->client_info.msg_window = 0; if (key_state_info) key_state_info->time = 0; if (was_virtual_desktop != is_virtual_desktop()) update_display_cache( TRUE ); + if (thread_info->desktop_shm) + { + NtUnmapViewOfSection( GetCurrentProcess(), (void *)thread_info->desktop_shm ); + thread_info->desktop_shm = NULL; + } } return ret; } @@ -606,6 +611,66 @@ static const WCHAR *get_default_desktop( void *buf, size_t buf_size ) return defaultW; } +static volatile void *map_shared_memory_section( const WCHAR *name, SIZE_T size, HANDLE root ) +{ + OBJECT_ATTRIBUTES attr; + UNICODE_STRING section_str; + HANDLE handle; + UINT status; + void *ptr; + + RtlInitUnicodeString( §ion_str, name ); + InitializeObjectAttributes( &attr, §ion_str, 0, root, NULL ); + if (!(status = NtOpenSection( &handle, SECTION_ALL_ACCESS, &attr ))) + { + ptr = NULL; + status = NtMapViewOfSection( handle, GetCurrentProcess(), &ptr, 0, 0, NULL, + &size, ViewUnmap, 0, PAGE_READONLY ); + NtClose( handle ); + } + + if (status) + { + WARN( "Failed to map view of section %s, status %#x\n", debugstr_w(name), status ); + return NULL; + } + + return ptr; +} + +const desktop_shm_t *get_desktop_shared_memory(void) +{ + static const WCHAR dir_desktop_maps[] = + { + '_','_','w','i','n','e','_','d','e','s','k','t','o','p','_','m','a','p','p','i','n','g','s','\\',0 + }; + struct user_thread_info *thread_info = get_user_thread_info(); + HANDLE root, handles[2]; + WCHAR buf[MAX_PATH], *ptr; + DWORD i, needed; + + if (thread_info->desktop_shm) return thread_info->desktop_shm; + + handles[0] = NtUserGetProcessWindowStation(); + handles[1] = NtUserGetThreadDesktop( GetCurrentThreadId() ); + + memcpy( buf, dir_desktop_maps, wcslen(dir_desktop_maps) * sizeof(WCHAR) ); + ptr = buf + wcslen(dir_desktop_maps); + + for (i = 0; i < 2; i++) + { + NtUserGetObjectInformation( handles[i], UOI_NAME, (void *)ptr, sizeof(buf) - (ptr - buf) * sizeof(WCHAR), &needed ); + ptr += needed / sizeof(WCHAR); + if (i == 0) *(ptr - 1) = '\\'; + } + + root = get_winstations_dir_handle(); + thread_info->desktop_shm = map_shared_memory_section( buf, sizeof(*thread_info->desktop_shm), root ); + NtClose( root ); + + return thread_info->desktop_shm; +} + /*********************************************************************** * winstation_init * -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3103
v4: Fix compilation. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_41310
Jinoh Kang (@iamahuman) commented about server/queue.c:
static cursor_pos_t cursor_history[64]; static unsigned int cursor_history_latest;
+#if defined(__i386__) || defined(__x86_64__) + +#define SHARED_WRITE_BEGIN( object, type ) \ + do { \ + type *shared = (type *)(object)->shared; \ + shared->seq++; \ (Maybe we could assert that the LSB of `seq` is 0 here, to catch erroneous nested writes.)
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_41379
Jinoh Kang (@iamahuman) commented about server/queue.c:
+ type *shared = (type *)(object)->shared; \ + shared->seq++; \ + do + +#define SHARED_WRITE_END \ + while(0); \ + shared->seq++; \ + } while(0); + +#else + +#define SHARED_WRITE_BEGIN( object, type ) \ + do { \ + type *shared = (type *)(object)->shared; \ + __atomic_add_fetch( &shared->seq, 1, __ATOMIC_RELEASE ); \ + } while(0) You forgot to keep the scope open.
```suggestion:-0+0 do ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_41380
Jinoh Kang (@iamahuman) commented about server/queue.c:
static cursor_pos_t cursor_history[64]; static unsigned int cursor_history_latest;
+#if defined(__i386__) || defined(__x86_64__) + +#define SHARED_WRITE_BEGIN( object, type ) \ + do { \ + type *shared = (type *)(object)->shared; \ If we can't use `typeof`, it might be a good idea to assert that the type matches. Something like:
```c (void)sizeof((object) - (type *)(object)); ``` If `__GCC__` is defined: ```c C_ASSERT(__builtin_types_compatible_p(typeof(object), type *)); ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_41381
On Thu Aug 3 15:59:27 2023 +0000, Rémi Bernon wrote:
Sure, we should perhaps unlink the mapping object too. Although that's probably a good thing to do, I'm not sure to see how it makes any difference wrt this `shared_ptr` member, and why we should get rid of this member. Sorry, I've got a bit sidetracked. Let's return to the main discussion.
why we should get rid of this member.
The reason I brought this up is simple; it's not immediately clear to the reader that both of the following holds: * We're lending this out to other server components. * That lease is valid *only until* the mapping object is freed. This leads to a few problems: 1. If the server mapping object gets released too early, it'll take away the mapped VMA with it while it's still borrowed. Later, someone may dereference the dangling mapping pointer, which points to unexpected memory. 2. In general, the caller of `create_shared_mapping()` has no control over the lifetime of the returned mapping pointer, making it less flexible. 3. It's extra overhead for every `struct mapping`, even if most sections aren't created via `create_shared_mapping()`. This `shared_ptr` member would be more easily justifiable if - there were some kind of accessor like `void *mapping_get_shared_ptr( struct mapping * );`, and - the client component accesses the shared pointer only through it. However, it's only ever used to be *freed*. In this case, I think it's better to let the caller of `create_shared_mapping()` free it instead. It gives a more clear picture of ownership management. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_41383
On Fri Aug 4 07:39:05 2023 +0000, Rémi Bernon wrote:
I don't think the desktop object should work as a directory. If the desktop object was a directory, we could use it as a RootDirectory and avoid the name aliasing problem above.
```c thread_info->desktop_shm = map_shared_memory_section( wine_desktop_mapping_str, sizeof(*thread_info->desktop_shm), NtUserGetThreadDesktop( GetCurrentThreadId() ) ); ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_41384
On Fri Aug 4 14:41:49 2023 +0000, Jinoh Kang wrote: > Sorry, I've got a bit sidetracked. Let's return to the main discussion. > > why we should get rid of this member. > The reason I brought this up is simple; it's not immediately clear to > the reader that both of the following holds: > * We're lending this out to other server components. > * That lease is valid *only until* the mapping object is freed. > This leads to a few problems: > 1. If the server mapping object gets released too early, it'll take away > the mapped VMA with it while it's still borrowed. Later, someone may > dereference the dangling mapping pointer, which points to unexpected memory. > 2. In general, the caller of `create_shared_mapping()` has no control > over the lifetime of the returned mapping pointer, making it less flexible. > 3. It's extra overhead for every `struct mapping`, even if most sections > aren't created via `create_shared_mapping()`. > This `shared_ptr` member would be more easily justifiable if > - there were some kind of accessor like `void *mapping_get_shared_ptr( > struct mapping * );`, and > - the client component accesses the shared pointer only through it. > However, it's only ever used to be *freed*. In this case, I think it's > better to let the caller of `create_shared_mapping()` free it instead. > It gives a more clear picture of ownership management. The callers hold a reference to the mapping object, and use its mapped pointer. The pointer stays valid as long as they keep a reference on the mapping object. The ownership seems clear to me, and it's simpler than having to manage the pointer lifetime separately which would require every user to munmap the pointer when they're done. Having the lifetime of the mapping and the pointer separate also makes it unclear how they are related together, and you could keep using the mapped pointer with the mapping reference released, which imho would be incorrect. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_41387
On Fri Aug 4 14:19:38 2023 +0000, Jinoh Kang wrote:
If we can't use `typeof`, it might be a good idea to assert that the type matches. Something like: ```c (void)sizeof((object) - (type *)(object)); ``` If `__GCC__` is defined: ```c C_ASSERT(__builtin_types_compatible_p(typeof(object), type *)); ``` I guess that's probably why I initially had an intermediate `const type *` variable before the cast, I'll restore it.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_41388
and it's simpler than having to manage the pointer lifetime separately which would require every user to munmap the pointer when they're done.
Managing the pointer for every use as you've described sounds more explicit and straightforward to me than implicit lifetime relationship. IMHO explicitness wins when we're dealing with delicate and potentially complex machinery like memory management. Besides, multiple writable pointer aliases to a shared memory sounds like a bug waiting to happen. Multiple readable aliases can be replaced by using accessor methods of the container object (e.g., desktop object states).
you could keep using the mapped pointer with the mapping reference released, which imho would be incorrect.
On the other hand, the user might want to keep the shared memory even if the mapping object is no longer used. For example, an unliked desktop object or a zombie process is never supposed to be used by clients, but the kernel may still be interested in its state. At the end of the day, we're just holding a reference to some Unix temporary file, and its existence doesn't need to be tied to whatever abstraction that Wine brings over. But again, this might just be a matter of taste so I wouldn't object to this MR on the grounds of `shared_ptr` usage. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3103#note_41390
participants (2)
-
Jinoh Kang (＠iamahuman) -
Rémi Bernon