Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

viosock: Remove Coinstaller Stuff from the INF File #1064

Merged
merged 8 commits into from
Jul 29, 2024
Merged

viosock: Remove Coinstaller Stuff from the INF File #1064

merged 8 commits into from
Jul 29, 2024

Conversation

MartinDrab
Copy link
Contributor

@MartinDrab MartinDrab commented Mar 22, 2024 •
edited

The driver is installed in the Driver Store (DIRID 13) instead of the System Drivers directory (DIRID 12).

@MartinDrab MartinDrab force-pushed the feature/viosock/remove-coinstaller branch from 175e5cf to f6c448c Compare March 22, 2024 21:07
@MartinDrab MartinDrab marked this pull request as ready for review March 24, 2024 15:33
@xuehuihui
Copy link
Contributor

Is the protocol not registered?

@MartinDrab
Copy link
Contributor Author

It is not. I think the INFs were not responsible for registering it even before this PR (I did not notice such registrations). I can of course test this (old INFs) on a fresh machine.

@xuehuihui
Copy link
Contributor

[VirtioSocket_Device_CoInstaller_AddReg]
HKR,,CoInstallers32,0x00010000,"viosocklib.dll,ViosockCoInstaller"
==>
The old INFs will register the protocol through the ViosockCoInstaller interface.

@MartinDrab
Copy link
Contributor Author

[VirtioSocket_Device_CoInstaller_AddReg] HKR,,CoInstallers32,0x00010000,"viosocklib.dll,ViosockCoInstaller" ==> The old INFs will register the protocol through the ViosockCoInstaller interface.

Ah, OK, I missed that. I will improve the PR.

@MartinDrab MartinDrab marked this pull request as draft March 25, 2024 14:17
kostyanf14
kostyanf14 reviewed Mar 25, 2024
View reviewed changes
viosock/sys/viosock.inx
@@ -26,7 +26,7 @@ DriverPackageDisplayName = %VirtioSocket.DeviceDesc%
PnpLockdown = 1

[DestinationDirs]
DefaultDestDir = 12
DefaultDestDir = 13
VirtioSocket_Lib_CopyFiles = 11
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to https://learn.microsoft.com/en-us/windows-hardware/drivers/develop/run-from-driver-store#other-files, you should also move VirtioSocket_Lib_CopyFiles to 13. Please check if is this possible or not. This does not work for RNG providers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will look into it, thanks. Yes, I know about RNG.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MartinDrab Please look into changes in #1075
HLK for 2022 and attestation signing fails with DIRID13 and without TargetOS decoration

@MartinDrab MartinDrab changed the title Viosock: Remove Coinstaller Stuff from the INF File [CI-NO-BUILD] Viosock: Remove Coinstaller Stuff from the INF File Apr 30, 2024
@MartinDrab MartinDrab force-pushed the feature/viosock/remove-coinstaller branch 2 times, most recently from cf7e51a to 78422de Compare April 30, 2024 15:26
@MartinDrab MartinDrab changed the title [CI-NO-BUILD] Viosock: Remove Coinstaller Stuff from the INF File Viosock: Remove Coinstaller Stuff from the INF File May 25, 2024
@MartinDrab MartinDrab force-pushed the feature/viosock/remove-coinstaller branch 2 times, most recently from 96d9493 to 33760a4 Compare May 26, 2024 19:34
kostyanf14
kostyanf14 reviewed May 27, 2024
View reviewed changes
viosock/sys/viosock.inx Outdated
@@ -56,6 +55,15 @@ WdfCoInstaller$KMDFCOINSTALLERVERSION$.dll = 1 ; make sure the number matches wi
[VirtioSocket_Device.NT]
CopyFiles=Drivers_Dir,VirtioSocket_Lib_CopyFiles

