* [PATCH 0/2] x86/E820: a fix and an improvement @ 2017-12-12 10:54 Jan Beulich 2017-12-12 11:10 ` [PATCH 1/2] x86/E820: don't overrun array Jan Beulich 2017-12-12 11:10 ` [PATCH 2/2] x86/E820: improve insn selection Jan Beulich 0 siblings, 2 replies; 8+ messages in thread From: Jan Beulich @ 2017-12-12 10:54 UTC (permalink / raw) To: xen-devel; +Cc: Andrew Cooper 1: don't overrun array 2: improve insn selection Signed-off-by: Jan Beulich <jbeulich@suse.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] x86/E820: don't overrun array 2017-12-12 10:54 [PATCH 0/2] x86/E820: a fix and an improvement Jan Beulich @ 2017-12-12 11:10 ` Jan Beulich 2017-12-12 11:18 ` Andrew Cooper 2017-12-12 11:10 ` [PATCH 2/2] x86/E820: improve insn selection Jan Beulich 1 sibling, 1 reply; 8+ messages in thread From: Jan Beulich @ 2017-12-12 11:10 UTC (permalink / raw) To: xen-devel; +Cc: Andrew Cooper The bounds check needs to be done after the increment, not before, or else it needs to use a one lower immediate. Also use word operations rather than byte ones for both the increment and the compare (allowing E820_BIOS_MAX to be more easily bumped, should the need ever arise). Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/x86/boot/mem.S +++ b/xen/arch/x86/boot/mem.S @@ -22,11 +22,10 @@ get_memory_map: cmpl $SMAP,%eax # check the return is `SMAP' jne .Lmem88 - movb bootsym(e820nr),%al # up to 128 entries - cmpb $E820_BIOS_MAX,%al + incw bootsym(e820nr) + cmpw $E820_BIOS_MAX,bootsym(e820nr) # up to this many entries jae .Lmem88 - incb bootsym(e820nr) movw %di,%ax addw $20,%ax movw %ax,%di _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] x86/E820: don't overrun array 2017-12-12 11:10 ` [PATCH 1/2] x86/E820: don't overrun array Jan Beulich @ 2017-12-12 11:18 ` Andrew Cooper 2017-12-12 14:21 ` Jan Beulich 0 siblings, 1 reply; 8+ messages in thread From: Andrew Cooper @ 2017-12-12 11:18 UTC (permalink / raw) To: Jan Beulich, xen-devel On 12/12/17 11:10, Jan Beulich wrote: > The bounds check needs to be done after the increment, not before, or > else it needs to use a one lower immediate. Also use word operations > rather than byte ones for both the increment and the compare (allowing > E820_BIOS_MAX to be more easily bumped, should the need ever arise). > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/xen/arch/x86/boot/mem.S > +++ b/xen/arch/x86/boot/mem.S > @@ -22,11 +22,10 @@ get_memory_map: > cmpl $SMAP,%eax # check the return is `SMAP' > jne .Lmem88 > > - movb bootsym(e820nr),%al # up to 128 entries > - cmpb $E820_BIOS_MAX,%al > + incw bootsym(e820nr) > + cmpw $E820_BIOS_MAX,bootsym(e820nr) # up to this many entries Space after the comma here please. Given your subsequent instruction scheduling patch, why the word operations? 32bit operations are faster than 16bit. As e820nr is already a 32bit value, I'd just move them straight to incl/cmpl. ~Andrew > jae .Lmem88 > > - incb bootsym(e820nr) > movw %di,%ax > addw $20,%ax > movw %ax,%di > > > _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] x86/E820: don't overrun array 2017-12-12 11:18 ` Andrew Cooper @ 2017-12-12 14:21 ` Jan Beulich 2017-12-20 19:28 ` Andrew Cooper 0 siblings, 1 reply; 8+ messages in thread From: Jan Beulich @ 2017-12-12 14:21 UTC (permalink / raw) To: Andrew Cooper; +Cc: xen-devel >>> On 12.12.17 at 12:18, <andrew.cooper3@citrix.com> wrote: > On 12/12/17 11:10, Jan Beulich wrote: >> The bounds check needs to be done after the increment, not before, or >> else it needs to use a one lower immediate. Also use word operations >> rather than byte ones for both the increment and the compare (allowing >> E820_BIOS_MAX to be more easily bumped, should the need ever arise). >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> >> --- a/xen/arch/x86/boot/mem.S >> +++ b/xen/arch/x86/boot/mem.S >> @@ -22,11 +22,10 @@ get_memory_map: >> cmpl $SMAP,%eax # check the return is `SMAP' >> jne .Lmem88 >> >> - movb bootsym(e820nr),%al # up to 128 entries >> - cmpb $E820_BIOS_MAX,%al >> + incw bootsym(e820nr) >> + cmpw $E820_BIOS_MAX,bootsym(e820nr) # up to this many entries > > Space after the comma here please. Granted the file isn't consistent, but I had intentionally not added a comma here, to keep things uniform with the neighboring blocks. > Given your subsequent instruction scheduling patch, why the word > operations? 32bit operations are faster than 16bit. Not in 16-bit mode. Along the lines of the other patch the primary goal isn't insn scheduling, but insn size (to keep the trampoline small), so I'd like to avoid the operand size overrides. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] x86/E820: don't overrun array 2017-12-12 14:21 ` Jan Beulich @ 2017-12-20 19:28 ` Andrew Cooper 0 siblings, 0 replies; 8+ messages in thread From: Andrew Cooper @ 2017-12-20 19:28 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel On 12/12/17 14:21, Jan Beulich wrote: >>>> On 12.12.17 at 12:18, <andrew.cooper3@citrix.com> wrote: >> On 12/12/17 11:10, Jan Beulich wrote: >>> The bounds check needs to be done after the increment, not before, or >>> else it needs to use a one lower immediate. Also use word operations >>> rather than byte ones for both the increment and the compare (allowing >>> E820_BIOS_MAX to be more easily bumped, should the need ever arise). >>> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] x86/E820: improve insn selection 2017-12-12 10:54 [PATCH 0/2] x86/E820: a fix and an improvement Jan Beulich 2017-12-12 11:10 ` [PATCH 1/2] x86/E820: don't overrun array Jan Beulich @ 2017-12-12 11:10 ` Jan Beulich 2017-12-12 11:21 ` Andrew Cooper 1 sibling, 1 reply; 8+ messages in thread From: Jan Beulich @ 2017-12-12 11:10 UTC (permalink / raw) To: xen-devel; +Cc: Andrew Cooper ..., largely to shrink code size a little: - use TEST instead of CMP with zero immediate - use MOVZWL instead of AND with 0xffff immediate - compute final highmem_bk value in registers, accessing memory just once Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/x86/boot/mem.S +++ b/xen/arch/x86/boot/mem.S @@ -29,8 +29,8 @@ get_memory_map: movw %di,%ax addw $20,%ax movw %ax,%di - cmpl $0,%ebx # check to see if - jne 1b # %ebx is set to EOF + testl %ebx,%ebx # check to see if + jnz 1b # %ebx is set to EOF .Lmem88: movb $0x88, %ah @@ -48,17 +48,17 @@ get_memory_map: int $0x15 jc .Lint12 - cmpw $0x0, %cx # Kludge to handle BIOSes - jne 1f # which report their extended - cmpw $0x0, %dx # memory in AX/BX rather than - jne 1f # CX/DX. The spec I have read + testw %cx, %cx # Kludge to handle BIOSes + jnz 1f # which report their extended + testw %dx, %dx # memory in AX/BX rather than + jnz 1f # CX/DX. The spec I have read movw %ax, %cx # seems to indicate AX/BX movw %bx, %dx # are more reasonable anyway... -1: andl $0xffff,%edx # clear sign extend +1: movzwl %dx, %edx shll $6,%edx # and go from 64k to 1k chunks + movzwl %cx, %ecx + addl %ecx, %edx # add in lower memory movl %edx,bootsym(highmem_kb) # store extended memory size - andl $0xffff,%ecx # clear sign extend - addl %ecx,bootsym(highmem_kb) # and add lower memory into .Lint12: int $0x12 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] x86/E820: improve insn selection 2017-12-12 11:10 ` [PATCH 2/2] x86/E820: improve insn selection Jan Beulich @ 2017-12-12 11:21 ` Andrew Cooper 2017-12-12 14:25 ` Jan Beulich 0 siblings, 1 reply; 8+ messages in thread From: Andrew Cooper @ 2017-12-12 11:21 UTC (permalink / raw) To: Jan Beulich, xen-devel On 12/12/17 11:10, Jan Beulich wrote: > ..., largely to shrink code size a little: > - use TEST instead of CMP with zero immediate > - use MOVZWL instead of AND with 0xffff immediate > - compute final highmem_bk value in registers, accessing memory just > once > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, altbeit it preferably with space in the first hunk. Any chance we can drop redundant size suffixes as we go? > > --- a/xen/arch/x86/boot/mem.S > +++ b/xen/arch/x86/boot/mem.S > @@ -29,8 +29,8 @@ get_memory_map: > movw %di,%ax > addw $20,%ax > movw %ax,%di > - cmpl $0,%ebx # check to see if > - jne 1b # %ebx is set to EOF > + testl %ebx,%ebx # check to see if > + jnz 1b # %ebx is set to EOF > > .Lmem88: > movb $0x88, %ah > @@ -48,17 +48,17 @@ get_memory_map: > int $0x15 > jc .Lint12 > > - cmpw $0x0, %cx # Kludge to handle BIOSes > - jne 1f # which report their extended > - cmpw $0x0, %dx # memory in AX/BX rather than > - jne 1f # CX/DX. The spec I have read > + testw %cx, %cx # Kludge to handle BIOSes > + jnz 1f # which report their extended > + testw %dx, %dx # memory in AX/BX rather than > + jnz 1f # CX/DX. The spec I have read > movw %ax, %cx # seems to indicate AX/BX > movw %bx, %dx # are more reasonable anyway... > -1: andl $0xffff,%edx # clear sign extend > +1: movzwl %dx, %edx > shll $6,%edx # and go from 64k to 1k chunks > + movzwl %cx, %ecx > + addl %ecx, %edx # add in lower memory > movl %edx,bootsym(highmem_kb) # store extended memory size > - andl $0xffff,%ecx # clear sign extend > - addl %ecx,bootsym(highmem_kb) # and add lower memory into > > .Lint12: > int $0x12 > > > _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] x86/E820: improve insn selection 2017-12-12 11:21 ` Andrew Cooper @ 2017-12-12 14:25 ` Jan Beulich 0 siblings, 0 replies; 8+ messages in thread From: Jan Beulich @ 2017-12-12 14:25 UTC (permalink / raw) To: Andrew Cooper; +Cc: xen-devel >>> On 12.12.17 at 12:21, <andrew.cooper3@citrix.com> wrote: > On 12/12/17 11:10, Jan Beulich wrote: >> ..., largely to shrink code size a little: >> - use TEST instead of CMP with zero immediate >> - use MOVZWL instead of AND with 0xffff immediate >> - compute final highmem_bk value in registers, accessing memory just >> once >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, altbeit it > preferably with space in the first hunk. Thanks; similar reason as in the first patch for why I didn't add one. > Any chance we can drop redundant size suffixes as we go? This I did consider too, but the file consistently uses size suffixes everywhere (except on branches, where they're really odd to use; even gas has special code to make them as unnecessary as possible there), so I'd rather not start making it mishmash. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-12-20 19:28 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-12-12 10:54 [PATCH 0/2] x86/E820: a fix and an improvement Jan Beulich 2017-12-12 11:10 ` [PATCH 1/2] x86/E820: don't overrun array Jan Beulich 2017-12-12 11:18 ` Andrew Cooper 2017-12-12 14:21 ` Jan Beulich 2017-12-20 19:28 ` Andrew Cooper 2017-12-12 11:10 ` [PATCH 2/2] x86/E820: improve insn selection Jan Beulich 2017-12-12 11:21 ` Andrew Cooper 2017-12-12 14:25 ` Jan Beulich
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.