[PATCH kvm-unit-tests 0/4] API test framework

[PATCH kvm-unit-tests 0/4] API test framework

[Avi Kivity]

This patchset adds an API test framework.  Rather than driving kvm from qemu,
we now have a way of calling the kvm API directly and observing the results.
We can switch to guest mode and back at will and see any micro effects such
as the result of executing particular instructions.

As an experiment, the framework is coded in C++.

Avi Kivity (4):
  Makefile: add support for C++
  Introduce a C++ wrapper for the kvm APIs
  Add support for calling a function in guest mode
  Add sample test using the api test harness

 Makefile              |    4 +-
 api/identity.cc       |   76 ++++++++++++++++++++++
 api/identity.h        |   28 ++++++++
 api/kvmxx.cc          |  168 +++++++++++++++++++++++++++++++++++++++++++++++++
 api/kvmxx.h           |   80 +++++++++++++++++++++++
 config-x86-common.mak |    8 +++
 x86/api-sample.cc     |   23 +++++++
 7 files changed, 386 insertions(+), 1 deletions(-)
 create mode 100644 api/identity.cc
 create mode 100644 api/identity.h
 create mode 100644 api/kvmxx.cc
 create mode 100644 api/kvmxx.h
 create mode 100644 x86/api-sample.cc

[PATCH kvm-unit-tests 1/4] Makefile: add support for C++

[Avi Kivity]

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 Makefile |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/Makefile b/Makefile
index d25e6f2..9b0256d 100644
--- a/Makefile
+++ b/Makefile
@@ -30,11 +30,13 @@ CFLAGS += -O1
 CFLAGS += $(autodepend-flags) -g -fomit-frame-pointer -Wall
 CFLAGS += $(call cc-option, -fno-stack-protector, "")
 CFLAGS += $(call cc-option, -fno-stack-protector-all, "")
+CFLAGS += -I.
 
-CXXFLAGS = $(autodepend-flags)
+CXXFLAGS += $(CFLAGS)
 
 autodepend-flags = -MMD -MF $(dir $*).$(notdir $*).d
 
+LDFLAGS += $(CFLAGS)
 LDFLAGS += -pthread -lrt
 
 kvmtrace_objs= kvmtrace.o
-- 
1.7.1

[PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Avi Kivity]

Introduce exception-safe objects for calling system, vm, and vcpu ioctls.

Signed-off-by: Avi Kivity <avi@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 0000000..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_cast<kvm_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_cast<kvm_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_cast<kvm_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_cast<kvm_msrs*>(malloc(size));
+    if (!ret) {
+	throw ENOMEM;
+    }
+    return ret;
+}
+
+std::vector<kvm_msr_entry> vcpu::msrs(std::vector<uint32_t> indices)
+{
+    std::auto_ptr<kvm_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::vector<kvm_msr_entry>(msrs->entries,
+				      msrs->entries + msrs->nmsrs);
+}
+
+void vcpu::set_msrs(const std::vector<kvm_msr_entry>& msrs)
+{
+    std::auto_ptr<kvm_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_cast<uint64_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 0000000..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 vcpu;
+class fd;
+
+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_cast<long>(arg));
+    }
+private:
+    int _fd;
+};
+
+class vcpu {
+public:
+    vcpu(vm& vm, int fd);
+    ~vcpu();
+    void run();
+    kvm_run *shared();
+    kvm_regs regs();
+    void set_regs(const kvm_regs& regs);
+    kvm_sregs sregs();
+    void set_sregs(const kvm_sregs& sregs);
+    std::vector<kvm_msr_entry> msrs(std::vector<uint32_t> indices);
+    void set_msrs(const std::vector<kvm_msr_entry>& msrs);
+    void set_debug(uint64_t dr[8], bool enabled, bool singlestep);
+private:
+    static kvm_msrs* alloc_msr_list(size_t nmsrs);
+private:
+    vm& _vm;
+    fd _fd;
+    kvm_run *_shared;
+    friend class vm;
+};
+
+class vm {
+public:
+    explicit vm(system& system);
+    void set_memory_region(int slot, void *addr, uint64_t gpa, size_t len);
+    void set_tss_addr(uint32_t addr);
+private:
+    system& _system;
+    fd _fd;
+    friend class system;
+    friend class vcpu;
+};
+
+class system {
+public:
+    explicit system(std::string device_node = "/dev/kvm");
+    bool check_extension(int extension);
+private:
+    fd _fd;
+    friend class vcpu;
+    friend class vm;
+};
+
+};
+
+#endif
-- 
1.7.1

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Alexander Graf]

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 <avi@redhat.com>


FWIW, I still disagree with C++ and believe this code to be hardly readable.


Alex

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Gleb Natapov]

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 <avi@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.

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Anthony Liguori]

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 Kivity<avi@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 majordomo@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

[Anthony Liguori]

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 Kivity<avi@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 majordomo@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

[Avi Kivity]

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 Kivity<avi@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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Avi Kivity]

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 Kivity<avi@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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Anthony Liguori]

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 Kivity<avi@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:
    Plug<PCIDevice *> 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:

Array<PITChannel, 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 majordomo@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

[Gleb Natapov]

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 Kivity<avi@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:
>    Plug<PCIDevice *> 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:
> 
> Array<PITChannel, 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 majordomo@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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
			Gleb.

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Anthony Liguori]

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 Kivity<avi@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:
>>     Plug<PCIDevice *>  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:
>>
>> Array<PITChannel, 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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Avi Kivity]

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

Re: Introduce a C++ wrapper for the kvm APIs

[Mike Day]

On 24/11/10 18:10 +0200, Avi Kivity wrote:
> On 11/24/2010 05:50 PM, Anthony Liguori wrote:

> 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.

Objective C is good (I like it better than C++) if you can live with
the message-passing architecture. 

Mike

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Gleb Natapov]

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 Kivity<avi@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:
> >>    Plug<PCIDevice *>  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:
> >>
> >>Array<PITChannel, 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 disable garbage
collection for certain part of the code. 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.
 
--
			Gleb.

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Avi Kivity]

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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Gleb Natapov]

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.

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Avi Kivity]

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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Gleb Natapov]

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.

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Avi Kivity]

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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Gleb Natapov]

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.

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Avi Kivity]

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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Gleb Natapov]

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.

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Avi Kivity]

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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Jes Sorensen]

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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Avi Kivity]

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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Anthony Liguori]

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:
    Plug<I8042> 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 Plug<Type> 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.
>

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Anthony Liguori]

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.
>    

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Gleb Natapov]

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.

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Anthony Liguori]

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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Gleb Natapov]

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.

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Anthony Liguori]

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.
>    

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Gleb Natapov]

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 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
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.

> 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.
I am not sure you can write useful program with few memory allocations.

> 
> 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.
> 
--
			Gleb.

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Avi Kivity]

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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Gleb Natapov]

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.

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Avi Kivity]

On 11/24/2010 07:41 PM, Gleb Natapov wrote:
> 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".

It's not in this code, but in it's callers.

   kvm::system sys;
   kvm::vm vm(sys);
   kvm::vcpu vcpu(vm, 0);
   mutex::guard guard(some_mutex);
   some_other_object blah;
   return do_something();

If an exception happens in any of these places, whatever's already 
constructed is rolled back and destroyed.  Contrast with

   struct kvm_system sys;
   struct kvm_vm vm;
   struct kvm_vcpu vcpu;
   struct some_other_object object;
   int r;

   r = kvm_system_init(&sys);
   if (r < 0)
       goto out_1;
   r = kvm_vm_init(&vm, &sys);
   if (r < 0)
       goto out_2;
   r = kvm_vcpu_init(&vcpu, &vm, 0);
   if (r < 0)
       goto out_3;
   mutex_aquire(&some_mutex);
   r = some_other_object_init(&object);
   if (r < 0)
       goto out_4;
   r = do_something();
   some_other_object_destroy(&object);
