[PATCH 8/8] tests: check KVM ioctl commands decoding

Dmitry V. Levin ldv at altlinux.org
Fri Dec 1 23:34:23 UTC 2017 

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 <josh at 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, &region);
> +	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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20171202/894b1665/attachment.bin>

More information about the Strace-devel mailing list