Page MenuHomeFreeBSD

bhyve: set lpc IDs to physical values
Needs ReviewPublic

Authored by c.koehne_beckhoff.com on Jan 22 2021, 10:59 AM.

Details

Reviewers
manu
markj
jhb
Group Reviewers
bhyve
Summary

The VID, DID, REVID, SUBVID and SUBDID of igd-lpc need aligned with physical one. Without these physical values, GVT-d GOP driver couldn't work.

Note: See https://github.com/Beckhoff/freebsd-src/commits/phab/corvink/lpc-id

Diff Detail

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

Event Timeline

markj added inline comments.
usr.sbin/bhyve/pci_lpc.c
478

Please limit capabilities on the descriptor, see the use of caph_rights_limit() and caph_ioctls_limit() in passthru_init(). Actually I do not see why the descriptor needs to be kept open indefinitely.

usr.sbin/bhyve/pci_lpc.c
93

I'd prefer to pass the descriptor as a parameter rather than introducing a global variable.

491

Sorry, I was not clear. It's not really worthwhile to limit rights if the descriptor is not kept open. Since it's closed right after this, the caph_* calls can be dropped IMO.

jhb added inline comments.
usr.sbin/bhyve/pci_lpc.c
500

In general I'd like some way to not force this on always perhaps? Or at least provide a configuration knob to allow it to be turned off in case it breaks other systems that don't need GVT-d.

Ideally I think what I would like is for the lpc device to honor config values to set these registers and have a hook here that sets the config variables if they aren't always set (so the user can always override them if needed).

c.koehne_beckhoff.com added reviewers: markj, jhb.
  • reuse read_config of pci_passthru
  • allow users to overwrite LPC ids

I've split my patch into multiple commits at https://github.com/Beckhoff/freebsd-src/commits/phab/corvink/lpc-id. It may be easier to review these small commits than the whole patch.

Btw: I can also create a pull request at https://github.com/Beckhoff/freebsd-src and we can talk in that pull request about the commits if you like to.

c.koehne_beckhoff.com added inline comments.
usr.sbin/bhyve/pci_lpc.c
500

What do you think about the current solution? It allows the user to override all values if he wants.

usr.sbin/bhyve/bhyve_config.5
138 ↗(On Diff #99009)

New sentences should start on new lines.

usr.sbin/bhyve/config.h
103 ↗(On Diff #99009)
115 ↗(On Diff #99009)
usr.sbin/bhyve/pci_lpc.c
473

Should we also check the device class and subclass?

494

Please add a comment explaining why we look at the host device.

usr.sbin/bhyve/pci_passthru.h
28 ↗(On Diff #99009)

Why does it need to be exported? Should this be rebased on D33769?

c.koehne_beckhoff.com marked an inline comment as done.
  • rebase on main
  • start sentences on new line in bhyve_config
  • check lpc class and subclass
c.koehne_beckhoff.com added inline comments.
usr.sbin/bhyve/config.h
103 ↗(On Diff #99009)

It's already merged. See D33770.

115 ↗(On Diff #99009)

It's already merged. See D33770.

usr.sbin/bhyve/pci_lpc.c
473

On physical systems, it's unnecessary. However, it might change in future and it could be different in nested VM situations. So yeah, it makes sense to check device class and subclass too.

494
/*
 * The VID, DID, REVID, SUBVID and SUBDID of lpc need to be
 * aligned with the physical ones. Without these physical
 * values, GPU passthrough of Intel integrated graphics devices
 * won't work properly. The Intel GOP driver checks these values
 * to proof that it runs on the correct platform.
 */

It's already there.

usr.sbin/bhyve/pci_passthru.h
28 ↗(On Diff #99009)

I've cherry picked that part from another patch, which requires it to be exported too. Yes, it should be rebase on D33769.