[vioscsi] vioscsi misses last target and last LUN #1442#1443
[vioscsi] vioscsi misses last target and last LUN #1442#1443hrosenfeld wants to merge 1 commit intovirtio-win:masterfrom
Conversation
|
[Jenkins CI]: Can one of the admins verify this patch? |
Summary of ChangesHello @hrosenfeld, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a compliance issue with the virtio specification in the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly fixes an off-by-one error in the validation of TargetId and Lun, which prevented access to the last target and LUN as reported in issue #1442. The change aligns with the virtio specification where max_target and max_lun are inclusive. While reviewing this change, I identified a similar potential bug in the SRB_PATH_ID check on the same line and have provided a suggestion to correct it as well.
| if ((SRB_PATH_ID(Srb) > (UCHAR)adaptExt->num_queues) || (TargetId > adaptExt->scsi_config.max_target) || | ||
| (Lun > adaptExt->scsi_config.max_lun) || adaptExt->bRemoved) |
There was a problem hiding this comment.
While you're fixing the boundary checks for TargetId and Lun, there appears to be a similar issue with the SRB_PATH_ID check.
- The check
SRB_PATH_ID(Srb) > adaptExt->num_queuesshould use>=instead of>. Ifnum_queuesis N, valid path IDs are 0 to N-1. The current check with>incorrectly allows an invalid path ID of N. - The cast
(UCHAR)adaptExt->num_queuesis unsafe.adaptExt->num_queuesis aULONGand can be up toMAX_CPU(256). If it is 256,(UCHAR)256truncates to 0, which would incorrectly reject all valid path IDs. The cast should be removed for a correct comparison.
I've included a suggestion to fix this.
if ((SRB_PATH_ID(Srb) >= adaptExt->num_queues) || (TargetId > adaptExt->scsi_config.max_target) ||
(Lun > adaptExt->scsi_config.max_lun) || adaptExt->bRemoved)
There was a problem hiding this comment.
Should probably implement this, yes?
|
ok to test |
|
@hrosenfeld Driver binary available on Dropbox of HCK-CI results. |
Thanks a lot! I've installed this driver on my Windows VM, but it seems my change isn't enough to fix the issue with detecting the last target. I wonder what other changes may be necessary to the driver to make this work, but I don't really know anything about Windows internals. Perhaps there's a mismatch between max_target as given by the virtio-scsi device and how Windows understands ConfigInfo->MaximumNumberOfTargets? |
|
According to this: https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/storport/ns-storport-_port_configuration_information MaximumNumberOfTargets is a UCHAR, and the maximum valid value is 255, which is also the default. But that document doesn't really explain how it's understood: Does a value of X mean there's X targets, with id's ranging from 0 to X-1? My VM has two targets (0 and 1), and as the virtio spec says bhyve sets max_target to 1. If Windows understands this as "there can be no more than 1 target", it would explain why it only sees target 0. I guess it would be worth to try changing the setting of MaximumNumberOfTargets for the next test. I only wonder what a value of 0 means. Does it mean "zero targets" or "256 targets"? If it's the former, it would imply Windows couldn't ever possibly address target 255... |
|
@vrozenfe Can you please take a look? Thanks. |
hrosenfeld
force-pushed
the
master
branch
2 times, most recently
from
d6cfba2 to
8e4218f
Compare
|
I've updated the change to set MaximumNumberOfTargets to max_target+1 and MaximumNumberOfLogicalUnits to max_lun+1. Not sure if this is correct, but I can test it if you can kindly provide me with a built driver binary again. |
|
rerun tests |
|
@hrosenfeld Please fix clang format. The driver binary will be available after the full build finishes in HCK-CI results. |
Done. |
|
@hrosenfeld Did you test the latest changes? |
Sorry, I was quite busy the past days. I did test them now. My test VM has two virtio-scsi targets configured with ids 0 and 1, max_target is set to 1 accordingly. With the latest patched driver, Win2022 is able to see and use both targets. |
|
Is there anything else I need to do to move this forward? |
|
Ping? |
| adaptExt->max_physical_breaks = adaptExt->scsi_config.max_sectors * SECTOR_SIZE / PAGE_SIZE; | ||
| } | ||
|
|
||
| adaptExt->max_physical_breaks = min(adaptExt->scsi_config.seg_max, adaptExt->max_physical_breaks); |
There was a problem hiding this comment.
This would perhaps only be appropriate in certain circumstances, and I'm fairly certain this would break most implementations... It might be best to just remove this addition and keep your PR focused on the target problem.
Notwithstanding the above, perhaps consider for informational purposes the following code from an old WIP to see what other options there might also be:
if (!adaptExt->dump_mode)
{
/* Allow user to override max_segments via reg key
* [HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Services\vioscsi\Parameters\Device]
* "MaxPhysicalSegments"={dword value here}
* OR the legacy value:
* "PhysicalBreaks"={dword value here}
*
* ATTENTION : This should be the maximum number of memory pages (typ. 4KiB each) in a transfer
* Equivalent to any of the following:
* NumberOfPhysicalBreaks - 1 (NOPB includes known off-by-one error)
* VIRTIO_MAX_SG - 1
* MaximumSGList - 1 (SCSI Port legacy value)
*
* ATTENTION : This should be the same as the max_segments value of the backing device.
*
* WARNING : This is adapter-wide. Using disks with different max_segments values will
* result in sub-optimal performance.
*/
if ((VioScsiReadRegistryParameter(DeviceExtension,
REGISTRY_MAX_PH_BREAKS,
FIELD_OFFSET(ADAPTER_EXTENSION, max_segments))) ||
(VioScsiReadRegistryParameter(DeviceExtension,
REGISTRY_MAX_PH_SEGMENTS,
FIELD_OFFSET(ADAPTER_EXTENSION, max_segments))))
{
/* Grab the maximum SEGMENTS value from the registry */
RhelDbgPrint(TRACE_LEVEL_VERBOSE,
" max_segments candidate was specified in the registry : %lu \n",
adaptExt->max_segments);
}
else
{
/* Grab the VirtIO reported maximum SEGMENTS value from the HBA and put it somewhere mutable */
adaptExt->max_segments = adaptExt->scsi_config.seg_max;
RhelDbgPrint(TRACE_LEVEL_VERBOSE,
" max_segments candidate was NOT specified in the registry. We will attempt to derive the "
"value...\n");
}
/* Use our maximum SEGMENTS value OR use PHYS_SEGMENTS... */
if (adaptExt->indirect)
{
max_segs_candidate[1] = adaptExt->max_segments;
RhelDbgPrint(TRACE_LEVEL_VERBOSE,
" max_segments candidate derived from MAX SEGMENTS (reported by QEMU/KVM) as "
"VIRTIO_RING_F_INDIRECT_DESC is ON: %lu \n",
max_segs_candidate[1]);
}
else
{
max_segs_candidate[1] = PHYS_SEGMENTS;
RhelDbgPrint(TRACE_LEVEL_VERBOSE,
" max_segments candidate derived from PHYS_SEGMENTS as VIRTIO_RING_F_INDIRECT_DESC is "
"OFF: %lu \n",
max_segs_candidate[1]);
}
/* Grab the VirtIO reported maximum SECTORS value from the HBA to start with */
max_segs_candidate[2] = (adaptExt->scsi_config.max_sectors * SECTOR_SIZE) / PAGE_SIZE;
RhelDbgPrint(TRACE_LEVEL_VERBOSE,
" max_segments candidate derived from %lu total sectors "
"(QEMU/KVM hint per VirtIO standard) : %lu \n",
adaptExt->scsi_config.max_sectors,
max_segs_candidate[2]);
/* Choose the best candidate... */
if (max_segs_candidate[1] == max_segs_candidate[2])
{
/* Start with a comparison of equality */
max_segs_candidate[0] = max_segs_candidate[1];
RhelDbgPrint(TRACE_LEVEL_VERBOSE,
" max_segs_candidate[0] : init - the candidates were the same value : %lu \n",
max_segs_candidate[0]);
}
else if ((max_segs_candidate[2] > 0) && (max_segs_candidate[2] < PHYS_SEGMENTS_LIMIT))
{
/* Use the value derived from the QEMU/KVM hint if it is below the PHYS_SEGMENTS_LIMIT */
max_segs_candidate[0] = max_segs_candidate[2];
RhelDbgPrint(TRACE_LEVEL_VERBOSE,
" max_segs_candidate[0] : init - the QEMU/KVM hint method (scsi_config.max_sectors "
"= %lu) was used to select the candidate : %lu \n",
adaptExt->scsi_config.max_sectors,
max_segs_candidate[0]);
}
else
{
/* Take the smallest candidate */
max_segs_candidate[0] = min((max_segs_candidate[1]), (max_segs_candidate[2]));
RhelDbgPrint(TRACE_LEVEL_VERBOSE,
" max_segs_candidate[0] : init - the smallest candidate was selected : %lu \n",
max_segs_candidate[0]);
}
/* Check the value is within SG list bounds */
max_segs_candidate[0] = min(max(SCSI_MINIMUM_PHYSICAL_BREAKS, max_segs_candidate[0]), (VIRTIO_MAX_SG - 1));
RhelDbgPrint(TRACE_LEVEL_VERBOSE,
" max_segs_candidate[0] : within SG list bounds (%lu) : %lu\n",
(VIRTIO_MAX_SG - 1),
max_segs_candidate[0]);
/* Check the value is within physical bounds */
max_segs_candidate[0] = min(max(SCSI_MINIMUM_PHYSICAL_BREAKS, max_segs_candidate[0]), PHYS_SEGMENTS_LIMIT);
RhelDbgPrint(TRACE_LEVEL_VERBOSE,
" max_segs_candidate[0] : within physical bounds (%lu) : %lu\n",
PHYS_SEGMENTS_LIMIT,
max_segs_candidate[0]);
/* Update max_segments with preferred candidate */
adaptExt->max_segments = max_segs_candidate[0];
}
/* Here we enforce legacy off-by-one NumberOfPhysicalBreaks (NOPB) behaviour for StorPort.
* This behaviour was retained in StorPort to maintain backwards compatibility.
* This is analogous to the legacy MaximumSGList parameter in the SCSI Port driver.
* Where:
* MaximumSGList = ((MAX_BLOCK_SIZE)/PAGE_SIZE) + 1
* The default x86/x64 values being:
* MaximumSGList = (64KiB/4KiB) + 1 = 16 + 1 = 17 (0x11)
* The MAX_BLOCK_SIZE limit is no longer 64KiB, but 2048KiB (2MiB):
* NOPB or MaximumSGList = (2048KiB/4KiB) + 1 = 512 + 1 = 513 (0x201)
*
* ATTENTION: The MS NOPB documentation for both the SCSI Port and StorPort drivers is incorrect.
*
* As max_segments = MAX_BLOCK_SIZE/PAGE_SIZE we use:
*/
ConfigInfo->NumberOfPhysicalBreaks = adaptExt->max_segments + 1;
/* Here we use the efficient single step calculation for MaximumTransferLength
*
* The alternative would be:
* ConfigInfo->MaximumTransferLength = adaptExt->max_segments;
* ConfigInfo->MaximumTransferLength <<= PAGE_SHIFT;
* ...where #define PAGE_SHIFT 12L
*
*/
ConfigInfo->MaximumTransferLength = adaptExt->max_segments * PAGE_SIZE;
RhelDbgPrint(TRACE_LEVEL_INFORMATION,
" adaptExt->max_segments : 0x%x (%lu) | ConfigInfo->NumberOfPhysicalBreaks : 0x%x (%lu) | "
"ConfigInfo->MaximumTransferLength : 0x%x (%lu) Bytes (%lu KiB) \n",
adaptExt->max_segments,
adaptExt->max_segments,
ConfigInfo->NumberOfPhysicalBreaks,
ConfigInfo->NumberOfPhysicalBreaks,
ConfigInfo->MaximumTransferLength,
ConfigInfo->MaximumTransferLength,
(ConfigInfo->MaximumTransferLength / 1024));There was a problem hiding this comment.
This would perhaps only be appropriate in certain circumstances, and I'm fairly certain this would break most implementations... It might be best to just remove this addition and keep your PR focused on the target problem.
I would like to know more. How can this break most implementations, when the spec says pretty clearly that with seg_max a device specifies the number of segments that can be in a command?
See VirtIO spec 1.2, section 5.6.4:
" seg_max
is the maximum number of segments that can be in a command. A bidirectional command can include seg_max input segments and seg_max output segments."
If you send more than the device announced it will handle, you can expect the device to be rightfully upset. bhyve most certainly is.
That being said, I don't mind keeping this separate from the target problem.
There was a problem hiding this comment.
I would like to know more. How can this break most implementations...
iirc, scsi_config.seg_max and scsi_config.max_sectors are NOT always accurately reported and in some aspects are not even fully implemented. QEMU / KVM haven't historically determined the proper max_segments value of the underlying backing. It can be a difficult problem to solve where there are multiple layers of abstraction before hitting physical disk.
The scsi_config.max_sectors is supposed to be the hint provided for by the standard, but is most often just 0xFFFF... This usually results in the default being 512 (MAX_PHYS_SEGMENTS) in most instances (when VIRTIO_RING_F_INDIRECT_DESC = true). In most cases this provides the best performance anyway.
If a specific (lesser) value for Physical Breaks is required, it is typically implemented via the registry hack. This is also a solution Sys Admins are familiar with.
That being said, I don't mind keeping this separate from the target problem.
That's probably safest and will help to avoid your commit being reverted if there's backlash.
There was a problem hiding this comment.
I just noticed your issue #1456 and am minded to raise a PR for the above from my old WIP.
What I didn't mention is that the registry hack is system wide, so must be set to the lowest value of all disks.
Even my WIP was still only adapter-wide but this depends on implementing a multi-HBA registry feature which has stalled.
I could pick this up again and hopefully that might solve your problem.
As mandated by the virtio spec, adjust the check in VioScsiBuildIo() to allow TargetId and Lun being equal to max_target and max_lun, respectively. Additionally, report max_target+1 and max_lun+1 to Windows as MaximumNumberOfTargets and MaximumNumberOfLogicalUnits, respectively. Signed-off-by: Hans Rosenfeld <rosenfeld@grumpf.hope-2000.org>
|
Hans, I'm not sure if you noticed the Gemini bot's suggestion, but that seems a somewhat important one to fix too... If you would prefer not to, let me know. I can raise a separate PR if that's helpful. |
I've seen it, I thought about for longer than I should have, and then I've decided to leave it as is because it's really not related to the problem of the last target/lun not being detected. But when I did think about what its suggestion, the impression I got from reading the source was that Gemini was most likely wrong. I'm really not that familiar with this code, though, so if you think this is a real problem, please raise a separate PR for it. |
|
What else do I need to do to move this forward and get the fix in? |
Hans, --- a/vioscsi/vioscsi.c
+++ b/vioscsi/vioscsi.c
@@ -393,10 +393,20 @@ VioScsiFindAdapter(IN PVOID DeviceExtension,
GetScsiConfig(DeviceExtension);
SetGuestFeatures(DeviceExtension);
- ConfigInfo->NumberOfBuses = 1;
- ConfigInfo->MaximumNumberOfTargets = min((UCHAR)adaptExt->scsi_config.max_target,
- 255 /*SCSI_MAXIMUM_TARGETS_PER_BUS*/);
- ConfigInfo->MaximumNumberOfLogicalUnits = min((UCHAR)adaptExt->scsi_config.max_lun, SCSI_MAXIMUM_LUNS_PER_TARGET);
+ /* The following *NumberOf* PORT_CONFIGURATION_INFORMATION members are set by values with a zero-based index,
+ * so add one (1) to each: NumberOfBuses, MaximumNumberOfTargets and MaximumNumberOfLogicalUnits.
+ * -----------------------------------------------------------------------------------------------------------
+ * Most hypervisors will report the max_channel value as zero (0) per the VirtIO standard,
+ * but the Storport limit is SCSI_MAXIMUM_BUSES_PER_ADAPTER (255). */
+ ConfigInfo->NumberOfBuses = (UCHAR)min(adaptExt->scsi_config.max_channel + 1, SCSI_MAXIMUM_BUSES_PER_ADAPTER);
+ /* Most hypervisors will report the max_target value as 255 per the VirtIO standard,
+ * and whilst SCSI port used SCSI_MAXIMUM_TARGETS_PER_BUS (128), in Storport the limit is 255. */
+ ConfigInfo->MaximumNumberOfTargets = (UCHAR)min(adaptExt->scsi_config.max_target + 1, 255);
+ /* Most hypervisors will report the max_lun value as 16383 per the VirtIO standard,
+ * but the Storport limit is SCSI_MAXIMUM_LUNS_PER_TARGET (255). */
+ ConfigInfo->MaximumNumberOfLogicalUnits = (UCHAR)min(adaptExt->scsi_config.max_lun + 1,
+ SCSI_MAXIMUM_LUNS_PER_TARGET);
+
ConfigInfo->MaximumTransferLength = SP_UNINITIALIZED_VALUE; // Unlimited
ConfigInfo->NumberOfPhysicalBreaks = SP_UNINITIALIZED_VALUE; // Unlimited
@@ -1303,6 +1313,7 @@ VioScsiBuildIo(IN PVOID DeviceExtension, IN PSCSI_REQUEST_BLOCK Srb)
PSRB_EXTENSION srbExt;
PSTOR_SCATTER_GATHER_LIST sgList;
VirtIOSCSICmd *cmd;
+ UCHAR Channel;
UCHAR TargetId;
UCHAR Lun;
@@ -1310,11 +1321,12 @@ VioScsiBuildIo(IN PVOID DeviceExtension, IN PSCSI_REQUEST_BLOCK Srb)
cdb = SRB_CDB(Srb);
srbExt = SRB_EXTENSION(Srb);
adaptExt = (PADAPTER_EXTENSION)DeviceExtension;
+ Channel = SRB_PATH_ID(Srb);
TargetId = SRB_TARGET_ID(Srb);
Lun = SRB_LUN(Srb);
- if ((SRB_PATH_ID(Srb) > (UCHAR)adaptExt->num_queues) || (TargetId >= adaptExt->scsi_config.max_target) ||
- (Lun >= adaptExt->scsi_config.max_lun) || adaptExt->bRemoved)
+ if ((Channel > adaptExt->scsi_config.max_channel) || (TargetId > adaptExt->scsi_config.max_target) ||
+ (Lun > adaptExt->scsi_config.max_lun) || adaptExt->bRemoved)
{
SRB_SET_SRB_STATUS(Srb, SRB_STATUS_NO_DEVICE);
SRB_SET_DATA_TRANSFER_LENGTH(Srb, 0);Do you think you could apply it to this PR...? Best regards, |
|
Or with better formatting on the comments: --- a/vioscsi/vioscsi.c
+++ b/vioscsi/vioscsi.c
@@ -393,10 +393,23 @@ VioScsiFindAdapter(IN PVOID DeviceExtension,
GetScsiConfig(DeviceExtension);
SetGuestFeatures(DeviceExtension);
- ConfigInfo->NumberOfBuses = 1;
- ConfigInfo->MaximumNumberOfTargets = min((UCHAR)adaptExt->scsi_config.max_target,
- 255 /*SCSI_MAXIMUM_TARGETS_PER_BUS*/);
- ConfigInfo->MaximumNumberOfLogicalUnits = min((UCHAR)adaptExt->scsi_config.max_lun, SCSI_MAXIMUM_LUNS_PER_TARGET);
+ /* The following *NumberOf* PORT_CONFIGURATION_INFORMATION members are set by values with a zero-based index,
+ * so add one (1) to each: NumberOfBuses, MaximumNumberOfTargets and MaximumNumberOfLogicalUnits.
+ * -----------------------------------------------------------------------------------------------------------
+ ** Most hypervisors will report the max_channel value as zero (0) per the VirtIO standard,
+ ** but the Storport limit is SCSI_MAXIMUM_BUSES_PER_ADAPTER (255).
+ **
+ ** Most hypervisors will report the max_target value as 255 per the VirtIO standard,
+ ** and whilst SCSI port used SCSI_MAXIMUM_TARGETS_PER_BUS (128), in Storport the limit is 255.
+ **
+ ** Most hypervisors will report the max_lun value as 16383 per the VirtIO standard,
+ ** but the Storport limit is SCSI_MAXIMUM_LUNS_PER_TARGET (255).
+ */
+ ConfigInfo->NumberOfBuses = (UCHAR)min(adaptExt->scsi_config.max_channel + 1, SCSI_MAXIMUM_BUSES_PER_ADAPTER);
+ ConfigInfo->MaximumNumberOfTargets = (UCHAR)min(adaptExt->scsi_config.max_target + 1, 255);
+ ConfigInfo->MaximumNumberOfLogicalUnits = (UCHAR)min(adaptExt->scsi_config.max_lun + 1,
+ SCSI_MAXIMUM_LUNS_PER_TARGET);
+
ConfigInfo->MaximumTransferLength = SP_UNINITIALIZED_VALUE; // Unlimited
ConfigInfo->NumberOfPhysicalBreaks = SP_UNINITIALIZED_VALUE; // Unlimited
@@ -1303,6 +1316,7 @@ VioScsiBuildIo(IN PVOID DeviceExtension, IN PSCSI_REQUEST_BLOCK Srb)
PSRB_EXTENSION srbExt;
PSTOR_SCATTER_GATHER_LIST sgList;
VirtIOSCSICmd *cmd;
+ UCHAR Channel;
UCHAR TargetId;
UCHAR Lun;
@@ -1310,11 +1324,12 @@ VioScsiBuildIo(IN PVOID DeviceExtension, IN PSCSI_REQUEST_BLOCK Srb)
cdb = SRB_CDB(Srb);
srbExt = SRB_EXTENSION(Srb);
adaptExt = (PADAPTER_EXTENSION)DeviceExtension;
+ Channel = SRB_PATH_ID(Srb);
TargetId = SRB_TARGET_ID(Srb);
Lun = SRB_LUN(Srb);
- if ((SRB_PATH_ID(Srb) > (UCHAR)adaptExt->num_queues) || (TargetId >= adaptExt->scsi_config.max_target) ||
- (Lun >= adaptExt->scsi_config.max_lun) || adaptExt->bRemoved)
+ if ((Channel > adaptExt->scsi_config.max_channel) || (TargetId > adaptExt->scsi_config.max_target) ||
+ (Lun > adaptExt->scsi_config.max_lun) || adaptExt->bRemoved)
{
SRB_SET_SRB_STATUS(Srb, SRB_STATUS_NO_DEVICE);
SRB_SET_DATA_TRANSFER_LENGTH(Srb, 0);Passes SDV... |
|
In relation to: ConfigInfo->NumberOfBuses = (UCHAR)min(adaptExt->scsi_config.max_channel + 1, SCSI_MAXIMUM_BUSES_PER_ADAPTER);There's perhaps an argument we should use this instead: UCHAR max_channels;
if (adaptExt->num_queues > 255)
{
max_channels = SCSI_MAXIMUM_BUSES_PER_ADAPTER;
}
else
{
max_channels = (UCHAR)adaptExt->num_queues;
}
ConfigInfo->NumberOfBuses = min(adaptExt->scsi_config.max_channel + 1, max_channels);Let me know if you would prefer me to raise a new PR and try to push this through. |
Hans, |
Yes, please do. Thanks! |
Thanks Hans. I've finished testing and I'm just waiting for the SDV build to complete before raising the PR. Please do not close this PR - at least not yet. AFAIK, QEMU / KVM does not support using more than one channel, nor a channel ID other than 0. As such, my PR may not get up as proposed. It still works just fine, and permits any hypervisor to use a max_channel value greater than 0 (the VirtIO standard says SHOULD use a value of 0 - and not MUST use), but that might be more flexibility than the maintainers wish to permit. I'm also not aware of any other hypervisor that might permit you to specify a Channel ID other than 0, so it could be this part of my solution is really just dead code. The existing I note my proposal is really just an alternative solution to the issue the bot identified. So it's possible the maintainers might prefer your PR and then I can propose a separate PR to address the So in the end it might be that the maintainers will either:
I'm guessing it will be one of the last two as I am able to build and test the changes. Best regards, |
| 255 /*SCSI_MAXIMUM_TARGETS_PER_BUS*/); | ||
| ConfigInfo->MaximumNumberOfLogicalUnits = min((UCHAR)adaptExt->scsi_config.max_lun, SCSI_MAXIMUM_LUNS_PER_TARGET); | ||
| ConfigInfo->MaximumNumberOfTargets = (UCHAR)min(adaptExt->scsi_config.max_target + 1, | ||
| 255 /*SCSI_MAXIMUM_TARGETS_PER_BUS*/); |
There was a problem hiding this comment.
Sorry, but why?
The fields mean the same think in virtio spec and scsi_config in virtio and in PORT_CONFIGURATION_INFORMATION
What am I missing?
There was a problem hiding this comment.
From MS doc:
_MaximumNumberOfTargets
Number of target peripherals the adapter can control. By default, the value of this member is SCSI_MAXIMUM_TARGETS_PER_BUS. A miniport driver can reset this member to a lesser value if the HBA has more limited capabilities or to a greater value, indicating that the HBA has extended bus capabilities. The maximum value for this member is 255._
From virtio spec
max_channel, max_target and max_lun
can be used by the driver as hints to constrain scanning the logical units on the host to channel/target/logical unit numbers that are less than or equal to the value of the fields. max_channel SHOULD be zero. max_target SHOULD be less than or equal to 255. max_lun SHOULD be less than or equal to 16383.
There was a problem hiding this comment.
To account for when the hypervisor reports max_target as 0 as it MAY because the standard only restricts with SHOULD. In PR #1511 I used:
ConfigInfo->MaximumNumberOfTargets = (UCHAR)min((ULONG)adaptExt->scsi_config.max_target + 1, 255);There was a problem hiding this comment.
The standard also says "max_target SHOULD be less than or equal to 255."
So we should allow for zero for this reason too.
|
|
||
| if ((SRB_PATH_ID(Srb) > (UCHAR)adaptExt->num_queues) || (TargetId >= adaptExt->scsi_config.max_target) || | ||
| (Lun >= adaptExt->scsi_config.max_lun) || adaptExt->bRemoved) | ||
| if ((SRB_PATH_ID(Srb) > (UCHAR)adaptExt->num_queues) || (TargetId > adaptExt->scsi_config.max_target) || |
There was a problem hiding this comment.
That change on other hand is correct.
4 participants
As mandated by the virtio spec, adjust the check in VioScsiBuildIo() to allow TargetId and Lun being equal to max_target and max_lun, respectively.
Should fix #1442, but as I don't have a way to rebuild the drivers for Windows I can't test this change. If someone else would rebuild the driver for me with this change, I can test them on bhyve on illumos and FreeBSD. Alternatively, I can help setting up an appropriate bhyve test environment.