[Qemu-devel] [PATCH 2.0] PPC: E500: Set PIR default reset value rather than SPR value

[Qemu-devel] [PATCH 2.0] PPC: E500: Set PIR default reset value rather than SPR value

[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>
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;
 
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 2.0] PPC: E500: Set PIR default reset value rather than SPR value

[Andreas Färber]

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

Re: [Qemu-devel] [PATCH 2.0] PPC: E500: Set PIR default reset value rather than SPR value

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH 2.0] PPC: E500: Set PIR default reset value rather than SPR value

[Alexander Graf]

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

Re: [Qemu-devel] [PATCH 2.0] PPC: E500: Set PIR default reset value rather than SPR value

[Alexey Kardashevskiy]

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

Re: [Qemu-devel] [PATCH 2.0] PPC: E500: Set PIR default reset value rather than SPR value

[Alexander Graf]

> 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

Re: [Qemu-devel] [PATCH 2.0] PPC: E500: Set PIR default reset value rather than SPR value

[Frederic Konrad]

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;
>