bhyve: add ROM emulation

[corvink]

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.

[markj]

usr.sbin/bhyve/pci_emul.h

237

Style is wrong.

usr.sbin/bhyve/pci_passthru.c

645

Use fstat() to get the file size.

652

Why the negative errno values?

670

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

1064

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

[corvink]

• fix style
• use fstat to detect romfile size
• use mmap to read romfile
• add error checking for passthru_addr_rom

[markj]

usr.sbin/bhyve/pci_emul.c

788

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.

790

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

627

662

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

747

1064

Shouldn't it be a fatal error?

[corvink]

usr.sbin/bhyve/pci_passthru.c

747

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

1064

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?

[corvink]

Address feedback

[corvink]

• fix formatting

[corvink]

usr.sbin/bhyve/pci_passthru.c

1064

@markj Should I change it to a fatal error?

[corvink]

• rebase on 14.0-CURRENT
• add bhyve_config entry
• fail hard if mapping the ROM fails

[corvink]

usr.sbin/bhyve/pci_passthru.c

1064

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.

[corvink]

• Rebase on 14.0-CURRENT

[corvink]

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.

[andy_omniosce.org]

usr.sbin/bhyve/pci_emul.c

735

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?

[andy_omniosce.org]

usr.sbin/bhyve/pci_emul.c

2036–2040

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)

[corvink]

usr.sbin/bhyve/pci_emul.c

735

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.

[corvink]

usr.sbin/bhyve/pci_emul.c

2036–2040

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]

usr.sbin/bhyve/pci_emul.c

735

Coverity also flagged this -- CID 1486777.

[corvink]

usr.sbin/bhyve/pci_emul.c

735

D34688

2036–2040

D34689