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

[Eduardo Habkost]

On Tue, Apr 17, 2018 at 06:31:57PM -0400, Cole Robinson wrote:
> On 04/17/2018 05:11 PM, Eduardo Habkost wrote:
> > On Tue, Apr 17, 2018 at 03:12:03PM -0400, Cole Robinson wrote:
> > [...]
> >> Reviving this... did any follow up changes happen?
> >>
> >> Marc-André patched virt-manager a few months back to enable -device
> >> vmcoreinfo for new VMs:
> >>
> >> https://www.redhat.com/archives/virt-tools-list/2018-February/msg00020.html
> >>
> >> And I see there's at least a bug tracking adding this to openstack for
> >> new VMs:
> >>
> >> https://bugzilla.redhat.com/show_bug.cgi?id=1555276
> >>
> >> If this feature doesn't really have any downsides, it would be nice to
> >> get this tied to new machine types. Saves a lot of churn for higher
> >> levels of the stack
> > 
> > I understand this would be nice to have considering the existing
> > stacks, but at the same time I would like the rest of the
> > stack(s) to really try to not depend on QEMU machine-types to
> > define policy/defaults.
> > 
> > Every feature that is hidden behind an opaque machine-type name
> > and not visible in the domain XML and QEMU command-line increases
> > the risk of migration and compatibility bugs.
> > 
> 
> What exactly is the migration compatibility issue with turning on the
> equivalent of -device vmcoreinfo for -M *-2.13+ ? Possibly prevents
> backwards migration to older qemu but is that even a goal?

I mean the extra migration compatibility code that needs to be
maintained on older machine-types.  It's extra maintenance burden
on both upstream and downstream QEMU trees.


> 
> > This was being discussed in a mail thread at:
> > https://www.mail-archive.com/ovirt-devel@redhat.com/msg01196.html
> > 
> > Quoting Daniel, on that thread:
> > 
> > ] Another case is the pvpanic device - while in theory that could
> > ] have been enabled by default for all guests, by QEMU or a config
> > ] generator library, doing so is not useful on its own. The hard
> > ] bit of the work is adding code to the mgmt app to choose the
> > ] action for when pvpanic triggers, and code to handle the results
> > ] of that action.
> > 
> > From that comment, I understand that simply making QEMU create a
> > pvpanic device by default on pc-2.13+ won't be useful at all?
> > 
> 
> This qemu-devel thread was about -device vmcoreinfo though, not pvpanic.
> vmcoreinfo doesn't need anything else to work AFAICT and shouldn't need
> any explicit config, heck it doesn't even have any -device properties.
> 
> Like Dan says pvpanic isn't a 'just works' thing, and I know for windows
> VMs it shows up in device manager which has considerations for things
> like SVVP. I think vmcoreinfo doesn't have the same impact
> 

Oops, nevermind.  I confused both.


> There are some guest visible things that we have turned on for new
> machine types in the past, pveoi and x2apic comes to mind.

Yes, we have tons of guest-visible things that we tie to the
machine-type.  What I'm looking for is a solution to make this
less frequent in the future.

-- 
Eduardo

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

[Cole Robinson]

On 04/17/2018 05:11 PM, Eduardo Habkost wrote:
> On Tue, Apr 17, 2018 at 03:12:03PM -0400, Cole Robinson wrote:
> [...]
>> Reviving this... did any follow up changes happen?
>>
>> Marc-André patched virt-manager a few months back to enable -device
>> vmcoreinfo for new VMs:
>>
>> https://www.redhat.com/archives/virt-tools-list/2018-February/msg00020.html
>>
>> And I see there's at least a bug tracking adding this to openstack for
>> new VMs:
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=1555276
>>
>> If this feature doesn't really have any downsides, it would be nice to
>> get this tied to new machine types. Saves a lot of churn for higher
>> levels of the stack
> 
> I understand this would be nice to have considering the existing
> stacks, but at the same time I would like the rest of the
> stack(s) to really try to not depend on QEMU machine-types to
> define policy/defaults.
> 
> Every feature that is hidden behind an opaque machine-type name
> and not visible in the domain XML and QEMU command-line increases
> the risk of migration and compatibility bugs.
> 

What exactly is the migration compatibility issue with turning on the
equivalent of -device vmcoreinfo for -M *-2.13+ ? Possibly prevents
backwards migration to older qemu but is that even a goal?

> This was being discussed in a mail thread at:
> https://www.mail-archive.com/ovirt-devel@redhat.com/msg01196.html
> 
> Quoting Daniel, on that thread:
> 
> ] Another case is the pvpanic device - while in theory that could
> ] have been enabled by default for all guests, by QEMU or a config
> ] generator library, doing so is not useful on its own. The hard
> ] bit of the work is adding code to the mgmt app to choose the
> ] action for when pvpanic triggers, and code to handle the results
> ] of that action.
> 
> From that comment, I understand that simply making QEMU create a
> pvpanic device by default on pc-2.13+ won't be useful at all?
> 

