[Libvir] PATCH 0/2: Capabilities and features
Hi, these two patches extend the features libvirt reports in the capabilities XML; in particular, features are now reported for the QEMU driver, and features for both Xen and QEMU include information on ACPI/APIC support. Tools that rely on feature reporting, in general, need to know whether a certain feature is on or off by default, and whether that can be toggled, because users will be asking for one of three things for their VM: * Feature X (e.g., ACPI) must be on/present in the VM * Feature X must not be on/present in the VM * It doesn't matter whether feature X is there or not For example, in QEMU, ACPI is on by default, but can be turned off. The APIC on the other hand, is always there and can't be turned off. That is now reported as <features> <acpi default="on" toggle="yes"/> <apic default="on" toggle="no"/> </features> It's unfortunate that the format for PAE (<pae/> vs. <nonpae/>) reporting doesn't fit into this scheme. I didn't want to start with having tags <X/> and <nonX/> for each feature; that means that parsers need to special case PAE. It's probably too late, but it would be nice if PAE was reported in a similar format, with <pae> correponding to <pae default="on" toggle="no"/>, <nonpae/> corresponding to <pae default="yes" toggle="no"/>, and having both together corresponding to <pae default="yes" toggle="yes"/>, but that's a separate discussion from these patches. David
This patch lets the qemu/kvm drivers report features for i686 and x86_64. The output is table-driven, like the rest of the capability generation. That should make it easy to report additional features for other arches (which I know zip about) David
David Lutterkort wrote:
This patch lets the qemu/kvm drivers report features for i686 and x86_64. The output is table-driven, like the rest of the capability generation. That should make it easy to report additional features for other arches (which I know zip about)
This patch is fine, except I'd recommend using the STREQ/STRNEQ macros (defined in src/internal.h) instead of: + if (strcmp(flags[i].name, "pae") == 0) { But it's not a big problem. Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
On Tue, Jul 24, 2007 at 10:31:10AM -0700, David Lutterkort wrote:
This patch lets the qemu/kvm drivers report features for i686 and x86_64. The output is table-driven, like the rest of the capability generation. That should make it easy to report additional features for other arches (which I know zip about)
Looks fine to me, except:
+ r = virBufferAdd(xml, "\ + <pae/>\n", -1);
I really can't understand what's the point of doing the indentation by cutting in the string parameter, I would really fix those before applying. Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
Daniel Veillard wrote:
On Tue, Jul 24, 2007 at 10:31:10AM -0700, David Lutterkort wrote:
This patch lets the qemu/kvm drivers report features for i686 and x86_64. The output is table-driven, like the rest of the capability generation. That should make it easy to report additional features for other arches (which I know zip about)
Looks fine to me, except:
+ r = virBufferAdd(xml, "\ + <pae/>\n", -1);
I really can't understand what's the point of doing the indentation by cutting in the string parameter, I would really fix those before applying.
It's like that in the current code. I did it that way so that the XML would be nicely formatted -- makes more sense on longer stretches of XML, eg: r = virBufferVSprintf (xml, "\ <capabilities>\n\ <host>\n\ <cpu>\n\ <arch>%s</arch>\n\ </cpu>\n\ </host>\n", utsname.machine); For single lines of code it seems like it'd be better to indent it normally. IIRC Dan Berrange was in favour of using libxml calls to build up the XML, at which point it is the job of libxml to generate nicely formatted output, but that's a lot of work to implement for only a tiny gain. Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
Patch updated with feedback (mostly formatting) David
On Fri, Jul 27, 2007 at 04:07:05PM -0700, David Lutterkort wrote:
Patch updated with feedback (mostly formatting)
Looks great, applied and commited, thanks ! Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
For HVM Xen, state that ACPI and APIC can be turned on and off. David
David Lutterkort wrote:
For HVM Xen, state that ACPI and APIC can be turned on and off.
I'm pretty sure that in recent versions of Xen, both of these features are unconditionally enabled. Regards, Anthony Liguori
David
On Tue, 2007-07-24 at 15:42 -0500, Anthony Liguori wrote:
David Lutterkort wrote:
For HVM Xen, state that ACPI and APIC can be turned on and off.
I'm pretty sure that in recent versions of Xen, both of these features are unconditionally enabled.
From testing this a little (after fighting silly fights with rawhide and xen config files) with an FC6 i386 guest on an x86_64 hypervisor:
* In RHEL5 (xen 3.0.3), for both plain xen config files + xm create and libvirt XML + virsh create: <acpi default='off' toggle='yes'/> <apic default='off' toggle='yes'/> <pae default='off' toggle='yes'/> * In rawhide (3.1.0) using xen config files and xm create: <acpi default='on' toggle='yes'/> <apic default='on' toggle='no'/> <pae default='on' toggle='yes'/> * In rawhide (3.1.0) using libvirt XML and virsh create: <acpi default='off' toggle='yes'/> <apic default='on' toggle='no'/> <pae default='off' toggle='yes'/> We really only care about the libvirt XML part, for which only apic has differences between 3.0.3 and 3.1.0. I'll update the patches to take the above into account, David
On Tue, Jul 24, 2007 at 10:32:34AM -0700, David Lutterkort wrote:
For HVM Xen, state that ACPI and APIC can be turned on and off.
Looks fine to me except for the point raised by Anthony and some cleanup:
if (r == -1) goto vir_buffer_failed; } + if (guest_archs[i].hvm) { + r = virBufferAdd (xml, + "\ + <acpi default=\"on\" toggle=\"yes\"/>\n\ + <apic default=\"on\" toggle=\"yes\"/>\n", -1); + if (r == -1) goto vir_buffer_failed; + } r = virBufferAdd (xml, "\
use ' for the delimiters of attributes values, that way you don't need an escaping, and as in previous patch I dislikethe way strings are cut for indenting at the C level. thanks ! Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
Updated patch, incorporating feedback. Besides formatting, changes how apic is reported - and that's not pretty. As danpb found out, in cset 12423:ebed72718263 in xen-unstable, setting the apic was removed from tools/libxc/xc_hvm_build.c. In a surprising turn of events, cset 12569:9d6bc06919e0 claims to reintroduce apic toggling (with a default of 1/on) by writing it into an hvm_info_table in tools/python/xen/lowlevel/xc/xc.c - I haven't figured out why the apic can't be toggled in rawhide, though. For now, the code uses the hypervisor version to determine what apic capability to report - most likely not the right thing to do, better suggestions much appreciated. David
On Fri, Jul 27, 2007 at 04:24:46PM -0700, David Lutterkort wrote:
Updated patch, incorporating feedback.
Besides formatting, changes how apic is reported - and that's not pretty. As danpb found out, in cset 12423:ebed72718263 in xen-unstable, setting the apic was removed from tools/libxc/xc_hvm_build.c.
In a surprising turn of events, cset 12569:9d6bc06919e0 claims to reintroduce apic toggling (with a default of 1/on) by writing it into an hvm_info_table in tools/python/xen/lowlevel/xc/xc.c - I haven't figured out why the apic can't be toggled in rawhide, though.
For now, the code uses the hypervisor version to determine what apic capability to report - most likely not the right thing to do, better suggestions much appreciated.
In the absence of a better idea, I commited this patch. I just changed a bit the XML output indenting, thanks a lot ! Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
participants (4)
-
Anthony Liguori -
Daniel Veillard -
David Lutterkort -
Richard W.M. Jones