viostor: Remove DmaWidth initialization in HwStorFindAdapter

[zjmletang]

Summary

Remove the DmaWidth initialization in HwStorFindAdapter to comply with Microsoft Storport driver requirements.

Problem

According to the Microsoft documentation, the DmaWidth field in PORT_CONFIGURATION_INFORMATION is initialized by Storport and miniport drivers must not modify it. However, the current code explicitly sets ConfigInfo->DmaWidth = Width32Bits; in the HwStorFindAdapter routine, which violates this requirement.

Changes

• Removed the line ConfigInfo->DmaWidth = Width32Bits; from virtio_stor.c

Testing

I tested the current behavior (with DmaWidth assignment) on Windows Server 2022 and found that:

• The explicit DmaWidth assignment does not appear to affect 64-bit DMA functionality in practice
• The driver works correctly with 64-bit DMA addressing even when DmaWidth is set to Width32Bits
• No functional issues were observed

However, since:

1. Testing was only performed on Server 2022
2. The MSDN documentation explicitly states that miniport drivers must not modify this field
3. It's better to strictly follow Microsoft's documented requirements to ensure compatibility across all Windows versions

We should remove this assignment to comply with the official documentation.

Reference

• PORT_CONFIGURATION_INFORMATION structure

[YanVugenfirer]

[Jenkins CI]: Can one of the admins verify this patch?

[gemini-code-assist]

Summary of Changes

Hello @zjmletang, I'm Gemini Code Assist¹! 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 within the viostor driver by removing an explicit DmaWidth initialization. The change ensures adherence to Microsoft's Storport driver documentation, which mandates that the Storport driver, not the miniport, manages this field. This proactive adjustment aims to prevent potential compatibility problems across various Windows versions, despite current observations showing no immediate functional impact.

Highlights

• Compliance with Storport Requirements: Removed the explicit initialization of DmaWidth in the HwStorFindAdapter routine to comply with Microsoft Storport driver requirements, which state that this field should be initialized by Storport and not modified by miniport drivers.
• Code Clean-up: The line ConfigInfo->DmaWidth = Width32Bits; was removed from virtio_stor.c.
• No Functional Impact Observed: Testing on Windows Server 2022 indicated that the explicit DmaWidth assignment did not appear to affect 64-bit DMA functionality, and the driver worked correctly even with DmaWidth set to Width32Bits. However, the change is made for strict compliance and future compatibility.

Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request correctly removes the explicit initialization of the DmaWidth field within the HwStorFindAdapter function. As noted in the description, Microsoft's Storport documentation states that miniport drivers must not modify this field. The change aligns the driver with the documented API contract, improving compliance and ensuring better compatibility across different Windows versions. The modification is well-justified and correct.

[YanVugenfirer]

@zjmletang Thank you for the patch.

I have one doubt about the patch - unfortunately we still have to support 32bit version of Windows 10, and I will be more happy to keep the old behavior for 32 bit versions or even for win10 and below in general.

@vrozenfe what do you think?

[zjmletang]

@zjmletang Thank you for the patch.

I have one doubt about the patch - unfortunately we still have to support 32bit version of Windows 10, and I will be more happy to keep the old behavior for 32 bit versions or even for win10 and below in general.

@vrozenfe what do you think?

@YanVugenfirer
Thanks for your concern. I'd like to clarify what was changed:

What was removed:

• ConfigInfo->DmaWidth = Width32Bits;

What was kept:

• ConfigInfo->Dma32BitAddresses = TRUE; (still present)
• ConfigInfo->Dma64BitAddresses = SCSI_DMA64_MINIPORT_FULL64BIT_SUPPORTED; (still present)

The change only removes the manual setting of DmaWidth, which according to Microsoft documentation should be initialized by Storport, not by miniport drivers. This field is explicitly documented as "Storport initializes this member and miniport drivers must not modify it."

I initially had the same concern about 32-bit system compatibility, but after reviewing the Microsoft documentation more carefully, I realized that:

• The Dma32BitAddresses and Dma64BitAddresses fields indicate the HBA hardware's DMA addressing capabilities, not the system architecture
• These settings remain unchanged, so the HBA's DMA capabilities are still properly declared
• Storport will automatically initialize DmaWidth with the appropriate value based on the system architecture and will handle DMA addressing accordingly

This change follows Microsoft's documented requirements and shouldn't affect functionality on any system architecture. if I misunderstood something, please let me know and I'm happy to discuss further or adjust the approach.

[zjmletang]

@YanVugenfirer Additionally, I should note that according to the MSDN documentation, Dma32BitAddresses also should not be set by miniport drivers - the documentation explicitly states "Storport initializes this member to TRUE" and "Miniport drivers must not modify this value". However, I chose to keep this assignment because:

1. The value we're setting (TRUE) is always the same as Storport's default initialization value, so there's no practical harm in setting it explicitly
2. Unlike DmaWidth, which could be misleading or potentially harmful (though I didn't observe any issues in my testing), Dma32BitAddresses = TRUE simply confirms what Storport would set anyway

If you prefer strict compliance with the documentation, we could also remove the Dma32BitAddresses assignment, but I believe it's less critical than removing DmaWidth since it doesn't introduce any discrepancy with Storport's default behavior.

[YanVugenfirer]

@zjmletang I understand compliance with documentation, but I also reminder the history of those flags. And they were specifically designed for HW that might have issues with 64 bit addressing. As verifying the effect of something that worked for ages on 32bit systems is hard to estimate, and more over 32bit systems are lower priority and are going to be deprecated anyway, that's why I suggest to make the change only on 64bit systems. Where it is definitely a bug and a remnant on old historical decision.

But in order not to make unknown side effects on 32bit systems, I suggest to use compile time decisions that will be removed later on anyway as we depreciate 32 bit systems.

I also discussed this with @ybendito and @vrozenfe - so this is a unified position from our side.

[zjmletang]

@YanVugenfirer Understood—thanks, I learned a lot! I've updated it with a new version.

[YanVugenfirer]

ok to test

[YanVugenfirer]

@kostyanf14 In Win11_24H2 disk stress test failed, is this something new?

[kostyanf14]

@YanVugenfirer triggered tests

[kostyanf14]

@zjmletang This patch cause BSOD on Win10 22H2 on Disk Stress test https://www.dropbox.com/scl/fo/hry3ofa5x328usfhcya4m/AKWuM6rEFF4Ab5aaq4bK1mU?rlkey=h64olhjkr37btpa3jnxlo6y9o&dl=0&lst=
Please investigate.