| Message ID | 20200819182856.4893-3-robert.foley@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | accel/tcg: remove implied BQL from cpu_handle_interrupt/exception path | expand |
On 8/19/20 11:28 AM, Robert Foley wrote: > Adding ->do_interrupt_locked to ARMCPUClass is preparation for > pushing the BQL down into the per-arch implementation of ->do_interrupt. > > This is needed since ARM's *_cpu_exec_interrupt calls to *_do_interrupt. > With the push down of the BQL into *_cpu_exec_interrupt and > *_do_interrupt, *_cpu_exec_interrupt will call to ->do_interrupt > with lock held. Since ->do_interrupt also has the lock, we need a way > to allow cpu_exec_interrupt to call do_interrupt with lock held. > This patch solves this issue of *_cpu_exec_interrupt needing > to call do_interrupt with lock held. > > This patch is part of a series of transitions to move the > BQL down into the do_interrupt per arch functions. This set of > transitions is needed to maintain bisectability. > > This approach was suggested by Paolo Bonzini. > For reference, here are two key posts in the discussion, explaining > the reasoning/benefits of this approach. > https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00784.html > https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg01517.html > https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08731.html > https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00044.html > > Signed-off-by: Robert Foley <robert.foley@linaro.org> > --- > target/arm/cpu-qom.h | 3 +++ > target/arm/cpu.c | 5 +++-- > target/arm/cpu_tcg.c | 5 +++-- > 3 files changed, 9 insertions(+), 4 deletions(-) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On 8/31/20 2:18 PM, Richard Henderson wrote: > On 8/19/20 11:28 AM, Robert Foley wrote: >> Adding ->do_interrupt_locked to ARMCPUClass is preparation for >> pushing the BQL down into the per-arch implementation of ->do_interrupt. >> >> This is needed since ARM's *_cpu_exec_interrupt calls to *_do_interrupt. >> With the push down of the BQL into *_cpu_exec_interrupt and >> *_do_interrupt, *_cpu_exec_interrupt will call to ->do_interrupt >> with lock held. Since ->do_interrupt also has the lock, we need a way >> to allow cpu_exec_interrupt to call do_interrupt with lock held. >> This patch solves this issue of *_cpu_exec_interrupt needing >> to call do_interrupt with lock held. >> >> This patch is part of a series of transitions to move the >> BQL down into the do_interrupt per arch functions. This set of >> transitions is needed to maintain bisectability. >> >> This approach was suggested by Paolo Bonzini. >> For reference, here are two key posts in the discussion, explaining >> the reasoning/benefits of this approach. >> https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00784.html >> https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg01517.html >> https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08731.html >> https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00044.html >> >> Signed-off-by: Robert Foley <robert.foley@linaro.org> >> --- >> target/arm/cpu-qom.h | 3 +++ >> target/arm/cpu.c | 5 +++-- >> target/arm/cpu_tcg.c | 5 +++-- >> 3 files changed, 9 insertions(+), 4 deletions(-) > > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> I take it back. These two cc->do_interrupt calls can be replaced with direct calls. > #ifndef CONFIG_USER_ONLY > cc->do_interrupt = arm_v7m_cpu_do_interrupt; > #endif > > cc->cpu_exec_interrupt = arm_v7m_cpu_exec_interrupt; If we are in arm_v7m_cpu_exec_interrupt we will always call arm_v7m_cpu_do_interrupt. I think the mismatch of #ifdef, which implies a different destination is possible, is a bug -- cc->do_interrupt is not otherwise assigned and in fact would be NULL. I suspect that some of these slots themselves should be ifdefed, so that we cannot assign to them when they are unused. That would help keep the ifdefs in the cpu init functions in sync. This same condition is *not* true for cris -- there is no crisv10_cpu_exec_interrupt -- so you do need the new do_interrupt_locked field there. r~
Le mar. 1 sept. 2020 00:02, Richard Henderson <richard.henderson@linaro.org> a écrit : > On 8/31/20 2:18 PM, Richard Henderson wrote: > > On 8/19/20 11:28 AM, Robert Foley wrote: > >> Adding ->do_interrupt_locked to ARMCPUClass is preparation for > >> pushing the BQL down into the per-arch implementation of ->do_interrupt. > >> > >> This is needed since ARM's *_cpu_exec_interrupt calls to *_do_interrupt. > >> With the push down of the BQL into *_cpu_exec_interrupt and > >> *_do_interrupt, *_cpu_exec_interrupt will call to ->do_interrupt > >> with lock held. Since ->do_interrupt also has the lock, we need a way > >> to allow cpu_exec_interrupt to call do_interrupt with lock held. > >> This patch solves this issue of *_cpu_exec_interrupt needing > >> to call do_interrupt with lock held. > >> > >> This patch is part of a series of transitions to move the > >> BQL down into the do_interrupt per arch functions. This set of > >> transitions is needed to maintain bisectability. > >> > >> This approach was suggested by Paolo Bonzini. > >> For reference, here are two key posts in the discussion, explaining > >> the reasoning/benefits of this approach. > >> https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00784.html > >> https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg01517.html > >> https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08731.html > >> https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00044.html > >> > >> Signed-off-by: Robert Foley <robert.foley@linaro.org> > >> --- > >> target/arm/cpu-qom.h | 3 +++ > >> target/arm/cpu.c | 5 +++-- > >> target/arm/cpu_tcg.c | 5 +++-- > >> 3 files changed, 9 insertions(+), 4 deletions(-) > > > > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > > I take it back. These two cc->do_interrupt calls can be replaced with > direct > calls. > > > #ifndef CONFIG_USER_ONLY > > cc->do_interrupt = arm_v7m_cpu_do_interrupt; > > #endif > > > > cc->cpu_exec_interrupt = arm_v7m_cpu_exec_interrupt; > > If we are in arm_v7m_cpu_exec_interrupt we will always call > arm_v7m_cpu_do_interrupt. > > I think the mismatch of #ifdef, which implies a different destination is > possible, is a bug -- cc->do_interrupt is not otherwise assigned and in > fact > would be NULL. > > I suspect that some of these slots themselves should be ifdefed, so that we > cannot assign to them when they are unused. That would help keep the > ifdefs in > the cpu init functions in sync. > I tried to do this once but this breaks sizeof(CPUState) archived in libqemu.a vs linking softmmu / user. IIRC Peter explained why we can't do that. I'll search the post tomorrow. > This same condition is *not* true for cris -- there is no > crisv10_cpu_exec_interrupt -- so you do need the new do_interrupt_locked > field > there. > > > r~ > > <div dir="auto"><div><br><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">Le mar. 1 sept. 2020 00:02, Richard Henderson <<a href="mailto:richard.henderson@linaro.org">richard.henderson@linaro.org</a>> a écrit :<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 8/31/20 2:18 PM, Richard Henderson wrote:<br> > On 8/19/20 11:28 AM, Robert Foley wrote:<br> >> Adding ->do_interrupt_locked to ARMCPUClass is preparation for<br> >> pushing the BQL down into the per-arch implementation of ->do_interrupt.<br> >><br> >> This is needed since ARM's *_cpu_exec_interrupt calls to *_do_interrupt.<br> >> With the push down of the BQL into *_cpu_exec_interrupt and<br> >> *_do_interrupt, *_cpu_exec_interrupt will call to ->do_interrupt<br> >> with lock held. Since ->do_interrupt also has the lock, we need a way<br> >> to allow cpu_exec_interrupt to call do_interrupt with lock held.<br> >> This patch solves this issue of *_cpu_exec_interrupt needing<br> >> to call do_interrupt with lock held.<br> >><br> >> This patch is part of a series of transitions to move the<br> >> BQL down into the do_interrupt per arch functions. This set of<br> >> transitions is needed to maintain bisectability.<br> >><br> >> This approach was suggested by Paolo Bonzini.<br> >> For reference, here are two key posts in the discussion, explaining<br> >> the reasoning/benefits of this approach.<br> >> <a href="https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00784.html" rel="noreferrer noreferrer" target="_blank">https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00784.html</a><br> >> <a href="https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg01517.html" rel="noreferrer noreferrer" target="_blank">https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg01517.html</a><br> >> <a href="https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08731.html" rel="noreferrer noreferrer" target="_blank">https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08731.html</a><br> >> <a href="https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00044.html" rel="noreferrer noreferrer" target="_blank">https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00044.html</a><br> >><br> >> Signed-off-by: Robert Foley <<a href="mailto:robert.foley@linaro.org" target="_blank" rel="noreferrer">robert.foley@linaro.org</a>><br> >> ---<br> >> target/arm/cpu-qom.h | 3 +++<br> >> target/arm/cpu.c | 5 +++--<br> >> target/arm/cpu_tcg.c | 5 +++--<br> >> 3 files changed, 9 insertions(+), 4 deletions(-)<br> > <br> > Reviewed-by: Richard Henderson <<a href="mailto:richard.henderson@linaro.org" target="_blank" rel="noreferrer">richard.henderson@linaro.org</a>><br> <br> I take it back. These two cc->do_interrupt calls can be replaced with direct<br> calls.<br> <br> > #ifndef CONFIG_USER_ONLY<br> > cc->do_interrupt = arm_v7m_cpu_do_interrupt;<br> > #endif<br> > <br> > cc->cpu_exec_interrupt = arm_v7m_cpu_exec_interrupt;<br> <br> If we are in arm_v7m_cpu_exec_interrupt we will always call<br> arm_v7m_cpu_do_interrupt.<br> <br> I think the mismatch of #ifdef, which implies a different destination is<br> possible, is a bug -- cc->do_interrupt is not otherwise assigned and in fact<br> would be NULL.<br> <br> I suspect that some of these slots themselves should be ifdefed, so that we<br> cannot assign to them when they are unused. That would help keep the ifdefs in<br> the cpu init functions in sync.<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">I tried to do this once but this breaks sizeof(CPUState) archived in libqemu.a vs linking softmmu / user.</div><div dir="auto">IIRC Peter explained why we can't do that. I'll search the post tomorrow. </div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <br> This same condition is *not* true for cris -- there is no<br> crisv10_cpu_exec_interrupt -- so you do need the new do_interrupt_locked field<br> there.<br> <br> <br> r~<br> <br> </blockquote></div></div></div>
diff --git a/target/arm/cpu-qom.h b/target/arm/cpu-qom.h index 56395b87f6..264280194c 100644 --- a/target/arm/cpu-qom.h +++ b/target/arm/cpu-qom.h @@ -48,6 +48,7 @@ void aarch64_cpu_register(const ARMCPUInfo *info); * ARMCPUClass: * @parent_realize: The parent class' realize handler. * @parent_reset: The parent class' reset handler. + * @do_interrupt_locked: Handler for interrupts (lock already held). * * An ARM CPU model. */ @@ -59,6 +60,8 @@ typedef struct ARMCPUClass { const ARMCPUInfo *info; DeviceRealize parent_realize; DeviceReset parent_reset; + + void (*do_interrupt_locked)(CPUState *cpu); } ARMCPUClass; typedef struct ARMCPU ARMCPU; diff --git a/target/arm/cpu.c b/target/arm/cpu.c index 46c1d92080..d15b459399 100644 --- a/target/arm/cpu.c +++ b/target/arm/cpu.c @@ -526,7 +526,7 @@ static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx, bool arm_cpu_exec_interrupt(CPUState *cs, int interrupt_request) { - CPUClass *cc = CPU_GET_CLASS(cs); + ARMCPUClass *acc = ARM_CPU_GET_CLASS(cs); CPUARMState *env = cs->env_ptr; uint32_t cur_el = arm_current_el(env); bool secure = arm_is_secure(env); @@ -573,7 +573,7 @@ bool arm_cpu_exec_interrupt(CPUState *cs, int interrupt_request) found: cs->exception_index = excp_idx; env->exception.target_el = target_el; - cc->do_interrupt(cs); + acc->do_interrupt_locked(cs); return true; } @@ -2225,6 +2225,7 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data) cc->gdb_write_register = arm_cpu_gdb_write_register; #ifndef CONFIG_USER_ONLY cc->do_interrupt = arm_cpu_do_interrupt_locked; + acc->do_interrupt_locked = arm_cpu_do_interrupt_locked; cc->get_phys_page_attrs_debug = arm_cpu_get_phys_page_attrs_debug; cc->asidx_from_attrs = arm_asidx_from_attrs; cc->vmsd = &vmstate_arm_cpu; diff --git a/target/arm/cpu_tcg.c b/target/arm/cpu_tcg.c index 2fc7a29340..caf0d54c2c 100644 --- a/target/arm/cpu_tcg.c +++ b/target/arm/cpu_tcg.c @@ -17,7 +17,7 @@ static bool arm_v7m_cpu_exec_interrupt(CPUState *cs, int interrupt_request) { - CPUClass *cc = CPU_GET_CLASS(cs); + ARMCPUClass *acc = ARM_CPU_GET_CLASS(cs); ARMCPU *cpu = ARM_CPU(cs); CPUARMState *env = &cpu->env; bool ret = false; @@ -33,7 +33,7 @@ static bool arm_v7m_cpu_exec_interrupt(CPUState *cs, int interrupt_request) if (interrupt_request & CPU_INTERRUPT_HARD && (armv7m_nvic_can_take_pending_exception(env->nvic))) { cs->exception_index = EXCP_IRQ; - cc->do_interrupt(cs); + acc->do_interrupt_locked(cs); ret = true; } return ret; @@ -602,6 +602,7 @@ static void arm_v7m_class_init(ObjectClass *oc, void *data) acc->info = data; #ifndef CONFIG_USER_ONLY cc->do_interrupt = arm_v7m_cpu_do_interrupt_locked; + acc->do_interrupt_locked = arm_v7m_cpu_do_interrupt_locked; #endif cc->cpu_exec_interrupt = arm_v7m_cpu_exec_interrupt;
Adding ->do_interrupt_locked to ARMCPUClass is preparation for pushing the BQL down into the per-arch implementation of ->do_interrupt. This is needed since ARM's *_cpu_exec_interrupt calls to *_do_interrupt. With the push down of the BQL into *_cpu_exec_interrupt and *_do_interrupt, *_cpu_exec_interrupt will call to ->do_interrupt with lock held. Since ->do_interrupt also has the lock, we need a way to allow cpu_exec_interrupt to call do_interrupt with lock held. This patch solves this issue of *_cpu_exec_interrupt needing to call do_interrupt with lock held. This patch is part of a series of transitions to move the BQL down into the do_interrupt per arch functions. This set of transitions is needed to maintain bisectability. This approach was suggested by Paolo Bonzini. For reference, here are two key posts in the discussion, explaining the reasoning/benefits of this approach. https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00784.html https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg01517.html https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08731.html https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00044.html Signed-off-by: Robert Foley <robert.foley@linaro.org> --- target/arm/cpu-qom.h | 3 +++ target/arm/cpu.c | 5 +++-- target/arm/cpu_tcg.c | 5 +++-- 3 files changed, 9 insertions(+), 4 deletions(-) -- 2.17.1