Page MenuHomeFreeBSD

bhyve: add ROM emulation
Needs ReviewPublic

Authored by c.koehne_beckhoff.com on Nov 26 2021, 2:22 PM.

Details

Reviewers
manu
markj
jhb
Group Reviewers
bhyve
Summary

Some PCI devices especially GPUs require a ROM to work properly. The
ROM is executed by boot firmware to initialize the device. To add
a ROM to a device use the new ROM option for passthru device (e.g.
-s passthru,0/2/0,rom=<path>/<to>/<rom>).

It's necessary that the ROM is executed by the boot firmware. It won't
be executed by any OS. Additionally, the boot firmware should be
configured to execute the ROM file. For that reason, it's only
possible to use a ROM when using OVMF with enabled bus enumeration.

Hint:
I've split this patch into multiple commits which are available at
https://github.com/Beckhoff/freebsd-src/commits/phab/corvink/rom_emulation.
These smaller commits may be easier to review. I can create a pull
request at github and we can talk about the commits at github if
you like to.

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

usr.sbin/bhyve/pci_emul.h
236

Style is wrong.

usr.sbin/bhyve/pci_passthru.c
652

Use fstat() to get the file size.

659

Why the negative errno values?

677

I think you can just mmap the ROM file and copy it directly, no? Seems like that would be simpler.

1075

There's no error checking. Why is that ok?

  • fix style
  • use fstat to detect romfile size
  • use mmap to read romfile
  • add error checking for passthru_addr_rom
usr.sbin/bhyve/pci_emul.c
854

Let's avoid conflating uint64_t with pointer types. This will not work on CHERI, where pointers store additional metadata that is lost when downcasting to uint64_t. Use a pointer here and return it to the caller, which can cast when storing the address in psc_bar.

856

Again here, it seems better to print a warning closer to the source of the error (note you can use warnc() to give a better hint) rather than returning a negative errno value that gets converted to -1 anyway.

usr.sbin/bhyve/pci_passthru.c
634
669

Some of the error paths leak the fd and the temporary mapping.

758
1075

Shouldn't it be a fatal error?

usr.sbin/bhyve/pci_passthru.c
758

Hmmm. I'm using autoformatting based on the .clang-format in current sources. It creates my version.

1075

I've used the same style like passthru_mmio_addr.
I'm not sure if it should be a fatal error or not. The guest could set an invalid rom address. Should bhyve be killed in that case?

usr.sbin/bhyve/pci_passthru.c
1075

@markj Should I change it to a fatal error?

  • rebase on 14.0-CURRENT
  • add bhyve_config entry
  • fail hard if mapping the ROM fails
c.koehne_beckhoff.com added inline comments.
usr.sbin/bhyve/pci_passthru.c
1075

It's a fatal error now. If someone has issue with that due to a buggy guest, it'll be easy to change it later. It's much harder to fix issues when it's ignored.