out_4:
   mutex_release(&mutex);
out_3:
   kvm_vcpu_destroy(&vcpu);
out_2:
   kvm_vm_destroy(&vm);
out_1:
   kvm_system_destroy(&sys);
   return r;

Which, in your opinion, is more readable?

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Gleb Natapov]

On Wed, Nov 24, 2010 at 07:50:06PM +0200, Avi Kivity wrote:
> On 11/24/2010 07:41 PM, Gleb Natapov wrote:
> >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".
> 
> It's not in this code, but in it's callers.
> 
>   kvm::system sys;
>   kvm::vm vm(sys);
>   kvm::vcpu vcpu(vm, 0);
>   mutex::guard guard(some_mutex);
>   some_other_object blah;
>   return do_something();
> 
> If an exception happens in any of these places, whatever's already
> constructed is rolled back and destroyed.  Contrast with
> 
>   struct kvm_system sys;
>   struct kvm_vm vm;
>   struct kvm_vcpu vcpu;
>   struct some_other_object object;
>   int r;
> 
>   r = kvm_system_init(&sys);
>   if (r < 0)
>       goto out_1;
>   r = kvm_vm_init(&vm, &sys);
>   if (r < 0)
>       goto out_2;
>   r = kvm_vcpu_init(&vcpu, &vm, 0);
>   if (r < 0)
>       goto out_3;
>   mutex_aquire(&some_mutex);
>   r = some_other_object_init(&object);
>   if (r < 0)
>       goto out_4;
>   r = do_something();
>   some_other_object_destroy(&object);
> out_4:
>   mutex_release(&mutex);
> out_3:
>   kvm_vcpu_destroy(&vcpu);
> out_2:
>   kvm_vm_destroy(&vm);
> out_1:
>   kvm_system_destroy(&sys);
>   return r;
> 
> Which, in your opinion, is more readable?
> 
It is not realistic example since you create objects on the stack which
in real code will be on a heap. And in your short C++ example
mutex::guard guard(some_mutex); part is ugly as hell. "Protect code from
here to end of the scope"?! Later somebody will add code at the end of
the function that will run under mutex needlessly. Proper
language allow you to protect scope with mutex like that:
 with(some_mutex) {
	some_other_object blah;
 }

--
			Gleb.

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Avi Kivity]

On 11/24/2010 08:23 PM, Gleb Natapov wrote:
> On Wed, Nov 24, 2010 at 07:50:06PM +0200, Avi Kivity wrote:
> >  On 11/24/2010 07:41 PM, Gleb Natapov wrote:
> >  >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".
> >
> >  It's not in this code, but in it's callers.
> >
> >    kvm::system sys;
> >    kvm::vm vm(sys);
> >    kvm::vcpu vcpu(vm, 0);
> >    mutex::guard guard(some_mutex);
> >    some_other_object blah;
> >    return do_something();
> >
> >  If an exception happens in any of these places, whatever's already
> >  constructed is rolled back and destroyed.  Contrast with
> >
> >    struct kvm_system sys;
> >    struct kvm_vm vm;
> >    struct kvm_vcpu vcpu;
> >    struct some_other_object object;
> >    int r;
> >
> >    r = kvm_system_init(&sys);
> >    if (r<  0)
> >        goto out_1;
> >    r = kvm_vm_init(&vm,&sys);
> >    if (r<  0)
> >        goto out_2;
> >    r = kvm_vcpu_init(&vcpu,&vm, 0);
> >    if (r<  0)
> >        goto out_3;
> >    mutex_aquire(&some_mutex);
> >    r = some_other_object_init(&object);
> >    if (r<  0)
> >        goto out_4;
> >    r = do_something();
> >    some_other_object_destroy(&object);
> >  out_4:
> >    mutex_release(&mutex);
> >  out_3:
> >    kvm_vcpu_destroy(&vcpu);
> >  out_2:
> >    kvm_vm_destroy(&vm);
> >  out_1:
> >    kvm_system_destroy(&sys);
> >    return r;
> >
> >  Which, in your opinion, is more readable?
> >
> It is not realistic example since you create objects on the stack which
> in real code will be on a heap.

Those are still the same issues and are solved in the same way.  Linux 
is riddled with these patterns and there are always patches in flight to 
fix error handling paths.

In fact with heap based objects C++ is even more compelling since you 
can do automatic reference counting.

>   And in your short C++ example
> mutex::guard guard(some_mutex); part is ugly as hell. "Protect code from
> here to end of the scope"?! Later somebody will add code at the end of
> the function that will run under mutex needlessly. Proper
> language allow you to protect scope with mutex like that:
>   with(some_mutex) {
> 	some_other_object blah;
>   }

I agree.  You can always introduce a new block scope though.

I prefer the ugliness to having to roll back the stack manually.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Anthony Liguori]

On 11/24/2010 11:33 AM, Gleb Natapov wrote:
> 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.
>    

Except the Mc16818a device is not a qdev device.

>> 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!"
>    

Implementing something correctly in the way that hardware does is not a 
short cut.

>> 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.

>>>   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
>>      
> 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.
>    

I'm using C++ to understand how to make a correct design model.  The C++ 
part is only 10% of what I care about.

I agree that Avi's example is not a strong justification for C++ and I 
also agree that it's uglier than the same implementation in C.

>> 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.
>>      
> I am not sure you can write useful program with few memory allocations.
>    

Ultimately, you're allocating the same amount of memory, but in fewer 
steps (because of static composition).

Regards,

Anthony Liguori

>> 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.
>>
>>      
> --
> 			Gleb.
>    

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Gleb Natapov]

On Wed, Nov 24, 2010 at 12:17:00PM -0600, Anthony Liguori wrote:
> On 11/24/2010 11:33 AM, Gleb Natapov wrote:
> >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.
> 
> Except the Mc16818a device is not a qdev device.
> 
There are other examples.

> >>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!"
> 
> Implementing something correctly in the way that hardware does is
> not a short cut.
> 
I think we disagree on what HW does.

> >>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.

> 
> >>>  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
> >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.
> 
> I'm using C++ to understand how to make a correct design model.  The
> C++ part is only 10% of what I care about.
> 
> I agree that Avi's example is not a strong justification for C++ and
> I also agree that it's uglier than the same implementation in C.
> 
I am glad we agree on that :)

> >>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.
> >I am not sure you can write useful program with few memory allocations.
> 
> Ultimately, you're allocating the same amount of memory, but in
> fewer steps (because of static composition).
> 
> Regards,
> 
> Anthony Liguori
> 
> >>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.
> >>
> >--
> >			Gleb.

--
			Gleb.

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Anthony Liguori]

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'm using C++ to understand how to make a correct design model.  The
>> C++ part is only 10% of what I care about.
>>
>> I agree that Avi's example is not a strong justification for C++ and
>> I also agree that it's uglier than the same implementation in C.
>>
>>      
> I am glad we agree on that :)
>    

Regards,

Anthony Liguori

>>>> 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.
>>>>          
>>> I am not sure you can write useful program with few memory allocations.
>>>        
>> Ultimately, you're allocating the same amount of memory, but in
>> fewer steps (because of static composition).
>>
>> Regards,
>>
>> Anthony Liguori
>>
>>      
>>>> 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.
>>>>
>>>>          
>>> --
>>> 			Gleb.
>>>        
> --
> 			Gleb.
>    

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Gleb Natapov]

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.

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Jes Sorensen]

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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Avi Kivity]

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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Jes Sorensen]

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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Anthony Liguori]

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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Avi Kivity]

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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Avi Kivity]

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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Jes Sorensen]

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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Avi Kivity]

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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Jes Sorensen]

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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Avi Kivity]

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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Jes Sorensen]

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 soooo well :(

Jes

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Avi Kivity]

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 soooo well :(

Let's see the C alternatives.  Anyone for elinks?

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Gleb Natapov]

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.

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Avi Kivity]

