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
>>>>> <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.
>>
>> 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 machine types. Saves a lot of churn for higher
levels of the stack

Thanks,
Cole