3:45 p.m.
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
4:08 p.m.
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
4:42 a.m.
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 :|
7:44 a.m.
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.
8:14 a.m.
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/
8:17 a.m.
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 :|
8:52 a.m.
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
4:02 a.m.
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
4:08 a.m.
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 :|
5:43 a.m.
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
7:36 a.m.
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.
7:48 a.m.
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.
8:18 a.m.
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 :|
5910
days inactive
5912
days old
12 comments
5 participants
participants (5)
-
Ben Guthro -
Daniel P. Berrange -
John Levon -
Konrad Rzeszutek -
Richard W.M. Jones