On 02/21/2018 11:31 AM, Marc-André Lureau wrote: > Hi > > On Wed, Feb 21, 2018 at 4:08 PM, Cole Robinson <crobinso@xxxxxxxxxx> wrote: >> On 02/20/2018 02:30 PM, marcandre.lureau@xxxxxxxxxx wrote: >>> From: Marc-André Lureau <marcandre.lureau@xxxxxxxxxx> >>> >>> 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 _______________________________________________ virt-tools-list mailing list virt-tools-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/virt-tools-list
