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

[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?


-- 
Alexey