On 11/24/2010 07:36 PM, Gleb Natapov wrote:
> >  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.

It's possible, but very nasty.  You add wrappers that convert error 
codes to exceptions and vice versa.  If your goal is to eventually 
convert the entire code base, those wrappers eventually disappear.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Anthony Liguori]

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
>    

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Avi Kivity]

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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Jes Sorensen]

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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Avi Kivity]

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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Gleb Natapov]

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.

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Avi Kivity]

On 11/24/2010 07:43 PM, Gleb Natapov wrote:
> >  >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!

Do you encounter many compiler bugs in your daily work?

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Gleb Natapov]

On Wed, Nov 24, 2010 at 07:50:44PM +0200, Avi Kivity wrote:
> On 11/24/2010 07:43 PM, Gleb Natapov wrote:
> >>  >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!
> 
> Do you encounter many compiler bugs in your daily work?
> 
Bug can still be yours but all this virtual inheritance make it much
harder to understand what happens. How is support for C++ debugging in
gdb this days? Can gdb tell type of object from object instance?

--
			Gleb.

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Avi Kivity]

On 11/24/2010 08:10 PM, Gleb Natapov wrote:
> On Wed, Nov 24, 2010 at 07:50:44PM +0200, Avi Kivity wrote:
> >  On 11/24/2010 07:43 PM, Gleb Natapov wrote:
> >  >>   >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!
> >
> >  Do you encounter many compiler bugs in your daily work?
> >
> Bug can still be yours but all this virtual inheritance make it much
> harder to understand what happens.

No, it's easier, since there's a lot less code to chase.

Of course, you have to learn the pattern in the language, but you do 
this once and keep reusing this.  If it's emulated in C, you have to 
read all the code over and over again (in different uses), always on the 
lookout to see if it's exactly the same pattern, or if some bug crept in.

> How is support for C++ debugging in
> gdb this days?

Last I debugged C++ was 5+ years ago.  I had no complaints (though I'm a 
mostly printf debugger).

> Can gdb tell type of object from object instance?

Yes, provided it has at least one virtual function.  Otherwise you only 
get the static type.  In fact this is much better than C where you have 
to look up the ops table and perform the container_of() manually.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Jes Sorensen]

On 11/24/10 19:55, Avi Kivity wrote:
> On 11/24/2010 08:10 PM, Gleb Natapov wrote:
>> On Wed, Nov 24, 2010 at 07:50:44PM +0200, Avi Kivity wrote:
>> >  On 11/24/2010 07:43 PM, Gleb Natapov wrote:
>> >  >>   Implicit code means that you don't need to debug it.  The
>> compiler
>> >  >>   gets it right every time.
>> >  >>
>> >  >Best joke ever!
>> >
>> >  Do you encounter many compiler bugs in your daily work?
>> >
>> Bug can still be yours but all this virtual inheritance make it much
>> harder to understand what happens.
> 
> No, it's easier, since there's a lot less code to chase.
> 
> Of course, you have to learn the pattern in the language, but you do
> this once and keep reusing this.  If it's emulated in C, you have to
> read all the code over and over again (in different uses), always on the
> lookout to see if it's exactly the same pattern, or if some bug crept in.

How does it get easier when you have to debug templated code? It is much
harder to read the assembly and match it to the code, so no it becomes
much harder to debug.

Jes

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Avi Kivity]

On 11/24/2010 09:29 PM, Jes Sorensen wrote:
> >>  >
> >>  Bug can still be yours but all this virtual inheritance make it much
> >>  harder to understand what happens.
> >
> >  No, it's easier, since there's a lot less code to chase.
> >
> >  Of course, you have to learn the pattern in the language, but you do
> >  this once and keep reusing this.  If it's emulated in C, you have to
> >  read all the code over and over again (in different uses), always on the
> >  lookout to see if it's exactly the same pattern, or if some bug crept in.
>
> How does it get easier when you have to debug templated code? It is much
> harder to read the assembly and match it to the code, so no it becomes
> much harder to debug.

Oh yes, looking at compiled template heavy code is hard, especially with 
aggressive inlining (quite common in template code).  But what are the C 
alternatives? forests of #defines, which are even harder, or abstracting 
via callbacks, which isn't what you want, or simply not doing it.

Try coding something like tr1::unordered_hash<> in C.  A simple 
resizeable hash table that doesn't use callbacks for everything.  You 
can't do it.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Jes Sorensen]

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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Avi Kivity]

On 11/24/2010 07:43 PM, Jes Sorensen wrote:
> 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,

Which is very rare, at least compared to my bug rate.

>   or when you don't know why something happened
> and you ended up where you did....

There's always a disassembler.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Anthony Liguori]

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
>
>    

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Jes Sorensen]

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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Avi Kivity]

On 11/24/2010 07:45 PM, Jes Sorensen wrote:
> >
> >  So I take it you avoid switch statements, while, or for loops in your C
> >  code.
>
> Those map easily to assembly code and gdb.
>

So do virtual functions.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Anthony Liguori]

On 11/24/2010 11:45 AM, Jes Sorensen wrote:
> 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.
>    

The mapping is definitely not simply especially after a couple rounds of 
loop unrolling and code motion.

Regards,

Anthony Liguori

> Jes
>
>
>    

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Avi Kivity]

On 11/24/2010 08:01 PM, Anthony Liguori wrote:
>>> So I take it you avoid switch statements, while, or for loops in your C
>>> code.
>> Those map easily to assembly code and gdb.
>
> The mapping is definitely not simply especially after a couple rounds 
> of loop unrolling and code motion.

You're discounting dead code elimination, which can simplify things 
considerably.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Gleb Natapov]

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.

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Avi Kivity]

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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Gleb Natapov]

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.

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Avi Kivity]

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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Anthony Liguori]

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
>    

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Jes Sorensen]

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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Michael S. Tsirkin]

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 Kivity<avi@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:
>    Plug<PCIDevice *> 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:
> 
> Array<PITChannel, 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 majordomo@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 majordomo@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

[Anthony Liguori]

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 Kivity<avi@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:
>>     Plug<PCIDevice *>  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:
>>
>> Array<PITChannel, 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 changes the approach people take to using arrays.  If it simply 
threw away invalid accesses, I might buy your argument.  But anyone that 
catches an out of bound exception as a normal error handling mechanism 
deserves to get beaten with a three day old trout.

>> 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.
>    

grep 'vmstate(.*, int16_t' *.hpp

Works perfectly fine.

Regards,

Anthony Liguori

>    
>>> Alex
>>>
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe kvm" in
>>> the body of a message to majordomo@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 majordomo@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

[Michael S. Tsirkin]

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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Anthony Liguori]

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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Michael S. Tsirkin]

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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Anthony Liguori]

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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Avi Kivity]

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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Jes Sorensen]

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 <avi@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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Avi Kivity]

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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Jes Sorensen]

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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Avi Kivity]

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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Michael S. Tsirkin]

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 <avi@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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Avi Kivity]

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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Michael S. Tsirkin]

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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Avi Kivity]

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_ptr<T>
   {
   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_ptr<T> 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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Michael S. Tsirkin]

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_ptr<T>
>   {
>   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_ptr<T> 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 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.

> >>  >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.

We seem to have trouble enough making the style uniform with C.

> -- 
> error compiling committee.c: too many arguments to function

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Avi Kivity]

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:
> >
> >    template<class T>
> >    class user_ptr<T>
> >    {
> >    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_ptr<T>  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 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.

I guess this is subjective.  If you use good tools this shouldn't happen 
though.

> >  >>    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.

Do a 'git grep hash' for examples of duplication.


-- 
error compiling committee.c: too many arguments to function

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Michael S. Tsirkin]

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_ptr<T>  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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Avi Kivity]

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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Michael S. Tsirkin]

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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Anthony Liguori]

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 majordomo@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

