configure | 2 +- qga/commands-win32.c | 295 +++++++++++++++++++++++++++++++++------------------ qga/qapi-schema.json | 3 +- 3 files changed, 197 insertions(+), 103 deletions(-)
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);
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
Patchew URL: https://patchew.org/QEMU/20190114090324.16176-1-mhines@scalecomputing.com/ Hi, This series seems to have some coding style problems. See output below for more information: Message-id: 20190114090324.16176-1-mhines@scalecomputing.com Subject: [Qemu-devel] [PATCH] QGA: Fix guest-get-fsinfo PCI address collection in Windows Type: series === TEST SCRIPT BEGIN === #!/bin/bash git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram ./scripts/checkpatch.pl --mailback base.. === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 Switched to a new branch 'test' 58d8d80 QGA: Fix guest-get-fsinfo PCI address collection in Windows === OUTPUT BEGIN === WARNING: Block comments should align the * on each line #189: FILE: qga/commands-win32.c:592: + /* + * CM API used here as opposed to WARNING: line over 80 characters #293: FILE: qga/commands-win32.c:661: + /* There is no need to allocate buffer in the next functions. The size WARNING: Block comments use a leading /* on a separate line #293: FILE: qga/commands-win32.c:661: + /* There is no need to allocate buffer in the next functions. The size WARNING: Block comments should align the * on each line #294: FILE: qga/commands-win32.c:662: + /* There is no need to allocate buffer in the next functions. The size + * is known and ULONG according to ERROR: spaces required around that '&' (ctx:VxV) #299: FILE: qga/commands-win32.c:667: + &type, (PBYTE)&bus, size, NULL)) { ^ WARNING: Block comments use a leading /* on a separate line #317: FILE: qga/commands-win32.c:673: + /* The function retrieves the device's address. This value will be WARNING: Block comments use a trailing */ on a separate line #318: FILE: qga/commands-win32.c:674: + * transformed into device function and number */ WARNING: Block comments should align the * on each line #318: FILE: qga/commands-win32.c:674: + /* The function retrieves the device's address. This value will be + * transformed into device function and number */ ERROR: spaces required around that '&' (ctx:VxV) #321: FILE: qga/commands-win32.c:677: + &type, (PBYTE)&addr, size, NULL)) { ^ WARNING: Block comments use a leading /* on a separate line #327: FILE: qga/commands-win32.c:683: + /* This call returns UINumber of DEVICE_CAPABILITIES structure. WARNING: Block comments use a trailing */ on a separate line #328: FILE: qga/commands-win32.c:684: + * This number is typically a user-perceived slot number. */ WARNING: Block comments should align the * on each line #328: FILE: qga/commands-win32.c:684: + /* This call returns UINumber of DEVICE_CAPABILITIES structure. + * This number is typically a user-perceived slot number. */ ERROR: spaces required around that '&' (ctx:VxV) #331: FILE: qga/commands-win32.c:687: + &type, (PBYTE)&slot, size, NULL)) { ^ WARNING: Block comments use a leading /* on a separate line #337: FILE: qga/commands-win32.c:693: + /* SetupApi gives us the same information as driver with WARNING: Block comments should align the * on each line #338: FILE: qga/commands-win32.c:694: + /* SetupApi gives us the same information as driver with + * IoGetDeviceProperty. According to Microsoft WARNING: Block comments use a trailing */ on a separate line #342: FILE: qga/commands-win32.c:698: + * SPDRP_ADDRESS is propertyAddress, so we do the same.*/ total: 3 errors, 13 warnings, 391 lines checked Commit 58d8d800f31a (QGA: Fix guest-get-fsinfo PCI address collection in Windows) has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. === OUTPUT END === Test command exited with code: 1 The full log is available at http://patchew.org/logs/20190114090324.16176-1-mhines@scalecomputing.com/testing.checkpatch/?type=message. --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-devel@redhat.com
On 1/14/19 3:03 AM, mhines@scalecomputing.com wrote: > From: Matt Hines <mhines@scalecomputing.com> > > Signed-off-by: Matt Hines <mhines@scalecomputing.com> The title says what (a fix), but no description of that fix or a "why" in the commit body. > --- > configure | 2 +- > qga/commands-win32.c | 295 +++++++++++++++++++++++++++++++++------------------ > qga/qapi-schema.json | 3 +- > 3 files changed, 197 insertions(+), 103 deletions(-) > +++ 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) Adding a member is more than just a fix. Also, this is missing a '(since 4.0)' tag. > # > # 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: > -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Number here is specific to the device type (and otherwise opaque) unfortunately per this IOCtl: https://docs.microsoft.com/en-us/windows/desktop/api/winioctl/ni-winioctl-ioctl_storage_get_device_number Should I put a comment in qapi-schema.json pointing to this? From: Eric Blake Sent: Monday, January 14, 2019 11:41 To: mhines@scalecomputing.com; qemu-devel@nongnu.org Cc: Michael Roth Subject: Re: [Qemu-devel] [PATCH] QGA: Fix guest-get-fsinfo PCI addresscollection in Windows On 1/14/19 3:03 AM, mhines@scalecomputing.com wrote: > From: Matt Hines <mhines@scalecomputing.com> > > Signed-off-by: Matt Hines <mhines@scalecomputing.com> The title says what (a fix), but no description of that fix or a "why" in the commit body. > --- > configure | 2 +- > qga/commands-win32.c | 295 +++++++++++++++++++++++++++++++++------------------ > qga/qapi-schema.json | 3 +- > 3 files changed, 197 insertions(+), 103 deletions(-) > +++ 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) Adding a member is more than just a fix. Also, this is missing a '(since 4.0)' tag. > # > # 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: > -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
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 >
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 >
From: Matt Hines <mhines@scalecomputing.com>
The Windows QEMU guest agent erroneously tries to collect PCI information
directly from the physical drive. However, windows stores SCSI/IDE information
with the drive and PCI information with the underlying storage controller
This changes get_pci_info to use the physical drive's underlying storage
controller to get PCI information.
Signed-off-by: Matt Hines <mhines@scalecomputing.com>
---
configure | 2 +-
qga/commands-win32.c | 301 +++++++++++++++++++++++++++++++++------------------
2 files changed, 198 insertions(+), 105 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..200dfbe428 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;
}
@@ -691,7 +783,8 @@ out_free:
return;
}
-static void get_single_disk_info(GuestDiskAddress *disk, Error **errp)
+static void get_single_disk_info(int disk_number,
+ GuestDiskAddress *disk, Error **errp)
{
SCSI_ADDRESS addr, *scsi_ad;
DWORD len;
@@ -720,7 +813,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 +829,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;
@@ -805,7 +898,7 @@ static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp)
disk = g_malloc0(sizeof(GuestDiskAddress));
disk->has_dev = true;
disk->dev = g_strdup(name);
- get_single_disk_info(disk, &local_err);
+ get_single_disk_info(0xffffffff, disk, &local_err);
if (local_err) {
g_debug("failed to get disk info, ignoring error: %s",
error_get_pretty(local_err));
@@ -839,9 +932,9 @@ static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp)
*/
disk->has_dev = true;
disk->dev = g_strdup_printf("\\\\.\\PhysicalDrive%lu",
- extents->Extents[i].DiskNumber);
+ extents->Extents[i].DiskNumber);
- get_single_disk_info(disk, &local_err);
+ get_single_disk_info(extents->Extents[i].DiskNumber, disk, &local_err);
if (local_err) {
error_propagate(errp, local_err);
goto out;
--
2.11.0.windows.1
Patchew URL: https://patchew.org/QEMU/20190115165710.20116-1-mhines@scalecomputing.com/ Hi, This series failed the docker-mingw@fedora build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. === TEST SCRIPT BEGIN === #!/bin/bash time make docker-test-mingw@fedora SHOW_ENV=1 J=14 === TEST SCRIPT END === CC hw/acpi/vmgenid.o CC hw/acpi/acpi_interface.o /tmp/qemu-test/src/block/sheepdog.c: In function 'find_vdi_name': /tmp/qemu-test/src/block/sheepdog.c:1239:5: error: 'strncpy' specified bound 256 equals destination size [-Werror=stringop-truncation] strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ cc1: all warnings being treated as errors --- make: *** Waiting for unfinished jobs.... In function 'acpi_table_install', inlined from 'acpi_table_add' at /tmp/qemu-test/src/hw/acpi/core.c:296:5: /tmp/qemu-test/src/hw/acpi/core.c:184:9: error: 'strncpy' specified bound 4 equals destination size [-Werror=stringop-truncation] strncpy(ext_hdr->sig, hdrs->sig, sizeof ext_hdr->sig); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /tmp/qemu-test/src/hw/acpi/core.c:203:9: error: 'strncpy' specified bound 6 equals destination size [-Werror=stringop-truncation] strncpy(ext_hdr->oem_id, hdrs->oem_id, sizeof ext_hdr->oem_id); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /tmp/qemu-test/src/hw/acpi/core.c:207:9: error: 'strncpy' specified bound 8 equals destination size [-Werror=stringop-truncation] strncpy(ext_hdr->oem_table_id, hdrs->oem_table_id, ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ sizeof ext_hdr->oem_table_id); ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /tmp/qemu-test/src/hw/acpi/core.c:216:9: error: 'strncpy' specified bound 4 equals destination size [-Werror=stringop-truncation] strncpy(ext_hdr->asl_compiler_id, hdrs->asl_compiler_id, ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ sizeof ext_hdr->asl_compiler_id); The full log is available at http://patchew.org/logs/20190115165710.20116-1-mhines@scalecomputing.com/testing.docker-mingw@fedora/?type=message. --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-devel@redhat.com
Patchew URL: https://patchew.org/QEMU/20190115165710.20116-1-mhines@scalecomputing.com/ Hi, This series seems to have some coding style problems. See output below for more information: Message-id: 20190115165710.20116-1-mhines@scalecomputing.com Type: series Subject: [Qemu-devel] [PATCH v2] QGA: Fix guest-get-fsinfo PCI address collection in Windows === TEST SCRIPT BEGIN === #!/bin/bash git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram ./scripts/checkpatch.pl --mailback base.. === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 Switched to a new branch 'test' 85a7940 QGA: Fix guest-get-fsinfo PCI address collection in Windows === OUTPUT BEGIN === WARNING: Block comments should align the * on each line #195: FILE: qga/commands-win32.c:592: + /* + * CM API used here as opposed to WARNING: line over 80 characters #299: FILE: qga/commands-win32.c:661: + /* There is no need to allocate buffer in the next functions. The size WARNING: Block comments use a leading /* on a separate line #299: FILE: qga/commands-win32.c:661: + /* There is no need to allocate buffer in the next functions. The size WARNING: Block comments should align the * on each line #300: FILE: qga/commands-win32.c:662: + /* There is no need to allocate buffer in the next functions. The size + * is known and ULONG according to ERROR: spaces required around that '&' (ctx:VxV) #305: FILE: qga/commands-win32.c:667: + &type, (PBYTE)&bus, size, NULL)) { ^ WARNING: Block comments use a leading /* on a separate line #323: FILE: qga/commands-win32.c:673: + /* The function retrieves the device's address. This value will be WARNING: Block comments use a trailing */ on a separate line #324: FILE: qga/commands-win32.c:674: + * transformed into device function and number */ WARNING: Block comments should align the * on each line #324: FILE: qga/commands-win32.c:674: + /* The function retrieves the device's address. This value will be + * transformed into device function and number */ ERROR: spaces required around that '&' (ctx:VxV) #327: FILE: qga/commands-win32.c:677: + &type, (PBYTE)&addr, size, NULL)) { ^ WARNING: Block comments use a leading /* on a separate line #333: FILE: qga/commands-win32.c:683: + /* This call returns UINumber of DEVICE_CAPABILITIES structure. WARNING: Block comments use a trailing */ on a separate line #334: FILE: qga/commands-win32.c:684: + * This number is typically a user-perceived slot number. */ WARNING: Block comments should align the * on each line #334: FILE: qga/commands-win32.c:684: + /* This call returns UINumber of DEVICE_CAPABILITIES structure. + * This number is typically a user-perceived slot number. */ ERROR: spaces required around that '&' (ctx:VxV) #337: FILE: qga/commands-win32.c:687: + &type, (PBYTE)&slot, size, NULL)) { ^ WARNING: Block comments use a leading /* on a separate line #343: FILE: qga/commands-win32.c:693: + /* SetupApi gives us the same information as driver with WARNING: Block comments should align the * on each line #344: FILE: qga/commands-win32.c:694: + /* SetupApi gives us the same information as driver with + * IoGetDeviceProperty. According to Microsoft WARNING: Block comments use a trailing */ on a separate line #348: FILE: qga/commands-win32.c:698: + * SPDRP_ADDRESS is propertyAddress, so we do the same.*/ total: 3 errors, 13 warnings, 394 lines checked Commit 85a794036de8 (QGA: Fix guest-get-fsinfo PCI address collection in Windows) has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. === OUTPUT END === Test command exited with code: 1 The full log is available at http://patchew.org/logs/20190115165710.20116-1-mhines@scalecomputing.com/testing.checkpatch/?type=message. --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-devel@redhat.com
From: Matt Hines <mhines@scalecomputing.com>
The Windows QEMU guest agent erroneously tries to collect PCI information
directly from the physical drive. However, windows stores SCSI/IDE information
with the drive and PCI information with the underlying storage controller
This changes get_pci_info to use the physical drive's underlying storage
controller to get PCI information.
* Additionally Fixes incorrect size being passed to DeviceIoControl
when getting volume extents. Can occasionally crash the guest agent
Signed-off-by: Matt Hines <mhines@scalecomputing.com>
---
configure | 2 +-
qga/commands-win32.c | 305 +++++++++++++++++++++++++++++++++------------------
2 files changed, 199 insertions(+), 108 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..5f8e797032 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;
}
@@ -691,7 +783,8 @@ out_free:
return;
}
-static void get_single_disk_info(GuestDiskAddress *disk, Error **errp)
+static void get_single_disk_info(int disk_number,
+ GuestDiskAddress *disk, Error **errp)
{
SCSI_ADDRESS addr, *scsi_ad;
DWORD len;
@@ -720,7 +813,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 +829,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;
@@ -784,12 +877,10 @@ static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp)
size = sizeof(VOLUME_DISK_EXTENTS);
extents = g_malloc0(size);
if (!DeviceIoControl(vol_h, IOCTL_VOLUME_GET_VOLUME_DISK_EXTENTS, NULL,
- 0, extents, size, NULL, NULL)) {
+ 0, extents, size, &size, NULL)) {
DWORD last_err = GetLastError();
if (last_err == ERROR_MORE_DATA) {
/* Try once more with big enough buffer */
- size = sizeof(VOLUME_DISK_EXTENTS)
- + extents->NumberOfDiskExtents*sizeof(DISK_EXTENT);
g_free(extents);
extents = g_malloc0(size);
if (!DeviceIoControl(
@@ -805,7 +896,7 @@ static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp)
disk = g_malloc0(sizeof(GuestDiskAddress));
disk->has_dev = true;
disk->dev = g_strdup(name);
- get_single_disk_info(disk, &local_err);
+ get_single_disk_info(0xffffffff, disk, &local_err);
if (local_err) {
g_debug("failed to get disk info, ignoring error: %s",
error_get_pretty(local_err));
@@ -839,9 +930,9 @@ static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp)
*/
disk->has_dev = true;
disk->dev = g_strdup_printf("\\\\.\\PhysicalDrive%lu",
- extents->Extents[i].DiskNumber);
+ extents->Extents[i].DiskNumber);
- get_single_disk_info(disk, &local_err);
+ get_single_disk_info(extents->Extents[i].DiskNumber, disk, &local_err);
if (local_err) {
error_propagate(errp, local_err);
goto out;
--
2.11.0.windows.1
Patchew URL: https://patchew.org/QEMU/20190128223056.19452-1-mhines@scalecomputing.com/ Hi, This series seems to have some coding style problems. See output below for more information: Subject: [Qemu-devel] [PATCH v3] QGA: Fix guest-get-fsinfo PCI address collection in Windows Type: series Message-id: 20190128223056.19452-1-mhines@scalecomputing.com === TEST SCRIPT BEGIN === #!/bin/bash git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram ./scripts/checkpatch.pl --mailback base.. === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 Switched to a new branch 'test' 6805362916 QGA: Fix guest-get-fsinfo PCI address collection in Windows === OUTPUT BEGIN === WARNING: Block comments should align the * on each line #198: FILE: qga/commands-win32.c:592: + /* + * CM API used here as opposed to WARNING: line over 80 characters #302: FILE: qga/commands-win32.c:661: + /* There is no need to allocate buffer in the next functions. The size WARNING: Block comments use a leading /* on a separate line #302: FILE: qga/commands-win32.c:661: + /* There is no need to allocate buffer in the next functions. The size WARNING: Block comments should align the * on each line #303: FILE: qga/commands-win32.c:662: + /* There is no need to allocate buffer in the next functions. The size + * is known and ULONG according to ERROR: spaces required around that '&' (ctx:VxV) #308: FILE: qga/commands-win32.c:667: + &type, (PBYTE)&bus, size, NULL)) { ^ WARNING: Block comments use a leading /* on a separate line #326: FILE: qga/commands-win32.c:673: + /* The function retrieves the device's address. This value will be WARNING: Block comments use a trailing */ on a separate line #327: FILE: qga/commands-win32.c:674: + * transformed into device function and number */ WARNING: Block comments should align the * on each line #327: FILE: qga/commands-win32.c:674: + /* The function retrieves the device's address. This value will be + * transformed into device function and number */ ERROR: spaces required around that '&' (ctx:VxV) #330: FILE: qga/commands-win32.c:677: + &type, (PBYTE)&addr, size, NULL)) { ^ WARNING: Block comments use a leading /* on a separate line #336: FILE: qga/commands-win32.c:683: + /* This call returns UINumber of DEVICE_CAPABILITIES structure. WARNING: Block comments use a trailing */ on a separate line #337: FILE: qga/commands-win32.c:684: + * This number is typically a user-perceived slot number. */ WARNING: Block comments should align the * on each line #337: FILE: qga/commands-win32.c:684: + /* This call returns UINumber of DEVICE_CAPABILITIES structure. + * This number is typically a user-perceived slot number. */ ERROR: spaces required around that '&' (ctx:VxV) #340: FILE: qga/commands-win32.c:687: + &type, (PBYTE)&slot, size, NULL)) { ^ WARNING: Block comments use a leading /* on a separate line #346: FILE: qga/commands-win32.c:693: + /* SetupApi gives us the same information as driver with WARNING: Block comments should align the * on each line #347: FILE: qga/commands-win32.c:694: + /* SetupApi gives us the same information as driver with + * IoGetDeviceProperty. According to Microsoft WARNING: Block comments use a trailing */ on a separate line #351: FILE: qga/commands-win32.c:698: + * SPDRP_ADDRESS is propertyAddress, so we do the same.*/ total: 3 errors, 13 warnings, 407 lines checked Commit 680536291660 (QGA: Fix guest-get-fsinfo PCI address collection in Windows) has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. === OUTPUT END === Test command exited with code: 1 The full log is available at http://patchew.org/logs/20190128223056.19452-1-mhines@scalecomputing.com/testing.checkpatch/?type=message. --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-devel@redhat.com
ping From: no-reply@patchew.org Sent: Thursday, January 31, 2019 9:53 To: mhines@scalecomputing.com Cc: fam@euphon.net; qemu-devel@nongnu.org; mhines@scalecomputing.com; mdroth@linux.vnet.ibm.com Subject: Re: [Qemu-devel] [PATCH v3] QGA: Fix guest-get-fsinfo PCI addresscollection in Windows Patchew URL: https://patchew.org/QEMU/20190128223056.19452-1-mhines@scalecomputing.com/ Hi, This series seems to have some coding style problems. See output below for more information: Subject: [Qemu-devel] [PATCH v3] QGA: Fix guest-get-fsinfo PCI address collection in Windows Type: series Message-id: 20190128223056.19452-1-mhines@scalecomputing.com === TEST SCRIPT BEGIN === #!/bin/bash git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram ./scripts/checkpatch.pl --mailback base.. === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 Switched to a new branch 'test' 6805362916 QGA: Fix guest-get-fsinfo PCI address collection in Windows === OUTPUT BEGIN === WARNING: Block comments should align the * on each line #198: FILE: qga/commands-win32.c:592: + /* + * CM API used here as opposed to WARNING: line over 80 characters #302: FILE: qga/commands-win32.c:661: + /* There is no need to allocate buffer in the next functions. The size WARNING: Block comments use a leading /* on a separate line #302: FILE: qga/commands-win32.c:661: + /* There is no need to allocate buffer in the next functions. The size WARNING: Block comments should align the * on each line #303: FILE: qga/commands-win32.c:662: + /* There is no need to allocate buffer in the next functions. The size + * is known and ULONG according to ERROR: spaces required around that '&' (ctx:VxV) #308: FILE: qga/commands-win32.c:667: + &type, (PBYTE)&bus, size, NULL)) { ^ WARNING: Block comments use a leading /* on a separate line #326: FILE: qga/commands-win32.c:673: + /* The function retrieves the device's address. This value will be WARNING: Block comments use a trailing */ on a separate line #327: FILE: qga/commands-win32.c:674: + * transformed into device function and number */ WARNING: Block comments should align the * on each line #327: FILE: qga/commands-win32.c:674: + /* The function retrieves the device's address. This value will be + * transformed into device function and number */ ERROR: spaces required around that '&' (ctx:VxV) #330: FILE: qga/commands-win32.c:677: + &type, (PBYTE)&addr, size, NULL)) { ^ WARNING: Block comments use a leading /* on a separate line #336: FILE: qga/commands-win32.c:683: + /* This call returns UINumber of DEVICE_CAPABILITIES structure. WARNING: Block comments use a trailing */ on a separate line #337: FILE: qga/commands-win32.c:684: + * This number is typically a user-perceived slot number. */ WARNING: Block comments should align the * on each line #337: FILE: qga/commands-win32.c:684: + /* This call returns UINumber of DEVICE_CAPABILITIES structure. + * This number is typically a user-perceived slot number. */ ERROR: spaces required around that '&' (ctx:VxV) #340: FILE: qga/commands-win32.c:687: + &type, (PBYTE)&slot, size, NULL)) { ^ WARNING: Block comments use a leading /* on a separate line #346: FILE: qga/commands-win32.c:693: + /* SetupApi gives us the same information as driver with WARNING: Block comments should align the * on each line #347: FILE: qga/commands-win32.c:694: + /* SetupApi gives us the same information as driver with + * IoGetDeviceProperty. According to Microsoft WARNING: Block comments use a trailing */ on a separate line #351: FILE: qga/commands-win32.c:698: + * SPDRP_ADDRESS is propertyAddress, so we do the same.*/ total: 3 errors, 13 warnings, 407 lines checked Commit 680536291660 (QGA: Fix guest-get-fsinfo PCI address collection in Windows) has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. === OUTPUT END === Test command exited with code: 1 The full log is available at http://patchew.org/logs/20190128223056.19452-1-mhines@scalecomputing.com/testing.checkpatch/?type=message. --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-devel@redhat.com
© 2016 - 2024 Red Hat, Inc.