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

[Sameeh Jubran]

On Mon, Jul 16, 2018 at 11:04 PM, Michael Roth <address@hidden>
wrote:

> 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?
>
Yes, I am using it actually.

> 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.
>
No problem it can wait for 3.1.

>
> > ---
> >  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
> >
>



-- 
Respectfully,
*Sameeh Jubran*
*Linkedin <https://il.linkedin.com/pub/sameeh-jubran/87/747/a8a>*
*Software Engineer @ Daynix <http://www.daynix.com>.*