Details

Reviewers

jrm
rgrimes
emaste

Group Reviewers

docs

Commits

R9:5ab12e8f5aaf: Handbook/Virtualization: Various improvements to bhyve content

Summary

Some initial update suggestions for bhyve-related documentation. I figured, I'll start small to get an understanding whether I'm on the right path before expanding on the content further.

I have a longer list of items to follow-up from conversations from our weekly bhyve calls - so I have to thank a lot of people who brought their ideas for this...

Diff Detail

Repository

R9 FreeBSD doc repository

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

freebsd_ny-central.org requested review of this revision.Feb 14 2024, 8:48 PM

freebsd_ny-central.org created this revision.

Thanks. These are nice updates. I added a few mostly nit-picky comments and added a few other reviewers.

documentation/content/en/books/handbook/virtualization/_index.adoc
496	Should we use the Oxford comma like in the original?
520	Grammarly tells me we don't need the comma after `this`.
553	I wonder if we should refer to the script at this point. Something like, "FreeBSD comes with an example script, vmrun.sh, for running a virtual machine in bhyve. Maybe it was intentional to not refer to the script here because if the name ever changes, it would require more updates. I still think it sounds awkward to only refer to it as `The script`.
555	I like turning this into a list. I see this isn't an exhaustive list of options, so maybe we could phrase it something like this. It takes a number of options to control the configuration of the machine, including:
588	Grammarly tells me we also need a comma after `alternatively`.
598–599	I don't think this renders as you intended. Did you mean to make this an unordered list?
782	Grammarly recommends that we hyphenate the adjective, so it's `metadata-only` cache.

jrm added a reviewer: docs.Feb 14 2024, 11:10 PM

emaste added a subscriber: emaste.Feb 15 2024, 12:15 AM

emaste added inline comments.Feb 15 2024, 12:22 AM

documentation/content/en/books/handbook/virtualization/_index.adoc
496	I'd probably drop "even" -- hopefully at this point it isn't surprising or unusual that we support Windows.
502	Processors with the necessary support have been available for years so we no longer need "newer." In a subsequent commit we may want to change this to list the one or two families of CPUs that lack support.
588	We don't need the (R) after the first use of a trademark. I know the existing text has it everywhere but no need to add more of them.

editor_callfortesting.org added a subscriber: editor_callfortesting.org.Feb 15 2024, 6:20 PM

Thanks for the great feedback. I'll update the patch accordingly.

Just some nits.

documentation/content/en/books/handbook/virtualization/_index.adoc
496	I'd probably drop "even" -- hopefully at this point it isn't surprising or unusual that we support Windows. I had the same feeling when I read this change before seeing Ed's comment
502	Perhaps simply "any processor nehalem or newer"? IIRC thats the cut off point
553	vmrun.sh was suppose to be just a stop gap thing, but since it has lasted throught 4 major releases I would say that stop gap status has sailed and directly mentioning it here would be good.
555	Dont document vmrun.sh here, simply link to the man page.
689	strike userspace, its nonsensical in the context of firmware.
707	We should be consistent on EFI vs UEFI, use one or the other in all places please
782	Also recommend the use of zvol's vs a file in a file system for the disk image.
812	I believe the correct and full disconnect sequence is Newline, tilde, dot. If your ~ does not follow a newline you wont get the disconnect, or at least that is how it works every where else.

Oh, one other comment - when you regenerate the new patch please use -U999999 to get full context - Phabricator will then allow the reader to expand the blocks in between what's been changed.

There's info on the wiki at https://wiki.freebsd.org/Phabricator

Updates after initial feedback - also fixed version of ISO files in the command line listings.

Sorry, just realized I missed the -U99999; included this now.

emaste added inline comments.Feb 15 2024, 7:17 PM

documentation/content/en/books/handbook/virtualization/_index.adoc
502	I think Nehalem is old enough that it's not going to be clear to users today where that lands. Maybe something like "Most processors made in 2008 or later?" (or whichever year) Or "Intel processors made in 2010 and later support these features" (picking up UG).
707	Heh, from efivar(8) efivar – UEFI environment variable interaction

Included additional updates after Rod's feedback. Thanks for the inputs!

And finally condensed the processor feature constraints into a simpler and shorter statement.

Included the relevant feedback points into this latest update. Thanks for the great inputs. Any further inputs/feedback welcome!

documentation/content/en/books/handbook/virtualization/_index.adoc
502	Good point. I too was feeling this is overly specific. I'll attempt to condense this further.
555	There appears to be no man page? At least I'm not getting anything for vmrun.sh on my 13.2-RELEASE. Might be worthwhile to create a man page for this in a separate review, I suppose.
782	Good point - there is a statement to this a few lines up; in my previous diff, this wasn't visible because I missed the -U99999 initially on my patch. Apologies for that.

Its getting much better, probably with pushing a commit soon.

documentation/content/en/books/handbook/virtualization/_index.adoc
502	Clear or not Nehalem is a well documented intel micro architecture, both with its own Wiki page, and as a member of the list of Intel CPU microarchitectures. The reduction to "most processors" is wrong, that would imply arm, risc-v, or even an at-meta has support for bhyve.
555	Ok, there is "vmrun.sh -usage" though. I dislike the idea of documenting it here, even partially and NOT providing a method to get the full syntax avaliable. Also the documenting here is likely to bit rot rather quickly
569	Is the "sh" even needed, this file should already be installed mode 755 so can be directly invoked. If its not installed 755 that should be fixed.
812	I do not see that this was incorporated???

This revision is now accepted and ready to land.Feb 16 2024, 6:00 PM

freebsd_ny-central.org marked an inline comment as not done.Feb 16 2024, 7:03 PM

freebsd_ny-central.org added inline comments.

documentation/content/en/books/handbook/virtualization/_index.adoc
812	Uh, right - apologies. Thanks for the feedback. I didn't read that right and mixed it up with a dot list syntax mistake I made previously. I'll fix this right now and update the diff one more time.

freebsd_ny-central.org added inline comments.Feb 16 2024, 7:11 PM

documentation/content/en/books/handbook/virtualization/_index.adoc
555	You're certainly right with the potential for bitrot. For now, I just changed the formatting of the previously listed options. I'll add some info about --usage to allow users to get a full list of available options.
569	Good catch. Unfortunately, it seems to be 444 right now. That's what it is on my 13.2-RELEASE at the moment, at least. I agree that it would be good to change that. I'll have a look at that for another review.

Updated paragraph on CPU architecture to reference Nehalem instead.
Added --usage reference for vmstart.sh
Fixed disconnect key sequence for cu

This revision now requires review to proceed.Feb 16 2024, 7:31 PM

freebsd_ny-central.org marked 5 inline comments as done.Feb 16 2024, 7:31 PM

I asked some doc committers on IRC to have a look.

[2024-02-16 15:40] <jrm> Any doc committers available to look at https://reviews.freebsd.org/D43899 ? It's been through quite a bit of review, so hopefully, a quick scan will suffice.

I'll commit with this log message unless there are objections.

Author: Chris Moerz <freebsd@ny-central.org>
Date:   2024-02-16 15:34

    Handbook/Virtualization: Various bhyve improvements

    Tweak text on:
      - supported guest OSes
      - supported processors
      - connecting to host network
      - vmrun.sh
      - booting VMs

    Add new content for:
      - storing UEFI variables
      - recommendations for using ZFS metadata-only cache
      - disconnecting from VM console
      - gracefully shutting down VMs
      - ports for managing VMs

    Reviewed by:    emaste, jrm, rgrimes
    Differential Revision:  https://reviews.freebsd.org/D43899

This revision is now accepted and ready to land.Feb 16 2024, 8:05 PM

emaste added inline comments.Feb 16 2024, 9:12 PM

documentation/content/en/books/handbook/virtualization/_index.adoc
502	Mentioning together "newer processors" and a processor family that is about a decade and a half old doesn't make any sense. To a first order approximation bhyve can be used on any x86_64 processor in use today. Sure, there are some old machines around still and there are retrocomputing enthusiasts, but for the majority of readers this isn't useful information. We also don't give any information about which AMD families are supported. I would just take this whole sentence out.

freebsd_ny-central.org added inline comments.Feb 16 2024, 9:34 PM

documentation/content/en/books/handbook/virtualization/_index.adoc
502	Fair point. I too was wondering about AMD, but since I couldn't find any readily available listing of processors with RVI, I left it as is. I guess it's a reasonable compromise to just take it out completely, because we have a paragraph on how to check whether the processor supports bhyve. I'll quickly remove the sentence and update the diff so we can get this landed.