[Qemu-devel] [PATCH v3] SGABIOS: fix wrong video attrs for int 10h, ah==13h

[Qemu-devel] [PATCH v3] SGABIOS: fix wrong video attrs for int 10h, ah==13h

[Herbie Robinson]

Fix Bug #1670509: wrong video attrs for int 10h, ah==13h
The subroutine set_cursor_position is trashing %bx.
Unfortunately, %bl contains the video attribute for write_string.

The fix saves %bx in the function prolog and restores it in
the epilog. 

Signed-off-by: Herbie Robinson <Herbie.Robinson@stratus.com>
---
Removed incorrect code deletions from set_current_cursor.
Removed explanation of same from the commit message.

BTW, I didn't see any test cases for this code, but if they exist
and somebody points me at them, I should be able to generate a 
very small qcow2 file that demonstrates the problem.

 sgabios.S | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/sgabios.S b/sgabios.S
index 275d063..96c4e23 100644
--- a/sgabios.S
+++ b/sgabios.S
@@ -1728,6 +1728,7 @@ write_string_tail:
 
 set_cursor_position:
   pushw %ax
+  pushw %bx
   pushw %ds
   pushw $BDA_SEG
   popw %ds                             /* ds = 0x40 */
@@ -1738,6 +1739,7 @@ set_cursor_position:
   movw %ax, %bx                        /* bx = cursor save offset */
   movw %dx, (%bx)                      /* save new cursor value */
   popw %ds
+  popw %bx
   popw %ax
   ret
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3] SGABIOS: fix wrong video attrs for int 10h, ah==13h

[Laurent Vivier]

On 10/03/2017 16:54, Herbie Robinson wrote:
> Fix Bug #1670509: wrong video attrs for int 10h, ah==13h
> The subroutine set_cursor_position is trashing %bx.
> Unfortunately, %bl contains the video attribute for write_string.
> 
> The fix saves %bx in the function prolog and restores it in
> the epilog. 
> 
> Signed-off-by: Herbie Robinson <Herbie.Robinson@stratus.com>

Reviewed-by: Laurent Vivier <lvivier@redhat.com>


> ---
> Removed incorrect code deletions from set_current_cursor.
> Removed explanation of same from the commit message.
> 
> BTW, I didn't see any test cases for this code, but if they exist
> and somebody points me at them, I should be able to generate a 
> very small qcow2 file that demonstrates the problem.
> 
>  sgabios.S | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/sgabios.S b/sgabios.S
> index 275d063..96c4e23 100644
> --- a/sgabios.S
> +++ b/sgabios.S
> @@ -1728,6 +1728,7 @@ write_string_tail:
>  
>  set_cursor_position:
>    pushw %ax
> +  pushw %bx
>    pushw %ds
>    pushw $BDA_SEG
>    popw %ds                             /* ds = 0x40 */
> @@ -1738,6 +1739,7 @@ set_cursor_position:
>    movw %ax, %bx                        /* bx = cursor save offset */
>    movw %dx, (%bx)                      /* save new cursor value */
>    popw %ds
> +  popw %bx
>    popw %ax
>    ret
>  
> 

Re: [Qemu-devel] [PATCH v3] SGABIOS: fix wrong video attrs for int 10h, ah==13h

[Michael Tokarev]

10.03.2017 18:54, Herbie Robinson wrote:
> Fix Bug #1670509: wrong video attrs for int 10h, ah==13h
> The subroutine set_cursor_position is trashing %bx.
> Unfortunately, %bl contains the video attribute for write_string.
> 
> The fix saves %bx in the function prolog and restores it in
> the epilog. 
> 
> Signed-off-by: Herbie Robinson <Herbie.Robinson@stratus.com>
> ---
> Removed incorrect code deletions from set_current_cursor.
> Removed explanation of same from the commit message.
> 
> BTW, I didn't see any test cases for this code, but if they exist
> and somebody points me at them, I should be able to generate a 
> very small qcow2 file that demonstrates the problem.
> 
>  sgabios.S | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/sgabios.S b/sgabios.S

Now I'm not quite sure what to do with this.

It is part of sgabios, which is not qemu code but a submodule.

Maybe it's better to apply it upstream?
Or should we apply it directly?

Thanks,

/mjt

Re: [Qemu-devel] [PATCH v3] SGABIOS: fix wrong video attrs for int 10h, ah==13h

[Laurent Vivier]

On 23/04/2017 11:03, Michael Tokarev wrote:
> 10.03.2017 18:54, Herbie Robinson wrote:
>> Fix Bug #1670509: wrong video attrs for int 10h, ah==13h
>> The subroutine set_cursor_position is trashing %bx.
>> Unfortunately, %bl contains the video attribute for write_string.
>>
>> The fix saves %bx in the function prolog and restores it in
>> the epilog. 
>>
>> Signed-off-by: Herbie Robinson <Herbie.Robinson@stratus.com>
>> ---
>> Removed incorrect code deletions from set_current_cursor.
>> Removed explanation of same from the commit message.
>>
>> BTW, I didn't see any test cases for this code, but if they exist
>> and somebody points me at them, I should be able to generate a 
>> very small qcow2 file that demonstrates the problem.
>>
>>  sgabios.S | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/sgabios.S b/sgabios.S
> 
> Now I'm not quite sure what to do with this.
> 
> It is part of sgabios, which is not qemu code but a submodule.
> 
> Maybe it's better to apply it upstream?
> Or should we apply it directly?

The original project is hosted by code.google.com, that has been closed
in 2015. I doesn't appear there is a real alive repository hosting this
project now (except some multiple clones on github).

So I think you should apply it directly (and regenerate sgabios.bin)

I cc Paolo as he has added the submodule.

Thanks,
Laurent