Details

Reviewers

grehan
neel
bcr
glebius

Group Reviewers

manpages

Commits

rS317777: Improve documentation of fbuf device.

Summary

document fbuf and xhci devices and their configuration
add example for an UEFI virtual machine

PS I think the 'bootrom' description should be extended too. Currently, the manpage only shows that it's a possible choice for '-l', but doesn't provide configuration details, i.e. that a path to the firmware file needs to be provided. I'm wondering if the '-l lpcdev[,conf]' bit should be structured in the same way as the '-s slot,emulation[,conf]' block with a little more detailed description of the 'bootrom' option? Feedback on that is welcome.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 8061
Build 8253: CI src build	Jenkins

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Herald added a reviewer: manpages. · View Herald TranscriptMar 15 2017, 3:32 PM

Herald added a subscriber: imp. · View Herald Transcript

Harbormaster failed remote builds in B8060: Diff 26276!Mar 15 2017, 3:38 PM

Added a note that external loaders are not needed when using boot ROM.

Harbormaster failed remote builds in B8061: Diff 26277!Mar 15 2017, 4:07 PM

Many thanks for doing this. I'll get some detailed comments shortly.

I have two suggestions.

bhyve/bhyve.8
321	You need a comma between "height" and "respectively".
429	I would add "pixels" after the 600.

Added missing comma and "pixels".

novel marked 2 inline comments as done.Mar 18 2017, 5:36 PM

Harbormaster failed remote builds in B8119: Diff 26392!Mar 18 2017, 5:40 PM

grehan added inline comments.Mar 20 2017, 4:27 AM

bhyve/bhyve.8
52	using a boot ROM
312	"tcp=" will be deprected. bhyve currenctly accepts "rfb" for the VNC protocol so that one should be documented.
312	The width, height and wait parameters are optional. Width/height defaults to 1024x768, with a minimum of 640x480. Another optional parameter is "vga=[on\|io\|off]", which controls whether the frame buffer exports standard VGA registers. Windows 7/2k8 guests require "io", which is the default. "on" should be used when using graphics with BIOS guests, and "off" is required for OpenBSD UEFI guests to prevent the early probe from assuming a VGA framebuffer is present.
385	using a boot ROM
390	Otherwise, the boot loader is not needed.

Thanks for feedback! Some questions inline.

bhyve/bhyve.8
312	Is there probably some general description of the "io" option? Also, are there any rules regarding mentioning Windows in the manual pages, i.e. should we put stuff like (tm) or (r) or anything like that? The first thing that came to my mind is wine(1) and it just goes with "Windows $Ver", though it's not a part of the FreeBSD base.
312	Oh, it's unfortunate that 'tcp=' is getting deprecated as I'm using that in the libvirt driver already. Is there a way to detect that in runtime? I can see that bhyve can report unsupported configuration for the fbuf device, but that happens pretty late in the VM creation process, i.e. I have to run in this way: /usr/sbin/bhyve -m 2048 -s 4:0,fbuf,tcp=127.0.0.1:5904,oops test It does not work if I omit "-m" and guest name. Also, it creates /dev/vmm/test, so I have to call destroy on it, which makes such kind of a probe a little expensive. PS In a message "Invalid fbuf emulation "oops" fbuf: {wait,}{vga=on\|io\|off,}rfb=<ip>:port" do commas have to go after the closing brace to follow common style?

grehan added inline comments.Mar 23 2017, 12:13 AM

bhyve/bhyve.8
312	PCI graphics cards have a dual personality in that they are standard PCI devices with BAR addressing, but may also implicitly decode legacy VGA i/o space (0x3c0-3df) and memory space (64KB at 0xA0000). A true non-legacy UEFI-only graphics adapter doesn't require any of the legacy decodes. However, Windows 7 and 2k8 still attempt to issue BIOS calls which result in i/o port queries. If the i/o decode is disabled, the Windows guest will fail to boot. This corresponds to the "vga=io" mode. Regarding explicit references to Windows - that could be changed to "some guests" and the FreeBSD wiki used to demonstrate options for particular guests. I can just leave "tcp=" in the code. But, best not to document it. I know we've chatted about ways to try and determine what features are available at run-time. Let me think a bit more about that. When using the CSM BIOS compatibility in UEFI to boot traditional BIOS guests, the complete i/o and memory decode needs to be present ("vga=on"). OpenBSD in UEFI mode will detect i/o ports and assume a VGA adapter is present. To prevent this, the "vga=off" mode is provided. Note that picking a default will prevent either Windows or OpenBSD from functioning. Since Win7 seems very prevalent, the default was selected to allow that.

