[PATCH v2 1/2] x86/boot: fix early error display

[PATCH v2 1/2] x86/boot: fix early error display

[Doug Goldstein]

From: David Esler <drumandstrum@gmail.com>

In 9180f5365524 a change was made to the send_chr function to take in
C-strings and print out a character at a time until a NULL was
encountered. However there is no code to increment the current character
position resulting in an endless loop of the first character. This adds
a simple increment.

Reviewed-by: Doug Goldstein <cardoe@cardoe.com>
Signed-off-by: David Esler <drumandstrum@gmail.com>
---
 xen/arch/x86/boot/head.S | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index fd6fc337fe..9cc35da558 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -173,10 +173,11 @@ not_multiboot:
 .Lget_vtb:
         mov     sym_esi(vga_text_buffer),%edi
 .Lsend_chr:
-        mov     (%esi),%bl
-        test    %bl,%bl        # Terminate on '\0' sentinel
+        lodsb
+        test    %al,%al        # Terminate on '\0' sentinel
         je      .Lhalt
         mov     $0x3f8+5,%dx   # UART Line Status Register
+        mov     %al,%bl
 2:      in      %dx,%al
         test    $0x20,%al      # Test THR Empty flag
         je      2b
@@ -185,7 +186,7 @@ not_multiboot:
         out     %al,%dx        # Send a character over the serial line
         test    %edi,%edi      # Is the VGA text buffer available?
         jz      .Lsend_chr
-        movsb                  # Write a character to the VGA text buffer
+        stosb                  # Write a character to the VGA text buffer
         mov     $7,%al
         stosb                  # Write an attribute to the VGA text buffer
         jmp     .Lsend_chr
-- 
2.13.5


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 2/2] x86/boot: rename send_chr to print_err

[Doug Goldstein]

From: David Esler <drumandstrum@gmail.com>

The send_chr function sends an entire C-string and not one character and
doesn't necessarily just send it over the serial UART anymore so rename
it to print_err so that its closer in name to what it does.

Reviewed-by: Doug Goldstein <cardoe@cardoe.com>
Signed-off-by: David Esler <drumandstrum@gmail.com>
---
 xen/arch/x86/boot/head.S | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index 9cc35da558..475c678f2c 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -161,7 +161,7 @@ not_multiboot:
          */
         add     $sym_offs(.Lbad_ldr_nbs),%esi   # Error message
         xor     %edi,%edi                       # No VGA text buffer
-        jmp     .Lsend_chr
+        jmp     .Lprint_err
 .Lmb2_efi_ia_32:
         /*
          * Here we are on EFI IA-32 platform. Then reliable vga_text_buffer zap is
@@ -169,10 +169,10 @@ not_multiboot:
          */
         add     $sym_offs(.Lbad_efi_msg),%esi   # Error message
         xor     %edi,%edi                       # No VGA text buffer
-        jmp     .Lsend_chr
+        jmp     .Lprint_err
 .Lget_vtb:
         mov     sym_esi(vga_text_buffer),%edi
-.Lsend_chr:
+.Lprint_err:
         lodsb
         test    %al,%al        # Terminate on '\0' sentinel
         je      .Lhalt
@@ -185,11 +185,11 @@ not_multiboot:
         mov     %bl,%al
         out     %al,%dx        # Send a character over the serial line
         test    %edi,%edi      # Is the VGA text buffer available?
-        jz      .Lsend_chr
+        jz      .Lprint_err
         stosb                  # Write a character to the VGA text buffer
         mov     $7,%al
         stosb                  # Write an attribute to the VGA text buffer
-        jmp     .Lsend_chr
+        jmp     .Lprint_err
 .Lhalt: hlt
         jmp     .Lhalt
 
-- 
2.13.5


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 1/2] x86/boot: fix early error display

[Jan Beulich]

>>> On 17.10.17 at 23:41, <cardoe@cardoe.com> wrote:
> From: David Esler <drumandstrum@gmail.com>
> 
> In 9180f5365524 a change was made to the send_chr function to take in
> C-strings and print out a character at a time until a NULL was
> encountered. However there is no code to increment the current character
> position resulting in an endless loop of the first character. This adds
> a simple increment.

This description is not accurate (it should have changed together with
the change to how you fix the issue) - with VGA the increment does
happen. Hence "display" in the title is perhaps also at least misleading.
I would be fine to adjust both while committing (and then adding my
R-b), but feel free to propose an alternative.

> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -173,10 +173,11 @@ not_multiboot:
>  .Lget_vtb:
>          mov     sym_esi(vga_text_buffer),%edi
>  .Lsend_chr:
> -        mov     (%esi),%bl
> -        test    %bl,%bl        # Terminate on '\0' sentinel
> +        lodsb
> +        test    %al,%al        # Terminate on '\0' sentinel
>          je      .Lhalt
>          mov     $0x3f8+5,%dx   # UART Line Status Register
> +        mov     %al,%bl
>  2:      in      %dx,%al
>          test    $0x20,%al      # Test THR Empty flag
>          je      2b

Daniel: What tells you that there is a UART at 0x3f8? Aren't we
risking to enter an infinite loop here if there isn't? At the very
least it would seem better to me if the VGA output was done
first - when you have a screen, you'd at least know something
was attempted to be output by seeing the first character. And
when you have neither screen nor UART (at the right port)
you're hosed anyway.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 2/2] x86/boot: rename send_chr to print_err

[Jan Beulich]

>>> On 17.10.17 at 23:41, <cardoe@cardoe.com> wrote:
> From: David Esler <drumandstrum@gmail.com>
> 
> The send_chr function sends an entire C-string and not one character and
> doesn't necessarily just send it over the serial UART anymore so rename
> it to print_err so that its closer in name to what it does.
> 
> Reviewed-by: Doug Goldstein <cardoe@cardoe.com>
> Signed-off-by: David Esler <drumandstrum@gmail.com>

Acked-by: Jan Beulich <jbeulich@suse.com>



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 1/2] x86/boot: fix early error display

[Daniel Kiper]

On Tue, Oct 17, 2017 at 04:41:37PM -0500, Doug Goldstein wrote:
> From: David Esler <drumandstrum@gmail.com>
>
> In 9180f5365524 a change was made to the send_chr function to take in
> C-strings and print out a character at a time until a NULL was
> encountered. However there is no code to increment the current character
> position resulting in an endless loop of the first character. This adds
> a simple increment.
>
> Reviewed-by: Doug Goldstein <cardoe@cardoe.com>

I was told that "Reviewed-by: ..." should be after SOB.

> Signed-off-by: David Esler <drumandstrum@gmail.com>

In general Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Though please take into account Jan's request WRT to commit
message. Or I am OK with Jan's changes before committing.

Daniel

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 2/2] x86/boot: rename send_chr to print_err

[Daniel Kiper]

On Tue, Oct 17, 2017 at 04:41:38PM -0500, Doug Goldstein wrote:
> From: David Esler <drumandstrum@gmail.com>
>
> The send_chr function sends an entire C-string and not one character and
> doesn't necessarily just send it over the serial UART anymore so rename
> it to print_err so that its closer in name to what it does.
>
> Reviewed-by: Doug Goldstein <cardoe@cardoe.com>

Ditto.

> Signed-off-by: David Esler <drumandstrum@gmail.com>

Anyway, Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 1/2] x86/boot: fix early error display

[Doug Goldstein]

On 10/18/17 4:50 AM, Jan Beulich wrote:
>>>> On 17.10.17 at 23:41, <cardoe@cardoe.com> wrote:
>> From: David Esler <drumandstrum@gmail.com>
>>
>> In 9180f5365524 a change was made to the send_chr function to take in
>> C-strings and print out a character at a time until a NULL was
>> encountered. However there is no code to increment the current character
>> position resulting in an endless loop of the first character. This adds
>> a simple increment.
> 
> This description is not accurate (it should have changed together with
> the change to how you fix the issue) - with VGA the increment does
> happen. Hence "display" in the title is perhaps also at least misleading.
> I would be fine to adjust both while committing (and then adding my
> R-b), but feel free to propose an alternative.

David and I are both ok with you changing the wording as necessary. I
apologize for not updating the commit message.

-- 
Doug Goldstein

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 1/2] x86/boot: fix early error display

[Doug Goldstein]

On 10/18/17 4:50 AM, Jan Beulich wrote:
>>>> On 17.10.17 at 23:41, <cardoe@cardoe.com> wrote:
>> From: David Esler <drumandstrum@gmail.com>
>>
>> In 9180f5365524 a change was made to the send_chr function to take in
>> C-strings and print out a character at a time until a NULL was
>> encountered. However there is no code to increment the current character
>> position resulting in an endless loop of the first character. This adds
>> a simple increment.
> 
> This description is not accurate (it should have changed together with
> the change to how you fix the issue) - with VGA the increment does
> happen. Hence "display" in the title is perhaps also at least misleading.
> I would be fine to adjust both while committing (and then adding my
> R-b), but feel free to propose an alternative.

Jan,

Can you queue this for 4.9 as well? That's where we ran into the issue
in the first place.

-- 
Doug Goldstein

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 1/2] x86/boot: fix early error display

[Jan Beulich]

>>> On 26.10.17 at 23:05, <cardoe@cardoe.com> wrote:
> On 10/18/17 4:50 AM, Jan Beulich wrote:
>>>>> On 17.10.17 at 23:41, <cardoe@cardoe.com> wrote:
>>> From: David Esler <drumandstrum@gmail.com>
>>>
>>> In 9180f5365524 a change was made to the send_chr function to take in
>>> C-strings and print out a character at a time until a NULL was
>>> encountered. However there is no code to increment the current character
>>> position resulting in an endless loop of the first character. This adds
>>> a simple increment.
>> 
>> This description is not accurate (it should have changed together with
>> the change to how you fix the issue) - with VGA the increment does
>> happen. Hence "display" in the title is perhaps also at least misleading.
>> I would be fine to adjust both while committing (and then adding my
>> R-b), but feel free to propose an alternative.
> 
> Jan,
> 
> Can you queue this for 4.9 as well? That's where we ran into the issue
> in the first place.

That how I did understand it, so I've queued this already, but for
it to become eligible to applying to 4.9 it first needs to pass the
push gate on master.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 1/2] x86/boot: fix early error display

[Doug Goldstein]

On 10/27/17 1:37 AM, Jan Beulich wrote:
>>>> On 26.10.17 at 23:05, <cardoe@cardoe.com> wrote:
>> On 10/18/17 4:50 AM, Jan Beulich wrote:
>>>>>> On 17.10.17 at 23:41, <cardoe@cardoe.com> wrote:
>>>> From: David Esler <drumandstrum@gmail.com>
>>>>
>>>> In 9180f5365524 a change was made to the send_chr function to take in
>>>> C-strings and print out a character at a time until a NULL was
>>>> encountered. However there is no code to increment the current character
>>>> position resulting in an endless loop of the first character. This adds
>>>> a simple increment.
>>>
>>> This description is not accurate (it should have changed together with
>>> the change to how you fix the issue) - with VGA the increment does
>>> happen. Hence "display" in the title is perhaps also at least misleading.
>>> I would be fine to adjust both while committing (and then adding my
>>> R-b), but feel free to propose an alternative.
>>
>> Jan,
>>
>> Can you queue this for 4.9 as well? That's where we ran into the issue
>> in the first place.
> 
> That how I did understand it, so I've queued this already, but for
> it to become eligible to applying to 4.9 it first needs to pass the
> push gate on master.
> 
> Jan
> 

Of course. I didn't mean to imply this should jump any existing process.
I had just realized that I didn't explicitly mention versions in my
original email and I just wanted to make sure I was explicit instead of
assuming.

Thanks again.
-- 
Doug Goldstein

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel