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.

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

NO-SDV [vioscsi] Tracing improvements #1176

Merged

Conversation

benyamin-codez
Copy link
Contributor

Includes:
a) New RUN_UNCHECKED and RUN_MIN_CHECKED definitions for compilation conditional tracing
b) DBG definition fix (remove superfluous "define")
c) Use keywords for WPP FLAGS
d) New INLINE function WPP definitions
e) WPP Optimisation
f) ETW Prettification
g) Suggestion to cull code for InitializeDebugPrints() when EVENT_TRACING

Includes:
a) New RUN_UNCHECKED and RUN_MIN_CHECKED definitions for compilation conditional tracing
b) DBG definition fix (remove superfluous "define")
c) Use keywords for WPP FLAGS
d) New INLINE function WPP definitions
e) WPP Optimisation
f) ETW Prettification
g) Suggestion to cull code for InitializeDebugPrints() when EVENT_TRACING

Signed-off-by: benyamin-codez <[email protected]>
Copy link
Contributor

@JonKohler JonKohler left a comment

Choose a reason for hiding this comment

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

LGTM with one incredibly small nitpick

vioscsi/trace.h Outdated Show resolved Hide resolved
vioscsi/utils.c Outdated Show resolved Hide resolved
@vrozenfe
Copy link
Collaborator

@benyamin-codez
We do need to keep the following configuration available
VirtioDebugPrintProc = DebugPrintFuncSerial;

It is the only way to troubleshoot the boot time, installation time and crash handler problems.

Best,
Vadim.

@benyamin-codez
Copy link
Contributor Author

Thanks @vrozenfe

Just to be clear, I'm not proposing to alter the DBG path but rather just the ETW path like this:

--- a/vioscsi/utils.c
+++ b/vioscsi/utils.c
@@ -86,34 +86,33 @@
 void InitializeDebugPrints(IN PDRIVER_OBJECT  DriverObject, IN PUNICODE_STRING RegistryPath)
 {
     //TBD - Read nDebugLevel and bDebugPrint from the registry
     bDebugPrint = 1;
     virtioDebugLevel = 0;
-    nVioscsiDebugLevel = TRACE_LEVEL_ERROR;
+#if !defined(RUN_UNCHECKED)
+    nVioscsiDebugLevel = TRACE_ALL;
+#endif
 
 #if defined(PRINT_DEBUG)
     VirtioDebugPrintProc = DebugPrintFunc;
 #elif defined(COM_DEBUG)
     VirtioDebugPrintProc = DebugPrintFuncSerial;
 #else
     VirtioDebugPrintProc = NoDebugPrintFunc;
 #endif
 }
 
-tDebugPrintFunc VirtioDebugPrintProc;
-#else
+#else // Everything above here is DBG, everything below is ETW
+static void NoDebugPrintFunc(const char *format, ...) {} // This is NOT strictly required for ETW 
 void InitializeDebugPrints(IN PDRIVER_OBJECT  DriverObject, IN PUNICODE_STRING RegistryPath)
 {
-    //TBD - Read nDebugLevel and bDebugPrint from the registry
-    bDebugPrint = 0;
-    virtioDebugLevel = 0;
-    nVioscsiDebugLevel = 4;// TRACE_LEVEL_ERROR;
+    VirtioDebugPrintProc = NoDebugPrintFunc; // This is NOT strictly required for ETW - neither DbgPrint nor DbgPrintEx will be called when EVENT_TRACING is defined
 }
-
-tDebugPrintFunc VirtioDebugPrintProc = DbgPrint;
 #endif
 
+// Both DBG and ETW from here...  
+tDebugPrintFunc VirtioDebugPrintProc; // This is necessary for compilation due to VirtIO\kdebugprint.h requisites
 
 #undef MAKE_CASE
 #define MAKE_CASE(scsiOpCode)    \
     case scsiOpCode:             \
         scsiOpStr = #scsiOpCode; \

Any concerns?

