From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54023) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YJ7bO-0008LM-E3 for qemu-devel@nongnu.org; Wed, 04 Feb 2015 16:35:19 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YJ7bK-0005i2-Dy for qemu-devel@nongnu.org; Wed, 04 Feb 2015 16:35:14 -0500 Received: from mail-wg0-x234.google.com ([2a00:1450:400c:c00::234]:35035) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YJ7bK-0005hn-0v for qemu-devel@nongnu.org; Wed, 04 Feb 2015 16:35:10 -0500 Received: by mail-wg0-f52.google.com with SMTP id y19so4155705wgg.11 for ; Wed, 04 Feb 2015 13:35:09 -0800 (PST) Message-ID: <54D29089.2050606@gmail.com> Date: Wed, 04 Feb 2015 23:35:05 +0200 From: Marcel Apfelbaum MIME-Version: 1.0 References: <1423064635-19045-1-git-send-email-marcel@redhat.com> <54D276F4.7050400@de.ibm.com> In-Reply-To: <54D276F4.7050400@de.ibm.com> Content-Type: text/plain; charset=iso-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 0/8] machine: query machine properties rather than qemu opts Reply-To: marcel@redhat.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Christian Borntraeger , Marcel Apfelbaum , qemu-devel@nongnu.org Cc: peter.maydell@linaro.org, peter.crosthwaite@xilinx.com, james.hogan@imgtec.com, mst@redhat.com, jan.kiszka@siemens.com, agraf@suse.de, scottwood@freescale.com, cornelia.huck@de.ibm.com, pbonzini@redhat.com, leon.alrae@imgtec.com, aurelien@aurel32.net On 02/04/2015 09:45 PM, Christian Borntraeger wrote: > Am 04.02.2015 um 16:43 schrieb Marcel Apfelbaum: >> Commit e79d5a6 ("machine: remove qemu_machine_opts global list") removed option >> descriptions from the -machine QemuOptsList to avoid repeating MachineState's QOM properties. >> >> This results in a Qemu crash if a non string option is queried using qemu opts. >> Fix this by querying machine properties through designated wrappers. >> >> I hope I didn't miss anything. >> Comments are appreciated as always. >> >> Thanks, >> Marcel >> >> Marcel Apfelbaum (8): >> machine: query iommu machine property rather than qemu opts >> hw/machine: kernel-irqchip property support for allowed/required >> machine: query kernel-irqchip machine property rather than qemu opts >> kvm: add machine state to kvm_arch_init >> machine: query kvm-shadow-mem machine property rather than qemu opts >> machine: query phandle-start machine property rather than qemu opts >> machine: query dump-guest-core machine property rather than qemu opts >> machine: query mem-merge machine property rather than qemu opts > > In general this seems to work. Thanks! > I have a question, though: > > What I like is a way to do some wrappers in the specific machines. > For example, we plan to add > > static inline void s390_machine_initfn(Object *obj) > { > object_property_add_bool(obj, "aes-key-wrap", > machine_get_aes_key_wrap, > machine_set_aes_key_wrap, NULL); > object_property_set_description(obj, "aes-key-wrap", > "enable/disable AES key wrapping using the CPACF wrapping key", > NULL); > object_property_add_bool(obj, "dea-key-wrap", > machine_get_dea_key_wrap, > machine_set_dea_key_wrap, NULL); > object_property_set_description(obj, "dea-key-wrap", > "enable/disable DEA key wrapping using the CPACF wrapping key", > NULL); > } OK, You add 2 QOM properties to TYPE_S390_CCW_MACHINE instances. Of course your setters/getters need to save the property values into an actual field, so you will need a S390State (derived from MachineState) typedef struct { MachineState parent; bool aes_key_wrap; ... } S390State > > > > Previously we used > if (qemu_opt_get_bool(opts, "aes-key-wrap", false)) > if target-s390/kvm.c > to query that. > > Now, these options are pretty specific to s390 and adding them to hw/core/machine.c > to create wrappers seems strange. Completely agree. This is the reason we wanted options(properties) per machine type. So implementing them in hw/s390/s390-virtio-ccw.c > seems a much better place. Indeed. > Would a function there that does gets S390_CCW_MACHINE(current_machine)->aes_key_wrap > considered ok, or do we need to pollute hw/core/machine.c with architecture specific > options? We surely don't add this to hw/core/machine.c because is specific to TYPE_S390_CCW_MACHINE. Let's say you want to use this property in kvm_arch_init of target-s390x/kvm.c. - After this series you already have an instance of MachineState initialized with your new properties. - My assumption is that TYPE_S390_CCW_MACHINE is the only s390 machine or the base type of all s390 machines. You have three options here: 1. Use QOM infrastucture: bool aes_key_wrap = object_property_get_bool(OBJECT(machine), "aes-key-wrap", NULL); 2. Add a wrapper somewhere in include/hw/s390x/ that gets MachineState, cast it into S390State and return the field value. 3. Directly downcast MachineState to S390State and get the value. * All of the above use my assumption that all s390 machines derive from this one. > > Christian > > PS: The same is somewhat true for qemu-options.hx. Having such specific machine option > in the global help offers room for improvement Can you please elaborate? I didn't fully understand what exactly are you referring to. Hope I helped, Marcel > >