Added a description of the "vga=" configuration options.

Harbormaster failed remote builds in B8258: Diff 26606!Mar 23 2017, 4:01 PM

novel marked 3 inline comments as done.Mar 23 2017, 4:07 PM

novel added inline comments.

bhyve/bhyve.8
312	Thanks for a great explanation! I've tried to summarize it up using the powerful copy/paste technique. Hopefully I didn't mess up with with the rest.

ping?

One small nit remains, then it is ready to go from my side.

bhyve/bhyve.8
390	This does not seem to be "done" yet.

novel updated this revision to Diff 27266.Apr 9 2017, 2:54 PM

novel marked an inline comment as done.

novel added inline comments.

bhyve/bhyve.8
390	Oops, I've added the comma after "otherwise", but didn't pay attention to the missing "the". Should be fine now, thanks for noting.

Harbormaster failed remote builds in B8638: Diff 27266!Apr 9 2017, 3:00 PM

novel marked an inline comment as done.Apr 9 2017, 3:00 PM

OK from manpages.

This revision is now accepted and ready to land.Apr 9 2017, 3:00 PM

Fine to commit after the max fbuf res added.

Once again - many thanks for doing this.

bhyve/bhyve.8
322	Probably worth documenting the current max of 1920 x 1200

bjk added a subscriber: bjk.Apr 9 2017, 10:33 PM

bjk added inline comments.

bhyve/bhyve.8
331	It is very strange to see zero-width-space+'.' on its own line like this. ".Dq off ." all on one line should be fine (and put the period outside the quotes), and there's always the Do and Dc macros if finer control is needed.
338	ditto here. (And the new sentence should start on a new line regardless.)
340	s/the guests/guests
342	mdoc.9 says "This macro should not be used" about .br; I think .Pp will work fine within the list environment. (many appearances)
347	I don't think I understand this sentence. Is there another word that can go with "memory decode is present" to clarify what exactly is present -- the decoded memory itself, or the ability to decode memory, or ...?
360	Could use .Lk for the link.
429	These things are always somewhat contentious, but I would pronounce "UEFI" as "You ee eff aye", which corresponds to just "a UEFI".

novel updated this revision to Diff 27302.Apr 10 2017, 3:40 PM

novel edited edge metadata.

This revision now requires review to proceed.Apr 10 2017, 3:40 PM

novel updated this revision to Diff 27303.Apr 10 2017, 3:42 PM

novel updated this revision to Diff 27304.

Harbormaster failed remote builds in B8655: Diff 27302!Apr 10 2017, 3:44 PM

Harbormaster failed remote builds in B8656: Diff 27303!Apr 10 2017, 3:48 PM

novel marked 6 inline comments as done.Apr 10 2017, 3:52 PM

novel added inline comments.

bhyve/bhyve.8
342	Yeah, and it even looks nicer with .Pp instead of .br.
347	I think that's "the ability to decode ...". Though I hope Peter could suggest something on that as he's clearly more familiar with the topic.

Harbormaster failed remote builds in B8657: Diff 27304!Apr 10 2017, 3:52 PM

novel added inline comments.Apr 27 2017, 2:59 PM

bhyve/bhyve.8
347	@grehan could you please help with this one?

grehan added inline comments.Apr 27 2017, 4:34 PM

bhyve/bhyve.8
347	How about "to boot traditional BIOS guests that require the legacy VGA I/O and memory regions to be available"

bjk added inline comments.Apr 28 2017, 1:58 AM

bhyve/bhyve.8
347	That sounds good to me, thanks.

Abandon because a similar change was committed already: https://svnweb.freebsd.org/changeset/base/317545

novel reclaimed this revision.Apr 28 2017, 6:42 PM

novel updated this revision to Diff 27838.