This qemu-devel thread was about -device vmcoreinfo though, not pvpanic.
vmcoreinfo doesn't need anything else to work AFAICT and shouldn't need
any explicit config, heck it doesn't even have any -device properties.

Like Dan says pvpanic isn't a 'just works' thing, and I know for windows
VMs it shows up in device manager which has considerations for things
like SVVP. I think vmcoreinfo doesn't have the same impact

There are some guest visible things that we have turned on for new
machine types in the past, pveoi and x2apic comes to mind.

Thanks,
Cole

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

[Eduardo Habkost]

On Tue, Apr 17, 2018 at 03:12:03PM -0400, Cole Robinson wrote:
[...]
> Reviving this... did any follow up changes happen?
> 
> Marc-André patched virt-manager a few months back to enable -device
> vmcoreinfo for new VMs:
> 
> https://www.redhat.com/archives/virt-tools-list/2018-February/msg00020.html
> 
> And I see there's at least a bug tracking adding this to openstack for
> new VMs:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1555276
> 
> If this feature doesn't really have any downsides, it would be nice to
> get this tied to new machine types. Saves a lot of churn for higher
> levels of the stack

I understand this would be nice to have considering the existing
stacks, but at the same time I would like the rest of the
stack(s) to really try to not depend on QEMU machine-types to
define policy/defaults.

Every feature that is hidden behind an opaque machine-type name
and not visible in the domain XML and QEMU command-line increases
the risk of migration and compatibility bugs.

This was being discussed in a mail thread at:
https://www.mail-archive.com/ovirt-devel@redhat.com/msg01196.html

Quoting Daniel, on that thread:

] Another case is the pvpanic device - while in theory that could
] have been enabled by default for all guests, by QEMU or a config
] generator library, doing so is not useful on its own. The hard
] bit of the work is adding code to the mgmt app to choose the
] action for when pvpanic triggers, and code to handle the results
] of that action.

>From that comment, I understand that simply making QEMU create a
pvpanic device by default on pc-2.13+ won't be useful at all?

-- 
Eduardo

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

[Cole Robinson]

On 10/20/2017 02:48 PM, Eduardo Habkost wrote:
> On Sun, Oct 15, 2017 at 04:56:28AM +0300, Michael S. Tsirkin wrote:
>> 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
>  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"  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.
>>
>> It certainly feels like a device. It has state
>> (that needs to be migrated), it has a host/guest interface.
> 
> (Sorry for the late reply)
> 
> That's convincing enough to me.  :)
> 
> 

 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.
>>
>>
>> Certainly not a fw cfg flag. Can be a machine flag I guess
>> but then we'd have to open-code each such device.
>> And don't forget auto - this is what Daniel asks for.
> 
> I'm not sure Daniel is really asking for "auto": he is just
> asking for a way to disable the new default.  If "vmcoreinfo=off"
> and "vmcoreinfo=off" works, there's no need for a user-visible
> "auto" value.
> 
> (Actually, "auto" values makes compatibility code even messier,
> because we would need one additional compat property/field to
> tell QEMU what "auto" means on each machine)
> 

Reviving this... did any follow up changes happen?

Marc-André patched virt-manager a few months back to enable -device
vmcoreinfo for new VMs:

https://www.redhat.com/archives/virt-tools-list/2018-February/msg00020.html

And I see there's at least a bug tracking adding this to openstack for
new VMs:

https://bugzilla.redhat.com/show_bug.cgi?id=1555276

If this feature doesn't really have any downsides, it would be nice to
get this tied to new 

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

[Eduardo Habkost]

On Sun, Oct 15, 2017 at 04:56:28AM +0300, Michael S. Tsirkin wrote:
> 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
> > > >  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"  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.
> 
> It certainly feels like a device. It has state
> (that needs to be migrated), it has a host/guest interface.

(Sorry for the late reply)

That's convincing enough to me.  :)


> > > 
> > > 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.
> 
> 
> Certainly not a fw cfg flag. Can be a machine flag I guess
> but then we'd have to open-code each such device.
> And don't forget auto - this is what Daniel asks for.

I'm not sure Daniel is really asking for "auto": he is just
asking for a way to disable the new default.  If "vmcoreinfo=off"
and "vmcoreinfo=off" works, there's no need for a user-visible
"auto" value.

(Actually, "auto" values makes compatibility code even messier,
because we would need one additional compat property/field to
tell QEMU what "auto" means on each machine)

-- 
Eduardo

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
> > >  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"  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

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
> > >  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"  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.

