Page MenuHomeFreeBSD

Fix a lock recursion introduced in r327065.
ClosedPublic

Authored by tychon on Feb 28 2018, 7:44 PM.

Details

Summary

D14382 notes that r327065 (D13573) broke bhyve if INVARIANTS is enabled with passthrough. As I mentioned in the comments to D14382, it's possible to fix this in a slightly more straightforward manner; an attempt to do that motivates this review.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

tychon created this revision.Feb 28 2018, 7:44 PM
tychon added a reviewer: grehan.
tychon added subscribers: kmacy, cem, dch, grehan.

Since your all here from the add of bhyve can you look at herald rule H97 https://reviews.freebsd.org/H97 , look for anything I might of missed in the list of file directories.
I am trying to get that upgraded to a global rule with an add non-blocking reviewer bhyve, but that requires higher access than I have.

I can see that I missed sys/amd64/include/vmm.h, vmm_dev.h from this diffential, but I believe that is ok as it would be very rare to touch that file and not touch something else that triggers.

rgrimes resigned from this revision.Feb 28 2018, 8:13 PM

Since your all here from the add of bhyve can you look at herald rule H97 https://reviews.freebsd.org/H97 , look for anything I might of missed in the list of file directories.
I am trying to get that upgraded to a global rule with an add non-blocking reviewer bhyve, but that requires higher access than I have.

I can see that I missed sys/amd64/include/vmm.h, vmm_dev.h from this diffential, but I believe that is ok as it would be very rare to touch that file and not touch something else that triggers.

cem removed a subscriber: cem.Feb 28 2018, 9:08 PM
jhb added a subscriber: jhb.Feb 28 2018, 11:04 PM
jhb added inline comments.
sys/amd64/vmm/intel/vmx.c
3275 ↗(On Diff #39838)

Do you want a KASSERT(vmexit->exitcode == VM_EXITCODE_HLT) here or is that too silly? It seem from my reading that this is in fact true (and there's a comment at the start of this function saying it is true), but it is not trivially obvious from the code (e.g. the vlapic_pending_intr() in vmx_inject_interrupts() is called from a non-HLT exit, but only in the !virtual_interrupt_delivery case and this function is only used to implement vlapic_pending_intr() in the virtual_interrupt_delivery case).

I do think the idea is sound and prefer it to D14382.

tychon updated this revision to Diff 39862.Mar 1 2018, 2:17 PM

Include KASSERT suggested by jhb.

grehan accepted this revision.Mar 1 2018, 3:28 PM
This revision is now accepted and ready to land.Mar 1 2018, 3:28 PM
jhb added inline comments.Mar 2 2018, 6:04 PM
sys/amd64/vmm/intel/vmx.c
2285 ↗(On Diff #39862)

I had one more question about this that I thought of after my first comment. Is there any chance that this VMCS field isn't valid to read such that vmcs_read() could fault in the !virtual_interrupt_delivery case? If the vmcs_read() will always succeed and just return zero in that case then this is fine. If it would fault on an old CPU that doesn't support virtual interrupts then it should be conditional. (I just can't recall if VMCS fields are always readable vs if some of them might fault if you try to read them for an unsupported feature.)

tychon added inline comments.Mar 2 2018, 9:45 PM
sys/amd64/vmm/intel/vmx.c
2285 ↗(On Diff #39862)

I've just tested this on system with hw.vmm.vmx.cap.virtual_interrupt_delivery = 0 and in this case vmcs_read(VMCS_GUEST_INTR_STATUS) simply returns zero.

I'm going to extrapolate that this is behavior on all generations of CPUs which don't support the 1-setting of the “virtual-interrupt delivery” VM-execution control :-)

tychon updated this revision to Diff 40004.Mar 6 2018, 7:43 PM

Not to make this my magnus opus, but based on some offline discussion with jhb, I had still left some room for improvement. I believe I've addressed those concerns with this updated diff.

This revision now requires review to proceed.Mar 6 2018, 7:43 PM
jhb accepted this revision.Mar 6 2018, 10:25 PM

Thanks

This revision is now accepted and ready to land.Mar 6 2018, 10:25 PM
This revision was automatically updated to reflect the committed changes.