Page MenuHomeFreeBSD
Differential D48241

usr.sbin/bhyve: add rendezvous layer for initialization step of bhyve
Needs ReviewPublic
Actions

Authored by aokblast on Dec 29 2024, 9:24 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Mar 18, 3:21 AM
Unknown Object (File)
Wed, Mar 18, 3:14 AM
Unknown Object (File)
Tue, Mar 17, 8:23 PM
Unknown Object (File)
Fri, Mar 6, 6:33 PM
Unknown Object (File)
Fri, Mar 6, 2:27 AM
Unknown Object (File)
Mon, Mar 2, 7:02 PM
Unknown Object (File)
Mon, Mar 2, 6:47 PM
Unknown Object (File)
Thu, Feb 26, 1:25 AM
View All Files
Subscribers
bcran
emaste
imp
khng
lwhsu
markj
rgrimes

Details

Reviewers
None
Group Reviewers
bhyve
Summary

THere maybe multiple wait operation in different components of bhyve (e.g. rfb, sock)

In original implementation, each wait operation will block the main thread thus
we can not connect to the socket in arbitrary order. Moreover, the actual
order depends on the argument order pass to the bhyve program

To better address this, we should add a rendezvous layer that can wait
all component at the same time

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 61385
Build 58269: arc lint + arc unit

Event Timeline

aokblast created this revision.Dec 29 2024, 9:24 AM
Herald added a reviewer: bhyve. · View Herald TranscriptDec 29 2024, 9:24 AM
Herald added subscribers: bcran, rgrimes, imp. · View Herald Transcript
aokblast requested review of this revision.Dec 29 2024, 9:24 AM
Harbormaster completed remote builds in B61384: Diff 148511.Dec 29 2024, 9:25 AM
aokblast added subscribers: lwhsu, markj.Dec 29 2024, 9:25 AM
aokblast updated this revision to Diff 148512.Dec 29 2024, 9:29 AM

Free members

Harbormaster completed remote builds in B61385: Diff 148512.Dec 29 2024, 9:30 AM
markj added a comment.Dec 30 2024, 3:20 PM

Are there any other potential consumers of this interface, besides rfb? I see the comment that mentions why gdb can't participate, which is a bit unsatisfying but okay.

usr.sbin/bhyve/rendezvous.c
74

This interface behaves basically like a semaphore: rendezvous_add_member() increases the count, rendezvous_member_notify() decreases it, and rendezvous() waits for it to decrease to zero.

I think it could be implemented more simply: just have a single mutex, CV, and a counter. I don't see any reason to have per-consumer state either.

92

We should check somehow that this operation is always done before rendezvous() is called.

usr.sbin/bhyve/rfb.c
1331

Is there any need for these functions to be separate? It looks like we might as well automatically add the new member when it's created.

1339

This conditional can simply be deleted.

aokblast updated this revision to Diff 148559.Dec 30 2024, 5:01 PM

Update implementation by using only one mutex and cond variable

Harbormaster completed remote builds in B61410: Diff 148559.Dec 30 2024, 5:01 PM
aokblast marked 2 inline comments as done.EditedDec 30 2024, 5:17 PM

Another consumer of rendezvous will be the raw socket over serial console that I have implemented a couple months ago. I haven't implement the wait operation on raw socket serial console. I would like to confirm the interface of rendezvous before I implement it.

BTW, for gdb, we can still wait gdb on rendezvous layer by deleting the set of vcpu0 and use rendezvous_notify. But I think setting vcpu0 maybe a better implementation when consider the behavior of gdb?

usr.sbin/bhyve/rendezvous.c
74

Ya, I think you are right. Fix it

92

Do you means the original implementation by separating add_member and member_create. If it is, I think the new implementation that combine these two have solve the problem

aokblast updated this revision to Diff 148560.Dec 30 2024, 5:25 PM

Fix condition when there is no rendezvous member

Harbormaster completed remote builds in B61411: Diff 148560.Dec 30 2024, 5:26 PM
markj added a comment.Dec 30 2024, 7:30 PM
In D48241#1100375, @aokblast wrote:

Another consumer of rendezvous will be the raw socket over serial console that I have implemented a couple months ago. I haven't implement the wait operation on raw socket serial console. I would like to confirm the interface of rendezvous before I implement it.

I left some inline comments. It's hard to make judgements about the interface when there is only one consumer. It looks ok to me, but maybe the uart driver has some extra requirements.

BTW, for gdb, we can still wait gdb on rendezvous layer by deleting the set of vcpu0 and use rendezvous_notify. But I think setting vcpu0 maybe a better implementation when consider the behavior of gdb?

Actually, I do not understand why gdb cannot use the rendezvous layer. bhyve_start_vcpu() is called after rendezvous(), so the following would happen:

  1. init_gdb() registers a notifier,
  2. rendezvous() blocks until notification is received
  3. a debugger attaches and new_connection() notifies the bhyverun thread
  4. bhyverun adds vCPUs
  5. vCPU threads block in gdb_cpu_add() until the debugger resumes the guest

What goes wrong here?

usr.sbin/bhyve/rendezvous.c
41

IMHO this facility should just be a couple of functions in bhyverun.c - it is specific to bhyve startup (e.g., it can only be used once) and not very abstract.

I think you can simply have a global struct in bhvyerun.c with public functions like

void bhyve_startup_block(); /* called to register a notifier and block startup */
void bhyve_startup_notify(); /* notify the blocked thread */