It certainly feels like a device. It has state
(that needs to be migrated), it has a host/guest interface.


> > 
> > 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.


Certainly not a fw cfg flag. Can be a machine flag I guess
but then we'd have to open-code each such device.
And don't forget auto - this is what Daniel asks for.

I don't necessarily see this device as so special and think a generic
interface to control what goes into the machine would be better (e.g.
look how you use hacky -global to control fw cfg options, it only works
if there's a single one), but if everyone thinks otherwise and agrees we
should have it in there by default, and a property to disable, fine.

Can be a patch on top though.


> -- 
> Eduardo

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

[Eduardo Habkost]

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"  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.

Does it mean every time we make a PC device configurable, we
should make it be disabled by -nodefaults, and require libvirt to
adapt?

I don't think that would be a good idea.  Imagine the hassle the
"pc: make .* configurable" patches[1] would generate for libvirt.

> 
> So it's up to you guys, you can add it to VMs by default if you want to.

To be honest, I think "no defaults" is a misleading name for an
option.  If it really meant "create no optional device at all",
it would eventually become a synonym for "-machine none", and I
don't think that's its goal.

I expect PC to always have a set of devices/features that are
disabled by -nodefaults, and a set of devices/features that are
not disabled by -nodefaults.  We need good judgement to decide on
which set the device will be, and I believe Daniel exposed good
arguments to put vmcoreinfo in the second set.

[1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg393493.html
Subject: [RFC PATCH v2 00/12] Guest startup time optimization

-- 
Eduardo

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

[Eduardo Habkost]

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
> >  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"  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.

> 
> 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.

-- 
Eduardo

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

[Michael S. Tsirkin]

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
> >  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"  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.
> 
> 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.
> 
> Regards,
> Daniel


Interesting. IIUC you are saying that a property can have on/off/auto
options, while -device only has on/off.

Seems like something that's worth looking into generally. I'm not sure
why is should this device be special though.

> -- 
> |: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

[Daniel P. Berrange]

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
>  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"  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.

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.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

[Michael S. Tsirkin]

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
>  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"  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?
> 
> thanks

I plan to merge this in the next pull request.

> -- 
> Marc-André Lureau

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

[Marc-André Lureau]

Hi

On Tue, Oct 10, 2017 at 10:31 AM, Daniel P. Berrange
 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"  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?

thanks

-- 
Marc-André Lureau

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

[Daniel P. Berrange]

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"  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.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

[Michael S. Tsirkin]

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"  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.

So it's up to you guys, you can add it to VMs by default if you want to.


> -- 
> |: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

[Daniel P. Berrange]

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"  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
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

[Igor Mammedov]

On Mon, 9 Oct 2017 12:03:36 +0100
"Daniel P. Berrange"  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.

> 
> Regards,
> Daniel

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

[Marc-André Lureau]

Hi

- Original Message -
> 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).

v5 did that, it was using a -global fw_cfg.vmcoreinfo=on that defaulted to on.

Michael preferred to have a separate -device rather than mix it with fw_cfg, 
since it's not directly related.

We could make -device vmcoreinfo default on on machine type >= 2.11 instead?

thanks

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

[Daniel P. Berrange]

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).

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

[Marc-André Lureau]

See docs/specs/vmcoreinfo.txt for details.

"etc/vmcoreinfo" fw_cfg entry is added when using "-device vmcoreinfo".

Signed-off-by: Marc-André Lureau 
---
 include/hw/misc/vmcoreinfo.h | 46 +
 hw/misc/vmcoreinfo.c | 96 
 docs/specs/vmcoreinfo.txt| 41 +++
 hw/misc/Makefile.objs|  1 +
 4 files changed, 184 insertions(+)
 create mode 100644 include/hw/misc/vmcoreinfo.h
 create mode 100644 hw/misc/vmcoreinfo.c
 create mode 100644 docs/specs/vmcoreinfo.txt

