Paths

Table of Contentst

Differential D47306

vmm: fix vcpu atomic load
ClosedPublic
Actions

Authored by br on Oct 28 2024, 1:15 PM.

Tags

None

Referenced Files

	Unknown Object (File)
	Fri, Jul 18, 6:37 PM

	Unknown Object (File)
	Thu, Jul 17, 6:46 AM

	Unknown Object (File)
	Thu, Jul 17, 6:29 AM

	Unknown Object (File)
	Thu, Jul 17, 5:47 AM

	Unknown Object (File)
	Tue, Jul 8, 8:56 PM

	Unknown Object (File)
	Mon, Jul 7, 12:10 PM

	Unknown Object (File)
	Sat, Jul 5, 4:21 PM

	Unknown Object (File)
	Thu, Jul 3, 2:02 PM

View All 47 Files

Subscribers

Details

Reviewers

jhb
markj
andrew
manu

Group Reviewers

Commits

rG72ae04c73347: vmm: fix vcpu atomic load

Summary

Load vcpu with acquire semantics as we are making a critical section between creating vcpu and using it.

Test Plan

tested on risc-v only

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

br created this revision.Oct 28 2024, 1:15 PM

Herald added a reviewer: andrew. · View Herald TranscriptOct 28 2024, 1:15 PM

Herald added a reviewer: andrew. · View Herald Transcript

Herald added a reviewer: manu. · View Herald Transcript

Herald added a reviewer: bhyve. · View Herald Transcript

Herald added subscribers: bcran, rgrimes, emaste. · View Herald Transcript

br requested review of this revision.Oct 28 2024, 1:15 PM

In particular, this is paired with an atomic_store_rel_ptr a few lines below. I think in practice this is probably a no-op as any dependent memory accesses are reads of data that vcpu points to, and if you had a mispredicted read it would never dereference a "wrong" pointer, it would just see a NULL pointer and stop once speculation ran into a fault. That said, I do think this is more correct.

This revision is now accepted and ready to land.Oct 28 2024, 1:29 PM

In D47306#1078749, @jhb wrote:

In particular, this is paired with an atomic_store_rel_ptr a few lines below. I think in practice this is probably a no-op as any dependent memory accesses are reads of data that vcpu points to, and if you had a mispredicted read it would never dereference a "wrong" pointer, it would just see a NULL pointer and stop once speculation ran into a fault. That said, I do think this is more correct.

This is perhaps a use-case for atomic_load_consume_ptr()? I think I agree that an acq fence is not formally required, but it looks wrong at first glance.

As an aside, we have #define atomic_load_ptr(p) (*(volatile __typeof(*p) *)(p)) - I wonder why atomic_load_acq_ptr isn't similarly defined.

br mentioned this in D45553: bhyve/riscv kernel part.Oct 28 2024, 2:32 PM

Closed by commit rG72ae04c73347: vmm: fix vcpu atomic load (authored by br). · Explain WhyOct 29 2024, 4:23 PM

This revision was automatically updated to reflect the committed changes.

br added a commit: rG72ae04c73347: vmm: fix vcpu atomic load.

Herald added a subscriber: imp. · View Herald TranscriptOct 29 2024, 4:23 PM

Revision Contents
Changeset List

Path

Size

sys/

amd64/

vmm/

3 lines

arm64/

vmm/

3 lines

Diff 145607

sys/amd64/vmm/vmm.c

Loading...

sys/arm64/vmm/vmm.c

Loading...