cc: @YanVugenfirer

@vrozenfe
Copy link
Collaborator

@benyamin-codez

Looks good to me. Please submit the final version when it is ready.

Best,
Vadim.

Includes:
a) Indentation fixes upon review (Credit to the eagle eye of @JonKohler..!)
b) Removes unnecessary and orphaned code
c) Culls unused code for InitializeDebugPrints() when EVENT_TRACING
d) Added comments to show necessary and necessary ETW inclusions

Signed-off-by: benyamin-codez <[email protected]>
@benyamin-codez benyamin-codez marked this pull request as ready for review October 30, 2024 18:10
@benyamin-codez
Copy link
Contributor Author

@kostyanf14

More failed checks with:

Error: 0x80070002, The system cannot find the file specified. FindFileAndCopy()::(null)::CAUSE:Cannot Find Pattern "C:\HLK\JobsWorkingDir\Tasks\WTTJobRun412AB604-3B97-EF11-B4C4-56000200DDDD\dv_events.wtl.trace"
...
Error: 0x80070002, The system cannot find the file specified. FindFileAndCopy()::(null)::CAUSE:Cannot Find Pattern "C:\HLK\JobsWorkingDir\Tasks\WTTJobRun7ACD44DE-3A97-EF11-9758-56000200DDDD\dv_events.wtl.trace"
...
etc.

@kostyanf14
Copy link
Member

kostyanf14 commented Oct 31, 2024

@benyamin-codez

Currently, we have vioscsi driver bug in the upstream, so before merging #1180 we can't identify the real reason of the fail

@kostyanf14 kostyanf14 changed the title [vioscsi] Tracing improvements NO-SDV [vioscsi] Tracing improvements Nov 3, 2024
@kostyanf14
Copy link
Member

rerun tests

@YanVugenfirer
Copy link
Collaborator

@kostyanf14 please re-run CI tests after DMA remapping rollback merge

@kostyanf14
Copy link
Member

@YanVugenfirer Static Tools Logo Test fails are expected as PR was built without SDV.

@benyamin-codez
Copy link
Contributor Author

@YanVugenfirer Static Tools Logo Test fails are expected as PR was built without SDV.

@kostyanf14, I'm guessing something started working again with recent changes to the build scripts..?

So, it would appear calling vioscsi\buildAll.bat Win10 amd64 skips SDV... 8^d

Calling vioscsi\buildAll.bat amd64 reveals two (2) SDV defects in vioscsi, including one Must-Fix defect. Both are quite historical. Perhaps you are already aware of them and mask them in some build configurations..?

Anyway, I will raise a PR to fix both defects - hopefully in the next couple of hours (I have already fixed them and just need to raise the PR after seeing to some other commitments).

I presume DDI versions 24H2 and higher only use CodeQL and not SDV. Fixing the above defects will likely fix this too.

I note viostor does not report any SDV defects. I still need to explicitly check VirtIO...

@benyamin-codez
Copy link
Contributor Author

A PR to fix SDV defects has been raised: #1181

@YanVugenfirer YanVugenfirer merged commit 334c143 into virtio-win:master Nov 4, 2024
1 of 5 checks passed
@kostyanf14
Copy link
Member

@benyamin-codez

I presume DDI versions 24H2 and higher only use CodeQL and not SDV. Fixing the above defects will likely fix this too.

No, this is a requirement from MS. Starting from 24H2/Server 2025 SDV is deprecated and removed.

Calling vioscsi\buildAll.bat amd64 reveals two (2) SDV defects in vioscsi, including one Must-Fix defect. Both are quite historical. Perhaps you are already aware of them and mask them in some build configurations..?

Must-Fix in SDV is not equal to Static Tools Logo Test fails. You can see Must-Fix but the test still will be passed. As currently, SDV is deprecated, we just stopped checking DVL.xml if the Static Tools Logo Test passed.

@benyamin-codez
Copy link
Contributor Author

@kostyanf14

