diff mbox

[edk2,v2,6/6] OvmfPkg XenBusDxe: Convert X64/TestAndClearBit.asm to NASM

Message ID 54649253.9000609@redhat.com
State New
Headers show

Commit Message

Laszlo Ersek Nov. 13, 2014, 11:13 a.m. UTC
Anthony,

On 11/06/14 12:24, Anthony PERARD wrote:
> The BaseTools/Scripts/ConvertMasmToNasm.py script was used to convert
> X64/TestAndClearBit.asm to X64/TestAndClearBit.nasm
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  OvmfPkg/XenBusDxe/X64/TestAndClearBit.S                      | 12 ------------
>  .../X64/{TestAndClearBit.asm => TestAndClearBit.nasm}        |  8 ++++----
>  OvmfPkg/XenBusDxe/XenBusDxe.inf                              |  3 +--
>  3 files changed, 5 insertions(+), 18 deletions(-)
>  delete mode 100644 OvmfPkg/XenBusDxe/X64/TestAndClearBit.S
>  rename OvmfPkg/XenBusDxe/X64/{TestAndClearBit.asm => TestAndClearBit.nasm} (63%)
> 
> diff --git a/OvmfPkg/XenBusDxe/X64/TestAndClearBit.S b/OvmfPkg/XenBusDxe/X64/TestAndClearBit.S
> deleted file mode 100644
> index 0372e83..0000000
> --- a/OvmfPkg/XenBusDxe/X64/TestAndClearBit.S
> +++ /dev/null
> @@ -1,12 +0,0 @@
> -# INT32
> -# EFIAPI
> -# TestAndClearBit (
> -#   IN  INT32 Bit,                // rcx
> -#   IN  volatile VOID* Address    // rdx
> -#   );
> -ASM_GLOBAL ASM_PFX(TestAndClearBit)
> -ASM_PFX(TestAndClearBit):
> -  lock
> -  btrl %ecx, (%rdx)
> -  sbbl %eax, %eax
> -  ret
> diff --git a/OvmfPkg/XenBusDxe/X64/TestAndClearBit.asm b/OvmfPkg/XenBusDxe/X64/TestAndClearBit.nasm
> similarity index 63%
> rename from OvmfPkg/XenBusDxe/X64/TestAndClearBit.asm
> rename to OvmfPkg/XenBusDxe/X64/TestAndClearBit.nasm
> index 3a25879..38ac549 100644
> --- a/OvmfPkg/XenBusDxe/X64/TestAndClearBit.asm
> +++ b/OvmfPkg/XenBusDxe/X64/TestAndClearBit.nasm
> @@ -1,4 +1,5 @@
> -.code
> +DEFAULT REL
> +SECTION .text
>  
>  ; INT32
>  ; EFIAPI
> @@ -6,11 +7,10 @@
>  ;   IN  INT32 Bit,                // rcx
>  ;   IN  volatile VOID* Address    // rdx
>  ;   );
> -TestAndClearBit PROC
> +global ASM_PFX(TestAndClearBit)
> +ASM_PFX(TestAndClearBit):
>    lock
>    btr [rdx], ecx
>    sbb eax, eax
>    ret
> -TestAndClearBit ENDP
>  
> -END
> diff --git a/OvmfPkg/XenBusDxe/XenBusDxe.inf b/OvmfPkg/XenBusDxe/XenBusDxe.inf
> index 61f7568..4ce4743 100644
> --- a/OvmfPkg/XenBusDxe/XenBusDxe.inf
> +++ b/OvmfPkg/XenBusDxe/XenBusDxe.inf
> @@ -56,8 +56,7 @@
>  [Sources.X64]
>    X64/hypercall.nasm
>    X64/InterlockedCompareExchange16.nasm
> -  X64/TestAndClearBit.S
> -  X64/TestAndClearBit.asm
> +  X64/TestAndClearBit.nasm
>  
>  [LibraryClasses]
>    UefiDriverEntryPoint
> 

when building this, my nasm reports:

Build/OvmfX64/DEBUG_GCC48/X64/OvmfPkg/XenBusDxe/XenBusDxe/OUTPUT/X64/TestAndClearBit.iii:12: warning: instruction is not lockable

Looking at the "insns.dat" file in the "nasm" source code, I can see

BTR             mem,reg64                       [mr:    hle o64 0f b3 /r]                       X64,SM,LOCK

So apparently the problem is that you don't pass the bit position in a 64-bit register (rcx); you pass it in a 32-bit register (ecx). I tried the following patch:

But nasm emits the same warning. Any ideas?

The warning is a bit strange though, because the disassembly confirms the BTR is locked:

Disassembly of section .text:

