kvm-guest-drivers-windows
kvm-guest-drivers-windows copied to clipboard
virtio-win
[CI-NO-BUILD] [viostor] Refactor VirtIoHwInitialize()
Shares some definitions with PR #1298. Waiting for that to merge before rebasing.
@benyamin-codez Can you please explain the purpose of this patch? Thanks, Vadim.
@vrozenfe
Can you please explain the purpose of this patch?
The StorPortInitializePerfOpts() part is refactored to make sure the trace has proper KAFFINITY values. The present method instantiates the mask afterwards when it needs to be first, resulting in unset values.
The instrumentation is also improved.
We will also address the find per @MartinCHarvey-Nutanix's post above. On that, isn't string data for Vendor, Device, etc. usually set elsewhere?
@vrozenfe
We will also address the find per @MartinCHarvey-Nutanix's post above. On that, isn't string data for Vendor, Device, etc. usually set elsewhere?
I should have said: On that, isn't string data for Vendor, Device, etc. usually set from elsewhere?
From *.rc or *.ver files...? If so, is there a particular one that should be updated or used?
@vrozenfe
We will also address the find per @MartinCHarvey-Nutanix's post above. On that, isn't string data for Vendor, Device, etc. usually set elsewhere?
I should note on that, the code is probably OK - ie, it's moving the memory without the null terminator, and does not overwrite anything, hence the trailing space. However, someone needs to have a look through the docs somewhere to find out whether the the string should be null terminated, optionally null terminated, or padded with spaces to the end of the (pretty small) amount of space provided. Since "Red Hat" is only 7 chars long, there's room for a null terminator.
Additionally, we should put a compile time assert in on the length of the string so that when some vendor (i.e. us and many others) tried to change some #define without thinking about it, it either barfs, or safely truncates the string.
Since "Red Hat" is only 7 chars long, there's room for a null terminator. ...whether the the string should be null terminated, optionally null terminated, or padded with spaces to the end...
It's presently padded due to VendorId[8].
With "VirtIO" (6) in ProductId[16], so 10 to go.
... and "0001" (4) in ProductRevisionLevel[4] - so all good here. A hint these may not be null terminated.
... and "0001" (4) in VendorSpecific[20], so 16 to go.
...we should put a compile time assert in on the length of the string...
We can also add informative comments to the macros (even though maybe not seen by anyone)... Something like:
// Set Vendor Information - Observe INQUIRYDATA struct length limits
#if !defined(VIRTIO_BLK_VENDOR_ID)
#define VIRTIO_BLK_VENDOR_ID "Red Hat " // UCHAR VendorId[8]
#define VIRTIO_BLK_PRODUCT_ID "VirtIO" // UCHAR ProductId[16]
#define VIRTIO_BLK_PROD_REV_LVL "1001" // UCHAR ProductRevisionLevel[4]
#define VIRTIO_BLK_VEND_SPECIFIC "1001" // UCHAR VendorSpecific[20]
#endif
Although the question remains as to where best to locate these macros... I think maybe elsewhere in the tree... @vrozenfe : Any advice?
And then:
StorPortMoveMemory(..., VIRTIO_BLK_VENDOR_ID, sizeof(((INQUIRYDATA){0}).VendorId));
StorPortMoveMemory(..., VIRTIO_BLK_PRODUCT_ID, sizeof(((INQUIRYDATA){0}).ProductId));
StorPortMoveMemory(..., VIRTIO_BLK_PROD_REV_LVL, sizeof(((INQUIRYDATA){0}).ProductRevisionLevel));
StorPortMoveMemory(..., VIRTIO_BLK_VEND_SPECIFIC, sizeof(((INQUIRYDATA){0}).VendorSpecific));
...?
EDIT: Added 0 to sizeof(((INQUIRYDATA){}).VendorId) ==> sizeof(((INQUIRYDATA){0}).VendorId)...
Something like:
// Set Vendor Information - Observe INQUIRYDATA struct length limits #if !defined(VIRTIO_BLK_VENDOR_ID) #define VIRTIO_BLK_VENDOR_ID "Red Hat " // UCHAR VendorId[8] ... #endif
So I presume these will all be const lstr and so null terminated. Therefore, we will likely need to trim.
In the past I've just used something much like CopyBufferToAnsiString() but left off adding \0 after copy:
USHORT CopyBufferAsUnterminatedString(void *_pDest, const void *_pSrc, const char delimiter, size_t _maxlength)
{
PCHAR dst = (PCHAR)_pDest;
PCHAR src = (PCHAR)_pSrc;
USHORT _length = _maxlength;
while (_length && (*src != delimiter))
{
*dst++ = *src++;
--_length;
};
return _length;
}
That being the solution, we can just use "Red Hat" (or any other value) without the padding.
...and implement like this:
#define VIRTIO_BLK_VENDOR_ID "Red Hat" // UCHAR VendorId[8]
UCHAR trimmed_vendor_id[8] = {0};
CopyBufferAsUnterminatedString(&trimmed_vendor_id, &VIRTIO_BLK_VENDOR_ID, '\0', sizeof(((INQUIRYDATA){0}).VendorId));
StorPortMoveMemory(&adaptExt->inquiry_data.VendorId, trimmed_vendor_id, sizeof(((INQUIRYDATA){0}).VendorId));
@MartinCHarvey-Nutanix
The above will safely truncate the string rather than barf... ¯\_(ツ)_/¯
@vrozenfe @YanVugenfirer
If happy with that approach (you as well as Martin), let me know and I'll split it off to a new PR and let this one be... 8^d
@MartinCHarvey-Nutanix
The above will safely truncate the string rather than barf... ¯\_(ツ)_/¯
No it won't.... Setting VendorId say to 123456789ABCDEF will result in concatenation and 12345678VirtIO...
Leave it with me... q8^{d>
However, someone needs to have a look through the docs somewhere to find out whether the the string should be null terminated, optionally null terminated, or padded with spaces to the end of the (pretty small) amount of space provided.
According to the doco they are padded with ASCII blanks equiv. to 0x20 or "space"...
Only the verbage for ProductId states:
"The data shall be left-aligned within this field and the unused bytes filled with ASCII blanks."
I guess it is implied for the others...
EDIT: That last point is probably unsafe to assume. I'm caught up with some competing priorities over the next few days, so might have to come back to this later - probably in a new PR would be good methinks.
