Page MenuHomeFreeBSD

bhyve: add ROM emulation
ClosedPublic

Authored by corvink on Nov 26 2021, 2:22 PM.
Tags
Referenced Files
Unknown Object (File)
Mon, Mar 18, 5:06 AM
Unknown Object (File)
Jan 27 2024, 10:11 AM
Unknown Object (File)
Jan 20 2024, 12:04 AM
Unknown Object (File)
Jan 15 2024, 11:02 PM
Unknown Object (File)
Jan 9 2024, 12:00 AM
Unknown Object (File)
Dec 20 2023, 7:31 AM
Unknown Object (File)
Dec 11 2023, 12:40 AM
Unknown Object (File)
Nov 28 2023, 9:54 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
Tests Skipped

Event Timeline

usr.sbin/bhyve/pci_emul.h
237

Style is wrong.

usr.sbin/bhyve/pci_passthru.c
653

Use fstat() to get the file size.

660

Why the negative errno values?

678

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

1076

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
855

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.

857

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
635
670

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

759
1076

Shouldn't it be a fatal error?

usr.sbin/bhyve/pci_passthru.c
759

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

1076

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
1076

@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
corvink added inline comments.
usr.sbin/bhyve/pci_passthru.c
1076

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 corvink, 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
806

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
806

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
806

Coverity also flagged this -- CID 1486777.

corvink added inline comments.
usr.sbin/bhyve/pci_emul.c
806
2123–2127