Re: [Qemu-devel] [PATCH v6 2/7] hw/misc: add vmcoreinfo device

[Michael S. Tsirkin]

On Tue, Oct 10, 2017 at 03:01:10PM -0300, Eduardo Habkost wrote:
> On Tue, Oct 10, 2017 at 04:06:28PM +0100, Daniel P. Berrange wrote:
> > On Tue, Oct 10, 2017 at 05:00:18PM +0200, Marc-André Lureau wrote:
> > > Hi
> > > 
> > > On Tue, Oct 10, 2017 at 10:31 AM, Daniel P. Berrange
> > > <address@hidden> wrote:
> > > > On Tue, Oct 10, 2017 at 12:44:26AM +0300, Michael S. Tsirkin wrote:
> > > >> On Mon, Oct 09, 2017 at 02:02:18PM +0100, Daniel P. Berrange wrote:
> > > >> > On Mon, Oct 09, 2017 at 02:43:44PM +0200, Igor Mammedov wrote:
> > > >> > > On Mon, 9 Oct 2017 12:03:36 +0100
> > > >> > > "Daniel P. Berrange" <address@hidden> wrote:
> > > >> > >
> > > >> > > > On Mon, Sep 11, 2017 at 06:59:24PM +0200, Marc-André Lureau 
> > > >> > > > wrote:
> > > >> > > > > See docs/specs/vmcoreinfo.txt for details.
> > > >> > > > >
> > > >> > > > > "etc/vmcoreinfo" fw_cfg entry is added when using "-device 
> > > >> > > > > vmcoreinfo".
> > > >> > > >
> > > >> > > > I'm wondering if you considered just adding the entry to fw_cfg 
> > > >> > > > by
> > > >> > > > default, without requiring any -device arg ? Unless I'm 
> > > >> > > > misunderstanding,
> > > >> > > > this doesn't feel like a device to me - its just a well known 
> > > >> > > > bucket
> > > >> > > > in fw_cfg IIUC ?  Obviously its existance would need to be tied 
> > > >> > > > to
> > > >> > > > the latest machine type for ABI reasons though. The benefit of 
> > > >> > > > this
> > > >> > > > is that it would "just work" without us having to plumb it 
> > > >> > > > through to
> > > >> > > > all the downstream applications that use QEMU for mgmt guest 
> > > >> > > > (OpenStack,
> > > >> > > > oVirt, GNOME Boxes, virt-manager, and countless other mgmt apps).
> > > >> > > it follows model set by pvpanic device, it's easier to manage from 
> > > >> > > migration
> > > >> > > POV, one could use it even for old machine types with new qemu 
> > > >> > > (just by adding
> > > >> > > device, it makes instance not backwards migratable to old qemu but 
> > > >> > > should work
> > > >> > > for forward migration) and if user doesn't need it, device could 
> > > >> > > be just omitted
> > > >> > > from CLI.
> > > >> >
> > > >> > Sure but it means that in effect no one will have this functionality 
> > > >> > enabled
> > > >> > for several years. pvpanic has been around a long time and I rarely 
> > > >> > see it
> > > >> > present in configured guests :-(
> > > >> >
> > > >> >
> > > >> > Regards,
> > > >> > Daniel
> > > >>
> > > >> libvirt runs with -nodefaults, right? I'd argue pretty strongly 
> > > >> -nodefaults
> > > >> shouldn't add optional devices anyway.
> > > >
> > > > This isn't really adding a device though is it - it is just a well known
> > > > location in fw_cfg to receive data.
> > > 
> > > Enabling the device on some configurations by default can be done as a
> > > follow-up patch. Can we get this series reviewed & merged?
> > 
> > The problem with the -device approach + turning it on by default is that 
> > there
> > is no way to turn it off again if you don't want it. eg there's way to undo
> > an implicit '-device foo' except via -nodefaults, but since libvirt uses 
> > that
> > already it would negate the effect of enabling it by default 
> > unconditionally.
> 
> It's still possible to add a -machine option that can
> enable/disable automatic creation of the device.
>
>
> But I also don't see why it needs to be implemented using -device
> if it's not really a device.  A boolean machine or fw_cfg
> property is good enough for that.

Device imho is a combination of guest/host interface and state.


> > 
> > Your previous approach of "-global fw_cfg.vmcoreinfo=on" is nicer in this
> > respect, as you can trivially turn it on/off, overriding the default state
> > in both directions.
> 
> Both "-global fw_cfg.vmcoreinfo=on|off" and
> "-machine vmcoreinfo=on|off" sound good enough to me.

I can live with the second option if people really want it.
I'd like to see some way to add these things without
adding to the mess that is the pc initialization
but oh well.

> -- 
> Eduardo