Re: [PATCH v3] qga: add command guest-get-devices for reporting VirtIO devices

[Marc-André Lureau]

Hi

On Thu, Sep 26, 2019 at 1:01 AM Tomáš Golembiovský <address@hidden> wrote:
>
> Add command for reporting devices on Windows guest. The intent is not so
> much to report the devices but more importantly the driver (and its
> version) that is assigned to the device. This gives caller the
> information whether VirtIO drivers are installed and/or whether
> inadequate driver is used on a device (e.g. QXL device with base VGA
> driver).
>
> Signed-off-by: Tomáš Golembiovský <address@hidden>
> ---
>  qga/commands-posix.c |   9 ++
>  qga/commands-win32.c | 204 ++++++++++++++++++++++++++++++++++++++++++-
>  qga/qapi-schema.json |  32 +++++++
>  3 files changed, 244 insertions(+), 1 deletion(-)
>
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index dfc05f5b8a..58e93feef9 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -2757,6 +2757,8 @@ GList *ga_command_blacklist_init(GList *blacklist)
>      blacklist = g_list_append(blacklist, g_strdup("guest-fstrim"));
>  #endif
>
> +    blacklist = g_list_append(blacklist, g_strdup("guest-get-devices"));
> +
>      return blacklist;
>  }
>
> @@ -2977,3 +2979,10 @@ GuestOSInfo *qmp_guest_get_osinfo(Error **errp)
>
>      return info;
>  }
> +
> +GuestDeviceInfoList *qmp_guest_get_devices(Error **errp)
> +{
> +    error_setg(errp, QERR_UNSUPPORTED);
> +
> +    return NULL;
> +}
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 6b67f16faf..139dbd7c9a 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -21,10 +21,11 @@
>  #ifdef CONFIG_QGA_NTDDSCSI
>  #include <winioctl.h>
>  #include <ntddscsi.h>
> +#endif
>  #include <setupapi.h>
>  #include <cfgmgr32.h>
>  #include <initguid.h>
> -#endif
> +#include <devpropdef.h>
>  #include <lm.h>
>  #include <wtsapi32.h>
>  #include <wininet.h>
> @@ -38,6 +39,36 @@
>  #include "qemu/host-utils.h"
>  #include "qemu/base64.h"
>
> +/*
> + * The following should be in devpkey.h, but it isn't. The key names were
> + * prefixed to avoid (future) name clashes. Once the definitions get into
> + * mingw the following lines can be removed.
> + */
> +DEFINE_DEVPROPKEY(qga_DEVPKEY_NAME, 0xb725f130, 0x47ef, 0x101a, 0xa5,
> +    0xf1, 0x02, 0x60, 0x8c, 0x9e, 0xeb, 0xac, 10);
> +    /* DEVPROP_TYPE_STRING */
> +DEFINE_DEVPROPKEY(qga_DEVPKEY_Device_HardwareIds, 0xa45c254e, 0xdf1c,
> +    0x4efd, 0x80, 0x20, 0x67, 0xd1, 0x46, 0xa8, 0x50, 0xe0, 3);
> +    /* DEVPROP_TYPE_STRING_LIST */
> +DEFINE_DEVPROPKEY(qga_DEVPKEY_Device_DriverDate, 0xa8b865dd, 0x2e3d,
> +    0x4094, 0xad, 0x97, 0xe5, 0x93, 0xa7, 0xc, 0x75, 0xd6, 2);
> +    /* DEVPROP_TYPE_FILETIME */
> +DEFINE_DEVPROPKEY(qga_DEVPKEY_Device_DriverVersion, 0xa8b865dd, 0x2e3d,
> +    0x4094, 0xad, 0x97, 0xe5, 0x93, 0xa7, 0xc, 0x75, 0xd6, 3);
> +    /* DEVPROP_TYPE_STRING */
> +/* The following shoud be in cfgmgr32.h, but it isn't */
> +#ifndef CM_Get_DevNode_Property
> +CMAPI CONFIGRET WINAPI CM_Get_DevNode_PropertyW(
> +    DEVINST          dnDevInst,
> +    CONST DEVPROPKEY * PropertyKey,
> +    DEVPROPTYPE      * PropertyType,
> +    PBYTE            PropertyBuffer,
> +    PULONG           PropertyBufferSize,
> +    ULONG            ulFlags
> +);
> +#define CM_Get_DevNode_Property CM_Get_DevNode_PropertyW
> +#endif
> +
>  #ifndef SHTDN_REASON_FLAG_PLANNED
>  #define SHTDN_REASON_FLAG_PLANNED 0x80000000
>  #endif
> @@ -92,6 +123,8 @@ static OpenFlags guest_file_open_modes[] = {
>      g_free(suffix); \
>  } while (0)
>
> +G_DEFINE_AUTOPTR_CLEANUP_FUNC(GuestDeviceInfo, qapi_free_GuestDeviceInfo)
> +
>  static OpenFlags *find_open_flag(const char *mode_str)
>  {
>      int mode;
> @@ -2234,3 +2267,172 @@ GuestOSInfo *qmp_guest_get_osinfo(Error **errp)
>
>      return info;
>  }
> +
> +/*
> + * Safely get device property. Returned strings are using wide characters.
> + * Caller is responsible for freeing the buffer.
> + */
> +static LPBYTE cm_get_property(DEVINST devInst, const DEVPROPKEY *propName,
> +    PDEVPROPTYPE propType)
> +{
> +    CONFIGRET cr;
> +    g_autofree LPBYTE buffer = NULL;
> +    ULONG buffer_len = 0;
> +
> +    /* First query for needed space */
> +    cr = CM_Get_DevNode_PropertyW(devInst, propName, propType,
> +        buffer, &buffer_len, 0);
> +    if (cr != CR_SUCCESS && cr != CR_BUFFER_SMALL) {
> +
> +        slog("failed to get property size, error=0x%lx", cr);
> +        return NULL;
> +    }
> +    buffer = g_new0(BYTE, buffer_len + 1);
> +    cr = CM_Get_DevNode_PropertyW(devInst, propName, propType,
> +        buffer, &buffer_len, 0);
> +    if (cr != CR_SUCCESS) {
> +        slog("failed to get device property, error=0x%lx", cr);
> +        return NULL;
> +    }
> +    return g_steal_pointer(&buffer);
> +}
> +
> +static GStrv ga_get_hardware_ids(DEVINST devInstance)
> +{
> +    GStrv hw_ids = NULL;
> +    GArray *values = NULL;
> +    DEVPROPTYPE cm_type;
> +    LPWSTR id;
> +    g_autofree LPWSTR property = (LPWSTR)cm_get_property(devInstance,
> +        &qga_DEVPKEY_Device_HardwareIds, &cm_type);
> +    if (property == NULL) {
> +        slog("failed to get hardware IDs");
> +        return NULL;
> +    }
> +    if (*property == '\0') {
> +        /* empty list */
> +        return NULL;
> +    }
> +    values = g_array_new(TRUE, TRUE, sizeof(gchar*));
> +    for (id = property; '\0' != *id; id += lstrlenW(id) + 1) {
> +        gchar* id8 = g_utf16_to_utf8(id, -1, NULL, NULL, NULL);
> +        g_array_append_val(values, id8);
> +    }
> +    hw_ids = (GStrv)g_array_free(values, FALSE);
> +    values = NULL;
> +    return hw_ids;

Why not return g_array_free(values, FALSE) directly?

> +}
> +
> +/*
> + * 
> https://docs.microsoft.com/en-us/windows-hardware/drivers/install/identifiers-for-pci-devices
> + */
> +#define DEVICE_PCI_RE "PCI\\\\VEN_(1AF4|1B36)&DEV_([0-9A-B]{4})(&|$)"
> +
> +GuestDeviceInfoList *qmp_guest_get_devices(Error **errp)
> +{
> +    GuestDeviceInfoList *head = NULL, *cur_item = NULL, *item = NULL;
> +    HDEVINFO dev_info = INVALID_HANDLE_VALUE;
> +    SP_DEVINFO_DATA dev_info_data;
> +    int i, j;
> +    GError *gerr = NULL;
> +    g_autoptr(GRegex) device_pci_re = NULL;
> +    DEVPROPTYPE cm_type;
> +
> +    device_pci_re = g_regex_new(DEVICE_PCI_RE,
> +        G_REGEX_ANCHORED | G_REGEX_OPTIMIZE, 0,
> +        &gerr);
> +    g_assert(device_pci_re != NULL);
> +
> +    dev_info_data.cbSize = sizeof(SP_DEVINFO_DATA);
> +    dev_info = SetupDiGetClassDevs(0, 0, 0, DIGCF_PRESENT | 
> DIGCF_ALLCLASSES);
> +    if (dev_info == INVALID_HANDLE_VALUE) {
> +        error_setg(errp, "failed to get device tree");
> +        return NULL;
> +    }
> +
> +    slog("enumerating devices");
> +    for (i = 0; SetupDiEnumDeviceInfo(dev_info, i, &dev_info_data); i++) {
> +        bool skip = true;
> +        SYSTEMTIME utc_date;
> +        g_autofree LPWSTR name = NULL;
> +        g_autofree LPFILETIME date = NULL;
> +        g_autofree LPWSTR version = NULL;
> +        g_auto(GStrv) hw_ids = NULL;
> +        g_autoptr(GuestDeviceInfo) device = g_new0(GuestDeviceInfo, 1);
> +        g_autofree char *vendor_id = NULL;
> +        g_autofree char *device_id = NULL;
> +
> +        name = (LPWSTR)cm_get_property(dev_info_data.DevInst,
> +            &qga_DEVPKEY_NAME, &cm_type);
> +        if (name == NULL) {
> +            slog("failed to get device description");
> +            continue;
> +        }
> +        device->driver_name = g_utf16_to_utf8(name, -1, NULL, NULL, NULL);
> +        if (device->driver_name == NULL) {
> +            error_setg(errp, "conversion to utf8 failed (driver name)");
> +            goto out;
> +        }
> +        slog("querying device: %s", device->driver_name);
> +        hw_ids = ga_get_hardware_ids(dev_info_data.DevInst);
> +        if (hw_ids == NULL) {
> +            continue;
> +        }
> +        for (j = 0; hw_ids[j] != NULL; j++) {
> +            GMatchInfo *match_info;
> +            if (!g_regex_match(device_pci_re, hw_ids[j], 0, &match_info)) {
> +                continue;
> +            }
> +            skip = false;
> +            vendor_id = g_match_info_fetch(match_info, 1);
> +            device_id = g_match_info_fetch(match_info, 2);
> +            device->vendor_id = g_ascii_strtoull(vendor_id, NULL, 16);
> +            device->device_id = g_ascii_strtoull(device_id, NULL, 16);
> +            g_match_info_free(match_info);
> +        }
> +        if (skip) {
> +            continue;
> +        }
> +
> +        version = (LPWSTR)cm_get_property(dev_info_data.DevInst,
> +            &qga_DEVPKEY_Device_DriverVersion, &cm_type);
> +        if (version == NULL) {
> +            slog("failed to get driver version");
> +            continue;
> +        }
> +        device->driver_version = g_utf16_to_utf8(version, -1, NULL,
> +            NULL, NULL);
> +        if (device->driver_version == NULL) {
> +            error_setg(errp, "conversion to utf8 failed (driver version)");
> +            goto out;
> +        }
> +
> +        date = (LPFILETIME)cm_get_property(dev_info_data.DevInst,
> +            &qga_DEVPKEY_Device_DriverDate, &cm_type);
> +        if (date == NULL) {
> +            slog("failed to get driver date");
> +            continue;
> +        }
> +        FileTimeToSystemTime(date, &utc_date);
> +        device->driver_date = g_strdup_printf("%04d-%02d-%02d",
> +            utc_date.wYear, utc_date.wMonth, utc_date.wDay);
> +
> +        slog("driver: %s\ndriver version: %s,%s\n", device->driver_name,
> +            device->driver_date, device->driver_version);
> +        item = g_new0(GuestDeviceInfoList, 1);
> +        item->value = g_steal_pointer(&device);
> +        if (!cur_item) {
> +            head = cur_item = item;
> +        } else {
> +            cur_item->next = item;
> +            cur_item = item;
> +        }
> +        continue;
> +    }
> +
> +out:
> +    if (dev_info != INVALID_HANDLE_VALUE) {
> +        SetupDiDestroyDeviceInfoList(dev_info);
> +    }
> +    return head;
> +}
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index fb4605cc19..fe04ff2f39 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -1242,3 +1242,35 @@
>  ##
>  { 'command': 'guest-get-osinfo',
>    'returns': 'GuestOSInfo' }
> +
> +##
> +# @GuestDeviceInfo:
> +#
> +# @vendor-id: vendor ID
> +# @device-id: device ID
> +# @driver-name: name of the associated driver
> +# @driver-date: driver release date in format YYYY-MM-DD
> +# @driver-version: driver version
> +#
> +# Since: 4.2
> +##
> +{ 'struct': 'GuestDeviceInfo',
> +  'data': {
> +      'vendor-id': 'uint16',
> +      'device-id': 'uint16',
> +      'driver-name': 'str',
> +      'driver-date': 'str',
> +      'driver-version': 'str'
> +      } }
> +
> +##
> +# @guest-get-devices:
> +#
> +# Retrieve information about device drivers in Windows guest
> +#
> +# Returns: @GuestDeviceInfo
> +#
> +# Since: 4.2
> +##
> +{ 'command': 'guest-get-devices',
> +  'returns': ['GuestDeviceInfo'] }
> --
> 2.23.0
>
>


other than that,

Reviewed-by: Marc-André Lureau <address@hidden>

-- 
Marc-André Lureau