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

[vioscsi] Fix SDV defects #1181

Merged
merged 1 commit into from
Nov 11, 2024

Conversation

benyamin-codez
Copy link
Contributor

Fixes SDV defects in vioscsi.c:
a) HwStorPortProhibitedDDIs Must-Fix defect in VioScsiInterrupt() IRQL for trace
b) NullCheck defect in CompleteRequest() OpCode for trace

SDV defects in vioscsi.c
a) HwStorPortProhibitedDDIs Must-Fix defect in VioScsiInterrupt() IRQL for trace
b) NullCheck defect in CompleteRequest() OpCode for trace

Signed-off-by: benyamin-codez <[email protected]>
@vrozenfe
Copy link
Collaborator

vrozenfe commented Nov 5, 2024

@benyamin-codez @YanVugenfirer

From my experience there is only one single case when we really need this printout. It is when the system installation on a virtio storage disk hangs and we need to troubleshoot this issue. In this case we might need to enable verbose output and redirect the printouts to COM port, otherwise I don't remember any single case when we need to know on which DIRQL level the ISR routine is running.
So, I'm fine with both solutions either removing it completely or keeping it as commented.

Best,
Vadim.

@benyamin-codez
Copy link
Contributor Author

benyamin-codez commented Nov 5, 2024

@vrozenfe @YanVugenfirer

From my experience there is only one single case when we really need this printout. It is when the system installation on a virtio storage disk hangs and we need to troubleshoot this issue. In this case we might need to enable verbose output and redirect the printouts to COM port, otherwise I don't remember any single case when we need to know on which DIRQL level the ISR routine is running.

Thanks for the insight, Vadim.

This might come in useful during any future work around removing InterruptLock spinlocks and retention of any necessary adaptExt->dpc_ok=TRUE checks, per comments in #1175. Testing in this space can have a high prevalence of DRIVER_IRQL_NOT_LESS_OR_EQUAL (0xD1) BSODs, and knowing the IRQL when the offending ISR is called is helpful.

As such, I'm further minded to retain it as commented, at least for the time being.

On the failing HCK checks, I'm building with Cobalt EWDK (22000/21H2) on Server2019 for Win10 and Germanium EWDK (26100/24H2) for Win11 on Win11_24H2 with SDV+CodeQL and CodeQL respectively and getting no defects.
Most recently, I've been using CodeQL v2.19.2 and WHCP_main. Although I think it most unlikely, maybe that's too new a version to reveal defects on Server2022 (21H2)...? For the Server2025 fail, maybe the down-level QEMU is too low (maybe the same for Server2022)...? Should I disregard these failures (@kostyanf14 previously mentioned an upstream bug), or is there something I should be looking for...?
¯\_(ツ)_/¯

Best regards,
Ben

EDIT: I note the Server2022 CI checks passed the Static Tools Logo Test, but failed the Flush Test and Disk Verification (LOGO). However, the Server2025 CI checks did in fact fail the Static Tools Logo Test and also Disk Stress (LOGO) and Disk Verification (LOGO).

@benyamin-codez
Copy link
Contributor Author

@kostyanf14

Thanks for the discussion over on #1176.

EDIT: I note the Server2022 CI checks passed the Static Tools Logo Test, but failed the Flush Test and Disk Verification (LOGO). However, the Server2025 CI checks did in fact fail the Static Tools Logo Test and also Disk Stress (LOGO) and Disk Verification (LOGO).

Are we safe to proceed here, or do we need to resolve the Flush Test and Disk XXXX (LOGO) failures...?

@kostyanf14
Copy link
Member

@benyamin-codez

Are we safe to proceed here, or do we need to resolve the Flush Test and Disk XXXX (LOGO) failures...?

Yes.

@benyamin-codez
Copy link
Contributor Author

@kostyanf14

Are we safe to proceed here, or do we need to resolve the Flush Test and Disk XXXX (LOGO) failures...?

Yes.

My apologies, Kostiantyn. Did you mean, "Yes, safe to proceed" or "Yes, we need to resolve..."?

We also had Disk XXXX (LOGO) failures for Win11 and Server2025 in #1174...
...and the similar pattern in #1175 (all NO-SDV / DMAR issues aside).

Best regards,
Ben

@kostyanf14
Copy link
Member

@benyamin-codez

Sorry, my mistake. Yes, safe to proceed.

@YanVugenfirer YanVugenfirer merged commit 879e95e into virtio-win:master Nov 11, 2024
3 of 5 checks passed
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.

4 participants