Stop calling set_syscall_retval() from linux_set_syscall_retval().
The former clobbers some registers that shouldn't be touched.
Details
- Reviewers
kib - Group Reviewers
Linux Emulation - Commits
- rS366810: Stop calling set_syscall_retval() from linux_set_syscall_retval().
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
sys/amd64/linux/linux_sysvec.c | ||
---|---|---|
224 ↗ | (On Diff #76927) | Because that's the end of the fast path for this function. The set_syscall_retval() does the same. It makes actual sense there, since it doesn't need PCB_FULL_IRET, but I've tried to keep two routines as similar as possible. |
239 ↗ | (On Diff #76927) | This one is needed; the ones we do unconditionally are to be removed (eventually). Again, same as with set_syscall_retval(). |
246 ↗ | (On Diff #76927) | It is complete - it applies to CURRENT. Otherwise this patch would depend on the other one, otherwise unrelated. |
sys/amd64/linux/linux_sysvec.c | ||
---|---|---|
224 ↗ | (On Diff #76927) | But you still set FULL_IRET, which completely undermines the meaning of the 'fast path'. |
239 ↗ | (On Diff #76927) | I do not see this patch as committable until the 'eventually' part is resolved. set_syscall_retval() does not set FULL_IRET twice. |
246 ↗ | (On Diff #76927) | I do not understand this: you cannot get proper review until one of the patches is resolved. |
sys/amd64/linux/linux_sysvec.c | ||
---|---|---|
224 ↗ | (On Diff #76927) | True. The plan was to keep it as close to set_syscall_retval() as possible, and later just remove the problematic calls to set_pcb_flags(). But you're probably right that the code should fit the current state of things, not the one we'd like to have in the future. |
sys/amd64/linux/linux_sysvec.c | ||
---|---|---|
230 ↗ | (On Diff #77305) | This paragraph does not make much sense in the context of linux_set_syscall_retval. |
234 ↗ | (On Diff #77305) | Isn't this half of the comment relevant to all paths ending in set(FULL_IRET) ? I suggest to move it right above set_pcb_flags() instead of the wishful thinking that is currently at lines 2490250. |
sys/amd64/linux/linux_sysvec.c | ||
---|---|---|
230 ↗ | (On Diff #77305) | True. I've reworded this somewhat; it still gives a useful hint. |
sys/amd64/linux/linux_sysvec.c | ||
---|---|---|
218 ↗ | (On Diff #78023) | Why is this assignment needed ? |
sys/amd64/linux/linux_sysvec.c | ||
---|---|---|
218 ↗ | (On Diff #78023) | It's not; I just didn't quite understood what it did and wanted to be extra safe there. Removed. |
sys/amd64/linux/linux_sysvec.c | ||
---|---|---|
215 ↗ | (On Diff #78315) | May be move this comment down as the first sentence before 'require full context restore ...'. It would avoid requiring to read everything to understand that comment. |
246 ↗ | (On Diff #78315) | I think it would be more up to point if you say there '... to get all registers except %rcx and %r11 restored ...'. |