[PATCH 0/2] x86/E820: a fix and an improvement

[PATCH 0/2] x86/E820: a fix and an improvement

[Jan Beulich]

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

[PATCH 1/2] x86/E820: don't overrun array

[Jan Beulich]

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

[PATCH 2/2] x86/E820: improve insn selection

[Jan Beulich]

..., 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

Re: [PATCH 1/2] x86/E820: don't overrun array

[Andrew Cooper]

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

Re: [PATCH 2/2] x86/E820: improve insn selection

[Andrew Cooper]

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

Re: [PATCH 1/2] x86/E820: don't overrun array

[Jan Beulich]

>>> 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

Re: [PATCH 2/2] x86/E820: improve insn selection

[Jan Beulich]

>>> 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

Re: [PATCH 1/2] x86/E820: don't overrun array

[Andrew Cooper]

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