[VirtioSocket_Device.10.0.15063.Software]
AddSoftware=WSKWSP,,VirtioSocket_Software
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. How do you expect to sign this driver for Windows Server 2016? After applying a Cumulative Update for Windows Server 2016 for x64-based Systems (KB4598243), the latest OS Build is 14393.4169.

  2. According to https://learn.microsoft.com/en-us/windows-hardware/drivers/install/inf-addsoftware-directive: Each AddSoftware directive describes the installation of standalone software. This directive should be used in an INF file of the **SoftwareComponent** setup class.
    How will this work in this INF with Class=System?

Copy link
Contributor Author

@MartinDrab MartinDrab Jun 1, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. It seems that the SoftwareComponent class is not a requirement. For example, INF for NVDA Display Driver on my machine uses the AddSoftware directive but is of class Display.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. well, I suppose there need to be special INF sections for handling OS versions that do not support the AddSoftware directive. I am currently experimenting with this.
    Unfortunately, it is not possible to register the Winsock Provider by just adding certain keys and values into the Registry (like in viorng case); the AddReg directive is not strong enough for that.

viosock/sys/viosock.inx Outdated
SoftwareType=1
SoftwareBinary=viosocklib-test.exe
SoftwareArguments="/i"
SoftwareVersion=1.0.0.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this version be equal to the driver version or the binary version?

@MartinDrab MartinDrab changed the title Viosock: Remove Coinstaller Stuff from the INF File [CI_NO_BUILD]: Viosock: Remove Coinstaller Stuff from the INF File Jun 1, 2024
@MartinDrab MartinDrab changed the title [CI_NO_BUILD]: Viosock: Remove Coinstaller Stuff from the INF File Viosock: Remove Coinstaller Stuff from the INF File Jun 2, 2024
@kostyanf14
Copy link
Member

@MartinDrab

The build failed

C:\EWDK11\Program Files\Microsoft Visual Studio\2019\BuildTools\MSBuild\Microsoft\VC\v160\Microsoft.CppBuild.targets(436,5): error MSB8013: This project doesn't contain the Configuration and Platform combination of Debug|Win32. [C:\workspace\VirtIO-EWDK-11-21H2-SDV\viosock\installer\viosock-installer.vcxproj]

@YanVugenfirer
Copy link
Collaborator

@MartinDrab https://learn.microsoft.com/en-us/windows-hardware/drivers/install/using-an-extension-inf-file did you try to check "Extension INFs"?

@MartinDrab
Copy link
Contributor Author

@MartinDrab https://learn.microsoft.com/en-us/windows-hardware/drivers/install/using-an-extension-inf-file did you try to check "Extension INFs"?

I came across them several days ago when working on another (unrelated) issue. I did not have time to look at them in more detail and possibly use them, however, I hope to get to it shortly.

@kostyanf14
Copy link
Member

@MartinDrab
We merge #1087 to switch to the new EWDK. For now, we just disable viosock build for Win11.

@MartinDrab
Copy link
Contributor Author

@MartinDrab We merge #1087 to switch to the new EWDK. For now, we just disable viosock build for Win11.

OK. I hope to work on this PR shortly and finish it. I apologize for this inconvenience.

@kostyanf14
Copy link
Member

@MartinDrab We merge #1087 to switch to the new EWDK. For now, we just disable viosock build for Win11.

OK. I hope to work on this PR shortly and finish it. I apologize for this inconvenience.

Not a problem. Disabling the build is not a complicated task. This is more problem for companies that release viosock.

@MartinDrab MartinDrab force-pushed the feature/viosock/remove-coinstaller branch 6 times, most recently from ff3e42d to e59ebee Compare July 23, 2024 13:02
@MartinDrab MartinDrab force-pushed the feature/viosock/remove-coinstaller branch 2 times, most recently from a3ede78 to ba85f14 Compare July 23, 2024 13:20
Martin Drab added 4 commits July 23, 2024 06:30
[viosock]: Remove coinstaller stuff from the INF file
932c175 
Signed-Off-By: Martin Drab <martin.drab@virtuozzo.com>
[viosock]: Install driver file to the Driver Store, not the drivers d…
cb9ff8a 
…irectory