[Anthony Liguori]

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 Kivity<avi@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 0000000..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;
> +}
>    

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_cast<kvm_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_cast<kvm_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_cast<kvm_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_cast<kvm_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::vector<kvm_msr_entry>  vcpu::msrs(std::vector<uint32_t>  indices)
> +{
> +    std::auto_ptr<kvm_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::vector<kvm_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::vector<kvm_msr_entry>&  msrs)
> +{
> +    std::auto_ptr<kvm_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_cast<uint64_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 0000000..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 vcpu;
> +class fd;
> +
> +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_cast<long>(arg));
> +    }
>    

I think mixing inline definitions with declarations is bad form.

Regards,

Anthony Liguori

> +private:
> +    int _fd;
> +};
> +
> +class vcpu {
> +public:
> +    vcpu(vm&  vm, int fd);
> +    ~vcpu();
> +    void run();
> +    kvm_run *shared();
> +    kvm_regs regs();
> +    void set_regs(const kvm_regs&  regs);
> +    kvm_sregs sregs();
> +    void set_sregs(const kvm_sregs&  sregs);
> +    std::vector<kvm_msr_entry>  msrs(std::vector<uint32_t>  indices);
> +    void set_msrs(const std::vector<kvm_msr_entry>&  msrs);
> +    void set_debug(uint64_t dr[8], bool enabled, bool singlestep);
> +private:
> +    static kvm_msrs* alloc_msr_list(size_t nmsrs);
> +private:
> +    vm&  _vm;
> +    fd _fd;
> +    kvm_run *_shared;
> +    friend class vm;
> +};
> +
> +class vm {
> +public:
> +    explicit vm(system&  system);
> +    void set_memory_region(int slot, void *addr, uint64_t gpa, size_t len);
> +    void set_tss_addr(uint32_t addr);
> +private:
> +    system&  _system;
> +    fd _fd;
> +    friend class system;
> +    friend class vcpu;
> +};
> +
> +class system {
> +public:
> +    explicit system(std::string device_node = "/dev/kvm");
> +    bool check_extension(int extension);
> +private:
> +    fd _fd;
> +    friend class vcpu;
> +    friend class vm;
> +};
> +
> +};
> +
> +#endif
>    

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Avi Kivity]

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_cast<kvm_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_cast<kvm_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::vector<kvm_msr_entry>  vcpu::msrs(std::vector<uint32_t>  indices)
>> +{
>> +    std::auto_ptr<kvm_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::vector<kvm_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_cast<long>(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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Anthony Liguori]

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_cast<kvm_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_cast<kvm_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::vector<kvm_msr_entry>  vcpu::msrs(std::vector<uint32_t>  indices)
>>> +{
>>> +    std::auto_ptr<kvm_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::vector<kvm_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_cast<long>(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.
>

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Avi Kivity]

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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Anthony Liguori]

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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Avi Kivity]

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_cast<kvm_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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Michael S. Tsirkin]

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 <avi@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 0000000..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_cast<kvm_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_cast<kvm_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_cast<kvm_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_cast<kvm_msrs*>(malloc(size));
> +    if (!ret) {
> +	throw ENOMEM;
> +    }
> +    return ret;
> +}
> +
> +std::vector<kvm_msr_entry> vcpu::msrs(std::vector<uint32_t> indices)
> +{
> +    std::auto_ptr<kvm_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::vector<kvm_msr_entry>(msrs->entries,
> +				      msrs->entries + msrs->nmsrs);
> +}
> +
> +void vcpu::set_msrs(const std::vector<kvm_msr_entry>& msrs)
> +{
> +    std::auto_ptr<kvm_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_cast<uint64_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 0000000..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 vcpu;
> +class fd;
> +
> +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_cast<long>(arg));
> +    }
> +private:
> +    int _fd;
> +};
> +
> +class vcpu {
> +public:
> +    vcpu(vm& vm, int fd);
> +    ~vcpu();
> +    void run();
> +    kvm_run *shared();
> +    kvm_regs regs();
> +    void set_regs(const kvm_regs& regs);
> +    kvm_sregs sregs();
> +    void set_sregs(const kvm_sregs& sregs);
> +    std::vector<kvm_msr_entry> msrs(std::vector<uint32_t> indices);
> +    void set_msrs(const std::vector<kvm_msr_entry>& msrs);
> +    void set_debug(uint64_t dr[8], bool enabled, bool singlestep);
> +private:
> +    static kvm_msrs* alloc_msr_list(size_t nmsrs);
> +private:
> +    vm& _vm;
> +    fd _fd;
> +    kvm_run *_shared;
> +    friend class vm;
> +};
> +
> +class vm {
> +public:
> +    explicit vm(system& system);
> +    void set_memory_region(int slot, void *addr, uint64_t gpa, size_t len);
> +    void set_tss_addr(uint32_t addr);
> +private:
> +    system& _system;
> +    fd _fd;
> +    friend class system;
> +    friend class vcpu;
> +};
> +
> +class system {
> +public:
> +    explicit system(std::string device_node = "/dev/kvm");
> +    bool check_extension(int extension);
> +private:
> +    fd _fd;
> +    friend class vcpu;
> +    friend class vm;
> +};
> +
> +};
> +
> +#endif
> -- 
> 1.7.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@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

[Michael S. Tsirkin]

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 Kivity<avi@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_cast<long>(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::vector<kvm_msr_entry>  vcpu::msrs(std::vector<uint32_t>  indices)
> >>  +{
> >>  +    std::auto_ptr<kvm_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.

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Avi Kivity]

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 Kivity<avi@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_cast<long>(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::vector<kvm_msr_entry>   vcpu::msrs(std::vector<uint32_t>   indices)
> >  >>   +{
> >  >>   +    std::auto_ptr<kvm_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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Michael S. Tsirkin]

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 Kivity<avi@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_cast<long>(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::vector<kvm_msr_entry>   vcpu::msrs(std::vector<uint32_t>   indices)
> >>  >>   +{
> >>  >>   +    std::auto_ptr<kvm_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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Avi Kivity]

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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Michael S. Tsirkin]

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.

------------------------------
[mst@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;
}
[mst@tuck ~]$ gcc -c -Os test.c 
[mst@tuck ~]$ size test.o 
   text    data     bss     dec     hex filename
     97       0       0      97      61 test.o
[mst@tuck ~]$ gcc -Os test.c
[mst@tuck ~]$ size a.out 
   text    data     bss     dec     hex filename
   1192     260       8    1460     5b4 a.out
[mst@tuck ~]$ wc -l test.c
22 test.c
------------------------------
[mst@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;
}
[mst@tuck ~]$ g++ -c -Os kvmxx.cpp 
[mst@tuck ~]$ size kvmxx.o 
   text    data     bss     dec     hex filename
    529       0       0     529     211 kvmxx.o
[mst@tuck ~]$ g++ -Os kvmxx.cpp 
[mst@tuck ~]$ size a.out 
   text    data     bss     dec     hex filename
   2254     308      16    2578     a12 a.out
[mst@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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Avi Kivity]

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.
>
> ------------------------------
> [mst@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;
> }


This code is not reusable.  Everywhere you use an fd, you have to repeat 
this code.

> [mst@tuck ~]$ gcc -c -Os test.c
> [mst@tuck ~]$ size test.o
>     text    data     bss     dec     hex filename
>       97       0       0      97      61 test.o
> [mst@tuck ~]$ gcc -Os test.c
> [mst@tuck ~]$ size a.out
>     text    data     bss     dec     hex filename
>     1192     260       8    1460     5b4 a.out
> [mst@tuck ~]$ wc -l test.c
> 22 test.c
> ------------------------------
> [mst@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;
> }

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).

> [mst@tuck ~]$ g++ -c -Os kvmxx.cpp
> [mst@tuck ~]$ size kvmxx.o
>     text    data     bss     dec     hex filename
>      529       0       0     529     211 kvmxx.o
> [mst@tuck ~]$ g++ -Os kvmxx.cpp
> [mst@tuck ~]$ size a.out
>     text    data     bss     dec     hex filename
>     2254     308      16    2578     a12 a.out
> [mst@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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Michael S. Tsirkin]

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.
> >
> >------------------------------
> >[mst@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;
> >}
> 
> 
> 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.


> >[mst@tuck ~]$ gcc -c -Os test.c
> >[mst@tuck ~]$ size test.o
> >    text    data     bss     dec     hex filename
> >      97       0       0      97      61 test.o
> >[mst@tuck ~]$ gcc -Os test.c
> >[mst@tuck ~]$ size a.out
> >    text    data     bss     dec     hex filename
> >    1192     260       8    1460     5b4 a.out
> >[mst@tuck ~]$ wc -l test.c
> >22 test.c
> >------------------------------
> >[mst@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;
> >}
> 
> 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.


> >[mst@tuck ~]$ g++ -c -Os kvmxx.cpp
> >[mst@tuck ~]$ size kvmxx.o
> >    text    data     bss     dec     hex filename
> >     529       0       0     529     211 kvmxx.o
> >[mst@tuck ~]$ g++ -Os kvmxx.cpp
> >[mst@tuck ~]$ size a.out
> >    text    data     bss     dec     hex filename
> >    2254     308      16    2578     a12 a.out
> >[mst@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.

I only wanted to see whether the compiler would optimize it away
completely as you said it should :).  Getting a convincing proof of
whether this matters would be much harder.

> >>  >>    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.

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 :).

