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

[Igor Mammedov]

On Wed, 4 Oct 2017 09:28:51 -0300
Eduardo Habkost <address@hidden> wrote:

> On Wed, Oct 04, 2017 at 01:12:32PM +0200, Igor Mammedov wrote:
> > On Tue, 3 Oct 2017 14:41:17 -0700
> > Alistair Francis <address@hidden> wrote:
> >   
> > > On Tue, Oct 3, 2017 at 1:36 PM, Eduardo Habkost <address@hidden> wrote:  
> > > > On Tue, Oct 03, 2017 at 01:05:13PM -0700, Alistair Francis wrote:    
> > > >> List all possible valid CPU options.
> > > >>
> > > >> Signed-off-by: Alistair Francis <address@hidden>
> > > >> ---
> > > >>
> > > >>  hw/arm/xlnx-zcu102.c         | 10 ++++++++++
> > > >>  hw/arm/xlnx-zynqmp.c         | 16 +++++++++-------
> > > >>  include/hw/arm/xlnx-zynqmp.h |  1 +
> > > >>  3 files changed, 20 insertions(+), 7 deletions(-)
> > > >>
> > > >> diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c
> > > >> index 519a16ed98..039649e522 100644
> > > >> --- a/hw/arm/xlnx-zcu102.c
> > > >> +++ b/hw/arm/xlnx-zcu102.c
> > > >> @@ -98,6 +98,8 @@ static void xlnx_zynqmp_init(XlnxZCU102 *s, 
> > > >> MachineState *machine)
> > > >>      object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),
> > > >>                                &error_abort);
> > > >>
> > > >> +    object_property_set_str(OBJECT(&s->soc), machine->cpu_type, 
> > > >> "cpu-type",
> > > >> +                            &error_fatal);    
> > > >
> > > > Do you have plans to support other CPU types to xlnx_zynqmp in
> > > > the future?  If not, I wouldn't bother adding the cpu-type
> > > > property and the extra boilerplate code if it's always going to
> > > > be set to cortex-a53.    
> > > 
> > > No, it'll always be A53.
> > > 
> > > I did think of that, but I also wanted to use the new option! I also
> > > think there is an advantage in sanely handling users '-cpu' option,
> > > before now we just ignored it, so I think it still does give a
> > > benefit. That'll be especially important on the Xilinx tree (sometimes
> > > people use our machines with a different CPU to 'benchmark' or test
> > > other CPUs with our CoSimulation setup). So I think it does make sense
> > > to keep in.  
> > if cpu isn't user settable, one could just outright die if cpu_type
> > is not NULL and say that user's CLI is wrong.
> > (i.e. don't give users illusion that they allowed to use '-cpu')  
> 
> Isn't it exactly what this patch does, by setting:
>     mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a53");
>     mc->valid_cpu_types = xlnx_zynqmp_valid_cpus;
> ?
> 
> Except that "-cpu cortex-a53" won't die, which is a good thing.
allowing "-cpu cortex-a53" here, would allow to use feature parsing
which weren't allowed or were ignored before if user supplied '-cpu'.
so I'd more strict and refuse any -cpu and break CLI that tries to use it
if board has non configurable cpu type. It would be easier to relax
restriction later if necessary.

using validate_cpus here just to have users for the new code,
doesn't seem like valid justification and at that it makes board
code more complex where it's not necessary and build in cpu type
works just fine.

wrt centralized way to refuse -cpu if board doesn't support it,
(which is not really related to this series) following could be done:

when cpu_model removal is completely done I plan to replace
  vl.c
     cpu_parse_cpu_model(machine_class->default_cpu_type, cpu_model)
with
     cpu_parse_cpu_model(DEFAULT_TARGET_CPU_TYPE, cpu_model)

so that we could drop temporary guard

     if (machine_class->default_cpu_type) {

with that it would be possible to tell from machine_run_board_init()
that board doesn't provide cpu but user provided '-cpu'
so we would be able to:
  if ((machine_class->default_cpu_type == NULL) &&
      (machine->cpu_type != NULL))
          error_fatal("machine doesn't support -cpu option");