From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54585) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YpxQb-0004m8-2C for qemu-devel@nongnu.org; Wed, 06 May 2015 07:23:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YpxQV-0003Jo-Qv for qemu-devel@nongnu.org; Wed, 06 May 2015 07:23:48 -0400 Received: from mx1.redhat.com ([209.132.183.28]:49240) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YpxQV-0003Jj-Ir for qemu-devel@nongnu.org; Wed, 06 May 2015 07:23:43 -0400 Date: Wed, 6 May 2015 08:23:32 -0300 From: Eduardo Habkost Message-ID: <20150506112332.GZ17796@thinpad.lan.raisama.net> References: <1430146411-34632-1-git-send-email-mimu@linux.vnet.ibm.com> <1430146411-34632-2-git-send-email-mimu@linux.vnet.ibm.com> <20150505135547.GR17796@thinpad.lan.raisama.net> <20150505181216.5b754d24@bee> <20150505174101.GV17796@thinpad.lan.raisama.net> <20150506111720.46e93edd@bee> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150506111720.46e93edd@bee> Subject: Re: [Qemu-devel] [PATCH v6 01/17] Introduce stub routine cpu_desc_avail List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Michael Mueller Cc: Cornelia Huck , Gleb Natapov , qemu-devel@nongnu.org, Alexander Graf , Christian Borntraeger , "Jason J. Herne" , Daniel Hansel , Paolo Bonzini , Andreas Faerber , Richard Henderson On Wed, May 06, 2015 at 11:17:20AM +0200, Michael Mueller wrote: > On Tue, 5 May 2015 14:41:01 -0300 > Eduardo Habkost wrote: > > > On Tue, May 05, 2015 at 06:12:16PM +0200, Michael Mueller wrote: > > > On Tue, 5 May 2015 10:55:47 -0300 > > > Eduardo Habkost wrote: > > > > > > > On Mon, Apr 27, 2015 at 04:53:15PM +0200, Michael Mueller wrote: > > > > > This patch introduces the function cpu_desc_avail() which returns by > > > > > default true if not architecture specific implemented. Its intention > > > > > is to indicate if the cpu model description is available for display > > > > > by list_cpus(). This change allows cpu model descriptions to become > > > > > dynamically created by evaluating the runtime context instead of > > > > > putting static cpu model information at display. > > > > > > > > Why are you deliberately breaking "-cpu ?" when cpu_desc_avail() is > > > > false? > > > > > > In the s390x case cpu_desc_avail() is per se false in this code section of vl.c: > > > > > > /* Init CPU def lists, based on config > > > * - Must be called after all the qemu_read_config_file() calls > > > * - Must be called before list_cpu() > > > * - Must be called before machine->init() > > > */ > > > > (Side note: I believe the above outdated, I will send a patch to update > > it.) > > Will be interesting to see what the change is, master is currently showing this code. We don't (and shouldn't) have the qemu_read_config_file() requirement, as CPU models are not loaded from config files anymore. And we should be able to eliminate cpudef_init() completely, soon. I think the only user of cpudef_init() is x86. > > > > > > 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. 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. > > > > > > > > > > > Once the accelerator has been initialized AND the S390 cpu classes have > > > been setup by means of the following code: > > > > > > static void kvm_setup_cpu_classes(KVMState *s) > > > { > > > S390MachineProps mach; > > > > > > if (!kvm_s390_get_machine_props(s, &mach)) { > > > s390_setup_cpu_classes(ACCEL_CURRENT, &mach, > > > s390_current_fac_list_mask()); > > > s390_setup_cpu_aliases(); > > > cpu_classes_initialized = true; > > > } > > > } > > > > > > cpu_desc_avail() becomes true. In case the selceted mode was "?" > > > the list_cpu() is now done right before the cpu model is used as part > > > of the cpu initialization (hw/s390-virtio.c): > > > > > > void s390_init_cpus(const char *cpu_model, uint8_t *storage_keys) > > > { > > > int i; > > > > > > if (cpu_model == NULL) { > > > cpu_model = "none"; > > > } > > > > > > if (is_help_option(cpu_model)) { > > > list_cpus(stdout, &fprintf, cpu_model); > > > exit(0); > > > } > > > > > > ... > > > for (i = 0; i < smp_cpus; i++) { > > > ... > > > cpu = cpu_s390x_init(cpu_model); > > > ... > > > } > > > } > > > > > > In other words, you just need to ensure that s390_cpu_list() run after > > kvm_setup_cpu_classes(). > > ... which is part of the KVM/accel init process but it could of course make use > of the ACCEL_TMP use case as query-cpu-definitions does. Exactly. I don't see a reason to not share code between query-cpu-definitions and "-cpu ?". > > > > > Can't you simply call s390_setup_cpu_classes(ACCEL_TEMP) inside > > s390_init_cpus(), just like arch_query_cpu_definitions()? You could even > > share code between both functions. > > That would not help with the current placement of list_cpus() in main() as it happens > way to early. Not s390_init_cpus() is the issue, the context information has been > processed already at that time. Currently I just kind of delay the list_cpus() until > all required information is available. I understand you are just delaying it. But it is requiring a hack inside generic main() code that could be avoided. About the actual reasons to delay it, see below. > > > > > (In the future, we should be able to implement "-cpu ?" by simply > > calling the query-cpu-definitions implementation.) > > Right but the -machine ,accel= options have to be processed already. About query-cpu-definitions: its code shouldn't depend on -machine options to work. Maybe it can use current_machine to get the default accel and machine argument, but it should always output the same data for a machine+accel combination even if you use "-machine none". 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. 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: 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(). -- Eduardo