* [Qemu-devel] [PATCH v2 1/4] qom: cpu: Add wrapper to the set-pc hook
2015-06-16 5:46 [Qemu-devel] [PATCH v2 0/4] qom-cpu: Wrap set_pc hook and use in bootloaders Peter Crosthwaite
@ 2015-06-16 5:46 ` Peter Crosthwaite
2015-06-16 11:29 ` Peter Maydell
2015-06-22 17:27 ` Andreas Färber
2015-06-16 5:46 ` [Qemu-devel] [PATCH v2 2/4] gdbstub: Use cpu_set_pc helper Peter Crosthwaite
` (2 subsequent siblings)
3 siblings, 2 replies; 19+ messages in thread
From: Peter Crosthwaite @ 2015-06-16 5:46 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, Peter Crosthwaite, afaerber, edgar.iglesias
Add a wrapper around the CPUClass::set_pc hook. Accepts an error
pointer to report the case where the hook is not set.
Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
---
include/qom/cpu.h | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 7db310e..97d4edf 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -600,6 +600,27 @@ static inline void cpu_unaligned_access(CPUState *cpu, vaddr addr,
#endif
/**
+ * cpu_set_pc:
+ * @cpu: The CPU to set the program counter for.
+ * @addr: Program counter value.
+ * @errp: Error pointer to populate in case of error.
+ *
+ * Set the program counter for a CPU. If there is no available implementation
+ * an error is raised.
+ */
+
+static inline void cpu_set_pc(CPUState *cpu, vaddr addr, Error **errp)
+{
+ CPUClass *cc = CPU_GET_CLASS(cpu);
+
+ if (cc->set_pc) {
+ cc->set_pc(cpu, addr);
+ } else {
+ error_setg(errp, "CPU does not implement set PC");
+ }
+}
+
+/**
* cpu_reset_interrupt:
* @cpu: The CPU to clear the interrupt on.
* @mask: The interrupt mask to clear.
--
1.9.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/4] qom: cpu: Add wrapper to the set-pc hook
2015-06-16 5:46 ` [Qemu-devel] [PATCH v2 1/4] qom: cpu: Add wrapper to the set-pc hook Peter Crosthwaite
@ 2015-06-16 11:29 ` Peter Maydell
2015-06-22 17:27 ` Andreas Färber
1 sibling, 0 replies; 19+ messages in thread
From: Peter Maydell @ 2015-06-16 11:29 UTC (permalink / raw)
To: Peter Crosthwaite
Cc: Edgar E. Iglesias, Peter Crosthwaite, QEMU Developers,
Andreas Färber
On 16 June 2015 at 06:46, Peter Crosthwaite <crosthwaitepeter@gmail.com> wrote:
> Add a wrapper around the CPUClass::set_pc hook. Accepts an error
> pointer to report the case where the hook is not set.
>
> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> ---
> include/qom/cpu.h | 21 +++++++++++++++++++++
> 1 file changed, 21 insertions(+)
>
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 7db310e..97d4edf 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -600,6 +600,27 @@ static inline void cpu_unaligned_access(CPUState *cpu, vaddr addr,
> #endif
>
> /**
> + * cpu_set_pc:
> + * @cpu: The CPU to set the program counter for.
> + * @addr: Program counter value.
> + * @errp: Error pointer to populate in case of error.
> + *
> + * Set the program counter for a CPU. If there is no available implementation
> + * an error is raised.
> + */
> +
> +static inline void cpu_set_pc(CPUState *cpu, vaddr addr, Error **errp)
> +{
> + CPUClass *cc = CPU_GET_CLASS(cpu);
> +
> + if (cc->set_pc) {
> + cc->set_pc(cpu, addr);
> + } else {
> + error_setg(errp, "CPU does not implement set PC");
> + }
> +}
I don't think there are any CPUClass implementations which
don't implement set_pc. The code in cpu-exec.c which does this:
if (cc->synchronize_from_tb) {
cc->synchronize_from_tb(cpu, tb);
} else {
assert(cc->set_pc);
cc->set_pc(cpu, tb->pc);
}
demonstrates that we only need to check the CPUs which implement
synchronize_from_tb (i386, mips, sh4, sparc, tricore) to check
they have a set_pc method too, and they all do. (You can also
just grep for 'cc->set_pc' in target-*/ and confirm they all
have one, as a crosscheck.)
So I think this can simplify down to just calling the class
method unconditionally.
thanks
-- PMM
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/4] qom: cpu: Add wrapper to the set-pc hook
2015-06-16 5:46 ` [Qemu-devel] [PATCH v2 1/4] qom: cpu: Add wrapper to the set-pc hook Peter Crosthwaite
2015-06-16 11:29 ` Peter Maydell
@ 2015-06-22 17:27 ` Andreas Färber
1 sibling, 0 replies; 19+ messages in thread
From: Andreas Färber @ 2015-06-22 17:27 UTC (permalink / raw)
To: Peter Crosthwaite, qemu-devel
Cc: peter.maydell, edgar.iglesias, Peter Crosthwaite
Hi,
Just "cpu: " please, compare git-log.
Am 16.06.2015 um 07:46 schrieb Peter Crosthwaite:
> Add a wrapper around the CPUClass::set_pc hook. Accepts an error
> pointer to report the case where the hook is not set.
>
> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> ---
> include/qom/cpu.h | 21 +++++++++++++++++++++
> 1 file changed, 21 insertions(+)
>
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 7db310e..97d4edf 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -600,6 +600,27 @@ static inline void cpu_unaligned_access(CPUState *cpu, vaddr addr,
> #endif
>
> /**
> + * cpu_set_pc:
> + * @cpu: The CPU to set the program counter for.
> + * @addr: Program counter value.
> + * @errp: Error pointer to populate in case of error.
> + *
> + * Set the program counter for a CPU. If there is no available implementation
> + * an error is raised.
> + */
> +
Please drop the white line here for consistency.
> +static inline void cpu_set_pc(CPUState *cpu, vaddr addr, Error **errp)
> +{
> + CPUClass *cc = CPU_GET_CLASS(cpu);
> +
> + if (cc->set_pc) {
> + cc->set_pc(cpu, addr);
> + } else {
> + error_setg(errp, "CPU does not implement set PC");
> + }
> +}
> +
> +/**
> * cpu_reset_interrupt:
> * @cpu: The CPU to clear the interrupt on.
> * @mask: The interrupt mask to clear.
Otherwise, modulo Peter M.'s comment, looks okay.
Regards,
Andreas
--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH v2 2/4] gdbstub: Use cpu_set_pc helper
2015-06-16 5:46 [Qemu-devel] [PATCH v2 0/4] qom-cpu: Wrap set_pc hook and use in bootloaders Peter Crosthwaite
2015-06-16 5:46 ` [Qemu-devel] [PATCH v2 1/4] qom: cpu: Add wrapper to the set-pc hook Peter Crosthwaite
@ 2015-06-16 5:46 ` Peter Crosthwaite
2015-06-22 17:31 ` Andreas Färber
2015-06-16 5:46 ` [Qemu-devel] [PATCH v2 3/4] arm: boot: Use cpu_set_pc Peter Crosthwaite
2015-06-16 5:46 ` [Qemu-devel] [PATCH v2 4/4] microblaze: " Peter Crosthwaite
3 siblings, 1 reply; 19+ messages in thread
From: Peter Crosthwaite @ 2015-06-16 5:46 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, Peter Crosthwaite, afaerber, edgar.iglesias
Use the cpu_set_pc helper which will take care of CPUClass retrieval
for us.
Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
---
gdbstub.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/gdbstub.c b/gdbstub.c
index 75563db..ceb60ac 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -754,12 +754,9 @@ static void gdb_breakpoint_remove_all(void)
static void gdb_set_cpu_pc(GDBState *s, target_ulong pc)
{
CPUState *cpu = s->c_cpu;
- CPUClass *cc = CPU_GET_CLASS(cpu);
cpu_synchronize_state(cpu);
- if (cc->set_pc) {
- cc->set_pc(cpu, pc);
- }
+ cpu_set_pc(cpu, pc, NULL);
}
static CPUState *find_cpu(uint32_t thread_id)
--
1.9.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/4] gdbstub: Use cpu_set_pc helper
2015-06-16 5:46 ` [Qemu-devel] [PATCH v2 2/4] gdbstub: Use cpu_set_pc helper Peter Crosthwaite
@ 2015-06-22 17:31 ` Andreas Färber
2015-06-24 2:50 ` Peter Crosthwaite
0 siblings, 1 reply; 19+ messages in thread
From: Andreas Färber @ 2015-06-22 17:31 UTC (permalink / raw)
To: Peter Crosthwaite, qemu-devel
Cc: peter.maydell, edgar.iglesias, Peter Crosthwaite
Am 16.06.2015 um 07:46 schrieb Peter Crosthwaite:
> Use the cpu_set_pc helper which will take care of CPUClass retrieval
> for us.
>
> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> ---
> gdbstub.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index 75563db..ceb60ac 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -754,12 +754,9 @@ static void gdb_breakpoint_remove_all(void)
> static void gdb_set_cpu_pc(GDBState *s, target_ulong pc)
> {
> CPUState *cpu = s->c_cpu;
> - CPUClass *cc = CPU_GET_CLASS(cpu);
>
> cpu_synchronize_state(cpu);
> - if (cc->set_pc) {
> - cc->set_pc(cpu, pc);
> - }
> + cpu_set_pc(cpu, pc, NULL);
I believe this argument will probably go away; otherwise this should've
been &error_abort or something instead of NULL.
Regards,
Andreas
> }
>
> static CPUState *find_cpu(uint32_t thread_id)
>
--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/4] gdbstub: Use cpu_set_pc helper
2015-06-22 17:31 ` Andreas Färber
@ 2015-06-24 2:50 ` Peter Crosthwaite
2015-06-24 10:01 ` Peter Maydell
0 siblings, 1 reply; 19+ messages in thread
From: Peter Crosthwaite @ 2015-06-24 2:50 UTC (permalink / raw)
To: Andreas Färber
Cc: Peter Maydell, Peter Crosthwaite, Peter Crosthwaite,
qemu-devel@nongnu.org Developers, Edgar E. Iglesias
On Mon, Jun 22, 2015 at 10:31 AM, Andreas Färber <afaerber@suse.de> wrote:
> Am 16.06.2015 um 07:46 schrieb Peter Crosthwaite:
>> Use the cpu_set_pc helper which will take care of CPUClass retrieval
>> for us.
>>
>> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
>> ---
>> gdbstub.c | 5 +----
>> 1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/gdbstub.c b/gdbstub.c
>> index 75563db..ceb60ac 100644
>> --- a/gdbstub.c
>> +++ b/gdbstub.c
>> @@ -754,12 +754,9 @@ static void gdb_breakpoint_remove_all(void)
>> static void gdb_set_cpu_pc(GDBState *s, target_ulong pc)
>> {
>> CPUState *cpu = s->c_cpu;
>> - CPUClass *cc = CPU_GET_CLASS(cpu);
>>
>> cpu_synchronize_state(cpu);
>> - if (cc->set_pc) {
>> - cc->set_pc(cpu, pc);
>> - }
>> + cpu_set_pc(cpu, pc, NULL);
>
> I believe this argument will probably go away; otherwise this should've
> been &error_abort or something instead of NULL.
>
I'm not sure. As I don't see what is catching the case of a gdb 'c'
packet for a CPU that doesn't implement set_pc. I'd rather preserve
the existing behaviour, and have the qom wrapper do nothing if it is
not implemented.
Regards,
Peter
> Regards,
> Andreas
>
>> }
>>
>> static CPUState *find_cpu(uint32_t thread_id)
>>
>
>
> --
> SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
> 21284 (AG Nürnberg)
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/4] gdbstub: Use cpu_set_pc helper
2015-06-24 2:50 ` Peter Crosthwaite
@ 2015-06-24 10:01 ` Peter Maydell
2015-06-24 17:04 ` Peter Crosthwaite
0 siblings, 1 reply; 19+ messages in thread
From: Peter Maydell @ 2015-06-24 10:01 UTC (permalink / raw)
To: Peter Crosthwaite
Cc: Edgar E. Iglesias, Peter Crosthwaite, Peter Crosthwaite,
Andreas Färber, qemu-devel@nongnu.org Developers
On 24 June 2015 at 03:50, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> On Mon, Jun 22, 2015 at 10:31 AM, Andreas Färber <afaerber@suse.de> wrote:
>> I believe this argument will probably go away; otherwise this should've
>> been &error_abort or something instead of NULL.
>>
>
> I'm not sure. As I don't see what is catching the case of a gdb 'c'
> packet for a CPU that doesn't implement set_pc. I'd rather preserve
> the existing behaviour, and have the qom wrapper do nothing if it is
> not implemented.
Well, this is one reason why every CPU needs to implement set_pc...
-- PMM
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/4] gdbstub: Use cpu_set_pc helper
2015-06-24 10:01 ` Peter Maydell
@ 2015-06-24 17:04 ` Peter Crosthwaite
2015-06-24 17:16 ` Andreas Färber
0 siblings, 1 reply; 19+ messages in thread
From: Peter Crosthwaite @ 2015-06-24 17:04 UTC (permalink / raw)
To: Peter Maydell
Cc: qemu-devel@nongnu.org Developers, Edgar E. Iglesias,
Peter Crosthwaite, Andreas Färber, Peter Crosthwaite
On Wed, Jun 24, 2015 at 3:01 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 24 June 2015 at 03:50, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> On Mon, Jun 22, 2015 at 10:31 AM, Andreas Färber <afaerber@suse.de> wrote:
>>> I believe this argument will probably go away; otherwise this should've
>>> been &error_abort or something instead of NULL.
>>>
>>
>> I'm not sure. As I don't see what is catching the case of a gdb 'c'
>> packet for a CPU that doesn't implement set_pc. I'd rather preserve
>> the existing behaviour, and have the qom wrapper do nothing if it is
>> not implemented.
>
> Well, this is one reason why every CPU needs to implement set_pc...
>
Well. I guess it works for a common case where a continue doesn't
change the PC? If the debugger doesn't change the PC the "c" should
work even without a set_pc call so we don't want to assert on this
valid use case.
Regards,
Peter
> -- PMM
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/4] gdbstub: Use cpu_set_pc helper
2015-06-24 17:04 ` Peter Crosthwaite
@ 2015-06-24 17:16 ` Andreas Färber
2015-06-24 17:28 ` Peter Crosthwaite
2015-06-24 19:09 ` Peter Maydell
0 siblings, 2 replies; 19+ messages in thread
From: Andreas Färber @ 2015-06-24 17:16 UTC (permalink / raw)
To: Peter Crosthwaite, Peter Maydell
Cc: Edgar E. Iglesias, Peter Crosthwaite,
qemu-devel@nongnu.org Developers, Peter Crosthwaite
Am 24.06.2015 um 19:04 schrieb Peter Crosthwaite:
> On Wed, Jun 24, 2015 at 3:01 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 24 June 2015 at 03:50, Peter Crosthwaite
>> <peter.crosthwaite@xilinx.com> wrote:
>>> On Mon, Jun 22, 2015 at 10:31 AM, Andreas Färber <afaerber@suse.de> wrote:
>>>> I believe this argument will probably go away; otherwise this should've
>>>> been &error_abort or something instead of NULL.
>>>>
>>>
>>> I'm not sure. As I don't see what is catching the case of a gdb 'c'
>>> packet for a CPU that doesn't implement set_pc. I'd rather preserve
>>> the existing behaviour, and have the qom wrapper do nothing if it is
>>> not implemented.
>>
>> Well, this is one reason why every CPU needs to implement set_pc...
>>
>
> Well. I guess it works for a common case where a continue doesn't
> change the PC? If the debugger doesn't change the PC the "c" should
> work even without a set_pc call so we don't want to assert on this
> valid use case.
Guys, is there any target that does not implement set_pc today? If so,
which? I'd rather implement it than carry around the iffery and
resulting complications.
I quickly counted 17 target-* in my tree and all 17 seemed to show up in
git-grep. Can you confirm? Didn't check the latest tilegx series.
Regards,
Andreas
--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/4] gdbstub: Use cpu_set_pc helper
2015-06-24 17:16 ` Andreas Färber
@ 2015-06-24 17:28 ` Peter Crosthwaite
2015-06-24 19:09 ` Peter Maydell
1 sibling, 0 replies; 19+ messages in thread
From: Peter Crosthwaite @ 2015-06-24 17:28 UTC (permalink / raw)
To: Andreas Färber
Cc: Peter Maydell, Peter Crosthwaite, Peter Crosthwaite,
qemu-devel@nongnu.org Developers, Edgar E. Iglesias
On Wed, Jun 24, 2015 at 10:16 AM, Andreas Färber <afaerber@suse.de> wrote:
> Am 24.06.2015 um 19:04 schrieb Peter Crosthwaite:
>> On Wed, Jun 24, 2015 at 3:01 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> On 24 June 2015 at 03:50, Peter Crosthwaite
>>> <peter.crosthwaite@xilinx.com> wrote:
>>>> On Mon, Jun 22, 2015 at 10:31 AM, Andreas Färber <afaerber@suse.de> wrote:
>>>>> I believe this argument will probably go away; otherwise this should've
>>>>> been &error_abort or something instead of NULL.
>>>>>
>>>>
>>>> I'm not sure. As I don't see what is catching the case of a gdb 'c'
>>>> packet for a CPU that doesn't implement set_pc. I'd rather preserve
>>>> the existing behaviour, and have the qom wrapper do nothing if it is
>>>> not implemented.
>>>
>>> Well, this is one reason why every CPU needs to implement set_pc...
>>>
>>
>> Well. I guess it works for a common case where a continue doesn't
>> change the PC? If the debugger doesn't change the PC the "c" should
>> work even without a set_pc call so we don't want to assert on this
>> valid use case.
>
> Guys, is there any target that does not implement set_pc today? If so,
> which? I'd rather implement it than carry around the iffery and
> resulting complications.
>
> I quickly counted 17 target-* in my tree and all 17 seemed to show up in
> git-grep. Can you confirm? Didn't check the latest tilegx series.
>
There are 17 targets and 18 sets by my count:
$ git grep "set_pc.*=.*set_pc" target-*
target-alpha/cpu.c: cc->set_pc = alpha_cpu_set_pc;
target-arm/cpu.c: cc->set_pc = arm_cpu_set_pc;
target-arm/cpu64.c: cc->set_pc = aarch64_cpu_set_pc;
target-cris/cpu.c: cc->set_pc = cris_cpu_set_pc;
target-i386/cpu.c: cc->set_pc = x86_cpu_set_pc;
target-lm32/cpu.c: cc->set_pc = lm32_cpu_set_pc;
target-m68k/cpu.c: cc->set_pc = m68k_cpu_set_pc;
target-microblaze/cpu.c: cc->set_pc = mb_cpu_set_pc;
target-mips/cpu.c: cc->set_pc = mips_cpu_set_pc;
target-moxie/cpu.c: cc->set_pc = moxie_cpu_set_pc;
target-openrisc/cpu.c: cc->set_pc = openrisc_cpu_set_pc;
target-ppc/translate_init.c: cc->set_pc = ppc_cpu_set_pc;
target-s390x/cpu.c: cc->set_pc = s390_cpu_set_pc;
target-sh4/cpu.c: cc->set_pc = superh_cpu_set_pc;
target-sparc/cpu.c: cc->set_pc = sparc_cpu_set_pc;
target-tricore/cpu.c: cc->set_pc = tricore_cpu_set_pc;
target-unicore32/cpu.c: cc->set_pc = uc32_cpu_set_pc;
target-xtensa/cpu.c: cc->set_pc = xtensa_cpu_set_pc;
ARM is doubled up. As long as no one else is missing a double up where
there is not default set by a single base class we are OK. If there is
a missing double up with a base class implementation, then there will
be no change by your proposal anyway.
Regards,
Peter
> Regards,
> Andreas
>
> --
> SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
> 21284 (AG Nürnberg)
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/4] gdbstub: Use cpu_set_pc helper
2015-06-24 17:16 ` Andreas Färber
2015-06-24 17:28 ` Peter Crosthwaite
@ 2015-06-24 19:09 ` Peter Maydell
1 sibling, 0 replies; 19+ messages in thread
From: Peter Maydell @ 2015-06-24 19:09 UTC (permalink / raw)
To: Andreas Färber
Cc: qemu-devel@nongnu.org Developers, Edgar E. Iglesias,
Peter Crosthwaite, Peter Crosthwaite, Peter Crosthwaite
On 24 June 2015 at 18:16, Andreas Färber <afaerber@suse.de> wrote:
> Guys, is there any target that does not implement set_pc today? If so,
> which? I'd rather implement it than carry around the iffery and
> resulting complications.
No, there are none, see my analysis in my review of patch 1 in this set.
-- PMM
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH v2 3/4] arm: boot: Use cpu_set_pc
2015-06-16 5:46 [Qemu-devel] [PATCH v2 0/4] qom-cpu: Wrap set_pc hook and use in bootloaders Peter Crosthwaite
2015-06-16 5:46 ` [Qemu-devel] [PATCH v2 1/4] qom: cpu: Add wrapper to the set-pc hook Peter Crosthwaite
2015-06-16 5:46 ` [Qemu-devel] [PATCH v2 2/4] gdbstub: Use cpu_set_pc helper Peter Crosthwaite
@ 2015-06-16 5:46 ` Peter Crosthwaite
2015-06-16 11:32 ` Peter Maydell
2015-06-16 5:46 ` [Qemu-devel] [PATCH v2 4/4] microblaze: " Peter Crosthwaite
3 siblings, 1 reply; 19+ messages in thread
From: Peter Crosthwaite @ 2015-06-16 5:46 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, Peter Crosthwaite, afaerber, edgar.iglesias
Use cpu_set_pc across the board for setting program counters. This
removes instances of system level code having to reach into the CPU
env.
Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
---
Changed since v1:
Lease thumb masking in boot.c
---
hw/arm/boot.c | 19 +++++++------------
1 file changed, 7 insertions(+), 12 deletions(-)
diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index d036624..a54add6 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -168,11 +168,9 @@ static void default_write_secondary(ARMCPU *cpu,
static void default_reset_secondary(ARMCPU *cpu,
const struct arm_boot_info *info)
{
- CPUARMState *env = &cpu->env;
-
address_space_stl_notdirty(&address_space_memory, info->smp_bootreg_addr,
0, MEMTXATTRS_UNSPECIFIED, NULL);
- env->regs[15] = info->smp_loader_start;
+ cpu_set_pc(CPU(cpu), info->smp_loader_start, &error_abort);
}
static inline bool have_dtb(const struct arm_boot_info *info)
@@ -452,12 +450,13 @@ static void do_cpu_reset(void *opaque)
if (info) {
if (!info->is_linux) {
/* Jump to the entry point. */
- if (env->aarch64) {
- env->pc = info->entry;
- } else {
- env->regs[15] = info->entry & 0xfffffffe;
+ uint64_t entry = info->entry;
+
+ if (!env->aarch64) {
env->thumb = info->entry & 1;
+ entry &= 0xfffffffe;
}
+ cpu_set_pc(CPU(cpu), entry, &error_abort);
} else {
/* If we are booting Linux then we need to check whether we are
* booting into secure or non-secure state and adjust the state
@@ -488,11 +487,7 @@ static void do_cpu_reset(void *opaque)
}
if (CPU(cpu) == first_cpu) {
- if (env->aarch64) {
- env->pc = info->loader_start;
- } else {
- env->regs[15] = info->loader_start;
- }
+ cpu_set_pc(CPU(cpu), info->loader_start, &error_abort);
if (!have_dtb(info)) {
if (old_param) {
--
1.9.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/4] arm: boot: Use cpu_set_pc
2015-06-16 5:46 ` [Qemu-devel] [PATCH v2 3/4] arm: boot: Use cpu_set_pc Peter Crosthwaite
@ 2015-06-16 11:32 ` Peter Maydell
2015-06-22 17:33 ` Andreas Färber
0 siblings, 1 reply; 19+ messages in thread
From: Peter Maydell @ 2015-06-16 11:32 UTC (permalink / raw)
To: Peter Crosthwaite
Cc: Edgar E. Iglesias, Peter Crosthwaite, QEMU Developers,
Andreas Färber
On 16 June 2015 at 06:46, Peter Crosthwaite <crosthwaitepeter@gmail.com> wrote:
> Use cpu_set_pc across the board for setting program counters. This
> removes instances of system level code having to reach into the CPU
> env.
>
> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> ---
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
though you'll want to drop the &error_abort argument to
cpu_set_pc() (see my review on patch 1).
thanks
-- PMM
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/4] arm: boot: Use cpu_set_pc
2015-06-16 11:32 ` Peter Maydell
@ 2015-06-22 17:33 ` Andreas Färber
0 siblings, 0 replies; 19+ messages in thread
From: Andreas Färber @ 2015-06-22 17:33 UTC (permalink / raw)
To: Peter Crosthwaite
Cc: Peter Maydell, Peter Crosthwaite, QEMU Developers, Edgar E. Iglesias
Am 16.06.2015 um 13:32 schrieb Peter Maydell:
> On 16 June 2015 at 06:46, Peter Crosthwaite <crosthwaitepeter@gmail.com> wrote:
>> Use cpu_set_pc across the board for setting program counters. This
>> removes instances of system level code having to reach into the CPU
>> env.
>>
>> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
>> ---
>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Andreas Färber <afaerber@suse.de>
>
> though you'll want to drop the &error_abort argument to
> cpu_set_pc() (see my review on patch 1).
Regards,
Andreas
--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH v2 4/4] microblaze: boot: Use cpu_set_pc
2015-06-16 5:46 [Qemu-devel] [PATCH v2 0/4] qom-cpu: Wrap set_pc hook and use in bootloaders Peter Crosthwaite
` (2 preceding siblings ...)
2015-06-16 5:46 ` [Qemu-devel] [PATCH v2 3/4] arm: boot: Use cpu_set_pc Peter Crosthwaite
@ 2015-06-16 5:46 ` Peter Crosthwaite
2015-06-16 11:33 ` Peter Maydell
2015-06-22 17:36 ` Andreas Färber
3 siblings, 2 replies; 19+ messages in thread
From: Peter Crosthwaite @ 2015-06-16 5:46 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, Peter Crosthwaite, afaerber, edgar.iglesias
Use cpu_set_pc for setting program counters when bootloading. This
removes an instance of system level code having to reach into the CPU
env.
Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
---
hw/microblaze/boot.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/microblaze/boot.c b/hw/microblaze/boot.c
index 4c44317..ec68479 100644
--- a/hw/microblaze/boot.c
+++ b/hw/microblaze/boot.c
@@ -54,7 +54,7 @@ static void main_cpu_reset(void *opaque)
env->regs[5] = boot_info.cmdline;
env->regs[6] = boot_info.initrd_start;
env->regs[7] = boot_info.fdt;
- env->sregs[SR_PC] = boot_info.bootstrap_pc;
+ cpu_set_pc(CPU(cpu), boot_info.bootstrap_pc, &error_abort);
if (boot_info.machine_cpu_reset) {
boot_info.machine_cpu_reset(cpu);
}
--
1.9.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/4] microblaze: boot: Use cpu_set_pc
2015-06-16 5:46 ` [Qemu-devel] [PATCH v2 4/4] microblaze: " Peter Crosthwaite
@ 2015-06-16 11:33 ` Peter Maydell
2015-06-16 15:38 ` Peter Crosthwaite
2015-06-22 17:36 ` Andreas Färber
1 sibling, 1 reply; 19+ messages in thread
From: Peter Maydell @ 2015-06-16 11:33 UTC (permalink / raw)
To: Peter Crosthwaite
Cc: Edgar E. Iglesias, Peter Crosthwaite, QEMU Developers,
Andreas Färber
On 16 June 2015 at 06:46, Peter Crosthwaite <crosthwaitepeter@gmail.com> wrote:
> Use cpu_set_pc for setting program counters when bootloading. This
> removes an instance of system level code having to reach into the CPU
> env.
>
> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> ---
> hw/microblaze/boot.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/microblaze/boot.c b/hw/microblaze/boot.c
> index 4c44317..ec68479 100644
> --- a/hw/microblaze/boot.c
> +++ b/hw/microblaze/boot.c
> @@ -54,7 +54,7 @@ static void main_cpu_reset(void *opaque)
> env->regs[5] = boot_info.cmdline;
> env->regs[6] = boot_info.initrd_start;
> env->regs[7] = boot_info.fdt;
> - env->sregs[SR_PC] = boot_info.bootstrap_pc;
> + cpu_set_pc(CPU(cpu), boot_info.bootstrap_pc, &error_abort);
Well, it sort of removes an instance of reaching into the CPU
env, but there's all those other ones in plain sight just above.
Is there much point in setting SR_PC indirectly if we don't
have a mechanism for setting the other regs indirectly?
-- PMM
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/4] microblaze: boot: Use cpu_set_pc
2015-06-16 11:33 ` Peter Maydell
@ 2015-06-16 15:38 ` Peter Crosthwaite
0 siblings, 0 replies; 19+ messages in thread
From: Peter Crosthwaite @ 2015-06-16 15:38 UTC (permalink / raw)
To: Peter Maydell
Cc: Edgar E. Iglesias, Peter Crosthwaite, QEMU Developers,
Andreas Färber, Peter Crosthwaite
On Tue, Jun 16, 2015 at 4:33 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 16 June 2015 at 06:46, Peter Crosthwaite <crosthwaitepeter@gmail.com> wrote:
>> Use cpu_set_pc for setting program counters when bootloading. This
>> removes an instance of system level code having to reach into the CPU
>> env.
>>
>> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
>> ---
>> hw/microblaze/boot.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/microblaze/boot.c b/hw/microblaze/boot.c
>> index 4c44317..ec68479 100644
>> --- a/hw/microblaze/boot.c
>> +++ b/hw/microblaze/boot.c
>> @@ -54,7 +54,7 @@ static void main_cpu_reset(void *opaque)
>> env->regs[5] = boot_info.cmdline;
>> env->regs[6] = boot_info.initrd_start;
>> env->regs[7] = boot_info.fdt;
>> - env->sregs[SR_PC] = boot_info.bootstrap_pc;
>> + cpu_set_pc(CPU(cpu), boot_info.bootstrap_pc, &error_abort);
>
> Well, it sort of removes an instance of reaching into the CPU
> env, but there's all those other ones in plain sight just above.
> Is there much point in setting SR_PC indirectly if we don't
> have a mechanism for setting the other regs indirectly?
>
Yes. Needs more patches :). I'm starting with the easy stuff, and I am
actually more interested in getting rid of the SR_PC macro usage at
this stage. ARMs solution to this is the machine code bootloader. That
would be one way. But what I want to propose was that we add a virtual
method to CPUs that sets these offsets that bootloaders can call. This
brings us closer to one bootloader that can do it all.
Regards,
Peter
> -- PMM
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/4] microblaze: boot: Use cpu_set_pc
2015-06-16 5:46 ` [Qemu-devel] [PATCH v2 4/4] microblaze: " Peter Crosthwaite
2015-06-16 11:33 ` Peter Maydell
@ 2015-06-22 17:36 ` Andreas Färber
1 sibling, 0 replies; 19+ messages in thread
From: Andreas Färber @ 2015-06-22 17:36 UTC (permalink / raw)
To: Peter Crosthwaite, qemu-devel
Cc: peter.maydell, edgar.iglesias, Peter Crosthwaite
Am 16.06.2015 um 07:46 schrieb Peter Crosthwaite:
> Use cpu_set_pc for setting program counters when bootloading. This
> removes an instance of system level code having to reach into the CPU
> env.
>
> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
Reviewed-by: Andreas Färber <afaerber@suse.de>
but please remember to use "cpu_set_pc()" convention here and elsewhere
in the commit messages including subjects.
Regards,
Andreas
--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)
^ permalink raw reply [flat|nested] 19+ messages in thread