Harbormaster failed remote builds in B8975: Diff 27838!Apr 28 2017, 6:46 PM

I'm very sorry for committing in front of you. Do you want me to commit your updates on top or would you prefer for someone else to do it?

This revision is now accepted and ready to land.Apr 29 2017, 2:10 AM

In D10014#218425, @glebius wrote:

I'm very sorry for committing in front of you. Do you want me to commit your updates on top or would you prefer for someone else to do it?

I'd love if you could commit it, thanks.

I think that some parts of my version gave more precise description of options. I am not sure about the mdoc formatting, though.

Here is your options string:

rfb=host:port[,w=width,h=height][,vga=vgaconf][,wait]

Here is mine:

[wait][,vga=<on|io|off>][,rfb=[IP:]port][,w=w][,h=h]

So, what's different.

In your desription the rfb= is mandatory, while indeed it is not. It is not required. After my recent changes it defaults to 127.0.0.1:5900, before them it defaulted to 0.0.0.0:random_port.
If you specify rfb= you may omit the IP address, but port must be present. Your description doesn't note that. Also my documentation focuses on that this is IPv4 address, not host. bhyve doesn't resolve neither supports IPv6, your version deletes that.
Your description implies setting w and h together, while indeed you can set either of them, leaving the other on default value.

I like very much your description of the vga= option.

Can you please check that and merge our versions better?

bhyve/bhyve.8
193–198	I think my description was better. Without details on VNC the description is too bare. What is our side of framebuffer device? May be it is just memory region to emulate VGA, that does nothing instead of providing VGA device to the guest?

This revision now requires changes to proceed.May 1 2017, 8:29 PM

novel updated this revision to Diff 27918.May 2 2017, 1:42 PM

novel edited edge metadata.

Harbormaster failed remote builds in B9017: Diff 27918!May 2 2017, 1:46 PM

In D10014#218880, @glebius wrote:

I think that some parts of my version gave more precise description of options. I am not sure about the mdoc formatting, though.

Here is your options string:

rfb=host:port[,w=width,h=height][,vga=vgaconf][,wait]

Here is mine:

[wait][,vga=<on|io|off>][,rfb=[IP:]port][,w=w][,h=h]

So, what's different.

In your desription the rfb= is mandatory, while indeed it is not. It is not required. After my recent changes it defaults to 127.0.0.1:5900, before them it defaulted to 0.0.0.0:random_port.

If you specify rfb= you may omit the IP address, but port must be present. Your description doesn't note that. Also my documentation focuses on that this is IPv4 address, not host. bhyve doesn't resolve neither supports IPv6, your version deletes that.

For 1) and 2) I've updated the option usage string and pulled in some bits from your original description.

Your description implies setting w and h together, while indeed you can set either of them, leaving the other on default value.

I've accounted that in the update option usage string as well, though decided not to explicitly mention that in text because it does not seem to be a very common case.

Also, I took courage to replace "A IP" -> "An IP" as the former looks more familiar to me (e.g. wikipedia seems to use that), though I'm not 100% sure it's correct.

I like very much your description of the vga= option.

Can you please check that and merge our versions better?

Done.

bhyve/bhyve.8
193–198	Agreed, will roll back to your version.

I missed one thing yesterday. The default for vga is "on", not "io". Lines 363-364 of pci_fbuf.c.

Do you want me to fix it before commit, or would you prefer to update the phab?

In D10014#219157, @glebius wrote:

I missed one thing yesterday. The default for vga is "on", not "io". Lines 363-364 of pci_fbuf.c.

The default is "io" - enabled/full set to 1/0 means io-only. See the combinations in pci_fbuf_parse_opts() when parsing "vga="

Sorry, I was wrong. Thanks.

Closed by commit rS317777: Improve documentation of fbuf device. (authored by glebius). · Explain WhyMay 3 2017, 9:44 PM

This revision was automatically updated to reflect the committed changes.

bhyve: document fbuf and xhci devices in the manpage
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 26277

bhyve/bhyve.8

bhyve: document fbuf and xhci devices in the manpageClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 26277

bhyve/bhyve.8

bhyve: document fbuf and xhci devices in the manpage
ClosedPublic
Actions

Revision Contents
Changeset List