Thanks for the reply. I thought it pertinent to clarify a couple of things...

I presume DDI versions 24H2 and higher only use CodeQL and not SDV. Fixing the above defects will likely fix this too.

No, this is a requirement from MS. Starting from 24H2/Server 2025 SDV is deprecated and removed.

I think we are saying the same thing here, i.e. that CodeQL is a must have from 24H2 as SDV is deprecated.
I note I should have said "CI target" rather than "DDI".

Calling vioscsi\buildAll.bat amd64 reveals two (2) SDV defects in vioscsi, including one Must-Fix defect. Both are quite historical. Perhaps you are already aware of them and mask them in some build configurations..?

Must-Fix in SDV is not equal to Static Tools Logo Test fails. You can see Must-Fix but the test still will be passed. As currently, SDV is deprecated, we just stopped checking DVL.xml if the Static Tools Logo Test passed.

SDV aside, would it be true that if CodeQL finds a Must-Fix defect, the Static Tools Logo Test will fail?

I noticed in the HLK Test Reference for the Static Tools Logo Test, that KM driver submissions need to pass all Must-Fix rules:

At present, the Static Tools Logo test requires that CodeQL be run and the "Must-Fix" set of queries passed for all kernel-mode drivers excluding graphics drivers. Note that running CodeQL on graphics drivers is highly recommended even though it is not currently required. Some queries may also find useful defects in user-mode components.
...
For Windows Server 2025 Certification, CodeQL will become the required tool for Static Tools Logo Test, this implies that all driver submissions must at a minimum pass all 'must fix' rules to be acceptable for WHCP. However, if you are certifying for Windows Server 2022 and below; CA, SDV and CodeQL can be used. Use WDK builds with the matching OS release versions.

Are you able to advise whether up-to-date standard CodeQL query suites are used, e.g. windows_driver_recommended.qls (which includes windows_driver_mustfix.qls) or is a custom query suite that includes windows_driver_mustfix.qls used instead? Moreover, is the same query suite used on all CI targets?

I'm curious as to why the CI check for Server2025 failed the Static Tools Logo Test in both #1180 and #1181, when the other CI targets passed. Any thoughts?

Best regards,
Ben

@kostyanf14
Copy link
Member

kostyanf14 commented Nov 6, 2024

@benyamin-codez

CodeQL finds a Must-Fix defect, the Static Tools Logo Test will fail?

Yes

Are you able to advise whether up-to-date standard CodeQL query suites are used, e.g. windows_driver_recommended.qls (which includes windows_driver_mustfix.qls) or is a custom query suite that includes windows_driver_mustfix.qls used instead?

See for details: https://github.com/virtio-win/kvm-guest-drivers-windows/blob/master/build/build.bat#L238
All version also specified in wiki: https://virtio-win.github.io/Development/Building-the-drivers-using-Windows-11-24H2-EWDK

Moreover, is the same query suite used on all CI targets?

Yes

I'm curious as to why the CI check for Server2025 failed the Static Tools Logo Test in both #1180 and #1181, when the other CI targets passed. Any thoughts?

See note in docs https://learn.microsoft.com/en-us/windows-hardware/test/hlk/testref/6ab6df93-423c-4af6-ad48-8ea1049155ae (look for "RoMetadata.dll could not be found"). We run CI on Server Core and this DLL is missing. I hope to workaround the issue soon.

@benyamin-codez
Copy link
Contributor Author

@kostyanf14

Many thanks for making the time to reply.

That's good to know that the query suite and tooling is the same for all CI targets, and that there's a known cause for the failure on Server2025 and a plan to address it.

On the versions specified in the wiki, I'm not sure if it's come to your attention, but there are some additional steps required - at least for WHCP_24H2 (and maybe others).

I'll raise a PR for your consideration to address this over on https://github.com/virtio-win/virtio-win.github.io.

Best regards,
Ben

@kostyanf14
Copy link
Member

@benyamin-codez Feel free to send PR for wiki

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants