svn commit: r336876 - in head/sys: amd64/amd64 amd64/ia32 amd64/include conf dev/hyperv/vmbus/amd64

[Oliver Pinter]

On 7/30/18, Konstantin Belousov <kostikbel at gmail.com> wrote:
> Please trim useless content.
> Did I missed anything interesting in your mail ?
>
> On Sun, Jul 29, 2018 at 11:57:47PM +0200, Oliver Pinter wrote:
>> On 7/29/18, Konstantin Belousov <kib at freebsd.org> wrote:
>> > +ENTRY(copyin_smap)
>> > +	PUSH_FRAME_POINTER
>> > +	movq	PCPU(CURPCB),%rax
>> > +	movq	$copyin_fault,PCB_ONFAULT(%rax)
>> > +	testq	%rdx,%rdx			/* anything to do? */
>> > +	jz	done_copyin
>> > +
>> > +	/*
>> > +	 * make sure address is valid
>> > +	 */
>> > +	movq	%rdi,%rax
>> > +	addq	%rdx,%rax
>> > +	jc	copyin_fault
>> > +	movq	$VM_MAXUSER_ADDRESS,%rcx
>> > +	cmpq	%rcx,%rax
>> > +	ja	copyin_fault
>> > +
>> > +	xchgq	%rdi,%rsi
>> > +	movq	%rdx,%rcx
>> > +	movb	%cl,%al
>> > +	shrq	$3,%rcx				/* copy longword-wise */
>>
>> missing cld from here
> In fact not.  It is copyin_nosmap that got unneeded cld.
>
> See r327820, apparently I mis-merged this commit into the SMAP branch.
>
>>
>> > +	stac
>> > +	rep
>> > +	movsq
>> > +	movb	%al,%cl
>> > +	andb	$7,%cl				/* copy remaining bytes */
>> >  	je	done_copyin
>> >  	rep
>> >  	movsb
>> > +	clac
>
>> > +ENTRY(copyinstr_smap)
>> > +	PUSH_FRAME_POINTER
>> > +	movq	%rdx,%r8			/* %r8 = maxlen */
>> > +	movq	%rcx,%r9			/* %r9 = *len */
>> > +	xchgq	%rdi,%rsi			/* %rdi = from, %rsi = to */
>> > +	movq	PCPU(CURPCB),%rcx
>> > +	movq	$cpystrflt,PCB_ONFAULT(%rcx)
>> > +
>> > +	movq	$VM_MAXUSER_ADDRESS,%rax
>> > +
>> > +	/* make sure 'from' is within bounds */
>> > +	subq	%rsi,%rax
>> > +	jbe	cpystrflt
>> > +
>> > +	/* restrict maxlen to <= VM_MAXUSER_ADDRESS-from */
>> > +	cmpq	%rdx,%rax
>> > +	jae	1f
>> > +	movq	%rax,%rdx
>> > +	movq	%rax,%r8
>> > +1:
>> > +	incq	%rdx
>>
>> missing cld here
> Same.
>
>>
>> > +
>> > +2:
>> > +	decq	%rdx
>> > +	jz	copyinstr_succ
>>
>
>> cpystrflt_x:
>>         /* set *lencopied and return %eax */
>>         movq    PCPU(CURPCB),%rcx
>>         movq    $0,PCB_ONFAULT(%rcx)
>>
>>         testq   %r9,%r9
>>         jz      1f
>>         subq    %rdx,%r8
>>         movq    %r8,(%r9) << Here you access user-space, with cleared
>> RFLAGS.AC from the fault handler.
> How does this instruction access userspace ?  I do not see.

As far as I remember from 4 years, the r9 may contained a user-space
address in 10-STABLE
in the case of starting the init. I've a stac/clac pair in my internal
version, but I haven't found
yet the relevant commit message.

 For a quick grep around - http://ix.io/1fje - I haven't found yet
this place, so it's looks good in your version.

>
>> 1:
>>         POP_FRAME_POINTER
>>         ret
>
> So the patch below removes unneeded (mismerged) cld's left in the
> support.S.
>
> diff --git a/sys/amd64/amd64/support.S b/sys/amd64/amd64/support.S
> index 9b8b2a40461..0aa307e6895 100644
> --- a/sys/amd64/amd64/support.S
> +++ b/sys/amd64/amd64/support.S
> @@ -307,7 +307,6 @@ ENTRY(copyout_smap)
>  	movq	%rdx,%rcx
>
>  	shrq	$3,%rcx
> -	cld
>  	stac
>  	rep
>  	movsq
> @@ -358,7 +357,6 @@ ENTRY(copyin_nosmap)
>  	movq	%rdx,%rcx
>  	movb	%cl,%al
>  	shrq	$3,%rcx				/* copy longword-wise */
> -	cld
>  	rep
>  	movsq
>  	movb	%al,%cl
> @@ -887,7 +885,6 @@ ENTRY(copyinstr_nosmap)
>  	movq	%rax,%r8
>  1:
>  	incq	%rdx
> -	cld
>
>  2:
>  	decq	%rdx

Looks fine to me.
>