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

[Igor Mammedov]

On Wed, 4 Oct 2017 14:39:20 -0700
Alistair Francis <address@hidden> wrote:

> On Wed, Oct 4, 2017 at 9:34 AM, Eduardo Habkost <address@hidden> wrote:
> > On Wed, Oct 04, 2017 at 03:08:16PM +0200, Igor Mammedov wrote:  
> >> 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.  
> >
> > It's up to the board maintainer to decide what's the best option.
> > Both features are independent from each other and can be
> > implemented by machine core.  
> 
> Noooo!
> 
> My hope with this series is that eventually we could hit a state where
> every single machine acts the same way with the -cpu option.
> 
> I really don't like what we do now where some boards use it, some
> boards error and some boars just ignore the option. I think we should
> agree on something and every machine should follow the same flow so
> that users know what to expect when they use the -cpu option.
> 
> If this means we allow machines to specify they don't support the
> option or only have a single element in the list of supported options
> doesn't really matter, but all machines should do the same thing.
> 
> >
> > In either case, the valid_cpu_types feature will be still very
> > useful for boards like pxa270 and sa1110, which support -cpu but
> > only with specific families of CPU types (grep for
> > "strncmp(cpu_type").
> >  
> >>
> >> 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) {  
> >
> > This sounds good to me, even if we don't reject -cpu on any
> > board.
> >
> >  
> >>
> >> 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");  
> >
> > I won't complain too much if a board maintainer really wants to
> > make the board reject -cpu completely, but it's up to them.  
> 
> I disagree. I think a standard way of doing it is better. At least for
> each architecture. The ARM -cpu option is very confusing at the moment
> and it really doesn't need to be that bad.
> 
> >
> > Personally, I'd prefer to have all boards setting
> > default_cpu_type even if they support only one CPU model, so
> > clients don't need a special case for boards that don't support
> > -cpu.  
> 
> I agree, I think having one CPU makes more sense. It makes it easier
> to add support for more cpus in the future and allows the users to use
> the -cpu option without killing QEMU.
I'm considering -cpu option as a legacy one that server 2 purposes now
 1: pick cpu type for running instance
 2: convert optional features/legacy syntax to global properties
    for related cpu type

It plays ok for machines with single type of cpu but doesn't really scale
to more and doesn't work well nor needed if we were to specify cpus on CLI
with -device (i.e. build machine from config/CLI)

So I would not extend usage '-cpu' to boards that have fixed cpu type,
because it really useless in that case and confuses users with idea that
they have ability/need to specify -cpu on fixed cpu board.

I'd be upfront with users and fail explicitly if -cpu is not supported
(yes, it is not uniform CLI behavior across machines but it makes
sense since not all machines are the same, there probably are other
options with which some machines error out with unsupported error,
-cpu is not any different case).

> 
> Thanks,
> Alistair
> 
> >
> > --
> > Eduardo
> >