From: Cupertino Miranda <Cupertino.Miranda@synopsys.com> To: Richard Henderson <richard.henderson@linaro.org>, "cupertinomiranda@gmail.com" <cupertinomiranda@gmail.com>, "qemu-devel@nongnu.org" <qemu-devel@nongnu.org> Cc: Shahab Vahedi <shahab.vahedi@gmail.com>, Claudiu Zissulescu <Claudiu.Zissulescu@synopsys.com>, "linux-snps-arc@lists.infradead.org" <linux-snps-arc@lists.infradead.org>, Claudiu Zissulescu <claziss@gmail.com>, Shahab Vahedi <Shahab.Vahedi@synopsys.com> Subject: Re: [PATCH 04/15] arc: TCG and decoder glue code and helpers Date: Fri, 15 Jan 2021 17:11:27 +0000 [thread overview] Message-ID: <a1ea9064-dab5-c683-9899-bb19785f8ee4@synopsys.com> (raw) In-Reply-To: <33ba8432-64c7-db76-459c-5fa6fd7e549a@linaro.org> >> +void QEMU_NORETURN helper_halt(CPUARCState *env, uint32_t npc) >> +{ >> + CPUState *cs = env_cpu(env); >> + if (env->stat.Uf) { >> + cs->exception_index = EXCP_PRIVILEGEV; >> + env->causecode = 0; >> + env->param = 0; >> + /* Restore PC such that we point at the faulty instruction. */ >> + env->eret = env->pc; > > Any reason not to handle Uf at translate time? Or at least create a single > helper function for that here. But it seems like translate will have to do a > lot of priv checking anyway and will already have that handy. Since we needed a helper anyway to deal with causecode and param, we thought it would be reasonable to do all in the helper. We did not made a TCG access for causecode and param enviroment values. > >> +void helper_enter(CPUARCState *env, uint32_t u6) >> +{ >> + /* nothing to do? then bye-bye! */ >> + if (!u6) { >> + return; >> + } >> + >> + uint8_t regs = u6 & 0x0f; /* u[3:0] determines registers to save */ >> + bool save_fp = u6 & 0x10; /* u[4] indicates if fp must be saved */ >> + bool save_blink = u6 & 0x20; /* u[5] indicates saving of blink */ >> + uint8_t stack_size = 4 * (regs + save_fp + save_blink); >> + >> + /* number of regs to be saved must be sane */ >> + check_enter_leave_nr_regs(env, regs, GETPC()); > > Both of these checks could be translate time. > >> + /* this cannot be executed in a delay/execution slot */ >> + check_delay_or_execution_slot(env, GETPC()); > > As could this. > >> + /* stack must be a multiple of 4 (32 bit aligned) */ >> + check_addr_is_word_aligned(env, CPU_SP(env) - stack_size, GETPC()); >> + >> + uint32_t tmp_sp = CPU_SP(env); >> + >> + if (save_fp) { >> + tmp_sp -= 4; >> + cpu_stl_data(env, tmp_sp, CPU_FP(env)); >> + } > > And what if these stores raise an exception? I doubt you're going to get an > exception at the correct pc. > >> +void helper_leave(CPUARCState *env, uint32_t u7) > > Similarly. I think that both of these could be implemented entirely in > translate, which is what > >> + bool restore_fp = u7 & 0x10; /* u[4] indicates if fp must be saved */ >> + bool restore_blink = u7 & 0x20; /* u[5] indicates saving of blink */ >> + bool jump_to_blink = u7 & 0x40; /* u[6] should we jump to blink? */ > > these bits strongly imply. > For lack of knowing better, it is unclear to me where to draw the line when choosing between a translate time (tcg) or helper implementation. Your suggestions for carry/overflow computation are sharp and we should have never used an helper, however I wonder what would be the benefit of implementing enter and leave through TCG. We have dealt with those exception issues by just changing SP in the end of the instruction implementation, when no exceptions can happen. As far as I understand when an exception happens in the middle of the helper or even on a TCG implementation, it jumps out of that TB execution to deal with the exception. On rtie instead of it returning to the same tcg_ld or tcg_st where it actually triggered the exception it will re-decode the same instruction which triggered the exception, and re-attempts to execute it. Is that the case in current TCG implementation, or did it improved and it is now able to return to previous execution flow (i.e translation block) ? _______________________________________________ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
WARNING: multiple messages have this Message-ID (diff)
From: Cupertino Miranda <Cupertino.Miranda@synopsys.com> To: Richard Henderson <richard.henderson@linaro.org>, "cupertinomiranda@gmail.com" <cupertinomiranda@gmail.com>, "qemu-devel@nongnu.org" <qemu-devel@nongnu.org> Cc: Shahab Vahedi <shahab.vahedi@gmail.com>, Claudiu Zissulescu <Claudiu.Zissulescu@synopsys.com>, "linux-snps-arc@lists.infradead.org" <linux-snps-arc@lists.infradead.org>, Claudiu Zissulescu <claziss@gmail.com>, Shahab Vahedi <Shahab.Vahedi@synopsys.com> Subject: Re: [PATCH 04/15] arc: TCG and decoder glue code and helpers Date: Fri, 15 Jan 2021 17:11:27 +0000 [thread overview] Message-ID: <a1ea9064-dab5-c683-9899-bb19785f8ee4@synopsys.com> (raw) In-Reply-To: <33ba8432-64c7-db76-459c-5fa6fd7e549a@linaro.org> >> +void QEMU_NORETURN helper_halt(CPUARCState *env, uint32_t npc) >> +{ >> + CPUState *cs = env_cpu(env); >> + if (env->stat.Uf) { >> + cs->exception_index = EXCP_PRIVILEGEV; >> + env->causecode = 0; >> + env->param = 0; >> + /* Restore PC such that we point at the faulty instruction. */ >> + env->eret = env->pc; > > Any reason not to handle Uf at translate time? Or at least create a single > helper function for that here. But it seems like translate will have to do a > lot of priv checking anyway and will already have that handy. Since we needed a helper anyway to deal with causecode and param, we thought it would be reasonable to do all in the helper. We did not made a TCG access for causecode and param enviroment values. > >> +void helper_enter(CPUARCState *env, uint32_t u6) >> +{ >> + /* nothing to do? then bye-bye! */ >> + if (!u6) { >> + return; >> + } >> + >> + uint8_t regs = u6 & 0x0f; /* u[3:0] determines registers to save */ >> + bool save_fp = u6 & 0x10; /* u[4] indicates if fp must be saved */ >> + bool save_blink = u6 & 0x20; /* u[5] indicates saving of blink */ >> + uint8_t stack_size = 4 * (regs + save_fp + save_blink); >> + >> + /* number of regs to be saved must be sane */ >> + check_enter_leave_nr_regs(env, regs, GETPC()); > > Both of these checks could be translate time. > >> + /* this cannot be executed in a delay/execution slot */ >> + check_delay_or_execution_slot(env, GETPC()); > > As could this. > >> + /* stack must be a multiple of 4 (32 bit aligned) */ >> + check_addr_is_word_aligned(env, CPU_SP(env) - stack_size, GETPC()); >> + >> + uint32_t tmp_sp = CPU_SP(env); >> + >> + if (save_fp) { >> + tmp_sp -= 4; >> + cpu_stl_data(env, tmp_sp, CPU_FP(env)); >> + } > > And what if these stores raise an exception? I doubt you're going to get an > exception at the correct pc. > >> +void helper_leave(CPUARCState *env, uint32_t u7) > > Similarly. I think that both of these could be implemented entirely in > translate, which is what > >> + bool restore_fp = u7 & 0x10; /* u[4] indicates if fp must be saved */ >> + bool restore_blink = u7 & 0x20; /* u[5] indicates saving of blink */ >> + bool jump_to_blink = u7 & 0x40; /* u[6] should we jump to blink? */ > > these bits strongly imply. > For lack of knowing better, it is unclear to me where to draw the line when choosing between a translate time (tcg) or helper implementation. Your suggestions for carry/overflow computation are sharp and we should have never used an helper, however I wonder what would be the benefit of implementing enter and leave through TCG. We have dealt with those exception issues by just changing SP in the end of the instruction implementation, when no exceptions can happen. As far as I understand when an exception happens in the middle of the helper or even on a TCG implementation, it jumps out of that TB execution to deal with the exception. On rtie instead of it returning to the same tcg_ld or tcg_st where it actually triggered the exception it will re-decode the same instruction which triggered the exception, and re-attempts to execute it. Is that the case in current TCG implementation, or did it improved and it is now able to return to previous execution flow (i.e translation block) ?
next prev parent reply other threads:[~2021-01-15 17:11 UTC|newest] Thread overview: 80+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-11-11 16:17 [PATCH 00/15] *** ARC port for review *** cupertinomiranda 2020-11-11 16:17 ` cupertinomiranda 2020-11-11 16:17 ` [PATCH 01/15] arc: Add initial core cpu files cupertinomiranda 2020-11-11 16:17 ` cupertinomiranda 2020-12-01 19:06 ` Richard Henderson 2020-12-01 19:06 ` Richard Henderson 2020-11-11 16:17 ` [PATCH 02/15] arc: Decoder code cupertinomiranda 2020-11-11 16:17 ` cupertinomiranda 2020-11-11 16:17 ` [PATCH 03/15] arc: Opcode definitions table cupertinomiranda 2020-11-11 16:17 ` cupertinomiranda 2020-12-01 20:22 ` Richard Henderson 2020-12-01 20:22 ` Richard Henderson 2021-01-15 17:11 ` Cupertino Miranda 2021-01-15 17:11 ` Cupertino Miranda 2021-01-15 19:52 ` Richard Henderson 2021-01-15 19:52 ` Richard Henderson 2020-11-11 16:17 ` [PATCH 04/15] arc: TCG and decoder glue code and helpers cupertinomiranda 2020-11-11 16:17 ` cupertinomiranda 2020-12-01 21:35 ` Richard Henderson 2020-12-01 21:35 ` Richard Henderson 2021-01-15 17:11 ` Cupertino Miranda [this message] 2021-01-15 17:11 ` Cupertino Miranda 2021-01-15 20:31 ` Richard Henderson 2021-01-15 20:31 ` Richard Henderson 2021-01-15 21:48 ` Cupertino Miranda 2021-01-15 21:48 ` Cupertino Miranda 2021-01-15 21:53 ` Richard Henderson 2021-01-15 21:53 ` Richard Henderson 2021-01-15 22:06 ` Cupertino Miranda 2021-01-15 22:06 ` Cupertino Miranda 2021-01-15 21:28 ` Shahab Vahedi 2021-01-15 21:28 ` Shahab Vahedi 2021-01-15 21:51 ` Richard Henderson 2021-01-15 21:51 ` Richard Henderson 2020-11-11 16:17 ` [PATCH 05/15] arc: TCG instruction generator and hand-definitions cupertinomiranda 2020-11-11 16:17 ` cupertinomiranda 2020-12-01 22:16 ` Richard Henderson 2020-12-01 22:16 ` Richard Henderson 2021-01-15 17:11 ` Cupertino Miranda 2021-01-15 17:11 ` Cupertino Miranda 2021-01-15 20:17 ` Richard Henderson 2021-01-15 20:17 ` Richard Henderson 2021-01-15 21:38 ` Cupertino Miranda 2021-01-15 21:38 ` Cupertino Miranda 2020-11-11 16:17 ` [PATCH 06/15] arc: TCG instruction definitions cupertinomiranda 2020-11-11 16:17 ` cupertinomiranda 2020-12-01 23:09 ` Richard Henderson 2020-12-01 23:09 ` Richard Henderson 2020-12-02 12:55 ` Cupertino Miranda 2020-12-02 12:55 ` Cupertino Miranda 2020-12-03 16:07 ` Richard Henderson 2020-12-03 16:07 ` Richard Henderson 2020-12-03 16:54 ` Cupertino Miranda 2020-12-03 16:54 ` Cupertino Miranda 2020-12-03 19:34 ` Richard Henderson 2020-12-03 19:34 ` Richard Henderson 2020-12-03 19:51 ` Cupertino Miranda 2020-12-03 19:51 ` Cupertino Miranda 2021-01-15 17:11 ` Cupertino Miranda 2021-01-15 17:11 ` Cupertino Miranda 2020-11-11 16:17 ` [PATCH 07/15] arc: Add BCR and AUX registers implementation cupertinomiranda 2020-11-11 16:17 ` cupertinomiranda 2020-11-11 16:17 ` [PATCH 08/15] arc: Add IRQ and timer subsystem support cupertinomiranda 2020-11-11 16:17 ` cupertinomiranda 2020-11-11 16:17 ` [PATCH 09/15] arc: Add memory management unit (MMU) support cupertinomiranda 2020-11-11 16:17 ` cupertinomiranda 2020-11-11 16:17 ` [PATCH 10/15] arc: Add memory protection unit (MPU) support cupertinomiranda 2020-11-11 16:17 ` cupertinomiranda 2020-11-11 16:17 ` [PATCH 11/15] arc: Add gdbstub and XML for debugging support cupertinomiranda 2020-11-11 16:17 ` cupertinomiranda 2020-11-11 16:17 ` [PATCH 12/15] arc: Add Synopsys ARC emulation boards cupertinomiranda 2020-11-11 16:17 ` cupertinomiranda 2020-11-11 16:17 ` [PATCH 13/15] arc: Add support for ARCv2 cupertinomiranda 2020-11-11 16:17 ` cupertinomiranda 2020-11-11 16:17 ` [PATCH 14/15] tests/tcg: ARC: Add TCG instruction definition tests cupertinomiranda 2020-11-11 16:17 ` cupertinomiranda 2020-11-11 16:17 ` [PATCH 15/15] tests/acceptance: ARC: Add linux boot testing cupertinomiranda 2020-11-11 16:17 ` cupertinomiranda 2020-11-11 16:43 ` [PATCH 00/15] *** ARC port for review *** no-reply 2020-11-11 16:43 ` no-reply
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=a1ea9064-dab5-c683-9899-bb19785f8ee4@synopsys.com \ --to=cupertino.miranda@synopsys.com \ --cc=Claudiu.Zissulescu@synopsys.com \ --cc=Shahab.Vahedi@synopsys.com \ --cc=claziss@gmail.com \ --cc=cupertinomiranda@gmail.com \ --cc=linux-snps-arc@lists.infradead.org \ --cc=qemu-devel@nongnu.org \ --cc=richard.henderson@linaro.org \ --cc=shahab.vahedi@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.