From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50045) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dS649-00043l-FA for qemu-devel@nongnu.org; Mon, 03 Jul 2017 14:27:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dS645-0001Kl-Co for qemu-devel@nongnu.org; Mon, 03 Jul 2017 14:27:21 -0400 Received: from mx1.redhat.com ([209.132.183.28]:56390) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1dS645-0001Jj-2c for qemu-devel@nongnu.org; Mon, 03 Jul 2017 14:27:17 -0400 Date: Mon, 3 Jul 2017 15:27:10 -0300 From: Eduardo Habkost Message-ID: <20170703182710.GS12152@localhost.localdomain> References: <20170629132310.18865-1-marcandre.lureau@redhat.com> <20170629132310.18865-2-marcandre.lureau@redhat.com> <56d715b2-4d6b-44ab-0fc6-9e8c0b451623@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <56d715b2-4d6b-44ab-0fc6-9e8c0b451623@redhat.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 1/7] vmgenid: replace x-write-pointer-available hack List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Laszlo Ersek Cc: =?iso-8859-1?Q?Marc-Andr=E9?= Lureau , qemu-devel@nongnu.org, Ben Warren , "Michael S. Tsirkin" , anderson@redhat.com, imammedo@redhat.com On Mon, Jul 03, 2017 at 08:06:33PM +0200, Laszlo Ersek wrote: > On 06/29/17 15:23, Marc-Andr=E9 Lureau wrote: > > This compat property sole function is to prevent the device from bein= g > > instantiated. Instead of requiring an extra compat property, check if > > fw_cfg has DMA enabled. > >=20 > > This has the additional benefit of handling other cases properly, lik= e: > >=20 > > $ qemu-system-x86_64 -device vmgenid -machine none > > qemu-system-x86_64: -device vmgenid: vmgenid requires DMA write sup= port in fw_cfg, which this machine type does not provide > > $ qemu-system-x86_64 -device vmgenid -machine pc-i440fx-2.9 -global= fw_cfg.dma_enabled=3Doff > > qemu-system-x86_64: -device vmgenid: vmgenid requires DMA write sup= port in fw_cfg, which this machine type does not provide > > $ qemu-system-x86_64 -device vmgenid -machine pc-i440fx-2.6 -global= fw_cfg.dma_enabled=3Don > > [boots normally] > >=20 > > Suggested-by: Eduardo Habkost > > Signed-off-by: Marc-Andr=E9 Lureau > > --- > > include/hw/acpi/bios-linker-loader.h | 2 ++ > > include/hw/compat.h | 4 ---- > > hw/acpi/bios-linker-loader.c | 6 ++++++ > > hw/acpi/vmgenid.c | 9 +-------- > > 4 files changed, 9 insertions(+), 12 deletions(-) > >=20 > > diff --git a/include/hw/acpi/bios-linker-loader.h b/include/hw/acpi/b= ios-linker-loader.h > > index efe17b0b9c..a711dbced8 100644 > > --- a/include/hw/acpi/bios-linker-loader.h > > +++ b/include/hw/acpi/bios-linker-loader.h > > @@ -7,6 +7,8 @@ typedef struct BIOSLinker { > > GArray *file_list; > > } BIOSLinker; > > =20 > > +bool bios_linker_loader_can_write_pointer(void); > > + > > BIOSLinker *bios_linker_loader_init(void); > > =20 > > void bios_linker_loader_alloc(BIOSLinker *linker, > > diff --git a/include/hw/compat.h b/include/hw/compat.h > > index 26cd5851a5..36f02179ac 100644 > > --- a/include/hw/compat.h > > +++ b/include/hw/compat.h > > @@ -150,10 +150,6 @@ > > .driver =3D "fw_cfg_io",\ > > .property =3D "dma_enabled",\ > > .value =3D "off",\ > > - },{\ > > - .driver =3D "vmgenid",\ > > - .property =3D "x-write-pointer-available",\ > > - .value =3D "off",\ > > }, > > =20 > > #define HW_COMPAT_2_3 \ > > diff --git a/hw/acpi/bios-linker-loader.c b/hw/acpi/bios-linker-loade= r.c > > index 046183a0f1..587d62cb93 100644 > > --- a/hw/acpi/bios-linker-loader.c > > +++ b/hw/acpi/bios-linker-loader.c > > @@ -168,6 +168,12 @@ bios_linker_find_file(const BIOSLinker *linker, = const char *name) > > return NULL; > > } > > =20 > > +bool bios_linker_loader_can_write_pointer(void) > > +{ > > + FWCfgState *fw_cfg =3D fw_cfg_find(); > > + return fw_cfg && fw_cfg_dma_enabled(fw_cfg); > > +} > > + > > /* > > * bios_linker_loader_alloc: ask guest to load file into guest memor= y. > > * > > diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c > > index a32b847fe0..ab5da293fd 100644 > > --- a/hw/acpi/vmgenid.c > > +++ b/hw/acpi/vmgenid.c > > @@ -205,17 +205,11 @@ static void vmgenid_handle_reset(void *opaque) > > memset(vms->vmgenid_addr_le, 0, ARRAY_SIZE(vms->vmgenid_addr_le)= ); > > } > > =20 > > -static Property vmgenid_properties[] =3D { > > - DEFINE_PROP_BOOL("x-write-pointer-available", VmGenIdState, > > - write_pointer_available, true), > > - DEFINE_PROP_END_OF_LIST(), > > -}; > > - > > static void vmgenid_realize(DeviceState *dev, Error **errp) > > { > > VmGenIdState *vms =3D VMGENID(dev); > > =20 > > - if (!vms->write_pointer_available) { > > + if (!bios_linker_loader_can_write_pointer()) { > > error_setg(errp, "%s requires DMA write support in fw_cfg, " > > "which this machine type does not provide", VMGEN= ID_DEVICE); > > return; > > @@ -239,7 +233,6 @@ static void vmgenid_device_class_init(ObjectClass= *klass, void *data) > > dc->vmsd =3D &vmstate_vmgenid; > > dc->realize =3D vmgenid_realize; > > dc->hotpluggable =3D false; > > - dc->props =3D vmgenid_properties; > > =20 > > object_class_property_add_str(klass, VMGENID_GUID, NULL, > > vmgenid_set_guid, NULL); > >=20 >=20 > I believe we discussed this approach back then (but I can't find the > relevant messages, of course). >=20 > What guarantees that, by the time you call fw_cfg_find() from > vmgenid_realize() -- that is, from the realize function of an > independent device --, the fw_cfg device will have been realized (with > its properties having taken their final values)? I don't see how the > ordering is guaranteed here; please explain (preferably in the commit > message). Good point. What makes this work is the fact that fw_cfg is a built-in device that is initialized very early by the machine init code. We could remove that requirement, but it would require reporting errors from the machine_done notifier (in acpi_setup(), probably). I'm not sure it would be worth the extra complexity. We have at least one other device that also assumes fw_cfg_find() can be safely used on realize: pvpanic. --=20 Eduardo