Re: [Qemu-devel] [PATCH] RFC: vmcoreinfo device

[Eduardo Habkost]

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);
  }

-- 
Eduardo