0000000000000000 <TestAndClearBit>:
   0:   f0 48 0f b3 0a          lock btr %rcx,(%rdx)
   5:   19 c0                   sbb    %eax,%eax
   7:   c3                      retq   

So maybe it's a nasm bug.

Thanks
Laszlo

------------------------------------------------------------------------------
Comprehensive Server Monitoring with Site24x7.
Monitor 10 servers for $9/Month.
Get alerted through email, SMS, voice calls or mobile push notifications.
Take corrective actions from your mobile device.
http://pubads.g.doubleclick.net/gampad/clk?id=154624111&iu=/4140/ostg.clktrk

Comments

Anthony PERARD Nov. 13, 2014, 5:55 p.m. UTC | #1
On Thu, Nov 13, 2014 at 12:13:23PM +0100, Laszlo Ersek wrote:
> Anthony,
> 
> On 11/06/14 12:24, Anthony PERARD wrote:
> > The BaseTools/Scripts/ConvertMasmToNasm.py script was used to convert
> > X64/TestAndClearBit.asm to X64/TestAndClearBit.nasm
> > 
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> > ---
> >  OvmfPkg/XenBusDxe/X64/TestAndClearBit.S                      | 12 ------------
> >  .../X64/{TestAndClearBit.asm => TestAndClearBit.nasm}        |  8 ++++----
> >  OvmfPkg/XenBusDxe/XenBusDxe.inf                              |  3 +--
> >  3 files changed, 5 insertions(+), 18 deletions(-)
> >  delete mode 100644 OvmfPkg/XenBusDxe/X64/TestAndClearBit.S
> >  rename OvmfPkg/XenBusDxe/X64/{TestAndClearBit.asm => TestAndClearBit.nasm} (63%)
> > 
> > diff --git a/OvmfPkg/XenBusDxe/X64/TestAndClearBit.S b/OvmfPkg/XenBusDxe/X64/TestAndClearBit.S
> > deleted file mode 100644
> > index 0372e83..0000000
> > --- a/OvmfPkg/XenBusDxe/X64/TestAndClearBit.S
> > +++ /dev/null
> > @@ -1,12 +0,0 @@
> > -# INT32
> > -# EFIAPI
> > -# TestAndClearBit (
> > -#   IN  INT32 Bit,                // rcx
> > -#   IN  volatile VOID* Address    // rdx
> > -#   );
> > -ASM_GLOBAL ASM_PFX(TestAndClearBit)
> > -ASM_PFX(TestAndClearBit):
> > -  lock
> > -  btrl %ecx, (%rdx)
> > -  sbbl %eax, %eax
> > -  ret
> > diff --git a/OvmfPkg/XenBusDxe/X64/TestAndClearBit.asm b/OvmfPkg/XenBusDxe/X64/TestAndClearBit.nasm
> > similarity index 63%
> > rename from OvmfPkg/XenBusDxe/X64/TestAndClearBit.asm
> > rename to OvmfPkg/XenBusDxe/X64/TestAndClearBit.nasm
> > index 3a25879..38ac549 100644
> > --- a/OvmfPkg/XenBusDxe/X64/TestAndClearBit.asm
> > +++ b/OvmfPkg/XenBusDxe/X64/TestAndClearBit.nasm
> > @@ -1,4 +1,5 @@
> > -.code
> > +DEFAULT REL
> > +SECTION .text
> >  
> >  ; INT32
> >  ; EFIAPI
> > @@ -6,11 +7,10 @@
> >  ;   IN  INT32 Bit,                // rcx
> >  ;   IN  volatile VOID* Address    // rdx
> >  ;   );
> > -TestAndClearBit PROC
> > +global ASM_PFX(TestAndClearBit)
> > +ASM_PFX(TestAndClearBit):
> >    lock
> >    btr [rdx], ecx
> >    sbb eax, eax
> >    ret
> > -TestAndClearBit ENDP
> >  
> > -END
> > diff --git a/OvmfPkg/XenBusDxe/XenBusDxe.inf b/OvmfPkg/XenBusDxe/XenBusDxe.inf
> > index 61f7568..4ce4743 100644
> > --- a/OvmfPkg/XenBusDxe/XenBusDxe.inf
> > +++ b/OvmfPkg/XenBusDxe/XenBusDxe.inf
> > @@ -56,8 +56,7 @@
> >  [Sources.X64]
> >    X64/hypercall.nasm
> >    X64/InterlockedCompareExchange16.nasm
> > -  X64/TestAndClearBit.S
> > -  X64/TestAndClearBit.asm
> > +  X64/TestAndClearBit.nasm
> >  
> >  [LibraryClasses]
> >    UefiDriverEntryPoint
> > 
> 
> when building this, my nasm reports:
> 
> Build/OvmfX64/DEBUG_GCC48/X64/OvmfPkg/XenBusDxe/XenBusDxe/OUTPUT/X64/TestAndClearBit.iii:12: warning: instruction is not lockable
> 
> Looking at the "insns.dat" file in the "nasm" source code, I can see
> 
> BTR             mem,reg64                       [mr:    hle o64 0f b3 /r]                       X64,SM,LOCK
> 
> So apparently the problem is that you don't pass the bit position in a 64-bit register (rcx); you pass it in a 32-bit register (ecx). I tried the following patch:
> 
> diff --git a/OvmfPkg/XenBusDxe/X64/TestAndClearBit.nasm b/OvmfPkg/XenBusDxe/X64/TestAndClearBit.nasm
> index 38ac549..ce49d7f 100644
> --- a/OvmfPkg/XenBusDxe/X64/TestAndClearBit.nasm
> +++ b/OvmfPkg/XenBusDxe/X64/TestAndClearBit.nasm
> @@ -10,7 +10,7 @@ SECTION .text
>  global ASM_PFX(TestAndClearBit)
>  ASM_PFX(TestAndClearBit):
>    lock
> -  btr [rdx], ecx
> +  btr [rdx], rcx
>    sbb eax, eax
>    ret
> 
> But nasm emits the same warning. Any ideas?

