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

[vioscsi] Conditionally manage DVL copying #1210

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[vioscsi] Conditionally manage DVL copying #1210

wants to merge 1 commit into from

Conversation

benyamin-codez
Copy link
Contributor

a) Updates the CustomBuildStep Command to conditionally copy COMPAT and Win10 DVL files. This permits COMPAT DVLs with Checksums and Win10 SDV DVLs to be copied if available.
b) Enables Win10_SDV for local buildAll.bat script.

kostyanf14
kostyanf14 reviewed Dec 5, 2024
View reviewed changes
vioscsi/vioscsi.vcxproj Outdated Show resolved Hide resolved
@benyamin-codez benyamin-codez mentioned this pull request Dec 5, 2024
Closed
@benyamin-codez
Copy link
Contributor Author

@kostyanf14

HCK-CI/vioscsi-Win2019x64 (Fedora 34, QEMU v9.0.0) — An error occurred while running HCK-CI

Static Tools Logo Test fails with:

Error: 0x80070002, The system cannot find the file specified.
FindFileAndCopy()::(null)::CAUSE:Cannot Find Pattern "C:\hlk\JobsWorkingDir\Tasks\WTTJobRunB86523D2-E0B3-EF11-A80E-56000200DDDD\Te.wtl.trace"

HCK-CI/vioscsi-Win2025x64_host_uefi (RHEL 9.4, QEMU qemu-kvm-8.2.0-11.el9_4) — 1 tests failed...

Disk Verification (LOGO) fails with:

Error: 0x80070002, The system cannot find the file specified. 
FindFileAndCopy()::(null)::CAUSE:Cannot Find Pattern "C:\HLK\JobsWorkingDir\Tasks\WTTJobRun62FFEB2C-0AB4-EF11-8489-56000200DDDD\wttcmd.wtl.trace"

Any idea what's happening here...?

Also from the code review:

Are you saying Win10 builds also need the Install\Win10\$(TargetArch)\vioscsi.DVL-win10.XML file for HCK?
Even though it is a Windows 10 build...? It made sense to me for Win11 builds, but not Win10.

Should it be the DVL-win10 or the DVL-compat file - or both - that contains the DVL1903 version...?
I had presumed only DVL-compat should have the DVL1903 version.

When C:\DVL1903 does NOT exist:
The error message The system cannot find the path specified. is produced in :VCEnd.
Do you have a process that relies on this particular error message...?

@benyamin-codez
Copy link
Contributor Author

@kostyanf14

HCK-CI/vioscsi-Win2022x64 (Fedora Linux 37, QEMU v9.0.0) — 1 tests failed out of 25 tests

Flush Test fails with:

Error: 0x80070002, The system cannot find the file specified.
FindFileAndCopy()::(null)::CAUSE:Cannot Find Pattern "C:\HLK\JobsWorkingDir\Tasks\WTTJobRunEDEDFC57-41B4-EF11-BD88-56000200DDDD\Te.wtl.trace"

These seem to be the same as previous PRs. Any idea why they're happening? Are they BSOD recoveries maybe?

@kostyanf14
Copy link
Member

@benyamin-codez

Are you saying Win10 builds also need the Install\Win10$(TargetArch)\vioscsi.DVL-win10.XML file for HCK?
Even though it is a Windows 10 build...? It made sense to me for Win11 builds, but not Win10.

Yes. We have different DVL.XML for different Win10. Win10 1507-1809/Server 2019 required another DVL than Win10 2004-21H2/Server 2022. Also, Server 2016 required compact DVL.XML based on Server 2019 DVL (without CodeQL). 3 different DVL.XML for Windows 10.

@kostyanf14
Copy link
Member

Flush Test fails with:

Please ignore the Flush Test results for now. This test was built for real hardware but we created some workaround for VMs. Currently, it is not so stable.

@benyamin-codez
Copy link
Contributor Author

@kostyanf14

Thanks for responding. Almost there... 8^d

Yes. We have different DVL.XML for different Win10. Win10 1507-1809/Server 2019 required another DVL than Win10 2004-21H2/Server 2022. Also, Server 2016 required compact DVL.XML based on Server 2019 DVL (without CodeQL). 3 different DVL.XML for Windows 10.

And from your previous comment:

Also, Windows Server 2016 required a COMPAT version (without checksum) but WITH SDV.

Ok, so this has been my understanding:

DVL.XML file - built with Cobalt EWDK (22000/21H2).
DVL-win10.XML file - built with Cobalt EWDK (22000/21H2).
DVL-compat.XML file - built with Titanium EWDK (1903/19H1).

You have variously mentioned the COMPAT version is without checksum / CodeQL. However, I note the Titanium EWDK does create a DVL with General.Checksum but does not include the Semmle.Summary (from CodeQL), but if the checksum must be stripped for Server 2016, then perhaps...

...it should be the following?

