bhyve reviewers group
Fri, Mar 15
- remove guest memory mapping device that is currently unused. The device was used to make snapshots faster by creating a separate Copy-on-Write mapping of the guest memory. This approach is not useful for guest suspends, and we may be able to avoid exporting it through a device even for snapshots (will be investigating that approach).
- remove some unused code
Sun, Mar 10
Fix issues pointed by inline comments
Sat, Mar 9
This appears to be used to deal with a core hang on specific CPU's, we need to see that we have mitigated that in the host, and the decide what the appropriate action here is, I am uncomfortable with just making this silently disappear. Deferning to jhb.
Thanks for the review. Is it possible for me to commit this myself or should someone else do it on my behalf? I have this code in a git branch on GitHub too: https://github.com/freebsd/freebsd/pull/392
I think this fix is good for the short term. I can confirm seeing failed writes to that MSR when running a Linux guest on Zen hardware (which were non-fatal to Linux).
I think that's a good idea. I would prefer if that was done in a different differential.
I would love to take on the challenge, but it could take me quite a while to implement that.
Though this does fix your issue by silencing the log, it does little to improve the situation. I think what we may want to do is to add yet another option to the bhyve command that along with -w to ignore invalid MSR access to do so silently so that we do not have to add code like this everything a new msr comes into play. John, Patrick your thoughts on this approach?
Fri, Mar 8
Hello, this is my first contribution to FreeBSD. I'm sorry if I'm doing something wrong, this is my first time using the system.
I have no idea on the internals, but some quick comments as I tried to read the code as-is.
Thu, Mar 7
I removed a redundant "to" in the description. Patch looks good to me. Copyright over to jhb@
Fri, Mar 1
This looks great and addresses my concern from an earlier iteration. Thanks for fixing it!
Thu, Feb 28
Yes, he was concerned about my use of the reserved bits. For a VMX-only case, it was probably fine, but if we ever want PCI-passthru to post interrupts directly to the guest, we'll need those bits. The write-up for OS-7354, which switched to the separate pending_prio field, covers the reasoning.
I recall seeing the earlier version of this before and approving it only to then have Tycho point out an issue I had failed to notice. I spent some time yesterday searching around in my MUA but couldn't find the thread. :-/ I think the issue you fixed from the bug you noted about not using the reserved PIR bits for the bitmask was the thing he had mentioned, I just wish I could confirm that for sure.
Wed, Feb 27
We do, in a sense, clear those bits on interrupt delivering since it involves clearing the pending bit, meaning the next 0 -> 1 transition would incur a clean of pending_prio. Clearing the lower bits in pending_prio as part of vmx_pending_intr carries no risk of incurring extra work, since interrupt notification is triggered from having an incoming prio greater than what is cached in pending_prio. Besides, spurious wake-ups from the HLT loop to check for interrupts are much preferred to missing one.
Hmm, I guess it would be expensive to clear the bits from pending_prio when interrupts are actually delivered? And/or we can't tell when that is when PIR is in use? The only thing that slightly worries me is if the last hunk throws away too much data by clearing all the bits except for prio_bit rather than only clearing higher bits. I guess even if there are still lower priority bits pending, we don't need to worry about sending a notification. When the vCPU lowers it's TPR that will then notice and handle those interrupts without 'pending_prio' being involved.
Fixed copyright attribution per Rod's comment.
Updated diff to include full context
Sat, Feb 23
Fri, Feb 22
I used git format-patch assuming phabricator would pick up on the context stuff, but apparently not. I'll be sure to increase the context for the next revision.
In the future it helps to upload full file diffs, not sure what vcs your using or what its command is, for svn I use svn diff -x U999999.
Jan 28 2019
Changes made as suggested by previous comments
Question regarding comments
Overall good, just style nits and a diff reduction.