Yes, put lock and btr on the same line ...
 ASM_PFX(TestAndClearBit):
 -  lock
 -  btr [rdx], ecx
 +  lock btr [rdx], ecx
    sbb eax, eax

This small patch appear to work, no more warning from nasm.

> The warning is a bit strange though, because the disassembly confirms the BTR is locked:
> 
> Disassembly of section .text:
> 
> 0000000000000000 <TestAndClearBit>:
>    0:   f0 48 0f b3 0a          lock btr %rcx,(%rdx)
>    5:   19 c0                   sbb    %eax,%eax
>    7:   c3                      retq   
> 
> So maybe it's a nasm bug.
Laszlo Ersek Nov. 13, 2014, 6 p.m. UTC | #2
On 11/13/14 18:55, Anthony PERARD wrote:
> On Thu, Nov 13, 2014 at 12:13:23PM +0100, Laszlo Ersek wrote:

>> But nasm emits the same warning. Any ideas?
> 
> Yes, put lock and btr on the same line ...
>  ASM_PFX(TestAndClearBit):
>  -  lock
>  -  btr [rdx], ecx
>  +  lock btr [rdx], ecx
>     sbb eax, eax
> 
> This small patch appear to work, no more warning from nasm.

Thanks. Can you please tack it as a standalone patch to your recent
series, as 5/4?

http://thread.gmane.org/gmane.comp.bios.tianocore.devel/11211

Cheers
Laszlo


------------------------------------------------------------------------------
Comprehensive Server Monitoring with Site24x7.
Monitor 10 servers for $9/Month.
Get alerted through email, SMS, voice calls or mobile push notifications.
Take corrective actions from your mobile device.
http://pubads.g.doubleclick.net/gampad/clk?id=154624111&iu=/4140/ostg.clktrk
Anthony PERARD Nov. 13, 2014, 6:13 p.m. UTC | #3
On Thu, Nov 13, 2014 at 07:00:18PM +0100, Laszlo Ersek wrote:
> On 11/13/14 18:55, Anthony PERARD wrote:
> > On Thu, Nov 13, 2014 at 12:13:23PM +0100, Laszlo Ersek wrote:
> 
> >> But nasm emits the same warning. Any ideas?
> > 
> > Yes, put lock and btr on the same line ...
> >  ASM_PFX(TestAndClearBit):
> >  -  lock
> >  -  btr [rdx], ecx
> >  +  lock btr [rdx], ecx
> >     sbb eax, eax
> > 
> > This small patch appear to work, no more warning from nasm.
> 
> Thanks. Can you please tack it as a standalone patch to your recent
> series, as 5/4?

yes.

> http://thread.gmane.org/gmane.comp.bios.tianocore.devel/11211
> 
> Cheers
> Laszlo
>
diff mbox

Patch

diff --git a/OvmfPkg/XenBusDxe/X64/TestAndClearBit.nasm b/OvmfPkg/XenBusDxe/X64/TestAndClearBit.nasm
index 38ac549..ce49d7f 100644
--- a/OvmfPkg/XenBusDxe/X64/TestAndClearBit.nasm
+++ b/OvmfPkg/XenBusDxe/X64/TestAndClearBit.nasm
@@ -10,7 +10,7 @@  SECTION .text
 global ASM_PFX(TestAndClearBit)
 ASM_PFX(TestAndClearBit):
   lock
-  btr [rdx], ecx
+  btr [rdx], rcx
   sbb eax, eax
   ret