| Message ID | 20240602100528.2135717-1-lixinyu20s@ict.ac.cn |
|---|---|
| State | New |
| Headers | show |
| Series | target/i386: fix memory opsize for Mov to/from Seg | expand |
On 6/2/24 12:05, lixinyu20s@ict.ac.cn wrote: > From: Xinyu Li <lixinyu@loongson.cn> > > 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 <lixinyu20s@ict.ac.cn> > --- > 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
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" <pbonzini@redhat.com>
> Sent Time: 2024-06-03 15:34:47 (Monday)
> To: lixinyu20s@ict.ac.cn, qemu-devel@nongnu.org
> Cc: richard.henderson@linaro.org, eduardo@habkost.net, "Xinyu Li" <lixinyu@loongson.cn>
> Subject: Re: [PATCH] target/i386: fix memory opsize for Mov to/from Seg
>
> On 6/2/24 12:05, lixinyu20s@ict.ac.cn wrote:
> > From: Xinyu Li <lixinyu@loongson.cn>
> >
> > 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 <lixinyu20s@ict.ac.cn>
> > ---
> > 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
</lixinyu20s@ict.ac.cn></lixinyu@loongson.cn></lixinyu@loongson.cn></pbonzini@redhat.com></sys></unistd.h></stdlib.h></stdint.h></stdio.h>
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), [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), [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, CPUState *cpu) s->override = -1; break; + case X86_SPECIAL_MovSeg: + if (decode.op[0].unit == X86_OP_INT && decode.op[0].has_ea) { + decode.op[0].ot = MO_16; + } else if (decode.op[1].unit == X86_OP_INT && decode.op[1].has_ea) { + decode.op[1].ot = MO_16; + } + default: break; } diff --git a/target/i386/tcg/decode-new.h b/target/i386/tcg/decode-new.h index 51ef0e621b..f29f5de7d1 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 to/from Seg is MO_16 */ + X86_SPECIAL_MovSeg, } X86InsnSpecial; /*