userboot: add callbacks to set unrestricted guest mode

[fabian.freyer_physik.tu-berlin.de]

Summary

The current userboot interface sets this capability using vm_setup_freebsd_registers_i386 in lib/libvmmapi/libvmmapi_freebsd.c. This patch adds support for loaders to explicitly query and/or set this capability without having to go through the exec callback, but set up registers themselves. To simplify setting up a default state, add a callback to wrap vcpu_reset.

This change is backwards-compatible as all new callbacks are added to the end of the callback structure.

Test Plan

test with a library implementing loader_main and calling these callbacks.

• Check whether capability is set.
• Check initial register states with bhyvectl --get-all

[fabian.freyer_physik.tu-berlin.de]

I may also add a vcpu_reset callback still, would be nice to have that in as well.

stand/userboot/userboot.h

45

Tbh this is no longer true, so I will update the patch to fix the comment

[grehan]

You may be able to get away with just the "set" call. Given that all h/w that is supported by bhyve, with the exception of only Nehalem systems, support this, you may want to use just the error from the set call to handle this low-probability error.

Note that AMD SVM has always supported this mode. "Unrestricted Guest" is an Intel-only feature, but given that the name is baked into a bhyve API already, there doesn't seem to be any harm in keeping it going.

[fabian.freyer_physik.tu-berlin.de]

• updated the comment at the version identifier to correctly list the changes introduced by this version
• added a callback to vcpu_reset

[grehan]

usr.sbin/bhyveload/bhyveload.c

566

Minor style(9) - should be an empty line before the rreturn - see the routine above.

567

Minor style(9) - continued lines should be indented by 4 spaces.

574

Minor style(9) - blank line between declarations and statement.

575

Minor style(9) - return expression should be enclosed in parens.

584

Minor style(9) - blank line needed before statement, and return expression should be in parens.

[fabian.freyer_physik.tu-berlin.de]

addressed @grehan's comments regarding style(9)

[fabian.freyer_physik.tu-berlin.de]

I hope I got them all now

[rgrimes]

I dont know why it did that, I tried to simply "accept revision" and it removed all other reviewers, so put them back.

[fabian.freyer_physik.tu-berlin.de]

pass USERBOOT_VERSION_5 to loader_main.

[fabian.freyer_physik.tu-berlin.de]

I'm sorry, I seem to have created the previous patch without full context, so here it is again, this time with full context.

[fabian.freyer_physik.tu-berlin.de]

ping?

Would these changes be welcome in base? If yes, what would still need to be done to this review in order to get it into a committable state?

[rgrimes]

If Peter accepts this and says I can go ahead with a commit I would do that, but I need his approval to commit it.

[grehan]

So a comment on this: in general, api's are not added to FreeBSD that don't have base-system clients that use them, or for a good reason. I think this falls into the latter but I'd like to cut it down a bit.

• can the get_unrestricted_guest() routine be removed ? There is an error return on the set, which seems like it can be used to determine if unrestricted mode is not available (e.g. that's how bhyve uses the ioctl).
• is there a need for vcpu_reset() ? The BSP should be initialized to power-on state.. That could be a bug in bhyve and better to have it fixed there.

[fabian.freyer_physik.tu-berlin.de]

In D14473#306827, @grehan wrote:

So a comment on this: in general, api's are not added to FreeBSD that don't have base-system clients that use them, or for a good reason. I think this falls into the latter but I'd like to cut it down a bit.

• can the get_unrestricted_guest() routine be removed ? There is an error return on the set, which seems like it can be used to determine if unrestricted mode is not available (e.g. that's how bhyve uses the ioctl).

Yes, it can.

• is there a need for vcpu_reset() ? The BSP should be initialized to power-on state.. That could be a bug in bhyve and better to have it fixed there.

Not necessarily, as everything in vcpu_reset() could also be accomplished with the other callbacks. I don't think bhyverun should call vcpu_reset(), as bhyveload sets up registers before. I guess this should happen before loader_main is called?

[jhb]

Do you still need the vm_set_unrestricted_guest callback? It seems easy to at least add that part of this patch.