On Thu, Jun 01, 2017 at 10:16:50AM +0000, Marc-André Lureau wrote:
> Hi
>
> On Wed, Apr 26, 2017 at 2:58 AM Eduardo Habkost <ehabk...@redhat.com> wrote:
>
> > On Tue, Apr 25, 2017 at 11:35:23PM +0300, Michael S. Tsirkin wrote:
> > > On Tue, Apr 25, 2017 at 05:29:20PM -0300, Eduardo Habkost wrote:
> > > > On Mon, Apr 24, 2017 at 05:03:55PM +0400, Marc-André Lureau wrote:
> > > > [...]
> > > > > diff --git a/include/hw/compat.h b/include/hw/compat.h
> > > > > index 5d5be91daf..d0c9b71902 100644
> > > > > --- a/include/hw/compat.h
> > > > > +++ b/include/hw/compat.h
> > > > > @@ -135,6 +135,10 @@
> > > > > .driver = "vmgenid",\
> > > > > .property = "x-write-pointer-available",\
> > > > > .value = "off",\
> > > > > + },{\
> > > > > + .driver = "vmcoreinfo",\
> > > > > + .property = "x-write-pointer-available",\
> > > > > + .value = "off",\
> > > > > },
> > > >
> > > > My first reaction to this was "we don't need this compat property,
> > because the
> > > > device didn't even exist in QEMU 2.4".
> > > >
> > > > But then I read commit f2a1ae45d8ec5ad494e66a9234499a2e0fbf4b40 and
> > now I see
> > > > why this is required: this is a compat property whose sole function is
> > to
> > > > prevent the device from being instantiated.
> > > >
> > > > Instead of requiring an extra compat property, I suggest just checking
> > if
> > > > fw_cfg has DMA enabled. e.g.:
> > > >
> > > > static void vmgenid_realize(DeviceState *dev, Error **errp)
> > > > {
> > > > VmGenIdState *vms = VMGENID(dev);
> > > > + FWCfgState *fw_cfg = FW_CFG(object_resolve_path_type("",
> > TYPE_FW_CFG, NULL));
> > > >
> > > > - if (!vms->write_pointer_available) {
> > > > + if (!fw_cfg || !fw_cfg_dma_enabled(fw_cfg)) {
> > > > error_setg(errp, "%s requires DMA write support in fw_cfg, "
> > > > "which this machine type does not provide",
> > VMGENID_DEVICE);
> > > > return;
> > > >
> > > >
> > > > This has the additional benefit of handling other cases properly, like:
> > > >
> > > > $ qemu-system-x86_64 -device vmgenid -machine none
> > > > qemu-system-x86_64: -device vmgenid: vmgenid requires DMA write
> > support 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=off
> > > > qemu-system-x86_64: -device vmgenid: vmgenid requires DMA write
> > support 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=on
> > > > [boots normally]
> > > > $
> > >
> > > It's quite ugly to make it poke at fw cfg internals though,
> > > it shouldn't know how is write pointer implemented.
> > > We need some kind of API that it can share with vm gen id.
> >
> > Do you mean adding something like this to bios-linker-loader.c?
> >
> > bool bios_linker_loader_can_write_pointer(void)
> > {
> > FWCfgState *fw_cfg = FW_CFG(object_resolve_path_type("",
> > TYPE_FW_CFG, NULL));
> > return fw_cfg && fw_cfg_dma_enabled(fw_cfg);
> > }
> >
> >
> Looks like a good change to me, are you going to send a patch?
I will add it to my (wish-)to-do list, but I will probably take a
while until I get to do it. If anybody wants to implement that,
please be my guest.
--
Eduardo
