-
Notifications
You must be signed in to change notification settings - Fork 529
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
Basic NMR integration for MsQuic #3961
Conversation
goto Error; | ||
} | ||
|
||
MsQuic = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verify the ProviderDispatch version first? Though I suppose since its version is currently set to zero, I think your check would be impossible to fail 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not exactly sure how to use all the version fields in NMR. There's also a version field in the NPI_REGISTRATION_INSTANCE passed in the attach callback. MsQuic's entry function also has an argument for version.
Would it make sense to just defer the version related stuff to MsQuicOpenVersion and assume there are always and only MsQuicOpenVersion and MsQuicClose in the provider dispatch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(the NMR doc just says for network modules set version to 0??)
@@ -24,6 +24,7 @@ | |||
#include <minwindef.h> | |||
#include <ntstatus.h> | |||
#include <basetsd.h> | |||
#include <netioddk.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the "public" header that external clients consume. Is there any risk of this breaking their builds?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might if their code conflicts with the definitions or something in the header but netioddk.h is a public header file published in DDK. I don't know what we can do to reduce the risk. Are there really external KM consumers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have external KM customers. I'm just worried about an ingest PR possibly breaking Windows build.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3961 +/- ##
==========================================
- Coverage 87.27% 87.16% -0.11%
==========================================
Files 56 56
Lines 16958 16958
==========================================
- Hits 14800 14782 -18
- Misses 2158 2176 +18 ☔ View full report in Codecov by Sentry. |
src/bin/winkernel/nmrprovider.c
Outdated
_In_ const void *ClientDispatch, | ||
_Out_ void **ProviderBindingContext, | ||
_Out_ const void **ProviderDispatch | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
) | |
) |
|
||
if (NmrClient.NmrClientHandle) { | ||
NTSTATUS Status = NmrDeregisterClient(NmrClient.NmrClientHandle); | ||
CXPLAT_FRE_ASSERTMSG(Status == STATUS_PENDING, "client deregistration failed"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we really bugcheck if this fails? Can OOM trigger this failure? What then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NmrDeregisterClient strictly only returns STATUS_PENDING. This is sort of a dummy check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. How can/should we test this more. For instance, any negative test cases?
Test coverage for NMR itself should be added in os repo and is being added (nmrtest). The more interesting test coverage will be added when we add msquic specific loading logic to NMR. Right now, msquic as an NMR provider is too simple to have more test cases. It literally just returns SUCCESS and sets the dispatch table for the client. |
Sounds good. Ship it. |
Description
The PR adds an NMR interface (NMR provider) for kernel mode msquic. The MsQuic provide only exposes 2 interfaces, MsQuicOpenVersion and MsQuicClose, which are the same as what are exposed in msquic.sys. This PR does not remove the exports.
Currently, only QUIC tests are hooked up with NMR.
Testing
CI