This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: PATCH: Remove memory check on SVME instructions
- From: "H.J. Lu" <hjl at lucon dot org>
- To: Jan Beulich <jbeulich at novell dot com>
- Cc: christophe dot harle at amd dot com, dwarak dot rajagopal at amd dot com, michael dot meissner at amd dot com, binutils at sources dot redhat dot com
- Date: Wed, 5 Sep 2007 23:01:55 -0700
- Subject: Re: PATCH: Remove memory check on SVME instructions
- References: <46DFA128020000780003C708@public.id2-vpn.continvity.gns.novell.com>
On Thu, Sep 06, 2007 at 06:41:44AM +0100, Jan Beulich wrote:
> >>> "H.J. Lu" <hjl@lucon.org> 09/06/07 6:42 AM >>>
> >On Wed, Sep 05, 2007 at 06:07:04PM +0100, Jan Beulich wrote:
> >> >>> "H.J. Lu" <hjl@lucon.org> 09/05/07 3:27 PM >>>
> >> >> >SVME instructions don't take any memory operand. You will get an
> >> >> >error if you try to use an memory operand.
> >> >>
> >> >> They really do, just the addressing is so that only rAX can be used. If you check
> >> >> the svme test case, you'll see that there are explict uses of memory operands. In
> >> >> the original patch implementation it may not have been that way, but since the
> >> >> meaning of the operands is memory-like, we agreed on a memory-like notation
> >> >> iirc.
> >> >
> >> >It is wrong and has been fixed.
> >>
> >> On what basis are you concluding this? As I said, we had specifically agreed on a
> >> memory operand like notation, to reflect the actual effect these opcodes have.
> >
> >It doesn't make those instructions take memory operands as specified
> >in IA32 encoding scheme.
>
> But that is the point I'm trying to make: No matter what the byte level encoding,
> mnemonics and operands should be expressing what an instruction does (e.g. I
> would also say that monitor hasn't been defined properly). And from that
> perspective, invlpga parallels invlpg, and vmload/vmrun/vmsave parallel fsave/
> frstor/etc (and I was actually promised the mnemonics in the manual would be
> changed accordingly, which unfortunately hasn't happened).
>
They didn't do it for a good reason. In x86 assembly, memory operand
is specified as base + index*scale + offset. If an operand can't be
expressed that way, it isn't a memory operand. It is different from
memory operand in high level language. SVME instructions do take
memory address in C and C++.
BTW, I double checked the spec. Some SVME instructions take 32bit
register in 64bit mode. This patch corrects it.
H.J.
----
gas/
2007-09-05 H.J. Lu <hongjiu.lu@intel.com>
* config/tc-i386.c (match_template): Handle invlpga, vmload,
vmrun and vmsave in SVME.
(process_suffix): Likewise.
gas/testsuite/
2007-09-05 H.J. Lu <hongjiu.lu@intel.com>
* gas/i386/svme.s: Updated to allow eax in 64bit.
* gas/i386/svme.d: Updated.
* gas/i386/svme64.d: Likewise.
opcodes/
2007-08-30 H.J. Lu <hongjiu.lu@intel.com>
* i386-opc.tbl: Correct SVME instructions to allow 32bit register
operand in 64bit mode.
* i386-tbl.h: Regenerated.
--- binutils/gas/config/tc-i386.c.svme 2007-09-05 22:15:04.000000000 -0700
+++ binutils/gas/config/tc-i386.c 2007-09-05 22:59:31.000000000 -0700
@@ -2664,9 +2664,15 @@ match_template (void)
|| !MATCH (overlap1, i.types[1], operand_types[1])
/* monitor in SSE3 is a very special case. The first
register and the second register may have different
- sizes. The same applies to crc32 in SSE4.2. */
+ sizes. The same applies to crc32 in SSE4.2. It is
+ also true for invlpga, vmload, vmrun and vmsave in
+ SVME. */
|| !((t->base_opcode == 0x0f01
- && t->extension_opcode == 0xc8)
+ && (t->extension_opcode == 0xc8
+ || t->extension_opcode == 0xd8
+ || t->extension_opcode == 0xda
+ || t->extension_opcode == 0xdb
+ || t->extension_opcode == 0xdf))
|| t->base_opcode == 0xf20f38f1
|| CONSISTENT_REGISTER_MATCH (overlap0, i.types[0],
operand_types[0],
@@ -3000,11 +3006,17 @@ process_suffix (void)
/* Now select between word & dword operations via the operand
size prefix, except for instructions that will ignore this
prefix anyway. */
- if (i.tm.base_opcode == 0x0f01 && i.tm.extension_opcode == 0xc8)
+ if (i.tm.base_opcode == 0x0f01
+ && (i.tm.extension_opcode == 0xc8
+ || i.tm.extension_opcode == 0xd8
+ || i.tm.extension_opcode == 0xda
+ || i.tm.extension_opcode == 0xdb
+ || i.tm.extension_opcode == 0xdf))
{
/* monitor in SSE3 is a very special case. The default size
of AX is the size of mode. The address size override
- prefix will change the size of AX. */
+ prefix will change the size of AX. It is also true for
+ invlpga, vmload, vmrun and vmsave in SVME. */
if (i.op->regs[0].reg_type &
(flag_code == CODE_32BIT ? Reg16 : Reg32))
if (!add_prefix (ADDR_PREFIX_OPCODE))
--- binutils/gas/testsuite/gas/i386/svme.d.svme 2007-08-31 16:00:02.000000000 -0700
+++ binutils/gas/testsuite/gas/i386/svme.d 2007-09-05 22:39:41.000000000 -0700
@@ -15,15 +15,15 @@ Disassembly of section .text:
[ ]*[0-9a-f]+:[ ]+0f 01 d8[ ]+vmrun[ ]*
[ ]*[0-9a-f]+:[ ]+0f 01 db[ ]+vmsave[ ]*
[0-9a-f]+ <att32>:
+[ ]*[0-9a-f]+:[ ]+0f 01 de[ ]+skinit[ ]*
[ ]*[0-9a-f]+:[ ]+0f 01 df[ ]+invlpga[ ]*
[ ]*[0-9a-f]+:[ ]+0f 01 da[ ]+vmload[ ]*
[ ]*[0-9a-f]+:[ ]+0f 01 d8[ ]+vmrun[ ]*
[ ]*[0-9a-f]+:[ ]+0f 01 db[ ]+vmsave[ ]*
-[ ]*[0-9a-f]+:[ ]+0f 01 de[ ]+skinit[ ]*
[0-9a-f]+ <intel32>:
+[ ]*[0-9a-f]+:[ ]+0f 01 de[ ]+skinit[ ]*
[ ]*[0-9a-f]+:[ ]+0f 01 df[ ]+invlpga[ ]*
[ ]*[0-9a-f]+:[ ]+0f 01 da[ ]+vmload[ ]*
[ ]*[0-9a-f]+:[ ]+0f 01 d8[ ]+vmrun[ ]*
[ ]*[0-9a-f]+:[ ]+0f 01 db[ ]+vmsave[ ]*
-[ ]*[0-9a-f]+:[ ]+0f 01 de[ ]+skinit[ ]*
#pass
--- binutils/gas/testsuite/gas/i386/svme.s.svme 2007-08-31 16:00:02.000000000 -0700
+++ binutils/gas/testsuite/gas/i386/svme.s 2007-09-05 22:40:19.000000000 -0700
@@ -19,20 +19,18 @@ common:
.ifdef __amd64__
att64:
do_args %rax, %ecx
-.else
-att32:
- do_args %eax, %ecx
.endif
+att32:
skinit %eax
+ do_args %eax, %ecx
.intel_syntax noprefix
.ifdef __amd64__
intel64:
do_args rax, ecx
-.else
-intel32:
- do_args eax, ecx
.endif
+intel32:
skinit eax
+ do_args eax, ecx
.p2align 4,0
--- binutils/gas/testsuite/gas/i386/svme64.d.svme 2007-08-31 16:00:02.000000000 -0700
+++ binutils/gas/testsuite/gas/i386/svme64.d 2007-09-05 22:41:31.000000000 -0700
@@ -21,11 +21,21 @@ Disassembly of section .text:
[ ]*[0-9a-f]+:[ ]+0f 01 da[ ]+vmload[ ]*
[ ]*[0-9a-f]+:[ ]+0f 01 d8[ ]+vmrun[ ]*
[ ]*[0-9a-f]+:[ ]+0f 01 db[ ]+vmsave[ ]*
+[0-9a-f]+ <att32>:
[ ]*[0-9a-f]+:[ ]+0f 01 de[ ]+skinit[ ]*
+[ ]*[0-9a-f]+:[ ]+67 0f 01 df[ ]+addr32 invlpga[ ]
+[ ]*[0-9a-f]+:[ ]+67 0f 01 da[ ]+addr32 vmload[ ]
+[ ]*[0-9a-f]+:[ ]+67 0f 01 d8[ ]+addr32 vmrun[ ]
+[ ]*[0-9a-f]+:[ ]+67 0f 01 db[ ]+addr32 vmsave[ ]
[0-9a-f]+ <intel64>:
[ ]*[0-9a-f]+:[ ]+0f 01 df[ ]+invlpga[ ]*
[ ]*[0-9a-f]+:[ ]+0f 01 da[ ]+vmload[ ]*
[ ]*[0-9a-f]+:[ ]+0f 01 d8[ ]+vmrun[ ]*
[ ]*[0-9a-f]+:[ ]+0f 01 db[ ]+vmsave[ ]*
+[0-9a-f]+ <intel32>:
[ ]*[0-9a-f]+:[ ]+0f 01 de[ ]+skinit[ ]*
+[ ]*[0-9a-f]+:[ ]+67 0f 01 df[ ]+addr32 invlpga[ ]
+[ ]*[0-9a-f]+:[ ]+67 0f 01 da[ ]+addr32 vmload[ ]
+[ ]*[0-9a-f]+:[ ]+67 0f 01 d8[ ]+addr32 vmrun[ ]
+[ ]*[0-9a-f]+:[ ]+67 0f 01 db[ ]+addr32 vmsave[ ]
#pass
--- binutils/opcodes/i386-opc.tbl.svme 2007-08-31 16:00:02.000000000 -0700
+++ binutils/opcodes/i386-opc.tbl 2007-09-05 22:33:51.000000000 -0700
@@ -1460,30 +1460,22 @@ rdtscp, 0, 0xf01, 0xf9, CpuSledgehammer,
// AMD Pacifica additions.
clgi, 0, 0xf01, 0xdd, CpuSVME, No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_xSuf|ImmExt, { 0 }
invlpga, 0, 0xf01, 0xdf, CpuSVME, No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_xSuf|ImmExt, { 0 }
-// FIXME: Need to ensure only "invlpga %eax,%ecx" is accepted.
-invlpga, 2, 0xf01, 0xdf, CpuSVME|CpuNo64, No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_xSuf|ImmExt, { Reg32, Reg32 }
-// FIXME: Need to ensure only "invlpga %rax,%ecx" is accepted.
-invlpga, 2, 0xf01, 0xdf, CpuSVME|Cpu64, No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_xSuf|ImmExt|NoRex64, { Reg64, Reg32 }
+// FIXME: Need to ensure only "invlpga %[re]ax,%ecx" is accepted.
+invlpga, 2, 0xf01, 0xdf, CpuSVME, No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_xSuf|ImmExt|NoRex64, { Reg32|Reg64, Reg32 }
skinit, 0, 0xf01, 0xde, CpuSVME, No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_xSuf|ImmExt, { 0 }
// FIXME: Need to ensure only "skinit %eax" is accepted.
skinit, 1, 0xf01, 0xde, CpuSVME, No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_xSuf|ImmExt, { Reg32 }
stgi, 0, 0xf01, 0xdc, CpuSVME, No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_xSuf|ImmExt, { 0 }
vmload, 0, 0xf01, 0xda, CpuSVME, No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_xSuf|ImmExt, { 0 }
-// FIXME: Need to ensure only "vmload %eax" is accepted.
-vmload, 1, 0xf01, 0xda, CpuSVME|CpuNo64, No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_xSuf|ImmExt, { Reg32 }
-// FIXME: Need to ensure only "vmload %rax" is accepted.
-vmload, 1, 0xf01, 0xda, CpuSVME|Cpu64, No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_xSuf|ImmExt|NoRex64, { Reg64 }
+// FIXME: Need to ensure only "vmload %[re]ax" is accepted.
+vmload, 1, 0xf01, 0xda, CpuSVME, No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_xSuf|ImmExt|NoRex64, { Reg32|Reg64 }
vmmcall, 0, 0xf01, 0xd9, CpuSVME, No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_xSuf|ImmExt, { 0 }
vmrun, 0, 0xf01, 0xd8, CpuSVME, No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_xSuf|ImmExt, { 0 }
-// FIXME: Need to ensure only "vmrun %eax" is accepted.
-vmrun, 1, 0xf01, 0xd8, CpuSVME|CpuNo64, No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_xSuf|ImmExt, { Reg32 }
-// FIXME: Need to ensure only "vmrun %rax" is accepted.
-vmrun, 1, 0xf01, 0xd8, CpuSVME|Cpu64, No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_xSuf|ImmExt|NoRex64, { Reg64 }
+// FIXME: Need to ensure only "vmrun %[re]ax" is accepted.
+vmrun, 1, 0xf01, 0xd8, CpuSVME, No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_xSuf|ImmExt|NoRex64, { Reg32|Reg64 }
vmsave, 0, 0xf01, 0xdb, CpuSVME, No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_xSuf|ImmExt, { 0 }
-// FIXME: Need to ensure only "vmsave %eax" is accepted.
-vmsave, 1, 0xf01, 0xdb, CpuSVME|CpuNo64, No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_xSuf|ImmExt, { Reg32 }
-// FIXME: Need to ensure only "vmsave %rax" is accepted.
-vmsave, 1, 0xf01, 0xdb, CpuSVME|Cpu64, No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_xSuf|ImmExt|NoRex64, { Reg64 }
+// FIXME: Need to ensure only "vmsave %[re]ax" is accepted.
+vmsave, 1, 0xf01, 0xdb, CpuSVME, No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_xSuf|ImmExt|NoRex64, { Reg32|Reg64 }
// SSE4a instructions
--- binutils/opcodes/i386-tbl.h.svme 2007-08-31 16:00:02.000000000 -0700
+++ binutils/opcodes/i386-tbl.h 2007-09-05 22:34:04.000000000 -0700
@@ -4189,13 +4189,9 @@ const template i386_optab[] =
{ "invlpga", 0, 0xf01, 0xdf, CpuSVME,
No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_xSuf|ImmExt,
{ 0 } },
- { "invlpga", 2, 0xf01, 0xdf, CpuSVME|CpuNo64,
- No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_xSuf|ImmExt,
- { Reg32,
- Reg32 } },
- { "invlpga", 2, 0xf01, 0xdf, CpuSVME|Cpu64,
+ { "invlpga", 2, 0xf01, 0xdf, CpuSVME,
No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_xSuf|ImmExt|NoRex64,
- { Reg64,
+ { Reg32|Reg64,
Reg32 } },
{ "skinit", 0, 0xf01, 0xde, CpuSVME,
No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_xSuf|ImmExt,
@@ -4209,33 +4205,24 @@ const template i386_optab[] =
{ "vmload", 0, 0xf01, 0xda, CpuSVME,
No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_xSuf|ImmExt,
{ 0 } },
- { "vmload", 1, 0xf01, 0xda, CpuSVME|CpuNo64,
- No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_xSuf|ImmExt,
- { Reg32 } },
- { "vmload", 1, 0xf01, 0xda, CpuSVME|Cpu64,
+ { "vmload", 1, 0xf01, 0xda, CpuSVME,
No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_xSuf|ImmExt|NoRex64,
- { Reg64 } },
+ { Reg32|Reg64 } },
{ "vmmcall", 0, 0xf01, 0xd9, CpuSVME,
No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_xSuf|ImmExt,
{ 0 } },
{ "vmrun", 0, 0xf01, 0xd8, CpuSVME,
No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_xSuf|ImmExt,
{ 0 } },
- { "vmrun", 1, 0xf01, 0xd8, CpuSVME|CpuNo64,
- No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_xSuf|ImmExt,
- { Reg32 } },
- { "vmrun", 1, 0xf01, 0xd8, CpuSVME|Cpu64,
+ { "vmrun", 1, 0xf01, 0xd8, CpuSVME,
No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_xSuf|ImmExt|NoRex64,
- { Reg64 } },
+ { Reg32|Reg64 } },
{ "vmsave", 0, 0xf01, 0xdb, CpuSVME,
No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_xSuf|ImmExt,
{ 0 } },
- { "vmsave", 1, 0xf01, 0xdb, CpuSVME|CpuNo64,
- No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_xSuf|ImmExt,
- { Reg32 } },
- { "vmsave", 1, 0xf01, 0xdb, CpuSVME|Cpu64,
+ { "vmsave", 1, 0xf01, 0xdb, CpuSVME,
No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_xSuf|ImmExt|NoRex64,
- { Reg64 } },
+ { Reg32|Reg64 } },
{ "movntsd", 2, 0xf20f2b, None, CpuSSE4a,
Modrm|IgnoreSize|No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_xSuf,
{ RegXMM,