[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 2/2] target/i386: Raise #GP on unaligned m128 accesses whe
|
From: |
Paolo Bonzini |
|
Subject: |
Re: [PATCH v3 2/2] target/i386: Raise #GP on unaligned m128 accesses when required. |
|
Date: |
Mon, 19 Sep 2022 19:34:34 +0200 |
Hi,
I will merge this patch already, but with simpler code that doesn't
look at PREFIX_VEX. The changes to the helpers and the addition of the
aligned argument help with the new decoder as well, and I will build
on top of them in the next submission of the AVX patches.
Thanks!
Paolo
On Sat, Sep 17, 2022 at 4:15 AM Ricky Zhou <ricky@rzhou.org> wrote:
>
> Checking back on the status of patch, I noticed that there are some
> exciting patches out for AVX support that may conflict with this,
> though I see that they are still in the RFC phase:
> 20220911230418.340941-1-pbonzini@redhat.com/">https://patchew.org/QEMU/20220911230418.340941-1-pbonzini@redhat.com/
>
> I'm not sure how far away AVX support is from being merged, but do let
> me know if there's any preference re applying this change vs. waiting
> to rebase on top the AVX support changes, etc.
>
> Thanks!
> Ricky
>
> On Mon, Aug 29, 2022 at 8:48 PM Ricky Zhou <ricky@rzhou.org> wrote:
> >
> > Many instructions which load/store 128-bit values are supposed to
> > raise #GP when the memory operand isn't 16-byte aligned. This includes:
> > - Instructions explicitly requiring memory alignment (Exceptions Type 1
> > in the "AVX and SSE Instruction Exception Specification" section of
> > the SDM)
> > - Legacy SSE instructions that load/store 128-bit values (Exceptions
> > Types 2 and 4).
> >
> > This change sets MO_ALIGN_16 on 128-bit memory accesses that require
> > 16-byte alignment. It adds cpu_record_sigbus and cpu_do_unaligned_access
> > hooks that simulate a #GP exception in qemu-user and qemu-system,
> > respectively.
> >
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/217
> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> > Signed-off-by: Ricky Zhou <ricky@rzhou.org>
> > ---
> > target/i386/tcg/excp_helper.c | 13 ++++++++
> > target/i386/tcg/helper-tcg.h | 28 ++++++++++-------
> > target/i386/tcg/sysemu/excp_helper.c | 8 +++++
> > target/i386/tcg/tcg-cpu.c | 2 ++
> > target/i386/tcg/translate.c | 45 +++++++++++++++++-----------
> > target/i386/tcg/user/excp_helper.c | 7 +++++
> > 6 files changed, 74 insertions(+), 29 deletions(-)
> >
> > diff --git a/target/i386/tcg/excp_helper.c b/target/i386/tcg/excp_helper.c
> > index c1ffa1c0ef..7c3c8dc7fe 100644
> > --- a/target/i386/tcg/excp_helper.c
> > +++ b/target/i386/tcg/excp_helper.c
> > @@ -140,3 +140,16 @@ G_NORETURN void raise_exception_ra(CPUX86State *env,
> > int exception_index,
> > {
> > raise_interrupt2(env, exception_index, 0, 0, 0, retaddr);
> > }
> > +
> > +G_NORETURN void handle_unaligned_access(CPUX86State *env, vaddr vaddr,
> > + MMUAccessType access_type,
> > + uintptr_t retaddr)
> > +{
> > + /*
> > + * Unaligned accesses are currently only triggered by SSE/AVX
> > + * instructions that impose alignment requirements on memory
> > + * operands. These instructions raise #GP(0) upon accessing an
> > + * unaligned address.
> > + */
> > + raise_exception_ra(env, EXCP0D_GPF, retaddr);
> > +}
> > diff --git a/target/i386/tcg/helper-tcg.h b/target/i386/tcg/helper-tcg.h
> > index 34167e2e29..cd1723389a 100644
> > --- a/target/i386/tcg/helper-tcg.h
> > +++ b/target/i386/tcg/helper-tcg.h
> > @@ -42,17 +42,6 @@ void x86_cpu_do_interrupt(CPUState *cpu);
> > bool x86_cpu_exec_interrupt(CPUState *cpu, int int_req);
> > #endif
> >
> > -/* helper.c */
> > -#ifdef CONFIG_USER_ONLY
> > -void x86_cpu_record_sigsegv(CPUState *cs, vaddr addr,
> > - MMUAccessType access_type,
> > - bool maperr, uintptr_t ra);
> > -#else
> > -bool x86_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
> > - MMUAccessType access_type, int mmu_idx,
> > - bool probe, uintptr_t retaddr);
> > -#endif
> > -
> > void breakpoint_handler(CPUState *cs);
> >
> > /* n must be a constant to be efficient */
> > @@ -78,6 +67,23 @@ G_NORETURN void raise_exception_err_ra(CPUX86State *env,
> > int exception_index,
> > int error_code, uintptr_t retaddr);
> > G_NORETURN void raise_interrupt(CPUX86State *nenv, int intno, int is_int,
> > int error_code, int next_eip_addend);
> > +G_NORETURN void handle_unaligned_access(CPUX86State *env, vaddr vaddr,
> > + MMUAccessType access_type,
> > + uintptr_t retaddr);
> > +#ifdef CONFIG_USER_ONLY
> > +void x86_cpu_record_sigsegv(CPUState *cs, vaddr addr,
> > + MMUAccessType access_type,
> > + bool maperr, uintptr_t ra);
> > +void x86_cpu_record_sigbus(CPUState *cs, vaddr addr,
> > + MMUAccessType access_type, uintptr_t ra);
> > +#else
> > +bool x86_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
> > + MMUAccessType access_type, int mmu_idx,
> > + bool probe, uintptr_t retaddr);
> > +G_NORETURN void x86_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
> > + MMUAccessType access_type,
> > + int mmu_idx, uintptr_t
> > retaddr);
> > +#endif
> >
> > /* cc_helper.c */
> > extern const uint8_t parity_table[256];
> > diff --git a/target/i386/tcg/sysemu/excp_helper.c
> > b/target/i386/tcg/sysemu/excp_helper.c
> > index 48feba7e75..796dc2a1f3 100644
> > --- a/target/i386/tcg/sysemu/excp_helper.c
> > +++ b/target/i386/tcg/sysemu/excp_helper.c
> > @@ -439,3 +439,11 @@ bool x86_cpu_tlb_fill(CPUState *cs, vaddr addr, int
> > size,
> > }
> > return true;
> > }
> > +
> > +G_NORETURN void x86_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
> > + MMUAccessType access_type,
> > + int mmu_idx, uintptr_t retaddr)
> > +{
> > + X86CPU *cpu = X86_CPU(cs);
> > + handle_unaligned_access(&cpu->env, vaddr, access_type, retaddr);
> > +}
> > diff --git a/target/i386/tcg/tcg-cpu.c b/target/i386/tcg/tcg-cpu.c
> > index 6fdfdf9598..d3c2b8fb49 100644
> > --- a/target/i386/tcg/tcg-cpu.c
> > +++ b/target/i386/tcg/tcg-cpu.c
> > @@ -75,10 +75,12 @@ static const struct TCGCPUOps x86_tcg_ops = {
> > #ifdef CONFIG_USER_ONLY
> > .fake_user_interrupt = x86_cpu_do_interrupt,
> > .record_sigsegv = x86_cpu_record_sigsegv,
> > + .record_sigbus = x86_cpu_record_sigbus,
> > #else
> > .tlb_fill = x86_cpu_tlb_fill,
> > .do_interrupt = x86_cpu_do_interrupt,
> > .cpu_exec_interrupt = x86_cpu_exec_interrupt,
> > + .do_unaligned_access = x86_cpu_do_unaligned_access,
> > .debug_excp_handler = breakpoint_handler,
> > .debug_check_breakpoint = x86_debug_check_breakpoint,
> > #endif /* !CONFIG_USER_ONLY */
> > diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
> > index 3ba5f76156..29104a49b9 100644
> > --- a/target/i386/tcg/translate.c
> > +++ b/target/i386/tcg/translate.c
> > @@ -2731,21 +2731,23 @@ static inline void gen_stq_env_A0(DisasContext *s,
> > int offset)
> > tcg_gen_qemu_st_i64(s->tmp1_i64, s->A0, s->mem_index, MO_LEUQ);
> > }
> >
> > -static inline void gen_ldo_env_A0(DisasContext *s, int offset)
> > +static inline void gen_ldo_env_A0(DisasContext *s, int offset, bool align)
> > {
> > int mem_index = s->mem_index;
> > - tcg_gen_qemu_ld_i64(s->tmp1_i64, s->A0, mem_index, MO_LEUQ);
> > + tcg_gen_qemu_ld_i64(s->tmp1_i64, s->A0, mem_index,
> > + MO_LEUQ | (align ? MO_ALIGN_16 : 0));
> > tcg_gen_st_i64(s->tmp1_i64, cpu_env, offset + offsetof(ZMMReg,
> > ZMM_Q(0)));
> > tcg_gen_addi_tl(s->tmp0, s->A0, 8);
> > tcg_gen_qemu_ld_i64(s->tmp1_i64, s->tmp0, mem_index, MO_LEUQ);
> > tcg_gen_st_i64(s->tmp1_i64, cpu_env, offset + offsetof(ZMMReg,
> > ZMM_Q(1)));
> > }
> >
> > -static inline void gen_sto_env_A0(DisasContext *s, int offset)
> > +static inline void gen_sto_env_A0(DisasContext *s, int offset, bool align)
> > {
> > int mem_index = s->mem_index;
> > tcg_gen_ld_i64(s->tmp1_i64, cpu_env, offset + offsetof(ZMMReg,
> > ZMM_Q(0)));
> > - tcg_gen_qemu_st_i64(s->tmp1_i64, s->A0, mem_index, MO_LEUQ);
> > + tcg_gen_qemu_st_i64(s->tmp1_i64, s->A0, mem_index,
> > + MO_LEUQ | (align ? MO_ALIGN_16 : 0));
> > tcg_gen_addi_tl(s->tmp0, s->A0, 8);
> > tcg_gen_ld_i64(s->tmp1_i64, cpu_env, offset + offsetof(ZMMReg,
> > ZMM_Q(1)));
> > tcg_gen_qemu_st_i64(s->tmp1_i64, s->tmp0, mem_index, MO_LEUQ);
> > @@ -3054,7 +3056,7 @@ static const struct SSEOpHelper_epp
> > sse_op_table6[256] = {
> > [0x25] = SSE41_OP(pmovsxdq),
> > [0x28] = SSE41_OP(pmuldq),
> > [0x29] = SSE41_OP(pcmpeqq),
> > - [0x2a] = SSE41_SPECIAL, /* movntqda */
> > + [0x2a] = SSE41_SPECIAL, /* movntdqa */
> > [0x2b] = SSE41_OP(packusdw),
> > [0x30] = SSE41_OP(pmovzxbw),
> > [0x31] = SSE41_OP(pmovzxbd),
> > @@ -3194,17 +3196,17 @@ static void gen_sse(CPUX86State *env, DisasContext
> > *s, int b,
> > break;
> > case 0x1e7: /* movntdq */
> > case 0x02b: /* movntps */
> > - case 0x12b: /* movntps */
> > + case 0x12b: /* movntpd */
> > if (mod == 3)
> > goto illegal_op;
> > gen_lea_modrm(env, s, modrm);
> > - gen_sto_env_A0(s, offsetof(CPUX86State, xmm_regs[reg]));
> > + gen_sto_env_A0(s, offsetof(CPUX86State, xmm_regs[reg]), true);
> > break;
> > case 0x3f0: /* lddqu */
> > if (mod == 3)
> > goto illegal_op;
> > gen_lea_modrm(env, s, modrm);
> > - gen_ldo_env_A0(s, offsetof(CPUX86State, xmm_regs[reg]));
> > + gen_ldo_env_A0(s, offsetof(CPUX86State, xmm_regs[reg]), false);
> > break;
> > case 0x22b: /* movntss */
> > case 0x32b: /* movntsd */
> > @@ -3273,7 +3275,9 @@ static void gen_sse(CPUX86State *env, DisasContext
> > *s, int b,
> > case 0x26f: /* movdqu xmm, ea */
> > if (mod != 3) {
> > gen_lea_modrm(env, s, modrm);
> > - gen_ldo_env_A0(s, offsetof(CPUX86State, xmm_regs[reg]));
> > + gen_ldo_env_A0(s, offsetof(CPUX86State, xmm_regs[reg]),
> > + /* movaps, movapd, movdqa */
> > + b == 0x028 || b == 0x128 || b == 0x16f);
> > } else {
> > rm = (modrm & 7) | REX_B(s);
> > gen_op_movo(s, offsetof(CPUX86State, xmm_regs[reg]),
> > @@ -3331,7 +3335,8 @@ static void gen_sse(CPUX86State *env, DisasContext
> > *s, int b,
> > case 0x212: /* movsldup */
> > if (mod != 3) {
> > gen_lea_modrm(env, s, modrm);
> > - gen_ldo_env_A0(s, offsetof(CPUX86State, xmm_regs[reg]));
> > + gen_ldo_env_A0(s, offsetof(CPUX86State, xmm_regs[reg]),
> > + !(s->prefix & PREFIX_VEX));
> > } else {
> > rm = (modrm & 7) | REX_B(s);
> > gen_op_movl(s, offsetof(CPUX86State,
> > xmm_regs[reg].ZMM_L(0)),
> > @@ -3373,7 +3378,8 @@ static void gen_sse(CPUX86State *env, DisasContext
> > *s, int b,
> > case 0x216: /* movshdup */
> > if (mod != 3) {
> > gen_lea_modrm(env, s, modrm);
> > - gen_ldo_env_A0(s, offsetof(CPUX86State, xmm_regs[reg]));
> > + gen_ldo_env_A0(s, offsetof(CPUX86State, xmm_regs[reg]),
> > + !(s->prefix & PREFIX_VEX));
> > } else {
> > rm = (modrm & 7) | REX_B(s);
> > gen_op_movl(s, offsetof(CPUX86State,
> > xmm_regs[reg].ZMM_L(1)),
> > @@ -3465,7 +3471,9 @@ static void gen_sse(CPUX86State *env, DisasContext
> > *s, int b,
> > case 0x27f: /* movdqu ea, xmm */
> > if (mod != 3) {
> > gen_lea_modrm(env, s, modrm);
> > - gen_sto_env_A0(s, offsetof(CPUX86State, xmm_regs[reg]));
> > + gen_sto_env_A0(s, offsetof(CPUX86State, xmm_regs[reg]),
> > + /* movaps, movapd, movdqa */
> > + b == 0x029 || b == 0x129 || b == 0x17f);
> > } else {
> > rm = (modrm & 7) | REX_B(s);
> > gen_op_movo(s, offsetof(CPUX86State, xmm_regs[rm]),
> > @@ -3622,7 +3630,7 @@ static void gen_sse(CPUX86State *env, DisasContext
> > *s, int b,
> > gen_lea_modrm(env, s, modrm);
> > op2_offset = offsetof(CPUX86State,xmm_t0);
> > if (b1) {
> > - gen_ldo_env_A0(s, op2_offset);
> > + gen_ldo_env_A0(s, op2_offset, true);
> > } else {
> > gen_ldq_env_A0(s, op2_offset);
> > }
> > @@ -3810,11 +3818,12 @@ static void gen_sse(CPUX86State *env, DisasContext
> > *s, int b,
> > tcg_gen_st16_tl(s->tmp0, cpu_env, op2_offset +
> > offsetof(ZMMReg, ZMM_W(0)));
> > break;
> > - case 0x2a: /* movntqda */
> > - gen_ldo_env_A0(s, op1_offset);
> > + case 0x2a: /* movntdqa */
> > + gen_ldo_env_A0(s, op1_offset, true);
> > return;
> > default:
> > - gen_ldo_env_A0(s, op2_offset);
> > + gen_ldo_env_A0(s, op2_offset,
> > + !(s->prefix & PREFIX_VEX));
> > }
> > }
> > } else {
> > @@ -4355,7 +4364,7 @@ static void gen_sse(CPUX86State *env, DisasContext
> > *s, int b,
> > } else {
> > op2_offset = offsetof(CPUX86State,xmm_t0);
> > gen_lea_modrm(env, s, modrm);
> > - gen_ldo_env_A0(s, op2_offset);
> > + gen_ldo_env_A0(s, op2_offset, !(s->prefix &
> > PREFIX_VEX));
> > }
> > } else {
> > op1_offset = offsetof(CPUX86State,fpregs[reg].mmx);
> > @@ -4473,7 +4482,7 @@ static void gen_sse(CPUX86State *env, DisasContext
> > *s, int b,
> > break;
> > default:
> > /* 128 bit access */
> > - gen_ldo_env_A0(s, op2_offset);
> > + gen_ldo_env_A0(s, op2_offset, !(s->prefix &
> > PREFIX_VEX));
> > break;
> > }
> > } else {
> > diff --git a/target/i386/tcg/user/excp_helper.c
> > b/target/i386/tcg/user/excp_helper.c
> > index cd507e2a1b..b3bdb7831a 100644
> > --- a/target/i386/tcg/user/excp_helper.c
> > +++ b/target/i386/tcg/user/excp_helper.c
> > @@ -48,3 +48,10 @@ void x86_cpu_record_sigsegv(CPUState *cs, vaddr addr,
> >
> > cpu_loop_exit_restore(cs, ra);
> > }
> > +
> > +void x86_cpu_record_sigbus(CPUState *cs, vaddr addr,
> > + MMUAccessType access_type, uintptr_t ra)
> > +{
> > + X86CPU *cpu = X86_CPU(cs);
> > + handle_unaligned_access(&cpu->env, addr, access_type, ra);
> > +}
> > --
> > 2.37.2
> >
>