Page MenuHomeFreeBSD

userboot: add callbacks to set unrestricted guest mode
Needs ReviewPublic

Authored by fabian.freyer_physik.tu-berlin.de on Feb 22 2018, 5:47 PM.

Details

Reviewers
imp
rgrimes
grehan
Group Reviewers
bhyve
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

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

imp accepted this revision.Feb 22 2018, 7:27 PM
This revision is now accepted and ready to land.Feb 22 2018, 7:27 PM

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 accepted this revision.Feb 22 2018, 10:49 PM
grehan added a subscriber: 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 edited the summary of this revision. (Show Details)
fabian.freyer_physik.tu-berlin.de edited the test plan for this revision. (Show Details)
  • updated the comment at the version identifier to correctly list the changes introduced by this version
  • added a callback to vcpu_reset
This revision now requires review to proceed.Feb 22 2018, 11:09 PM
grehan added inline comments.Feb 22 2018, 11:13 PM
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.

addressed @grehan's comments regarding style(9)

I hope I got them all now

rgrimes accepted this revision as: rgrimes.Feb 23 2018, 12:03 AM
rgrimes removed reviewers: bhyve, grehan.
This revision is now accepted and ready to land.Feb 23 2018, 12:03 AM

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

pass USERBOOT_VERSION_5 to loader_main.

This revision now requires review to proceed.Feb 23 2018, 7:16 PM

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

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?

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 added a comment.Mar 7 2018, 4:55 PM

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.

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?