Page MenuHomeFreeBSD

bhyve: add hook for PCI header of passthru devices
ClosedPublic

Authored by corvink on Nov 16 2021, 12:23 PM.
Tags
Referenced Files
Unknown Object (File)
Mon, Oct 7, 12:21 PM
Unknown Object (File)
Thu, Oct 3, 8:23 AM
Unknown Object (File)
Wed, Oct 2, 10:06 AM
Unknown Object (File)
Tue, Oct 1, 3:21 AM
Unknown Object (File)
Tue, Oct 1, 12:27 AM
Unknown Object (File)
Tue, Oct 1, 12:07 AM
Unknown Object (File)
Mon, Sep 30, 5:52 AM
Unknown Object (File)
Fri, Sep 27, 7:00 AM

Details

Summary
Most register of the PCI header are either constant values or require
emulation anyway. The command and status register are the only exception which
require hardware access. So, we're adding an emulation handler for all
other register.

As this emulation handler will be reused by some future features like
GPU passthrough, we directly export it.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

corvink edited the summary of this revision. (Show Details)
corvink added reviewers: jhb, markj.
usr.sbin/bhyve/pci_passthru.c
617

Humm, I would maybe just use int or u_int instead of uint32_t for config register addresses throughout? The new function hooks all use int.

617

I would maybe use int or u_int instead of uint32_t for PCI config space addresses? If you really wanted a fixed-width type, uint16_t would be the most accurate, but a simple int is what the new function hooks use already.

Also, I would maybe use i <= PCIR_MAXLAT instead of adding a new constant.

624

Humm, is it really safe to read all these as individual bytes? I would be tempted to read things like BARs as 32-bit values as I'm not sure if all hardware is going to tolerate byte reads?

664

80 cols?

893

Maybe "Default PCI config registers to accessing the config register of the physical device."

895–896

The whitespace seems inconsistent here and in the clauses below?

897

Newlines before comments.

Also, I would maybe reword this somewhat to be more like "Emulate most PCI header registers",

940

How is this handled now? Seems like you need to add a new call to set emulated hooks for this range when this value is true?

945

()'s around the "argument" to return

946–947

Hummm, shouldn't these be handled by setting the ranges for these config registers to the emulated hooks earlier?

That is, I guess you need custom handlers for write, but having passthru_cfgwrite_msicap seems more consistent with the change you are making here?

948

I think it would be beneficial for this patch to move this into a separate function.

  • split patch into multiple
corvink retitled this revision from bhyve: use handlers for cfg reads/writes of passthru devices to bhyve: add hook for PCI header of passthru devices.
corvink edited the summary of this revision. (Show Details)
usr.sbin/bhyve/pci_passthru.c
894

But now we don't emulate accesses to fields below PCIR_BAR(0)?

usr.sbin/bhyve/pci_passthru.c
894

I should improve my commit message.

Except the command and status register, the PCI header contains fixed values or values which require emulation anyways. So, IMHO, we should always emulate the whole PCI header. It makes the code simpler and avoids hardware accesses.

PCIR_CAP_PTR is always emulated. So, we can remove one LEGACY_SUPPORT define.

  • remove static from emulation handler to properly export them
markj added inline comments.
usr.sbin/bhyve/pci_passthru.c
618
619
This revision is now accepted and ready to land.Apr 11 2023, 8:05 PM