The Intel manual states, "Move lower 16 bits of r/m64 to segment register,"
which is somewhat ambiguous. Therefore, I have written the following test to
verify this.
#include <stdio.h>
#include <stdint.h>
#include <stdlib.h>
#include <unistd.h>
#include <sys mman.h="">
int main (int argc, char** argv) {
uint16_t gs;
int ps = getpagesize();
__asm__("mov %%gs, %0" : "=r" (gs));
void* p = mmap(NULL, ps * 2, PROT_NONE, MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
if (p == MAP_FAILED) {
perror("mmap");
abort();
}
if (mprotect(p, ps, PROT_READ | PROT_WRITE) != 0) {
perror("mprotect");
abort();
}
uint16_t* page_bnd = (uint16_t*)(p + ps - 2);
*page_bnd = gs;
__asm__ volatile ("mov (%0), %%gs" :: "r" (page_bnd));
__asm__ volatile (".byte 0x48 \n\t mov (%0), %%gs" :: "r" (page_bnd));
return 0;
}
The loading of a 2-byte segment at the page boundary did not trigger an
exception,
indicating that the processor actually performed only a 2-byte load.
Additionally, the previous translation also only involved a two-byte load.
case 0x8e: /* mov seg, Gv */
modrm = x86_ldub_code(env, s);
reg = (modrm >> 3) & 7;
if (reg >= 6 || reg == R_CS)
goto illegal_op;
gen_ldst_modrm(env, s, modrm, MO_16, OR_TMP0, 0);
gen_movl_seg_T0(s, reg);
break;
Therefore, a two-byte load seems reasonable and should not cause any errors.
Thank you!
Xinyu Li
> -----Original Messages-----
> From: "Paolo Bonzini" <[email protected]>
> Sent Time: 2024-06-03 15:34:47 (Monday)
> To: [email protected], [email protected]
> Cc: [email protected], [email protected], "Xinyu Li"
<[email protected]>
> Subject: Re: [PATCH] target/i386: fix memory opsize for Mov to/from Seg
>
> On 6/2/24 12:05, [email protected] wrote:
> > From: Xinyu Li <[email protected]>
> >
> > This commit fixes an issue with MOV instructions (0x8C and 0x8E)
> > involving segment registers by explicitly setting the memory operand
> > size to 16 bits. It introduces a new flag X86_SPECIAL_MovSeg to handle
> > this specification correctly.
> >
> > Signed-off-by: Xinyu Li <[email protected]>
> > ---
> > target/i386/tcg/decode-new.c.inc | 12 ++++++++++--
> > target/i386/tcg/decode-new.h | 2 ++
> > 2 files changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/target/i386/tcg/decode-new.c.inc
b/target/i386/tcg/decode-new.c.inc
> > index 0ec849b003..ab7dbb45f9 100644
> > --- a/target/i386/tcg/decode-new.c.inc
> > +++ b/target/i386/tcg/decode-new.c.inc
> > @@ -202,6 +202,7 @@
> > #define avx_movx .special = X86_SPECIAL_AVXExtMov,
> > #define sextT0 .special = X86_SPECIAL_SExtT0,
> > #define zextT0 .special = X86_SPECIAL_ZExtT0,
> > +#define movseg .special = X86_SPECIAL_MovSeg,
> >
> > #define vex1 .vex_class = 1,
> > #define vex1_rep3 .vex_class = 1, .vex_special = X86_VEX_REPScalar,
> > @@ -1576,9 +1577,9 @@ static const X86OpEntry opcodes_root[256] = {
> > [0x89] = X86_OP_ENTRY3(MOV, E,v, G,v, None, None),
> > [0x8A] = X86_OP_ENTRY3(MOV, G,b, E,b, None, None),
> > [0x8B] = X86_OP_ENTRY3(MOV, G,v, E,v, None, None),
> > - [0x8C] = X86_OP_ENTRY3(MOV, E,v, S,w, None, None),
> > + [0x8C] = X86_OP_ENTRY3(MOV, E,v, S,w, None, None, movseg),
>
> Indeed this was a mistake, thanks! The manual doesn't list it
> in Table A-2, but the description of the instruction mentions
> "r16/r32/m16" which is what you are implementing it.
>
> > [0x8D] = X86_OP_ENTRY3(LEA, G,v, M,v, None, None, noseg),
> > - [0x8E] = X86_OP_ENTRY3(MOV, S,w, E,v, None, None),
> > + [0x8E] = X86_OP_ENTRY3(MOV, S,w, E,v, None, None, movseg),
>
> This is also wrong, but here the manual is correct. It can be
> written as "S,w, E,w" instead without special casing it.
>
> Therefore the new X86InsnSpecial only needs to cover op[0]...
>
> > + /* Memory operand size of Mov to/from Seg is MO_16 */
> > + X86_SPECIAL_MovSeg,
>
> ... and I would call it Op0_Mw, for consistency with other similar
> entries of X86InsnSpecial.
>
> So I queued your patch with a few small changes:
>
> diff --git a/target/i386/tcg/decode-new.h b/target/i386/tcg/decode-new.h
> index 51ef0e621b9..d6335597a13 100644
> --- a/target/i386/tcg/decode-new.h
> +++ b/target/i386/tcg/decode-new.h
> @@ -203,6 +203,8 @@ typedef enum X86InsnSpecial {
> /* When loaded into s->T0, register operand 1 is zero/sign
extended. */
> X86_SPECIAL_SExtT0,
> X86_SPECIAL_ZExtT0,
> + /* Memory operand size of MOV from segment register is MO_16 */
> + X86_SPECIAL_Op0_Mw,
> } X86InsnSpecial;
>
> /*
> diff --git a/target/i386/tcg/decode-new.c.inc
b/target/i386/tcg/decode-new.c.inc
> index 0ec849b0035..599047df94a 100644
> --- a/target/i386/tcg/decode-new.c.inc
> +++ b/target/i386/tcg/decode-new.c.inc
> @@ -202,6 +202,7 @@
> #define avx_movx .special = X86_SPECIAL_AVXExtMov,
> #define sextT0 .special = X86_SPECIAL_SExtT0,
> #define zextT0 .special = X86_SPECIAL_ZExtT0,
> +#define op0_Mw .special = X86_SPECIAL_Op0_Mw,
>
> #define vex1 .vex_class = 1,
> #define vex1_rep3 .vex_class = 1, .vex_special = X86_VEX_REPScalar,
> @@ -1576,9 +1577,9 @@ static const X86OpEntry opcodes_root[256] = {
> [0x89] = X86_OP_ENTRY3(MOV, E,v, G,v, None, None),
> [0x8A] = X86_OP_ENTRY3(MOV, G,b, E,b, None, None),
> [0x8B] = X86_OP_ENTRY3(MOV, G,v, E,v, None, None),
> - [0x8C] = X86_OP_ENTRY3(MOV, E,v, S,w, None, None),
> + [0x8C] = X86_OP_ENTRY3(MOV, E,v, S,w, None, None, op0_Mw),
> [0x8D] = X86_OP_ENTRY3(LEA, G,v, M,v, None, None, noseg),
> - [0x8E] = X86_OP_ENTRY3(MOV, S,w, E,v, None, None),
> + [0x8E] = X86_OP_ENTRY3(MOV, S,w, E,w, None, None),
> [0x8F] = X86_OP_GROUPw(group1A, E,v),
>
> [0x98] = X86_OP_ENTRY1(CBW, 0,v), /* rAX */
> @@ -2514,6 +2515,13 @@ static void disas_insn(DisasContext *s,
> s->override = -1;
> break;
>
> + case X86_SPECIAL_Op0_Mw:
> + assert(decode.op[0].unit == X86_OP_INT);
> + if (decode.op[0].has_ea) {
> + decode.op[0].ot = MO_16;
> + }
> + break;
> +
> default:
> break;
> }
>
> Thank you very much!
>
> Paolo
</[email protected]></[email protected]></[email protected]></[email protected]></sys></unistd.h></stdlib.h></stdint.h></stdio.h>
