NtSetContextThread, unlike NtContinue, should not set nonvolatile registers.
Signed-off-by: Jacek Caban jacek@codeweavers.com --- dlls/ntdll/unix/server.c | 2 +- dlls/ntdll/unix/signal_arm.c | 18 +++++++++++++----- dlls/ntdll/unix/signal_arm64.c | 20 ++++++++++++++------ dlls/ntdll/unix/signal_i386.c | 14 +++++++++++++- dlls/ntdll/unix/signal_x86_64.c | 15 ++++++++++++++- dlls/ntdll/unix/unix_private.h | 1 + 6 files changed, 56 insertions(+), 14 deletions(-)
On 1/22/21 9:51 AM, Jacek Caban wrote:
NtSetContextThread, unlike NtContinue, should not set nonvolatile registers.
Signed-off-by: Jacek Caban jacek@codeweavers.com
dlls/ntdll/unix/server.c | 2 +- dlls/ntdll/unix/signal_arm.c | 18 +++++++++++++----- dlls/ntdll/unix/signal_arm64.c | 20 ++++++++++++++------ dlls/ntdll/unix/signal_i386.c | 14 +++++++++++++- dlls/ntdll/unix/signal_x86_64.c | 15 ++++++++++++++- dlls/ntdll/unix/unix_private.h | 1 + 6 files changed, 56 insertions(+), 14 deletions(-)
Just to clarify—our NtSetContextThread does set nonvolatile registers, and continues to do so after this series. Is that incorrect behaviour?
Also:
diff --git a/dlls/ntdll/unix/server.c b/dlls/ntdll/unix/server.c index db09d7759da..fe581cca7b2 100644 --- a/dlls/ntdll/unix/server.c +++ b/dlls/ntdll/unix/server.c @@ -726,7 +726,7 @@ NTSTATUS WINAPI NtContinue( CONTEXT *context, BOOLEAN alertable ) status = server_select( NULL, 0, SELECT_INTERRUPTIBLE | SELECT_ALERTABLE, 0, NULL, NULL, &apc ); if (status == STATUS_USER_APC) invoke_apc( context, &apc ); }
- return NtSetContextThread( GetCurrentThread(), context );
- signal_set_cpu_context( context );
}
Would it be simpler just to move NtContinue() to signal_*.c instead?
One further question regarding this patch series / patch 0017: I assume you're also planning to also change NtContinue() to use the syscall exit code instead of set_full_cpu_context()?
On 05.02.2021 03:43, Zebediah Figura (she/her) wrote:
On 1/22/21 9:51 AM, Jacek Caban wrote:
NtSetContextThread, unlike NtContinue, should not set nonvolatile registers.
Signed-off-by: Jacek Caban jacek@codeweavers.com
dlls/ntdll/unix/server.c | 2 +- dlls/ntdll/unix/signal_arm.c | 18 +++++++++++++----- dlls/ntdll/unix/signal_arm64.c | 20 ++++++++++++++------ dlls/ntdll/unix/signal_i386.c | 14 +++++++++++++- dlls/ntdll/unix/signal_x86_64.c | 15 ++++++++++++++- dlls/ntdll/unix/unix_private.h | 1 + 6 files changed, 56 insertions(+), 14 deletions(-)
Just to clarify—our NtSetContextThread does set nonvolatile registers, and continues to do so after this series. Is that incorrect behaviour?
No, it does not continue to do so after this series. This is covered by the test at the end of the series. NtSetContextThread does set nonvolatile registers in syscall frame so that if you set them and then query them before syscall returns, you will get modified values. However, syscall dispatcher does not restore them from syscall frame. It means that if you call NtSetContectThread on current thread, it will not set nonvolatile registers to passed values.
Also:
diff --git a/dlls/ntdll/unix/server.c b/dlls/ntdll/unix/server.c index db09d7759da..fe581cca7b2 100644 --- a/dlls/ntdll/unix/server.c +++ b/dlls/ntdll/unix/server.c @@ -726,7 +726,7 @@ NTSTATUS WINAPI NtContinue( CONTEXT *context, BOOLEAN alertable ) status = server_select( NULL, 0, SELECT_INTERRUPTIBLE | SELECT_ALERTABLE, 0, NULL, NULL, &apc ); if (status == STATUS_USER_APC) invoke_apc( context, &apc ); } - return NtSetContextThread( GetCurrentThread(), context ); + signal_set_cpu_context( context ); }
Would it be simpler just to move NtContinue() to signal_*.c instead?
We'd then need to expose invoke_apc somehow, so I'm not sure if that would be better.
One further question regarding this patch series / patch 0017: I assume you're also planning to also change NtContinue() to use the syscall exit code instead of set_full_cpu_context()?
I considered it. My very first prototype worked like that because it always restored all registers, except for rax/eax which becomes a bit tricky for that use case. I was thinking about providing an alternative syscall leave implementation (NtContinue could replace its own return address to opt-in to this), which would do full restore. I didn't try implementing that variant yet to see if it's worth it.
For potential future plans, there are also call_user_apc_dispatcher and call_user_exception_dispatcher. They could potentially use the same mechanism as well instead of manually implementing crossing syscall barrier. They are more tricky to change, because we'd need to allow using stack is a more similar way to actual syscalls first. A simple solution would be to preserve enough unused stack on syscall entry, but a more complete solution like separated stacks could be even better.
BTW, as far as this series is considered, I think we will want a few specialized implementations for different CPU generations, so I plan to look at moving syscall dispatcher code from winebuild to ntdll first. ntdll.so would then set proper dispatcher depending on CPU capabilities. That requires changes to the way we access syscall table. I didn't do the actual implementation yet.
Jacek
On 2/5/21 9:18 AM, Jacek Caban wrote:
On 05.02.2021 03:43, Zebediah Figura (she/her) wrote:
On 1/22/21 9:51 AM, Jacek Caban wrote:
NtSetContextThread, unlike NtContinue, should not set nonvolatile registers.
Signed-off-by: Jacek Caban jacek@codeweavers.com
dlls/ntdll/unix/server.c | 2 +- dlls/ntdll/unix/signal_arm.c | 18 +++++++++++++----- dlls/ntdll/unix/signal_arm64.c | 20 ++++++++++++++------ dlls/ntdll/unix/signal_i386.c | 14 +++++++++++++- dlls/ntdll/unix/signal_x86_64.c | 15 ++++++++++++++- dlls/ntdll/unix/unix_private.h | 1 + 6 files changed, 56 insertions(+), 14 deletions(-)
Just to clarify—our NtSetContextThread does set nonvolatile registers, and continues to do so after this series. Is that incorrect behaviour?
No, it does not continue to do so after this series. This is covered by the test at the end of the series. NtSetContextThread does set nonvolatile registers in syscall frame so that if you set them and then query them before syscall returns, you will get modified values. However, syscall dispatcher does not restore them from syscall frame. It means that if you call NtSetContectThread on current thread, it will not set nonvolatile registers to passed values.
Ah, I see. That is a bit confusing, because it does still set them in the stored syscall frame, but doesn't restore them from the syscall frame.
I guess that kind of answers my below question too—I had assumed that the syscall frame was actually restoring these nonvolatile registers, and managed to not actually correctly interpret the tests to realize that wasn't the case. In that case it does make sense to have two different syscall exit paths, I think...?
Also:
diff --git a/dlls/ntdll/unix/server.c b/dlls/ntdll/unix/server.c index db09d7759da..fe581cca7b2 100644 --- a/dlls/ntdll/unix/server.c +++ b/dlls/ntdll/unix/server.c @@ -726,7 +726,7 @@ NTSTATUS WINAPI NtContinue( CONTEXT *context, BOOLEAN alertable ) status = server_select( NULL, 0, SELECT_INTERRUPTIBLE | SELECT_ALERTABLE, 0, NULL, NULL, &apc ); if (status == STATUS_USER_APC) invoke_apc( context, &apc ); } - return NtSetContextThread( GetCurrentThread(), context ); + signal_set_cpu_context( context ); }
Would it be simpler just to move NtContinue() to signal_*.c instead?
We'd then need to expose invoke_apc somehow, so I'm not sure if that would be better.
One further question regarding this patch series / patch 0017: I assume you're also planning to also change NtContinue() to use the syscall exit code instead of set_full_cpu_context()?
I considered it. My very first prototype worked like that because it always restored all registers, except for rax/eax which becomes a bit tricky for that use case. I was thinking about providing an alternative syscall leave implementation (NtContinue could replace its own return address to opt-in to this), which would do full restore. I didn't try implementing that variant yet to see if it's worth it.
For potential future plans, there are also call_user_apc_dispatcher and call_user_exception_dispatcher. They could potentially use the same mechanism as well instead of manually implementing crossing syscall barrier. They are more tricky to change, because we'd need to allow using stack is a more similar way to actual syscalls first. A simple solution would be to preserve enough unused stack on syscall entry, but a more complete solution like separated stacks could be even better.
BTW, as far as this series is considered, I think we will want a few specialized implementations for different CPU generations, so I plan to look at moving syscall dispatcher code from winebuild to ntdll first. ntdll.so would then set proper dispatcher depending on CPU capabilities. That requires changes to the way we access syscall table. I didn't do the actual implementation yet.
Jacek
-
Jacek Caban -
Zebediah Figura (she/her)