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

Anthony Liguori
2010-11-24T14:10:47Z

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
>