Re: [PATCH] xen/arch/x86: Fix early boot command line parsing

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 03/12/13 10:34, Jan Beulich wrote:
>>>> On 03.12.13 at 03:14, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> On 02/12/2013 19:15, Daniel Kiper wrote:
>>> There is no reliable way to encode nul character as a character so encode
>>> it as a number. Read: 
>> http://sourceware.org/binutils/docs/as/Characters.html.
>>> Octal and hex encoding does not work on at least my system (GNU assembler
>>> version 2.22 (x86_64-linux-gnu) using BFD version (GNU Binutils for Debian) 
>> 2.22).
>>> Without this fix e.g. no-real-mode option at the end of xen.gz command line
>>> is not detected. Additionally, encode other characters accordingly to
>>> the gas documentation.
>>>
>>> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
>>> ---
>>>  xen/arch/x86/boot/cmdline.S |    6 +++---
>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/boot/cmdline.S b/xen/arch/x86/boot/cmdline.S
>>> index 05ffb94..4ea56b3 100644
>>> --- a/xen/arch/x86/boot/cmdline.S
>>> +++ b/xen/arch/x86/boot/cmdline.S
>>> @@ -138,11 +138,11 @@
>>>          call    .Lstrlen
>>>          add     $4,%esp
>>>          xadd    %eax,%ebx
>>> -        cmpb    $'\0',(%ebx)
>>> +        cmpb    $0,(%ebx)
>>>          je      3f
>>> -        cmpb    $' ',(%ebx)
>>> +        cmpb    $' ,(%ebx)
>>>          je      3f
>>> -        cmpb    $'=',(%ebx)
>>> +        cmpb    $'=,(%ebx)
>>>          jne     1b
>>>  3:      pop     %ebx
>>>          ret
>> My version of binutils 2.22 for Debian Wheezy does perfectly well with
>> these constants as-are.
> Does it? The '\0' is consistently being translated as '0' rather than
> the intended NIL for every gas from 2.18 to 2.23.2.
>
> I'm not eager to take the other two adjustments though - the
> code is better readable/understandable the with a closing quote
> in place.
>
> Jan
>

Ah - I missed the qualification of "at the end of the command line" - my
appologies.  My "perfectly well" was based on my recent use of
"no-real-mode" working fine, but it wasn't at the end of the command line.

Looking at the current disassembly,

0f c1 c3                xadd   %eax,%ebx
80 3b 30                cmpb   $0x30,(%ebx)
74 0a                   je     8028c2b1 <idx+0x8028c0b1>
80 3b 20                cmpb   $0x20,(%ebx)
74 05                   je     8028c2b1 <idx+0x8028c0b1>
80 3b 3d                cmpb   $0x3d,(%ebx)
75 c2                   jne    8028c273 <idx+0x8028c073>
5b                      pop    %ebx
c3                      ret


The NULL check is indeed being mistranslated, but the space and = are fine.

I would suggest switching it to $0 and leaving a line comment indicating
that this is a null check?  It is sad that '\0' doesn't work, as it is
far more informative of the intent of the code.

~Andrew