> -- 
> error compiling committee.c: too many arguments to function

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Anthony Liguori]

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 majordomo@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

[Avi Kivity]

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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Michael S. Tsirkin]

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 <avi@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 0000000..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_cast<kvm_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_cast<kvm_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_cast<kvm_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_cast<kvm_msrs*>(malloc(size));
> +    if (!ret) {
> +	throw ENOMEM;
> +    }
> +    return ret;
> +}
> +
> +std::vector<kvm_msr_entry> vcpu::msrs(std::vector<uint32_t> indices)
> +{
> +    std::auto_ptr<kvm_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::vector<kvm_msr_entry>(msrs->entries,
> +				      msrs->entries + msrs->nmsrs);
> +}
> +
> +void vcpu::set_msrs(const std::vector<kvm_msr_entry>& msrs)
> +{
> +    std::auto_ptr<kvm_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_cast<uint64_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 0000000..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 vcpu;
> +class fd;
> +
> +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_cast<long>(arg));
> +    }
> +private:
> +    int _fd;
> +};
> +
> +class vcpu {
> +public:
> +    vcpu(vm& vm, int fd);
> +    ~vcpu();
> +    void run();
> +    kvm_run *shared();
> +    kvm_regs regs();
> +    void set_regs(const kvm_regs& regs);
> +    kvm_sregs sregs();
> +    void set_sregs(const kvm_sregs& sregs);
> +    std::vector<kvm_msr_entry> msrs(std::vector<uint32_t> indices);
> +    void set_msrs(const std::vector<kvm_msr_entry>& msrs);
> +    void set_debug(uint64_t dr[8], bool enabled, bool singlestep);
> +private:
> +    static kvm_msrs* alloc_msr_list(size_t nmsrs);
> +private:
> +    vm& _vm;
> +    fd _fd;
> +    kvm_run *_shared;
> +    friend class vm;
> +};
> +
> +class vm {
> +public:
> +    explicit vm(system& system);
> +    void set_memory_region(int slot, void *addr, uint64_t gpa, size_t len);
> +    void set_tss_addr(uint32_t addr);
> +private:
> +    system& _system;
> +    fd _fd;
> +    friend class system;
> +    friend class vcpu;
> +};
> +
> +class system {
> +public:
> +    explicit system(std::string device_node = "/dev/kvm");
> +    bool check_extension(int extension);
> +private:
> +    fd _fd;
> +    friend class vcpu;
> +    friend class vm;
> +};
> +
> +};
> +
> +#endif
> -- 
> 1.7.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@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

[Avi Kivity]

On 11/28/2010 01:49 PM, Michael S. Tsirkin wrote:
> >  +++ 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.

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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Michael S. Tsirkin]

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"
> >>  +#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.
> 
> 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

Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Avi Kivity]

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"
> >  >>   +#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.
> >
> >  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

[PATCH kvm-unit-tests 3/4] Add support for calling a function in guest mode

[Avi Kivity]

This patch provides a way to establish an "identity" guest which has
a 1:1 gva->hva translation.  This allows the host to switch to guest
mode, call a function in the same address space, and return.

Because long mode virtual addresses are 47 bits long, and some hosts
have smaller physical addresses, we target 32-bit mode only.  On
x86_64 the code needs to be run with 'setarch i386 -3' to limit the
address space to 3GB, so the address space occupied by the local
APIC is left unused.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 api/identity.cc       |   76 +++++++++++++++++++++++++++++++++++++++++++++++++
 api/identity.h        |   28 ++++++++++++++++++
 config-x86-common.mak |    2 +
 3 files changed, 106 insertions(+), 0 deletions(-)
 create mode 100644 api/identity.cc
 create mode 100644 api/identity.h

diff --git a/api/identity.cc b/api/identity.cc
new file mode 100644
index 0000000..8e86db1
--- /dev/null
+++ b/api/identity.cc
@@ -0,0 +1,76 @@
+
+#include "identity.h"
+#include <stdio.h>
+
+namespace identity {
+
+typedef unsigned long ulong;
+
+void setup_vm(kvm::vm& vm)
+{
+    vm.set_memory_region(0, NULL, 0, 3UL << 30);
+    vm.set_tss_addr(3UL << 30);
+}
+
+void vcpu::setup_sregs()
+{
+    kvm_sregs sregs = { };
+    kvm_segment dseg = { };
+    dseg.base = 0; dseg.limit = -1U; dseg.type = 3; dseg.present = 1;
+    dseg.dpl = 3; dseg.db = 1; dseg.s = 1; dseg.l = 0; dseg.g = 1;
+    kvm_segment cseg = dseg;
+    cseg.type = 11;
+
+    sregs.cs = cseg; asm ("mov %%cs, %0" : "=rm"(sregs.cs.selector));
+    sregs.ds = dseg; asm ("mov %%ds, %0" : "=rm"(sregs.ds.selector));
+    sregs.es = dseg; asm ("mov %%es, %0" : "=rm"(sregs.es.selector));
+    sregs.fs = dseg; asm ("mov %%fs, %0" : "=rm"(sregs.fs.selector));
+    sregs.gs = dseg; asm ("mov %%gs, %0" : "=rm"(sregs.gs.selector));
+    sregs.ss = dseg; asm ("mov %%ss, %0" : "=rm"(sregs.ss.selector));
+
+    uint32_t gsbase;
+    asm ("mov %%gs:0, %0" : "=r"(gsbase));
+    sregs.gs.base = gsbase;
+
+    sregs.tr.base = reinterpret_cast<ulong>(&*_stack.begin());
+    sregs.tr.type = 11;
+    sregs.tr.s = 0;
+    sregs.tr.present = 1;
+
+    sregs.cr0 = 0x11; /* PE, ET, !PG */
+    sregs.cr4 = 0;
+    sregs.efer = 0;
+    sregs.apic_base = 0xfee00000;
+    _vcpu.set_sregs(sregs);
+}
+
+void vcpu::thunk(vcpu* zis)
+{
+    zis->_guest_func();
+    asm volatile("outb %%al, %%dx" : : "a"(0), "d"(0));
+}
+
+void vcpu::setup_regs()
+{
+    kvm_regs regs = {};
+    regs.rflags = 0x3202;
+    regs.rsp = reinterpret_cast<ulong>(&*_stack.end());
+    regs.rsp &= ~15UL;
+    ulong* sp = reinterpret_cast<ulong *>(regs.rsp);
+    *--sp = reinterpret_cast<ulong>((char*)this);
+    *--sp = 0;
+    regs.rsp = reinterpret_cast<ulong>(sp);
+    regs.rip = reinterpret_cast<ulong>(&vcpu::thunk);
+    printf("rip %llx\n", regs.rip);
+    _vcpu.set_regs(regs);
+}
+
+vcpu::vcpu(kvm::vcpu& vcpu, boost::function<void ()> guest_func,
+           unsigned long stack_size)
+    : _vcpu(vcpu), _guest_func(guest_func), _stack(stack_size)
+{
+    setup_sregs();
+    setup_regs();
+}
+
+}
diff --git a/api/identity.h b/api/identity.h
new file mode 100644
index 0000000..025177a
--- /dev/null
+++ b/api/identity.h
@@ -0,0 +1,28 @@
+#ifndef API_IDENTITY_H
+#define API_IDENTITY_H
+
+#include "kvmxx.h"
+#include <boost/function.hpp>
+#include <vector>
+
+namespace identity {
+
+void setup_vm(kvm::vm& vm);
+
+class vcpu {
+public:
+    vcpu(kvm::vcpu& vcpu, boost::function<void ()> guest_func,
+	 unsigned long stack_size = 256 * 1024);
+private:
+    static void thunk(vcpu* vcpu);
+    void setup_regs();
+    void setup_sregs();
+private:
+    kvm::vcpu& _vcpu;
+    boost::function<void ()> _guest_func;
+    std::vector<char> _stack;
+};
+
+}
+
+#endif
diff --git a/config-x86-common.mak b/config-x86-common.mak
index b541c1c..0f3387b 100644
--- a/config-x86-common.mak
+++ b/config-x86-common.mak
@@ -79,3 +79,5 @@ arch_clean:
 	$(TEST_DIR)/.*.d $(TEST_DIR)/lib/.*.d $(TEST_DIR)/lib/*.o
 
 -include $(TEST_DIR)/.*.d lib/.*.d lib/x86/.*.d
+
+api/%.o: CFLAGS += -m32
\ No newline at end of file
-- 
1.7.1

Re: [PATCH kvm-unit-tests 3/4] Add support for calling a function in guest mode

[Michael S. Tsirkin]

On Wed, Nov 24, 2010 at 12:52:12PM +0200, Avi Kivity wrote:
> This patch provides a way to establish an "identity" guest which has
> a 1:1 gva->hva translation.  This allows the host to switch to guest
> mode, call a function in the same address space, and return.
> 
> Because long mode virtual addresses are 47 bits long, and some hosts
> have smaller physical addresses, we target 32-bit mode only.  On
> x86_64 the code needs to be run with 'setarch i386 -3' to limit the
> address space to 3GB, so the address space occupied by the local
> APIC is left unused.
> 
> Signed-off-by: Avi Kivity <avi@redhat.com>
> ---
>  api/identity.cc       |   76 +++++++++++++++++++++++++++++++++++++++++++++++++
>  api/identity.h        |   28 ++++++++++++++++++
>  config-x86-common.mak |    2 +
>  3 files changed, 106 insertions(+), 0 deletions(-)
>  create mode 100644 api/identity.cc
>  create mode 100644 api/identity.h
> 
> diff --git a/api/identity.cc b/api/identity.cc
> new file mode 100644
> index 0000000..8e86db1
> --- /dev/null
> +++ b/api/identity.cc
> @@ -0,0 +1,76 @@
> +
> +#include "identity.h"
> +#include <stdio.h>
> +
> +namespace identity {
> +
> +typedef unsigned long ulong;
> +
> +void setup_vm(kvm::vm& vm)
> +{
> +    vm.set_memory_region(0, NULL, 0, 3UL << 30);
> +    vm.set_tss_addr(3UL << 30);
> +}
> +
> +void vcpu::setup_sregs()
> +{
> +    kvm_sregs sregs = { };
> +    kvm_segment dseg = { };
> +    dseg.base = 0; dseg.limit = -1U; dseg.type = 3; dseg.present = 1;
> +    dseg.dpl = 3; dseg.db = 1; dseg.s = 1; dseg.l = 0; dseg.g = 1;
> +    kvm_segment cseg = dseg;
> +    cseg.type = 11;
> +
> +    sregs.cs = cseg; asm ("mov %%cs, %0" : "=rm"(sregs.cs.selector));
> +    sregs.ds = dseg; asm ("mov %%ds, %0" : "=rm"(sregs.ds.selector));
> +    sregs.es = dseg; asm ("mov %%es, %0" : "=rm"(sregs.es.selector));
> +    sregs.fs = dseg; asm ("mov %%fs, %0" : "=rm"(sregs.fs.selector));
> +    sregs.gs = dseg; asm ("mov %%gs, %0" : "=rm"(sregs.gs.selector));
> +    sregs.ss = dseg; asm ("mov %%ss, %0" : "=rm"(sregs.ss.selector));
> +
> +    uint32_t gsbase;
> +    asm ("mov %%gs:0, %0" : "=r"(gsbase));
> +    sregs.gs.base = gsbase;
> +
> +    sregs.tr.base = reinterpret_cast<ulong>(&*_stack.begin());
> +    sregs.tr.type = 11;
> +    sregs.tr.s = 0;
> +    sregs.tr.present = 1;
> +
> +    sregs.cr0 = 0x11; /* PE, ET, !PG */
> +    sregs.cr4 = 0;
> +    sregs.efer = 0;
> +    sregs.apic_base = 0xfee00000;
> +    _vcpu.set_sregs(sregs);
> +}
> +
> +void vcpu::thunk(vcpu* zis)
> +{
> +    zis->_guest_func();
> +    asm volatile("outb %%al, %%dx" : : "a"(0), "d"(0));
> +}
> +
> +void vcpu::setup_regs()
> +{
> +    kvm_regs regs = {};
> +    regs.rflags = 0x3202;
> +    regs.rsp = reinterpret_cast<ulong>(&*_stack.end());
> +    regs.rsp &= ~15UL;
> +    ulong* sp = reinterpret_cast<ulong *>(regs.rsp);
> +    *--sp = reinterpret_cast<ulong>((char*)this);
> +    *--sp = 0;
> +    regs.rsp = reinterpret_cast<ulong>(sp);
> +    regs.rip = reinterpret_cast<ulong>(&vcpu::thunk);
> +    printf("rip %llx\n", regs.rip);
> +    _vcpu.set_regs(regs);
> +}
> +
> +vcpu::vcpu(kvm::vcpu& vcpu, boost::function<void ()> guest_func,
> +           unsigned long stack_size)
> +    : _vcpu(vcpu), _guest_func(guest_func), _stack(stack_size)
> +{
> +    setup_sregs();
> +    setup_regs();
> +}
> +
> +}
> diff --git a/api/identity.h b/api/identity.h
> new file mode 100644
> index 0000000..025177a
> --- /dev/null
> +++ b/api/identity.h
> @@ -0,0 +1,28 @@
> +#ifndef API_IDENTITY_H
> +#define API_IDENTITY_H
> +
> +#include "kvmxx.h"
> +#include <boost/function.hpp>