Signed-Off-By: Martin Drab <martin.drab@virtuozzo.com>
[viosocklib]: Remove the coinstaller code
d1d2ce5 
Signed-Off-By: Martin Drab <martin.drab@virtuozzo.com>
[viosock]: Remove coinstaller INX files
a575077 
Signed-Off-By: Martin Drab <martin.drab@virtuozzo.com>
@MartinDrab MartinDrab force-pushed the feature/viosock/remove-coinstaller branch from ba85f14 to 6e226b5 Compare July 23, 2024 13:30
@MartinDrab MartinDrab marked this pull request as ready for review July 23, 2024 13:31
@MartinDrab
Copy link
Contributor Author

@MartinDrab We merge #1087 to switch to the new EWDK. For now, we just disable viosock build for Win11.

OK. I hope to work on this PR shortly and finish it. I apologize for this inconvenience.

Not a problem. Disabling the build is not a complicated task. This is more problem for companies that release viosock.

Hello,

I hope I overcame issues regarding the co-installer and the socket WSP installation from the INF file. In the end, I decided to separate the WSP installation into a special service since it is possible to install and start it from the INF file. Other possibilities seem to be problematic:

  • WDK complains about the co-installer even when it is used only on old versions of Windows 10,
  • the AddSoftware directive is not supported on old versions of Windows 10.

I hope this should finally pass the tests and build successfully also with new (E)WDKs.

@MartinDrab MartinDrab force-pushed the feature/viosock/remove-coinstaller branch from 6e226b5 to 8ea8467 Compare July 23, 2024 13:45
@kostyanf14
Copy link
Member

kostyanf14 commented Jul 23, 2024 •
edited

@MartinDrab

Thanks for your work.
Unfortunately, the build failed again.

Please also revert f764600 and update buildAll.bat (ad1aed4 disabled Win11 for viosock)

if errorlevel 1 goto :fail
call tools\build.bat viosock\sys\viosock.vcxproj "Win10_SDV Win11_SDV" %*
if errorlevel 1 goto :fail
call tools\build.bat viosock\wsk\wsk.vcxproj "Win10_SDV Win11_SDV" %*
if errorlevel 1 goto :fail
call tools\build.bat viosock\viosock-wsk-test\viosock-wsk-test.vcxproj "Win10_SDV Win11_SDV" %*

Martin Drab added 3 commits July 23, 2024 12:12
[viosock]: Introduce the WSP service
ed41f62 
This service is responsible for keeping the WSP for the Viosock driver installed as long as it is running (i.e. it installs the WSP during its initialization and removes it on its stop action). This approach seems to be the best/simplest regarding WSP installation from the INF file on Windows 10 since:
- co-installer is no longer supported by newer WDKs,
- the AddSoftware INF directive is not supported by some old (but before EoL) versions of Windows 10,
- the co-installer is prohibited regardless of the Windows version (i.e. the WDK complains even if corresponding INF section is used only on old versions of Windows).

Signed-Off-By: Martin Drab <martin.drab@virtuozzo.com>
[viosock]: Update INXs to use the WSP service
f92b1df 
Signed-Off-By: Martin Drab <martin.drab@virtuozzo.com>
Revert "viosock: workaround disable build in solution for Win11"
385e669 
This reverts commit f764600.
@MartinDrab MartinDrab force-pushed the feature/viosock/remove-coinstaller branch from 8ea8467 to 385e669 Compare July 23, 2024 19:13
[viosock]: Run Win11 SDV
643376e 
Signed-Off-By: Martin Drab <martin.drab@virtuozzo.com>
@MartinDrab MartinDrab changed the title Viosock: Remove Coinstaller Stuff from the INF File viosock: Remove Coinstaller Stuff from the INF File Jul 24, 2024
@YanVugenfirer YanVugenfirer reopened this Jul 29, 2024
@YanVugenfirer YanVugenfirer merged commit e01a90c into virtio-win:master Jul 29, 2024
21 of 24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kostyanf14 kostyanf14 kostyanf14 left review comments

@YanVugenfirer YanVugenfirer YanVugenfirer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@MartinDrab @xuehuihui @kostyanf14 @YanVugenfirer