Skip to content

virt_mshv_vtl/mshv: Cleanup TLB lock tracking #1389

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

Merged
merged 6 commits into from
May 28, 2025

Conversation

smalis-msft
Copy link
Contributor

@smalis-msft smalis-msft commented May 20, 2025

Don't bother telling the hypervisor what it already knows. Also fixup ARM code, since it wasn't handling this.

@smalis-msft smalis-msft requested a review from a team as a code owner May 20, 2025 21:08
@smalis-msft smalis-msft enabled auto-merge (squash) May 20, 2025 21:57
@mebersol mebersol added the backport_2505 Change should be backported to the release/2505 branch label May 20, 2025
@smalis-msft smalis-msft removed the backport_2505 Change should be backported to the release/2505 branch label May 21, 2025
@smalis-msft smalis-msft disabled auto-merge May 21, 2025 20:01
@smalis-msft smalis-msft enabled auto-merge (squash) May 27, 2025 14:42
chris-oo
chris-oo previously approved these changes May 27, 2025
@jstarks
Copy link
Member

jstarks commented May 27, 2025

I don't understand this change. AFAICT, the only non-assertion place where we call this is when we are taking the TLB lock. We sure don't want to add an extra hypercall into that path--we might as well just take the lock.

What's the motivation behind this change?

Copy link
Member

@jstarks jstarks left a comment

Choose a reason for hiding this comment

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

x

@smalis-msft
Copy link
Contributor Author

The motivation behind this is that there were cases found where said debug_asserts were failing. This fixes them. The logic beforehand was incomplete.

@smalis-msft smalis-msft enabled auto-merge (squash) May 27, 2025 18:28
@jstarks
Copy link
Member

jstarks commented May 27, 2025

The motivation behind this is that there were cases found where said debug_asserts were failing. This fixes them. The logic beforehand was incomplete.

If there are cases where we know the TLB is locked (we are asserting on it, after all), and we expect the hypervisor to say the TLB is locked, doesn't that imply we just failed to record our knowledge somewhere? I.e., shouldn't we have manually set the local cache in some place?

@smalis-msft
Copy link
Contributor Author

The case that I found was when we are provided an initial_gva_translation from the hypervisor. We use that instead of translating the gpa ourselves, which means we never interact with our own tlb lock machinery. We don't really have a great place to set our local lock in that flow. Also there's nothing stopping the hypervisor from locking the TLB on other paths, and this code should be correct in the general case for any future callers.

@smalis-msft smalis-msft changed the title virt_mshv_vtl/mshv: Always ask the hypervisor if the TLB is locked virt_mshv_vtl/mshv: Cleanup TLB lock tracking May 27, 2025
@smalis-msft smalis-msft force-pushed the hypervisor-tlb-lock-always branch from 62192fd to e04abfc Compare May 27, 2025 19:48
@smalis-msft smalis-msft disabled auto-merge May 27, 2025 19:57
@smalis-msft smalis-msft merged commit ecd2742 into microsoft:main May 28, 2025
28 checks passed
@smalis-msft smalis-msft deleted the hypervisor-tlb-lock-always branch May 28, 2025 16:56
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