* Re: [Qemu-devel] [PATCH 2.0] PPC: E500: Set PIR default reset value rather than SPR value
2014-04-03 18:48 [Qemu-devel] [PATCH 2.0] PPC: E500: Set PIR default reset value rather than SPR value Alexander Graf
@ 2014-04-03 18:50 ` Andreas Färber
2014-04-03 18:55 ` Peter Maydell
2014-04-04 8:18 ` Frederic Konrad
2 siblings, 0 replies; 7+ messages in thread
From: Andreas Färber @ 2014-04-03 18:50 UTC (permalink / raw)
To: Alexander Graf, qemu-devel
Cc: Alexey Kardashevskiy, Peter Maydell, Greg Kurz, qemu-ppc,
Frederic Konrad
Am 03.04.2014 20:48, schrieb Alexander Graf:
> We now reset SPRs to their reset values on CPU reset. So if we want
> to have an SPR persistently changed, we need to change its default
> reset value rather than the value itself manually.
>
> Do this for SPR_BOOKE_PIR, fixing e500v2 SMP boot.
>
> Reported-by: Frederic Konrad <fred.konrad@greensocs.com>
Suggested-by: Andreas Färber <afaerber@suse.de>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
> hw/ppc/e500.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index d7ba25f..f984b3e 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -649,7 +649,7 @@ void ppce500_init(QEMUMachineInitArgs *args, PPCE500Params *params)
> input = (qemu_irq *)env->irq_inputs;
> irqs[i][OPENPIC_OUTPUT_INT] = input[PPCE500_INPUT_INT];
> irqs[i][OPENPIC_OUTPUT_CINT] = input[PPCE500_INPUT_CINT];
> - env->spr[SPR_BOOKE_PIR] = cs->cpu_index = i;
> + env->spr_cb[SPR_BOOKE_PIR].default_value = cs->cpu_index = i;
> env->mpic_iack = MPC8544_CCSRBAR_BASE +
> MPC8544_MPIC_REGS_OFFSET + 0xa0;
>
.default_value then. ;)
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH 2.0] PPC: E500: Set PIR default reset value rather than SPR value
2014-04-03 18:48 [Qemu-devel] [PATCH 2.0] PPC: E500: Set PIR default reset value rather than SPR value Alexander Graf
2014-04-03 18:50 ` Andreas Färber
@ 2014-04-03 18:55 ` Peter Maydell
2014-04-03 18:58 ` Alexander Graf
2014-04-04 8:18 ` Frederic Konrad
2 siblings, 1 reply; 7+ messages in thread
From: Peter Maydell @ 2014-04-03 18:55 UTC (permalink / raw)
To: Alexander Graf
Cc: Alexey Kardashevskiy, QEMU Developers, qemu-ppc@nongnu.org,
Greg Kurz, Andreas Färber, Frederic Konrad
On 3 April 2014 19:48, Alexander Graf <agraf@suse.de> wrote:
> We now reset SPRs to their reset values on CPU reset. So if we want
> to have an SPR persistently changed, we need to change its default
> reset value rather than the value itself manually.
>
> Do this for SPR_BOOKE_PIR, fixing e500v2 SMP boot.
>
> Reported-by: Frederic Konrad <fred.konrad@greensocs.com>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
> hw/ppc/e500.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index d7ba25f..f984b3e 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -649,7 +649,7 @@ void ppce500_init(QEMUMachineInitArgs *args, PPCE500Params *params)
> input = (qemu_irq *)env->irq_inputs;
> irqs[i][OPENPIC_OUTPUT_INT] = input[PPCE500_INPUT_INT];
> irqs[i][OPENPIC_OUTPUT_CINT] = input[PPCE500_INPUT_CINT];
> - env->spr[SPR_BOOKE_PIR] = cs->cpu_index = i;
> + env->spr_cb[SPR_BOOKE_PIR].default_value = cs->cpu_index = i;
Not an issue introduced with this patch, but should we really
be missing with CPUState::cpu_index in a board file?
(Nowhere else does, it's otherwise simply set to an
appropriately incremented index by cpu_exec_init().)
I rather suspect that cpu_index will already be set
to the value we set it, in any case...
thanks
-- PMM
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH 2.0] PPC: E500: Set PIR default reset value rather than SPR value
2014-04-03 18:55 ` Peter Maydell
@ 2014-04-03 18:58 ` Alexander Graf
2014-04-04 0:26 ` Alexey Kardashevskiy
0 siblings, 1 reply; 7+ messages in thread
From: Alexander Graf @ 2014-04-03 18:58 UTC (permalink / raw)
To: Peter Maydell
Cc: Alexey Kardashevskiy, QEMU Developers, qemu-ppc@nongnu.org,
Greg Kurz, Andreas Färber, Frederic Konrad
On 03.04.2014, at 20:55, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 3 April 2014 19:48, Alexander Graf <agraf@suse.de> wrote:
>> We now reset SPRs to their reset values on CPU reset. So if we want
>> to have an SPR persistently changed, we need to change its default
>> reset value rather than the value itself manually.
>>
>> Do this for SPR_BOOKE_PIR, fixing e500v2 SMP boot.
>>
>> Reported-by: Frederic Konrad <fred.konrad@greensocs.com>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>> hw/ppc/e500.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
>> index d7ba25f..f984b3e 100644
>> --- a/hw/ppc/e500.c
>> +++ b/hw/ppc/e500.c
>> @@ -649,7 +649,7 @@ void ppce500_init(QEMUMachineInitArgs *args, PPCE500Params *params)
>> input = (qemu_irq *)env->irq_inputs;
>> irqs[i][OPENPIC_OUTPUT_INT] = input[PPCE500_INPUT_INT];
>> irqs[i][OPENPIC_OUTPUT_CINT] = input[PPCE500_INPUT_CINT];
>> - env->spr[SPR_BOOKE_PIR] = cs->cpu_index = i;
>> + env->spr_cb[SPR_BOOKE_PIR].default_value = cs->cpu_index = i;
>
> Not an issue introduced with this patch, but should we really
> be missing with CPUState::cpu_index in a board file?
> (Nowhere else does, it's otherwise simply set to an
> appropriately incremented index by cpu_exec_init().)
> I rather suspect that cpu_index will already be set
> to the value we set it, in any case...
Most likely, but I'd rather not touch that logic for 2.0 :)
Alex
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH 2.0] PPC: E500: Set PIR default reset value rather than SPR value
2014-04-03 18:58 ` Alexander Graf
@ 2014-04-04 0:26 ` Alexey Kardashevskiy
2014-04-04 6:50 ` Alexander Graf
0 siblings, 1 reply; 7+ messages in thread
From: Alexey Kardashevskiy @ 2014-04-04 0:26 UTC (permalink / raw)
To: Alexander Graf, Peter Maydell
Cc: Frederic Konrad, qemu-ppc@nongnu.org, QEMU Developers, Greg Kurz,
Andreas Färber
On 04/04/2014 05:58 AM, Alexander Graf wrote:
>
> On 03.04.2014, at 20:55, Peter Maydell <peter.maydell@linaro.org> wrote:
>
>> On 3 April 2014 19:48, Alexander Graf <agraf@suse.de> wrote:
>>> We now reset SPRs to their reset values on CPU reset. So if we want
>>> to have an SPR persistently changed, we need to change its default
>>> reset value rather than the value itself manually.
>>>
>>> Do this for SPR_BOOKE_PIR, fixing e500v2 SMP boot.
>>>
>>> Reported-by: Frederic Konrad <fred.konrad@greensocs.com>
>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>> ---
>>> hw/ppc/e500.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
>>> index d7ba25f..f984b3e 100644
>>> --- a/hw/ppc/e500.c
>>> +++ b/hw/ppc/e500.c
>>> @@ -649,7 +649,7 @@ void ppce500_init(QEMUMachineInitArgs *args, PPCE500Params *params)
>>> input = (qemu_irq *)env->irq_inputs;
>>> irqs[i][OPENPIC_OUTPUT_INT] = input[PPCE500_INPUT_INT];
>>> irqs[i][OPENPIC_OUTPUT_CINT] = input[PPCE500_INPUT_CINT];
>>> - env->spr[SPR_BOOKE_PIR] = cs->cpu_index = i;
>>> + env->spr_cb[SPR_BOOKE_PIR].default_value = cs->cpu_index = i;
>>
>> Not an issue introduced with this patch, but should we really
>> be missing with CPUState::cpu_index in a board file?
>> (Nowhere else does, it's otherwise simply set to an
>> appropriately incremented index by cpu_exec_init().)
>> I rather suspect that cpu_index will already be set
>> to the value we set it, in any case...
>
> Most likely, but I'd rather not touch that logic for 2.0 :)
git grep "cpu_index\s\+=[^=]":
cpus.c:1404: cpu_index = 0;
exec.c:482: cpu_index = 0;
exec.c:486: cpu->cpu_index = cpu_index;
hmp.c:742: cpu_index = qdict_get_int(qdict, "index");
hw/ppc/e500.c:652: env->spr[SPR_BOOKE_PIR] = cs->cpu_index = i;
monitor.c:2225: int cpu_index = qdict_get_int(qdict, "cpu_index");
scripts/qmp/qmp-shell:183: self.__cpu_index = 0
scripts/qmp/qmp-shell:216: def __cmd_passthrough(self, cmdline,
cpu_index = 0):
scripts/qmp/qmp-shell:229: self.__cpu_index = idx
Assigning cpu_index in e500.c looks to me as a strange idea. And it is
hidden so nicely with two "=" in a row :-/
Out of curiosity - in the real e500 hardware, PIR does not get reset on CPU
reset?
--
Alexey
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH 2.0] PPC: E500: Set PIR default reset value rather than SPR value
2014-04-04 0:26 ` Alexey Kardashevskiy
@ 2014-04-04 6:50 ` Alexander Graf
0 siblings, 0 replies; 7+ messages in thread
From: Alexander Graf @ 2014-04-04 6:50 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: Peter Maydell, QEMU Developers, qemu-ppc@nongnu.org, Greg Kurz,
Andreas Färber, Frederic Konrad
> Am 04.04.2014 um 02:26 schrieb Alexey Kardashevskiy <aik@ozlabs.ru>:
>
>> On 04/04/2014 05:58 AM, Alexander Graf wrote:
>>
>>> On 03.04.2014, at 20:55, Peter Maydell <peter.maydell@linaro.org> wrote:
>>>
>>>> On 3 April 2014 19:48, Alexander Graf <agraf@suse.de> wrote:
>>>> We now reset SPRs to their reset values on CPU reset. So if we want
>>>> to have an SPR persistently changed, we need to change its default
>>>> reset value rather than the value itself manually.
>>>>
>>>> Do this for SPR_BOOKE_PIR, fixing e500v2 SMP boot.
>>>>
>>>> Reported-by: Frederic Konrad <fred.konrad@greensocs.com>
>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>> ---
>>>> hw/ppc/e500.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
>>>> index d7ba25f..f984b3e 100644
>>>> --- a/hw/ppc/e500.c
>>>> +++ b/hw/ppc/e500.c
>>>> @@ -649,7 +649,7 @@ void ppce500_init(QEMUMachineInitArgs *args, PPCE500Params *params)
>>>> input = (qemu_irq *)env->irq_inputs;
>>>> irqs[i][OPENPIC_OUTPUT_INT] = input[PPCE500_INPUT_INT];
>>>> irqs[i][OPENPIC_OUTPUT_CINT] = input[PPCE500_INPUT_CINT];
>>>> - env->spr[SPR_BOOKE_PIR] = cs->cpu_index = i;
>>>> + env->spr_cb[SPR_BOOKE_PIR].default_value = cs->cpu_index = i;
>>>
>>> Not an issue introduced with this patch, but should we really
>>> be missing with CPUState::cpu_index in a board file?
>>> (Nowhere else does, it's otherwise simply set to an
>>> appropriately incremented index by cpu_exec_init().)
>>> I rather suspect that cpu_index will already be set
>>> to the value we set it, in any case...
>>
>> Most likely, but I'd rather not touch that logic for 2.0 :)
>
> git grep "cpu_index\s\+=[^=]":
>
> cpus.c:1404: cpu_index = 0;
> exec.c:482: cpu_index = 0;
> exec.c:486: cpu->cpu_index = cpu_index;
> hmp.c:742: cpu_index = qdict_get_int(qdict, "index");
> hw/ppc/e500.c:652: env->spr[SPR_BOOKE_PIR] = cs->cpu_index = i;
> monitor.c:2225: int cpu_index = qdict_get_int(qdict, "cpu_index");
> scripts/qmp/qmp-shell:183: self.__cpu_index = 0
> scripts/qmp/qmp-shell:216: def __cmd_passthrough(self, cmdline,
> cpu_index = 0):
> scripts/qmp/qmp-shell:229: self.__cpu_index = idx
>
>
> Assigning cpu_index in e500.c looks to me as a strange idea. And it is
> hidden so nicely with two "=" in a row :-/
>
> Out of curiosity - in the real e500 hardware, PIR does not get reset on CPU
> reset?
On real hw PIR is a hardwired read-only register :)
Alex
>
>
> --
> Alexey
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH 2.0] PPC: E500: Set PIR default reset value rather than SPR value
2014-04-03 18:48 [Qemu-devel] [PATCH 2.0] PPC: E500: Set PIR default reset value rather than SPR value Alexander Graf
2014-04-03 18:50 ` Andreas Färber
2014-04-03 18:55 ` Peter Maydell
@ 2014-04-04 8:18 ` Frederic Konrad
2 siblings, 0 replies; 7+ messages in thread
From: Frederic Konrad @ 2014-04-04 8:18 UTC (permalink / raw)
To: Alexander Graf, qemu-devel
Cc: Alexey Kardashevskiy, Peter Maydell, qemu-ppc, afaerber,
Greg Kurz
Hi Alex,
Seems to works for me ;).
On 03/04/2014 20:48, Alexander Graf wrote:
> We now reset SPRs to their reset values on CPU reset. So if we want
> to have an SPR persistently changed, we need to change its default
> reset value rather than the value itself manually.
>
> Do this for SPR_BOOKE_PIR, fixing e500v2 SMP boot.
>
> Reported-by: Frederic Konrad <fred.konrad@greensocs.com>
> Signed-off-by: Alexander Graf <agraf@suse.de>
Tested-by: KONRAD Frederic <fred.konrad@greensocs.com>
Thanks,
Fred
> ---
> hw/ppc/e500.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index d7ba25f..f984b3e 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -649,7 +649,7 @@ void ppce500_init(QEMUMachineInitArgs *args, PPCE500Params *params)
> input = (qemu_irq *)env->irq_inputs;
> irqs[i][OPENPIC_OUTPUT_INT] = input[PPCE500_INPUT_INT];
> irqs[i][OPENPIC_OUTPUT_CINT] = input[PPCE500_INPUT_CINT];
> - env->spr[SPR_BOOKE_PIR] = cs->cpu_index = i;
> + env->spr_cb[SPR_BOOKE_PIR].default_value = cs->cpu_index = i;
> env->mpic_iack = MPC8544_CCSRBAR_BASE +
> MPC8544_MPIC_REGS_OFFSET + 0xa0;
>
^ permalink raw reply [flat|nested] 7+ messages in thread