This seems to use boost, which is not part of the standard library.
Do we want this dependency?
We'd need a configure check to verify it's installed.

> +#include <vector>
> +
> +namespace identity {
> +
> +void setup_vm(kvm::vm& vm);
> +
> +class vcpu {
> +public:
> +    vcpu(kvm::vcpu& vcpu, boost::function<void ()> guest_func,
> +	 unsigned long stack_size = 256 * 1024);

So the thread stack is moved to use the heap instead?
Can we use pthread_attr_getstacksize and use the regular thread stack?
Good for portability.

> +private:
> +    static void thunk(vcpu* vcpu);
> +    void setup_regs();
> +    void setup_sregs();
> +private:
> +    kvm::vcpu& _vcpu;
> +    boost::function<void ()> _guest_func;
> +    std::vector<char> _stack;
> +};
> +
> +}
> +
> +#endif
> diff --git a/config-x86-common.mak b/config-x86-common.mak
> index b541c1c..0f3387b 100644
> --- a/config-x86-common.mak
> +++ b/config-x86-common.mak
> @@ -79,3 +79,5 @@ arch_clean:
>  	$(TEST_DIR)/.*.d $(TEST_DIR)/lib/.*.d $(TEST_DIR)/lib/*.o
>  
>  -include $(TEST_DIR)/.*.d lib/.*.d lib/x86/.*.d
> +
> +api/%.o: CFLAGS += -m32
> \ No newline at end of file
> -- 
> 1.7.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH kvm-unit-tests 3/4] Add support for calling a function in guest mode

[Michael S. Tsirkin]

On Sat, Nov 27, 2010 at 11:15:18AM +0200, Avi Kivity wrote:
> On 11/26/2010 04:17 PM, Michael S. Tsirkin wrote:
> >>  +
> >>  +#include "kvmxx.h"
> >>  +#include<boost/function.hpp>
> >
> >This seems to use boost, which is not part of the standard library.
> >Do we want this dependency?
> >We'd need a configure check to verify it's installed.
> >
> 
> I don't mind a boost dependency, but given this is in tr1, I
> switched to that instead.
> 
> >>  +#include<vector>
> >>  +
> >>  +namespace identity {
> >>  +
> >>  +void setup_vm(kvm::vm&  vm);
> >>  +
> >>  +class vcpu {
> >>  +public:
> >>  +    vcpu(kvm::vcpu&  vcpu, boost::function<void ()>  guest_func,
> >>  +	 unsigned long stack_size = 256 * 1024);
> >
> >So the thread stack is moved to use the heap instead?
> 
> Only in guest mode.
> 
> >Can we use pthread_attr_getstacksize and use the regular thread stack?
> >Good for portability.
> 
> In practice 256K should be sufficient.  This is for a unit test, not
> a qemu replacement.


Portability matters though. Isn't it a good idea to replace a bunch
or asm calls with pthread use?

> 
> -- 
> I have a truly marvellous patch that fixes the bug which this
> signature is too narrow to contain.

Re: [PATCH kvm-unit-tests 3/4] Add support for calling a function in guest mode

[Avi Kivity]

On 11/28/2010 10:59 AM, Michael S. Tsirkin wrote:
> On Sat, Nov 27, 2010 at 11:15:18AM +0200, Avi Kivity wrote:
> >  On 11/26/2010 04:17 PM, Michael S. Tsirkin wrote:
> >  >>   +
> >  >>   +#include "kvmxx.h"
> >  >>   +#include<boost/function.hpp>
> >  >
> >  >This seems to use boost, which is not part of the standard library.
> >  >Do we want this dependency?
> >  >We'd need a configure check to verify it's installed.
> >  >
> >
> >  I don't mind a boost dependency, but given this is in tr1, I
> >  switched to that instead.
> >
> >  >>   +#include<vector>
> >  >>   +
> >  >>   +namespace identity {
> >  >>   +
> >  >>   +void setup_vm(kvm::vm&   vm);
> >  >>   +
> >  >>   +class vcpu {
> >  >>   +public:
> >  >>   +    vcpu(kvm::vcpu&   vcpu, boost::function<void ()>   guest_func,
> >  >>   +	 unsigned long stack_size = 256 * 1024);
> >  >
> >  >So the thread stack is moved to use the heap instead?
> >
> >  Only in guest mode.
> >
> >  >Can we use pthread_attr_getstacksize and use the regular thread stack?
> >  >Good for portability.
> >
> >  In practice 256K should be sufficient.  This is for a unit test, not
> >  a qemu replacement.
>
>
> Portability matters though. Isn't it a good idea to replace a bunch
> or asm calls with pthread use?
>

I don't see how?  The host/guest thunk has no pthread equivalent.

-- 
error compiling committee.c: too many arguments to function

[PATCH kvm-unit-tests 4/4] Add sample test using the api test harness

[Avi Kivity]

Call a function setting a global variable.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 config-x86-common.mak |    8 +++++++-
 x86/api-sample.cc     |   23 +++++++++++++++++++++++
 2 files changed, 30 insertions(+), 1 deletions(-)
 create mode 100644 x86/api-sample.cc

diff --git a/config-x86-common.mak b/config-x86-common.mak
index 0f3387b..4fef070 100644
--- a/config-x86-common.mak
+++ b/config-x86-common.mak
@@ -29,6 +29,8 @@ tests-common = $(TEST_DIR)/vmexit.flat $(TEST_DIR)/tsc.flat \
                $(TEST_DIR)/hypercall.flat $(TEST_DIR)/sieve.flat \
                $(TEST_DIR)/kvmclock_test.flat
 
+tests-common += $(TEST_DIR)/api-sample
+
 tests_and_config = $(TEST_DIR)/*.flat $(TEST_DIR)/unittests.cfg
 
 test_cases: $(tests-common) $(tests)
@@ -80,4 +82,8 @@ arch_clean:
 
 -include $(TEST_DIR)/.*.d lib/.*.d lib/x86/.*.d
 
-api/%.o: CFLAGS += -m32
\ No newline at end of file
+api/%.o x86/api-sample.o x86/api-sample: CFLAGS += -m32
+
+x86/api-sample: LDLIBS += -lstdc++
+
+x86/api-sample: x86/api-sample.o api/kvmxx.o api/identity.o
\ No newline at end of file
diff --git a/x86/api-sample.cc b/x86/api-sample.cc
new file mode 100644
index 0000000..a603328
--- /dev/null
+++ b/x86/api-sample.cc
@@ -0,0 +1,23 @@
+
+#include "api/kvmxx.h"
+#include "api/identity.h"
+#include "stdio.h"
+
+static int global = 0;
+
+static void set_global()
+{
+    global = 1;
+}
+
+int main(int ac, char **av)
+{
+    kvm::system system;
+    kvm::vm vm(system);
+    identity::setup_vm(vm);
+    kvm::vcpu vcpu(vm, 0);
+    identity::vcpu thread(vcpu, set_global);
+    vcpu.run();
+    printf("global %d\n", global);
+    return global == 1;
+}
-- 
1.7.1

Re: [PATCH kvm-unit-tests 4/4] Add sample test using the api test harness

[Michael S. Tsirkin]

On Wed, Nov 24, 2010 at 12:52:13PM +0200, Avi Kivity wrote:
> Call a function setting a global variable.
> 
> Signed-off-by: Avi Kivity <avi@redhat.com>
> ---
>  config-x86-common.mak |    8 +++++++-
>  x86/api-sample.cc     |   23 +++++++++++++++++++++++
>  2 files changed, 30 insertions(+), 1 deletions(-)
>  create mode 100644 x86/api-sample.cc
> 
> diff --git a/config-x86-common.mak b/config-x86-common.mak
> index 0f3387b..4fef070 100644
> --- a/config-x86-common.mak
> +++ b/config-x86-common.mak
> @@ -29,6 +29,8 @@ tests-common = $(TEST_DIR)/vmexit.flat $(TEST_DIR)/tsc.flat \
>                 $(TEST_DIR)/hypercall.flat $(TEST_DIR)/sieve.flat \
>                 $(TEST_DIR)/kvmclock_test.flat
>  
> +tests-common += $(TEST_DIR)/api-sample
> +
>  tests_and_config = $(TEST_DIR)/*.flat $(TEST_DIR)/unittests.cfg
>  
>  test_cases: $(tests-common) $(tests)
> @@ -80,4 +82,8 @@ arch_clean:
>  
>  -include $(TEST_DIR)/.*.d lib/.*.d lib/x86/.*.d
>  
> -api/%.o: CFLAGS += -m32
> \ No newline at end of file
> +api/%.o x86/api-sample.o x86/api-sample: CFLAGS += -m32
> +
> +x86/api-sample: LDLIBS += -lstdc++
> +
> +x86/api-sample: x86/api-sample.o api/kvmxx.o api/identity.o
> \ No newline at end of file
> diff --git a/x86/api-sample.cc b/x86/api-sample.cc
> new file mode 100644
> index 0000000..a603328
> --- /dev/null
> +++ b/x86/api-sample.cc
> @@ -0,0 +1,23 @@
> +
> +#include "api/kvmxx.h"
> +#include "api/identity.h"
> +#include "stdio.h"
> +
> +static int global = 0;
> +
> +static void set_global()
> +{
> +    global = 1;
> +}
> +
> +int main(int ac, char **av)
> +{
> +    kvm::system system;
> +    kvm::vm vm(system);
> +    identity::setup_vm(vm);
> +    kvm::vcpu vcpu(vm, 0);
> +    identity::vcpu thread(vcpu, set_global);
> +    vcpu.run();
> +    printf("global %d\n", global);
> +    return global == 1;
> +}

I note that no exceptions are caught.  So any failure will get us a crash,
is there an indication what went wrong?

> -- 
> 1.7.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH kvm-unit-tests 4/4] Add sample test using the api test harness

[Michael S. Tsirkin]

On Sat, Nov 27, 2010 at 11:15:54AM +0200, Avi Kivity wrote:
> On 11/26/2010 04:17 PM, Michael S. Tsirkin wrote:
> >>  +
> >>  +int main(int ac, char **av)
> >>  +{
> >>  +    kvm::system system;
> >>  +    kvm::vm vm(system);
> >>  +    identity::setup_vm(vm);
> >>  +    kvm::vcpu vcpu(vm, 0);
> >>  +    identity::vcpu thread(vcpu, set_global);
> >>  +    vcpu.run();
> >>  +    printf("global %d\n", global);
> >>  +    return global == 1;
> >>  +}
> >
> >I note that no exceptions are caught.  So any failure will get us a crash,
> >is there an indication what went wrong?
> >
> 
> I'll add a try/catch.

So we'll get a "test failed: error 11" at high level?
That's still not very helpful for debugging.
The specific test might be better served by assert calls after each ioctl
than an elaborate exception handling scheme.

> -- 
> I have a truly marvellous patch that fixes the bug which this
> signature is too narrow to contain.

Re: [PATCH kvm-unit-tests 4/4] Add sample test using the api test harness

[Avi Kivity]

On 11/28/2010 11:04 AM, Michael S. Tsirkin wrote:
> On Sat, Nov 27, 2010 at 11:15:54AM +0200, Avi Kivity wrote:
> >  On 11/26/2010 04:17 PM, Michael S. Tsirkin wrote:
> >  >>   +
> >  >>   +int main(int ac, char **av)
> >  >>   +{
> >  >>   +    kvm::system system;
> >  >>   +    kvm::vm vm(system);
> >  >>   +    identity::setup_vm(vm);
> >  >>   +    kvm::vcpu vcpu(vm, 0);
> >  >>   +    identity::vcpu thread(vcpu, set_global);
> >  >>   +    vcpu.run();
> >  >>   +    printf("global %d\n", global);
> >  >>   +    return global == 1;
> >  >>   +}
> >  >
> >  >I note that no exceptions are caught.  So any failure will get us a crash,
> >  >is there an indication what went wrong?
> >  >
> >
> >  I'll add a try/catch.
>
> So we'll get a "test failed: error 11" at high level?
> That's still not very helpful for debugging.
> The specific test might be better served by assert calls after each ioctl
> than an elaborate exception handling scheme.

Particular tests will use a report(message, bool) scheme like the other 
unit tests.  If a test checks for an error, it will have an explicit 
try/catch (or better, expect_error(std::tr1::function<>, errno).

The top-level try/catch is for unexpected errors.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH kvm-unit-tests 0/4] API test framework

[Marcelo Tosatti]

On Wed, Nov 24, 2010 at 12:52:09PM +0200, Avi Kivity wrote:
> This patchset adds an API test framework.  Rather than driving kvm from qemu,
> we now have a way of calling the kvm API directly and observing the results.
> We can switch to guest mode and back at will and see any micro effects such
> as the result of executing particular instructions.
> 
> As an experiment, the framework is coded in C++.
> 
> Avi Kivity (4):
>   Makefile: add support for C++
>   Introduce a C++ wrapper for the kvm APIs
>   Add support for calling a function in guest mode
>   Add sample test using the api test harness
> 
>  Makefile              |    4 +-
>  api/identity.cc       |   76 ++++++++++++++++++++++
>  api/identity.h        |   28 ++++++++
>  api/kvmxx.cc          |  168 +++++++++++++++++++++++++++++++++++++++++++++++++
>  api/kvmxx.h           |   80 +++++++++++++++++++++++
>  config-x86-common.mak |    8 +++
>  x86/api-sample.cc     |   23 +++++++
>  7 files changed, 386 insertions(+), 1 deletions(-)
>  create mode 100644 api/identity.cc
>  create mode 100644 api/identity.h
>  create mode 100644 api/kvmxx.cc
>  create mode 100644 api/kvmxx.h
>  create mode 100644 x86/api-sample.cc

I fail to see practical advantages of this compared to current unit
tests. Could you give some exciting examples?

The fact that it does not run inside QEMU
means you can't test QEMU interactions (re:
http://permalink.gmane.org/gmane.comp.emulators.kvm.devel/59060).

Perhaps you can write a save/restore example as mentioned in the URL 
above?

To me it seems having an interface to QEMU from unit tests would be more
beneficial.

Re: [PATCH kvm-unit-tests 0/4] API test framework

[Avi Kivity]

On 11/29/2010 06:09 PM, Marcelo Tosatti wrote:
> I fail to see practical advantages of this compared to current unit
> tests. Could you give some exciting examples?

You can test the API directly, or set up specific states that are hard 
to reach from a guest.

Examples:
- test mst's dirty log fix by writing repeatedly from a vcpu while 
polling the dirty log from another thread (plan to do this for v2)
- set up wierd states (nmi-blocked-by-sti), force an exit (using pio or 
debug registers), verify state save shows the expected values

> The fact that it does not run inside QEMU
> means you can't test QEMU interactions (re:
> http://permalink.gmane.org/gmane.comp.emulators.kvm.devel/59060).

Using qemu means restricting outselves to the subset of kvm that qemu 
uses.  Since it's a very large subset, it's not a problem, but still.  
Also, it's hard to test corner cases with qemu, see the examples above.

> Perhaps you can write a save/restore example as mentioned in the URL
> above?

Should be easy, will try for v2 or v3.

> To me it seems having an interface to QEMU from unit tests would be more
> beneficial.

It's still very roundabout compared to:

- set up mce handler (can do it in host code)
- issue ioctl
- run()
- see whether the handler was invoked or not.


-- 
error compiling committee.c: too many arguments to function