| Message ID | 20240430140035.3889879-2-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | target/arm: Implement FEAT_WFxT | expand |
On 30/4/24 16:00, Peter Maydell wrote: > The TCGCPUOps::cpu_exec_halt method is called from cpu_handle_halt() > when the CPU is halted, so that a target CPU emulation can do > anything target-specific it needs to do. (At the moment we only use > this on i386.) > > The current specification of the method doesn't allow the target > specific code to do something different if the CPU is about to come > out of the halt state, because cpu_handle_halt() only determines this > after the method has returned. (If the method called cpu_has_work() > itself this would introduce a potential race if an interrupt arrived > between the target's method implementation checking and > cpu_handle_halt() repeating the check.) > > Change the definition of the method so that it returns a bool to > tell cpu_handle_halt() whether to stay in halt or not. > > We will want this for the Arm target, where FEAT_WFxT wants to do > some work only for the case where the CPU is in halt but about to > leave it. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > include/hw/core/tcg-cpu-ops.h | 11 +++++++++-- > target/i386/tcg/helper-tcg.h | 2 +- > accel/tcg/cpu-exec.c | 7 +++++-- > target/i386/tcg/sysemu/seg_helper.c | 3 ++- > 4 files changed, 17 insertions(+), 6 deletions(-) Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Peter Maydell <peter.maydell@linaro.org> writes: > The TCGCPUOps::cpu_exec_halt method is called from cpu_handle_halt() > when the CPU is halted, so that a target CPU emulation can do > anything target-specific it needs to do. (At the moment we only use > this on i386.) > > The current specification of the method doesn't allow the target > specific code to do something different if the CPU is about to come > out of the halt state, because cpu_handle_halt() only determines this > after the method has returned. (If the method called cpu_has_work() > itself this would introduce a potential race if an interrupt arrived > between the target's method implementation checking and > cpu_handle_halt() repeating the check.) > > Change the definition of the method so that it returns a bool to > tell cpu_handle_halt() whether to stay in halt or not. > > We will want this for the Arm target, where FEAT_WFxT wants to do > some work only for the case where the CPU is in halt but about to > leave it. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > include/hw/core/tcg-cpu-ops.h | 11 +++++++++-- > target/i386/tcg/helper-tcg.h | 2 +- > accel/tcg/cpu-exec.c | 7 +++++-- > target/i386/tcg/sysemu/seg_helper.c | 3 ++- > 4 files changed, 17 insertions(+), 6 deletions(-) > > diff --git a/include/hw/core/tcg-cpu-ops.h b/include/hw/core/tcg-cpu-ops.h > index dc1f16a9777..f3ac76e6f6d 100644 > --- a/include/hw/core/tcg-cpu-ops.h > +++ b/include/hw/core/tcg-cpu-ops.h > @@ -111,8 +111,15 @@ struct TCGCPUOps { > void (*do_interrupt)(CPUState *cpu); > /** @cpu_exec_interrupt: Callback for processing interrupts in cpu_exec */ > bool (*cpu_exec_interrupt)(CPUState *cpu, int interrupt_request); > - /** @cpu_exec_halt: Callback for handling halt in cpu_exec */ > - void (*cpu_exec_halt)(CPUState *cpu); > + /** > + * @cpu_exec_halt: Callback for handling halt in cpu_exec. > + * > + * Return true to indicate that the CPU should now leave halt, false > + * if it should remain in the halted state. > + * If this method is not provided, the default is to leave halt > + * if cpu_has_work() returns true. > + */ > + bool (*cpu_exec_halt)(CPUState *cpu); Would it be too much to rename the method to cpu_exec_leave_halt() to make it clearer on use the sense of the return value? > /** > * @tlb_fill: Handle a softmmu tlb miss > * > diff --git a/target/i386/tcg/helper-tcg.h b/target/i386/tcg/helper-tcg.h > index effc2c1c984..85957943bf3 100644 > --- a/target/i386/tcg/helper-tcg.h > +++ b/target/i386/tcg/helper-tcg.h > @@ -39,7 +39,7 @@ QEMU_BUILD_BUG_ON(TCG_PHYS_ADDR_BITS > TARGET_PHYS_ADDR_SPACE_BITS); > */ > void x86_cpu_do_interrupt(CPUState *cpu); > #ifndef CONFIG_USER_ONLY > -void x86_cpu_exec_halt(CPUState *cpu); > +bool x86_cpu_exec_halt(CPUState *cpu); > bool x86_need_replay_interrupt(int interrupt_request); > bool x86_cpu_exec_interrupt(CPUState *cpu, int int_req); > #endif > diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c > index 5c70748060a..550f93b19ce 100644 > --- a/accel/tcg/cpu-exec.c > +++ b/accel/tcg/cpu-exec.c > @@ -669,11 +669,14 @@ static inline bool cpu_handle_halt(CPUState *cpu) > #ifndef CONFIG_USER_ONLY > if (cpu->halted) { > const TCGCPUOps *tcg_ops = cpu->cc->tcg_ops; > + bool leave_halt; > > if (tcg_ops->cpu_exec_halt) { > - tcg_ops->cpu_exec_halt(cpu); > + leave_halt = tcg_ops->cpu_exec_halt(cpu); > + } else { > + leave_halt = cpu_has_work(cpu); > } > - if (!cpu_has_work(cpu)) { > + if (!leave_halt) { > return true; > } > > diff --git a/target/i386/tcg/sysemu/seg_helper.c b/target/i386/tcg/sysemu/seg_helper.c > index 2db8083748e..9ba94deb3aa 100644 > --- a/target/i386/tcg/sysemu/seg_helper.c > +++ b/target/i386/tcg/sysemu/seg_helper.c > @@ -128,7 +128,7 @@ void x86_cpu_do_interrupt(CPUState *cs) > } > } > > -void x86_cpu_exec_halt(CPUState *cpu) > +bool x86_cpu_exec_halt(CPUState *cpu) > { > if (cpu->interrupt_request & CPU_INTERRUPT_POLL) { > X86CPU *x86_cpu = X86_CPU(cpu); > @@ -138,6 +138,7 @@ void x86_cpu_exec_halt(CPUState *cpu) > cpu_reset_interrupt(cpu, CPU_INTERRUPT_POLL); > bql_unlock(); > } > + return cpu_has_work(cpu); The x86 version is essentially being called for side effects. Do we want to document this usage in the method? > } > > bool x86_need_replay_interrupt(int interrupt_request)
On 4/30/24 07:00, Peter Maydell wrote: > The TCGCPUOps::cpu_exec_halt method is called from cpu_handle_halt() > when the CPU is halted, so that a target CPU emulation can do > anything target-specific it needs to do. (At the moment we only use > this on i386.) > > The current specification of the method doesn't allow the target > specific code to do something different if the CPU is about to come > out of the halt state, because cpu_handle_halt() only determines this > after the method has returned. (If the method called cpu_has_work() > itself this would introduce a potential race if an interrupt arrived > between the target's method implementation checking and > cpu_handle_halt() repeating the check.) > > Change the definition of the method so that it returns a bool to > tell cpu_handle_halt() whether to stay in halt or not. > > We will want this for the Arm target, where FEAT_WFxT wants to do > some work only for the case where the CPU is in halt but about to > leave it. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > include/hw/core/tcg-cpu-ops.h | 11 +++++++++-- > target/i386/tcg/helper-tcg.h | 2 +- > accel/tcg/cpu-exec.c | 7 +++++-- > target/i386/tcg/sysemu/seg_helper.c | 3 ++- > 4 files changed, 17 insertions(+), 6 deletions(-) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> I like Alex's suggested rename. > --- a/accel/tcg/cpu-exec.c > +++ b/accel/tcg/cpu-exec.c > @@ -669,11 +669,14 @@ static inline bool cpu_handle_halt(CPUState *cpu) > #ifndef CONFIG_USER_ONLY > if (cpu->halted) { > const TCGCPUOps *tcg_ops = cpu->cc->tcg_ops; > + bool leave_halt; > > if (tcg_ops->cpu_exec_halt) { > - tcg_ops->cpu_exec_halt(cpu); > + leave_halt = tcg_ops->cpu_exec_halt(cpu); > + } else { > + leave_halt = cpu_has_work(cpu); > } > - if (!cpu_has_work(cpu)) { > + if (!leave_halt) { > return true; > } As a followup, I would also suggest making implementation of the hook mandatory. We already require the has_work hook to be set; it would simply be a matter of copying the function pointer to the second slot. Also, the assert in cpu_has_work could be moved to startup, as Phil has started to do with some of the other hooks. r~
On Tue, 30 Apr 2024 at 18:15, Alex Bennée <alex.bennee@linaro.org> wrote: > > Peter Maydell <peter.maydell@linaro.org> writes: > > > The TCGCPUOps::cpu_exec_halt method is called from cpu_handle_halt() > > when the CPU is halted, so that a target CPU emulation can do > > anything target-specific it needs to do. (At the moment we only use > > this on i386.) > > > > The current specification of the method doesn't allow the target > > specific code to do something different if the CPU is about to come > > out of the halt state, because cpu_handle_halt() only determines this > > after the method has returned. (If the method called cpu_has_work() > > itself this would introduce a potential race if an interrupt arrived > > between the target's method implementation checking and > > cpu_handle_halt() repeating the check.) > > > > Change the definition of the method so that it returns a bool to > > tell cpu_handle_halt() whether to stay in halt or not. > > > > We will want this for the Arm target, where FEAT_WFxT wants to do > > some work only for the case where the CPU is in halt but about to > > leave it. > > > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > --- > > include/hw/core/tcg-cpu-ops.h | 11 +++++++++-- > > target/i386/tcg/helper-tcg.h | 2 +- > > accel/tcg/cpu-exec.c | 7 +++++-- > > target/i386/tcg/sysemu/seg_helper.c | 3 ++- > > 4 files changed, 17 insertions(+), 6 deletions(-) > > > > diff --git a/include/hw/core/tcg-cpu-ops.h b/include/hw/core/tcg-cpu-ops.h > > index dc1f16a9777..f3ac76e6f6d 100644 > > --- a/include/hw/core/tcg-cpu-ops.h > > +++ b/include/hw/core/tcg-cpu-ops.h > > @@ -111,8 +111,15 @@ struct TCGCPUOps { > > void (*do_interrupt)(CPUState *cpu); > > /** @cpu_exec_interrupt: Callback for processing interrupts in cpu_exec */ > > bool (*cpu_exec_interrupt)(CPUState *cpu, int interrupt_request); > > - /** @cpu_exec_halt: Callback for handling halt in cpu_exec */ > > - void (*cpu_exec_halt)(CPUState *cpu); > > + /** > > + * @cpu_exec_halt: Callback for handling halt in cpu_exec. > > + * > > + * Return true to indicate that the CPU should now leave halt, false > > + * if it should remain in the halted state. > > + * If this method is not provided, the default is to leave halt > > + * if cpu_has_work() returns true. > > + */ > > + bool (*cpu_exec_halt)(CPUState *cpu); > > Would it be too much to rename the method to cpu_exec_leave_halt() to > make it clearer on use the sense of the return value? We could, but that makes it sound like it's a method to say "should we leave halt?", which ... > > -void x86_cpu_exec_halt(CPUState *cpu) > > +bool x86_cpu_exec_halt(CPUState *cpu) > > { > > if (cpu->interrupt_request & CPU_INTERRUPT_POLL) { > > X86CPU *x86_cpu = X86_CPU(cpu); > > @@ -138,6 +138,7 @@ void x86_cpu_exec_halt(CPUState *cpu) > > cpu_reset_interrupt(cpu, CPU_INTERRUPT_POLL); > > bql_unlock(); > > } > > + return cpu_has_work(cpu); > > The x86 version is essentially being called for side effects. Do we want > to document this usage in the method? ...is not how the x86 target is using it, as you note. thanks -- PMM
On Tue, 30 Apr 2024 at 18:15, Alex Bennée <alex.bennee@linaro.org> wrote: > The x86 version is essentially being called for side effects. Do we want > to document this usage in the method? I plan to take these two patches into target-arm.next, with a slightly beefed up doc comment: /** * @cpu_exec_halt: Callback for handling halt in cpu_exec. * * The target CPU should do any special processing here that it needs * to do when the CPU is in the halted state. * * Return true to indicate that the CPU should now leave halt, false * if it should remain in the halted state. * * If this method is not provided, the default is to do nothing, and * to leave halt if cpu_has_work() returns true. */ which hopefully makes it a little clearer to the reader that "do stuff you need to do when in halt" is the primary purpose of the method. I'll do the follow-on tidyup rth suggests once these have made it into git. thanks -- PMM
diff --git a/include/hw/core/tcg-cpu-ops.h b/include/hw/core/tcg-cpu-ops.h index dc1f16a9777..f3ac76e6f6d 100644 --- a/include/hw/core/tcg-cpu-ops.h +++ b/include/hw/core/tcg-cpu-ops.h @@ -111,8 +111,15 @@ struct TCGCPUOps { void (*do_interrupt)(CPUState *cpu); /** @cpu_exec_interrupt: Callback for processing interrupts in cpu_exec */ bool (*cpu_exec_interrupt)(CPUState *cpu, int interrupt_request); - /** @cpu_exec_halt: Callback for handling halt in cpu_exec */ - void (*cpu_exec_halt)(CPUState *cpu); + /** + * @cpu_exec_halt: Callback for handling halt in cpu_exec. + * + * Return true to indicate that the CPU should now leave halt, false + * if it should remain in the halted state. + * If this method is not provided, the default is to leave halt + * if cpu_has_work() returns true. + */ + bool (*cpu_exec_halt)(CPUState *cpu); /** * @tlb_fill: Handle a softmmu tlb miss * diff --git a/target/i386/tcg/helper-tcg.h b/target/i386/tcg/helper-tcg.h index effc2c1c984..85957943bf3 100644 --- a/target/i386/tcg/helper-tcg.h +++ b/target/i386/tcg/helper-tcg.h @@ -39,7 +39,7 @@ QEMU_BUILD_BUG_ON(TCG_PHYS_ADDR_BITS > TARGET_PHYS_ADDR_SPACE_BITS); */ void x86_cpu_do_interrupt(CPUState *cpu); #ifndef CONFIG_USER_ONLY -void x86_cpu_exec_halt(CPUState *cpu); +bool x86_cpu_exec_halt(CPUState *cpu); bool x86_need_replay_interrupt(int interrupt_request); bool x86_cpu_exec_interrupt(CPUState *cpu, int int_req); #endif diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c index 5c70748060a..550f93b19ce 100644 --- a/accel/tcg/cpu-exec.c +++ b/accel/tcg/cpu-exec.c @@ -669,11 +669,14 @@ static inline bool cpu_handle_halt(CPUState *cpu) #ifndef CONFIG_USER_ONLY if (cpu->halted) { const TCGCPUOps *tcg_ops = cpu->cc->tcg_ops; + bool leave_halt; if (tcg_ops->cpu_exec_halt) { - tcg_ops->cpu_exec_halt(cpu); + leave_halt = tcg_ops->cpu_exec_halt(cpu); + } else { + leave_halt = cpu_has_work(cpu); } - if (!cpu_has_work(cpu)) { + if (!leave_halt) { return true; } diff --git a/target/i386/tcg/sysemu/seg_helper.c b/target/i386/tcg/sysemu/seg_helper.c index 2db8083748e..9ba94deb3aa 100644 --- a/target/i386/tcg/sysemu/seg_helper.c +++ b/target/i386/tcg/sysemu/seg_helper.c @@ -128,7 +128,7 @@ void x86_cpu_do_interrupt(CPUState *cs) } } -void x86_cpu_exec_halt(CPUState *cpu) +bool x86_cpu_exec_halt(CPUState *cpu) { if (cpu->interrupt_request & CPU_INTERRUPT_POLL) { X86CPU *x86_cpu = X86_CPU(cpu); @@ -138,6 +138,7 @@ void x86_cpu_exec_halt(CPUState *cpu) cpu_reset_interrupt(cpu, CPU_INTERRUPT_POLL); bql_unlock(); } + return cpu_has_work(cpu); } bool x86_need_replay_interrupt(int interrupt_request)
The TCGCPUOps::cpu_exec_halt method is called from cpu_handle_halt() when the CPU is halted, so that a target CPU emulation can do anything target-specific it needs to do. (At the moment we only use this on i386.) The current specification of the method doesn't allow the target specific code to do something different if the CPU is about to come out of the halt state, because cpu_handle_halt() only determines this after the method has returned. (If the method called cpu_has_work() itself this would introduce a potential race if an interrupt arrived between the target's method implementation checking and cpu_handle_halt() repeating the check.) Change the definition of the method so that it returns a bool to tell cpu_handle_halt() whether to stay in halt or not. We will want this for the Arm target, where FEAT_WFxT wants to do some work only for the case where the CPU is in halt but about to leave it. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- include/hw/core/tcg-cpu-ops.h | 11 +++++++++-- target/i386/tcg/helper-tcg.h | 2 +- accel/tcg/cpu-exec.c | 7 +++++-- target/i386/tcg/sysemu/seg_helper.c | 3 ++- 4 files changed, 17 insertions(+), 6 deletions(-)