Re: [PATCH 58/79] OvmfPkg/Qemu: Fix various typos

["Laszlo Ersek" <lersek@redhat.com>]

On 12/09/19 13:55, Philippe Mathieu-Daudé wrote:
> Hi Laszlo,
> 
> On 12/3/19 10:00 PM, Laszlo Ersek wrote:
>> On 12/03/19 17:15, Philippe Mathieu-Daude wrote:
>>> From: Antoine Coeur <coeur@gmx.fr>
>>>
>>> Fix various typos in comments and documentation.
>>>
>>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
>>> Signed-off-by: Philippe Mathieu-Daude <philmd@redhat.com>
>>> ---
>>>   OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf | 
>>> 4 ++--
>>>   OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf        | 
>>> 4 ++--
>>>   OvmfPkg/AcpiPlatformDxe/Qemu.c                                   | 
>>> 4 ++--
>>>   OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c                          | 
>>> 2 +-
>>>   OvmfPkg/Library/DxePciLibI440FxQ35/PciLib.c                      | 
>>> 4 ++--
>>>   OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.c              | 
>>> 2 +-
>>>   OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c                      | 
>>> 2 +-
>>>   OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbInfo.c                 | 
>>> 4 ++--
>>>   OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c          | 
>>> 8 ++++----
>>>   OvmfPkg/QemuVideoDxe/Driver.c                                    | 
>>> 2 +-
>>>   OvmfPkg/QemuVideoDxe/VbeShim.asm                                 |
>>> 14 +++++++-------
>>>   11 files changed, 25 insertions(+), 25 deletions(-)
>>>
> [...]>> diff --git a/OvmfPkg/QemuVideoDxe/VbeShim.asm
> b/OvmfPkg/QemuVideoDxe/VbeShim.asm
>>> index cb2a60d8278d..1d284b264124 100644
>>> --- a/OvmfPkg/QemuVideoDxe/VbeShim.asm
>>> +++ b/OvmfPkg/QemuVideoDxe/VbeShim.asm
>>> @@ -49,7 +49,7 @@ Handler:
>>>     je         ReadEdid
>>>     cmp        ah, 0x00
>>>     je         SetModeLegacy
>>> -  DebugLog   StrUnkownFunction
>>> +  DebugLog   StrUnknownFunction
>>>   Hang:
>>>     jmp        Hang
>>>   @@ -93,7 +93,7 @@ GetModeInfo:
>>>     and        cx, ~0x4000 ; clear potentially set LFB bit in mode
>>> number
>>>     cmp        cx, 0x00f1
>>>     je         KnownMode1
>>> -  DebugLog   StrUnkownMode
>>> +  DebugLog   StrUnknownMode
>>>     jmp        Hang
>>>   KnownMode1:
>>>     ; target (es:di) set on input
>>> @@ -155,7 +155,7 @@ SetMode:
>>>       cmp        bx, 0x40f1
>>>     je         KnownMode2
>>> -  DebugLog   StrUnkownMode
>>> +  DebugLog   StrUnknownMode
>>>     jmp        Hang
>>>   KnownMode2:
>>>   @@ -203,7 +203,7 @@ SetModeLegacy:
>>>     je         KnownMode3
>>>     cmp        al, 0x12
>>>     je         KnownMode4
>>> -  DebugLog   StrUnkownMode
>>> +  DebugLog   StrUnknownMode
>>>     jmp        Hang
>>>   KnownMode3:
>>>     mov        al, 0x30
>>> @@ -252,7 +252,7 @@ StrExitSuccess:
>>>   StrExitUnsupported:
>>>     db 'Unsupported', 0x0a, 0
>>>   -StrUnkownFunction:
>>> +StrUnknownFunction:
>>>     db 'Unknown Function', 0x0a, 0
>>>     StrEnterGetInfo:
>>> @@ -270,8 +270,8 @@ StrEnterSetMode:
>>>   StrEnterSetModeLegacy:
>>>     db 'SetModeLegacy', 0x0a, 0
>>>   -StrUnkownMode:
>>> -  db 'Unkown Mode', 0x0a, 0
>>> +StrUnknownMode:
>>> +  db 'Unknown Mode', 0x0a, 0
>>>     StrGetPmCapabilities:
>>>     db 'GetPmCapabilities', 0x0a, 0
>>>
>>
>> If you modify "VbeShim.asm", then please re-run "VbeShim.sh", for
>> updating "VbeShim.h".
> 
> The DEBUG macro at the head of VbeShim.asm is commented out:
> 
>   ;%define DEBUG
> 
> 
> So the string "Unknown Mode" is not used.

Hmmm, OK. I did realize that DEBUG was commented out, and that that made
"StrUnkownMode" un-referenced.

However, I didn't know that NASM would eliminate unreferenced data.

> 
> Side note, running VbeShim.sh on Fedora 30 I get:
> 
> $ git diff --word-diff
> --- a/OvmfPkg/QemuVideoDxe/VbeShim.h
> +++ b/OvmfPkg/QemuVideoDxe/VbeShim.h
> @@ -523,13 +523,13 @@ STATIC CONST UINT8 mVbeShim[] = {
>   /* 0000020A cmp ax,0x4f02                  */  0x3D, 0x02, 0x4F,
>   /* 0000020D jz 0x269                       */  0x74, 0x5A,
>   /* 0000020F cmp ax,0x4f03                  */  0x3D, 0x03, 0x4F,
>   /* 00000212 jz [-word-]{+near+} 0x331                  */  0x0F, 0x84,
> 0x1B, 0x01,
>   /* 00000216 cmp ax,0x4f10                  */  0x3D, 0x10, 0x4F,
>   /* 00000219 jz [-word-]{+near+} 0x336                  */  0x0F, 0x84,
> 0x19, 0x01,
>   /* 0000021D cmp ax,0x4f15                  */  0x3D, 0x15, 0x4F,
>   /* 00000220 jz [-word-]{+near+} 0x338                  */  0x0F, 0x84,
> 0x14, 0x01,
>   /* 00000224 cmp ah,0x0                     */  0x80, 0xFC, 0x00,
>   /* 00000227 jz [-word-]{+near+} 0x33a                  */  0x0F, 0x84,
> 0x0F, 0x01,
>   /* 0000022B jmp short 0x22b                */  0xEB, 0xFE,
>   /* 0000022D push es                        */  0x06,
>   /* 0000022E push di                        */  0x57,
> @@ -547,7 +547,7 @@ STATIC CONST UINT8 mVbeShim[] = {
>   /* 0000023F pop ds                         */  0x1F,
>   /* 00000240 pop di                         */  0x5F,
>   /* 00000241 pop es                         */  0x07,
>   /* 00000242 jmp[-word-] 0x34c                      */  0xE9, 0x07, 0x01,
>   /* 00000245 push es                        */  0x06,
>   /* 00000246 push di                        */  0x57,
>   /* 00000247 push ds                        */  0x1E,
> @@ -568,7 +568,7 @@ STATIC CONST UINT8 mVbeShim[] = {
>   /* 00000263 pop ds                         */  0x1F,
>   /* 00000264 pop di                         */  0x5F,
>   /* 00000265 pop es                         */  0x07,
>   /* 00000266 jmp[-word-] 0x34c                      */  0xE9, 0xE3, 0x00,
>   /* 00000269 push dx                        */  0x52,
>   /* 0000026A push ax                        */  0x50,
>   /* 0000026B cmp bx,0x40f1                  */  0x81, 0xFB, 0xF1, 0x40,
> @@ -688,14 +688,14 @@ STATIC CONST UINT8 mVbeShim[] = {
>   /* 0000033C jz 0x345                       */  0x74, 0x07,
>   /* 0000033E cmp al,0x12                    */  0x3C, 0x12,
>   /* 00000340 jz 0x349                       */  0x74, 0x07,
>   /* 00000342 jmp[-word-] 0x22b                      */  0xE9, 0xE6, 0xFE,
>   /* 00000345 mov al,0x30                    */  0xB0, 0x30,
>   /* 00000347 jmp short 0x34b                */  0xEB, 0x02,
>   /* 00000349 mov al,0x20                    */  0xB0, 0x20,
>   /* 0000034B [-iretw-]{+iret+}                           */  0xCF,
>   /* 0000034C mov ax,0x4f                    */  0xB8, 0x4F, 0x00,
>   /* 0000034F [-iretw-]{+iret+}                           */  0xCF,
>   /* 00000350 mov ax,0x14f                   */  0xB8, 0x4F, 0x01,
>   /* 00000353 [-iretw-]{+iret+}                           */  0xCF,
> };
> #endif
> 
> The nasm packaged is:
> 
> $ nasm -v
> NASM version 2.13.03 compiled on Feb  2 2019
> 
> While NASM 2.12 is documented here:
> 
> https://github.com/tianocore/tianocore.github.io/wiki/Nasm-Setup
> 
> Should I downgrade it?
> 

No, don't bother, just please update the commit message that the byte
array in "VbeShim.h" does *not* change, as a result of this patch.

With the commit message update:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks!
Laszlo