DVL.XML file - built with Cobalt EWDK (22000/21H2).
DVL-win10.XML file - built with Titanium EWDK (1903/19H1).
DVL-compat.XML file - built with Titanium EWDK (1903/19H1) + strip checksum.

...Or something else...?

@benyamin-codez
Copy link
Contributor Author

Perhaps better posted in PR #1212...
...but, FYI, the end of buildAll.bat looks like this on output when SKIP_SDV_ACTUAL=NULL:

virtio-win_dvl-compat_full_with_sdv

@benyamin-codez benyamin-codez marked this pull request as draft December 9, 2024 14:44
@benyamin-codez
Copy link
Contributor Author

@kostyanf14

So I've confirmed with a Redstone 1 EWDK (1607, b14393 = Server2016) DVL build that there's no checksum section.

So it should be:
DVL.XML file - built with Cobalt EWDK (22000/21H2).
DVL-win10.XML file - built with Titanium EWDK (1903/19H1).
DVL-compat.XML file - built with Redstone 1 EWDK (1607).

So we do:
DVL.XML file - built with Cobalt EWDK (22000/21H2).
DVL-win10.XML file - built with Titanium EWDK (1903/19H1).
DVL-compat.XML file - fudged from Cobalt EWDK (22000/21H2).

However, the DVL-compat.XML file can be fudged from a file produced by both Cobalt and Titanium EWDKs, provided there remains no General.Checksum or Semmle.Summary sections. That being said, as a DVL file produced from the Titanium EWDK doesn't have a Semmle.Summary section and associated newline, a fudged DVL-compat.XML file produced from it, won't have a double newline space in the tail, so it is a little tidier.

Let me have a think about this and your comments above, including amongst other things, troubleshooting build and Static Tools Logo Test failures. I'm mindful that any changes need to be of benefit and not hinder existing processes.

@benyamin-codez benyamin-codez mentioned this pull request Dec 11, 2024
Draft
@YanVugenfirer
Copy link
Collaborator

I understand the need to have the ability to create DVL from the local buildall.bat.

But I don't want to change any thing related to DVL copy or manipulation for now. This is legacy stuff, with different manipulations for older OSes that works. Breaking it and debugging will be a headache. I prefer to remove all of it at some point in the future instead of trying to maintain it.

@benyamin-codez
Copy link
Contributor Author

benyamin-codez commented Dec 12, 2024 •
edited
Loading

@YanVugenfirer

But I don't want to change any thing related to DVL copy or manipulation for now. This is legacy stuff, with different manipulations for older OSes that works. Breaking it and debugging will be a headache. I prefer to remove all of it at some point in the future instead of trying to maintain it.

Hold that thought...!

I've redone most of this one and produced a build\makeLegacyDVLs.bat file to handle all the legacy DVL creation. 8^d
This splits off the legacy stuff and should make it easier to remove in the future.

The only functional change I made was to build the DVL-compat.XML file from the RS1 EWDK or fudge it from DVL-win10.XML (Titanium EWDK) if the legacy EWDK is not available. Currently it is fudged from DVL.XML (Cobalt EWDK).
Using the earlier released Titanium EWDK removes the extra CRLF in the tail of the resultant file, making it look identical to one legitimately produced using the RS1 EWDK.

I also added logging for both the CustomBuildStep Command and also for the build\makeLegacyDVLs.bat file.

The former appears in stdout (and the build_log.txt file if built in HCK/CI), and the latter is processed at the tail of root\buildAll.bat along with an indicator of SDV build status (checks whether the SDV folder exists).

This should greatly simplify any DVL troubleshooting for anyone, whether familiar with, or casual to, the build process.

It also removes any ambiguity surrounding errors such as "The system cannot find the path specified." as a reason will be given as to why a DVL component might be missing. These errors are what prompted me to look into this in the first place.

I propose I push the new commits and drop some screenshots here for your (re)consideration.

EDIT:
These changes enable a change in root\buildAll.bat to avoid clobbering DVL-compat.XML files.
Those commits will be in PR #1212.

@benyamin-codez
Copy link
Contributor Author

I've pushed those changes:

Addendum to 7348ad4.

  1. Refactors changes to the CustomBuildStep Command following code review. We now perform all steps regardless of Target OS. We also now create DVL files in the Project (driver) folder with version numbers in the file name for DVL files created with legacy EWDKs, and with the "latest" label for current EWDKs. These files are then copied to the relevant .\Install folders and renamed for extant WHCP submission operations. The elements have also been updated to permit this to be used in other projects, i.e. NetKVM and viostor.
  2. New build\makeLegacyDVLs.bat now creates all legacy DVL files. This functionally replaces build\dvl1903.bat and extends it to service any legacy DVL build. It provides a framework to manage legacy DVL creation and a common place to lay out version-specific DVL operations and manipulations. Logging is also enabled to provide feedback when read at the tail of root\buildAll.bat. Please see PR #1212 for details on these changes.

