From: Matt Hines <mhines@scalecomputing.com>
To: Michael Roth <mdroth@linux.vnet.ibm.com>,
"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>
Subject: Re: [Qemu-devel] [PATCH] QGA: Fix guest-get-fsinfo PCI address collection inWindows
Date: Thu, 24 Jan 2019 09:59:06 -0800 [thread overview]
Message-ID: <5c49fcec.1c69fb81.43be4.1097@mx.google.com> (raw)
In-Reply-To: <154835257877.21382.11783971245610071087@sif>
Patch v2 removed the device number and added a summary
From: Michael Roth
Sent: Thursday, January 24, 2019 9:56
To: mhines@scalecomputing.com; qemu-devel@nongnu.org
Cc: Matt Hines
Subject: Re: [PATCH] QGA: Fix guest-get-fsinfo PCI address collection inWindows
Quoting mhines@scalecomputing.com (2019-01-14 03:03:23)
> From: Matt Hines <mhines@scalecomputing.com>
>
> Signed-off-by: Matt Hines <mhines@scalecomputing.com>
> ---
> configure | 2 +-
> qga/commands-win32.c | 295 +++++++++++++++++++++++++++++++++------------------
> qga/qapi-schema.json | 3 +-
> 3 files changed, 197 insertions(+), 103 deletions(-)
>
> diff --git a/configure b/configure
> index 5b1d83ea26..46f21c089f 100755
> --- a/configure
> +++ b/configure
> @@ -4694,7 +4694,7 @@ int main(void) {
> EOF
> if compile_prog "" "" ; then
> guest_agent_ntddscsi=yes
> - libs_qga="-lsetupapi $libs_qga"
> + libs_qga="-lsetupapi -lcfgmgr32 $libs_qga"
> fi
> fi
>
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 62e1b51dfe..8c8f3a2c65 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -26,6 +26,7 @@
> #include <winioctl.h>
> #include <ntddscsi.h>
> #include <setupapi.h>
> +#include <cfgmgr32.h>
> #include <initguid.h>
> #endif
> #include <lm.h>
> @@ -491,56 +492,29 @@ static GuestDiskBusType find_bus_type(STORAGE_BUS_TYPE bus)
> return win2qemu[(int)bus];
> }
>
> -/* XXX: The following function is BROKEN!
> - *
> - * It does not work and probably has never worked. When we query for list of
> - * disks we get cryptic names like "\Device\0000001d" instead of
> - * "\PhysicalDriveX" or "\HarddiskX". Whether the names can be translated one
> - * way or the other for comparison is an open question.
> - *
> - * When we query volume names (the original version) we are able to match those
> - * but then the property queries report error "Invalid function". (duh!)
> - */
> -
> -/*
> -DEFINE_GUID(GUID_DEVINTERFACE_VOLUME,
> - 0x53f5630dL, 0xb6bf, 0x11d0, 0x94, 0xf2,
> - 0x00, 0xa0, 0xc9, 0x1e, 0xfb, 0x8b);
> -*/
> DEFINE_GUID(GUID_DEVINTERFACE_DISK,
> 0x53f56307L, 0xb6bf, 0x11d0, 0x94, 0xf2,
> 0x00, 0xa0, 0xc9, 0x1e, 0xfb, 0x8b);
> +DEFINE_GUID(GUID_DEVINTERFACE_STORAGEPORT,
> + 0x2accfe60L, 0xc130, 0x11d2, 0xb0, 0x82,
> + 0x00, 0xa0, 0xc9, 0x1e, 0xfb, 0x8b);
>
> -
> -static GuestPCIAddress *get_pci_info(char *guid, Error **errp)
> +static GuestPCIAddress *get_pci_info(int number, Error **errp)
> {
> HDEVINFO dev_info;
> SP_DEVINFO_DATA dev_info_data;
> - DWORD size = 0;
> + SP_DEVICE_INTERFACE_DATA dev_iface_data;
> + HANDLE dev_file;
> int i;
> - char dev_name[MAX_PATH];
> - char *buffer = NULL;
> GuestPCIAddress *pci = NULL;
> - char *name = NULL;
> bool partial_pci = false;
> +
> pci = g_malloc0(sizeof(*pci));
> pci->domain = -1;
> pci->slot = -1;
> pci->function = -1;
> pci->bus = -1;
>
> - if (g_str_has_prefix(guid, "\\\\.\\") ||
> - g_str_has_prefix(guid, "\\\\?\\")) {
> - name = g_strdup(guid + 4);
> - } else {
> - name = g_strdup(guid);
> - }
> -
> - if (!QueryDosDevice(name, dev_name, ARRAY_SIZE(dev_name))) {
> - error_setg_win32(errp, GetLastError(), "failed to get dos device name");
> - goto out;
> - }
> -
> dev_info = SetupDiGetClassDevs(&GUID_DEVINTERFACE_DISK, 0, 0,
> DIGCF_PRESENT | DIGCF_DEVICEINTERFACE);
> if (dev_info == INVALID_HANDLE_VALUE) {
> @@ -550,90 +524,208 @@ static GuestPCIAddress *get_pci_info(char *guid, Error **errp)
>
> g_debug("enumerating devices");
> dev_info_data.cbSize = sizeof(SP_DEVINFO_DATA);
> + dev_iface_data.cbSize = sizeof(SP_DEVICE_INTERFACE_DATA);
> for (i = 0; SetupDiEnumDeviceInfo(dev_info, i, &dev_info_data); i++) {
> - DWORD addr, bus, slot, data, size2;
> - int func, dev;
> - while (!SetupDiGetDeviceRegistryProperty(dev_info, &dev_info_data,
> - SPDRP_PHYSICAL_DEVICE_OBJECT_NAME,
> - &data, (PBYTE)buffer, size,
> - &size2)) {
> - size = MAX(size, size2);
> - if (GetLastError() == ERROR_INSUFFICIENT_BUFFER) {
> - g_free(buffer);
> - /* Double the size to avoid problems on
> - * W2k MBCS systems per KB 888609.
> - * https://support.microsoft.com/en-us/kb/259695 */
> - buffer = g_malloc(size * 2);
> - } else {
> + PSP_DEVICE_INTERFACE_DETAIL_DATA pdev_iface_detail_data = NULL;
> + STORAGE_DEVICE_NUMBER sdn;
> + char *parent_dev_id = NULL;
> + HDEVINFO parent_dev_info;
> + SP_DEVINFO_DATA parent_dev_info_data;
> + DWORD j;
> + DWORD size = 0;
> +
> + g_debug("getting device path");
> + if (SetupDiEnumDeviceInterfaces(dev_info, &dev_info_data,
> + &GUID_DEVINTERFACE_DISK, 0,
> + &dev_iface_data)) {
> + while (!SetupDiGetDeviceInterfaceDetail(dev_info, &dev_iface_data,
> + pdev_iface_detail_data,
> + size, &size,
> + &dev_info_data)) {
> + if (GetLastError() == ERROR_INSUFFICIENT_BUFFER) {
> + pdev_iface_detail_data = g_malloc(size);
> + pdev_iface_detail_data->cbSize =
> + sizeof(*pdev_iface_detail_data);
> + } else {
> + error_setg_win32(errp, GetLastError(),
> + "failed to get device interfaces");
> + goto free_dev_info;
> + }
> + }
> +
> + dev_file = CreateFile(pdev_iface_detail_data->DevicePath, 0,
> + FILE_SHARE_READ, NULL, OPEN_EXISTING, 0,
> + NULL);
> + g_free(pdev_iface_detail_data);
> +
> + if (!DeviceIoControl(dev_file, IOCTL_STORAGE_GET_DEVICE_NUMBER,
> + NULL, 0, &sdn, sizeof(sdn), &size, NULL)) {
> + CloseHandle(dev_file);
> error_setg_win32(errp, GetLastError(),
> - "failed to get device name");
> + "failed to get device slot number");
> goto free_dev_info;
> }
> - }
>
> - if (g_strcmp0(buffer, dev_name)) {
> - continue;
> + CloseHandle(dev_file);
> + if (sdn.DeviceNumber != number) {
> + continue;
> + }
> + } else {
> + error_setg_win32(errp, GetLastError(),
> + "failed to get device interfaces");
> + goto free_dev_info;
> }
> - g_debug("found device %s", dev_name);
>
> - /* There is no need to allocate buffer in the next functions. The size
> - * is known and ULONG according to
> - * https://support.microsoft.com/en-us/kb/253232
> - * https://msdn.microsoft.com/en-us/library/windows/hardware/ff543095(v=vs.85).aspx
> - */
> - if (!SetupDiGetDeviceRegistryProperty(dev_info, &dev_info_data,
> - SPDRP_BUSNUMBER, &data, (PBYTE)&bus, size, NULL)) {
> - debug_error("failed to get bus");
> - bus = -1;
> - partial_pci = true;
> + g_debug("found device slot %d. Getting storage controller", number);
> + {
> + CONFIGRET cr;
> + DEVINST dev_inst, parent_dev_inst;
> + ULONG dev_id_size = 0;
> +
> + size = 0;
> + while (!SetupDiGetDeviceInstanceId(dev_info, &dev_info_data,
> + parent_dev_id, size, &size)) {
> + if (GetLastError() == ERROR_INSUFFICIENT_BUFFER) {
> + parent_dev_id = g_malloc(size);
> + } else {
> + error_setg_win32(errp, GetLastError(),
> + "failed to get device instance ID");
> + goto out;
> + }
> + }
> +
> + /*
> + * CM API used here as opposed to
> + * SetupDiGetDeviceProperty(..., DEVPKEY_Device_Parent, ...)
> + * which exports are only available in mingw-w64 6+
> + */
> + cr = CM_Locate_DevInst(&dev_inst, parent_dev_id, 0);
> + if (cr != CR_SUCCESS) {
> + g_error("CM_Locate_DevInst failed with code %lx", cr);
> + error_setg_win32(errp, GetLastError(),
> + "failed to get device instance");
> + goto out;
> + }
> + cr = CM_Get_Parent(&parent_dev_inst, dev_inst, 0);
> + if (cr != CR_SUCCESS) {
> + g_error("CM_Get_Parent failed with code %lx", cr);
> + error_setg_win32(errp, GetLastError(),
> + "failed to get parent device instance");
> + goto out;
> + }
> +
> + cr = CM_Get_Device_ID_Size(&dev_id_size, parent_dev_inst, 0);
> + if (cr != CR_SUCCESS) {
> + g_error("CM_Get_Device_ID_Size failed with code %lx", cr);
> + error_setg_win32(errp, GetLastError(),
> + "failed to get parent device ID length");
> + goto out;
> + }
> +
> + ++dev_id_size;
> + if (dev_id_size > size) {
> + g_free(parent_dev_id);
> + parent_dev_id = g_malloc(dev_id_size);
> + }
> +
> + cr = CM_Get_Device_ID(parent_dev_inst, parent_dev_id, dev_id_size,
> + 0);
> + if (cr != CR_SUCCESS) {
> + g_error("CM_Get_Device_ID failed with code %lx", cr);
> + error_setg_win32(errp, GetLastError(),
> + "failed to get parent device ID");
> + goto out;
> + }
> }
>
> - /* The function retrieves the device's address. This value will be
> - * transformed into device function and number */
> - if (!SetupDiGetDeviceRegistryProperty(dev_info, &dev_info_data,
> - SPDRP_ADDRESS, &data, (PBYTE)&addr, size, NULL)) {
> - debug_error("failed to get address");
> - addr = -1;
> - partial_pci = true;
> + g_debug("querying storage controller %s for PCI information",
> + parent_dev_id);
> + parent_dev_info =
> + SetupDiGetClassDevs(&GUID_DEVINTERFACE_STORAGEPORT, parent_dev_id,
> + NULL, DIGCF_PRESENT | DIGCF_DEVICEINTERFACE);
> + g_free(parent_dev_id);
> +
> + if (parent_dev_info == INVALID_HANDLE_VALUE) {
> + error_setg_win32(errp, GetLastError(),
> + "failed to get parent device");
> + goto out;
> }
>
> - /* This call returns UINumber of DEVICE_CAPABILITIES structure.
> - * This number is typically a user-perceived slot number. */
> - if (!SetupDiGetDeviceRegistryProperty(dev_info, &dev_info_data,
> - SPDRP_UI_NUMBER, &data, (PBYTE)&slot, size, NULL)) {
> - debug_error("failed to get slot");
> - slot = -1;
> - partial_pci = true;
> + parent_dev_info_data.cbSize = sizeof(SP_DEVINFO_DATA);
> + if (!SetupDiEnumDeviceInfo(parent_dev_info, 0, &parent_dev_info_data)) {
> + error_setg_win32(errp, GetLastError(),
> + "failed to get parent device data");
> + goto out;
> }
>
> - /* SetupApi gives us the same information as driver with
> - * IoGetDeviceProperty. According to Microsoft
> - * https://support.microsoft.com/en-us/kb/253232
> - * FunctionNumber = (USHORT)((propertyAddress) & 0x0000FFFF);
> - * DeviceNumber = (USHORT)(((propertyAddress) >> 16) & 0x0000FFFF);
> - * SPDRP_ADDRESS is propertyAddress, so we do the same.*/
> -
> - if (partial_pci) {
> - pci->domain = -1;
> - pci->slot = -1;
> - pci->function = -1;
> - pci->bus = -1;
> - } else {
> - func = ((int) addr == -1) ? -1 : addr & 0x0000FFFF;
> - dev = ((int) addr == -1) ? -1 : (addr >> 16) & 0x0000FFFF;
> - pci->domain = dev;
> - pci->slot = (int) slot;
> - pci->function = func;
> - pci->bus = (int) bus;
> + for (j = 0;
> + SetupDiEnumDeviceInfo(parent_dev_info, j, &parent_dev_info_data);
> + j++) {
> + DWORD addr, bus, slot, type;
> + int func, dev;
> +
> + /* There is no need to allocate buffer in the next functions. The size
> + * is known and ULONG according to
> + * https://msdn.microsoft.com/en-us/library/windows/hardware/ff543095(v=vs.85).aspx
> + */
> + if (!SetupDiGetDeviceRegistryProperty(
> + parent_dev_info, &parent_dev_info_data, SPDRP_BUSNUMBER,
> + &type, (PBYTE)&bus, size, NULL)) {
> + debug_error("failed to get PCI bus");
> + bus = -1;
> + partial_pci = true;
> + }
> +
> + /* The function retrieves the device's address. This value will be
> + * transformed into device function and number */
> + if (!SetupDiGetDeviceRegistryProperty(
> + parent_dev_info, &parent_dev_info_data, SPDRP_ADDRESS,
> + &type, (PBYTE)&addr, size, NULL)) {
> + debug_error("failed to get PCI address");
> + addr = -1;
> + partial_pci = true;
> + }
> +
> + /* This call returns UINumber of DEVICE_CAPABILITIES structure.
> + * This number is typically a user-perceived slot number. */
> + if (!SetupDiGetDeviceRegistryProperty(
> + parent_dev_info, &parent_dev_info_data, SPDRP_UI_NUMBER,
> + &type, (PBYTE)&slot, size, NULL)) {
> + debug_error("failed to get PCI slot");
> + slot = -1;
> + partial_pci = true;
> + }
> +
> + /* SetupApi gives us the same information as driver with
> + * IoGetDeviceProperty. According to Microsoft
> + * https://docs.microsoft.com/en-us/windows/desktop/api/setupapi/nf-setupapi-setupdigetdeviceregistrypropertya
> + * FunctionNumber = (USHORT)((propertyAddress) & 0x0000FFFF);
> + * DeviceNumber = (USHORT)(((propertyAddress) >> 16) & 0x0000FFFF);
> + * SPDRP_ADDRESS is propertyAddress, so we do the same.*/
> +
> + if (partial_pci) {
> + pci->domain = -1;
> + pci->slot = -1;
> + pci->function = -1;
> + pci->bus = -1;
> + continue;
> + } else {
> + func = ((int)addr == -1) ? -1 : addr & 0x0000FFFF;
> + dev = ((int)addr == -1) ? -1 : (addr >> 16) & 0x0000FFFF;
> + pci->domain = dev;
> + pci->slot = (int)slot;
> + pci->function = func;
> + pci->bus = (int)bus;
> + break;
> + }
> }
> + SetupDiDestroyDeviceInfoList(parent_dev_info);
> break;
> }
>
> free_dev_info:
> SetupDiDestroyDeviceInfoList(dev_info);
> out:
> - g_free(buffer);
> - g_free(name);
> return pci;
> }
>
> @@ -720,7 +812,7 @@ static void get_single_disk_info(GuestDiskAddress *disk, Error **errp)
> * if that doesn't hold since that suggests some other unexpected
> * breakage
> */
> - disk->pci_controller = get_pci_info(disk->dev, &local_err);
> + disk->pci_controller = get_pci_info(disk->number, &local_err);
> if (local_err) {
> error_propagate(errp, local_err);
> goto err_close;
> @@ -736,7 +828,7 @@ static void get_single_disk_info(GuestDiskAddress *disk, Error **errp)
> /* We are able to use the same ioctls for different bus types
> * according to Microsoft docs
> * https://technet.microsoft.com/en-us/library/ee851589(v=ws.10).aspx */
> - g_debug("getting pci-controller info");
> + g_debug("getting SCSI info");
> if (DeviceIoControl(disk_h, IOCTL_SCSI_GET_ADDRESS, NULL, 0, scsi_ad,
> sizeof(SCSI_ADDRESS), &len, NULL)) {
> disk->unit = addr.Lun;
> @@ -838,8 +930,9 @@ static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp)
> * https://docs.microsoft.com/en-us/windows/desktop/FileIO/naming-a-file#win32-device-namespaces
> */
> disk->has_dev = true;
> + disk->number = extents->Extents[i].DiskNumber;
> disk->dev = g_strdup_printf("\\\\.\\PhysicalDrive%lu",
> - extents->Extents[i].DiskNumber);
> + extents->Extents[i].DiskNumber);
Do we really need an additional OS-specific 'number' field if it is already
encoded in the OS-specific disk->dev path?
Also this part seems like a seperate patch that is unrelated to fixing PCI
address reporting. If the 2 cannot be seperated please explain why in
the commit log.
Also please provide a basic summary of what your patch is doing in the commit
log. We generally expect this for anything other than trivial patches.
Thank you for working on fixing this.
>
> get_single_disk_info(disk, &local_err);
> if (local_err) {
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index c6725b3ec8..0a78fb2e3a 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -836,6 +836,7 @@
> # @unit: unit id
> # @serial: serial number (since: 3.1)
> # @dev: device node (POSIX) or device UNC (Windows) (since: 3.1)
> +# @number: device slot index (Windows)
> #
> # Since: 2.2
> ##
> @@ -843,7 +844,7 @@
> 'data': {'pci-controller': 'GuestPCIAddress',
> 'bus-type': 'GuestDiskBusType',
> 'bus': 'int', 'target': 'int', 'unit': 'int',
> - '*serial': 'str', '*dev': 'str'} }
> + '*serial': 'str', '*dev': 'str', 'number':'int'} }
>
> ##
> # @GuestFilesystemInfo:
> --
> 2.11.0.windows.1
>
next prev parent reply other threads:[~2019-01-24 17:59 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-01-14 9:03 [Qemu-devel] [PATCH] QGA: Fix guest-get-fsinfo PCI address collection in Windows mhines
2019-01-14 9:24 ` no-reply
2019-01-14 19:41 ` Eric Blake
2019-01-14 20:37 ` [Qemu-devel] [PATCH] QGA: Fix guest-get-fsinfo PCI addresscollection " Matt Hines
2019-01-15 16:57 ` [Qemu-devel] [PATCH v2] QGA: Fix guest-get-fsinfo PCI address collection " mhines
2019-01-21 4:31 ` no-reply
2019-01-21 4:42 ` no-reply
2019-01-24 17:56 ` [Qemu-devel] [PATCH] " Michael Roth
2019-01-24 17:59 ` Matt Hines [this message]
2019-01-28 22:30 ` [Qemu-devel] [PATCH v3] " mhines
2019-01-31 17:53 ` no-reply
2019-02-28 1:41 ` [Qemu-devel] [PATCH v3] QGA: Fix guest-get-fsinfo PCI addresscollection " Matt Hines
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5c49fcec.1c69fb81.43be4.1097@mx.google.com \
--to=mhines@scalecomputing.com \
--cc=mdroth@linux.vnet.ibm.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).