Differential D26406 Diff 76927 sys/amd64/linux/linux_sysvec.c

Changeset View

Standalone View

sys/amd64/linux/linux_sysvec.c

Show First 20 Lines • Show All 201 Lines • ▼ Show 20 Lines	linux_fetch_syscall_args(struct thread *td)

td->td_retval[0] = 0;		td->td_retval[0] = 0;
return (0);		return (0);
}		}

static void		static void
linux_set_syscall_retval(struct thread *td, int error)		linux_set_syscall_retval(struct thread *td, int error)
{		{
struct trapframe *frame = td->td_frame;		struct trapframe *frame;

		frame = td->td_frame;

/*		/*
* On Linux only %rcx and %r11 values are not preserved across		* On Linux only %rcx and %r11 values are not preserved across
* the syscall. So, do not clobber %rdx and %r10.		* the syscall. So, do not clobber %rdx and %r10.
*/		*/
		kibUnsubmitted Not Done Inline Actions 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. kib: May be move this comment down as the first sentence before 'require full context restore ...'.
td->td_retval[1] = frame->tf_rdx;
if (error != EJUSTRETURN)		if (__predict_true(error == 0)) {
		frame->tf_rax = td->td_retval[0];
frame->tf_r10 = frame->tf_rcx;		frame->tf_r10 = frame->tf_rcx;
		/* XXX: This shouldn't be needed. */
		set_pcb_flags(td->td_pcb, PCB_FULL_IRET);
		return;
		kibUnsubmitted Done Inline Actions So why return instead of break ? kib: So why return instead of break ?
		traszAuthorUnsubmitted Done Inline Actions 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. trasz: Because that's the end of the fast path for this function. The set_syscall_retval() does the…
		kibUnsubmitted Done Inline Actions But you still set FULL_IRET, which completely undermines the meaning of the 'fast path'. kib: But you still set FULL_IRET, which completely undermines the meaning of the 'fast path'.
		traszAuthorUnsubmitted Done Inline Actions 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. trasz: True. The plan was to keep it as close to set_syscall_retval() as possible, and later just…
		}
		kibUnsubmitted Done Inline Actions Why is this assignment needed ? kib: Why is this assignment needed ?
		traszAuthorUnsubmitted Done Inline Actions It's not; I just didn't quite understood what it did and wanted to be extra safe there. Removed. trasz: It's not; I just didn't quite understood what it did and wanted to be extra safe there.

cpu_set_syscall_retval(td, error);		switch (error) {
		case ERESTART:
		/*
		* Reconstruct pc, we know that 'syscall' is 2 bytes,
		* lcall $X,y is 7 bytes, int 0x80 is 2 bytes.
		* We saved this in tf_err.
		*
		* Require full context restore to get the arguments
		* in the registers reloaded at return to usermode.
		*/
		frame->tf_rip -= frame->tf_err;
		frame->tf_r10 = frame->tf_rcx;
		set_pcb_flags(td->td_pcb, PCB_FULL_IRET);
		kibUnsubmitted Done Inline Actions Why do this twice ? kib: Why do this twice ?
		traszAuthorUnsubmitted Done Inline Actions This one is needed; the ones we do unconditionally are to be removed (eventually). Again, same as with set_syscall_retval(). trasz: This one is needed; the ones we do unconditionally are to be removed (eventually). Again, same…
		kibUnsubmitted Done Inline Actions I do not see this patch as committable until the 'eventually' part is resolved. set_syscall_retval() does not set FULL_IRET twice. kib: I do not see this patch as committable until the 'eventually' part is resolved.
		break;

/* Restore all registers. */		case EJUSTRETURN:
		break;

		kibUnsubmitted Not Done Inline Actions I think it would be more up to point if you say there '... to get all registers except %rcx and %r11 restored ...'. kib: I think it would be more up to point if you say there '... to get all registers except %rcx and…
		default:
		kibUnsubmitted Done Inline Actions This paragraph does not make much sense in the context of linux_set_syscall_retval. kib: This paragraph does not make much sense in the context of linux_set_syscall_retval.
		traszAuthorUnsubmitted Done Inline Actions True. I've reworded this somewhat; it still gives a useful hint. trasz: True. I've reworded this somewhat; it still gives a useful hint.
		frame->tf_rax = SV_ABI_ERRNO(td->td_proc, error);
		kibUnsubmitted Done Inline Actions And you are trying to remove this macro in other review ? Why do you put incomplete change to review ? kib: And you are trying to remove this macro in other review ? Why do you put incomplete change to…
		traszAuthorUnsubmitted Done Inline Actions It is complete - it applies to CURRENT. Otherwise this patch would depend on the other one, otherwise unrelated. trasz: It is complete - it applies to CURRENT. Otherwise this patch would depend on the other one…
		kibUnsubmitted Done Inline Actions I do not understand this: you cannot get proper review until one of the patches is resolved. kib: I do not understand this: you cannot get proper review until one of the patches is resolved.
		frame->tf_r10 = frame->tf_rcx;
		set_pcb_flags(td->td_pcb, PCB_FULL_IRET);
		break;
		kibUnsubmitted Done Inline Actions 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. kib: Isn't this half of the comment relevant to all paths ending in set(FULL_IRET) ? I suggest to…
		}

		/* Restore all registers. XXX: This shouldn't be needed. */
set_pcb_flags(td->td_pcb, PCB_FULL_IRET);		set_pcb_flags(td->td_pcb, PCB_FULL_IRET);
}		}

static int		static int
linux_copyout_auxargs(struct image_params *imgp, uintptr_t base)		linux_copyout_auxargs(struct image_params *imgp, uintptr_t base)
{		{
Elf_Auxargs *args;		Elf_Auxargs *args;
Elf_Auxinfo argarray, pos;		Elf_Auxinfo argarray, pos;
▲ Show 20 Lines • Show All 705 Lines • Show Last 20 Lines