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

[aokblast]

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

[aokblast]

Free members

[markj]

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 ↗

(On Diff #148512)

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 ↗

(On Diff #148512)

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

usr.sbin/bhyve/rfb.c

1327

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.

1333–1334

This conditional can simply be deleted.

[aokblast]

Update implementation by using only one mutex and cond variable

[aokblast]

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 ↗

(On Diff #148512)

Ya, I think you are right. Fix it

92 ↗

(On Diff #148512)

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]

Fix condition when there is no rendezvous member

[markj]

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

40 ↗

(On Diff #148560)

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.

96 ↗

(On Diff #148560)

We should assert that the counter is > 0.

115 ↗

(On Diff #148560)

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]

• Add wait operation for uart

[aokblast]

• usr.sbin/bhyve: add wait operation for uart

[aokblast]

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]

Remove useless thread

[markj]

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

128

This typedef is not needed.

137

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

171

Unneeded return statement.

usr.sbin/bhyve/uart_backend.c

441–444

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]

Minor fix

[aokblast]

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.