| Message ID | 1490608598-11197-2-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 27.03.17 at 11:56, <andrew.cooper3@citrix.com> wrote: > @@ -2332,9 +2333,9 @@ x86_decode_twobyte( > if ( vex.pfx == vex_f3 ) /* movq xmm/m64,xmm */ > { > case X86EMUL_OPC_VEX_F3(0, 0x7e): /* vmovq xmm/m64,xmm */ > - state->desc = DstImplicit | SrcMem | Mov; > + state->desc = DstImplicit | SrcMem | TwoOp; Why? This is a move after all. > @@ -2374,11 +2375,25 @@ x86_decode_twobyte( > case X86EMUL_OPC_VEX_66(0, 0xc4): /* vpinsrw */ > state->desc = DstReg | SrcMem16; > break; > + > + case 0xf0: > + ctxt->opcode |= MASK_INSR(vex.pfx, X86EMUL_OPC_PFX_MASK); > + if ( vex.pfx == vex_f2 ) /* lddqu mem,xmm */ > + { > + /* fall through */ > + case X86EMUL_OPC_VEX_F2(0, 0xf0): /* vlddqu mem,{x,y}mm */ > + state->desc = DstImplicit | SrcMem | TwoOp; I'd prefer it to be Mov here too, as the insn is a move even if its name doesn't say so. Jan
On 27/03/17 12:24, Jan Beulich wrote: >>>> On 27.03.17 at 11:56, <andrew.cooper3@citrix.com> wrote: >> @@ -2332,9 +2333,9 @@ x86_decode_twobyte( >> if ( vex.pfx == vex_f3 ) /* movq xmm/m64,xmm */ >> { >> case X86EMUL_OPC_VEX_F3(0, 0x7e): /* vmovq xmm/m64,xmm */ >> - state->desc = DstImplicit | SrcMem | Mov; >> + state->desc = DstImplicit | SrcMem | TwoOp; > Why? This is a move after all. > >> @@ -2374,11 +2375,25 @@ x86_decode_twobyte( >> case X86EMUL_OPC_VEX_66(0, 0xc4): /* vpinsrw */ >> state->desc = DstReg | SrcMem16; >> break; >> + >> + case 0xf0: >> + ctxt->opcode |= MASK_INSR(vex.pfx, X86EMUL_OPC_PFX_MASK); >> + if ( vex.pfx == vex_f2 ) /* lddqu mem,xmm */ >> + { >> + /* fall through */ >> + case X86EMUL_OPC_VEX_F2(0, 0xf0): /* vlddqu mem,{x,y}mm */ >> + state->desc = DstImplicit | SrcMem | TwoOp; > I'd prefer it to be Mov here too, as the insn is a move even if its > name doesn't say so. The fact that TwoOp and Mov are the same constant is confusing (and why I didn't particularly like its introduction in the first place), especially in this context where TwoOp is the more important semantic piece of information. ~Andrew
>>> On 27.03.17 at 14:10, <andrew.cooper3@citrix.com> wrote: > On 27/03/17 12:24, Jan Beulich wrote: >>>>> On 27.03.17 at 11:56, <andrew.cooper3@citrix.com> wrote: >>> @@ -2332,9 +2333,9 @@ x86_decode_twobyte( >>> if ( vex.pfx == vex_f3 ) /* movq xmm/m64,xmm */ >>> { >>> case X86EMUL_OPC_VEX_F3(0, 0x7e): /* vmovq xmm/m64,xmm */ >>> - state->desc = DstImplicit | SrcMem | Mov; >>> + state->desc = DstImplicit | SrcMem | TwoOp; >> Why? This is a move after all. >> >>> @@ -2374,11 +2375,25 @@ x86_decode_twobyte( >>> case X86EMUL_OPC_VEX_66(0, 0xc4): /* vpinsrw */ >>> state->desc = DstReg | SrcMem16; >>> break; >>> + >>> + case 0xf0: >>> + ctxt->opcode |= MASK_INSR(vex.pfx, X86EMUL_OPC_PFX_MASK); >>> + if ( vex.pfx == vex_f2 ) /* lddqu mem,xmm */ >>> + { >>> + /* fall through */ >>> + case X86EMUL_OPC_VEX_F2(0, 0xf0): /* vlddqu mem,{x,y}mm */ >>> + state->desc = DstImplicit | SrcMem | TwoOp; >> I'd prefer it to be Mov here too, as the insn is a move even if its >> name doesn't say so. > > The fact that TwoOp and Mov are the same constant is confusing (and why > I didn't particularly like its introduction in the first place), > especially in this context where TwoOp is the more important semantic > piece of information. Well, okay, if we consider that there aren't any real RMW SIMD insns (albeit for now we emulate a few as such), I can buy this argument, so Acked-by: Jan Beulich <jbeulich@suse.com> Jan
diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c index bb67be6..497cc77 100644 --- a/xen/arch/x86/x86_emulate/x86_emulate.c +++ b/xen/arch/x86/x86_emulate/x86_emulate.c @@ -2310,7 +2310,8 @@ x86_decode_twobyte( case 0x7f: case 0xc2 ... 0xc3: case 0xc5 ... 0xc6: - case 0xd0 ... 0xfe: + case 0xd0 ... 0xef: + case 0xf1 ... 0xfe: ctxt->opcode |= MASK_INSR(vex.pfx, X86EMUL_OPC_PFX_MASK); break; @@ -2332,9 +2333,9 @@ x86_decode_twobyte( if ( vex.pfx == vex_f3 ) /* movq xmm/m64,xmm */ { case X86EMUL_OPC_VEX_F3(0, 0x7e): /* vmovq xmm/m64,xmm */ - state->desc = DstImplicit | SrcMem | Mov; + state->desc = DstImplicit | SrcMem | TwoOp; state->simd_size = simd_other; - /* Avoid the state->desc adjustment below. */ + /* Avoid the state->desc clobbering of TwoOp below. */ return X86EMUL_OKAY; } break; @@ -2374,11 +2375,25 @@ x86_decode_twobyte( case X86EMUL_OPC_VEX_66(0, 0xc4): /* vpinsrw */ state->desc = DstReg | SrcMem16; break; + + case 0xf0: + ctxt->opcode |= MASK_INSR(vex.pfx, X86EMUL_OPC_PFX_MASK); + if ( vex.pfx == vex_f2 ) /* lddqu mem,xmm */ + { + /* fall through */ + case X86EMUL_OPC_VEX_F2(0, 0xf0): /* vlddqu mem,{x,y}mm */ + state->desc = DstImplicit | SrcMem | TwoOp; + state->simd_size = simd_other; + /* Avoid the state->desc clobbering of TwoOp below. */ + return X86EMUL_OKAY; + } + break; } /* * Scalar forms of most VEX-encoded TwoOp instructions have - * three operands. + * three operands. Those which do really have two operands + * should have exited earlier. */ if ( state->simd_size && vex.opcx && (vex.pfx & VEX_PREFIX_SCALAR_MASK) )
vlddqu is encoded with 0xf2 which causes it to fall into the Scalar general case in x86_decode_twobyte(). However, it really does have just two operands, so must remain TwoOp AFL discovered that the instruction c5 5b f0 3c e5 95 0a cd 63 was considered valid despite it being a two operand instruction and VEX.vvvv having the value 11. The resulting use in a stub yielded #UD. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> From manually decoding the instruciton, I believe Xen's interpretation of disp32(none*8) is correct. binutils 2.25 (Debian Jessie) yields: 0: c5 5b f0 3c e5 95 0a vlddqu 0x63cd0a95(,%riz,8),%xmm15 7: cd 63 where it has accounted for disp32 in its decode of instruction, but failed to properly move its instruction pointer on. Intel XED OTOH simply gives up with: ERROR: GENERAL_ERROR Could not decode at offset: 0x0 PC: 0x0: [C55BF03CE5950ACD63000000000000] --- xen/arch/x86/x86_emulate/x86_emulate.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-)