Re: [PATCH v6 06/18] hw/arm/allwinner: add CPU Configuration module

[Peter Maydell]

On Tue, 3 Mar 2020 at 20:15, Niek Linnenbank <address@hidden> wrote:
> On Tue, Mar 3, 2020 at 1:09 PM Alex Bennée <address@hidden> wrote:
>> Niek Linnenbank <address@hidden> writes:
>> > +static void allwinner_cpucfg_cpu_reset(AwCpuCfgState *s, uint8_t cpu_id)
>> > +{
>> > +    int ret;
>> > +
>> > +    trace_allwinner_cpucfg_cpu_reset(cpu_id, s->entry_addr);
>> > +
>> > +    ret = arm_set_cpu_on(cpu_id, s->entry_addr, 0,
>> > +                         CPU_EXCEPTION_LEVEL_ON_RESET, false);
>>
>> According to the arm_set_cpu_on code:
>>
>>     if (!target_aa64 && arm_feature(&target_cpu->env, ARM_FEATURE_AARCH64)) {
>>         /*
>>          * For now we don't support booting an AArch64 CPU in AArch32 mode
>>          * TODO: We should add this support later
>>          */
>>         qemu_log_mask(LOG_UNIMP,
>>                       "[ARM]%s: Starting AArch64 CPU %" PRId64
>>                       " in AArch32 mode is not supported yet\n",
>>                       __func__, cpuid);
>>         return QEMU_ARM_POWERCTL_INVALID_PARAM;
>>     }
>>
>> Do you really want to reboot in aarch32 mode on a reset? If so we should
>> fix the TODO.
>
> The Allwinner H3 has four ARM Cortex-A7 cores and are 32-bit, so in the
> early version I filled the @target_aa64 parameter with false to
> keep it in 32-bit mode. And for usage in the current Allwinner H3 SoC
> that is sufficient, but as soon as a potential new SoC implementation
> with a 64-bit CPU starts to use this module, the hardcoded @target_aa64
> parameter will become a problem.

> Maybe I should just use some function to check if the current emulated
> CPU its 32-bit or 64-bit and use that for @target_aa64?

If the intended behaviour is "just power on the CPU into EL3
in the aarch32/aarch64 state it would naturally have out of reset"
then you can get the right value to pass to target_aa64 like this:

   ARMCPU *target_cpu = ARM_CPU(arm_get_cpu_by_id(cpu_id));

   if (!target_cpu) {
       /*
        * handle the case of "guest wrote bogus value to RST_CTRL
        * field, probably by returning doing nothing
        */
   }
   target_aa64 = arm_feature(&target_cpu->env, ARM_FEATURE_AARCH64);

Or if the required behaviour for a 64-bit CPU is more complicated
you can just assert or something with a comment that this would need
updating if we ever modelled a 64-bit Allwinner SoC.

thanks
-- PMM