A couple of comments below.
On Thu, Jan 19, 2017 at 04:32:19PM +0800, Terrence Xu wrote:
> GVT-g (Intel® Graphics Virtualization Technology) is a full GPU
> virtualization solution with mediated pass-through support.
>
> This tool is for create basic Linux guest based on KVMGT with VFIO
> framework, it including create vgpu, create guest, check IP address,
> destroy guest, remove vgpu,check dmesg log, etc functions.
>
> v2: Treat this case as a free-standing tool, with detect & skip when
> it's not running on GVT-g capable platform or running without the
> required tools.
>
> Signed-off-by: Terrence Xu <terrence.xu@xxxxxxxxx>
> Signed-off-by: Benyu Xu <benyux.xu@xxxxxxxxx>
> ---
> tools/Makefile.sources | 1 +
> tools/intel_gvtg_test.c | 355 ++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 356 insertions(+)
> create mode 100644 tools/intel_gvtg_test.c
>
> diff --git a/tools/Makefile.sources b/tools/Makefile.sources
> index e2451ea..05ef0b7 100644
> --- a/tools/Makefile.sources
> +++ b/tools/Makefile.sources
> @@ -30,6 +30,7 @@ tools_prog_lists = \
> intel_stepping \
> intel_watermark \
> intel_gem_info \
> + intel_gvtg_test \
> $(NULL)
Check the indentation of that line.
Please add intel_gvtg_test to tools/.gitignore.
>
> dist_bin_SCRIPTS = intel_gpu_abrt
> diff --git a/tools/intel_gvtg_test.c b/tools/intel_gvtg_test.c
> new file mode 100644
> index 0000000..4c89800
> --- /dev/null
> +++ b/tools/intel_gvtg_test.c
> @@ -0,0 +1,355 @@
> +/*
> + * Copyright 2016 Intel Corporation
> + * Terrence Xu <terrence.xu@xxxxxxxxx>
> + *
> + * 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.
> + */
> +
> +/*
> + * This program is intended for testing of validate GVT-g virtual machine
> + * creation of allow for testing of KVMGT / XenGT.
> + *
> + * TODO:
> + * Enable more GVT-g related test cases.
> + */
> +#ifdef HAVE_CONFIG_H
> +#include "config.h"
> +#endif
> +
> +#include "igt.h"
> +#include <errno.h>
> +#include <getopt.h>
> +#include <math.h>
> +#include <stdint.h>
> +#include <stdbool.h>
> +#include <strings.h>
> +#include <unistd.h>
> +#include <termios.h>
> +#include <sys/poll.h>
> +#include <sys/time.h>
> +#include <sys/ioctl.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <string.h>
> +#include <stdlib.h>
> +#include <signal.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <time.h>
> +#include <unistd.h>
> +#include <pwd.h>
> +#include <limits.h>
> +
> +#define RANDOM(x) (rand() % x)
> +
> +static uint32_t devid;
> +
> +static unsigned char mac_addr[17] = {'0', '0', ':', '0', '0', ':',
> + '0', '0', ':', '0', '0', ':', '0', '0', ':', '0', '0'};
> +static char uuid[40];
> +static char guest_ip[32];
> +
> +static char qemu_path[PATH_MAX] = {0};
> +static char hda_path[PATH_MAX] = {0};
> +static char bios_path[PATH_MAX] = {0};
> +
> +static int super_system(const char *cmd, char *retmsg, int msg_len)
> +{
> + FILE *fp;
> + int res = -1;
> + if (cmd == NULL || retmsg == NULL || msg_len < 0) {
> + igt_info("Error: %s system paramer invalid!\n", __func__);
paramer -> parameter
> + return 1;
> + }
> + fp = popen(cmd, "r");
> + if (fp == NULL) {
> + perror("popen");
> + igt_info("Error: %s popen error: %s\n", __func__, strerror(errno));
> + return 2;
> + } else {
> + memset(retmsg, 0, msg_len);
> + while (fgets(retmsg, msg_len, fp)) {
> + ;
> + };
Remove that semicolon after while's body's closing brace.
> + res = pclose(fp);
> + if (res == -1) {
> + igt_info("Error:%s close popen file pointer fp error!\n", __func__);
> + return 3;
> + }
> + retmsg[strlen(retmsg) - 1] = '\0';
> + return 0;
> + }
> +}
> +
> +static int check_platform(void)
> +{
> + devid = intel_get_pci_device()->device_id;
> + if (IS_BROADWELL(devid) || (IS_SKYLAKE(devid))) {
> + return 0;
> + } else {
> + return 1;
> + }
> +}
This is not future-proof. And it doesn't really tell whether the
running kernel has GVT-g support enabled. Is it possible to detect
GVT-g support here?
> +
> +static int check_tools(void)
> +{
> + if (system("which uuidgen") != 0) {
> + return 1;
> + } else if (system("which arp-scan") != 0) {
> + return 2;
> + }
> + return 0;
> +}
> +
> +static void create_guest(void)
> +{
> + char create_qcow_cmd[PATH_MAX] = {0};
> + char create_vgpu_cmd[PATH_MAX] = {0};
> + char create_instance_cmd[PATH_MAX] = {0};
> +
> + sprintf(create_qcow_cmd, "qemu-img create -b %s -f qcow2 %s.qcow2",
> + hda_path, hda_path);
> + sprintf(create_vgpu_cmd, "echo \"%s\" > /sys/bus/pci/devices/0000:00:02.0/"
> + "mdev_supported_types/i915-GVTg_V4_1/create", uuid);
> + sprintf(create_instance_cmd, "%s -m 2048 -smp 2 -M pc -name gvtg_guest"
> + " -hda %s.qcow2 -bios %s -enable-kvm --net nic,macaddr=%s -net"
> + " tap,script=/etc/qemu-ifup -vga none -device isa-vga -k en-us"
> + " -serial stdio -vnc :1 -machine kernel_irqchip=on -global"
> + " PIIX4_PM.disable_s3=1 -global PIIX4_PM.disable_s4=1 -cpu host"
> + " -usb -usbdevice tablet -device vfio-pci,sysfsdev="
> + "/sys/bus/pci/devices/0000:00:02.0/%s &",
> + qemu_path, hda_path, bios_path, mac_addr, uuid);
> + igt_assert_eq(system(create_qcow_cmd), 0);
> + igt_assert_eq(system(create_vgpu_cmd), 0);
> + igt_assert_eq(system(create_instance_cmd), 0);
> +}
> +
> +static void destroy_all_guest(void)
> +{
> + system("pkill qemu");
> +}
> +
> +static void remove_vgpu(void)
> +{
> + char cmd[128] = {0};
> + sprintf(cmd, "echo 1 > /sys/bus/pci/devices/0000:00:02.0/%s/remove", uuid);
> + igt_assert_eq(system(cmd), 0);
> +}
Doesn't this echo need root?
> +
> +static void gen_mac_addr(void)
> +{
> + unsigned char mac_enum[16] = {'0', '1', '2', '3', '4', '5', '6',
> + '7', '8', '9', 'A', 'B', 'C', 'D', 'E', 'F'};
> + unsigned short i, n = 2;
> + srand(getpid());
> + for (i = 2, n = 2; i < 15; i++, n++) {
> + if (mac_addr[n] != ':') {
> + mac_addr[n] = mac_enum[RANDOM(16)];
> + }
> + }
> +}
> +
> +static void gen_uuid(void)
> +{
> + igt_assert_eq(super_system("uuidgen", uuid, sizeof(uuid)), 0);
> +}
> +
> +static char *fetch_ip_by_mac(unsigned char *mac)
> +{
> + char cmd[1024] = {0};
> + sprintf(cmd, "arp-scan -l -I $(ip addr show|grep inet|grep global|"
> + "awk '{print $NF;}')|grep -i %s|awk '{print $1}'", mac);
> + igt_assert_eq(super_system(cmd, guest_ip, sizeof(guest_ip)), 0);
> + return guest_ip;
> +}
> +
> +static int check_guest_ip(void)
> +{
> + int timeout = 0;
> + while (timeout <= 12) {
> + igt_info("Try to connnect guest,the %d time.\n", timeout);
Maybe: "Trying to connect to guest, attempt %d.\n"
> + if (timeout == 12) {
> + igt_info("Cannot connect to guest.\n");
> + return 1;
> + break;
> + }
> +
> + fetch_ip_by_mac(mac_addr);
> +
> + if (guest_ip[0] != '\0') {
> + igt_info("Fetched guest ip address: %s.\n", guest_ip);
> + break;
> + }
> + timeout++;
> + sleep(5);
> + }
> + return 0;
> +}
> +
> +static void clear_dmesg(void)
> +{
> + igt_assert_eq(system("dmesg -c"), 0);
> +}
> +
> +static int check_dmesg(void)
> +{
> + char errorlog[PATH_MAX] = {0};
> +
> + igt_assert_eq(super_system("dmesg|grep -E \"GPU HANG|gfx reset|BUG\"",
> + errorlog, sizeof(errorlog)), 0);
> +
> + if (errorlog[0] == '\0') {
> + return 0;
> + } else {
> + return 1;
> + }
> +}
> +
> +static const char optstr[] = "h";
> +
> +static void print_help(void)
> +{
> + igt_info("\n [options]\n"
> + "-h, --help display usage\n"
> + "-q, --qemu the qemu path\n"
> + "-a, --hda the hda raw image / qcow path\n"
> + "-b, --bios the seabios path\n\n"
> + );
> +}
> +
> +static void arg_mismatch(void)
> +{
> + igt_info(" argument mismatch!\n");
> +}
I don't see anything below in main() that tells the user which
argument wasn't recognized.
> +
> +int main(int argc, char **argv)
> +{
> + int opt = -1;
> + const char *optstring = "hq:a:b:";
> + static struct option long_options[] = {
> + {"help", 0, NULL, 'h'},
> + {"qemu", 1, NULL, 'q'},
> + {"hda", 1, NULL, 'a'},
> + {"bios", 1, NULL, 'b'},
> + {0, 0, 0, 0}
> + };
> +
> + int ret = 0;
> + int flag_cnt = 0;
> + int h_flag = 0;
> + int q_flag = 0;
> + int a_flag = 0;
> + int b_flag = 0;
> +
> + if (check_platform() == 1) {
> + igt_skip("GVT-g technology only support Intel Broadwell and Skylake \
> + platform.\n");
This will print an awful looking line with loads of spaces, but the
comment above on check_platform() applies here anyway.
--
Petri Latvala
> + }
> + if (check_tools() == 1) {
> + igt_skip("Please install the \"uuid-runtime\" tool.\n");
> + } else if (check_tools() == 2) {
> + igt_skip("Please install the \"arp-scan\" tool.\n");
> + }
> +
> + if (argc == 1) {
> + print_help();
> + return 0;
> + }
> +
> + while ((opt = getopt_long(argc, argv, optstring, long_options, NULL))
> + != -1) {
> + switch (opt) {
> + case 'h':
> + h_flag = 1;
> + break;
> + case 'q':
> + strcpy(qemu_path, optarg);
> + q_flag = 1;
> + break;
> + case 'a':
> + strcpy(hda_path, optarg);
> + a_flag = 1;
> + break;
> + case 'b':
> + strcpy(bios_path, optarg);
> + b_flag = 1;
> + break;
> + case ':':
> + ret = -1;
> + break;
> + case '?':
> + ret = -1;
> + break;
> + }
> +
> + if (ret) {
> + break;
> + }
> + };
> +
> + if (ret != 0) {
> + arg_mismatch();
> + return ret;
> + }
> +
> + flag_cnt = h_flag + q_flag + a_flag + b_flag;
> +
> + if (flag_cnt == 1) {
> + if (h_flag == 1) {
> + print_help();
> + return 0;
> + } else {
> + arg_mismatch();
> + return -1;
> + }
> + } else if (flag_cnt == 3) {
> + if (q_flag == 1 && a_flag == 1 && b_flag == 1) {
> + igt_info("\nqemu_path: %s\n hda_path: %s\n bios_path: %s\n",
> + qemu_path, hda_path, bios_path);
> + } else {
> + arg_mismatch();
> + return -1;
> + }
> + } else {
> + arg_mismatch();
> + return -1;
> + }
> +
> + destroy_all_guest();
> + clear_dmesg();
> +
> + gen_mac_addr();
> + gen_uuid();
> + create_guest();
> +
> + if (check_guest_ip()) {
> + ret = 1;
> + }
> +
> + destroy_all_guest();
> + sleep(5);
> + remove_vgpu();
> +
> + if (check_dmesg() > 0) {
> + ret = 1;
> + }
> +
> + igt_assert_eq(ret, 0);
> + igt_exit();
> +}
> --
> 1.9.1
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
