Re: [PATCH v2] Add the new tool for create GVT-g Linux guest based on KVMGT

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



It is OK for me for all your comments, thanks!
Just one question need to confirm with you inline.

>A couple of comments below.
>
>
>On Thu, Jan 19, 2017 at 04:32:19PM +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.
>>
>> 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?
[Xu, Terrence] Yes the root is needed for all our "echo" related commands, can I suppose the case is running under root privilege? Or need to return "skip" when it is not under root privilege?
>
>> +
>> +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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux