Re: [Qemu-devel] [PATCH v6 01/17] Introduce stub routine cpu_desc_avail

[Michael Mueller]

On Wed, 6 May 2015 14:06:16 -0300
Eduardo Habkost <address@hidden> wrote:

> On Wed, May 06, 2015 at 06:23:05PM +0200, Michael Mueller wrote:
> > On Wed, 6 May 2015 08:23:32 -0300
> > Eduardo Habkost <address@hidden> wrote:
> [...]
> > > > > >    cpudef_init();
> > > > > > 
> > > > > >    if (cpu_model && cpu_desc_avail() && is_help_option(cpu_model)) {
> > > > > >        list_cpus(stdout, &fprintf, cpu_model);
> > > > > >        exit(0);
> > > > > >    }
> > > > > > 
> > > > > > That is because the output does not solely depend on static 
> > > > > > definitions
> > > > > > but also on runtime context. Here the host machine type this 
> > > > > > instance of
> > > > > > QEMU is running on, at least for the KVM case.
> > > > > 
> > > > > Is this a required feature? I would prefer to have the main() code
> > > > > simple even if it means not having runnable information in "-cpu ?" by
> > > > > now (about possible ways to implement this without cpu_desc_avail(), 
> > > > > see
> > > > > below).
> > > > 
> > > > I think it is more than a desired feature because one might end up with 
> > > > a failed
> > > > CPU object instantiation although the help screen claims to CPU model 
> > > > to be valid. 
> > > 
> > > I think you are more likely to confuse users by not showing information
> > > on "-cpu ?" when -machine is not present. I believe most people use
> > > "-cpu ?" with no other arguments, to see what the QEMU binary is capable
> > > of.
> > 
> > I don't disagree with that, both cases are to some extend confusing...
> > But the accelerator makes a big difference and a tended user should really 
> > be aware
> > of that.
> > 
> > Also that TCG is the default:
> > 
> > $ ./s390x-softmmu/qemu-system-s390x -cpu ?
> > s390             host
> > 
> > And I don't see a way to make a user belief that all the defined CPU models 
> > are available to
> > a TCG user in the S390 case where most of the CPU facilities are not 
> > implemented.
> 
> Well, we could simply add a "KVM required" note (maybe just an asterisk beside
> the CPU model description). But maybe we have a reasonable alternative below:
> 
> > 
> > > 
> > > Anyway, whatever we decide to do, I believe we should start with
> > > something simple to get things working, and after that we can look for
> > > ways improve the help output with "runnable" info.
> > 
> > I don't see how to solve this without cpu_desc_avail() or some other 
> > comparable mechanism, the
> > aliases e.g. are also dynamic...
> 
> What bothers me in cpu_desc_avail() is that it depends on global state that is
> non-trivial (one needs to follow the whole KVM initialization path to find out
> if cpu_desc_avail() will be true or false).
> 
> We could instead simply skip the cpu_list() call unconditionally on s390. 
> e.g.:
> 
> target-s390x/cpu.h:
>     /* Delete the existing cpu_list macro */
> 
> cpus.c:
>     int list_cpus(FILE *f, fprintf_function cpu_fprintf, const char *optarg)
>     {
>     #if defined(cpu_list)
>         cpu_list(f, cpu_fprintf);
>         return 1;
>     #else
>         return 0;
>     #endif
>     }
> 
> vl.c:
>     if (cpu_model && is_help_option(cpu_model)) {
>         /* zero list_cpus() return value means "-cpu ?" will be
>          * handled later by machine initialization code */
>         if (list_cpus(stdout, &fprintf, cpu_model)) {
>             exit(0);
>         }
>     }

That approach is will do the job as well. I will prepare a patch for the next 
version.

Thanks!

> 
> [...]
> > > 
> > > About "-cpu ?": do we really want it to depend on -machine processing?
> > > Today, help output shows what the QEMU binary is capable of, not just
> > > what the host system and -machine option are capable of.
> > 
> > I think we have to take it into account because the available CPU models 
> > might
> > deviate substantially as in the case for S390 for KVM and TCG.
> 
> That's true, on s390 the set of available CPU models is very different on both
> cases. It breaks assumptions in the existing "-cpu ?" handling code in main().
> 
> >  
> > > 
> > > If we decide to change that assumption, let's do it in a generic way and
> > > not as a arch-specific hack. The options I see are:
> > 
> > welcome
> > 
> > > 
> > > 1) Continue with the current policy where "-cpu ?" does not depend on
> > >    -machine arguments, and show all CPU models on "-cpu ?".
> > > 2) Deciding that, yes, it is OK to make "-cpu ?" depend on -machine
> > >    arguments, and move the list_cpus() call after machine initialization
> > >    inside generic main() code for all arches.
> > >    2.1) We could delay the list_cpus() call inside main() on all cases.
> > >    2.2) We could delay the list_cpus() call inside main() only if
> > >         an explicit -machine option is present.
> > > 
> > > I prefer (1) and my second choice would be (2.2), but the main point is
> > > that none of the options above require making s390 special and
> > > introducing cpu_desc_avail().
> > 
> > My take here is 2.1 because omitting option -machine is a decision to some
> > defaults for machine type and accelerator type already.  
> 
> The problem with 2.1 is that some machine init functions require that
> additional command-line parameters are set and will abort (e.g. mips 
> machines).
> So we can't do that unconditionally for all architectures.
> 
> The proposal above is like 2.1, but conditional: it will delay "-cpu ?"
> handling only on architectures that don't define cpu_list().

perfect.

Michael

>