>>> On 20.02.17 at 17:05, <andrew.coop...@citrix.com> wrote: > On 15/02/17 11:14, Jan Beulich wrote: >> @@ -2207,12 +2231,12 @@ x86_decode_twobyte( >> switch ( modrm_reg & 7 ) >> { >> case 2: /* {,v}ldmxcsr */ >> - state->desc = DstImplicit | SrcMem | ModRM | Mov; >> + state->desc = DstImplicit | SrcMem | Mov; >> op_bytes = 4; >> break; >> >> case 3: /* {,v}stmxcsr */ >> - state->desc = DstMem | SrcImplicit | ModRM | Mov; >> + state->desc = DstMem | SrcImplicit | Mov; >> op_bytes = 4; >> break; >> } > > Shouldn't this be folded into patch 7?
I don't think so - the re-purposing of the ModRM bit starts only in the patch here. >> @@ -2571,6 +2595,25 @@ x86_decode( >> } >> break; >> } >> + break; >> + >> + case vex_0f38: >> + d = ext0f38_table[b].to_memory ? DstMem | SrcReg >> + : DstReg | SrcMem; >> + if ( ext0f38_table[b].two_op ) >> + d |= TwoOp; >> + if ( ext0f38_table[b].vsib ) >> + d |= vSIB; > > What prevents vSIB becoming set for a non-vex encoded 0f38 instruction? Coding discipline. The intention here is to have this bit available for future use in the ext0f3a_table[] case as well as, once they need adding, any XOP ones. At that point consumers of the bit would need to become sensitive to the extension space an insn is in, but for now we can keep the code simple in this regard. >> @@ -7382,7 +7438,7 @@ x86_insn_modrm(const struct x86_emulate_ >> { >> check_state(state); >> >> - if ( !(state->desc & ModRM) ) >> + if ( state->modrm_mod > 3 ) > > Speaking of, there is still a bug with some existing x86_insn_modrm() > callsites. > > Consider this a ping on the "Re: [Xen-devel] [PATCH] x86/svm: Adjust > ModRM Mode check in is_invlpg()" thread. Not sure what you mean here - I did provide my ack to your patch, and even made the condition on it conditional (with the alternative of reverting). Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel