Re: [Qemu-devel] [PATCH v3 4/5] xlnx-zcu102: Specify the valid CPUs

[Igor Mammedov]

On Thu, 26 Oct 2017 11:59:16 +0200
Alistair Francis <address@hidden> wrote:

> On Mon, Oct 23, 2017 at 1:14 PM, Philippe Mathieu-Daudé <address@hidden> 
> wrote:
> > On 10/17/2017 07:31 PM, Alistair Francis wrote:  
> >> List all possible valid CPU options.
> >>
> >> Signed-off-by: Alistair Francis <address@hidden>
> >> ---
> >>
> >> An implementation for single CPU machines is still being discussed. A
> >> solution proposed by Eduardo is this:
> >>
> >> 1) Change the default on TYPE_MACHINE to:
> >>      mc->valid_cpu_types = { TYPE_CPU, NULL };
> >>
> >>    This will keep the existing behavior for all boards.
> >>
> >> 2) mc->valid_cpu_types=NULL be interpreted as "no CPU model
> >>    except the default is accepted" or "-cpu is not accepted" in
> >>    machine_run_board_init() (I prefer the former, but both
> >>    options would be correct)
> >>
> >> 3) Boards like xlnx_zynqmp could then just do this:
> >>
> >>    static void xxx_class_init(...) {
> >>        mc->default_cpu_type = MY_CPU_TYPE;
> >>        /* Reason: XXX_init() is hardcoded to MY_CPU_TYPE */
> >>        mc->valid_cpu_types = NULL;
> >>    }
> >>
> >> V3:
> >>  - Make variable static
> >> V2:
> >>  - Don't use the users -cpu
> >>  - Fixup allignment
> >>
> >>  hw/arm/xlnx-zcu102.c | 17 +++++++++++++++++
> >>  1 file changed, 17 insertions(+)
> >>
> >> diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c
> >> index 519a16ed98..d5a5425356 100644
> >> --- a/hw/arm/xlnx-zcu102.c
> >> +++ b/hw/arm/xlnx-zcu102.c
> >> @@ -160,6 +160,11 @@ static void xlnx_zynqmp_init(XlnxZCU102 *s, 
> >> MachineState *machine)
> >>      arm_load_kernel(s->soc.boot_cpu_ptr, &xlnx_zcu102_binfo);
> >>  }
> >>
> >> +static const char *xlnx_zynqmp_valid_cpus[] = {
> >> +                                            
> >> ARM_CPU_TYPE_NAME("cortex-a53"),
> >> +                                            NULL
> >> +                                              };  
> >
> > Why so many spaces? :) Maybe Peter can clean it when applying.
> >
> > static const char *xlnx_zynqmp_valid_cpus[] = {
> >     ARM_CPU_TYPE_NAME("cortex-a53"),
> >     NULL
> > };
+1 to what Philippe is suggesting, this is common pattern in QEMU code.

> The spaces are there to keep the elements under the array (while being
> less then 80 characters).
it's not aligned on 4SP from the beginning or 4SP after {}
so it's not exactly matching codding style and catches eye.
Also in case one would need to add an extra cpu there with
longer name beyond 80chr, one would need to move the rest
elements to new alignment.

the same applies for 4-5/5,
I'd would repost fixed up series.

> 
> Alistair
> 
> >
> > Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
> >  
> >> +
> >>  static void xlnx_ep108_init(MachineState *machine)
> >>  {
> >>      XlnxZCU102 *s = EP108_MACHINE(machine);
> >> @@ -185,6 +190,12 @@ static void xlnx_ep108_machine_class_init(ObjectClass 
> >> *oc, void *data)
> >>      mc->block_default_type = IF_IDE;
> >>      mc->units_per_default_bus = 1;
> >>      mc->ignore_memory_transaction_failures = true;
> >> +    mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a53");
> >> +    /* The ZynqMP SoC is always a Cortex-A53. We add this here to give
> >> +     * users a sane error if they specify a different CPU, but we never
> >> +     * use their CPU choice.
> >> +     */
> >> +    mc->valid_cpu_types = xlnx_zynqmp_valid_cpus;
> >>  }
> >>
> >>  static const TypeInfo xlnx_ep108_machine_init_typeinfo = {
> >> @@ -240,6 +251,12 @@ static void 
> >> xlnx_zcu102_machine_class_init(ObjectClass *oc, void *data)
> >>      mc->block_default_type = IF_IDE;
> >>      mc->units_per_default_bus = 1;
> >>      mc->ignore_memory_transaction_failures = true;
> >> +    mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a53");
> >> +    /* The ZynqMP SoC is always a Cortex-A53. We add this here to give
> >> +     * users a sane error if they specify a different CPU, but we never
> >> +     * use their CPU choice.
> >> +     */
> >> +    mc->valid_cpu_types = xlnx_zynqmp_valid_cpus;
> >>  }
> >>
> >>  static const TypeInfo xlnx_zcu102_machine_init_typeinfo = {
> >>