diff --git a/include/hw/misc/vmcoreinfo.h b/include/hw/misc/vmcoreinfo.h
new file mode 100644
index 00..c3aa856545
--- /dev/null
+++ b/include/hw/misc/vmcoreinfo.h
@@ -0,0 +1,46 @@
+/*
+ * Virtual Machine coreinfo device
+ *
+ * Copyright (C) 2017 Red Hat, Inc.
+ *
+ * Authors: Marc-André Lureau 
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+#ifndef VMCOREINFO_H
+#define VMCOREINFO_H
+
+#include "hw/qdev.h"
+
+#define VMCOREINFO_DEVICE "vmcoreinfo"
+#define VMCOREINFO(obj) OBJECT_CHECK(VMCoreInfoState, (obj), VMCOREINFO_DEVICE)
+
+#define VMCOREINFO_FORMAT_NONE 0x0
+#define VMCOREINFO_FORMAT_ELF 0x1
+
+/* all fields are little-endian */
+typedef struct FWCfgVMCoreInfo {
+uint16_t host_format; /* set on reset */
+uint16_t guest_format;
+uint32_t size;
+uint64_t paddr;
+} QEMU_PACKED FWCfgVMCoreInfo;
+
+typedef struct VMCoreInfoState {
+DeviceClass parent_obj;
+
+bool has_vmcoreinfo;
+FWCfgVMCoreInfo vmcoreinfo;
+} VMCoreInfoState;
+
+/* returns NULL unless there is exactly one device */
+static inline VMCoreInfoState *vmcoreinfo_find(void)
+{
+Object *o = object_resolve_path_type("", VMCOREINFO_DEVICE, NULL);
+
+return o ? VMCOREINFO(o) : NULL;
+}
+
+#endif
diff --git a/hw/misc/vmcoreinfo.c b/hw/misc/vmcoreinfo.c
new file mode 100644
index 00..a618e12677
--- /dev/null
+++ b/hw/misc/vmcoreinfo.c
@@ -0,0 +1,96 @@
+/*
+ * Virtual Machine coreinfo device
+ *
+ * Copyright (C) 2017 Red Hat, Inc.
+ *
+ * Authors: Marc-André Lureau 
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "hw/nvram/fw_cfg.h"
+#include "hw/misc/vmcoreinfo.h"
+
+static void fw_cfg_vmci_write(void *dev, off_t offset, size_t len)
+{
+VMCoreInfoState *s = VMCOREINFO(dev);
+
+s->has_vmcoreinfo = offset == 0 && len == sizeof(s->vmcoreinfo)
+&& s->vmcoreinfo.guest_format != VMCOREINFO_FORMAT_NONE;
+}
+
+static void vmcoreinfo_reset(void *dev)
+{
+VMCoreInfoState *s = VMCOREINFO(dev);
+
+s->has_vmcoreinfo = false;
+memset(>vmcoreinfo, 0, sizeof(s->vmcoreinfo));
+s->vmcoreinfo.host_format = cpu_to_le16(VMCOREINFO_FORMAT_ELF);
+}
+
+static void vmcoreinfo_realize(DeviceState *dev, Error **errp)
+{
+VMCoreInfoState *s = VMCOREINFO(dev);
+FWCfgState *fw_cfg = fw_cfg_find();
+
+/* Given that this function is executing, there is at least one VMCOREINFO
+ * device. Check if there are several.
+ */
+if (!vmcoreinfo_find()) {
+error_setg(errp, "at most one %s device is permitted",
+   VMCOREINFO_DEVICE);
+return;
+}
+
+if (!fw_cfg || !fw_cfg->dma_enabled) {
+error_setg(errp, "%s device requires fw_cfg with DMA",
+   VMCOREINFO_DEVICE);
+return;
+}
+
+fw_cfg_add_file_callback(fw_cfg, "etc/vmcoreinfo",
+ NULL, fw_cfg_vmci_write, s,
+ >vmcoreinfo, sizeof(s->vmcoreinfo), false);
+
+qemu_register_reset(vmcoreinfo_reset, dev);
+}
+
+static const VMStateDescription vmstate_vmcoreinfo = {
+.name = "vmcoreinfo",
+.version_id = 1,
+.minimum_version_id = 1,
+.fields = (VMStateField[]) {
+VMSTATE_BOOL(has_vmcoreinfo, VMCoreInfoState),
+VMSTATE_UINT16(vmcoreinfo.host_format, VMCoreInfoState),
+VMSTATE_UINT16(vmcoreinfo.guest_format, VMCoreInfoState),
+VMSTATE_UINT32(vmcoreinfo.size, VMCoreInfoState),
+VMSTATE_UINT64(vmcoreinfo.paddr, VMCoreInfoState),
+VMSTATE_END_OF_LIST()
+},
+};
+
+static void vmcoreinfo_device_class_init(ObjectClass *klass, void *data)
+{
+DeviceClass *dc = DEVICE_CLASS(klass);
+
+dc->vmsd = _vmcoreinfo;
+dc->realize = vmcoreinfo_realize;
+dc->hotpluggable = false;
+}
+
+static const TypeInfo vmcoreinfo_device_info = {
+.name  = VMCOREINFO_DEVICE,
+.parent= TYPE_DEVICE,
+.instance_size = sizeof(VMCoreInfoState),
+.class_init= vmcoreinfo_device_class_init,
+};
+
+static void vmcoreinfo_register_types(void)
+{
+type_register_static(_device_info);
+}
+
+type_init(vmcoreinfo_register_types)
diff --git