Re: [Qemu-devel] [PATCH 2/2] qga-win: fsinfo: pci-info: allow partial info

[Michael Roth]

Quoting Sameeh Jubran (2018-06-26 10:10:38)
> From: Sameeh Jubran <address@hidden>
> 
> The call to SetupDiGetDeviceRegistryProperty might fail because the
> value doesn't exist in the registry, in this case we shouldn't exit from
> the loop but instead continue to look for other available values in the
> registry and set this value as unavailable (-1).
> 
> Signed-off-by: Sameeh Jubran <address@hidden>

The values I'm seeing look off:

win7:

{'execute':'guest-get-fsinfo','arguments':{}}

{"return": [{"name":
"\\\\?\\Volume{2823bc2b-b90f-11e7-9ea5-806e6f6e6963}\\", "total-bytes":
316628992, "mountpoint": "E:\\", "disk": [{"bus-type": "ide", "bus": 0,
"unit": 0, "pci-controller": {"bus": -1, "slot": -1, "domain": -1,
"function": -1}, "target": 0}], "used-bytes": 316628992, "type":
"CDFS"}, {"name":
"\\\\?\\Volume{a1ed8064-3f4a-11e1-972d-806e6f6e6963}\\", "total-bytes":
21367877632, "mountpoint": "C:\\", "disk": [{"bus-type": "scsi", "bus":
0, "unit": 0, "pci-controller": {"bus": -1, "slot": -1, "domain": 0,
"function": 2}, "target": 0}], "used-bytes": 19656269824, "type":
"NTFS"}, {"name":
"\\\\?\\Volume{a1ed8063-3f4a-11e1-972d-806e6f6e6963}\\", "mountpoint":
"System Reserved", "disk": [{"bus-type": "scsi", "bus": 0, "unit": 0,
"pci-controller": {"bus": -1, "slot": -1, "domain": 0, "function": 1},
"target": 0}], "type": "NTFS"}]}

win10:

{'execute':'guest-get-fsinfo','arguments':{}}

{"return": [{"name":
"\\\\?\\Volume{83835b2d-0032-11e6-a84f-806e6f6e6963}\\", "total-bytes":
160755712, "mountpoint": "D:\\", "disk": [{"bus-type": "ide", "bus": 0,
"unit": 0, "pci-controller": {"bus": -1, "slot": -1, "domain": -1,
"function": -1}, "target": 0}], "used-bytes": 160755712, "type":
"CDFS"}, {"name":
"\\\\?\\Volume{2ea839c6-0000-0000-0000-80620c000000}\\", "mountpoint":
"System Reserved", "disk": [{"bus-type": "scsi", "bus": 0, "unit": 0,
"pci-controller": {"bus": -1, "slot": -1, "domain": 0, "function": 3},
"target": 0}], "type": "NTFS"}, {"name":
"\\\\?\\Volume{2ea839c6-0000-0000-0000-501f00000000}\\", "total-bytes":
52665839616, "mountpoint": "C:\\", "disk": [{"bus-type": "scsi", "bus":
0, "unit": 0, "pci-controller": {"bus": -1, "slot": -1, "domain": 0,
"function": 2}, "target": 0}], "used-bytes": 29645811712, "type":
"NTFS"}, {"name":
"\\\\?\\Volume{2ea839c6-0000-0000-0000-100000000000}\\", "mountpoint":
"System Reserved", "disk": [{"bus-type": "scsi", "bus": 0, "unit": 0,
"pci-controller": {"bus": -1, "slot": -1, "domain": 0, "function": 1},
"target": 0}], "type": "NTFS"}]}

If any one of domain/bus/slot/func fail, we should initialize everything
to -1 since we can't partially report a PCI address. The fact that we
get partial success makes me think SPDRP_ADDRESS, SPDRP_UI_NUMBER, etc.
aren't reporting what we expect they should. The documentation seems
a bit nebulous. Also I'm not using multifunction to the non-0 function
values seem off.

Do you know of anyone manually setting CONFIG_QGA_NTDDSCSI in practice?
It's been left disabled in configure (left misnamed as CONFIG_QGA_NTDDDISK
due to some problems that popped up when we tried to enable it that I
don't quite recall, maybe similar issues to what you're seeing). I'm
starting to think it's better to leave this for 3.1 since it's not
technically a supported feature yet and may need some reworking outside
of the issue you were originally addressing.

> ---
>  qga/commands-win32.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index c5f1c884e1..55e460dee3 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -505,7 +505,8 @@ static GuestPCIAddress *get_pci_info(char *guid, Error 
> **errp)
> 
>      dev_info_data.cbSize = sizeof(SP_DEVINFO_DATA);
>      for (i = 0; SetupDiEnumDeviceInfo(dev_info, i, &dev_info_data); i++) {
> -        DWORD addr, bus, slot, func, dev, data, size2;
> +        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,
> @@ -535,21 +536,21 @@ static GuestPCIAddress *get_pci_info(char *guid, Error 
> **errp)
>           */
>          if (!SetupDiGetDeviceRegistryProperty(dev_info, &dev_info_data,
>                     SPDRP_BUSNUMBER, &data, (PBYTE)&bus, size, NULL)) {
> -            break;
> +            bus = -1;
>          }
> 
>          /* 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)) {
> -            break;
> +            addr = -1;
>          }
> 
>          /* 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)) {
> -            break;
> +            slot = -1;
>          }
> 
>          /* SetupApi gives us the same information as driver with
> @@ -559,12 +560,12 @@ static GuestPCIAddress *get_pci_info(char *guid, Error 
> **errp)
>           * DeviceNumber = (USHORT)(((propertyAddress) >> 16) & 0x0000FFFF);
>           * SPDRP_ADDRESS is propertyAddress, so we do the same.*/
> 
> -        func = addr & 0x0000FFFF;
> -        dev = (addr >> 16) & 0x0000FFFF;
> +        func = ((int) addr == -1) ? -1 : addr & 0x0000FFFF;
> +        dev = ((int) addr == -1) ? -1 : (addr >> 16) & 0x0000FFFF;
>          pci->domain = dev;
> -        pci->slot = slot;
> +        pci->slot = (int) slot;
>          pci->function = func;
> -        pci->bus = bus;
> +        pci->bus = (int) bus;
>          break;
>      }
> 
> -- 
> 2.13.6
>