On Sat, Dec 02, 2017 at 04:05:32AM +0900, Masatake YAMATO wrote: > * tests/Makefile.am: Add ioctl_kvm. > * tests/ioctl_kvm.c: New test target file. > Taken from https://lwn.net/Articles/658512/. > * tests/ioctl_kvm.expected: New expected file. > * tests/ioctl_kvm.test: New test driver. > > Changes in v2: > * Skip the test case if kvm.h is not available. > * Skip the test case if opening /dev/kvm is failed. > * Include sys/typtes.h first. > > All items are suggested by ldv. > --- > tests/Makefile.am | 1 + > tests/ioctl_kvm.c | 162 > +++++++++++++++++++++++++++++++++++++++++++++++ > tests/ioctl_kvm.expected | 11 ++++ > tests/ioctl_kvm.test | 12 ++++ > 4 files changed, 186 insertions(+) > create mode 100644 tests/ioctl_kvm.c > create mode 100644 tests/ioctl_kvm.expected > create mode 100755 tests/ioctl_kvm.test > > diff --git a/tests/Makefile.am b/tests/Makefile.am > index f2109fd4..6aa998e5 100644 > --- a/tests/Makefile.am > +++ b/tests/Makefile.am > @@ -102,6 +102,7 @@ check_PROGRAMS = $(PURE_EXECUTABLES) \ > int_0x80 \ > ioctl_dm-v \ > ioctl_evdev-v \ > + ioctl_kvm \ > ioctl_loop-nv \ > ioctl_loop-v \ > ioctl_nsfs \ > diff --git a/tests/ioctl_kvm.c b/tests/ioctl_kvm.c > new file mode 100644 > index 00000000..63865f91 > --- /dev/null > +++ b/tests/ioctl_kvm.c > @@ -0,0 +1,162 @@ > +/* Based on the program explained on the page, > https://lwn.net/Articles/658512/ */ > + > +/* Sample code for /dev/kvm API > + * > + * Copyright (c) 2015 Intel Corporation > + * Author: Josh Triplett <j...@joshtriplett.org> > + * > + * Permission is hereby granted, free of charge, to any person obtaining a > copy > + * of this software and associated documentation files (the "Software"), to > + * deal in the Software without restriction, including without limitation the > + * rights to use, copy, modify, merge, publish, distribute, sublicense, > and/or > + * sell copies of the Software, and to permit persons to whom the Software is > + * furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice shall be included in > + * all copies or substantial portions of the Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > THE > + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER > DEALINGS > + * IN THE SOFTWARE. > + */ > + > +#include "tests.h" > + > +#ifdef HAVE_LINUX_KVM_H > + > +#include <sys/types.h> > +#include <err.h> > +#include <fcntl.h> > +#include <linux/kvm.h> > +#include <stdint.h> > +#include <stdio.h> > +#include <stdlib.h> > +#include <string.h> > +#include <sys/ioctl.h> > +#include <sys/mman.h> > +#include <sys/stat.h>
It's generally a good idea to place includes like <linux/kvm.h> to the end
of include list.
I bet the following of these includes are not needed:
#include <sys/types.h>
#include <sys/stat.h>
I'd also like to get rid of <err.h>, see below.
> +
> +int main(void)
> +{
> + int kvm, vmfd, vcpufd, ret;
> + const uint8_t code[] = {
> + 0xba, 0xf8, 0x03, /* mov $0x3f8, %dx */
> + 0x00, 0xd8, /* add %bl, %al */
> + 0x04, '0', /* add $'0', %al */
> + 0xee, /* out %al, (%dx) */
> + 0xb0, '\n', /* mov $'\n', %al */
> + 0xee, /* out %al, (%dx) */
> + 0xf4, /* hlt */
> + };
This looks very x86-specific. Should the whole test be limited
to #ifdef __x86_64__?
> + uint8_t *mem;
> + struct kvm_sregs sregs;
> + size_t mmap_size;
> + struct kvm_run *run;
> +
> + kvm = open("/dev/kvm", O_RDWR | O_CLOEXEC);
> + if (kvm == -1)
> + perror_msg_and_skip("open: %s", "/dev/kvm");
> +
> + /* Make sure we have the stable version of the API */
> + ret = ioctl(kvm, KVM_GET_API_VERSION, NULL);
> + if (ret == -1)
> + err(1, "KVM_GET_API_VERSION");
I'm not quite fond of err(), especially in this case where
perror_msg_and_skip seems to be more suitable: as we are not testing the
kernel, let's skip the test when the kernel is too old.
> + if (ret != 12)
> + errx(1, "KVM_GET_API_VERSION %d, expected 12", ret);
Why 12? Is it the value of KVM_API_VERSION since kernel commit
v2.6.22-rc1~1008^2? Why can't KVM_API_VERSION be used here instead?
The macro was introduced by kernel commit v2.6.20-rc2~56 which is not far
from commit v2.6.20-rc1~15^2~39 when linux/kvm.h itself was introduced.
> + vmfd = ioctl(kvm, KVM_CREATE_VM, (unsigned long)0);
> + if (vmfd == -1)
> + err(1, "KVM_CREATE_VM");
Likewise, I'd rather skip the test if this or any other KVM_* ioctl failed.
> + /* Allocate one aligned page of guest memory to hold the code. */
> + mem = mmap(NULL, 0x1000, PROT_READ | PROT_WRITE, MAP_SHARED |
> MAP_ANONYMOUS, -1, 0);
> + if (!mem)
> + err(1, "allocating guest memory");
mmap returns MAP_FAILED on error.
I'd used perror_msg_and_fail() rather than err(1).
> + memcpy(mem, code, sizeof(code));
> +
> + /* Map it to the second page frame (to avoid the real-mode IDT at 0). */
> + struct kvm_userspace_memory_region region = {
> + .slot = 0,
> + .guest_phys_addr = 0x1000,
> + .memory_size = 0x1000,
> + .userspace_addr = (uint64_t)mem,
Some compilers warn about casting from pointer to integer of different size,
please change (uint64_t)mem to (uintptr_t) mem.
> + };
> + ret = ioctl(vmfd, KVM_SET_USER_MEMORY_REGION, ®ion);
> + if (ret == -1)
> + err(1, "KVM_SET_USER_MEMORY_REGION");
> +
> + vcpufd = ioctl(vmfd, KVM_CREATE_VCPU, (unsigned long)0);
0UL has the same meaning as (unsigned long)0.
> + if (vcpufd == -1)
> + err(1, "KVM_CREATE_VCPU");
> +
> + /* Map the shared kvm_run structure and following data. */
> + ret = ioctl(kvm, KVM_GET_VCPU_MMAP_SIZE, NULL);
> + if (ret == -1)
> + err(1, "KVM_GET_VCPU_MMAP_SIZE");
> + mmap_size = ret;
> + if (mmap_size < sizeof(*run))
> + errx(1, "KVM_GET_VCPU_MMAP_SIZE unexpectedly small");
> + run = mmap(NULL, mmap_size, PROT_READ | PROT_WRITE, MAP_SHARED, vcpufd,
> 0);
> + if (!run)
> + err(1, "mmap vcpu");
> +
> + /* Initialize CS to point at 0, via a read-modify-write of sregs. */
> + ret = ioctl(vcpufd, KVM_GET_SREGS, &sregs);
> + if (ret == -1)
> + err(1, "KVM_GET_SREGS");
> + sregs.cs.base = 0;
> + sregs.cs.selector = 0;
> + ret = ioctl(vcpufd, KVM_SET_SREGS, &sregs);
> + if (ret == -1)
> + err(1, "KVM_SET_SREGS");
> +
> + /* Initialize registers: instruction pointer for our code, addends, and
> + * initial flags required by x86 architecture. */
> + struct kvm_regs regs = {
> + .rip = 0x1000,
> + .rax = 2,
> + .rbx = 2,
> + .rflags = 0x2,
> + };
Looks like the whole test has to be limited to #ifdef __x86_64__
while it relies on these very x86-specific fields of kvm_sregs and kvm_regs.
--
ldv
signature.asc
Description: PGP signature
------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________ Strace-devel mailing list Strace-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/strace-devel
