[virt-tools-list] [PATCH] virtinst: add <vmcoreinfo/> feature support

[Cole Robinson]

On 02/21/2018 11:31 AM, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Feb 21, 2018 at 4:08 PM, Cole Robinson <crobinso at redhat.com> wrote:
>> On 02/20/2018 02:30 PM, marcandre.lureau at redhat.com wrote:
>>> From: Marc-André Lureau <marcandre.lureau at redhat.com>
>>>
>>> The <vmcoreinfo> feature allows a guest to provide debug details when
>>> producing dump. It's useful in particular for Linux guests with KASLR
>>> enabled, as otherwise the dump are difficult to analyze.
>>>
>>
>> Thanks for the patch, but, ugh I wish this didn't end up in libvirt as a
>> boolean property, there's a reason things are generally tristate these
>> days, since there's no way to distinguish between 'use hypervisor
>> default' and 'explicitly disable this feature'. What if a future version
>> of qemu or machine type defaults to vmcoreinfo=on, how does the user
>> request libvirt disable that setting?
>>
> 
> It translates to -device vmcoreinfo, so you have no other way but to
> rely on -nodefaults at qemu level to disable it if some day it is
> added by default.
> 
> So it will probably have to be specified explicitly all the time by
> the domain XML.
> 
> Or I would like to know what else we could do..
> 

Hmm yeah I guess -device is hard. When I see <feature> I think things
like apic eoi which was opt in at first but became the default later.
Similarish to cpu x2apic emulation. Maybe a -machine option? But if it's
a device it probably doesn't map cleanly

>> Maybe in this particular case it's minor but tristate is much preferred
>> IMO. It's probably not too late to change at the libvirt level and
>> convert <vmcoreinfo/> to <vmcoreinfo enabled='on'/>, the only reason we
>> never did that with preexisting boolean properties is that they were
>> around long enough that apps were kinda dependent on it, for reading the
>> XML at least.
> 
> I also think it can be changed to support the tristate later on, if it
> makes sense.
> 

Once apps start encoding the logic of ./domain/features/vmcoreinfo ==
vmcoreinfo is enabled, we are kind of stuck with the old XML format
though. That's why we never changed <acpi/> and <apic/>, worry that apps
were dependent on it. So sooner the better IMO

>>
>>> This patch adds virt-install support for vmcoreinfo domain
>>> feature. Whenever the host libvirt/qemu is recent enough, and the VM
>>> is x86 or arm-virt, we can assume <vmcoreinfo/> is supported and
>>> enable it safely by default (unless --feature vmcoreinfo=on/off is
>>> given, or changed via API)
>>>
>>
>> Better I think to split this patch in two parts, the xml/cli wiring
>> which is straightforward, the 'adding it as default' which is always
>> more interesting to discuss.
>>
>> My two questions are: 1) are there any downsides to enabling this, 2) if
>> we should enable it by default why isn't it enabled by default in qemu,
>> maybe tied to new machine types
> 
> I agree it would be nice to enable it by default on new machine types.
> But since it's a -device, it should be removed anyway with
> -nodefaults. Hence setting it should be higher up the stack, when
> creating a domain XML.
> 

Are there downsides though?

It just kinda stinks with features like this, means patches to
virt-manager, boxes, rhev, openstack, and who knows else. But if it
can't be avoided it's fine

Thanks,
Cole