Hi, We're finding that we are going to be needing the cpu flags (as reported in /proc/cpuinfo) ...specifically to find out if we are a vmx enabled machine. So - off I went looking into this for a patch to submit upstream. Unfortunately, I ran into some questions which need answering before I really proceed with this It seems to me that this info would best be parsed in src/nodeinfo.c This is where other cpuinfo things are parsed...and stored in the nodeinfo struct Perhaps we store this as a bitmask encoded int, as defined in /usr/include/asm/cpufeature.h and tack this onto the end of sad struct. My concern is that adding to the nodeinfo struct breaks the API - such that the structs will be different sizes between versions. Also - this seems to be x86 specific. Are we primarily destined for x86? Or would this type of change be unacceptable due to not working on PPC, for example? Thoughts? Ben
On Tue, Sep 16, 2008 at 04:45:09PM -0400, Ben Guthro wrote:
Also - this seems to be x86 specific. Are we primarily destined for x86? Or would this type of change be unacceptable due to not working on PPC, for example?
You would at least need an OS-agnostic implementation and API. In particular you couldn't just give out an integer and say "the bits are like in Linux's headers". regards john
On Tue, Sep 16, 2008 at 04:45:09PM -0400, Ben Guthro wrote:
Hi,
We're finding that we are going to be needing the cpu flags (as reported in /proc/cpuinfo) ..specifically to find out if we are a vmx enabled machine.
So - off I went looking into this for a patch to submit upstream. Unfortunately, I ran into some questions which need answering before I really proceed with this
It seems to me that this info would best be parsed in src/nodeinfo.c This is where other cpuinfo things are parsed...and stored in the nodeinfo struct Perhaps we store this as a bitmask encoded int, as defined in /usr/include/asm/cpufeature.h and tack this onto the end of sad struct.
My concern is that adding to the nodeinfo struct breaks the API - such that the structs will be different sizes between versions.
Indeed - any struct or API in include/libvirt/libvirt.h is immutable to preserve ABI.
Also - this seems to be x86 specific. Are we primarily destined for x86? Or would this type of change be unacceptable due to not working on PPC, for example?
No,we need to work on every architecture - KVM for example is ported to x86, ia64, ppc, and s390, Xen is on arm, x86, ia64. Now the set of flags will likely be different across each processor - and some may not even have a concept of flags, but we shouldn't be specifically x86 in API. I think the most likely place for exposing CPU flags would be in the capabilities XML format. We do in fact already expose 3 flags there, PAE, VMX and SVM. As John mentions we also need to be careful to specify a naming that is consistent across OS - though that may be as simple as defining that the naming is required to match Linux /proc/cpuinfo standards. The intent is that if you have the same CPUs in 2 machines, one running Linux and the other Solaris then libvirt must report the same names. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Wed, Sep 17, 2008 at 10:42:00AM +0100, Daniel P. Berrange wrote:
On Tue, Sep 16, 2008 at 04:45:09PM -0400, Ben Guthro wrote:
Hi,
We're finding that we are going to be needing the cpu flags (as reported in /proc/cpuinfo) ..specifically to find out if we are a vmx enabled machine.
So - off I went looking into this for a patch to submit upstream. Unfortunately, I ran into some questions which need answering before I really proceed with this
It seems to me that this info would best be parsed in src/nodeinfo.c This is where other cpuinfo things are parsed...and stored in the nodeinfo struct Perhaps we store this as a bitmask encoded int, as defined in /usr/include/asm/cpufeature.h and tack this onto the end of sad struct.
My concern is that adding to the nodeinfo struct breaks the API - such that the structs will be different sizes between versions.
Would it be possible to hack around this? For example you could use a unused bit of the old structure to set a flag for 'new-type-of-struct'. The old libvirt would ignore it, the new one would look for it. For example if it was an int enty and it was used as a flag entry wherein only 4 bits would be used, you could use the 5th bit that would tell it: "this is version 2 of the struct" and then the libvirt would cast it to the new struct which would contain the extra data-bits. This is how it is done in the RHEL kernels to work-around the kABI.
On Wed, Sep 17, 2008 at 08:44:09AM -0400, Konrad Rzeszutek wrote:
Would it be possible to hack around this? For example you could use a unused bit of the old structure to set a flag for 'new-type-of-struct'. The old libvirt would ignore it, the new one would look for it.
That doesn't help for virNodeGetInfo because the caller passes an uninitialized struct that could contain anything. I don't think there's anything that can help for this old interface. We'll never be able to change the size or contents of that structure. In any case it doesn't matter because the solution is to return the flags in the capabilities XML. Rich. -- Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://et.redhat.com/~rjones/virt-df/
On Wed, Sep 17, 2008 at 08:44:09AM -0400, Konrad Rzeszutek wrote:
On Wed, Sep 17, 2008 at 10:42:00AM +0100, Daniel P. Berrange wrote:
On Tue, Sep 16, 2008 at 04:45:09PM -0400, Ben Guthro wrote:
Hi,
We're finding that we are going to be needing the cpu flags (as reported in /proc/cpuinfo) ..specifically to find out if we are a vmx enabled machine.
So - off I went looking into this for a patch to submit upstream. Unfortunately, I ran into some questions which need answering before I really proceed with this
It seems to me that this info would best be parsed in src/nodeinfo.c This is where other cpuinfo things are parsed...and stored in the nodeinfo struct Perhaps we store this as a bitmask encoded int, as defined in /usr/include/asm/cpufeature.h and tack this onto the end of sad struct.
My concern is that adding to the nodeinfo struct breaks the API - such that the structs will be different sizes between versions.
Would it be possible to hack around this? For example you could use a unused bit of the old structure to set a flag for 'new-type-of-struct'. The old libvirt would ignore it, the new one would look for it. For example if it was an int enty and it was used as a flag entry wherein only 4 bits would be used, you could use the 5th bit that would tell it: "this is version 2 of the struct" and then the libvirt would cast it to the new struct which would contain the extra data-bits.
I don't want us to rely on obscure tricks like that in public facing APIs. We have a very simple rule - everything in include/libvirt is append only. No existing APIs or structs can be changed. New APIs & structs can be added. Clear, easy to understand and risk free.
This is how it is done in the RHEL kernels to work-around the kABI.
This is true, but the usage in RHEL kernels is internal API and isolated to RHEL-5's branch of the kernel. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
I think the most likely place for exposing CPU flags would be in the capabilities XML format. We do in fact already expose 3 flags there, PAE, VMX and SVM.
This looks like all of the info that I need - I guess I overlooked this part of the code prior to my message. The problem that I see currently is that despite the capabilities string containing the "host" tag, with this cpu info - it is up to each hypervisor driver to implement the broadcasting of host features. For example - Xen will report PAE,VMX,SVM - but my ovirt node running KVM currently only reports the following: <host> <cpu> <arch>x86_64</arch> </cpu> </host> It seems to me that it might be useful for some sort of "node" info driver, where we might be able to share code for hypervisor independent info about the physical machine it is running on. We should not have to re-write the scanning of /proc/cpuinfo in every hypervisor driver, IMHO. Ben
On Wed, Sep 17, 2008 at 09:52:08AM -0400, Ben Guthro wrote:
We should not have to re-write the scanning of /proc/cpuinfo in every hypervisor driver, IMHO.
Yes, sharing code like that is the way to go. Rich. -- Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into Xen guests. http://et.redhat.com/~rjones/virt-p2v
On Wed, Sep 17, 2008 at 09:52:08AM -0400, Ben Guthro wrote:
I think the most likely place for exposing CPU flags would be in the capabilities XML format. We do in fact already expose 3 flags there, PAE, VMX and SVM.
This looks like all of the info that I need - I guess I overlooked this part of the code prior to my message.
The problem that I see currently is that despite the capabilities string containing the "host" tag, with this cpu info - it is up to each hypervisor driver to implement the broadcasting of host features.
For example - Xen will report PAE,VMX,SVM - but my ovirt node running KVM currently only reports the following:
<host> <cpu> <arch>x86_64</arch> </cpu> </host>
It seems to me that it might be useful for some sort of "node" info driver, where we might be able to share code for hypervisor independent info about the physical machine it is running on.
We already have a place for this useful reusable node info code - the cunningly named src/nodeinfo.c :-)
We should not have to re-write the scanning of /proc/cpuinfo in every hypervisor driver, IMHO.
Totally agreed , this is why we put the node stuff in a separate nodeinfo.c file even though only QEMU driver currently uses it. Which reminds me that I need to hook it into the LXC driver. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Tue, Sep 16, 2008 at 04:45:09PM -0400, Ben Guthro wrote:
My concern is that adding to the nodeinfo struct breaks the API - such that the structs will be different sizes between versions.
Extending this structure would break the A _B_ I. <aside> Specifically, because of dynamic linking you can have two situations arising: (1) caller compiled against old libvirt links to newer libvirt (2) caller compiled against new libvirt links to older libvirt You cannot tell just from the pointer passed to virNodeGetInfo how large the caller's structure is, so you could end up overwriting memory beyond the structure in case (1). In calls such as virDomainInterfaceStats, I fixed this by having the caller pass both a pointer to the structure and the size of the caller's structure. This allows us to expand the structure in future in a way which won't break either case (1) or (2). I would encourage people designing future libvirt APIs which take a pointer to a structure, to also get the caller to pass the size of the structure. http://libvirt.org/html/libvirt-libvirt.html#virDomainInterfaceStats Another way to solve this, less elegant I think, is to have callers explicitly say "I was compiled for libvirt 0.4.5" when they connect, and then we can tell what size of structure they are expecting. This is somewhat analogous to the Linux personality(2) system call. </aside> In any case, I think the best place to return the flags is in the capabilities XML as simple text strings (the same strings as are used in /proc/cpuinfo). http://libvirt.org/html/libvirt-libvirt.html#virConnectGetCapabilities There shouldn't be any API or ABI problems if you extend the capabilities.
Also - this seems to be x86 specific. Are we primarily destined for x86? Or would this type of change be unacceptable due to not working on PPC, for example?
If you're returning text strings in the capabilities XML then it presumably will also work on ppc and other architectures. Rich. -- Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://et.redhat.com/~rjones/virt-top
On Wed, Sep 17, 2008 at 11:43:19AM +0100, Richard W.M. Jones wrote:
On Tue, Sep 16, 2008 at 04:45:09PM -0400, Ben Guthro wrote:
My concern is that adding to the nodeinfo struct breaks the API - such that the structs will be different sizes between versions.
Extending this structure would break the A _B_ I.
<aside>
Specifically, because of dynamic linking you can have two situations arising:
(1) caller compiled against old libvirt links to newer libvirt (2) caller compiled against new libvirt links to older libvirt
You cannot tell just from the pointer passed to virNodeGetInfo how large the caller's structure is, so you could end up overwriting memory beyond the structure in case (1).
In calls such as virDomainInterfaceStats, I fixed this by having the caller pass both a pointer to the structure and the size of the caller's structure. This allows us to expand the structure in future in a way which won't break either case (1) or (2). I would encourage people designing future libvirt APIs which take a pointer to a
How about just having a virVersion field that would tell you what version of the struct it is? This being on top of the check you have. That way you can also guard against functions that change the number of arguments, which would not change the size of the caller's structure.
On Wed, Sep 17, 2008 at 08:36:50AM -0400, Konrad Rzeszutek wrote:
On Wed, Sep 17, 2008 at 11:43:19AM +0100, Richard W.M. Jones wrote:
On Tue, Sep 16, 2008 at 04:45:09PM -0400, Ben Guthro wrote:
My concern is that adding to the nodeinfo struct breaks the API - such that the structs will be different sizes between versions.
Extending this structure would break the A _B_ I.
<aside>
Specifically, because of dynamic linking you can have two situations arising:
(1) caller compiled against old libvirt links to newer libvirt (2) caller compiled against new libvirt links to older libvirt
You cannot tell just from the pointer passed to virNodeGetInfo how large the caller's structure is, so you could end up overwriting memory beyond the structure in case (1).
In calls such as virDomainInterfaceStats, I fixed this by having the caller pass both a pointer to the structure and the size of the caller's structure. This allows us to expand the structure in future in a way which won't break either case (1) or (2). I would encourage people designing future libvirt APIs which take a pointer to a
How about just having a virVersion field that would tell you what version of the struct it is? This being on top of the check you have.
The more I think about this, having the caller pass both the pointer to the structure and the version would guard against the issues you mentioned (new fields added to the structure) and the case where a function prototype gets its arguments swapped around for example, and also for:
That way you can also guard against functions that change the number of arguments, which would not change the size of the caller's structure.
On Wed, Sep 17, 2008 at 08:36:50AM -0400, Konrad Rzeszutek wrote:
On Wed, Sep 17, 2008 at 11:43:19AM +0100, Richard W.M. Jones wrote:
On Tue, Sep 16, 2008 at 04:45:09PM -0400, Ben Guthro wrote:
My concern is that adding to the nodeinfo struct breaks the API - such that the structs will be different sizes between versions.
Extending this structure would break the A _B_ I.
<aside>
Specifically, because of dynamic linking you can have two situations arising:
(1) caller compiled against old libvirt links to newer libvirt (2) caller compiled against new libvirt links to older libvirt
You cannot tell just from the pointer passed to virNodeGetInfo how large the caller's structure is, so you could end up overwriting memory beyond the structure in case (1).
In calls such as virDomainInterfaceStats, I fixed this by having the caller pass both a pointer to the structure and the size of the caller's structure. This allows us to expand the structure in future in a way which won't break either case (1) or (2). I would encourage people designing future libvirt APIs which take a pointer to a
How about just having a virVersion field that would tell you what version of the struct it is? This being on top of the check you have.
That way you can also guard against functions that change the number of arguments, which would not change the size of the caller's structure.
I don't want us playing tricks with ABI based on the version number. It is quite easy to add new structs & APIs to the public interface without breaking ABI of existing structs & APIs. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
participants (5)
-
Ben Guthro -
Daniel P. Berrange -
John Levon -
Konrad Rzeszutek -
Richard W.M. Jones