Page MenuHomeFreeBSD

bhyve: add ROM emulation
ClosedPublic

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

Details

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
rG 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.

This patch is ready to be merged. Any comments on it?

FYI, this patch will enable GPU passthrough for AMD GPUs on Linux/BSD guests.

This revision was not accepted when it landed; it landed in state Needs Review.Mar 10 2022, 11:32 AM
Closed by commit rGe47fe3183e1f: bhyve: add ROM emulation (authored by c.koehne_beckhoff.com, committed by manu). · Explain Why
This revision was automatically updated to reflect the committed changes.
andy_omniosce.org added inline comments.
usr.sbin/bhyve/pci_emul.c
805

Porting this over to illumos, and the compiler is saying:

pci_emul.c:835:24: error: 'addr' may be used uninitialized in this function [-Werror=maybe-uninitialized]
  819 |  pdi->pi_bar[idx].addr = addr;

Seems right - do we need to set addr = 0; in this case block?

usr.sbin/bhyve/pci_emul.c
2123–2127

Porting to illumos, I'm getting:

pci_emul.c:2123 pci_cfgrw() error: buffer overflow 'pi->pi_bar' 7 <= 8

and several more.

I think line 2118 should be:

if (coff < PCIR_BIOS)
usr.sbin/bhyve/pci_emul.c
805

You're right. I think I've missed it because guests usually write to the ROM register before reading it. We should set addr = 0 in this case block.

usr.sbin/bhyve/pci_emul.c
2123–2127

Writes that aren't 4 byte aligned, are ignored. For that reason, if (coff != PCIR_BIOS) works as intented. However, making it more clear might be a good idea.

cem added inline comments.
usr.sbin/bhyve/pci_emul.c
805

Coverity also flagged this -- CID 1486777.

c.koehne_beckhoff.com added inline comments.
usr.sbin/bhyve/pci_emul.c
805
2123–2127