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

[Anthony Liguori <anthony@codemonkey.ws>]

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
>