Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On Sun, Nov 28, 2010 at 05:13:41PM -0600, Anthony Liguori wrote: On 11/28/2010 04:28 PM, Michael S. Tsirkin wrote: But rather need to use ugly factory functions with all sorts of DO_UPCAST. This is really unfriendly especially for writing test cases. Yes, I agree. Just moving memory allocation out of there will fix most of the ugliness. So here's a short list of things I've been working on that I don't believe have nice implementations in C. 1) Factories with string based parameters with natural constructor arguments. This was the only item, right? So in fact, this is needed as part of configuration file/command line/monitor parser. IMHO, this really should be separate from the device model. The fact that qdev currently mixes the device model with argument parsing is bad IMO. So we ended up with saying .driver = PCI in hw/pc_piix.c instead of an instance of the structure. There's no compile-time check that the correct string is used and that is pretty bad IMO. Yes, this makes it easier to add new properties, but making it easy is exactly the wrong thing to do because we really have to support such properties forever. So how about a compromise: libqemu written in C, with APIs that should not deal with string parsing at all, and should above all else make sense: i8254_init_drift_mode i8254_init_catchup net_set_link_up net_set_link_down (and it really needs to be C for portability: so that management written in C can use it). This API should be properly versioned, with a backwards compatibility story, and we should be careful about adding interfaces there. On top of this you can have a management interface written in any other language, and have that deal with string parsing. -- MST -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/28/2010 06:57 PM, Michael S. Tsirkin wrote:
sparce lets you solve C problems that C++ inherited as is.
E.g. if you have a pointer you can always dereference it.
It's the other way round.
For example __user cannot be done in C. It has to be done as an add-on.
In C++ it's simply:
templateclass T
class user_ptrT
{
public:
explicit user_ptr(unsigned long addr);
void copy_from(T to); // throws EFAULT
void copy_to(const T from); // throws EFAULT
private:
unsigned long addr;
};
This does not allow simple uses such as arithmetic,
Add a raw_addr() method that returns addr.
void,
Fixable.
builtin
types,
should work
sizeof,
sizeof(T)?
arrays,
should work
NULL comparizon,
Do we ever compare __user pointers against NULL? It's a valid address.
inheritance, cast,
What do these mean in the context of user pointers?
memory management.
And this?
Examples to ponder: what is the appropriate value of T for void *?
Probably a specialization user_ptr or a separate class user_void_ptr.
After all you can't do anthing with such a pointer.
What if you want a shared/auto ptr to manage this memory?
What does it mean for user pointers?
Some of these might be fixable, with a lot of code.
Boost might haver some solutions, I haven't looked.
Meanwhile sparse is already there.
With sparse you have to implement every rule in a separate compiler.
With C++ you introduce the rules into the code.
No need for an additional toolchain.
It's a feature :) This way you are not forced to rewrite all code each
time you realize you need an extra check, and checks can be added
gradually without breaking build.
You can see that user_ptr is not just for the checks, it adds
functionality (sizeof-less copy_from and copy_to). That's usually the
case. If there's something you must not do because of some rule,
there's also something you want to do, and those become member functions.
In C++ you could also introduce user_ptr gradually, it won't break
anything.
Things like __user are easily done in C++.
Some of what sparce does can be done if you create a separate type for
all address spaces. This can be done in C too, and the result won't
be like __user at all.
That's quite a lot of work.
Sparse: T __user *foo;
C++: user_ptrT foo;
Sparse has some advantages: it makes the contract obvious so you clearly
see it's a pointer and know -, [], + will work, * and will not.
I don't really see how you can tell this from __user. You have to look
up the definition. For user_ptr, the definition is actually available.
C : struct T_user_ptr { unsigned long addr } foo; + lots of accessors.
Some kind of macro can be closer to user_ptr above.
Those macros are called templates, and the compiler can check that they
are used correctly.
C++ support in gdb has some limitations
if you use overloading, exceptions, templates. The example posted here
uses two of these, so it would be harder to debug.
I haven't seen issues with overloading or exceptions.
Build your test with -g, fire up gdb from command line,
try to put a breakpoint in the constructor of
the fd object, maybe you will see what I mean :)
(gdb) break 'kvm::fd::fd'
Breakpoint 3 at 0x8049650: file api/kvmxx.cc, line 25.
Breakpoint 4 at 0x8049628: file api/kvmxx.cc, line 31.
Breakpoint 5 at 0x8049080: file api/kvmxx.cc, line 21
But it's hard to figure out that you need the kvm namespace. Your code
only has one namespace, but with multiple namespaces, you don't even
know in which namespace to look up the fd. With templates you might not
even know the fd class.
If you like, you can avoid namespaces and prefix everything with kvm_.
I never found it necessary.
An example of an issue with overloading is that gdb seems unable to resolve
them properly depending on the current scope. So you see a call to
foo() and want to put a breakpoint there, first problem is just to find
one which namespace it is in. Once you did the best
it can do it prompt you to select one of the overloaded options.
How do you know which one do you want? You don't, so you guess. Sometimes
gdb will guess, because of a complex set of name resolution rules, and
sometimes it will this wrongly.
Which is not what I want to spend mental cycles on when I am debugging a
problem.
Functions using exceptions can not be called from the gdb prompt
(gdb is not smart enough to catch them).
There are more issues.
That's not restricted to gdb. C has just three scopes: block (may
be nested), file static, and global. C++ has more. Stating
everything leads to verbose code and potential conflicts. Having
more scopes allows tighter code usually
but more head-scratching if
something goes wrong.
In my experience conflicts are very rare. But it's true that when
they happen
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/28/2010 04:40 PM, Michael S. Tsirkin wrote: On Sun, Nov 28, 2010 at 03:14:17PM +0200, Avi Kivity wrote: On 11/28/2010 01:44 PM, Michael S. Tsirkin wrote: On Sun, Nov 28, 2010 at 11:54:26AM +0200, Avi Kivity wrote: On 11/28/2010 11:50 AM, Michael S. Tsirkin wrote: Another problem is that there seem to be two memory allocations and a copy here, apparently just to simplify error handling. It might be fine for this test but won't scale for when performance matters. When it matters, we can fix it. I don't see msr read/write becoming a hot path. It will be very painful to fix it. Why? Because the API returns a vector. Returning an object does not involve a copy (return value optimization). Yes, but assigning the value in the code that uses it will, unless again you do this in an initializer. So do that. This code is not reusable. Everywhere you use an fd, you have to repeat this code. But that's not a lot of code. And you can abstract it away at a higher level. For example kvm_init and kvm_cleanup would setup/cleanup state in a consistent way. That's not what we see in C code. The error handling gets everywhere, obscuring what's actually going on, and usually getting it wrong by leaking. My experience tells me C++ code has much more boilerplate code that you are forced to repeat over and over. This is especially true for unix system programming: by the time you are done wrapping all of unix you have created more LOC than you are ever likely to save. You don't need to wrap everything. Sometimes you can get away with if (ret == -1) throw. But things like file descriptors are essential. class kvm::fd is reusable, if you embed it in another object you don't have to worry about errors any more (as long as the object's methods are exception safe). To get exception safe code, you have to constantly worry about errors. And it's easier to spot an unhandled return code than exception-unsafe code: gcc actually has __attribute__((warn_unused_result)) which might help catch common errors. No such tool to catch exception-unsafe code AFAIK. We see exactly how easy it is by the constant stream of patches that fix error paths into Linux. Most user space programs don't even care about errors because they're so difficult and annoying to get right. Yes, all the correctness is more or less pointless here. Like I said, this is an experiment to see what things look like. I guess each side will it as proving its claims. This is exactly what seems to be happening. I did my best to try and be objective and point out real issues, but you probably guessed which side I am on already :). The lack of the C++ compiler eats babies and line noise comments is appreciated. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/28/2010 04:49 PM, Michael S. Tsirkin wrote:
On Sun, Nov 28, 2010 at 03:15:52PM +0200, Avi Kivity wrote:
On 11/28/2010 01:49 PM, Michael S. Tsirkin wrote:
+++ b/api/kvmxx.cc
@@ -0,0 +1,168 @@
+#include kvmxx.h
+#includefcntl.h
+#includesys/ioctl.h
+#includesys/mman.h
I just realized this is wrong: I think you should wrap
the headers in extern C. Same for other headers.
I think system headers already do this
(otherwise it won't link -
int foo() is different than extern C { int foo(void); })
okay, but they aren't there for linux/kvm.h, are they?
I don't think they are needed - they only declare structures and
constants, not functions.
--
error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On Mon, Nov 29, 2010 at 11:22:44AM +0200, Avi Kivity wrote:
No need for an additional toolchain.
It's a feature :) This way you are not forced to rewrite all code each
time you realize you need an extra check, and checks can be added
gradually without breaking build.
You can see that user_ptr is not just for the checks, it adds
functionality (sizeof-less copy_from and copy_to). That's usually
the case. If there's something you must not do because of some
rule, there's also something you want to do, and those become member
functions.
In C++ you could also introduce user_ptr gradually, it won't break
anything.
Yes but in
void foo(void *p) {
bar(p);
}
both foo and bar must be converted.
Things like __user are easily done in C++.
Some of what sparce does can be done if you create a separate type for
all address spaces. This can be done in C too, and the result won't
be like __user at all.
That's quite a lot of work.
Sparse: T __user *foo;
C++: user_ptrT foo;
Sparse has some advantages: it makes the contract obvious so you clearly
see it's a pointer and know -, [], + will work, * and will not.
I don't really see how you can tell this from __user.
I can tell this is a pointer from T *foo :), and I can tell
it has some attribute.
You have to
look up the definition. For user_ptr, the definition is actually
available.
The definition for __user is also available:
#ifdef __CHECKER__
# define __user__attribute__((noderef, address_space(1)))
#else
# define __user
#endif
Which is a very transparent way to say: this is just a checker
attribute, it does not affect actual code.
With a template we go 'I have overridden + but compiler should
optimize it back to original'. Note the should :)
Templates are
indeed harder to debug, simply because names can become very long.
That's not the only problem. A bigger one is when you type tab to
complete function name and get a list of options to select from for each
of the times a template was instantiated. Only one of them is relevant
in a given scope. No hint is given which. Further when you step into
the template, the source does not give you any hint about the types
used.
Some of this is true for macros as well of course. Except people know
macros are bad and so make them thin wrappers around proper functions.
Or they simply avoid it and duplicate the code. You can't always
wrap functions with macros.
Always is a strong word. What are the examples of such duplicated code
in qemu? Let's see if they are easy to fix.
qemu in fact uses macros extensively (glue()), they are hardly readable.
Well, we don't have so many instances left anymore.
We used to have this in pci and I got rid of it pretty easily,
just passing in length at runtime. And it turned out the only reason
for it was because we didn't pass in the transaction length.
That technique (clean up APIs so we don't have to work around
them with macros) leads IMO to better code than just sticking
a tamplate around it.
Do a 'git grep hash' for examples of duplication.
I see. Don't know enough about tcg to fix unfortunately, but
the logic is different: e.g. sparc opcode table is completely
static, translation cache is dynamic, jump cache has
static size but dynamic content, qdict could do
with linear lookups just as well. So it could
just be different optimization strategies.
--
error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/29/2010 12:47 PM, Michael S. Tsirkin wrote:
On Mon, Nov 29, 2010 at 11:22:44AM +0200, Avi Kivity wrote:
No need for an additional toolchain.
It's a feature :) This way you are not forced to rewrite all code each
time you realize you need an extra check, and checks can be added
gradually without breaking build.
You can see that user_ptr is not just for the checks, it adds
functionality (sizeof-less copy_from and copy_to). That's usually
the case. If there's something you must not do because of some
rule, there's also something you want to do, and those become member
functions.
In C++ you could also introduce user_ptr gradually, it won't break
anything.
Yes but in
void foo(void *p) {
bar(p);
}
both foo and bar must be converted.
No. You can convert from user_ptr to __user * and back.
Sparse has some advantages: it makes the contract obvious so you clearly
see it's a pointer and know -, [], + will work, * and will not.
I don't really see how you can tell this from __user.
I can tell this is a pointer from T *foo :), and I can tell
it has some attribute.
You have to
look up the definition. For user_ptr, the definition is actually
available.
The definition for __user is also available:
#ifdef __CHECKER__
# define __user__attribute__((noderef, address_space(1)))
#else
# define __user
#endif
Which is a very transparent way to say: this is just a checker
attribute, it does not affect actual code.
Can you tell you must not dereference it?
With a template we go 'I have overridden + but compiler should
optimize it back to original'. Note the should :)
We do this in C all the time with pte_t and lots of inlines.
Do a 'git grep hash' for examples of duplication.
I see. Don't know enough about tcg to fix unfortunately, but
the logic is different: e.g. sparc opcode table is completely
static, translation cache is dynamic, jump cache has
static size but dynamic content, qdict could do
with linear lookups just as well. So it could
just be different optimization strategies.
The default is duplication. With C++ your default is
tr1::unordered_map and you can optimize it later if you like.
--
error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On Mon, Nov 29, 2010 at 12:52:29PM +0200, Avi Kivity wrote: The default is duplication. With C++ your default is tr1::unordered_map and you can optimize it later if you like. BTW not relevant to kvm, but for qemu, some people seem to care about building with an old migw compiler in Debian stable which is unlikely to have tr1. If we drop this we'll also have a working %llx in printf :) -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/29/2010 05:26 AM, Michael S. Tsirkin wrote: On Mon, Nov 29, 2010 at 12:52:29PM +0200, Avi Kivity wrote: The default is duplication. With C++ your default is tr1::unordered_map and you can optimize it later if you like. BTW not relevant to kvm, but for qemu, some people seem to care about building with an old migw compiler in Debian stable which is unlikely to have tr1. If we drop this we'll also have a working %llx in printf :) Boost has a very compatible tr1 library. Regards, Anthony Liguori -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/29/2010 02:04 AM, Michael S. Tsirkin wrote: On Sun, Nov 28, 2010 at 05:13:41PM -0600, Anthony Liguori wrote: On 11/28/2010 04:28 PM, Michael S. Tsirkin wrote: But rather need to use ugly factory functions with all sorts of DO_UPCAST. This is really unfriendly especially for writing test cases. Yes, I agree. Just moving memory allocation out of there will fix most of the ugliness. So here's a short list of things I've been working on that I don't believe have nice implementations in C. 1) Factories with string based parameters with natural constructor arguments. This was the only item, right? So in fact, this is needed as part of configuration file/command line/monitor parser. I really see it more as an API interface. I think the best long term architecture for QEMU is where qemu is launched as essentially a daemon and is manipulated via an RPC interface to create an initial device model, etc. IMHO, this really should be separate from the device model. The fact that qdev currently mixes the device model with argument parsing is bad IMO. So we ended up with saying .driver = PCI in hw/pc_piix.c instead of an instance of the structure. There's no compile-time check that the correct string is used and that is pretty bad IMO. Indeed. Yes, this makes it easier to add new properties, but making it easy is exactly the wrong thing to do because we really have to support such properties forever. One advantage of having a factory and the objects in a single place is that the factory is the long term supported interface and the objects can evolve in a more natural fashion. So how about a compromise: libqemu written in C, with APIs that should not deal with string parsing at all, and should above all else make sense: i8254_init_drift_mode i8254_init_catchup net_set_link_up net_set_link_down (and it really needs to be C for portability: so that management written in C can use it). This API should be properly versioned, with a backwards compatibility story, and we should be careful about adding interfaces there. On top of this you can have a management interface written in any other language, and have that deal with string parsing. I still dislike the idea of implementing an object system in C. Besides reinventing vtables, we'll have to also reinvent RTTI to allow for safe upcasting (which is unavoidable). But really, let's defer this discussion for when patches are available. I understand your objections but I'm pretty convinced that the code will speak for itself when it's ready. Regards, Anthony Liguori -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/29/2010 03:44 PM, Anthony Liguori wrote: But really, let's defer this discussion for when patches are available. I understand your objections but I'm pretty convinced that the code will speak for itself when it's ready. It will, but everyone will hear something different. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On Sat, Nov 27, 2010 at 11:12:58AM +0200, Avi Kivity wrote:
On 11/26/2010 12:16 PM, Michael S. Tsirkin wrote:
On Wed, Nov 24, 2010 at 12:52:11PM +0200, Avi Kivity wrote:
Introduce exception-safe objects for calling system, vm, and vcpu ioctls.
Signed-off-by: Avi Kivitya...@redhat.com
ioctlp calls below ignore possible errors.
Somre more comments below.
Can you elaborate? The simply propagate the exception.
I was confused by this:
+long ioctlp(unsigned nr, void *arg) {
+ return ioctl(nr, reinterpret_castlong(arg));
+}
ioctl here is not the C ioctl function. It's the local method that
throws exceptions on errors. This will likely confuse others
as well.
+
+vcpu::~vcpu()
+{
+unsigned mmap_size = _vm._system._fd.ioctl(KVM_GET_VCPU_MMAP_SIZE,
0);
This might throw an exception on destructor path, if this happens
because an exception was thrown terminate is called.
Fixed.
+
+std::vectorkvm_msr_entry vcpu::msrs(std::vectoruint32_t indices)
+{
+std::auto_ptrkvm_msrs msrs(alloc_msr_list(indices.size()));
This looks wrong. auto_ptr frees memory with delete,
alloc_msr_list allocates it with malloc.
Anthony pointed this out as well.
Another problem is that there seem to be two memory allocations and a
copy here, apparently just to simplify error handling. It might be fine
for this test but won't scale for when performance matters.
Fixed by replacing
alloc_msr_list() by an object.
It seems that any action which has side effects which needs to be
undone on error, we will have to have a new class with constructor doing
the work. This will likely create much more lines of code
than a simple goto end strategy.
One also wonders how well will the compiler be able to optimize
such complex code, and by how much will compile times go up.
Any input on that?
--
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/28/2010 10:58 AM, Michael S. Tsirkin wrote:
On Sat, Nov 27, 2010 at 11:12:58AM +0200, Avi Kivity wrote:
On 11/26/2010 12:16 PM, Michael S. Tsirkin wrote:
On Wed, Nov 24, 2010 at 12:52:11PM +0200, Avi Kivity wrote:
Introduce exception-safe objects for calling system, vm, and vcpu
ioctls.
Signed-off-by: Avi Kivitya...@redhat.com
ioctlp calls below ignore possible errors.
Somre more comments below.
Can you elaborate? The simply propagate the exception.
I was confused by this:
+long ioctlp(unsigned nr, void *arg) {
+ return ioctl(nr, reinterpret_castlong(arg));
+}
ioctl here is not the C ioctl function. It's the local method that
throws exceptions on errors. This will likely confuse others
as well.
Could do this-ioctl(), though I don't much like it.
+
+std::vectorkvm_msr_entry vcpu::msrs(std::vectoruint32_t
indices)
+{
+std::auto_ptrkvm_msrs msrs(alloc_msr_list(indices.size()));
This looks wrong. auto_ptr frees memory with delete,
alloc_msr_list allocates it with malloc.
Anthony pointed this out as well.
Another problem is that there seem to be two memory allocations and a
copy here, apparently just to simplify error handling. It might be fine
for this test but won't scale for when performance matters.
When it matters, we can fix it. I don't see msr read/write becoming a
hot path.
Fixed by replacing
alloc_msr_list() by an object.
It seems that any action which has side effects which needs to be
undone on error, we will have to have a new class with constructor doing
the work. This will likely create much more lines of code
than a simple goto end strategy.
It creates correctness. The equivalent in qemu is to create a constant
size array on the stack, because people can't be bothered with error
checking.
The lines of code pay back as soon as there is some reuse (2x in this case).
One also wonders how well will the compiler be able to optimize
such complex code, and by how much will compile times go up.
Any input on that?
The compiler should optimize it away completely. There's been a lot of
work in gcc on that.
About compile times, I don't care much.
--
error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On Sun, Nov 28, 2010 at 11:31:09AM +0200, Avi Kivity wrote:
On 11/28/2010 10:58 AM, Michael S. Tsirkin wrote:
On Sat, Nov 27, 2010 at 11:12:58AM +0200, Avi Kivity wrote:
On 11/26/2010 12:16 PM, Michael S. Tsirkin wrote:
On Wed, Nov 24, 2010 at 12:52:11PM +0200, Avi Kivity wrote:
Introduce exception-safe objects for calling system, vm, and vcpu
ioctls.
Signed-off-by: Avi Kivitya...@redhat.com
ioctlp calls below ignore possible errors.
Somre more comments below.
Can you elaborate? The simply propagate the exception.
I was confused by this:
+long ioctlp(unsigned nr, void *arg) {
+ return ioctl(nr, reinterpret_castlong(arg));
+}
ioctl here is not the C ioctl function. It's the local method that
throws exceptions on errors. This will likely confuse others
as well.
Could do this-ioctl(), though I don't much like it.
+
+std::vectorkvm_msr_entry vcpu::msrs(std::vectoruint32_t
indices)
+{
+std::auto_ptrkvm_msrs msrs(alloc_msr_list(indices.size()));
This looks wrong. auto_ptr frees memory with delete,
alloc_msr_list allocates it with malloc.
Anthony pointed this out as well.
Another problem is that there seem to be two memory allocations and a
copy here, apparently just to simplify error handling. It might be fine
for this test but won't scale for when performance matters.
When it matters, we can fix it. I don't see msr read/write becoming
a hot path.
It will be very painful to fix it.
Fixed by replacing
alloc_msr_list() by an object.
It seems that any action which has side effects which needs to be
undone on error, we will have to have a new class with constructor doing
the work. This will likely create much more lines of code
than a simple goto end strategy.
It creates correctness. The equivalent in qemu is to create a
constant size array on the stack, because people can't be bothered
with error checking.
The lines of code pay back as soon as there is some reuse (2x in this case).
One also wonders how well will the compiler be able to optimize
such complex code, and by how much will compile times go up.
Any input on that?
The compiler should optimize it away completely.
Should as opposed to does. Want me to try a simple test?
There's been a lot
of work in gcc on that.
About compile times, I don't care much.
I do. You will too when we have codebase that can be built as fast as
we commit things, so buildbot breaks.
This is common in C++ based projects.
--
error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/28/2010 11:50 AM, Michael S. Tsirkin wrote: Another problem is that there seem to be two memory allocations and a copy here, apparently just to simplify error handling. It might be fine for this test but won't scale for when performance matters. When it matters, we can fix it. I don't see msr read/write becoming a hot path. It will be very painful to fix it. Why? One copy is necessary (it's due to the bad kvm API), but we can avoid the others. In any case the data will be copied by the kernel. The compiler should optimize it away completely. Should as opposed to does. Want me to try a simple test? Please. There's been a lot of work in gcc on that. About compile times, I don't care much. I do. You will too when we have codebase that can be built as fast as we commit things, so buildbot breaks. This is common in C++ based projects. If kvm-unit-tests.git takes to long to compile, I'll be very happy. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On Sun, Nov 28, 2010 at 11:54:26AM +0200, Avi Kivity wrote:
On 11/28/2010 11:50 AM, Michael S. Tsirkin wrote:
Another problem is that there seem to be two memory allocations and a
copy here, apparently just to simplify error handling. It might be fine
for this test but won't scale for when performance matters.
When it matters, we can fix it. I don't see msr read/write becoming
a hot path.
It will be very painful to fix it.
Why?
Because the API returns a vector.
One copy is necessary (it's due to the bad kvm API), but we
can avoid the others.
In any case the data will be copied by the kernel.
The compiler should optimize it away completely.
Should as opposed to does. Want me to try a simple test?
Please.
Just for fun: optimize for size, and compare code sizes.
The C++ code is yours but I have removed all use of STL to make
it more of an even fight. I checked by object and executable size.
Note that this is shared library build: a C++ executable
will pull in a large C++ library, a C executable won't.
If you are interested in an STL based example let me know.
You can take it from here and make it more real if you like,
or look at the assembler output.
--
[...@tuck ~]$ cat test.c
#include sys/ioctl.h
#include sys/types.h
#include sys/stat.h
#include fcntl.h
#include unistd.h
#include errno.h
int main(int argc, char **argv)
{
int fd = open(/dev/kvm, O_RDWR);
int r;
if (fd 0)
goto open_err;
r = ioctl(fd, 0, 0);
if (r 0)
goto ioctl_err;
return 0;
ioctl_err:
close(fd);
open_err:
return -1;
}
[...@tuck ~]$ gcc -c -Os test.c
[...@tuck ~]$ size test.o
textdata bss dec hex filename
97 0 0 97 61 test.o
[...@tuck ~]$ gcc -Os test.c
[...@tuck ~]$ size a.out
textdata bss dec hex filename
1192 260 81460 5b4 a.out
[...@tuck ~]$ wc -l test.c
22 test.c
--
[...@tuck ~]$ cat kvmxx.cpp
extern C {
#include sys/ioctl.h
#include sys/types.h
#include sys/stat.h
#include fcntl.h
#include unistd.h
#include errno.h
}
namespace kvm {
class fd {
public:
explicit fd(const char *path, int flags);
~fd() { ::close(_fd); }
long ioctl(unsigned nr, long arg);
private:
int _fd;
};
};
namespace kvm {
static long check_error(long r)
{
if (r == -1) {
throw errno;
}
return r;
}
fd::fd(const char *device_node, int flags)
: _fd(::open(device_node, flags))
{
check_error(_fd);
}
long fd::ioctl(unsigned nr, long arg)
{
return check_error(::ioctl(_fd, nr, arg));
}
}
int main(int ac, char **av)
{
try {
kvm::fd fd(/dev/kvm, O_RDWR);
fd.ioctl(0, 0);
} catch (...) {
return -1;
}
return 0;
}
[...@tuck ~]$ g++ -c -Os kvmxx.cpp
[...@tuck ~]$ size kvmxx.o
textdata bss dec hex filename
529 0 0 529 211 kvmxx.o
[...@tuck ~]$ g++ -Os kvmxx.cpp
[...@tuck ~]$ size a.out
textdata bss dec hex filename
2254 308 162578 a12 a.out
[...@tuck ~]$ wc kvmxx.cpp
56 kvmxx.cpp
--
One interesting thing is that the object size grew
faster than linked executable size.
This might mean that the compiler generated some
dead code that the linker then threw out.
It's also interesting that C++ managed to use up
more data/bss storage.
There's been a lot
of work in gcc on that.
About compile times, I don't care much.
I do. You will too when we have codebase that can be built as fast as
we commit things, so buildbot breaks.
This is common in C++ based projects.
If kvm-unit-tests.git takes to long to compile, I'll be very happy.
If the claim is it's so small it does not matter what it's written in
then I guess don't mind. But then - why do we care about
error handling so much? For the test, it's probably ok to just assert
after each ioctl/malloc and be done with it.
--
error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On Wed, Nov 24, 2010 at 12:52:11PM +0200, Avi Kivity wrote:
Introduce exception-safe objects for calling system, vm, and vcpu ioctls.
Signed-off-by: Avi Kivity a...@redhat.com
---
api/kvmxx.cc | 168
++
api/kvmxx.h | 80 +++
2 files changed, 248 insertions(+), 0 deletions(-)
create mode 100644 api/kvmxx.cc
create mode 100644 api/kvmxx.h
diff --git a/api/kvmxx.cc b/api/kvmxx.cc
new file mode 100644
index 000..2f8fc27
--- /dev/null
+++ b/api/kvmxx.cc
@@ -0,0 +1,168 @@
+#include kvmxx.h
+#include fcntl.h
+#include sys/ioctl.h
+#include sys/mman.h
I just realized this is wrong: I think you should wrap
the headers in extern C. Same for other headers.
+#include memory
+#include algorithm
+
+namespace kvm {
+
+static long check_error(long r)
+{
+if (r == -1) {
+ throw errno;
+}
+return r;
+}
+
+fd::fd(int fd)
+: _fd(fd)
+{
+}
+
+fd::fd(const fd other)
+: _fd(::dup(other._fd))
+{
+check_error(_fd);
+}
+
+fd::fd(std::string device_node, int flags)
+: _fd(::open(device_node.c_str(), flags))
+{
+check_error(_fd);
+}
+
+long fd::ioctl(unsigned nr, long arg)
+{
+return check_error(::ioctl(_fd, nr, arg));
+}
+
+vcpu::vcpu(vm vm, int id)
+: _vm(vm), _fd(vm._fd.ioctl(KVM_CREATE_VCPU, id)), _shared(NULL)
+{
+unsigned mmap_size = _vm._system._fd.ioctl(KVM_GET_VCPU_MMAP_SIZE, 0);
+kvm_run *shared = static_castkvm_run*(::mmap(NULL, mmap_size,
+PROT_READ | PROT_WRITE,
+MAP_SHARED,
+_fd.get(), 0));
+if (shared == MAP_FAILED) {
+ throw errno;
+}
+_shared = shared;
+}
+
+vcpu::~vcpu()
+{
+unsigned mmap_size = _vm._system._fd.ioctl(KVM_GET_VCPU_MMAP_SIZE, 0);
+munmap(_shared, mmap_size);
+}
+
+void vcpu::run()
+{
+_fd.ioctl(KVM_RUN, 0);
+}
+
+kvm_regs vcpu::regs()
+{
+kvm_regs regs;
+_fd.ioctlp(KVM_GET_REGS, regs);
+return regs;
+}
+
+void vcpu::set_regs(const kvm_regs regs)
+{
+_fd.ioctlp(KVM_SET_REGS, const_castkvm_regs*(regs));
+}
+
+kvm_sregs vcpu::sregs()
+{
+kvm_sregs sregs;
+_fd.ioctlp(KVM_GET_SREGS, sregs);
+return sregs;
+}
+
+void vcpu::set_sregs(const kvm_sregs sregs)
+{
+_fd.ioctlp(KVM_SET_SREGS, const_castkvm_sregs*(sregs));
+}
+
+kvm_msrs* vcpu::alloc_msr_list(size_t nmsrs)
+{
+size_t size = sizeof(kvm_msrs) + sizeof(kvm_msr_entry) * nmsrs;
+kvm_msrs* ret = static_castkvm_msrs*(malloc(size));
+if (!ret) {
+ throw ENOMEM;
+}
+return ret;
+}
+
+std::vectorkvm_msr_entry vcpu::msrs(std::vectoruint32_t indices)
+{
+std::auto_ptrkvm_msrs msrs(alloc_msr_list(indices.size()));
+msrs-nmsrs = indices.size();
+for (unsigned i = 0; i msrs-nmsrs; ++i) {
+ msrs-entries[i].index = indices[i];
+}
+_fd.ioctlp(KVM_GET_MSRS, msrs.get());
+return std::vectorkvm_msr_entry(msrs-entries,
+ msrs-entries + msrs-nmsrs);
+}
+
+void vcpu::set_msrs(const std::vectorkvm_msr_entry msrs)
+{
+std::auto_ptrkvm_msrs _msrs(alloc_msr_list(msrs.size()));
+_msrs-nmsrs = msrs.size();
+std::copy(msrs.begin(), msrs.end(), _msrs-entries);
+_fd.ioctlp(KVM_SET_MSRS, _msrs.get());
+}
+
+void vcpu::set_debug(uint64_t dr[8], bool enabled, bool singlestep)
+{
+kvm_guest_debug gd;
+
+gd.control = 0;
+if (enabled) {
+ gd.control |= KVM_GUESTDBG_ENABLE;
+}
+if (singlestep) {
+ gd.control |= KVM_GUESTDBG_SINGLESTEP;
+}
+for (int i = 0; i 8; ++i) {
+ gd.arch.debugreg[i] = dr[i];
+}
+_fd.ioctlp(KVM_SET_GUEST_DEBUG, gd);
+}
+
+vm::vm(system system)
+: _system(system), _fd(system._fd.ioctl(KVM_CREATE_VM, 0))
+{
+}
+
+void vm::set_memory_region(int slot, void *addr, uint64_t gpa, size_t len)
+{
+struct kvm_userspace_memory_region umr;
+
+umr.slot = slot;
+umr.flags = 0;
+umr.guest_phys_addr = gpa;
+umr.memory_size = len;
+umr.userspace_addr = reinterpret_castuint64_t(addr);
+_fd.ioctlp(KVM_SET_USER_MEMORY_REGION, umr);
+}
+
+void vm::set_tss_addr(uint32_t addr)
+{
+_fd.ioctl(KVM_SET_TSS_ADDR, addr);
+}
+
+system::system(std::string device_node)
+: _fd(device_node, O_RDWR)
+{
+}
+
+bool system::check_extension(int extension)
+{
+return _fd.ioctl(KVM_CHECK_EXTENSION, extension);
+}
+
+};
diff --git a/api/kvmxx.h b/api/kvmxx.h
new file mode 100644
index 000..716e400
--- /dev/null
+++ b/api/kvmxx.h
@@ -0,0 +1,80 @@
+#ifndef KVMXX_H
+#define KVMXX_H
+
+#include string
+#include signal.h
+#include unistd.h
+#include vector
+#include errno.h
+#include linux/kvm.h
+#include stdint.h
+
+namespace kvm {
+
+class system;
+class vm;
+class
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On Wed, Nov 24, 2010 at 01:59:14PM +0100, Alexander Graf wrote: On 24.11.2010, at 11:52, Avi Kivity wrote: Introduce exception-safe objects for calling system, vm, and vcpu ioctls. Signed-off-by: Avi Kivity a...@redhat.com FWIW, I still disagree with C++ and believe this code to be hardly readable. A major issue is existing tools. Using C++ would prevent us from using sparce for static code checking. We should be adding more annotations instead of throwing existing ones out. ctags is also broken with C++ which will make it much harder for me to browse the codebase. C++ support in gdb has some limitations if you use overloading, exceptions, templates. The example posted here uses two of these, so it would be harder to debug. I also hoped we'll be able to adopt checkpatch at some point for coding style enforcement, C++ syntax is just too complex for a perl script to be of any use. -- MST -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On Wed, Nov 24, 2010 at 08:41:26AM -0600, Anthony Liguori wrote:
On 11/24/2010 06:59 AM, Alexander Graf wrote:
On 24.11.2010, at 11:52, Avi Kivity wrote:
Introduce exception-safe objects for calling system, vm, and vcpu ioctls.
Signed-off-by: Avi Kivitya...@redhat.com
FWIW, I still disagree with C++ and believe this code to be hardly readable.
There's a general prettiness that well written C++ code will have
over C when there's heavy object modelling. This can be subjective
but for me, it's fairly significant.
My guess is this comes from the fact that you are rewriting large pieces
of code from scratch so it suits your personal style perfectly :)
If history teaches us anything, as with most projects in qemu, what we will
end up with is a half done conversion of maybe 30% of the codebase.
The result might be anything: safer, more correct - but it won't be prettier.
The fact that objects are easily created on the stack and on the
heap is also pretty significant.
Significant how?
To create an object on the stack, you must have the class definition in
a public header and a public constructor/destructor.
This is exactly the same in C.
When considering device models, we
struggle today with device composition.
IMO this has nothing to do with the language and everything to do with
how trying to do the changes in small incremental steps and keep a lot
of configurations working.
In real hardware, the i8042 (keyboard controller) is actually
implemented in the PIIX3 which is a chip that is part of the i440fx.
The i440fx acts as both the memory controller and as the PCI Host
controller. So you get something that looks like:
class PIIX3 : public PCIDevice
{
private:
I8042 i8042;
RTC rtc;
// ...
};
class I440FX : public PCIHostController
{
I440FX(void) {
this-slots[1].plug(this-piix3); // piix3 is always in slot 1
}
private:
PlugPCIDevice * slots[32]; // slot 0 is the PMC
PIIX3 piix3;
};
We can have the same thing today. In fact, getting rid of the UP_CAST
and opaque pointers should be a priority.
So whereas we have this very complicate machine create function that
attempts to create and composite all of these devices after the
fact, when written in C++, partially due to good design, but
partially due to the fact that the languages forces you to think a
certain way, you get a tremendous simplification.
A proper C++ device model turns a vast majority of our device
creation complexity into a single new I440FX. Then it's just a
matter of instantiating and plugging the appropriate set of PCI
devices.
Of course, this can be wrapped in a factory to make it drivable via
an API or config file.
Another area that C++ shines is safety. C++ enables you to inject
safe versions of things that you really can't do in C. For
instance, the PIT has three channels but the mask to select a
channel is two bits. There was a kernel exploit that found a way to
trick selection of a forth channel because of a missing check.
In C++, you can convert:
PITChannel channnels[3];
Into:
ArrayPITChannel, 3 channels;
It behaves in every other way just like a normal array. The memory
is stack allocated, the type has a fixed size. The only difference
is that you can overload the [] operators and implement bounds
checking for array accesses. This means that as long as you use
Array, array overflows disappear from the code base. That's a big
deal.
Except that you get used to the fact that [] is safe,
and then forget to check the value in a dynamically
sized array access. Boom.
Much better to be able to differentiate between
safe and unsafe calls IMO.
Another area C++ shines is generating metacode. Consider the
ugliness around VMState. The crux of the problem is that it's not
possible to write type-neutral code in C. This all gets simplified
with C++. Instead of having a bunch of macros like:
VMSTATE_INT8(val0, ...)
VMSTATE_INT16(val1, ...)
You can just have:
vmstate(val0)
vmstate(val1)
And use type overloading to implement different behaviors. Combined
with template specialization and an Array wrapper, the same thing
works for arrays too.
Regards,
Anthony Liguori
Regards,
Anthony Liguori
At least with VMSTATE_INT16 I can grep and find the definition.
Alex
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/28/2010 01:59 PM, Michael S. Tsirkin wrote: FWIW, I still disagree with C++ and believe this code to be hardly readable. A major issue is existing tools. Using C++ would prevent us from using sparce for static code checking. C++ static checking is way better than anything sparse offers. Things like __user are easily done in C++. We should be adding more annotations instead of throwing existing ones out. ctags is also broken with C++ which will make it much harder for me to browse the codebase. C++ does want a good IDE. C++ support in gdb has some limitations if you use overloading, exceptions, templates. The example posted here uses two of these, so it would be harder to debug. I haven't seen issues with overloading or exceptions. Templates are indeed harder to debug, simply because names can become very long. I also hoped we'll be able to adopt checkpatch at some point for coding style enforcement, C++ syntax is just too complex for a perl script to be of any use. Not much of a loss IMO. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/28/2010 01:44 PM, Michael S. Tsirkin wrote:
On Sun, Nov 28, 2010 at 11:54:26AM +0200, Avi Kivity wrote:
On 11/28/2010 11:50 AM, Michael S. Tsirkin wrote:
Another problem is that there seem to be two memory allocations and a
copy here, apparently just to simplify error handling. It might be
fine
for this test but won't scale for when performance matters.
When it matters, we can fix it. I don't see msr read/write becoming
a hot path.
It will be very painful to fix it.
Why?
Because the API returns a vector.
Returning an object does not involve a copy (return value optimization).
The compiler should optimize it away completely.
Should as opposed to does. Want me to try a simple test?
Please.
Just for fun: optimize for size, and compare code sizes.
The C++ code is yours but I have removed all use of STL to make
it more of an even fight. I checked by object and executable size.
Note that this is shared library build: a C++ executable
will pull in a large C++ library, a C executable won't.
If you are interested in an STL based example let me know.
You can take it from here and make it more real if you like,
or look at the assembler output.
--
[...@tuck ~]$ cat test.c
#includesys/ioctl.h
#includesys/types.h
#includesys/stat.h
#includefcntl.h
#includeunistd.h
#includeerrno.h
int main(int argc, char **argv)
{
int fd = open(/dev/kvm, O_RDWR);
int r;
if (fd 0)
goto open_err;
r = ioctl(fd, 0, 0);
if (r 0)
goto ioctl_err;
return 0;
ioctl_err:
close(fd);
open_err:
return -1;
}
This code is not reusable. Everywhere you use an fd, you have to repeat
this code.
[...@tuck ~]$ gcc -c -Os test.c
[...@tuck ~]$ size test.o
textdata bss dec hex filename
97 0 0 97 61 test.o
[...@tuck ~]$ gcc -Os test.c
[...@tuck ~]$ size a.out
textdata bss dec hex filename
1192 260 81460 5b4 a.out
[...@tuck ~]$ wc -l test.c
22 test.c
--
[...@tuck ~]$ cat kvmxx.cpp
extern C {
#includesys/ioctl.h
#includesys/types.h
#includesys/stat.h
#includefcntl.h
#includeunistd.h
#includeerrno.h
}
namespace kvm {
class fd {
public:
explicit fd(const char *path, int flags);
~fd() { ::close(_fd); }
long ioctl(unsigned nr, long arg);
private:
int _fd;
};
};
namespace kvm {
static long check_error(long r)
{
if (r == -1) {
throw errno;
}
return r;
}
fd::fd(const char *device_node, int flags)
: _fd(::open(device_node, flags))
{
check_error(_fd);
}
long fd::ioctl(unsigned nr, long arg)
{
return check_error(::ioctl(_fd, nr, arg));
}
}
int main(int ac, char **av)
{
try {
kvm::fd fd(/dev/kvm, O_RDWR);
fd.ioctl(0, 0);
} catch (...) {
return -1;
}
return 0;
}
class kvm::fd is reusable, if you embed it in another object you don't
have to worry about errors any more (as long as the object's methods are
exception safe).
[...@tuck ~]$ g++ -c -Os kvmxx.cpp
[...@tuck ~]$ size kvmxx.o
textdata bss dec hex filename
529 0 0 529 211 kvmxx.o
[...@tuck ~]$ g++ -Os kvmxx.cpp
[...@tuck ~]$ size a.out
textdata bss dec hex filename
2254 308 162578 a12 a.out
[...@tuck ~]$ wc kvmxx.cpp
56 kvmxx.cpp
--
One interesting thing is that the object size grew
faster than linked executable size.
This might mean that the compiler generated some
dead code that the linker then threw out.
It's also interesting that C++ managed to use up
more data/bss storage.
C++ will have much larger data and code sizes because it uses DWARF
tables for unwinding and generates stack unwinding code. These are all
out of the hot path.
There's been a lot
of work in gcc on that.
About compile times, I don't care much.
I do. You will too when we have codebase that can be built as fast as
we commit things, so buildbot breaks.
This is common in C++ based projects.
If kvm-unit-tests.git takes to long to compile, I'll be very happy.
If the claim is it's so small it does not matter what it's written in
then I guess don't mind. But then - why do we care about
error handling so much? For the test, it's probably ok to just assert
after each ioctl/malloc and be done with it.
Yes, all the correctness is more or less pointless here. Like I said,
this is an experiment to see what things look like. I guess each side
will it as proving its claims.
--
error compiling committee.c: too many arguments to function
--
To
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/28/2010 01:49 PM, Michael S. Tsirkin wrote:
+++ b/api/kvmxx.cc
@@ -0,0 +1,168 @@
+#include kvmxx.h
+#includefcntl.h
+#includesys/ioctl.h
+#includesys/mman.h
I just realized this is wrong: I think you should wrap
the headers in extern C. Same for other headers.
I think system headers already do this (otherwise it won't link - int
foo() is different than extern C { int foo(void); })
--
error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On Sun, Nov 28, 2010 at 03:02:18PM +0200, Avi Kivity wrote: On 11/28/2010 01:59 PM, Michael S. Tsirkin wrote: FWIW, I still disagree with C++ and believe this code to be hardly readable. A major issue is existing tools. Using C++ would prevent us from using sparce for static code checking. C++ static checking is way better than anything sparse offers. This seems a second system effect at work. sparce lets you solve C problems that C++ inherited as is. E.g. if you have a pointer you can always dereference it. Things like __user are easily done in C++. Some of what sparce does can be done if you create a separate type for all address spaces. This can be done in C too, and the result won't be like __user at all. We should be adding more annotations instead of throwing existing ones out. ctags is also broken with C++ which will make it much harder for me to browse the codebase. C++ does want a good IDE. For some definitions of good :). Let's start a vi versus emacs flamewar? C++ support in gdb has some limitations if you use overloading, exceptions, templates. The example posted here uses two of these, so it would be harder to debug. I haven't seen issues with overloading or exceptions. Build your test with -g, fire up gdb from command line, try to put a breakpoint in the constructor of the fd object, maybe you will see what I mean :) An example of an issue with overloading is that gdb seems unable to resolve them properly depending on the current scope. So you see a call to foo() and want to put a breakpoint there, first problem is just to find one which namespace it is in. Once you did the best it can do it prompt you to select one of the overloaded options. How do you know which one do you want? You don't, so you guess. Sometimes gdb will guess, because of a complex set of name resolution rules, and sometimes it will this wrongly. Which is not what I want to spend mental cycles on when I am debugging a problem. Functions using exceptions can not be called from the gdb prompt (gdb is not smart enough to catch them). There are more issues. Templates are indeed harder to debug, simply because names can become very long. That's not the only problem. A bigger one is when you type tab to complete function name and get a list of options to select from for each of the times a template was instantiated. Only one of them is relevant in a given scope. No hint is given which. Further when you step into the template, the source does not give you any hint about the types used. Some of this is true for macros as well of course. Except people know macros are bad and so make them thin wrappers around proper functions. I also hoped we'll be able to adopt checkpatch at some point for coding style enforcement, C++ syntax is just too complex for a perl script to be of any use. Not much of a loss IMO. This is just an example of how coding style is harder to specify and enforce. There are just much more ways to do any given thing. Uniform style is much harder to achieve. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/28/2010 03:57 PM, Michael S. Tsirkin wrote:
On Sun, Nov 28, 2010 at 03:02:18PM +0200, Avi Kivity wrote:
On 11/28/2010 01:59 PM, Michael S. Tsirkin wrote:
FWIW, I still disagree with C++ and believe this code to be hardly
readable.
A major issue is existing tools.
Using C++ would prevent us from using sparce for static code checking.
C++ static checking is way better than anything sparse offers.
This seems a second system effect at work.
sparce lets you solve C problems that C++ inherited as is.
E.g. if you have a pointer you can always dereference it.
It's the other way round.
For example __user cannot be done in C. It has to be done as an add-on.
In C++ it's simply:
template class T
class user_ptrT
{
public:
explicit user_ptr(unsigned long addr);
void copy_from(T to); // throws EFAULT
void copy_to(const T from); // throws EFAULT
private:
unsigned long addr;
};
No need for an additional toolchain.
Things like __user are easily done in C++.
Some of what sparce does can be done if you create a separate type for
all address spaces. This can be done in C too, and the result won't
be like __user at all.
That's quite a lot of work.
Sparse: T __user *foo;
C++: user_ptrT foo;
C : struct T_user_ptr { unsigned long addr } foo; + lots of accessors.
We should be adding more annotations instead of throwing existing ones
out. ctags is also broken with C++ which will make it much harder
for me to browse the codebase.
C++ does want a good IDE.
For some definitions of good :).
Let's start a vi versus emacs flamewar?
Like we haven't enough...
C++ support in gdb has some limitations
if you use overloading, exceptions, templates. The example posted here
uses two of these, so it would be harder to debug.
I haven't seen issues with overloading or exceptions.
Build your test with -g, fire up gdb from command line,
try to put a breakpoint in the constructor of
the fd object, maybe you will see what I mean :)
(gdb) break 'kvm::fd::fd'
Breakpoint 3 at 0x8049650: file api/kvmxx.cc, line 25.
Breakpoint 4 at 0x8049628: file api/kvmxx.cc, line 31.
Breakpoint 5 at 0x8049080: file api/kvmxx.cc, line 21
An example of an issue with overloading is that gdb seems unable to resolve
them properly depending on the current scope. So you see a call to
foo() and want to put a breakpoint there, first problem is just to find
one which namespace it is in. Once you did the best
it can do it prompt you to select one of the overloaded options.
How do you know which one do you want? You don't, so you guess. Sometimes
gdb will guess, because of a complex set of name resolution rules, and
sometimes it will this wrongly.
Which is not what I want to spend mental cycles on when I am debugging a
problem.
Functions using exceptions can not be called from the gdb prompt
(gdb is not smart enough to catch them).
There are more issues.
That's not restricted to gdb. C has just three scopes: block (may be
nested), file static, and global. C++ has more. Stating everything
leads to verbose code and potential conflicts. Having more scopes
allows tighter code usually but more head-scratching if something goes
wrong.
In my experience conflicts are very rare. But it's true that when they
happen they can be surprising.
Templates are
indeed harder to debug, simply because names can become very long.
That's not the only problem. A bigger one is when you type tab to
complete function name and get a list of options to select from for each
of the times a template was instantiated. Only one of them is relevant
in a given scope. No hint is given which. Further when you step into
the template, the source does not give you any hint about the types
used.
Some of this is true for macros as well of course. Except people know
macros are bad and so make them thin wrappers around proper functions.
Or they simply avoid it and duplicate the code. You can't always wrap
functions with macros.
I also hoped we'll be able to adopt checkpatch at some point for coding
style enforcement, C++ syntax is just too complex for a perl script to
be of any use.
Not much of a loss IMO.
This is just an example of how coding style is harder to
specify and enforce. There are just much more ways to do
any given thing. Uniform style is much harder to achieve.
The language does more, so it's more complicated.
--
error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On Sun, Nov 28, 2010 at 03:14:17PM +0200, Avi Kivity wrote:
On 11/28/2010 01:44 PM, Michael S. Tsirkin wrote:
On Sun, Nov 28, 2010 at 11:54:26AM +0200, Avi Kivity wrote:
On 11/28/2010 11:50 AM, Michael S. Tsirkin wrote:
Another problem is that there seem to be two memory allocations and
a
copy here, apparently just to simplify error handling. It might be
fine
for this test but won't scale for when performance matters.
When it matters, we can fix it. I don't see msr read/write becoming
a hot path.
It will be very painful to fix it.
Why?
Because the API returns a vector.
Returning an object does not involve a copy (return value optimization).
Yes, but assigning the value in the code that uses it will, unless again
you do this in an initializer.
The compiler should optimize it away completely.
Should as opposed to does. Want me to try a simple test?
Please.
Just for fun: optimize for size, and compare code sizes.
The C++ code is yours but I have removed all use of STL to make
it more of an even fight. I checked by object and executable size.
Note that this is shared library build: a C++ executable
will pull in a large C++ library, a C executable won't.
If you are interested in an STL based example let me know.
You can take it from here and make it more real if you like,
or look at the assembler output.
--
[...@tuck ~]$ cat test.c
#includesys/ioctl.h
#includesys/types.h
#includesys/stat.h
#includefcntl.h
#includeunistd.h
#includeerrno.h
int main(int argc, char **argv)
{
int fd = open(/dev/kvm, O_RDWR);
int r;
if (fd 0)
goto open_err;
r = ioctl(fd, 0, 0);
if (r 0)
goto ioctl_err;
return 0;
ioctl_err:
close(fd);
open_err:
return -1;
}
This code is not reusable. Everywhere you use an fd, you have to
repeat this code.
But that's not a lot of code. And you can abstract it away at a higher
level. For example kvm_init and kvm_cleanup would setup/cleanup
state in a consistent way.
My experience tells me C++ code has much more boilerplate code that you
are forced to repeat over and over. This is especially true for unix
system programming: by the time you are done wrapping all of unix you
have created more LOC than you are ever likely to save.
[...@tuck ~]$ gcc -c -Os test.c
[...@tuck ~]$ size test.o
textdata bss dec hex filename
97 0 0 97 61 test.o
[...@tuck ~]$ gcc -Os test.c
[...@tuck ~]$ size a.out
textdata bss dec hex filename
1192 260 81460 5b4 a.out
[...@tuck ~]$ wc -l test.c
22 test.c
--
[...@tuck ~]$ cat kvmxx.cpp
extern C {
#includesys/ioctl.h
#includesys/types.h
#includesys/stat.h
#includefcntl.h
#includeunistd.h
#includeerrno.h
}
namespace kvm {
class fd {
public:
explicit fd(const char *path, int flags);
~fd() { ::close(_fd); }
long ioctl(unsigned nr, long arg);
private:
int _fd;
};
};
namespace kvm {
static long check_error(long r)
{
if (r == -1) {
throw errno;
}
return r;
}
fd::fd(const char *device_node, int flags)
: _fd(::open(device_node, flags))
{
check_error(_fd);
}
long fd::ioctl(unsigned nr, long arg)
{
return check_error(::ioctl(_fd, nr, arg));
}
}
int main(int ac, char **av)
{
try {
kvm::fd fd(/dev/kvm, O_RDWR);
fd.ioctl(0, 0);
} catch (...) {
return -1;
}
return 0;
}
class kvm::fd is reusable, if you embed it in another object you
don't have to worry about errors any more (as long as the object's
methods are exception safe).
To get exception safe code, you have to constantly worry about errors.
And it's easier to spot an unhandled return code than exception-unsafe
code: gcc actually has __attribute__((warn_unused_result)) which
might help catch common errors. No such tool to catch
exception-unsafe code AFAIK.
[...@tuck ~]$ g++ -c -Os kvmxx.cpp
[...@tuck ~]$ size kvmxx.o
textdata bss dec hex filename
529 0 0 529 211 kvmxx.o
[...@tuck ~]$ g++ -Os kvmxx.cpp
[...@tuck ~]$ size a.out
textdata bss dec hex filename
2254 308 162578 a12 a.out
[...@tuck ~]$ wc kvmxx.cpp
56 kvmxx.cpp
--
One interesting thing is that the object size grew
faster than linked executable size.
This might mean that the compiler generated some
dead code that the linker then threw out.
It's also interesting that C++
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On Sun, Nov 28, 2010 at 03:15:52PM +0200, Avi Kivity wrote:
On 11/28/2010 01:49 PM, Michael S. Tsirkin wrote:
+++ b/api/kvmxx.cc
@@ -0,0 +1,168 @@
+#include kvmxx.h
+#includefcntl.h
+#includesys/ioctl.h
+#includesys/mman.h
I just realized this is wrong: I think you should wrap
the headers in extern C. Same for other headers.
I think system headers already do this
(otherwise it won't link -
int foo() is different than extern C { int foo(void); })
okay, but they aren't there for linux/kvm.h, are they?
--
error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On Sun, Nov 28, 2010 at 04:34:39PM +0200, Avi Kivity wrote:
On 11/28/2010 03:57 PM, Michael S. Tsirkin wrote:
On Sun, Nov 28, 2010 at 03:02:18PM +0200, Avi Kivity wrote:
On 11/28/2010 01:59 PM, Michael S. Tsirkin wrote:
FWIW, I still disagree with C++ and believe this code to be hardly
readable.
A major issue is existing tools.
Using C++ would prevent us from using sparce for static code checking.
C++ static checking is way better than anything sparse offers.
This seems a second system effect at work.
sparce lets you solve C problems that C++ inherited as is.
E.g. if you have a pointer you can always dereference it.
It's the other way round.
For example __user cannot be done in C. It has to be done as an add-on.
In C++ it's simply:
template class T
class user_ptrT
{
public:
explicit user_ptr(unsigned long addr);
void copy_from(T to); // throws EFAULT
void copy_to(const T from); // throws EFAULT
private:
unsigned long addr;
};
This does not allow simple uses such as arithmetic, void, builtin
types, sizeof, arrays, NULL comparizon, inheritance, cast,
memory management.
Examples to ponder: what is the appropriate value of T for void *?
What if you want a shared/auto ptr to manage this memory?
Some of these might be fixable, with a lot of code.
Boost might haver some solutions, I haven't looked.
Meanwhile sparse is already there.
No need for an additional toolchain.
It's a feature :) This way you are not forced to rewrite all code each
time you realize you need an extra check, and checks can be added
gradually without breaking build.
Things like __user are easily done in C++.
Some of what sparce does can be done if you create a separate type for
all address spaces. This can be done in C too, and the result won't
be like __user at all.
That's quite a lot of work.
Sparse: T __user *foo;
C++: user_ptrT foo;
Sparse has some advantages: it makes the contract obvious so you clearly
see it's a pointer and know -, [], + will work, * and will not.
C : struct T_user_ptr { unsigned long addr } foo; + lots of accessors.
Some kind of macro can be closer to user_ptr above.
We should be adding more annotations instead of throwing existing ones
out. ctags is also broken with C++ which will make it much harder
for me to browse the codebase.
C++ does want a good IDE.
For some definitions of good :).
Let's start a vi versus emacs flamewar?
Like we haven't enough...
C++ support in gdb has some limitations
if you use overloading, exceptions, templates. The example posted here
uses two of these, so it would be harder to debug.
I haven't seen issues with overloading or exceptions.
Build your test with -g, fire up gdb from command line,
try to put a breakpoint in the constructor of
the fd object, maybe you will see what I mean :)
(gdb) break 'kvm::fd::fd'
Breakpoint 3 at 0x8049650: file api/kvmxx.cc, line 25.
Breakpoint 4 at 0x8049628: file api/kvmxx.cc, line 31.
Breakpoint 5 at 0x8049080: file api/kvmxx.cc, line 21
But it's hard to figure out that you need the kvm namespace. Your code
only has one namespace, but with multiple namespaces, you don't even
know in which namespace to look up the fd. With templates you might not
even know the fd class.
An example of an issue with overloading is that gdb seems unable to resolve
them properly depending on the current scope. So you see a call to
foo() and want to put a breakpoint there, first problem is just to find
one which namespace it is in. Once you did the best
it can do it prompt you to select one of the overloaded options.
How do you know which one do you want? You don't, so you guess. Sometimes
gdb will guess, because of a complex set of name resolution rules, and
sometimes it will this wrongly.
Which is not what I want to spend mental cycles on when I am debugging a
problem.
Functions using exceptions can not be called from the gdb prompt
(gdb is not smart enough to catch them).
There are more issues.
That's not restricted to gdb. C has just three scopes: block (may
be nested), file static, and global. C++ has more. Stating
everything leads to verbose code and potential conflicts. Having
more scopes allows tighter code usually
but more head-scratching if
something goes wrong.
In my experience conflicts are very rare. But it's true that when
they happen they can be surprising.
There is some difference: when you write code, conflicts are rare
because you let the compiler guess the right call from the scope. When
you read code or debug, conflicts are everywhere. They are rarely
surprising, they are always time consuming to resolve.
Templates are
indeed harder to debug, simply because names can become very long.
That's not the only problem. A bigger one is when you type tab to
complete function name and get a list of options to select from for
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/28/2010 06:27 AM, Michael S. Tsirkin wrote:
On Wed, Nov 24, 2010 at 08:41:26AM -0600, Anthony Liguori wrote:
On 11/24/2010 06:59 AM, Alexander Graf wrote:
On 24.11.2010, at 11:52, Avi Kivity wrote:
Introduce exception-safe objects for calling system, vm, and vcpu ioctls.
Signed-off-by: Avi Kivitya...@redhat.com
FWIW, I still disagree with C++ and believe this code to be hardly readable.
There's a general prettiness that well written C++ code will have
over C when there's heavy object modelling. This can be subjective
but for me, it's fairly significant.
My guess is this comes from the fact that you are rewriting large pieces
of code from scratch so it suits your personal style perfectly :)
That's probably a good observations.
If history teaches us anything, as with most projects in qemu, what we will
end up with is a half done conversion of maybe 30% of the codebase.
The result might be anything: safer, more correct - but it won't be prettier.
This is where things need to be different. I'm not at all interested in
introducing C++ to QEMU because of exactly what you describe above.
I think the only viable approach is one where we have a segregated code
base that is correct with an incremental movement of code from the old
code base to the new way of doing things.
I've always thought that the device model should be a library and I
think that's the way to structure it. Have a libqemuhw and only move
devices into it as they are converted properly.
The fact that objects are easily created on the stack and on the
heap is also pretty significant.
Significant how?
To create an object on the stack, you must have the class definition in
a public header and a public constructor/destructor.
This is exactly the same in C.
It's really more of a design statement than a statement about C++ vs. C.
In qdev today, we mix object initialization with a user-exposed
factory. This means that you cannot do something simple like:
struct i440fx {
struct piix3 piix;
};
void i440fx_init(struct i440fx_init *obj) {
piix3_init(obj-piix);
}
But rather need to use ugly factory functions with all sorts of
DO_UPCAST. This is really unfriendly especially for writing test cases.
But this isn't C vs. C++, this is just about device model design. I
think C++ makes it quite a bit more obvious though how to design
correctly though.
In real hardware, the i8042 (keyboard controller) is actually
implemented in the PIIX3 which is a chip that is part of the i440fx.
The i440fx acts as both the memory controller and as the PCI Host
controller. So you get something that looks like:
class PIIX3 : public PCIDevice
{
private:
I8042 i8042;
RTC rtc;
// ...
};
class I440FX : public PCIHostController
{
I440FX(void) {
this-slots[1].plug(this-piix3); // piix3 is always in slot 1
}
private:
PlugPCIDevice * slots[32]; // slot 0 is the PMC
PIIX3 piix3;
};
We can have the same thing today. In fact, getting rid of the UP_CAST
and opaque pointers should be a priority.
I find that you end up writing a lot more boiler plate code trying to do
this properly in C. I think gobject is probably the best example of
this. If you've ever written a GTK widget from scratch in C and then
written one in gtkmm, the difference is night and day.
So whereas we have this very complicate machine create function that
attempts to create and composite all of these devices after the
fact, when written in C++, partially due to good design, but
partially due to the fact that the languages forces you to think a
certain way, you get a tremendous simplification.
A proper C++ device model turns a vast majority of our device
creation complexity into a single new I440FX. Then it's just a
matter of instantiating and plugging the appropriate set of PCI
devices.
Of course, this can be wrapped in a factory to make it drivable via
an API or config file.
Another area that C++ shines is safety. C++ enables you to inject
safe versions of things that you really can't do in C. For
instance, the PIT has three channels but the mask to select a
channel is two bits. There was a kernel exploit that found a way to
trick selection of a forth channel because of a missing check.
In C++, you can convert:
PITChannel channnels[3];
Into:
ArrayPITChannel, 3 channels;
It behaves in every other way just like a normal array. The memory
is stack allocated, the type has a fixed size. The only difference
is that you can overload the [] operators and implement bounds
checking for array accesses. This means that as long as you use
Array, array overflows disappear from the code base. That's a big
deal.
Except that you get used to the fact that [] is safe,
and then forget to check the value in a dynamically
sized array access. Boom.
I don't think the fact that you get deterministic vs. non-deterministic
behavior
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/28/2010 08:40 AM, Michael S. Tsirkin wrote: This code is not reusable. Everywhere you use an fd, you have to repeat this code. But that's not a lot of code. And you can abstract it away at a higher level. For example kvm_init and kvm_cleanup would setup/cleanup state in a consistent way. My experience tells me C++ code has much more boilerplate code that you are forced to repeat over and over. This is especially true for unix system programming: by the time you are done wrapping all of unix you have created more LOC than you are ever likely to save. I've said it earlier but I think it's worth repeating--trying to wrap libc in pretty C++ interfaces is a waste of time. In an ideal world, the device model doesn't interact at all with libc. Most of the code simply manipulates internal state and some code will provide a bridge to well defined internal interfaces. The nice thing about C++ though is that you can still write C code and compile it with g++ (or even gcc if you care). So you can use objects where they make sense and not use them where they don't. class kvm::fd is reusable, if you embed it in another object you don't have to worry about errors any more (as long as the object's methods are exception safe). To get exception safe code, you have to constantly worry about errors. And it's easier to spot an unhandled return code than exception-unsafe code: gcc actually has __attribute__((warn_unused_result)) which might help catch common errors. No such tool to catch exception-unsafe code AFAIK. I tend to agree that writing exception safe code and using exceptions for expected errors is a bad idea. I think exceptions should be used primarily for unexpected errors. Regards, Anthony Liguori -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On Sun, Nov 28, 2010 at 04:04:34PM -0600, Anthony Liguori wrote:
To create an object on the stack, you must have the class definition in
a public header and a public constructor/destructor.
This is exactly the same in C.
It's really more of a design statement than a statement about C++ vs. C.
In qdev today, we mix object initialization with a user-exposed
factory. This means that you cannot do something simple like:
struct i440fx {
struct piix3 piix;
};
void i440fx_init(struct i440fx_init *obj) {
piix3_init(obj-piix);
}
But rather need to use ugly factory functions with all sorts of
DO_UPCAST. This is really unfriendly especially for writing test
cases.
Yes, I agree. Just moving memory allocation out of there
will fix most of the ugliness.
--
MST
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/28/2010 04:28 PM, Michael S. Tsirkin wrote:
But rather need to use ugly factory functions with all sorts of
DO_UPCAST. This is really unfriendly especially for writing test
cases.
Yes, I agree. Just moving memory allocation out of there
will fix most of the ugliness.
So here's a short list of things I've been working on that I don't
believe have nice implementations in C.
1) Factories with string based parameters with natural constructor
arguments.
I want to have:
I8254(DriftMode mode)
or:
void i8254_init(I8524 *self, DriftMode mode)
And either be able to directly instantiate the object or create it with
the following command:
dev = create(i8524, mode, catchup, NULL);
In C++, I can do:
static Device *i8524_create(DriftMode mode)
{
return new I8524(mode);
}
//...
add_factory(i8524, i8524_create, mode);
This works because we can use templates to figure out the argument type
to create and automatically set things up for the mode parameter.
Because of overloading, we can have variable arguments so this works for
1, 2, or 10 arguments. In C, at best you would have to do something like:
static Device *i8524_create(ParameterList *list)
{
DriftMode mode;
I8524 *i8524;
if (param_list_has(list, mode)) {
// should we check the type?
mode = (DriftMode)param_get_as_int(list, mode); // or do we
teach param about DriftMode?
} else {
// should we error?
}
i8524 = malloc(sizeof(*i8524));
if (i8524) {
return i8524-dev; // I have to know how to get to this
}
return NULL;
}
struct factory_info i8524_info = {
.args = (FactoryArgument[]){
{ mode, DriftMode },
{ }
}
};
add_factory(i8524, i8524_create, i8524_info);
And there's so many bad decisions I can make in the process. Do I have
a mechanism to register new types external to the system and make all
types convertable?
My C++ example uses a template specialization to convert from types to
strings. It's simple and extensible. There's no list that needs to be
kept at run time because the compiler does all of the book keeping.
I can let the compiler figure out how to go from I8524 to Device *
because it knows how to.
The same is true for generic get/set properties. In C++, I can do
something like:
class I8524 : public PropertyAccessible {
public:
enum LinkStatus {
LINK_UP,
LINK_DOWN,
};
LinkStatus get_link_status(void);
void set_link_status(void);
};
template
I8254::LinkStatus type_from_str(std::string value)
{
if (value == Down) {
return LINK_DOWN;
} else if (value == Up) {
return LINK_UP;
}
throw new InvalidTypeConversionException(LinkStatus, value);
}
template
std::string type_to_str(I8254::LinkStatus status)
{
if (status == LINK_DOWN) {
return Down;
} else {
return Up;
}
}
I8524::I8524(void) {
this-add_property(link_status, I8524::get_link_status,
I8524::set_link_status);
}
And now I can do:
Device *dev = new I8524();
cout dev-get(link_status) endl;
And it will print: Down
Simple and elegant. The main class has strong error checking, adding
the property is absolutely painless, supporting new types is absolutely
painless. The actual class implementation never sees a string--ever.
There is only one place string-type conversion happens.
I'm not going to show the C version because I know what it involves and
I suspect you do too. It totally sucks and I don't want to spend my
Sunday evening writing it. And that's really my motivation for using
C++. I get tired of doing the work that the compiler should be doing
for me. I want to spend my time doing interesting things like writing
device model test cases instead of writing code to work around the fact
that the C type system is about as powerful as a Vespa.
Regars,
Anthony Liguori
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On Wed, Nov 24, 2010 at 12:52:11PM +0200, Avi Kivity wrote:
Introduce exception-safe objects for calling system, vm, and vcpu ioctls.
Signed-off-by: Avi Kivity a...@redhat.com
ioctlp calls below ignore possible errors.
Somre more comments below.
---
api/kvmxx.cc | 168
++
api/kvmxx.h | 80 +++
2 files changed, 248 insertions(+), 0 deletions(-)
create mode 100644 api/kvmxx.cc
create mode 100644 api/kvmxx.h
diff --git a/api/kvmxx.cc b/api/kvmxx.cc
new file mode 100644
index 000..2f8fc27
--- /dev/null
+++ b/api/kvmxx.cc
@@ -0,0 +1,168 @@
+#include kvmxx.h
+#include fcntl.h
+#include sys/ioctl.h
+#include sys/mman.h
+#include memory
+#include algorithm
+
+namespace kvm {
+
+static long check_error(long r)
+{
+if (r == -1) {
+ throw errno;
+}
+return r;
+}
+
+fd::fd(int fd)
+: _fd(fd)
+{
+}
+
+fd::fd(const fd other)
+: _fd(::dup(other._fd))
+{
+check_error(_fd);
+}
+
+fd::fd(std::string device_node, int flags)
+: _fd(::open(device_node.c_str(), flags))
+{
+check_error(_fd);
+}
+
+long fd::ioctl(unsigned nr, long arg)
+{
+return check_error(::ioctl(_fd, nr, arg));
+}
+
+vcpu::vcpu(vm vm, int id)
+: _vm(vm), _fd(vm._fd.ioctl(KVM_CREATE_VCPU, id)), _shared(NULL)
+{
+unsigned mmap_size = _vm._system._fd.ioctl(KVM_GET_VCPU_MMAP_SIZE, 0);
+kvm_run *shared = static_castkvm_run*(::mmap(NULL, mmap_size,
+PROT_READ | PROT_WRITE,
+MAP_SHARED,
+_fd.get(), 0));
+if (shared == MAP_FAILED) {
+ throw errno;
+}
+_shared = shared;
+}
+
+vcpu::~vcpu()
+{
+unsigned mmap_size = _vm._system._fd.ioctl(KVM_GET_VCPU_MMAP_SIZE, 0);
This might throw an exception on destructor path, if this happens
because an exception was thrown terminate is called.
+munmap(_shared, mmap_size);
+}
+
+void vcpu::run()
+{
+_fd.ioctl(KVM_RUN, 0);
+}
+
+kvm_regs vcpu::regs()
+{
+kvm_regs regs;
+_fd.ioctlp(KVM_GET_REGS, regs);
+return regs;
+}
+
+void vcpu::set_regs(const kvm_regs regs)
+{
+_fd.ioctlp(KVM_SET_REGS, const_castkvm_regs*(regs));
+}
+
+kvm_sregs vcpu::sregs()
+{
+kvm_sregs sregs;
+_fd.ioctlp(KVM_GET_SREGS, sregs);
+return sregs;
+}
+
+void vcpu::set_sregs(const kvm_sregs sregs)
+{
+_fd.ioctlp(KVM_SET_SREGS, const_castkvm_sregs*(sregs));
+}
+
+kvm_msrs* vcpu::alloc_msr_list(size_t nmsrs)
+{
+size_t size = sizeof(kvm_msrs) + sizeof(kvm_msr_entry) * nmsrs;
+kvm_msrs* ret = static_castkvm_msrs*(malloc(size));
+if (!ret) {
+ throw ENOMEM;
+}
+return ret;
+}
+
+std::vectorkvm_msr_entry vcpu::msrs(std::vectoruint32_t indices)
+{
+std::auto_ptrkvm_msrs msrs(alloc_msr_list(indices.size()));
This looks wrong. auto_ptr frees memory with delete,
alloc_msr_list allocates it with malloc.
+msrs-nmsrs = indices.size();
+for (unsigned i = 0; i msrs-nmsrs; ++i) {
+ msrs-entries[i].index = indices[i];
+}
+_fd.ioctlp(KVM_GET_MSRS, msrs.get());
+return std::vectorkvm_msr_entry(msrs-entries,
+ msrs-entries + msrs-nmsrs);
+}
+
+void vcpu::set_msrs(const std::vectorkvm_msr_entry msrs)
+{
+std::auto_ptrkvm_msrs _msrs(alloc_msr_list(msrs.size()));
As above.
+_msrs-nmsrs = msrs.size();
+std::copy(msrs.begin(), msrs.end(), _msrs-entries);
+_fd.ioctlp(KVM_SET_MSRS, _msrs.get());
+}
+
+void vcpu::set_debug(uint64_t dr[8], bool enabled, bool singlestep)
+{
+kvm_guest_debug gd;
+
+gd.control = 0;
+if (enabled) {
+ gd.control |= KVM_GUESTDBG_ENABLE;
+}
+if (singlestep) {
+ gd.control |= KVM_GUESTDBG_SINGLESTEP;
+}
+for (int i = 0; i 8; ++i) {
+ gd.arch.debugreg[i] = dr[i];
+}
+_fd.ioctlp(KVM_SET_GUEST_DEBUG, gd);
+}
+
+vm::vm(system system)
+: _system(system), _fd(system._fd.ioctl(KVM_CREATE_VM, 0))
+{
+}
+
+void vm::set_memory_region(int slot, void *addr, uint64_t gpa, size_t len)
+{
+struct kvm_userspace_memory_region umr;
+
+umr.slot = slot;
+umr.flags = 0;
+umr.guest_phys_addr = gpa;
+umr.memory_size = len;
+umr.userspace_addr = reinterpret_castuint64_t(addr);
+_fd.ioctlp(KVM_SET_USER_MEMORY_REGION, umr);
+}
+
+void vm::set_tss_addr(uint32_t addr)
+{
+_fd.ioctl(KVM_SET_TSS_ADDR, addr);
+}
+
+system::system(std::string device_node)
+: _fd(device_node, O_RDWR)
+{
+}
+
+bool system::check_extension(int extension)
+{
+return _fd.ioctl(KVM_CHECK_EXTENSION, extension);
+}
+
+};
diff --git a/api/kvmxx.h b/api/kvmxx.h
new file mode 100644
index 000..716e400
--- /dev/null
+++ b/api/kvmxx.h
@@ -0,0 +1,80 @@
+#ifndef KVMXX_H
+#define KVMXX_H
+
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On Wed, Nov 24, 2010 at 12:53:00PM -0600, Anthony Liguori wrote: On 11/24/2010 12:34 PM, Gleb Natapov wrote: Right, and real HW does composition in the PIIX3 device. So let's not regret making everything an ISA device later. You design by packaging not functionality? No, if you want the ability to remove devices from the PIIX3, fine, but don't call them ISA devices just for the sake of it. PIIX3 may be totally implemented in FPGA code. I do not care. This is implementation detail. What important is that it provide the same functionality as ISA devices in the first IBM PC. That is why they are ISA devices and should be modeled as such. Otherwise you will have to redesign everything when you'll want to move to another chipset emulation that uses new and improved SuperIO-2XT technology inside. The ISA slots (technically XT) were basically daughter board connectors. They routed pins from a bunch of devices on the motherboard to external daughterboards. This included the *unused* IRQ pins, the left over DMA channels, etc. Everything else was hard wired. The XT connector only exposes pins 3, 4, 5, 6, and 7. The ISA connector then added IRQs 10, 11, 12, 14, and 15. So if a device uses IRQ 0, 1, 2, 8, 9, or 13, it's by definition not an ISA device. It was hard wired on the motherboard. I don't understand why do you claim that device that is hard wired on the motherboard is not ISA device. We have many PCI devices internally in modern chipsets. They are as PCI devices as add on PCI cards. -- Gleb. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/24/2010 04:29 PM, Avi Kivity wrote:
+kvm_msrs* vcpu::alloc_msr_list(size_t nmsrs)
+{
+size_t size = sizeof(kvm_msrs) + sizeof(kvm_msr_entry) * nmsrs;
+kvm_msrs* ret = static_castkvm_msrs*(malloc(size));
+if (!ret) {
+throw ENOMEM;
+}
+return ret;
+}
malloc?
Mixing C and C++ allocations is nasty stuff. Would be nicer to new
an object and return it such that delete can be used consistently.
5 years of C.
It has to be a malloc() in fact. We're allocating a variable size
object that is not an array. You're right that we can't give it to an
auto_ptr though, since that uses delete.
--
error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 24.11.2010, at 11:52, Avi Kivity wrote: Introduce exception-safe objects for calling system, vm, and vcpu ioctls. Signed-off-by: Avi Kivity a...@redhat.com FWIW, I still disagree with C++ and believe this code to be hardly readable. Alex -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On Wed, Nov 24, 2010 at 01:59:14PM +0100, Alexander Graf wrote: On 24.11.2010, at 11:52, Avi Kivity wrote: Introduce exception-safe objects for calling system, vm, and vcpu ioctls. Signed-off-by: Avi Kivity a...@redhat.com FWIW, I still disagree with C++ and believe this code to be hardly readable. C++ is so powerful and so easy to write correct code in that real C++ programmer never reads the code twice. -- Gleb. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/24/2010 04:52 AM, Avi Kivity wrote:
Introduce exception-safe objects for calling system, vm, and vcpu ioctls.
Signed-off-by: Avi Kivitya...@redhat.com
---
api/kvmxx.cc | 168 ++
api/kvmxx.h | 80 +++
2 files changed, 248 insertions(+), 0 deletions(-)
create mode 100644 api/kvmxx.cc
create mode 100644 api/kvmxx.h
diff --git a/api/kvmxx.cc b/api/kvmxx.cc
new file mode 100644
index 000..2f8fc27
--- /dev/null
+++ b/api/kvmxx.cc
@@ -0,0 +1,168 @@
+#include kvmxx.h
+#includefcntl.h
+#includesys/ioctl.h
+#includesys/mman.h
+#includememory
+#includealgorithm
+
+namespace kvm {
+
+static long check_error(long r)
+{
+if (r == -1) {
+ throw errno;
+}
+return r;
+}
It's generally nicer for exceptions to be objects and to inherit from
std::exception. Otherwise, you can run into nasty issues when catching
the wrong type.
+fd::fd(int fd)
+: _fd(fd)
+{
+}
There's no need to prefix with '_'. Every compiler has been able to do
the right thing with : fd(fd) for a long time.
+
+fd::fd(const fd other)
+: _fd(::dup(other._fd))
+{
+check_error(_fd);
+}
+
+fd::fd(std::string device_node, int flags)
+: _fd(::open(device_node.c_str(), flags))
+{
+check_error(_fd);
+}
+
+long fd::ioctl(unsigned nr, long arg)
+{
+return check_error(::ioctl(_fd, nr, arg));
+}
+
+vcpu::vcpu(vm vm, int id)
+: _vm(vm), _fd(vm._fd.ioctl(KVM_CREATE_VCPU, id)), _shared(NULL)
+{
+unsigned mmap_size = _vm._system._fd.ioctl(KVM_GET_VCPU_MMAP_SIZE, 0);
+kvm_run *shared = static_castkvm_run*(::mmap(NULL, mmap_size,
+ PROT_READ | PROT_WRITE,
+ MAP_SHARED,
+ _fd.get(), 0));
+if (shared == MAP_FAILED) {
+ throw errno;
+}
+_shared = shared;
+}
+
+vcpu::~vcpu()
+{
+unsigned mmap_size = _vm._system._fd.ioctl(KVM_GET_VCPU_MMAP_SIZE, 0);
+munmap(_shared, mmap_size);
+}
+
+void vcpu::run()
+{
+_fd.ioctl(KVM_RUN, 0);
+}
+
+kvm_regs vcpu::regs()
+{
+kvm_regs regs;
+_fd.ioctlp(KVM_GET_REGS,regs);
+return regs;
+}
+
+void vcpu::set_regs(const kvm_regs regs)
+{
+_fd.ioctlp(KVM_SET_REGS, const_castkvm_regs*(regs));
+}
+
+kvm_sregs vcpu::sregs()
+{
+kvm_sregs sregs;
+_fd.ioctlp(KVM_GET_SREGS,sregs);
+return sregs;
+}
I think you're missing an opportunity by just returning the structures
in their raw form as opposed to wrapping them in an object.
+
+void vcpu::set_sregs(const kvm_sregs sregs)
+{
+_fd.ioctlp(KVM_SET_SREGS, const_castkvm_sregs*(sregs));
+}
+
+kvm_msrs* vcpu::alloc_msr_list(size_t nmsrs)
+{
+size_t size = sizeof(kvm_msrs) + sizeof(kvm_msr_entry) * nmsrs;
+kvm_msrs* ret = static_castkvm_msrs*(malloc(size));
+if (!ret) {
+ throw ENOMEM;
+}
+return ret;
+}
malloc?
Mixing C and C++ allocations is nasty stuff. Would be nicer to new an
object and return it such that delete can be used consistently.
+
+std::vectorkvm_msr_entry vcpu::msrs(std::vectoruint32_t indices)
+{
+std::auto_ptrkvm_msrs msrs(alloc_msr_list(indices.size()));
+msrs-nmsrs = indices.size();
+for (unsigned i = 0; i msrs-nmsrs; ++i) {
+ msrs-entries[i].index = indices[i];
+}
+_fd.ioctlp(KVM_GET_MSRS, msrs.get());
+return std::vectorkvm_msr_entry(msrs-entries,
+ msrs-entries + msrs-nmsrs);
+}
auto_ptr has pretty awful semantics. tr1::shared_ptr is now available.
+
+void vcpu::set_msrs(const std::vectorkvm_msr_entry msrs)
+{
+std::auto_ptrkvm_msrs _msrs(alloc_msr_list(msrs.size()));
+_msrs-nmsrs = msrs.size();
+std::copy(msrs.begin(), msrs.end(), _msrs-entries);
+_fd.ioctlp(KVM_SET_MSRS, _msrs.get());
+}
+
+void vcpu::set_debug(uint64_t dr[8], bool enabled, bool singlestep)
+{
+kvm_guest_debug gd;
+
+gd.control = 0;
+if (enabled) {
+ gd.control |= KVM_GUESTDBG_ENABLE;
+}
+if (singlestep) {
+ gd.control |= KVM_GUESTDBG_SINGLESTEP;
+}
+for (int i = 0; i 8; ++i) {
+ gd.arch.debugreg[i] = dr[i];
+}
+_fd.ioctlp(KVM_SET_GUEST_DEBUG,gd);
+}
+
+vm::vm(system system)
+: _system(system), _fd(system._fd.ioctl(KVM_CREATE_VM, 0))
+{
+}
+
+void vm::set_memory_region(int slot, void *addr, uint64_t gpa, size_t len)
+{
+struct kvm_userspace_memory_region umr;
+
+umr.slot = slot;
+umr.flags = 0;
+umr.guest_phys_addr = gpa;
+umr.memory_size = len;
+umr.userspace_addr = reinterpret_castuint64_t(addr);
+_fd.ioctlp(KVM_SET_USER_MEMORY_REGION,umr);
+}
+
+void vm::set_tss_addr(uint32_t addr)
+{
+_fd.ioctl(KVM_SET_TSS_ADDR, addr);
+}
+
+system::system(std::string device_node)
+: _fd(device_node, O_RDWR)
+{
+}
+
+bool system::check_extension(int extension)
+{
+return _fd.ioctl(KVM_CHECK_EXTENSION,
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/24/2010 06:59 AM, Alexander Graf wrote: On 24.11.2010, at 11:52, Avi Kivity wrote: Introduce exception-safe objects for calling system, vm, and vcpu ioctls. Signed-off-by: Avi Kivitya...@redhat.com FWIW, I still disagree with C++ and believe this code to be hardly readable. Take a look at http://repo.or.cz/w/qemupp.git Start with test/test-mc146818a.cpp This test infrastructure is arguably a bad place to start because it doesn't benefit really at all from C++. The object hierarchy is extremely simple and most of the time you're interfacing with legacy C interfaces so a lot of effort is spent bridging those interfaces. OTOH, the device models interact very little with the external world and the object model is complex is subtle ways. I think C++ is a much better fit there. In the above repo, I've converted the PIT, RTC, keyboard controller, and have started working on the i440fx. The i440fx is a lot of work but it's also one of the more interesting devices to try and model correctly. For all devices, there are very intensive unit tests that use fuzzing. I hope to have something postable for serious consideration by the end of the year. BTW, given the ability to test programmatically, I ended up rewriting a good bit of the PIT and RTC. The way that the unit conversion is handled right now in QEMU is a train wreck. The new version is much more accurate and as a consequence, the interrupt catch up logic is much more precise. There are also three different catch up algorithms that are selectable at run time. This is all tested with a time simulation that can simulate random amounts of drift over random periods of time. Regards, Anthony Liguori Alex -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/24/2010 08:18 AM, Anthony Liguori wrote: On 11/24/2010 06:59 AM, Alexander Graf wrote: On 24.11.2010, at 11:52, Avi Kivity wrote: Introduce exception-safe objects for calling system, vm, and vcpu ioctls. Signed-off-by: Avi Kivitya...@redhat.com FWIW, I still disagree with C++ and believe this code to be hardly readable. Take a look at http://repo.or.cz/w/qemupp.git repo.or.cz seems to be down so: http://git.qemu.org/qemupp.git/ Regards, Anthony Liguori Start with test/test-mc146818a.cpp This test infrastructure is arguably a bad place to start because it doesn't benefit really at all from C++. The object hierarchy is extremely simple and most of the time you're interfacing with legacy C interfaces so a lot of effort is spent bridging those interfaces. OTOH, the device models interact very little with the external world and the object model is complex is subtle ways. I think C++ is a much better fit there. In the above repo, I've converted the PIT, RTC, keyboard controller, and have started working on the i440fx. The i440fx is a lot of work but it's also one of the more interesting devices to try and model correctly. For all devices, there are very intensive unit tests that use fuzzing. I hope to have something postable for serious consideration by the end of the year. BTW, given the ability to test programmatically, I ended up rewriting a good bit of the PIT and RTC. The way that the unit conversion is handled right now in QEMU is a train wreck. The new version is much more accurate and as a consequence, the interrupt catch up logic is much more precise. There are also three different catch up algorithms that are selectable at run time. This is all tested with a time simulation that can simulate random amounts of drift over random periods of time. Regards, Anthony Liguori Alex -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/24/2010 04:10 PM, Anthony Liguori wrote:
On 11/24/2010 04:52 AM, Avi Kivity wrote:
Introduce exception-safe objects for calling system, vm, and vcpu
ioctls.
+
+namespace kvm {
+
+static long check_error(long r)
+{
+if (r == -1) {
+throw errno;
+}
+return r;
+}
It's generally nicer for exceptions to be objects and to inherit from
std::exception. Otherwise, you can run into nasty issues when
catching the wrong type.
Yeah, I'm lazy.
+fd::fd(int fd)
+: _fd(fd)
+{
+}
There's no need to prefix with '_'. Every compiler has been able to
do the right thing with : fd(fd) for a long time.
Ok.
+
+kvm_sregs vcpu::sregs()
+{
+kvm_sregs sregs;
+_fd.ioctlp(KVM_GET_SREGS,sregs);
+return sregs;
+}
I think you're missing an opportunity by just returning the structures
in their raw form as opposed to wrapping them in an object.
What would the object do besides adding tons of accessors?
+
+void vcpu::set_sregs(const kvm_sregs sregs)
+{
+_fd.ioctlp(KVM_SET_SREGS, const_castkvm_sregs*(sregs));
+}
+
+kvm_msrs* vcpu::alloc_msr_list(size_t nmsrs)
+{
+size_t size = sizeof(kvm_msrs) + sizeof(kvm_msr_entry) * nmsrs;
+kvm_msrs* ret = static_castkvm_msrs*(malloc(size));
+if (!ret) {
+throw ENOMEM;
+}
+return ret;
+}
malloc?
Mixing C and C++ allocations is nasty stuff. Would be nicer to new an
object and return it such that delete can be used consistently.
5 years of C.
+
+std::vectorkvm_msr_entry vcpu::msrs(std::vectoruint32_t indices)
+{
+std::auto_ptrkvm_msrs msrs(alloc_msr_list(indices.size()));
+msrs-nmsrs = indices.size();
+for (unsigned i = 0; i msrs-nmsrs; ++i) {
+msrs-entries[i].index = indices[i];
+}
+_fd.ioctlp(KVM_GET_MSRS, msrs.get());
+return std::vectorkvm_msr_entry(msrs-entries,
+ msrs-entries + msrs-nmsrs);
+}
auto_ptr has pretty awful semantics. tr1::shared_ptr is now available.
For this it's perfectly fine.
+
+class fd {
+public:
+explicit fd(int n);
+explicit fd(std::string path, int flags);
+fd(const fd other);
+~fd() { ::close(_fd); }
+int get() { return _fd; }
+long ioctl(unsigned nr, long arg);
+long ioctlp(unsigned nr, void *arg) {
+return ioctl(nr, reinterpret_castlong(arg));
+}
I think mixing inline definitions with declarations is bad form.
It is, but on the other hand I hate the verbosity of all those little
accessors.
--
error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/24/2010 02:59 PM, Alexander Graf wrote: On 24.11.2010, at 11:52, Avi Kivity wrote: Introduce exception-safe objects for calling system, vm, and vcpu ioctls. Signed-off-by: Avi Kivitya...@redhat.com FWIW, I still disagree with C++ and believe this code to be hardly readable. The equivalent C code would have either no error checking or it would be a mess of error handling intermixed with bugs and a few lines which do actual work. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/24/2010 04:18 PM, Anthony Liguori wrote: On 11/24/2010 06:59 AM, Alexander Graf wrote: On 24.11.2010, at 11:52, Avi Kivity wrote: Introduce exception-safe objects for calling system, vm, and vcpu ioctls. Signed-off-by: Avi Kivitya...@redhat.com FWIW, I still disagree with C++ and believe this code to be hardly readable. Take a look at http://repo.or.cz/w/qemupp.git repo.or.cz is down. Now if _that_ doesn't say anything about C++, then I don't know what does. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/24/2010 06:59 AM, Alexander Graf wrote:
On 24.11.2010, at 11:52, Avi Kivity wrote:
Introduce exception-safe objects for calling system, vm, and vcpu ioctls.
Signed-off-by: Avi Kivitya...@redhat.com
FWIW, I still disagree with C++ and believe this code to be hardly readable.
There's a general prettiness that well written C++ code will have over C
when there's heavy object modelling. This can be subjective but for me,
it's fairly significant.
The fact that objects are easily created on the stack and on the heap is
also pretty significant. When considering device models, we struggle
today with device composition.
In real hardware, the i8042 (keyboard controller) is actually
implemented in the PIIX3 which is a chip that is part of the i440fx.
The i440fx acts as both the memory controller and as the PCI Host
controller. So you get something that looks like:
class PIIX3 : public PCIDevice
{
private:
I8042 i8042;
RTC rtc;
// ...
};
class I440FX : public PCIHostController
{
I440FX(void) {
this-slots[1].plug(this-piix3); // piix3 is always in slot 1
}
private:
PlugPCIDevice * slots[32]; // slot 0 is the PMC
PIIX3 piix3;
};
So whereas we have this very complicate machine create function that
attempts to create and composite all of these devices after the fact,
when written in C++, partially due to good design, but partially due to
the fact that the languages forces you to think a certain way, you get a
tremendous simplification.
A proper C++ device model turns a vast majority of our device creation
complexity into a single new I440FX. Then it's just a matter of
instantiating and plugging the appropriate set of PCI devices.
Of course, this can be wrapped in a factory to make it drivable via an
API or config file.
Another area that C++ shines is safety. C++ enables you to inject safe
versions of things that you really can't do in C. For instance, the PIT
has three channels but the mask to select a channel is two bits. There
was a kernel exploit that found a way to trick selection of a forth
channel because of a missing check.
In C++, you can convert:
PITChannel channnels[3];
Into:
ArrayPITChannel, 3 channels;
It behaves in every other way just like a normal array. The memory is
stack allocated, the type has a fixed size. The only difference is
that you can overload the [] operators and implement bounds checking for
array accesses. This means that as long as you use Array, array
overflows disappear from the code base. That's a big deal.
Another area C++ shines is generating metacode. Consider the ugliness
around VMState. The crux of the problem is that it's not possible to
write type-neutral code in C. This all gets simplified with C++.
Instead of having a bunch of macros like:
VMSTATE_INT8(val0, ...)
VMSTATE_INT16(val1, ...)
You can just have:
vmstate(val0)
vmstate(val1)
And use type overloading to implement different behaviors. Combined
with template specialization and an Array wrapper, the same thing works
for arrays too.
Regards,
Anthony Liguori
Regards,
Anthony Liguori
Alex
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/24/2010 08:29 AM, Avi Kivity wrote:
On 11/24/2010 04:10 PM, Anthony Liguori wrote:
On 11/24/2010 04:52 AM, Avi Kivity wrote:
Introduce exception-safe objects for calling system, vm, and vcpu
ioctls.
+
+namespace kvm {
+
+static long check_error(long r)
+{
+if (r == -1) {
+throw errno;
+}
+return r;
+}
It's generally nicer for exceptions to be objects and to inherit from
std::exception. Otherwise, you can run into nasty issues when
catching the wrong type.
Yeah, I'm lazy.
+fd::fd(int fd)
+: _fd(fd)
+{
+}
There's no need to prefix with '_'. Every compiler has been able to
do the right thing with : fd(fd) for a long time.
Ok.
+
+kvm_sregs vcpu::sregs()
+{
+kvm_sregs sregs;
+_fd.ioctlp(KVM_GET_SREGS,sregs);
+return sregs;
+}
I think you're missing an opportunity by just returning the
structures in their raw form as opposed to wrapping them in an object.
What would the object do besides adding tons of accessors?
I would think that you'd have a single object that represented the full
CPU state and then you'd have methods that allowed individual groups to
be refreshed.
Something like:
struct x86_vcpu : public vcpu
{
uint64_t eax;
uint64_t ebx;
uint64_t ecx;
//...
void get_gps(void);
void put_gps(void);
void get_sregs(void);
void put_sregs(void);
std::string repr(void);
};
I'm not of the opinion that all members need getters and setters. I
think it's perfectly fine to have public variables if the reads and
writes don't have side effects.
Regards,
Anthony Liguori
+
+void vcpu::set_sregs(const kvm_sregs sregs)
+{
+_fd.ioctlp(KVM_SET_SREGS, const_castkvm_sregs*(sregs));
+}
+
+kvm_msrs* vcpu::alloc_msr_list(size_t nmsrs)
+{
+size_t size = sizeof(kvm_msrs) + sizeof(kvm_msr_entry) * nmsrs;
+kvm_msrs* ret = static_castkvm_msrs*(malloc(size));
+if (!ret) {
+throw ENOMEM;
+}
+return ret;
+}
malloc?
Mixing C and C++ allocations is nasty stuff. Would be nicer to new
an object and return it such that delete can be used consistently.
5 years of C.
+
+std::vectorkvm_msr_entry vcpu::msrs(std::vectoruint32_t indices)
+{
+std::auto_ptrkvm_msrs msrs(alloc_msr_list(indices.size()));
+msrs-nmsrs = indices.size();
+for (unsigned i = 0; i msrs-nmsrs; ++i) {
+msrs-entries[i].index = indices[i];
+}
+_fd.ioctlp(KVM_GET_MSRS, msrs.get());
+return std::vectorkvm_msr_entry(msrs-entries,
+ msrs-entries + msrs-nmsrs);
+}
auto_ptr has pretty awful semantics. tr1::shared_ptr is now available.
For this it's perfectly fine.
+
+class fd {
+public:
+explicit fd(int n);
+explicit fd(std::string path, int flags);
+fd(const fd other);
+~fd() { ::close(_fd); }
+int get() { return _fd; }
+long ioctl(unsigned nr, long arg);
+long ioctlp(unsigned nr, void *arg) {
+return ioctl(nr, reinterpret_castlong(arg));
+}
I think mixing inline definitions with declarations is bad form.
It is, but on the other hand I hate the verbosity of all those little
accessors.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/24/2010 04:45 PM, Anthony Liguori wrote:
What would the object do besides adding tons of accessors?
I would think that you'd have a single object that represented the
full CPU state and then you'd have methods that allowed individual
groups to be refreshed.
Something like:
struct x86_vcpu : public vcpu
{
uint64_t eax;
uint64_t ebx;
uint64_t ecx;
//...
void get_gps(void);
void put_gps(void);
void get_sregs(void);
void put_sregs(void);
std::string repr(void);
};
I'm not of the opinion that all members need getters and setters. I
think it's perfectly fine to have public variables if the reads and
writes don't have side effects.
I see. Well that belongs elsewhere, this is intended as a thin wrapper
that doesn't allows you to exercise the API directly. I don't want
things to happen automatically here, this is for a test framework.
--
error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/24/2010 08:53 AM, Avi Kivity wrote:
struct x86_vcpu : public vcpu
{
uint64_t eax;
uint64_t ebx;
uint64_t ecx;
//...
void get_gps(void);
void put_gps(void);
void get_sregs(void);
void put_sregs(void);
std::string repr(void);
};
I'm not of the opinion that all members need getters and setters. I
think it's perfectly fine to have public variables if the reads and
writes don't have side effects.
I see. Well that belongs elsewhere, this is intended as a thin
wrapper that doesn't allows you to exercise the API directly. I don't
want things to happen automatically here, this is for a test framework.
That's fine.
Regards,
Anthony Liguori
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On Wed, Nov 24, 2010 at 08:41:26AM -0600, Anthony Liguori wrote:
On 11/24/2010 06:59 AM, Alexander Graf wrote:
On 24.11.2010, at 11:52, Avi Kivity wrote:
Introduce exception-safe objects for calling system, vm, and vcpu ioctls.
Signed-off-by: Avi Kivitya...@redhat.com
FWIW, I still disagree with C++ and believe this code to be hardly readable.
There's a general prettiness that well written C++ code will have
over C when there's heavy object modelling. This can be subjective
but for me, it's fairly significant.
The fact that objects are easily created on the stack and on the
heap is also pretty significant. When considering device models, we
struggle today with device composition.
In real hardware, the i8042 (keyboard controller) is actually
implemented in the PIIX3 which is a chip that is part of the i440fx.
The i440fx acts as both the memory controller and as the PCI Host
controller. So you get something that looks like:
class PIIX3 : public PCIDevice
{
private:
I8042 i8042;
RTC rtc;
// ...
};
The fact that in physical implementation they sit in the same silicon
does not mean that logically they belong to the same class. PIIX3
is ISA bridge. It doesn't mean it owns devices on the ISA bus it
provides. The information that you are trying to convey here belongs to
configuration file. Here I go factory design pattern.
class I440FX : public PCIHostController
{
I440FX(void) {
this-slots[1].plug(this-piix3); // piix3 is always in slot 1
}
private:
PlugPCIDevice * slots[32]; // slot 0 is the PMC
PIIX3 piix3;
};
So whereas we have this very complicate machine create function that
attempts to create and composite all of these devices after the
fact, when written in C++, partially due to good design, but
partially due to the fact that the languages forces you to think a
certain way, you get a tremendous simplification.
Forcing to think you in certain way does not make you suddenly make good
design decisions (if only programming was so easy). But it makes it even
harder to fix bad decision since suddenly all you design depends on it.
A proper C++ device model turns a vast majority of our device
creation complexity into a single new I440FX. Then it's just a
matter of instantiating and plugging the appropriate set of PCI
devices.
That is if you are using code as data. With other design you actually
read I440FX configuration from file and build it from smaller parts.
You see C++ doesn't stop us from arguing what design is correct.
Of course, this can be wrapped in a factory to make it drivable via
an API or config file.
Exactly.
Another area that C++ shines is safety. C++ enables you to inject
safe versions of things that you really can't do in C. For
instance, the PIT has three channels but the mask to select a
channel is two bits. There was a kernel exploit that found a way to
trick selection of a forth channel because of a missing check.
In C++, you can convert:
PITChannel channnels[3];
Into:
ArrayPITChannel, 3 channels;
Any sane modern language gives you that. Why C++?
It behaves in every other way just like a normal array. The memory
is stack allocated, the type has a fixed size. The only difference
is that you can overload the [] operators and implement bounds
checking for array accesses. This means that as long as you use
Array, array overflows disappear from the code base. That's a big
deal.
Another area C++ shines is generating metacode. Consider the
ugliness around VMState. The crux of the problem is that it's not
possible to write type-neutral code in C. This all gets simplified
with C++. Instead of having a bunch of macros like:
VMSTATE_INT8(val0, ...)
VMSTATE_INT16(val1, ...)
You can just have:
vmstate(val0)
vmstate(val1)
And use type overloading to implement different behaviors. Combined
with template specialization and an Array wrapper, the same thing
works for arrays too.
Regards,
Anthony Liguori
Regards,
Anthony Liguori
Alex
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Gleb.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/24/2010 09:40 AM, Gleb Natapov wrote:
On Wed, Nov 24, 2010 at 08:41:26AM -0600, Anthony Liguori wrote:
On 11/24/2010 06:59 AM, Alexander Graf wrote:
On 24.11.2010, at 11:52, Avi Kivity wrote:
Introduce exception-safe objects for calling system, vm, and vcpu ioctls.
Signed-off-by: Avi Kivitya...@redhat.com
FWIW, I still disagree with C++ and believe this code to be hardly readable.
There's a general prettiness that well written C++ code will have
over C when there's heavy object modelling. This can be subjective
but for me, it's fairly significant.
The fact that objects are easily created on the stack and on the
heap is also pretty significant. When considering device models, we
struggle today with device composition.
In real hardware, the i8042 (keyboard controller) is actually
implemented in the PIIX3 which is a chip that is part of the i440fx.
The i440fx acts as both the memory controller and as the PCI Host
controller. So you get something that looks like:
class PIIX3 : public PCIDevice
{
private:
I8042 i8042;
RTC rtc;
// ...
};
The fact that in physical implementation they sit in the same silicon
does not mean that logically they belong to the same class. PIIX3
is ISA bridge. It doesn't mean it owns devices on the ISA bus it
provides. The information that you are trying to convey here belongs to
configuration file.
Why would we specify a PIIX3 device based on a configuration file?
There is only one PIIX3 device in the world. I don't see a lot of need
to create arbitrary types of devices.
The real world hierarchy matters when you're trying to model I/O dispatch.
Here I go factory design pattern.
I think it's a lot of abstraction for very little gain and leads to
bizarreness like treating any platform device as an ISA device.
An RTC is *not* an ISA device. It may sit *behind* an ISA bus because
the SuperIO chip happens to be on the ISA bus. But on modern systems,
the ISA bus has disappeared in favor of the LPC but that doesn't
fundamentally change what the RTC is.
The problem with what you describe is that there are far fewer devices
that actually sit on busses than what QEMU tries to model today.
class I440FX : public PCIHostController
{
I440FX(void) {
this-slots[1].plug(this-piix3); // piix3 is always in slot 1
}
private:
PlugPCIDevice * slots[32]; // slot 0 is the PMC
PIIX3 piix3;
};
So whereas we have this very complicate machine create function that
attempts to create and composite all of these devices after the
fact, when written in C++, partially due to good design, but
partially due to the fact that the languages forces you to think a
certain way, you get a tremendous simplification.
Forcing to think you in certain way does not make you suddenly make good
design decisions (if only programming was so easy). But it makes it even
harder to fix bad decision since suddenly all you design depends on it.
A proper C++ device model turns a vast majority of our device
creation complexity into a single new I440FX. Then it's just a
matter of instantiating and plugging the appropriate set of PCI
devices.
That is if you are using code as data. With other design you actually
read I440FX configuration from file and build it from smaller parts.
You see C++ doesn't stop us from arguing what design is correct.
That's fair. I think 90% of what we need is better design. I think a
better language only gives us 10%.
Another area that C++ shines is safety. C++ enables you to inject
safe versions of things that you really can't do in C. For
instance, the PIT has three channels but the mask to select a
channel is two bits. There was a kernel exploit that found a way to
trick selection of a forth channel because of a missing check.
In C++, you can convert:
PITChannel channnels[3];
Into:
ArrayPITChannel, 3 channels;
Any sane modern language gives you that. Why C++?
Because I don't think we can implement a reasonable device model using a
garbage collected language. Garbage collection introduces
non-determinism and in QEMU we need to ensure that when we're running in
a VCPU context, we exit back to the guest as quickly as possible.
Regards,
Anthony Liguori
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/24/2010 05:50 PM, Anthony Liguori wrote: Any sane modern language gives you that. Why C++? Because I don't think we can implement a reasonable device model using a garbage collected language. Garbage collection introduces non-determinism and in QEMU we need to ensure that when we're running in a VCPU context, we exit back to the guest as quickly as possible. My answer is that C++ is the only language that allows you to evolve away from C, with mixed C/C++ source (not just linkage level compatibility). If there are others, I want to know about them. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On Wed, Nov 24, 2010 at 09:50:35AM -0600, Anthony Liguori wrote:
On 11/24/2010 09:40 AM, Gleb Natapov wrote:
On Wed, Nov 24, 2010 at 08:41:26AM -0600, Anthony Liguori wrote:
On 11/24/2010 06:59 AM, Alexander Graf wrote:
On 24.11.2010, at 11:52, Avi Kivity wrote:
Introduce exception-safe objects for calling system, vm, and vcpu ioctls.
Signed-off-by: Avi Kivitya...@redhat.com
FWIW, I still disagree with C++ and believe this code to be hardly
readable.
There's a general prettiness that well written C++ code will have
over C when there's heavy object modelling. This can be subjective
but for me, it's fairly significant.
The fact that objects are easily created on the stack and on the
heap is also pretty significant. When considering device models, we
struggle today with device composition.
In real hardware, the i8042 (keyboard controller) is actually
implemented in the PIIX3 which is a chip that is part of the i440fx.
The i440fx acts as both the memory controller and as the PCI Host
controller. So you get something that looks like:
class PIIX3 : public PCIDevice
{
private:
I8042 i8042;
RTC rtc;
// ...
};
The fact that in physical implementation they sit in the same silicon
does not mean that logically they belong to the same class. PIIX3
is ISA bridge. It doesn't mean it owns devices on the ISA bus it
provides. The information that you are trying to convey here belongs to
configuration file.
Why would we specify a PIIX3 device based on a configuration file?
There is only one PIIX3 device in the world. I don't see a lot of
need to create arbitrary types of devices.
Why deny this flexibility from those who need it for modelling
different HW? Besides, as I said, PIIX3 is ISA bridge and this
is what class should implement. We have fw_cfg on ISA bus too
which does not exits on real HW and we may want to have other
devices. We should be able to add them without changing PIIX3
class.
The real world hierarchy matters when you're trying to model I/O dispatch.
Or build device path. Any time we do something not as real HW we
regret it later.
Here I go factory design pattern.
I think it's a lot of abstraction for very little gain and leads to
bizarreness like treating any platform device as an ISA device.
Why?
An RTC is *not* an ISA device. It may sit *behind* an ISA bus
because the SuperIO chip happens to be on the ISA bus. But on
modern systems, the ISA bus has disappeared in favor of the LPC but
that doesn't fundamentally change what the RTC is.
Agree. This is a device sitting on the ISA bus on a PC, but it can
sit on other buses too. And it happily does so on other architectures.
The problem with what you describe is that there are far fewer
devices that actually sit on busses than what QEMU tries to model
today.
All devices sit on some buses.
class I440FX : public PCIHostController
{
I440FX(void) {
this-slots[1].plug(this-piix3); // piix3 is always in slot 1
}
private:
PlugPCIDevice * slots[32]; // slot 0 is the PMC
PIIX3 piix3;
};
So whereas we have this very complicate machine create function that
attempts to create and composite all of these devices after the
fact, when written in C++, partially due to good design, but
partially due to the fact that the languages forces you to think a
certain way, you get a tremendous simplification.
Forcing to think you in certain way does not make you suddenly make good
design decisions (if only programming was so easy). But it makes it even
harder to fix bad decision since suddenly all you design depends on it.
A proper C++ device model turns a vast majority of our device
creation complexity into a single new I440FX. Then it's just a
matter of instantiating and plugging the appropriate set of PCI
devices.
That is if you are using code as data. With other design you actually
read I440FX configuration from file and build it from smaller parts.
You see C++ doesn't stop us from arguing what design is correct.
That's fair. I think 90% of what we need is better design. I think
a better language only gives us 10%.
Another area that C++ shines is safety. C++ enables you to inject
safe versions of things that you really can't do in C. For
instance, the PIT has three channels but the mask to select a
channel is two bits. There was a kernel exploit that found a way to
trick selection of a forth channel because of a missing check.
In C++, you can convert:
PITChannel channnels[3];
Into:
ArrayPITChannel, 3 channels;
Any sane modern language gives you that. Why C++?
Because I don't think we can implement a reasonable device model
using a garbage collected language. Garbage collection introduces
non-determinism and in QEMU we need to ensure that when we're
running in a VCPU context, we exit back to the guest as quickly as
possible.
IIRC there are garbage collected languages that allow you to
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/24/2010 06:12 PM, Gleb Natapov wrote: Why would we specify a PIIX3 device based on a configuration file? There is only one PIIX3 device in the world. I don't see a lot of need to create arbitrary types of devices. Why deny this flexibility from those who need it for modelling different HW? The various components exist and can be reused. Besides, as I said, PIIX3 is ISA bridge and this is what class should implement. Isn't it an ISA bridge + a few ISA devices? We have fw_cfg on ISA bus too which does not exits on real HW and we may want to have other devices. We should be able to add them without changing PIIX3 class. fw_cfg should certainly not be a member of PIIX3. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On Wed, Nov 24, 2010 at 06:14:22PM +0200, Avi Kivity wrote: On 11/24/2010 06:12 PM, Gleb Natapov wrote: Why would we specify a PIIX3 device based on a configuration file? There is only one PIIX3 device in the world. I don't see a lot of need to create arbitrary types of devices. Why deny this flexibility from those who need it for modelling different HW? The various components exist and can be reused. So you are saying lets use code as data for some and config files for others. If you have support for building chipsets from data why not simply have 440fx.cfg somewhere? Besides what qemu provides no is not stock PIIX3. We have parts of PIIX4 for power management. Besides, as I said, PIIX3 is ISA bridge and this is what class should implement. Isn't it an ISA bridge + a few ISA devices? Why? Because they happen to be on the same silicon? So then in SoC all devices are in cpu? We have fw_cfg on ISA bus too which does not exits on real HW and we may want to have other devices. We should be able to add them without changing PIIX3 class. fw_cfg should certainly not be a member of PIIX3. It is really not much different from others. -- Gleb. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/24/2010 06:21 PM, Gleb Natapov wrote: On Wed, Nov 24, 2010 at 06:14:22PM +0200, Avi Kivity wrote: On 11/24/2010 06:12 PM, Gleb Natapov wrote: Why would we specify a PIIX3 device based on a configuration file? There is only one PIIX3 device in the world. I don't see a lot of need to create arbitrary types of devices. Why deny this flexibility from those who need it for modelling different HW? The various components exist and can be reused. So you are saying lets use code as data for some and config files for others. If you have support for building chipsets from data why not simply have 440fx.cfg somewhere? I don't object to it. If it can be done, it's a good thing. Often integrated chipsets have lots of little special cases though. For example some pins acting as GPIO if an embedded device is disabled. Besides what qemu provides no is not stock PIIX3. We have parts of PIIX4 for power management. That's a bug. Besides, as I said, PIIX3 is ISA bridge and this is what class should implement. Isn't it an ISA bridge + a few ISA devices? Why? Because they happen to be on the same silicon? So then in SoC all devices are in cpu? PIIX3 is what the PIIX3 spec says it is. We decompose it into the PIIX3 ISA bridge, real time clock, etc. Some of these components are standardized and can be used stand-alone. We have fw_cfg on ISA bus too which does not exits on real HW and we may want to have other devices. We should be able to add them without changing PIIX3 class. fw_cfg should certainly not be a member of PIIX3. It is really not much different from others. I couldn't find it in the PIIX3 spec. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/24/10 13:59, Alexander Graf wrote: On 24.11.2010, at 11:52, Avi Kivity wrote: Introduce exception-safe objects for calling system, vm, and vcpu ioctls. Signed-off-by: Avi Kivity a...@redhat.com FWIW, I still disagree with C++ and believe this code to be hardly readable. YUCK! It's got :'s all over the place that makes no sense whatsoever :( Jes -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On Wed, Nov 24, 2010 at 06:25:44PM +0200, Avi Kivity wrote: On 11/24/2010 06:21 PM, Gleb Natapov wrote: On Wed, Nov 24, 2010 at 06:14:22PM +0200, Avi Kivity wrote: On 11/24/2010 06:12 PM, Gleb Natapov wrote: Why would we specify a PIIX3 device based on a configuration file? There is only one PIIX3 device in the world. I don't see a lot of need to create arbitrary types of devices. Why deny this flexibility from those who need it for modelling different HW? The various components exist and can be reused. So you are saying lets use code as data for some and config files for others. If you have support for building chipsets from data why not simply have 440fx.cfg somewhere? I don't object to it. If it can be done, it's a good thing. Often integrated chipsets have lots of little special cases though. For example some pins acting as GPIO if an embedded device is disabled. Besides what qemu provides no is not stock PIIX3. We have parts of PIIX4 for power management. That's a bug. Besides, as I said, PIIX3 is ISA bridge and this is what class should implement. Isn't it an ISA bridge + a few ISA devices? Why? Because they happen to be on the same silicon? So then in SoC all devices are in cpu? PIIX3 is what the PIIX3 spec says it is. We decompose it into the PIIX3 ISA bridge, real time clock, etc. Some of these components are standardized and can be used stand-alone. So PIIX3 is just a packaging of mostly independent components for cost and space cutting. Just like SoC. We have fw_cfg on ISA bus too which does not exits on real HW and we may want to have other devices. We should be able to add them without changing PIIX3 class. fw_cfg should certainly not be a member of PIIX3. It is really not much different from others. I couldn't find it in the PIIX3 spec. I couldn't find it in _any_ spec. Should we get rid of it? -- Gleb. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/24/2010 06:29 PM, Gleb Natapov wrote: Besides, as I said, PIIX3 is ISA bridge and this is what class should implement. Isn't it an ISA bridge + a few ISA devices? Why? Because they happen to be on the same silicon? So then in SoC all devices are in cpu? PIIX3 is what the PIIX3 spec says it is. We decompose it into the PIIX3 ISA bridge, real time clock, etc. Some of these components are standardized and can be used stand-alone. So PIIX3 is just a packaging of mostly independent components for cost and space cutting. Just like SoC. Plus some magic glue. You can't say it is an ISA bridge. It's exactly what its spec says it is. We have fw_cfg on ISA bus too which does not exits on real HW and we may want to have other devices. We should be able to add them without changing PIIX3 class. fw_cfg should certainly not be a member of PIIX3. It is really not much different from others. I couldn't find it in the PIIX3 spec. I couldn't find it in _any_ spec. Should we get rid of it? Or write a spec. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/24/2010 06:29 PM, Jes Sorensen wrote: FWIW, I still disagree with C++ and believe this code to be hardly readable. YUCK! It's got :'s all over the place that makes no sense whatsoever :( You've got two in one sentence! Practice what you preach. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/24/10 16:50, Anthony Liguori wrote:
On 11/24/2010 09:40 AM, Gleb Natapov wrote:
On Wed, Nov 24, 2010 at 08:41:26AM -0600, Anthony Liguori wrote:
In real hardware, the i8042 (keyboard controller) is actually
implemented in the PIIX3 which is a chip that is part of the i440fx.
The i440fx acts as both the memory controller and as the PCI Host
controller. So you get something that looks like:
class PIIX3 : public PCIDevice
{
private:
I8042 i8042;
RTC rtc;
// ...
};
The fact that in physical implementation they sit in the same silicon
does not mean that logically they belong to the same class. PIIX3
is ISA bridge. It doesn't mean it owns devices on the ISA bus it
provides. The information that you are trying to convey here belongs to
configuration file.
Why would we specify a PIIX3 device based on a configuration file?
There is only one PIIX3 device in the world. I don't see a lot of need
to create arbitrary types of devices.
Well the problem here is that the i8042 is in the i440fx.c file, it
shouldn't be there in the first place. The gluing together things in
silicon is really just a way to shorten the wires and make it easier,
they are still separate devices and as long as the i8042 requires ISA
access, and to be treated like an ISA device, we should glue it onto the
virtual ISA bus within QEMU.
What you did above is making the exact same mistake as is done with the
current i440fx.c code.
I think it's a lot of abstraction for very little gain and leads to
bizarreness like treating any platform device as an ISA device.
An RTC is *not* an ISA device. It may sit *behind* an ISA bus because
the SuperIO chip happens to be on the ISA bus. But on modern systems,
the ISA bus has disappeared in favor of the LPC but that doesn't
fundamentally change what the RTC is.
The problem with what you describe is that there are far fewer devices
that actually sit on busses than what QEMU tries to model today.
A PCI device is mapped on a PCI bus as a PCI function etc. If the device
is hard wired to a fixed address, or worse a fixed IO port, then it is
an ISA device.
So whereas we have this very complicate machine create function that
attempts to create and composite all of these devices after the
fact, when written in C++, partially due to good design, but
partially due to the fact that the languages forces you to think a
certain way, you get a tremendous simplification.
That is clearly dependent on the eyes of the person who looks at it.
That is if you are using code as data. With other design you actually
read I440FX configuration from file and build it from smaller parts.
You see C++ doesn't stop us from arguing what design is correct.
That's fair. I think 90% of what we need is better design. I think a
better language only gives us 10%.
Well the problem is the 10% you are talking about is another 30% loss
because the code is now practically unreadable, plus you open up the can
of worms that people will start using some of the totally broken
features of C++. Sure you can try hard to make sure they don't sneak in,
but down the line they will, and at that point the codebase is totally
ruined.
Avi's unittest code is a perfect example of code that is unreadable for
a C programmer. Or to quote a smart man 'the code is clear as perl!'.
So you have now lost or at least crippled half your developer base,
making it way harder for them to contribute something useful.
This is a big step in the wrong direction :(
Jes
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/24/2010 10:12 AM, Gleb Natapov wrote: Why would we specify a PIIX3 device based on a configuration file? There is only one PIIX3 device in the world. I don't see a lot of need to create arbitrary types of devices. Why deny this flexibility from those who need it for modelling different HW? Besides, as I said, PIIX3 is ISA bridge and this is what class should implement. We have fw_cfg on ISA bus too which does not exits on real HW and we may want to have other devices. We should be able to add them without changing PIIX3 class. Because this flexibility is a misfeature. It's something that noone actually uses and I've never seen anyone ask to have. It introduces a PC-centric view of the world where you artificially make a bunch of devices have bus specific logic that they really don't have. If you want to go down this route, the right approach to take is to make a separate IsaMc161818a device that has a Mc16818a device with a has-a relation (i.e. composition). But I don't see the point. Just stick the Mc16818a device in the PIIX3 via composition as it exists on normal hardware and call it a day. BTW, the only reason any of this works is because of the fact that the PIIX3 does subtractive decoding which allows arbitrary ISA devices to be plugged into the ISA bus. But subtractive decoding is very expensive and on modern LPC implementations, positive decoding is used which means that the chipset has a white list of ports for devices that sit on it. PCs are simply not as architecturally clean as I think you're advocating we try to model them. The real world hierarchy matters when you're trying to model I/O dispatch. Or build device path. Any time we do something not as real HW we regret it later. Right, and real HW does composition in the PIIX3 device. So let's not regret making everything an ISA device later. An RTC is *not* an ISA device. It may sit *behind* an ISA bus because the SuperIO chip happens to be on the ISA bus. But on modern systems, the ISA bus has disappeared in favor of the LPC but that doesn't fundamentally change what the RTC is. Agree. This is a device sitting on the ISA bus on a PC, but it can sit on other buses too. And it happily does so on other architectures. It doesn't sit on the ISA bus. A SuperIO chip sits on the ISA bus and the SuperIO chip decodes the ISA bus traffic and dispatches approriately. On the PIIX3, the SuperIO chip is part of the chipset. An ISA transaction isn't necessary to talk to the RTC--it would simply be too expensive to do this because of subtractive decoding. The problem with what you describe is that there are far fewer devices that actually sit on busses than what QEMU tries to model today. All devices sit on some buses. That's simply not true. Let's be specific about what a bus is. A bus is a set of lines that is shared by multiple devices with some mechanism to arbitrate who's using the bus at any given time. In PCI, roughly speaking, a transaction is sent from the controller to all devices on the bus and the device that is to handle the transaction will raise it's DEVSEL line to indicate that it's handling it. But there are plenty of cases where a device is hard wired to another device. In a simple microcontroller, it's extremely common to tie a set of GPIO pins to a specific device. There's no bus here. The i8042 is another good example. The i8042 has two ps2 plugs but it's not a bus. The keyboard port takes a keyboard and the aux port takes a mouse. You can't hook up two keyboards and you can't hook up two mice. Because I don't think we can implement a reasonable device model using a garbage collected language. Garbage collection introduces non-determinism and in QEMU we need to ensure that when we're running in a VCPU context, we exit back to the guest as quickly as possible. IIRC there are garbage collected languages that allow you to disable garbage collection for certain part of the code. Yeah, but very few languages that I know of are sophisticated here. Garbage collection can be done while vcpu executes guest code and in proper implementation vcpu thread should execute device model code for a very brief time and do not sleep there. All this makes me think that garbage collection shouldn't be a big issue for kvm userspace. But I don't see the point. If you look at my repository, there's very few memory allocations. This is largely due to the fact that I don't overly abstract busses and rely on simple composition where appropriate. Plus, with tr1::smart_pointers, you can be leak free without every worrying about explicit freeing. There are, of course, possibilities of having circular references but it's not too hard to avoid that in practice. Regards, Anthony Liguori -- Gleb. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/24/2010 10:21 AM, Gleb Natapov wrote: On Wed, Nov 24, 2010 at 06:14:22PM +0200, Avi Kivity wrote: On 11/24/2010 06:12 PM, Gleb Natapov wrote: Why would we specify a PIIX3 device based on a configuration file? There is only one PIIX3 device in the world. I don't see a lot of need to create arbitrary types of devices. Why deny this flexibility from those who need it for modelling different HW? The various components exist and can be reused. So you are saying lets use code as data for some and config files for others. If you have support for building chipsets from data why not simply have 440fx.cfg somewhere? Besides what qemu provides no is not stock PIIX3. We have parts of PIIX4 for power management. Besides, as I said, PIIX3 is ISA bridge and this is what class should implement. Isn't it an ISA bridge + a few ISA devices? Why? Because they happen to be on the same silicon? So then in SoC all devices are in cpu? They *aren't* ISA devices. Look at the PIIX3 spec. All of the ports for these devices are positively decoded and not sent over the ISA bus. You could model them as being behind the ISA bus but you could also model them as being behind the PCI bus. Regards, Anthony Liguori We have fw_cfg on ISA bus too which does not exits on real HW and we may want to have other devices. We should be able to add them without changing PIIX3 class. fw_cfg should certainly not be a member of PIIX3. It is really not much different from others. -- Gleb. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/24/10 17:34, Avi Kivity wrote: On 11/24/2010 06:29 PM, Jes Sorensen wrote: FWIW, I still disagree with C++ and believe this code to be hardly readable. YUCK! It's got :'s all over the place that makes no sense whatsoever :( You've got two in one sentence! Practice what you preach. If you want to reject my patches due to my broken English, go ahead. At least it should be somewhat readable. Jes -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/24/2010 06:40 PM, Jes Sorensen wrote: The fact that in physical implementation they sit in the same silicon does not mean that logically they belong to the same class. PIIX3 is ISA bridge. It doesn't mean it owns devices on the ISA bus it provides. The information that you are trying to convey here belongs to configuration file. Why would we specify a PIIX3 device based on a configuration file? There is only one PIIX3 device in the world. I don't see a lot of need to create arbitrary types of devices. Well the problem here is that the i8042 is in the i440fx.c file, it shouldn't be there in the first place. The gluing together things in silicon is really just a way to shorten the wires and make it easier, they are still separate devices and as long as the i8042 requires ISA access, and to be treated like an ISA device, we should glue it onto the virtual ISA bus within QEMU. What you did above is making the exact same mistake as is done with the current i440fx.c code. If a real life 440fx has an i8042, then an emulated 440fx should have an emulated i8042. It's not complicated. So whereas we have this very complicate machine create function that attempts to create and composite all of these devices after the fact, when written in C++, partially due to good design, but partially due to the fact that the languages forces you to think a certain way, you get a tremendous simplification. That is clearly dependent on the eyes of the person who looks at it. It's independent of language. It should be done in C as well. C++ just makes it easier by reducing the boilerplate. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On Wed, Nov 24, 2010 at 10:43:57AM -0600, Anthony Liguori wrote: On 11/24/2010 10:21 AM, Gleb Natapov wrote: On Wed, Nov 24, 2010 at 06:14:22PM +0200, Avi Kivity wrote: On 11/24/2010 06:12 PM, Gleb Natapov wrote: Why would we specify a PIIX3 device based on a configuration file? There is only one PIIX3 device in the world. I don't see a lot of need to create arbitrary types of devices. Why deny this flexibility from those who need it for modelling different HW? The various components exist and can be reused. So you are saying lets use code as data for some and config files for others. If you have support for building chipsets from data why not simply have 440fx.cfg somewhere? Besides what qemu provides no is not stock PIIX3. We have parts of PIIX4 for power management. Besides, as I said, PIIX3 is ISA bridge and this is what class should implement. Isn't it an ISA bridge + a few ISA devices? Why? Because they happen to be on the same silicon? So then in SoC all devices are in cpu? They *aren't* ISA devices. Look at the PIIX3 spec. All of the ports for these devices are positively decoded and not sent over the ISA bus. Over the external ISA bus you mean? You could model them as being behind the ISA bus but you could also model them as being behind the PCI bus. Just yesterday I checked how different ports behave if you use inw/inl to read data from them. They behave very different from what PCI spec says. This was recent HW. -- Gleb. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/24/2010 06:44 PM, Jes Sorensen wrote: It's got :'s all over the place that makes no sense whatsoever :( You've got two in one sentence! Practice what you preach. If you want to reject my patches due to my broken English, go ahead. At least it should be somewhat readable. I was referring to your : count. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/24/10 17:47, Avi Kivity wrote: On 11/24/2010 06:40 PM, Jes Sorensen wrote: Well the problem here is that the i8042 is in the i440fx.c file, it shouldn't be there in the first place. The gluing together things in silicon is really just a way to shorten the wires and make it easier, they are still separate devices and as long as the i8042 requires ISA access, and to be treated like an ISA device, we should glue it onto the virtual ISA bus within QEMU. What you did above is making the exact same mistake as is done with the current i440fx.c code. If a real life 440fx has an i8042, then an emulated 440fx should have an emulated i8042. It's not complicated. It's a question of how it is accessed, if it is treated like an ISA device by the silicon, we should treat it like an ISA device in QEMU, rather than pretend it is something that it isn't. So whereas we have this very complicate machine create function that attempts to create and composite all of these devices after the fact, when written in C++, partially due to good design, but partially due to the fact that the languages forces you to think a certain way, you get a tremendous simplification. That is clearly dependent on the eyes of the person who looks at it. It's independent of language. It should be done in C as well. C++ just makes it easier by reducing the boilerplate. Right we need good design for our C code, which we are lacking to a large extend. However that has nothing to do with the language, that has to do with the developers. Jes -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On Wed, Nov 24, 2010 at 06:33:17PM +0200, Avi Kivity wrote: On 11/24/2010 06:29 PM, Gleb Natapov wrote: Besides, as I said, PIIX3 is ISA bridge and this is what class should implement. Isn't it an ISA bridge + a few ISA devices? Why? Because they happen to be on the same silicon? So then in SoC all devices are in cpu? PIIX3 is what the PIIX3 spec says it is. We decompose it into the PIIX3 ISA bridge, real time clock, etc. Some of these components are standardized and can be used stand-alone. So PIIX3 is just a packaging of mostly independent components for cost and space cutting. Just like SoC. Plus some magic glue. You can't say it is an ISA bridge. It's exactly what its spec says it is. First thing my spec says is Bridge Between the PCI Bus and ISA Bus We have fw_cfg on ISA bus too which does not exits on real HW and we may want to have other devices. We should be able to add them without changing PIIX3 class. fw_cfg should certainly not be a member of PIIX3. It is really not much different from others. I couldn't find it in the PIIX3 spec. I couldn't find it in _any_ spec. Should we get rid of it? Or write a spec. It will not make it part of any existing system. -- Gleb. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/24/2010 10:40 AM, Jes Sorensen wrote: Well the problem here is that the i8042 is in the i440fx.c file, it shouldn't be there in the first place. The gluing together things in silicon is really just a way to shorten the wires and make it easier, they are still separate devices and as long as the i8042 requires ISA access, and to be treated like an ISA device, we should glue it onto the virtual ISA bus within QEMU. Nope. The PC is much, much uglier than everyone seems to realize. Part of the reason the PIIX3 implements the i8042 (and the i440fx implements the PIIX3) is because the i8042 is needed to lower the a20 line. IOW, the i8042 has to signal to the memory controller to modify accesses to memory addresses from the CPU. It's not just a statement of cost savings. One of the reasons everything's in the same piece of silicon is because of the horribly incestuous nature of the PC architecture. It's not a nice tree of parent busses and children devices. It's an interbreeding group of devices who's parent's are their siblings and brother's are their uncles. I think it's a lot of abstraction for very little gain and leads to bizarreness like treating any platform device as an ISA device. An RTC is *not* an ISA device. It may sit *behind* an ISA bus because the SuperIO chip happens to be on the ISA bus. But on modern systems, the ISA bus has disappeared in favor of the LPC but that doesn't fundamentally change what the RTC is. The problem with what you describe is that there are far fewer devices that actually sit on busses than what QEMU tries to model today. A PCI device is mapped on a PCI bus as a PCI function etc. If the device is hard wired to a fixed address, or worse a fixed IO port, then it is an ISA device. Wrong. The PCI controller sends all port and memory I/O operations over the bus. Any PCI device can select to handle any address they want by simply issuing a DEVSEL. The ability to do switching wasn't introduced until PCI-X. The way a PCI-to-ISA bridge works is by waiting for a full bus cycle to see that no device has issued a DEVSEL, and then issuing a DEVSEL. This is called subtractive decoding. It then forwards the I/O request over the ISA bus and roughly the same thing happens on the ISA bus. That is if you are using code as data. With other design you actually read I440FX configuration from file and build it from smaller parts. You see C++ doesn't stop us from arguing what design is correct. That's fair. I think 90% of what we need is better design. I think a better language only gives us 10%. Well the problem is the 10% you are talking about is another 30% loss because the code is now practically unreadable, plus you open up the can of worms that people will start using some of the totally broken features of C++. Did you look at my code or Avi's code? Look at my code and then tell me it's practically unreadable. http://git.qemu.org/qemupp.git/tree/test/test-mc146818a.cpp Sure you can try hard to make sure they don't sneak in, but down the line they will, and at that point the codebase is totally ruined. Avi's unittest code is a perfect example of code that is unreadable for a C programmer. Or to quote a smart man 'the code is clear as perl!'. So you have now lost or at least crippled half your developer base, making it way harder for them to contribute something useful. This is a big step in the wrong direction :( I wouldn't have written the unittest code to use classes or exceptions at all. I don't think it's a good fit. Regards, Anthony Liguori Jes -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On Wed, Nov 24, 2010 at 06:47:28PM +0200, Avi Kivity wrote: On 11/24/2010 06:40 PM, Jes Sorensen wrote: The fact that in physical implementation they sit in the same silicon does not mean that logically they belong to the same class. PIIX3 is ISA bridge. It doesn't mean it owns devices on the ISA bus it provides. The information that you are trying to convey here belongs to configuration file. Why would we specify a PIIX3 device based on a configuration file? There is only one PIIX3 device in the world. I don't see a lot of need to create arbitrary types of devices. Well the problem here is that the i8042 is in the i440fx.c file, it shouldn't be there in the first place. The gluing together things in silicon is really just a way to shorten the wires and make it easier, they are still separate devices and as long as the i8042 requires ISA access, and to be treated like an ISA device, we should glue it onto the virtual ISA bus within QEMU. What you did above is making the exact same mistake as is done with the current i440fx.c code. If a real life 440fx has an i8042, then an emulated 440fx should have an emulated i8042. It's not complicated. Correct. But it can be achieved by making 440fx a class that includes other classes or by building it from different classes linked through common interfaces. -- Gleb. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/24/2010 10:48 AM, Gleb Natapov wrote: They *aren't* ISA devices. Look at the PIIX3 spec. All of the ports for these devices are positively decoded and not sent over the ISA bus. Over the external ISA bus you mean? There is no internal ISA bus. The reality is that the PIIX3 is a microcontroller and most of the platform devices are probably written in microcode. That's certainly the case with modern SuperIO chips. Very specifically, the PIIX3 has a white list of addresses that when it sees the a PCI bus transaction for those addresses, it asserts DEVSEL# and then routes the request to the write part of the chip to handle it. For unhandled transactions, it then forwards them to the ISA bus. Regards, Anthony Liguori -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/24/2010 06:52 PM, Gleb Natapov wrote: Plus some magic glue. You can't say it is an ISA bridge. It's exactly what its spec says it is. First thing my spec says is Bridge Between the PCI Bus and ISA Bus It's the first item in a list of features. Be serious. I couldn't find it in the PIIX3 spec. I couldn't find it in _any_ spec. Should we get rid of it? Or write a spec. It will not make it part of any existing system. That is not a requirement. We have virtio as well. When we do have a spec for something, we should implement it as the spec says, not according to our ideas of how it should be. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/24/2010 10:51 AM, Jes Sorensen wrote: On 11/24/10 17:47, Avi Kivity wrote: On 11/24/2010 06:40 PM, Jes Sorensen wrote: Well the problem here is that the i8042 is in the i440fx.c file, it shouldn't be there in the first place. The gluing together things in silicon is really just a way to shorten the wires and make it easier, they are still separate devices and as long as the i8042 requires ISA access, and to be treated like an ISA device, we should glue it onto the virtual ISA bus within QEMU. What you did above is making the exact same mistake as is done with the current i440fx.c code. If a real life 440fx has an i8042, then an emulated 440fx should have an emulated i8042. It's not complicated. It's a question of how it is accessed, if it is treated like an ISA device by the silicon, we should treat it like an ISA device in QEMU, rather than pretend it is something that it isn't. Does anyone have any evidence that the i8042 has anything to do with the ISA bus at all other than the fact that people have some weird notion in their head that if a pio/mmio operation isn't for a PCI device, it must be ISA? Regards, Anthony Liguori -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/24/2010 06:51 PM, Jes Sorensen wrote: On 11/24/10 17:47, Avi Kivity wrote: On 11/24/2010 06:40 PM, Jes Sorensen wrote: Well the problem here is that the i8042 is in the i440fx.c file, it shouldn't be there in the first place. The gluing together things in silicon is really just a way to shorten the wires and make it easier, they are still separate devices and as long as the i8042 requires ISA access, and to be treated like an ISA device, we should glue it onto the virtual ISA bus within QEMU. What you did above is making the exact same mistake as is done with the current i440fx.c code. If a real life 440fx has an i8042, then an emulated 440fx should have an emulated i8042. It's not complicated. It's a question of how it is accessed, if it is treated like an ISA device by the silicon, we should treat it like an ISA device in QEMU, rather than pretend it is something that it isn't. No one is saying it shouldn't be an ISA device if that's how PIIX3 treats it. But if PIIX3 contains it, then the code that emulates PIIX3 should contain the code that emulates i8042. So whereas we have this very complicate machine create function that attempts to create and composite all of these devices after the fact, when written in C++, partially due to good design, but partially due to the fact that the languages forces you to think a certain way, you get a tremendous simplification. That is clearly dependent on the eyes of the person who looks at it. It's independent of language. It should be done in C as well. C++ just makes it easier by reducing the boilerplate. Right we need good design for our C code, which we are lacking to a large extend. However that has nothing to do with the language, that has to do with the developers. I'm sure patches will be welcome. C++ doesn't enforce good design. But it allows a good design to be enforced. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/24/2010 06:55 PM, Gleb Natapov wrote:
What you did above is making the exact same mistake as is done with the
current i440fx.c code.
If a real life 440fx has an i8042, then an emulated 440fx should
have an emulated i8042. It's not complicated.
Correct. But it can be achieved by making 440fx a class that includes
other classes or by building it from different classes linked through
common interfaces.
Both are fine, and not in conflict with the example that started this.
If the i8042 is completely stock, we write
class i440fx {
private:
i8042 kbc;
}
(or the C equivalent)
If it's not completely stock, we substitute some subclass that takes
care of the differences.
--
error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On Wed, Nov 24, 2010 at 06:56:52PM +0200, Avi Kivity wrote: On 11/24/2010 06:52 PM, Gleb Natapov wrote: Plus some magic glue. You can't say it is an ISA bridge. It's exactly what its spec says it is. First thing my spec says is Bridge Between the PCI Bus and ISA Bus It's the first item in a list of features. Be serious. I am serious. The fact that it provides IDE or kbd doesn't make this IDE or kbd special. It means that it has gates that provide functionality of those chips. Just like SoC really. IDE doesn't become part of ARM cpu just because some SoC somewhere include them on the same silicon. I couldn't find it in the PIIX3 spec. I couldn't find it in _any_ spec. Should we get rid of it? Or write a spec. It will not make it part of any existing system. That is not a requirement. We have virtio as well. When we do have a spec for something, we should implement it as the spec says, not according to our ideas of how it should be. PIIX3 is composite device. It is not one device. To emulate it you need to provide functionality of all devices included om PIIX3. Spec says nothing about how it should be implemented. -- Gleb. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/24/10 17:53, Anthony Liguori wrote: On 11/24/2010 10:40 AM, Jes Sorensen wrote: Well the problem is the 10% you are talking about is another 30% loss because the code is now practically unreadable, plus you open up the can of worms that people will start using some of the totally broken features of C++. Did you look at my code or Avi's code? Yes I looked at Avi's patch before commenting, it is unreadable! I have no clue what is going on in that code. Random use of :: and bleh which makes no sense whatsoever. Look at my code and then tell me it's practically unreadable. http://git.qemu.org/qemupp.git/tree/test/test-mc146818a.cpp Looked at that, and there is nothing in that file that couldn't been done just as cleanly in pure C. Sure you can try hard to make sure they don't sneak in, but down the line they will, and at that point the codebase is totally ruined. Avi's unittest code is a perfect example of code that is unreadable for a C programmer. Or to quote a smart man 'the code is clear as perl!'. So you have now lost or at least crippled half your developer base, making it way harder for them to contribute something useful. This is a big step in the wrong direction :( I wouldn't have written the unittest code to use classes or exceptions at all. I don't think it's a good fit. I don't see qemupp being a good fit either :( Jes -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On Wed, Nov 24, 2010 at 10:56:15AM -0600, Anthony Liguori wrote: On 11/24/2010 10:48 AM, Gleb Natapov wrote: They *aren't* ISA devices. Look at the PIIX3 spec. All of the ports for these devices are positively decoded and not sent over the ISA bus. Over the external ISA bus you mean? There is no internal ISA bus. The reality is that the PIIX3 is a microcontroller and most of the platform devices are probably written in microcode. That's certainly the case with modern SuperIO chips. Qemu also does not have ISA bus. It has code that emulates ISA bus. It is not important how legacy functionality is implemented as long as it is compliant to legacy specification. Very specifically, the PIIX3 has a white list of addresses that when it sees the a PCI bus transaction for those addresses, it asserts DEVSEL# and then routes the request to the write part of the chip to handle it. For unhandled transactions, it then forwards them to the ISA bus. Yeah. In other words it serves some ISA transactions internally and others forward to external bus. -- Gleb. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/24/10 17:59, Avi Kivity wrote: On 11/24/2010 06:51 PM, Jes Sorensen wrote: Right we need good design for our C code, which we are lacking to a large extend. However that has nothing to do with the language, that has to do with the developers. I'm sure patches will be welcome. C++ doesn't enforce good design. But it allows a good design to be enforced. Sorry but that is utterly and completely bogus! The enforcement is only as good as the developers and maintainers make it, and C++ has more trapdoors than C has ever had. Sure you can shoot yourself in the foot in C, in C++ you take the whole leg off instead. Jes -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/24/2010 07:02 PM, Gleb Natapov wrote:
On Wed, Nov 24, 2010 at 06:56:52PM +0200, Avi Kivity wrote:
On 11/24/2010 06:52 PM, Gleb Natapov wrote:
Plus some magic glue. You can't say it is an ISA bridge. It's
exactly what its spec says it is.
First thing my spec says is Bridge Between the PCI Bus and ISA Bus
It's the first item in a list of features. Be serious.
I am serious. The fact that it provides IDE or kbd doesn't make this IDE
or kbd special. It means that it has gates that provide functionality of
those chips. Just like SoC really. IDE doesn't become part of ARM cpu
just because some SoC somewhere include them on the same silicon.
They aren't special. They're just part of the PIIX3 device.
When we do have a spec for something, we should implement it as the
spec says, not according to our ideas of how it should be.
PIIX3 is composite device. It is not one device. To emulate it you need
to provide functionality of all devices included om PIIX3. Spec says
nothing about how it should be implemented.
Right, we could (and probably should) implement it as
class PIIX3 {
private:
class isa_bridge { };
isa_bridge isa_bridge;
i8042 kbc;
...
}
So the thing becomes just a glue layer for various components. But when
pc.c says PIIX3, it gets what the spec says, not something else.
--
error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/24/10 18:07, Avi Kivity wrote: On 11/24/2010 07:02 PM, Gleb Natapov wrote: On Wed, Nov 24, 2010 at 06:56:52PM +0200, Avi Kivity wrote: On 11/24/2010 06:52 PM, Gleb Natapov wrote: Plus some magic glue. You can't say it is an ISA bridge. It's exactly what its spec says it is. First thing my spec says is Bridge Between the PCI Bus and ISA Bus It's the first item in a list of features. Be serious. I am serious. The fact that it provides IDE or kbd doesn't make this IDE or kbd special. It means that it has gates that provide functionality of those chips. Just like SoC really. IDE doesn't become part of ARM cpu just because some SoC somewhere include them on the same silicon. They aren't special. They're just part of the PIIX3 device. No, it is the other way round, the PIIX3 device is a glue together of these different devices. Jes -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/24/2010 07:06 PM, Jes Sorensen wrote:
On 11/24/10 17:59, Avi Kivity wrote:
On 11/24/2010 06:51 PM, Jes Sorensen wrote:
Right we need good design for our C code, which we are lacking to a
large extend. However that has nothing to do with the language, that has
to do with the developers.
I'm sure patches will be welcome.
C++ doesn't enforce good design. But it allows a good design to be
enforced.
Sorry but that is utterly and completely bogus! The enforcement is only
as good as the developers and maintainers make it,
class File {
public:
virtual ~File() {}
virtual void read(...) = 0;
virtual void write(...) = 0;
};
Anyone wishing to implement this interface is forced to implement read
and write methods (callbacks) with exactly the right signature. The
compiler will complain if they don't. So if File is a good interface,
we can make the compiler force people to use it correctly.
We can emulate this in C with -ops- things, but that's just
boilerplate and more places for people to get things wrong, or lazy and
take shortcuts.
--
error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/24/2010 07:10 PM, Jes Sorensen wrote: On 11/24/10 18:07, Avi Kivity wrote: On 11/24/2010 07:02 PM, Gleb Natapov wrote: On Wed, Nov 24, 2010 at 06:56:52PM +0200, Avi Kivity wrote: On 11/24/2010 06:52 PM, Gleb Natapov wrote: Plus some magic glue. You can't say it is an ISA bridge. It's exactly what its spec says it is. First thing my spec says is Bridge Between the PCI Bus and ISA Bus It's the first item in a list of features. Be serious. I am serious. The fact that it provides IDE or kbd doesn't make this IDE or kbd special. It means that it has gates that provide functionality of those chips. Just like SoC really. IDE doesn't become part of ARM cpu just because some SoC somewhere include them on the same silicon. They aren't special. They're just part of the PIIX3 device. No, it is the other way round, the PIIX3 device is a glue together of these different devices. That's exactly what I'm saying. PIIX3 contains all those devices. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/24/2010 11:07 AM, Avi Kivity wrote:
On 11/24/2010 07:02 PM, Gleb Natapov wrote:
On Wed, Nov 24, 2010 at 06:56:52PM +0200, Avi Kivity wrote:
On 11/24/2010 06:52 PM, Gleb Natapov wrote:
Plus some magic glue. You can't say it is an ISA bridge. It's
exactly what its spec says it is.
First thing my spec says is Bridge Between the PCI Bus and ISA Bus
It's the first item in a list of features. Be serious.
I am serious. The fact that it provides IDE or kbd doesn't make this IDE
or kbd special. It means that it has gates that provide functionality of
those chips. Just like SoC really. IDE doesn't become part of ARM cpu
just because some SoC somewhere include them on the same silicon.
They aren't special. They're just part of the PIIX3 device.
So based on the IRC discussion that pbrook and I have been having, you
can think of things like this:
class PIIX3 {
public:
PlugI8042 i8042;
};
A plug allows conditional composition whereas the absence of a plug
means unconditional composition. There is a mechanism to name plugs and
you can have an i8042 factory to create the device and then
conditionally attach it to the plug.
This is fine even though I think it's unnecessary abstraction. What I
object to is introducing an artificial ISA bus layering.
A PlugType is basically a Type * except it has some additional
features. It provides null-pointer dereference checking, provides a
marshalling interface, and supports the ability to be locked which
enables a device to enforce that plugging cannot happen after the device
is realized (power-on).
When we do have a spec for something, we should implement it as the
spec says, not according to our ideas of how it should be.
PIIX3 is composite device. It is not one device. To emulate it you need
to provide functionality of all devices included om PIIX3. Spec says
nothing about how it should be implemented.
Right, we could (and probably should) implement it as
class PIIX3 {
private:
class isa_bridge { };
isa_bridge isa_bridge;
No, PIIX3 is an PCI-to-ISA bridge.
It's a fundamental part of the definition of it as a PCI device.
Regards,
Anthony Liguori
i8042 kbc;
...
}
So the thing becomes just a glue layer for various components. But
when pc.c says PIIX3, it gets what the spec says, not something else.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On Wed, Nov 24, 2010 at 07:01:00PM +0200, Avi Kivity wrote:
On 11/24/2010 06:55 PM, Gleb Natapov wrote:
What you did above is making the exact same mistake as is done with the
current i440fx.c code.
If a real life 440fx has an i8042, then an emulated 440fx should
have an emulated i8042. It's not complicated.
Correct. But it can be achieved by making 440fx a class that includes
other classes or by building it from different classes linked through
common interfaces.
Both are fine, and not in conflict with the example that started this.
If the i8042 is completely stock, we write
class i440fx {
private:
i8042 kbc;
}
(or the C equivalent)
If it's not completely stock, we substitute some subclass that takes
care of the differences.
And if you want to connect ISA sound blaster to that you make subclass
with SB device? No. You make ISA bus functionality available from i440fx
and connect SB there. So now you have bunch of devices that are part of
i440fx class and others that are connected via ISA bus functionality and
all that because you read to deeply into PIIX3 spec?
--
Gleb.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/24/10 18:11, Avi Kivity wrote:
On 11/24/2010 07:06 PM, Jes Sorensen wrote:
Sorry but that is utterly and completely bogus! The enforcement is only
as good as the developers and maintainers make it,
class File {
public:
virtual ~File() {}
virtual void read(...) = 0;
virtual void write(...) = 0;
};
Anyone wishing to implement this interface is forced to implement read
and write methods (callbacks) with exactly the right signature. The
compiler will complain if they don't. So if File is a good interface,
we can make the compiler force people to use it correctly.
We can emulate this in C with -ops- things, but that's just
boilerplate and more places for people to get things wrong, or lazy and
take shortcuts.
In the mean time we spend our time debugging the runtime because the
virtual functions don't behave as expected. In C we know what is going
on, in C++ it is pray and hope.
Jes
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/24/2010 07:17 PM, Jes Sorensen wrote: Anyone wishing to implement this interface is forced to implement read and write methods (callbacks) with exactly the right signature. The compiler will complain if they don't. So if File is a good interface, we can make the compiler force people to use it correctly. We can emulate this in C with -ops- things, but that's just boilerplate and more places for people to get things wrong, or lazy and take shortcuts. In the mean time we spend our time debugging the runtime because the virtual functions don't behave as expected. In C we know what is going on, in C++ it is pray and hope. That is pure bullshit. All major browsers are written in C++, all major office suites, one leading free desktop, google, countless other projects. There is a lot more C++ code in the world than C code. If virtual functions didn't behave as expected, surely we'd hear by now. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/24/2010 07:16 PM, Gleb Natapov wrote:
If the i8042 is completely stock, we write
class i440fx {
private:
i8042 kbc;
}
(or the C equivalent)
If it's not completely stock, we substitute some subclass that takes
care of the differences.
And if you want to connect ISA sound blaster to that you make subclass
with SB device? No. You make ISA bus functionality available from i440fx
and connect SB there.
Correct. A PIIX3 doesn't contain an SB device.
So now you have bunch of devices that are part of
i440fx class and others that are connected via ISA bus functionality and
all that because you read to deeply into PIIX3 spec?
Yes.
--
error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/24/2010 11:17 AM, Jes Sorensen wrote:
On 11/24/10 18:11, Avi Kivity wrote:
On 11/24/2010 07:06 PM, Jes Sorensen wrote:
Sorry but that is utterly and completely bogus! The enforcement is only
as good as the developers and maintainers make it,
class File {
public:
virtual ~File() {}
virtual void read(...) = 0;
virtual void write(...) = 0;
};
Anyone wishing to implement this interface is forced to implement read
and write methods (callbacks) with exactly the right signature. The
compiler will complain if they don't. So if File is a good interface,
we can make the compiler force people to use it correctly.
We can emulate this in C with -ops- things, but that's just
boilerplate and more places for people to get things wrong, or lazy and
take shortcuts.
In the mean time we spend our time debugging the runtime because the
virtual functions don't behave as expected. In C we know what is going
on, in C++ it is pray and hope.
The alternative in C is:
struct file_operations {
void (*release)(void);
void (*read)(...);
void (*write)(...);
};
struct file {
struct file_operations *ops;
};
If I do:
static file_operations posix_file_ops = {
.read = posix_file_read,
.write = posix_file_write,
};
struct posix_file {
struct file parent;
int fd;
};
void posix_file_init(struct posix_file *obj)
{
obj-parent.ops = posix_file_ops;
obj-fd = ...;
};
The compiler won't generate an error. Only upon a call to
file_release() will a null pointer dereference happens whereas in C++,
because this paradigm is structured in the language, the compiler can
help assist you.
Regards,
Anthony Liguori
Jes
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/24/10 18:25, Avi Kivity wrote: On 11/24/2010 07:17 PM, Jes Sorensen wrote: In the mean time we spend our time debugging the runtime because the virtual functions don't behave as expected. In C we know what is going on, in C++ it is pray and hope. That is pure bullshit. All major browsers are written in C++, all major office suites, one leading free desktop, google, countless other projects. There is a lot more C++ code in the world than C code. If virtual functions didn't behave as expected, surely we'd hear by now. And those office suites and web browsers all work s well :( Jes -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/24/2010 06:57 PM, Anthony Liguori wrote: It's a question of how it is accessed, if it is treated like an ISA device by the silicon, we should treat it like an ISA device in QEMU, rather than pretend it is something that it isn't. Does anyone have any evidence that the i8042 has anything to do with the ISA bus at all other than the fact that people have some weird notion in their head that if a pio/mmio operation isn't for a PCI device, it must be ISA? It cannot be an ISA device since ISA does not provide a way to assert IRQ1. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/24/2010 07:28 PM, Jes Sorensen wrote: On 11/24/10 18:25, Avi Kivity wrote: On 11/24/2010 07:17 PM, Jes Sorensen wrote: In the mean time we spend our time debugging the runtime because the virtual functions don't behave as expected. In C we know what is going on, in C++ it is pray and hope. That is pure bullshit. All major browsers are written in C++, all major office suites, one leading free desktop, google, countless other projects. There is a lot more C++ code in the world than C code. If virtual functions didn't behave as expected, surely we'd hear by now. And those office suites and web browsers all work s well :( Let's see the C alternatives. Anyone for elinks? -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On Wed, Nov 24, 2010 at 10:40:05AM -0600, Anthony Liguori wrote: On 11/24/2010 10:12 AM, Gleb Natapov wrote: Why would we specify a PIIX3 device based on a configuration file? There is only one PIIX3 device in the world. I don't see a lot of need to create arbitrary types of devices. Why deny this flexibility from those who need it for modelling different HW? Besides, as I said, PIIX3 is ISA bridge and this is what class should implement. We have fw_cfg on ISA bus too which does not exits on real HW and we may want to have other devices. We should be able to add them without changing PIIX3 class. Because this flexibility is a misfeature. It's something that noone actually uses and I've never seen anyone ask to have. It introduces a PC-centric view of the world where you artificially make a bunch of devices have bus specific logic that they really don't have. Actually other way around. Non PC embedded systems are those that needs this flexibility the most. If you want to go down this route, the right approach to take is to make a separate IsaMc161818a device that has a Mc16818a device with a has-a relation (i.e. composition). There are many devices in qemu that accessed via isa bus on PCI and via MMIO in other archs, so the thing you propose here is logical and actually implemented this way in qdev. But I don't see the point. Just stick the Mc16818a device in the PIIX3 via composition as it exists on normal hardware and call it a day. Shortcuts, shortcuts. I think this is how qemu was written initially. Lets put this here and call it a day! BTW, the only reason any of this works is because of the fact that the PIIX3 does subtractive decoding which allows arbitrary ISA devices to be plugged into the ISA bus. But subtractive decoding is very expensive and on modern LPC implementations, positive decoding is used which means that the chipset has a white list of ports for devices that sit on it. PCs are simply not as architecturally clean as I think you're advocating we try to model them. The real world hierarchy matters when you're trying to model I/O dispatch. Or build device path. Any time we do something not as real HW we regret it later. Right, and real HW does composition in the PIIX3 device. So let's not regret making everything an ISA device later. You design by packaging not functionality? An RTC is *not* an ISA device. It may sit *behind* an ISA bus because the SuperIO chip happens to be on the ISA bus. But on modern systems, the ISA bus has disappeared in favor of the LPC but that doesn't fundamentally change what the RTC is. Agree. This is a device sitting on the ISA bus on a PC, but it can sit on other buses too. And it happily does so on other architectures. It doesn't sit on the ISA bus. A SuperIO chip sits on the ISA bus and the SuperIO chip decodes the ISA bus traffic and dispatches approriately. On the PIIX3, the SuperIO chip is part of the chipset. An ISA transaction isn't necessary to talk to the RTC--it would simply be too expensive to do this because of subtractive decoding. You are talking about internal chip implementation. I am talking about functionality. Qemu also doesn't exactly has ISA bus inside. The problem with what you describe is that there are far fewer devices that actually sit on busses than what QEMU tries to model today. All devices sit on some buses. That's simply not true. Let's be specific about what a bus is. A bus is a set of lines that is shared by multiple devices with some mechanism to arbitrate who's using the bus at any given time. In PCI, roughly speaking, a transaction is sent from the controller to all devices on the bus and the device that is to handle the transaction will raise it's DEVSEL line to indicate that it's handling it. But there are plenty of cases where a device is hard wired to another device. In a simple microcontroller, it's extremely common to tie a set of GPIO pins to a specific device. There's no bus here. Agree. We could call thous GPIO pins ad-hoc bus too. But I see you point. The i8042 is another good example. The i8042 has two ps2 plugs but it's not a bus. The keyboard port takes a keyboard and the aux port takes a mouse. You can't hook up two keyboards and you can't hook up two mice. Because I don't think we can implement a reasonable device model using a garbage collected language. Garbage collection introduces non-determinism and in QEMU we need to ensure that when we're running in a VCPU context, we exit back to the guest as quickly as possible. IIRC there are garbage collected languages that allow you to disable garbage collection for certain part of the code. Yeah, but very few languages that I know of are sophisticated here. Garbage collection can be done while vcpu executes guest code and in proper implementation vcpu thread should execute device model code for a very brief time and
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/24/2010 07:27 PM, Anthony Liguori wrote:
The alternative in C is:
struct file_operations {
void (*release)(void);
void (*read)(...);
void (*write)(...);
};
struct file {
struct file_operations *ops;
};
If I do:
static file_operations posix_file_ops = {
.read = posix_file_read,
.write = posix_file_write,
};
struct posix_file {
struct file parent;
int fd;
};
void posix_file_init(struct posix_file *obj)
{
obj-parent.ops = posix_file_ops;
obj-fd = ...;
};
The compiler won't generate an error. Only upon a call to
file_release() will a null pointer dereference happens whereas in C++,
because this paradigm is structured in the language, the compiler can
help assist you.
You forgot to mention:
static void posix_file_read(struct file *_file, ...)
{
struct posix_file *file = container_of(_file, struct posix_file,
parent);
...
}
Even more boilerplate and potential for error. But I think the worst
thing about this is that it makes it hard to do the right thing, so
people hack quick and dirty workarounds. C++ makes this pattern trivial
(if : laden), so people are encouraged to use it.
--
error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On Wed, Nov 24, 2010 at 07:25:08PM +0200, Avi Kivity wrote: On 11/24/2010 07:17 PM, Jes Sorensen wrote: Anyone wishing to implement this interface is forced to implement read and write methods (callbacks) with exactly the right signature. The compiler will complain if they don't. So if File is a good interface, we can make the compiler force people to use it correctly. We can emulate this in C with -ops- things, but that's just boilerplate and more places for people to get things wrong, or lazy and take shortcuts. In the mean time we spend our time debugging the runtime because the virtual functions don't behave as expected. In C we know what is going on, in C++ it is pray and hope. That is pure bullshit. All major browsers are written in C++, all major office suites, one leading free desktop, google, countless other projects. There is a lot more C++ code in the world than C code. If virtual functions didn't behave as expected, surely we'd hear by now. Google don't use exceptions though :) They claim it is hard to integrate with legacy code if exceptions are used. Our case BTW. -- Gleb. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/24/10 18:27, Anthony Liguori wrote: The compiler won't generate an error. Only upon a call to file_release() will a null pointer dereference happens whereas in C++, because this paradigm is structured in the language, the compiler can help assist you. Explicit code means you know what is going on, it means you can debug it in gdb and match it. That is a *good* thing! Jes -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/24/2010 07:33 PM, Gleb Natapov wrote: But I don't see the point. If you look at my repository, there's The point is that C++ is ugly language. The short code Avi sent remind me perl (aka line noise). It is almost impossible to parse it into what code it actually does. Most symbols are there for C++ syntax not functionality. No. They're there for error handling. A C++ wrapper, doesn't add any functionality, so you can say that all of the lines of codes do nothing and are just syntax. But they do allow you to pair init and uninit (in the constructor and destructor). When you use the wrapper (as opposed to the bare C interface) you get the value by not having to code long error handling sequences (with a high probability of getting them wrong and never finding out in testing). -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On Wed, Nov 24, 2010 at 07:39:39PM +0200, Avi Kivity wrote: On 11/24/2010 07:33 PM, Gleb Natapov wrote: But I don't see the point. If you look at my repository, there's The point is that C++ is ugly language. The short code Avi sent remind me perl (aka line noise). It is almost impossible to parse it into what code it actually does. Most symbols are there for C++ syntax not functionality. No. They're there for error handling. A C++ wrapper, doesn't add any functionality, so you can say that all of the lines of codes do nothing and are just syntax. But they do allow you to pair init and uninit (in the constructor and destructor). When you use the wrapper (as opposed to the bare C interface) you get the value by not having to code long error handling sequences (with a high probability of getting them wrong and never finding out in testing). So how errors are handled there? By throwing exceptions? Sorry this is not error handling. -- Gleb. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/24/2010 07:36 PM, Jes Sorensen wrote: On 11/24/10 18:27, Anthony Liguori wrote: The compiler won't generate an error. Only upon a call to file_release() will a null pointer dereference happens whereas in C++, because this paradigm is structured in the language, the compiler can help assist you. Explicit code means you know what is going on, it means you can debug it in gdb and match it. That is a *good* thing! Implicit code means that you don't need to debug it. The compiler gets it right every time. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On Wed, Nov 24, 2010 at 07:41:48PM +0200, Avi Kivity wrote: On 11/24/2010 07:36 PM, Jes Sorensen wrote: On 11/24/10 18:27, Anthony Liguori wrote: The compiler won't generate an error. Only upon a call to file_release() will a null pointer dereference happens whereas in C++, because this paradigm is structured in the language, the compiler can help assist you. Explicit code means you know what is going on, it means you can debug it in gdb and match it. That is a *good* thing! Implicit code means that you don't need to debug it. The compiler gets it right every time. Best joke ever! -- Gleb. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/24/10 18:41, Avi Kivity wrote: On 11/24/2010 07:36 PM, Jes Sorensen wrote: On 11/24/10 18:27, Anthony Liguori wrote: The compiler won't generate an error. Only upon a call to file_release() will a null pointer dereference happens whereas in C++, because this paradigm is structured in the language, the compiler can help assist you. Explicit code means you know what is going on, it means you can debug it in gdb and match it. That is a *good* thing! Implicit code means that you don't need to debug it. The compiler gets it right every time. Except when it doesn't, or when you don't know why something happened and you ended up where you did Jes -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/24/2010 11:36 AM, Jes Sorensen wrote: On 11/24/10 18:27, Anthony Liguori wrote: The compiler won't generate an error. Only upon a call to file_release() will a null pointer dereference happens whereas in C++, because this paradigm is structured in the language, the compiler can help assist you. Explicit code means you know what is going on, it means you can debug it in gdb and match it. That is a *good* thing! So I take it you avoid switch statements, while, or for loops in your C code. Regards, Anthony Liguori Jes -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs
On 11/24/10 18:43, Anthony Liguori wrote: On 11/24/2010 11:36 AM, Jes Sorensen wrote: On 11/24/10 18:27, Anthony Liguori wrote: The compiler won't generate an error. Only upon a call to file_release() will a null pointer dereference happens whereas in C++, because this paradigm is structured in the language, the compiler can help assist you. Explicit code means you know what is going on, it means you can debug it in gdb and match it. That is a *good* thing! So I take it you avoid switch statements, while, or for loops in your C code. Those map easily to assembly code and gdb. Jes -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
