| Message ID | 20210212123622.15834-9-cfontana@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | i386 cleanup PART 2 | expand |
On 2/12/21 1:36 PM, Claudio Fontana wrote: > smm is only really useful for softmmu, split in two modules > around the CONFIG_USER_ONLY, in order to remove the ifdef > and use the build system instead. > > Signed-off-by: Claudio Fontana <cfontana@suse.de> > --- > target/i386/helper.h | 4 ++++ > target/i386/tcg/seg_helper.c | 2 ++ > target/i386/tcg/{ => softmmu}/smm_helper.c | 19 ++----------------- > target/i386/tcg/translate.c | 2 ++ > target/i386/tcg/meson.build | 1 - > target/i386/tcg/softmmu/meson.build | 1 + > 6 files changed, 11 insertions(+), 18 deletions(-) > rename target/i386/tcg/{ => softmmu}/smm_helper.c (98%) > > diff --git a/target/i386/helper.h b/target/i386/helper.h > index c2ae2f7e61..8ffda4cdc6 100644 > --- a/target/i386/helper.h > +++ b/target/i386/helper.h > @@ -70,7 +70,11 @@ DEF_HELPER_1(clac, void, env) > DEF_HELPER_1(stac, void, env) > DEF_HELPER_3(boundw, void, env, tl, int) > DEF_HELPER_3(boundl, void, env, tl, int) > + > +#ifndef CONFIG_USER_ONLY > DEF_HELPER_1(rsm, void, env) > +#endif /* !CONFIG_USER_ONLY */ > + > DEF_HELPER_2(into, void, env, int) > DEF_HELPER_2(cmpxchg8b_unlocked, void, env, tl) > DEF_HELPER_2(cmpxchg8b, void, env, tl) > diff --git a/target/i386/tcg/seg_helper.c b/target/i386/tcg/seg_helper.c > index 180d47f0e9..f0cb1bffe7 100644 > --- a/target/i386/tcg/seg_helper.c > +++ b/target/i386/tcg/seg_helper.c > @@ -1351,7 +1351,9 @@ bool x86_cpu_exec_interrupt(CPUState *cs, int interrupt_request) > case CPU_INTERRUPT_SMI: > cpu_svm_check_intercept_param(env, SVM_EXIT_SMI, 0, 0); > cs->interrupt_request &= ~CPU_INTERRUPT_SMI; > +#ifndef CONFIG_USER_ONLY > do_smm_enter(cpu); > +#endif > break; > case CPU_INTERRUPT_NMI: > cpu_svm_check_intercept_param(env, SVM_EXIT_NMI, 0, 0); > diff --git a/target/i386/tcg/smm_helper.c b/target/i386/tcg/softmmu/smm_helper.c > similarity index 98% > rename from target/i386/tcg/smm_helper.c > rename to target/i386/tcg/softmmu/smm_helper.c > index 62d027abd3..ee53b26629 100644 > --- a/target/i386/tcg/smm_helper.c > +++ b/target/i386/tcg/softmmu/smm_helper.c > @@ -1,5 +1,5 @@ > /* > - * x86 SMM helpers > + * x86 SMM helpers (softmmu-only) > * > * Copyright (c) 2003 Fabrice Bellard > * > @@ -18,27 +18,14 @@ > */ > > #include "qemu/osdep.h" > -#include "qemu/main-loop.h" > #include "cpu.h" > #include "exec/helper-proto.h" > #include "exec/log.h" > -#include "helper-tcg.h" > +#include "tcg/helper-tcg.h" > > > /* SMM support */ > > -#if defined(CONFIG_USER_ONLY) > - > -void do_smm_enter(X86CPU *cpu) > -{ > -} > - > -void helper_rsm(CPUX86State *env) > -{ > -} > - > -#else > - > #ifdef TARGET_X86_64 > #define SMM_REVISION_ID 0x00020064 > #else > @@ -330,5 +317,3 @@ void helper_rsm(CPUX86State *env) > qemu_log_mask(CPU_LOG_INT, "SMM: after RSM\n"); > log_cpu_state_mask(CPU_LOG_INT, CPU(cpu), CPU_DUMP_CCOP); > } > - > -#endif /* !CONFIG_USER_ONLY */ > diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c > index af1faf9342..5075ac4830 100644 > --- a/target/i386/tcg/translate.c > +++ b/target/i386/tcg/translate.c > @@ -8321,7 +8321,9 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu) > goto illegal_op; > gen_update_cc_op(s); > gen_jmp_im(s, s->pc - s->cs_base); > +#ifndef CONFIG_USER_ONLY > gen_helper_rsm(cpu_env); > +#endif /* CONFIG_USER_ONLY */ > gen_eob(s); > break; Hello Alex, this is something I wanted to bring in the foreground: while before we were generating an empty helper call for CONFIG_USER_ONLY, now we are not generating anything. > case 0x1b8: /* SSE4.2 popcnt */ > diff --git a/target/i386/tcg/meson.build b/target/i386/tcg/meson.build > index 68fa0c3187..ec5daa1edc 100644 > --- a/target/i386/tcg/meson.build > +++ b/target/i386/tcg/meson.build > @@ -8,7 +8,6 @@ i386_ss.add(when: 'CONFIG_TCG', if_true: files( > 'misc_helper.c', > 'mpx_helper.c', > 'seg_helper.c', > - 'smm_helper.c', > 'svm_helper.c', > 'tcg-cpu.c', > 'translate.c'), if_false: files('tcg-stub.c')) > diff --git a/target/i386/tcg/softmmu/meson.build b/target/i386/tcg/softmmu/meson.build > index 4ab30cc32e..35ba16dc3d 100644 > --- a/target/i386/tcg/softmmu/meson.build > +++ b/target/i386/tcg/softmmu/meson.build > @@ -1,3 +1,4 @@ > i386_softmmu_ss.add(when: ['CONFIG_TCG', 'CONFIG_SOFTMMU'], if_true: files( > 'tcg-cpu.c', > + 'smm_helper.c', > )) >
Claudio Fontana <cfontana@suse.de> writes: > On 2/12/21 1:36 PM, Claudio Fontana wrote: >> smm is only really useful for softmmu, split in two modules >> around the CONFIG_USER_ONLY, in order to remove the ifdef >> and use the build system instead. >> >> Signed-off-by: Claudio Fontana <cfontana@suse.de> >> --- >> target/i386/helper.h | 4 ++++ >> target/i386/tcg/seg_helper.c | 2 ++ >> target/i386/tcg/{ => softmmu}/smm_helper.c | 19 ++----------------- >> target/i386/tcg/translate.c | 2 ++ >> target/i386/tcg/meson.build | 1 - >> target/i386/tcg/softmmu/meson.build | 1 + >> 6 files changed, 11 insertions(+), 18 deletions(-) >> rename target/i386/tcg/{ => softmmu}/smm_helper.c (98%) >> >> diff --git a/target/i386/helper.h b/target/i386/helper.h >> index c2ae2f7e61..8ffda4cdc6 100644 >> --- a/target/i386/helper.h >> +++ b/target/i386/helper.h >> @@ -70,7 +70,11 @@ DEF_HELPER_1(clac, void, env) >> DEF_HELPER_1(stac, void, env) >> DEF_HELPER_3(boundw, void, env, tl, int) >> DEF_HELPER_3(boundl, void, env, tl, int) >> + >> +#ifndef CONFIG_USER_ONLY >> DEF_HELPER_1(rsm, void, env) >> +#endif /* !CONFIG_USER_ONLY */ >> + >> DEF_HELPER_2(into, void, env, int) >> DEF_HELPER_2(cmpxchg8b_unlocked, void, env, tl) >> DEF_HELPER_2(cmpxchg8b, void, env, tl) >> diff --git a/target/i386/tcg/seg_helper.c b/target/i386/tcg/seg_helper.c >> index 180d47f0e9..f0cb1bffe7 100644 >> --- a/target/i386/tcg/seg_helper.c >> +++ b/target/i386/tcg/seg_helper.c >> @@ -1351,7 +1351,9 @@ bool x86_cpu_exec_interrupt(CPUState *cs, int interrupt_request) >> case CPU_INTERRUPT_SMI: >> cpu_svm_check_intercept_param(env, SVM_EXIT_SMI, 0, 0); >> cs->interrupt_request &= ~CPU_INTERRUPT_SMI; >> +#ifndef CONFIG_USER_ONLY >> do_smm_enter(cpu); >> +#endif >> break; >> case CPU_INTERRUPT_NMI: >> cpu_svm_check_intercept_param(env, SVM_EXIT_NMI, 0, 0); >> diff --git a/target/i386/tcg/smm_helper.c b/target/i386/tcg/softmmu/smm_helper.c >> similarity index 98% >> rename from target/i386/tcg/smm_helper.c >> rename to target/i386/tcg/softmmu/smm_helper.c >> index 62d027abd3..ee53b26629 100644 >> --- a/target/i386/tcg/smm_helper.c >> +++ b/target/i386/tcg/softmmu/smm_helper.c >> @@ -1,5 +1,5 @@ >> /* >> - * x86 SMM helpers >> + * x86 SMM helpers (softmmu-only) >> * >> * Copyright (c) 2003 Fabrice Bellard >> * >> @@ -18,27 +18,14 @@ >> */ >> >> #include "qemu/osdep.h" >> -#include "qemu/main-loop.h" >> #include "cpu.h" >> #include "exec/helper-proto.h" >> #include "exec/log.h" >> -#include "helper-tcg.h" >> +#include "tcg/helper-tcg.h" >> >> >> /* SMM support */ >> >> -#if defined(CONFIG_USER_ONLY) >> - >> -void do_smm_enter(X86CPU *cpu) >> -{ >> -} >> - >> -void helper_rsm(CPUX86State *env) >> -{ >> -} >> - >> -#else >> - >> #ifdef TARGET_X86_64 >> #define SMM_REVISION_ID 0x00020064 >> #else >> @@ -330,5 +317,3 @@ void helper_rsm(CPUX86State *env) >> qemu_log_mask(CPU_LOG_INT, "SMM: after RSM\n"); >> log_cpu_state_mask(CPU_LOG_INT, CPU(cpu), CPU_DUMP_CCOP); >> } >> - >> -#endif /* !CONFIG_USER_ONLY */ >> diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c >> index af1faf9342..5075ac4830 100644 >> --- a/target/i386/tcg/translate.c >> +++ b/target/i386/tcg/translate.c >> @@ -8321,7 +8321,9 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu) >> goto illegal_op; >> gen_update_cc_op(s); >> gen_jmp_im(s, s->pc - s->cs_base); >> +#ifndef CONFIG_USER_ONLY >> gen_helper_rsm(cpu_env); >> +#endif /* CONFIG_USER_ONLY */ >> gen_eob(s); >> break; > > Hello Alex, > > this is something I wanted to bring in the foreground: > > while before we were generating an empty helper call for CONFIG_USER_ONLY, > now we are not generating anything. Surely that says we only generate the helper call when we are not CONFIG_USER_ONLY? > > > >> case 0x1b8: /* SSE4.2 popcnt */ >> diff --git a/target/i386/tcg/meson.build b/target/i386/tcg/meson.build >> index 68fa0c3187..ec5daa1edc 100644 >> --- a/target/i386/tcg/meson.build >> +++ b/target/i386/tcg/meson.build >> @@ -8,7 +8,6 @@ i386_ss.add(when: 'CONFIG_TCG', if_true: files( >> 'misc_helper.c', >> 'mpx_helper.c', >> 'seg_helper.c', >> - 'smm_helper.c', >> 'svm_helper.c', >> 'tcg-cpu.c', >> 'translate.c'), if_false: files('tcg-stub.c')) >> diff --git a/target/i386/tcg/softmmu/meson.build b/target/i386/tcg/softmmu/meson.build >> index 4ab30cc32e..35ba16dc3d 100644 >> --- a/target/i386/tcg/softmmu/meson.build >> +++ b/target/i386/tcg/softmmu/meson.build >> @@ -1,3 +1,4 @@ >> i386_softmmu_ss.add(when: ['CONFIG_TCG', 'CONFIG_SOFTMMU'], if_true: files( >> 'tcg-cpu.c', >> + 'smm_helper.c', >> )) >>
On 2/15/21 1:32 PM, Alex Bennée wrote: > > Claudio Fontana <cfontana@suse.de> writes: > >> On 2/12/21 1:36 PM, Claudio Fontana wrote: >>> smm is only really useful for softmmu, split in two modules >>> around the CONFIG_USER_ONLY, in order to remove the ifdef >>> and use the build system instead. >>> >>> Signed-off-by: Claudio Fontana <cfontana@suse.de> >>> --- >>> target/i386/helper.h | 4 ++++ >>> target/i386/tcg/seg_helper.c | 2 ++ >>> target/i386/tcg/{ => softmmu}/smm_helper.c | 19 ++----------------- >>> target/i386/tcg/translate.c | 2 ++ >>> target/i386/tcg/meson.build | 1 - >>> target/i386/tcg/softmmu/meson.build | 1 + >>> 6 files changed, 11 insertions(+), 18 deletions(-) >>> rename target/i386/tcg/{ => softmmu}/smm_helper.c (98%) >>> >>> diff --git a/target/i386/helper.h b/target/i386/helper.h >>> index c2ae2f7e61..8ffda4cdc6 100644 >>> --- a/target/i386/helper.h >>> +++ b/target/i386/helper.h >>> @@ -70,7 +70,11 @@ DEF_HELPER_1(clac, void, env) >>> DEF_HELPER_1(stac, void, env) >>> DEF_HELPER_3(boundw, void, env, tl, int) >>> DEF_HELPER_3(boundl, void, env, tl, int) >>> + >>> +#ifndef CONFIG_USER_ONLY >>> DEF_HELPER_1(rsm, void, env) >>> +#endif /* !CONFIG_USER_ONLY */ >>> + >>> DEF_HELPER_2(into, void, env, int) >>> DEF_HELPER_2(cmpxchg8b_unlocked, void, env, tl) >>> DEF_HELPER_2(cmpxchg8b, void, env, tl) >>> diff --git a/target/i386/tcg/seg_helper.c b/target/i386/tcg/seg_helper.c >>> index 180d47f0e9..f0cb1bffe7 100644 >>> --- a/target/i386/tcg/seg_helper.c >>> +++ b/target/i386/tcg/seg_helper.c >>> @@ -1351,7 +1351,9 @@ bool x86_cpu_exec_interrupt(CPUState *cs, int interrupt_request) >>> case CPU_INTERRUPT_SMI: >>> cpu_svm_check_intercept_param(env, SVM_EXIT_SMI, 0, 0); >>> cs->interrupt_request &= ~CPU_INTERRUPT_SMI; >>> +#ifndef CONFIG_USER_ONLY >>> do_smm_enter(cpu); >>> +#endif >>> break; >>> case CPU_INTERRUPT_NMI: >>> cpu_svm_check_intercept_param(env, SVM_EXIT_NMI, 0, 0); >>> diff --git a/target/i386/tcg/smm_helper.c b/target/i386/tcg/softmmu/smm_helper.c >>> similarity index 98% >>> rename from target/i386/tcg/smm_helper.c >>> rename to target/i386/tcg/softmmu/smm_helper.c >>> index 62d027abd3..ee53b26629 100644 >>> --- a/target/i386/tcg/smm_helper.c >>> +++ b/target/i386/tcg/softmmu/smm_helper.c >>> @@ -1,5 +1,5 @@ >>> /* >>> - * x86 SMM helpers >>> + * x86 SMM helpers (softmmu-only) >>> * >>> * Copyright (c) 2003 Fabrice Bellard >>> * >>> @@ -18,27 +18,14 @@ >>> */ >>> >>> #include "qemu/osdep.h" >>> -#include "qemu/main-loop.h" >>> #include "cpu.h" >>> #include "exec/helper-proto.h" >>> #include "exec/log.h" >>> -#include "helper-tcg.h" >>> +#include "tcg/helper-tcg.h" >>> >>> >>> /* SMM support */ >>> >>> -#if defined(CONFIG_USER_ONLY) >>> - >>> -void do_smm_enter(X86CPU *cpu) >>> -{ >>> -} >>> - >>> -void helper_rsm(CPUX86State *env) >>> -{ >>> -} >>> - >>> -#else >>> - >>> #ifdef TARGET_X86_64 >>> #define SMM_REVISION_ID 0x00020064 >>> #else >>> @@ -330,5 +317,3 @@ void helper_rsm(CPUX86State *env) >>> qemu_log_mask(CPU_LOG_INT, "SMM: after RSM\n"); >>> log_cpu_state_mask(CPU_LOG_INT, CPU(cpu), CPU_DUMP_CCOP); >>> } >>> - >>> -#endif /* !CONFIG_USER_ONLY */ >>> diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c >>> index af1faf9342..5075ac4830 100644 >>> --- a/target/i386/tcg/translate.c >>> +++ b/target/i386/tcg/translate.c >>> @@ -8321,7 +8321,9 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu) >>> goto illegal_op; >>> gen_update_cc_op(s); >>> gen_jmp_im(s, s->pc - s->cs_base); >>> +#ifndef CONFIG_USER_ONLY >>> gen_helper_rsm(cpu_env); >>> +#endif /* CONFIG_USER_ONLY */ >>> gen_eob(s); >>> break; >> >> Hello Alex, >> >> this is something I wanted to bring in the foreground: >> >> while before we were generating an empty helper call for CONFIG_USER_ONLY, >> now we are not generating anything. > > Surely that says we only generate the helper call when we are not > CONFIG_USER_ONLY? Yes. The difference between before the patch and after the patch is that before we were still going through all the code in tcg_gen_callN, via the call to gen_helper_rsm macro, only to call finally an empty function for CONFIG_USER_ONLY (helper_rsm() {}), while now we do not generate anything, we do not call the gen_helper_rsm macro at all, so we don't go through tcg_gen_callN. > > >> >> >> >>> case 0x1b8: /* SSE4.2 popcnt */ >>> diff --git a/target/i386/tcg/meson.build b/target/i386/tcg/meson.build >>> index 68fa0c3187..ec5daa1edc 100644 >>> --- a/target/i386/tcg/meson.build >>> +++ b/target/i386/tcg/meson.build >>> @@ -8,7 +8,6 @@ i386_ss.add(when: 'CONFIG_TCG', if_true: files( >>> 'misc_helper.c', >>> 'mpx_helper.c', >>> 'seg_helper.c', >>> - 'smm_helper.c', >>> 'svm_helper.c', >>> 'tcg-cpu.c', >>> 'translate.c'), if_false: files('tcg-stub.c')) >>> diff --git a/target/i386/tcg/softmmu/meson.build b/target/i386/tcg/softmmu/meson.build >>> index 4ab30cc32e..35ba16dc3d 100644 >>> --- a/target/i386/tcg/softmmu/meson.build >>> +++ b/target/i386/tcg/softmmu/meson.build >>> @@ -1,3 +1,4 @@ >>> i386_softmmu_ss.add(when: ['CONFIG_TCG', 'CONFIG_SOFTMMU'], if_true: files( >>> 'tcg-cpu.c', >>> + 'smm_helper.c', >>> )) >>> > >
On 15/02/21 13:59, Claudio Fontana wrote: > Yes. The difference between before the patch and after the patch > is that before we were still going through all the code in tcg_gen_callN, > via the call to gen_helper_rsm macro, only to call finally an empty function for CONFIG_USER_ONLY (helper_rsm() {}), > > while now we do not generate anything, we do not call the gen_helper_rsm macro at all, so we don't go through tcg_gen_callN. > Can we even have an abort() for such cases? Paolo
On 2/15/21 2:30 PM, Paolo Bonzini wrote: > On 15/02/21 13:59, Claudio Fontana wrote: >> Yes. The difference between before the patch and after the patch >> is that before we were still going through all the code in tcg_gen_callN, >> via the call to gen_helper_rsm macro, only to call finally an empty function for CONFIG_USER_ONLY (helper_rsm() {}), >> >> while now we do not generate anything, we do not call the gen_helper_rsm macro at all, so we don't go through tcg_gen_callN. >> > > Can we even have an abort() for such cases? > > Paolo > Hi Paolo, where are you suggesting to have an abort()? You mean that we should abort() QEMU as soon as we detect in translate.c an RSM instruction in user-mode? Again the translate.c code for reference: case 0x1aa: /* rsm */ gen_svm_check_intercept(s, pc_start, SVM_EXIT_RSM); if (!(s->flags & HF_SMM_MASK)) goto illegal_op; gen_update_cc_op(s); gen_jmp_im(s, s->pc - s->cs_base); #ifndef CONFIG_USER_ONLY gen_helper_rsm(cpu_env); #endif /* CONFIG_USER_ONLY */ gen_eob(s); break; --- Thanks, CLaudio
On 15/02/21 15:05, Claudio Fontana wrote: > On 2/15/21 2:30 PM, Paolo Bonzini wrote: >> On 15/02/21 13:59, Claudio Fontana wrote: >>> Yes. The difference between before the patch and after the patch >>> is that before we were still going through all the code in >>> tcg_gen_callN, via the call to gen_helper_rsm macro, only to call >>> finally an empty function for CONFIG_USER_ONLY (helper_rsm() >>> {}), >>> >>> while now we do not generate anything, we do not call the >>> gen_helper_rsm macro at all, so we don't go through >>> tcg_gen_callN. >>> >> >> Can we even have an abort() for such cases? >> >> Paolo >> > > Hi Paolo, > > where are you suggesting to have an abort()? > > You mean that we should abort() QEMU as soon as we detect in > translate.c an RSM instruction in user-mode? Translating it is okay (it's just a guaranteed SIGILL), but I'm thinking of aborting if s->flags & HF_SMM_MASK is true. Likewise if we see CPU_INTERRUPT_SMI. Paolo > > case 0x1aa: /* rsm */ > gen_svm_check_intercept(s, pc_start, SVM_EXIT_RSM); > if (!(s->flags & HF_SMM_MASK)) > goto illegal_op; > gen_update_cc_op(s); > gen_jmp_im(s, s->pc - s->cs_base); > #ifndef CONFIG_USER_ONLY > gen_helper_rsm(cpu_env); > #endif /* CONFIG_USER_ONLY */ > gen_eob(s); > break;
On 2/15/21 3:13 PM, Paolo Bonzini wrote: > On 15/02/21 15:05, Claudio Fontana wrote: >> On 2/15/21 2:30 PM, Paolo Bonzini wrote: >>> On 15/02/21 13:59, Claudio Fontana wrote: >>>> Yes. The difference between before the patch and after the patch >>>> is that before we were still going through all the code in >>>> tcg_gen_callN, via the call to gen_helper_rsm macro, only to call >>>> finally an empty function for CONFIG_USER_ONLY (helper_rsm() >>>> {}), >>>> >>>> while now we do not generate anything, we do not call the >>>> gen_helper_rsm macro at all, so we don't go through >>>> tcg_gen_callN. >>>> >>> >>> Can we even have an abort() for such cases? >>> >>> Paolo >>> >> >> Hi Paolo, >> >> where are you suggesting to have an abort()? >> >> You mean that we should abort() QEMU as soon as we detect in >> translate.c an RSM instruction in user-mode? > > Translating it is okay (it's just a guaranteed SIGILL), but I'm thinking > of aborting if s->flags & HF_SMM_MASK is true. Likewise if we see > CPU_INTERRUPT_SMI. > > Paolo > Ok, will rework as you suggest, thanks! >> >> case 0x1aa: /* rsm */ >> gen_svm_check_intercept(s, pc_start, SVM_EXIT_RSM); >> if (!(s->flags & HF_SMM_MASK)) >> goto illegal_op; >> gen_update_cc_op(s); >> gen_jmp_im(s, s->pc - s->cs_base); >> #ifndef CONFIG_USER_ONLY >> gen_helper_rsm(cpu_env); >> #endif /* CONFIG_USER_ONLY */ >> gen_eob(s); >> break; > >
On 2/15/21 3:39 PM, Claudio Fontana wrote: > On 2/15/21 3:13 PM, Paolo Bonzini wrote: >> On 15/02/21 15:05, Claudio Fontana wrote: >>> On 2/15/21 2:30 PM, Paolo Bonzini wrote: >>>> On 15/02/21 13:59, Claudio Fontana wrote: >>>>> Yes. The difference between before the patch and after the patch >>>>> is that before we were still going through all the code in >>>>> tcg_gen_callN, via the call to gen_helper_rsm macro, only to call >>>>> finally an empty function for CONFIG_USER_ONLY (helper_rsm() >>>>> {}), >>>>> >>>>> while now we do not generate anything, we do not call the >>>>> gen_helper_rsm macro at all, so we don't go through >>>>> tcg_gen_callN. >>>>> >>>> >>>> Can we even have an abort() for such cases? >>>> >>>> Paolo >>>> >>> >>> Hi Paolo, >>> >>> where are you suggesting to have an abort()? >>> >>> You mean that we should abort() QEMU as soon as we detect in >>> translate.c an RSM instruction in user-mode? >> >> Translating it is okay (it's just a guaranteed SIGILL), but I'm thinking >> of aborting if s->flags & HF_SMM_MASK is true. Likewise if we see >> CPU_INTERRUPT_SMI. >> >> Paolo >> > > Ok, will rework as you suggest, thanks! By the way, in the case of gen_bpt_io, is it a similar situation, where we should abort in user-mode if we see s->flags & HF_IOBPT_MASK ? static void gen_bpt_io(DisasContext *s, TCGv_i32 t_port, int ot) { #ifndef CONFIG_USER_ONLY if (s->flags & HF_IOBPT_MASK) { TCGv_i32 t_size = tcg_const_i32(1 << ot); TCGv t_next = tcg_const_tl(s->pc - s->cs_base); gen_helper_bpt_io(cpu_env, t_port, t_size, t_next); tcg_temp_free_i32(t_size); tcg_temp_free(t_next); } #endif /* !CONFIG_USER_ONLY */ } What about other cases like case 0xd8: /* VMRUN */ if (!(s->flags & HF_SVME_MASK) || !s->pe) { goto illegal_op; } ... gen_helper_vmrun(cpu_env, tcg_const_i32(s->aflag - 1), tcg_const_i32(s->pc - pc_start)); should we abort there as well if CONFIG_USER_ONLY? And there are many more probably, should it be its own patch? Ciao, Claudio > >>> >>> case 0x1aa: /* rsm */ >>> gen_svm_check_intercept(s, pc_start, SVM_EXIT_RSM); >>> if (!(s->flags & HF_SMM_MASK)) >>> goto illegal_op; >>> gen_update_cc_op(s); >>> gen_jmp_im(s, s->pc - s->cs_base); >>> #ifndef CONFIG_USER_ONLY >>> gen_helper_rsm(cpu_env); >>> #endif /* CONFIG_USER_ONLY */ >>> gen_eob(s); >>> break; >> >> > >
diff --git a/target/i386/helper.h b/target/i386/helper.h index c2ae2f7e61..8ffda4cdc6 100644 --- a/target/i386/helper.h +++ b/target/i386/helper.h @@ -70,7 +70,11 @@ DEF_HELPER_1(clac, void, env) DEF_HELPER_1(stac, void, env) DEF_HELPER_3(boundw, void, env, tl, int) DEF_HELPER_3(boundl, void, env, tl, int) + +#ifndef CONFIG_USER_ONLY DEF_HELPER_1(rsm, void, env) +#endif /* !CONFIG_USER_ONLY */ + DEF_HELPER_2(into, void, env, int) DEF_HELPER_2(cmpxchg8b_unlocked, void, env, tl) DEF_HELPER_2(cmpxchg8b, void, env, tl) diff --git a/target/i386/tcg/seg_helper.c b/target/i386/tcg/seg_helper.c index 180d47f0e9..f0cb1bffe7 100644 --- a/target/i386/tcg/seg_helper.c +++ b/target/i386/tcg/seg_helper.c @@ -1351,7 +1351,9 @@ bool x86_cpu_exec_interrupt(CPUState *cs, int interrupt_request) case CPU_INTERRUPT_SMI: cpu_svm_check_intercept_param(env, SVM_EXIT_SMI, 0, 0); cs->interrupt_request &= ~CPU_INTERRUPT_SMI; +#ifndef CONFIG_USER_ONLY do_smm_enter(cpu); +#endif break; case CPU_INTERRUPT_NMI: cpu_svm_check_intercept_param(env, SVM_EXIT_NMI, 0, 0); diff --git a/target/i386/tcg/smm_helper.c b/target/i386/tcg/softmmu/smm_helper.c similarity index 98% rename from target/i386/tcg/smm_helper.c rename to target/i386/tcg/softmmu/smm_helper.c index 62d027abd3..ee53b26629 100644 --- a/target/i386/tcg/smm_helper.c +++ b/target/i386/tcg/softmmu/smm_helper.c @@ -1,5 +1,5 @@ /* - * x86 SMM helpers + * x86 SMM helpers (softmmu-only) * * Copyright (c) 2003 Fabrice Bellard * @@ -18,27 +18,14 @@ */ #include "qemu/osdep.h" -#include "qemu/main-loop.h" #include "cpu.h" #include "exec/helper-proto.h" #include "exec/log.h" -#include "helper-tcg.h" +#include "tcg/helper-tcg.h" /* SMM support */ -#if defined(CONFIG_USER_ONLY) - -void do_smm_enter(X86CPU *cpu) -{ -} - -void helper_rsm(CPUX86State *env) -{ -} - -#else - #ifdef TARGET_X86_64 #define SMM_REVISION_ID 0x00020064 #else @@ -330,5 +317,3 @@ void helper_rsm(CPUX86State *env) qemu_log_mask(CPU_LOG_INT, "SMM: after RSM\n"); log_cpu_state_mask(CPU_LOG_INT, CPU(cpu), CPU_DUMP_CCOP); } - -#endif /* !CONFIG_USER_ONLY */ diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c index af1faf9342..5075ac4830 100644 --- a/target/i386/tcg/translate.c +++ b/target/i386/tcg/translate.c @@ -8321,7 +8321,9 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu) goto illegal_op; gen_update_cc_op(s); gen_jmp_im(s, s->pc - s->cs_base); +#ifndef CONFIG_USER_ONLY gen_helper_rsm(cpu_env); +#endif /* CONFIG_USER_ONLY */ gen_eob(s); break; case 0x1b8: /* SSE4.2 popcnt */ diff --git a/target/i386/tcg/meson.build b/target/i386/tcg/meson.build index 68fa0c3187..ec5daa1edc 100644 --- a/target/i386/tcg/meson.build +++ b/target/i386/tcg/meson.build @@ -8,7 +8,6 @@ i386_ss.add(when: 'CONFIG_TCG', if_true: files( 'misc_helper.c', 'mpx_helper.c', 'seg_helper.c', - 'smm_helper.c', 'svm_helper.c', 'tcg-cpu.c', 'translate.c'), if_false: files('tcg-stub.c')) diff --git a/target/i386/tcg/softmmu/meson.build b/target/i386/tcg/softmmu/meson.build index 4ab30cc32e..35ba16dc3d 100644 --- a/target/i386/tcg/softmmu/meson.build +++ b/target/i386/tcg/softmmu/meson.build @@ -1,3 +1,4 @@ i386_softmmu_ss.add(when: ['CONFIG_TCG', 'CONFIG_SOFTMMU'], if_true: files( 'tcg-cpu.c', + 'smm_helper.c', ))
smm is only really useful for softmmu, split in two modules around the CONFIG_USER_ONLY, in order to remove the ifdef and use the build system instead. Signed-off-by: Claudio Fontana <cfontana@suse.de> --- target/i386/helper.h | 4 ++++ target/i386/tcg/seg_helper.c | 2 ++ target/i386/tcg/{ => softmmu}/smm_helper.c | 19 ++----------------- target/i386/tcg/translate.c | 2 ++ target/i386/tcg/meson.build | 1 - target/i386/tcg/softmmu/meson.build | 1 + 6 files changed, 11 insertions(+), 18 deletions(-) rename target/i386/tcg/{ => softmmu}/smm_helper.c (98%)