Thanks Petri, agree with all your comments!
Thanks
Terrence
>-----Original Message-----
>From: Latvala, Petri
>Sent: Thursday, March 9, 2017 6:58 PM
>To: Xu, Terrence <terrence.xu@xxxxxxxxx>
>Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Jin, Gordon <gordon.jin@xxxxxxxxx>; Sarvela,
>Tomi P <tomi.p.sarvela@xxxxxxxxx>; Lv, Zhiyuan <zhiyuan.lv@xxxxxxxxx>; Wang,
>Zhenyu Z <zhenyu.z.wang@xxxxxxxxx>; Wang, Hongbo
><hongbo.wang@xxxxxxxxx>; Xu, BenyuX <benyux.xu@xxxxxxxxx>
>Subject: Re: [PATCH i-g-t v3] Add the new tool for create GVT-g Linux guest based
>on KVMGT
>
>On Fri, Mar 10, 2017 at 03:34:49AM +0800, Terrence Xu wrote:
>> GVT-g (Intel(r) 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.
>>
>> v3: Make some optimizations: like "update the generate mac address
>> scripts", "provide more useful information for end user", etc.
>>
>> Signed-off-by: Terrence Xu <terrence.xu@xxxxxxxxx>
>> Signed-off-by: Benyu Xu <benyux.xu@xxxxxxxxx>
>> ---
>> tools/.gitignore | 1 +
>> tools/Makefile.sources | 1 +
>> tools/intel_gvtg_test.c | 359
>> ++++++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 361 insertions(+)
>> create mode 100644 tools/intel_gvtg_test.c
>>
>> diff --git a/tools/.gitignore b/tools/.gitignore index
>> 13825a3..f777f2b 100644
>> --- a/tools/.gitignore
>> +++ b/tools/.gitignore
>> @@ -30,5 +30,6 @@ intel_reg_checker
>> intel_residency
>> intel_stepping
>> intel_watermark
>> +intel_gvtg_test
>> skl_compute_wrpll
>> skl_ddb_allocation
>
>
>Sort alphabetically.
>
>
>> diff --git a/tools/Makefile.sources b/tools/Makefile.sources index
>> e2451ea..5f1e747 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)
>>
>> 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..ec6dd99
>> --- /dev/null
>> +++ b/tools/intel_gvtg_test.c
>> @@ -0,0 +1,359 @@
>> +/*
>> + * Copyright 2016 Intel Corporation
>> + * Terrence Xu <terrence.xu@xxxxxxxxx>
>> + *
>
>
>This should be
>
>
> * Copyright (c) 2016-2017 Intel Corporation
>
>
>> + * 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 (the tool should be
>> +root
>> + * privilege only).
>> + *
>> + * 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>
>> +#include <dirent.h>
>> +
>> +#define RANDOM(x) (rand() % x)
>> +
>> +
>> +static char uuid[40];
>> +static char guest_ip[32];
>> +
>> +static char mac_addr[20] = {0};
>> +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 parameter invalid!\n", __func__);
>> + 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)) {
>> + ;
>> + }
>> + 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_gvtg_support(void)
>> +{
>> + DIR *mdevdir = opendir("/sys/bus/pci/devices/0000:00:02.0/"
>> + "mdev_supported_types");
>> +
>> + if (mdevdir == NULL) {
>> + return 1;
>> + }
>> + closedir(mdevdir);
>> + return 0;
>> +}
>> +
>> +static int check_tools(void)
>> +{
>> + if (system("which uuidgen > /dev/null") != 0) {
>> + return 1;
>> + } else if (system("which arp-scan > /dev/null") != 0) {
>> + return 2;
>> + } else if (system("which /etc/qemu-ifup > /dev/null") != 0) {
>> + return 3;
>> + }
>> + 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/$(ls /sys/bus/pci/devices/0000:00:02.0/"
>> + "mdev_supported_types |awk {'print $1'}|tail -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 cirrus -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);
>> +}
>> +
>> +static void gen_mac_addr(void)
>> +{
>> + srand(getpid());
>> + sprintf(mac_addr, "52:54:00:%2X:%02X:%02X", RANDOM(256),
>> +RANDOM(256), RANDOM(256)); }
>> +
>> +static void gen_uuid(void)
>> +{
>> + igt_assert_eq(super_system("uuidgen", uuid, sizeof(uuid)), 0); }
>> +
>
>That ending brace slipped to the previous line.
>
>
>> +static char *fetch_ip_by_mac(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("Trying to connnect guest, attempt %d.\n", timeout);
>
>
>Typo fix, connnect -> connect
>
>
>> + 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 > /dev/null"), 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"
>> + "[example]\n"
>> + " ./intel_gvtg_test -q /usr/bin/qemu-system-x86_64 -a "
>> + "/home/img/ubuntu-16.04.img -b /usr/bin/bios.bin\n"
>> + );
>> +}
>> +
>> +static void arg_mismatch(char *arg)
>> +{
>> + igt_info("argument mismatch: %s\n", arg); }
>> +
>> +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_gvtg_support() == 1) {
>> + igt_skip("GVT-g technology is not supported in your system.\n");
>> + }
>> +
>> + 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");
>> + } else if (check_tools() == 3) {
>> + igt_skip("Please prepare the \"qemu-ifup\" script.\n");
>> + }
>> +
>
>The pattern
>
> if (something) {
> igt_skip("text");
> }
>
>can be written as
>
> igt_skip_on_f(something, "text")
>
>but that's not a huge deal.
>
>Calling check_tools() multiple times is a bit wasteful. Also not a huge deal.
>
>
>Otherwise looks good to me.
>
>
>--
>Petri Latvala
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
