From patchwork Mon Jun 19 16:57:33 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mark Cave-Ayland X-Patchwork-Id: 777872 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3wrxxn43lYz9s89 for ; Tue, 20 Jun 2017 02:58:21 +1000 (AEST) Received: from localhost ([::1]:43542 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dN00I-0001I3-Uu for incoming@patchwork.ozlabs.org; Mon, 19 Jun 2017 12:58:18 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59359) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dMzzz-0001Hr-S9 for qemu-devel@nongnu.org; Mon, 19 Jun 2017 12:58:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dMzzx-0003ZN-5s for qemu-devel@nongnu.org; Mon, 19 Jun 2017 12:57:59 -0400 Received: from chuckie.co.uk ([82.165.15.123]:33523 helo=s16892447.onlinehome-server.info) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1dMzzw-0003XW-V1 for qemu-devel@nongnu.org; Mon, 19 Jun 2017 12:57:57 -0400 Received: from [86.188.254.49] (helo=[172.30.1.185]) by s16892447.onlinehome-server.info with esmtpsa (TLS1.0:DHE_RSA_AES_128_CBC_SHA1:16) (Exim 4.76) (envelope-from ) id 1dMzzq-0007Ck-Sb; Mon, 19 Jun 2017 17:57:52 +0100 To: Eduardo Habkost References: <1497877149-19253-1-git-send-email-mark.cave-ayland@ilande.co.uk> <1497877149-19253-4-git-send-email-mark.cave-ayland@ilande.co.uk> <20170619142852.GS5016@thinpad.lan.raisama.net> From: Mark Cave-Ayland Message-ID: <7d5904ae-e66c-5594-cfff-747578d91f4b@ilande.co.uk> Date: Mon, 19 Jun 2017 17:57:33 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <20170619142852.GS5016@thinpad.lan.raisama.net> X-SA-Exim-Connect-IP: 86.188.254.49 X-SA-Exim-Mail-From: mark.cave-ayland@ilande.co.uk X-SA-Exim-Version: 4.2.1 (built Sun, 08 Jan 2012 02:45:44 +0000) X-SA-Exim-Scanned: Yes (on s16892447.onlinehome-server.info) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x [fuzzy] X-Received-From: 82.165.15.123 Subject: Re: [Qemu-devel] [PATCHv6 3/5] fw_cfg: move assert() and linking of fw_cfg device to the machine into instance_init() X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: peter.maydell@linaro.org, mst@redhat.com, somlo@cmu.edu, qemu-devel@nongnu.org, rjones@redhat.com, pbonzini@redhat.com, imammedo@redhat.com, lersek@redhat.com Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: "Qemu-devel" On 19/06/17 15:28, Eduardo Habkost wrote: > On Mon, Jun 19, 2017 at 01:59:07PM +0100, Mark Cave-Ayland wrote: >> In preparation for calling fw_cfg_init1() during realize rather than during >> init, move the assert() checking for existing fw_cfg devices and the linking >> of the device to the machine with object_property_add_child() to a new >> fw_cfg instance_init() function. >> >> This guarantees that we will still assert() correctly if more than one fw_cfg >> device is instantiated by accident. >> >> Signed-off-by: Mark Cave-Ayland >> Reviewed-by: Laszlo Ersek >> Tested-by: Laszlo Ersek >> --- >> hw/nvram/fw_cfg.c | 14 ++++++++++---- >> 1 file changed, 10 insertions(+), 4 deletions(-) >> >> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c >> index 99bdbc2..af45012 100644 >> --- a/hw/nvram/fw_cfg.c >> +++ b/hw/nvram/fw_cfg.c >> @@ -915,10 +915,6 @@ static void fw_cfg_init1(DeviceState *dev) >> MachineState *machine = MACHINE(qdev_get_machine()); >> uint32_t version = FW_CFG_VERSION; >> >> - assert(!object_resolve_path(FW_CFG_PATH, NULL)); >> - >> - object_property_add_child(OBJECT(machine), FW_CFG_NAME, OBJECT(s), NULL); >> - >> qdev_init_nofail(dev); >> >> fw_cfg_add_bytes(s, FW_CFG_SIGNATURE, (char *)"QEMU", 4); >> @@ -1020,6 +1016,15 @@ FWCfgState *fw_cfg_find(void) >> return FW_CFG(object_resolve_path(FW_CFG_PATH, NULL)); >> } >> >> +static void fw_cfg_init(Object *obj) >> +{ >> + MachineState *machine = MACHINE(qdev_get_machine()); >> + >> + assert(!object_resolve_path(FW_CFG_PATH, NULL)); >> + >> + object_property_add_child(OBJECT(machine), FW_CFG_NAME, obj, NULL); > > I don't think this belongs to instance_init. We must always be > able to instantiate objects without crashing QEMU or affecting > QEMU global state. This patch makes device-list-properties > crash: > > $ qemu-system-x86_64 -display none -qmp unix:/tmp/qmp,server,nowait & > [1] 2848 > $ echo 'device-list-properties typename=fw_cfg_mem' | ./scripts/qmp/qmp-shell /tmp/qmp > Welcome to the QMP low-level shell! > Connected to QEMU 2.9.50 > > qemu-system-x86_64: qemu/hw/nvram/fw_cfg.c:974: fw_cfg_init: Assertion `!object_resolve_path(FW_CFG_PATH, NULL)' failed. > (QEMU) Disconnected > [1]+ Aborted (core dumped) qemu-system-x86_64 -display none -qmp unix:/tmp/qmp,server,nowait > $ > > > I suggest moving this check to realize, like the rest of > fw_cfg_init1(), but change it to do proper error reporting > instead of asserting. In that case what do you think is the best way to prevent realization of a second instance? With some playing I've managed to come up with the following (partial) diff that seems to work in some simple local tests: What seems to happen is that calling object_property_add_child() only succeeds for the first instance and so a simple comparison is enough to determine that the device already exists at FW_CFG_PATH. Or is this a fairly terrible (ab)use of the QOM APIs? ATB, Mark. diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c index 9b0aaa2..e678ec0 100644 --- a/hw/nvram/fw_cfg.c +++ b/hw/nvram/fw_cfg.c @@ -862,12 +862,20 @@ static void fw_cfg_machine_ready(struct Notifier *n, void *data) -static void fw_cfg_common_realize(DeviceState *dev) +static void fw_cfg_common_realize(DeviceState *dev, Error **errp) { FWCfgState *s = FW_CFG(dev); MachineState *machine = MACHINE(qdev_get_machine()); uint32_t version = FW_CFG_VERSION; - + Object *obj; + + obj = object_resolve_path(FW_CFG_PATH, NULL); + if (obj != OBJECT(dev)) { + error_setg(errp, "Only one fw_cfg device can be instantiated per " + "machine"); + return; + } + fw_cfg_add_bytes(s, FW_CFG_SIGNATURE, (char *)"QEMU", 4); fw_cfg_add_bytes(s, FW_CFG_UUID, &qemu_uuid, 16); fw_cfg_add_i16(s, FW_CFG_NOGRAPHIC, (uint16_t)!machine->enable_graphics);