On 2014-12-14 23:31:28, Tian, Feng wrote: > Hi, John > > Thanks for raising this issue again. > > Here is the original patch proposed by Jordan, > - movw %es, %rax > + mov %es, %rax > > Here is the explanation in IA32 manual. > 1) IF Instruction is MOVW, THEN OperandSize is 16; > 2) MOV r/m64,Sreg Move zero extended 16-bit segment register to r/m64. > > That’s why I think the proposed patch may be inconsistent on behavior (The > former will not change the value of high 48bits. But the latter will set them > to 0). If I made some mistakes, please let me know. > > There was no progress because I didn’t get feedback from the original patch > owner.
I was opting for a simple change that was more consistent with other similar code in EDK II as I mentioned: http://permalink.gmane.org/gmane.comp.bios.tianocore.devel/10502 > For me, I prefer to use movw %es, ax if it could pass build. Any idea > on this, John & Jordan? I don't use DebugSupportDxe (nor would I recommend it to anyone at this time), so I don't think I'm in a good position to make a different change. If you want to fix the code differently than my patch, then I think you are in a better position to test it. I would only hope you would also make the code consistent across EDK II. $ git grep -E 'mov\s+rax,\s*[defg]s' EdkCompatibilityPkg/Compatibility/BootScriptSaveOnS3SaveStateThunk/X64/AsmDispatchExecute.asm: mov rax, ds IntelFspWrapperPkg/Library/BaseFspApiLib/X64/Thunk64To32.asm: mov rax, ds MdeModulePkg/Universal/DebugSupportDxe/X64/AsmFuncs.asm: mov rax, ds MdeModulePkg/Universal/DebugSupportDxe/X64/AsmFuncs.asm: mov rax, es MdeModulePkg/Universal/DebugSupportDxe/X64/AsmFuncs.asm: mov rax, fs MdeModulePkg/Universal/DebugSupportDxe/X64/AsmFuncs.asm: mov rax, gs UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.asm: mov rax, ds UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.asm: mov rax, es UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.asm: mov rax, fs UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.asm: mov rax, gs $ git grep -E 'movw?\s+%[defg]s,\s*%rax' MdeModulePkg/Universal/DebugSupportDxe/X64/AsmFuncs.S: mov %ds, %rax MdeModulePkg/Universal/DebugSupportDxe/X64/AsmFuncs.S: movw %es, %rax MdeModulePkg/Universal/DebugSupportDxe/X64/AsmFuncs.S: mov %fs, %rax MdeModulePkg/Universal/DebugSupportDxe/X64/AsmFuncs.S: mov %gs, %rax SourceLevelDebugPkg/Library/DebugAgent/DebugAgentCommon/X64/AsmFuncs.S: movw %ds, %rax SourceLevelDebugPkg/Library/DebugAgent/DebugAgentCommon/X64/AsmFuncs.S: movw %es, %rax SourceLevelDebugPkg/Library/DebugAgent/DebugAgentCommon/X64/AsmFuncs.S: movw %fs, %rax SourceLevelDebugPkg/Library/DebugAgent/DebugAgentCommon/X64/AsmFuncs.S: movw %gs, %rax -Jordan > From: John Smith [mailto:[email protected]] > Sent: Sunday, December 14, 2014 08:26 > To: [email protected] > Cc: Tian, Feng > Subject: Re: [edk2] [PATCH] MdeModulePkg DebugSupportDxe: Fix build error with > GNU assembler > > > > Oops, and I meant Feng, not Jiang. Was reading some other email from a > Jiang... > > V/R > > JRS > > > > On Sat, Dec 13, 2014 at 7:25 PM, John Smith <[email protected]> > wrote: > > I don't know a way to reply to a thread that predates my being on the list > but for this thread (which I've copied the last message, circa Oct 22 > below) it's still relevant. Because I just got this error in an Ubuntu > 14.10 VM, and the easy fix was just to change the asm. I don't know why > Jiang thinks the patch could change the upper 48 bits. He's probably so > used to looking at instructions in Intel syntax that he got confused over > which direction the move was happening. > > Anyway, point is, you can't build UDK2014 SP1 on Ubuntu 14.10 (with gcc > 4.9.1) without changing this single instruction. I chose to make it an ax > instead of rax. 6 of one, half dozen of the other. > > > > V/R > > > > JRS > > > > From: Tian, Feng <feng.tian@in...> - 2014-10-22 08:55:08 > > Jordan, > > > > Your patch is: > > > > - movw %es, %rax > > > > + mov %es, %rax > > > > The disassemble code through https://defuse.ca/online-x86-assembler.htm > is (I change it from AT&T syntax to Masm): > > 66 8c c0 movw rax, es > > 48 8c c0 mov rax, es > > > > I am not sure if it will cause behavior change, such as RAX's high 48 bit > is cleaned to 0? At least the binary of instruction is changed. Could you > help confirm this? > > > > Thanks > > Feng > > > > From: Fan, Jeff > > Sent: Wednesday, October 22, 2014 3:54 PM > > To: Andrew Fish; edk2-devel@... > > Cc: Tian, Feng > > Subject: RE: [edk2] [PATCH] MdeModulePkg DebugSupportDxe: Fix build error > with GNU assembler > > > > I agree eax is better (0 extended and small size) > > > > Jordan, I could clean up the incorrect usage in other packages.:-) > > > > Jeff > > From: Andrew Fish [mailto:afish@...] > > Sent: Wednesday, October 22, 2014 2:55 PM > > To: edk2-devel@...<mailto:edk2-devel@...> > > Cc: Fan, Jeff; Tian, Feng > > Subject: Re: [edk2] [PATCH] MdeModulePkg DebugSupportDxe: Fix build error > with GNU assembler > > > > > > On Oct 21, 2014, at 11:27 PM, Jordan Justen > <jljusten@...<mailto:jljusten@...>> wrote: > > > > On Sun, Oct 19, 2014 at 7:09 PM, Fan, Jeff > <jeff.fan@...<mailto:jeff.fan@...>> wrote: > > SourceLevelDebugPkg/DebugAgentLib x64 also used movw %es, %rax. > > We need to fix it also. > > > > And UefiCpuPkg/Library/CpuExceptionLib x64 used movl %es, %eax. > > We could clean it to make a consistence in code base. > > > > More than that, it appears that the MASM code does a mov rax, es too. > > > > I'm not sure that mov ax, es is really much of an improvement, and I'm > > not interested in cleaning all that code up. I'd just like > > MdeModulePkg to build. :) Would one of you be interested in pursuing > > this cleanup? > > > > I'm also not sure, but it could be that the mov rax, es actually does > > a zero extended move, so maybe the mov ax, es would be different. > > Would that impact the code that looks at this value later? > > > > Actually it looks like eax is the correct form for X64 code ... > > > > On the MASM side for X64 https://defuse.ca/online-x86-assembler.htm > > mov rax, es > > mov eax ,es > > mov ax, es > > mov es, rax > > mov es, eax > > mov es, ax > > 0: 48 8c c0 mov rax,es > > 3: 8c c0 mov eax,es > > 5: 66 8c c0 mov ax,es > > 8: 48 8e c0 mov es,rax > > b: 8e c0 mov es,eax > > d: 8e c0 mov es,eax > > ax generates 0x66 - operand-size override prefix > > rax generates 0x48 - REX use 64-bit operand size. > > > > Anyway, I think that my original patch makes the code more consistent > > with the current other GNU asm and MASM code. So, maybe it is okay as > > a small next step? > ------------------------------------------------------------------------------ Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server from Actuate! Instantly Supercharge Your Business Reports and Dashboards with Interactivity, Sharing, Native Excel Exports, App Integration & more Get technology previously reserved for billion-dollar corporations, FREE http://pubads.g.doubleclick.net/gampad/clk?id=164703151&iu=/4140/ostg.clktrk _______________________________________________ edk2-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/edk2-devel