and have bhyve_startup_wait() in implemented privately in bhyverun.c.

The rendezvous implementation is good but the interface is not general enough to warrant its own file, IMHO.

97

We should assert that the counter is > 0.

116
pthread_mutex_lock(...);
while (softc->counter > 0)
    pthread_cond_wait(...);
pthread_mutex_unlock(...);

would be a bit simpler and avoids extra (un)locking when executing the loop multiple times.

aokblast updated this revision to Diff 148644.Jan 1 2025, 3:58 PM
  • Add wait operation for uart
Harbormaster completed remote builds in B61447: Diff 148644.Jan 1 2025, 3:58 PM
aokblast updated this revision to Diff 148645.Jan 1 2025, 4:05 PM
  • usr.sbin/bhyve: add wait operation for uart
Harbormaster completed remote builds in B61448: Diff 148645.Jan 1 2025, 4:05 PM
aokblast added a comment.EditedJan 1 2025, 4:12 PM

I discover the original wait operation for gdb is not working now. I configure one vm with rfb and gdb. When gdb is with wait operation, the rfb does not have any output. But if the gdb is not waiting, the rfb will have output and gdb can attach to vm after boot up. I checkout to the main branch and it is also the case.

So I decide to implement wait operation for raw tcp console now to prove these stuff work.

BTW, a change is that I discover mevent dispatcher is executed at the bottom of main thread. That means we have to execute the bhyve_init_wait at each vcpu_thread instead of main thread or we cannot get mevent dispatched (because mevent_dispatch is not executed), then the bhyve_init_wait will not be notified.

aokblast updated this revision to Diff 148647.Jan 1 2025, 5:15 PM

Remove useless thread

Harbormaster completed remote builds in B61449: Diff 148647.Jan 1 2025, 5:15 PM
markj added a comment.Jan 2 2025, 6:55 PM
In D48241#1101029, @aokblast wrote:

I discover the original wait operation for gdb is not working now. I configure one vm with rfb and gdb. When gdb is with wait operation, the rfb does not have any output. But if the gdb is not waiting, the rfb will have output and gdb can attach to vm after boot up. I checkout to the main branch and it is also the case.

I guess it works if you attach a debugger first, then the rfb client?

So I decide to implement wait operation for raw tcp console now to prove these stuff work.

Why can't the gdb stub use this mechanism too?

BTW, a change is that I discover mevent dispatcher is executed at the bottom of main thread. That means we have to execute the bhyve_init_wait at each vcpu_thread instead of main thread or we cannot get mevent dispatched (because mevent_dispatch is not executed), then the bhyve_init_wait will not be notified.

I see, that seems reasonable.

usr.sbin/bhyve/bhyverun.c
129

This typedef is not needed.

138

This can just be a static global variable, there's no reason to allocate it lazily, since we always allocate it exactly once.

172

Unneeded return statement.

usr.sbin/bhyve/uart_backend.c
439 ↗(On Diff #148647)

It would be cleaner to first use strchr(',') to break up the string into different options. Then you can use sscanf() to extract the address and port, and handle the rest separately. Otherwise adding new options in the future will be painful.

aokblast updated this revision to Diff 149284.Jan 15 2025, 11:41 AM

Minor fix

Harbormaster completed remote builds in B61732: Diff 149284.Jan 15 2025, 11:41 AM
aokblast added a comment.Jan 15 2025, 11:44 AM

I don't implement it on gdb just because I cannot make wait operation on gdb work on my PC. Thus I am unable to test. Still checking where is the problem.

aokblast updated this revision to Diff 151192.Feb 19 2025, 3:15 PM

Fix bhyve gdb startup

Herald added a subscriber: emaste. · View Herald TranscriptFeb 19 2025, 3:15 PM
Harbormaster completed remote builds in B62516: Diff 151192.Feb 19 2025, 3:15 PM
aokblast added a comment.Feb 19 2025, 3:29 PM

A quick find-up is that gdb_cpu_add storing the cpu suspend status and recover it in gdb wait mode.

gdb_cpu_add is executed in cpu_thread. At the same time, the task of resume BSP is at the bottom of main_thread.

If cpu_thread run faster than main_thread, the cpu suspend status will include BSP because BSP haven't been resumed (by the main_thread) yet.

It means when gdb is connected and suspend status is recovered, the BSP will be included thus the machine cannot be boot (no CPU is resumed) up.

A fix is that we use the block mechanism we proposed in this patch without suspend cpu0.

But a new problem I discover is that when gdb connected at the time when bhyve haven't booted up to OS, the bhyve will exit.

I attach bhyve to gdb and gdb shows vm triggered the TRIPLEFAULT and then exit with return value 3.

Still findout what is the problem but I think this facility works right now.

lwhsu added a subscriber: khng.Jul 30 2025, 1:30 PM

Revision Contents
Changeset List

PathSize
usr.sbin/
bhyve/
1 line
3 lines
1 line
32 lines
115 lines
25 lines

Diff 148512

View Options

usr.sbin/bhyve/Makefile

Loading...
View Options

usr.sbin/bhyve/bhyverun.c

Loading...
View Options

usr.sbin/bhyve/debug.h

Loading...
View Options

usr.sbin/bhyve/rendezvous.h

Loading...
View Options

usr.sbin/bhyve/rendezvous.c

Loading...
View Options

usr.sbin/bhyve/rfb.c

Loading...