benyamin-codez added a commit to benyamin-codez/kvm-guest-drivers-windows that referenced this pull request Dec 14, 2024
@benyamin-codez
[build] Fix and extend build scripting
74f5391 
Addendum to da96c89.

1. Further changes to buildAll.bat
   a) Added @ prefix to FOR do commands
   b) Extended logging around creation of COMPAT (RS1/1607) DVL files
   c) Added SDV and Legacy DVL (DVL files created with legacy EWDKs)
      reporting. The latter queries the <PROJECT>.legacy_dvl_result.txt
      log file created by build\makeLegacyDVLs.bat and reports accordingly.
      Please see PR virtio-win#1210 for more information.
   d) Consolidates exiting function (whether success or fail) to :leave
   e) Creates a scheduled task to call build\clean_build_log.bat before leaving.

2. Created build\clean_build_log.bat to remove colour pallette artefacts
   from build_log.txt created during HCK/CI using Jenkins config.
   This script also deletes the scheduled task created in 1(d) above.

Signed-off-by: benyamin-codez <[email protected]>
This was referenced Dec 14, 2024
Draft
Open
@benyamin-codez
Copy link
Contributor Author

@YanVugenfirer @kostyanf14 @vrozenfe

I believe this one is ready to go too.
Wanting to check I've satisfactorily addressed everyone's concerns before switching from draft to Ready for review...
Any checks need to be re-run...?

Promised pics follow... 8^d

Some pics of logging from vioscsi build (implemented by this PR):

  1. Result with SDV
    vioscsi_good_with_sdv

  2. Result without C:\DVL1903 or C:\DVL1607 legacy DVL folders, but with SDV
    vioscsi_bad_no_folders_with_sdv

  3. Result without C:\DVL1607 legacy DVL folder, but with SDV
    vioscsi_bad_no_dvl1607_folder_with_sdv

  4. Result with both legacy DVL folders but no SDV
    vioscsi_bad_no_sdv

Some pics of logging from main buildAll.bat (partially implemented by this PR, enabled by PR #1212):

  1. Result with SDV
    full_bld_end

  2. Result without C:\DVL1903 or C:\DVL1607 legacy DVL folders, but with SDV
    full_bld_bad_no_legacy_dvl_folders_with_sdv

  3. Result without C:\DVL1607 legacy DVL folder, but with SDV
    full_bld_bad_no_legacy_dvl1607_folder_with_sdv

  4. Result with both legacy DVL folders but no SDV
    full_bld_end_no_sdv

I didn't bother with screenshots for the scenario of no legacy DVL folders and no SDV, but it would show the combination of items above, i.e. SDV directory checking and DVL logged results, as would be logically expected.

Let me know what you think.
Ready to proceed or more work..?

@kostyanf14
Copy link
Member

@benyamin-codez

PR looks good. Squash all commits, force push branch, and set it ready for review.

@kostyanf14
Copy link
Member

As we need DVL1903 in 3 drivers, please update viostor and NetKVM after this one PR will be merged.

@benyamin-codez
[vioscsi] Conditionally manage DVL copying
ba9d513 
1. Refactors the CustomBuildStep Command to manage DVL file creation.
   We now create DVL files in the Project (driver) folder with version
   numbers in the file name for DVL files created with legacy EWDKs,
   and with the "latest" label for current EWDKs. These files are
   then copied to the relevant .\Install folders and renamed for
   extant WHCP submission operations.

2. New build\makeLegacyDVLs.bat now creates all legacy DVL files.
   This functionally replaces build\dvl1903.bat and extends it to
   service any legacy DVL build. It provides a framework to manage
   legacy DVL creation and a common place to lay out version-specific
   DVL operations and manipulations. Logging is also enabled to
   provide feedback when read at the tail of root\buildAll.bat.
   Please see PR virtio-win#1212 for details on these changes.

3. Enables Win10_SDV for local (vioscsi) buildAll.bat script.

Signed-off-by: benyamin-codez <[email protected]>
@benyamin-codez benyamin-codez marked this pull request as ready for review December 29, 2024 15:44
@benyamin-codez
Copy link
Contributor Author

PR looks good. Squash all commits, force push branch, and set it ready for review.

Thanks, Kostiantyn. That's now done.

As we need DVL1903 in 3 drivers, please update viostor and NetKVM after this one PR will be merged.

Did you mean to wait for this to merge first?

@YanVugenfirer
Copy link
Collaborator

PR looks good. Squash all commits, force push branch, and set it ready for review.

Thanks, Kostiantyn. That's now done.

As we need DVL1903 in 3 drivers, please update viostor and NetKVM after this one PR will be merged.

Did you mean to wait for this to merge first?

Sure, if you can do it in separate PRs

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

@vrozenfe vrozenfe Awaiting requested review from vrozenfe vrozenfe is a code owner

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

Successfully merging this pull request may close these issues.
3 participants
@benyamin-codez @kostyanf14 @YanVugenfirer