The Intel GOP driver checks the lpc ids to detect the platform it's running on. The GOP driver only works on the platforms it's written for. Maybe other Intel driver have the same behaviour. For that reason, we should use the lpc ids of the FreeBSD host for GPU passthrough to work properly. We don't know if setting different lpc ids have any side effect. Therefore, don't use the host lpc ids by default on Intel system. Give the user the opportunity to modify the lpc ids. This option accepts hexadecimal numbers and the special "host" keyword to simplify setting host lpc ids.
Details
- Reviewers
manu markj jhb - Group Reviewers
bhyve - Commits
- rGd4cae9c5e63e: bhyve: add config option to modify LPC IDs
rGf4ceaff56dda: bhyve: add config option to modify LPC IDs
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Skipped - Unit
Tests Skipped - Build Status
Buildable 49565 Build 46455: arc lint + arc unit
Event Timeline
usr.sbin/bhyve/pci_lpc.c | ||
---|---|---|
508 | 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 | ||
---|---|---|
531 | 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). |
- 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.
usr.sbin/bhyve/pci_lpc.c | ||
---|---|---|
531 | 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 | 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 | ||
490 | Should we also check the device class and subclass? | |
511 | 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? |
- rebase on main
- start sentences on new line in bhyve_config
- check lpc class and subclass
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 | ||
490 | 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. | |
511 | /* * 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. |
Seems ok to me, just some notes about the comments and man page.
sys/dev/pci/pcireg.h | ||
---|---|---|
1105 ↗ | (On Diff #103825) | This define already exists in bhyve in multiple places. The duplication of vendor IDs is annoying and should be solved, but I don't think this is the right place to do it. |
usr.sbin/bhyve/bhyve_config.5 | ||
143 | same below | |
144 | What happens on AMD systems? I can't quickly check, do they have an Intel-compatible LPC device? | |
145 | ||
161 | There is a "LPC device settings" section in the man page, this info should appear there. Rather than repeating the same verbiage for each variable, I'd suggest briefly explaining how and why bhyve populates the default values using the physical device. | |
usr.sbin/bhyve/pci_lpc.c | ||
488 | ||
494 | ||
498 | or "prove" |
sys/dev/pci/pcireg.h | ||
---|---|---|
1105 ↗ | (On Diff #103825) | Oh I'd love that especially if the output names match the ones we take from Linux left and right so we can, like for USB, just use the one header file everywhere without having to define our own ones everywhere. Would be wonderful. Thank you two for working on this! Super-Heroes! |
usr.sbin/bhyve/pci_lpc.c | ||
---|---|---|
517 | The way this is handled in pci_hostbridge.c might be a bit simpler than needing set_config_value_if_unset, etc. It does something like this: u_int vendor, device, revid, subvendor, subdevice; const char *value; vendor = LPC_DEV; device = 0; ... /* On Intel systems... */ if (....) { vendor = read_config(&sel, PCIR_VENDOR, 2); ... } /* Check for config variable overrides. */ value = get_config_value_node(nvl, "vendor"); if (value != NULL) vendor = strtol(value, NULL, 0); ... It might also be nice to use a single PCIOCGETCONF request to fetch the 'conf' structure for the LPC rather than a bunch of read_config ioctls. I'm also still a bit torn on if we need a separate 'lpc.use_host_ids' or the like that gated the if clause to override the values with host values. |
Thanks for your feedback. I'm going to update this patch when @manu finished his vendor id work.
- use a config option to set the lpc id
- the user can specify any lpc id
- the special "host" config option uses the same lpc id as the host lpc
In general I like this approach. My only thought is that I know we already have one other place to set the device/vendor ID of host bridges that uses a different schema (the variables are set on the PCI device itself and I haven't checked if the naming matches). If nothing else, it might be nice to try to fix the two to be consistent on how the config variables are named. I would also maybe consider spelling out "pcireg" instead of "pcir"?
usr.sbin/bhyve/pci_lpc.c | ||
---|---|---|
448–449 | ||
464 | Could maybe s/def/default/. :) |
usr.sbin/bhyve/pci_lpc.c | ||
---|---|---|
464 | error: invalid parameter name: 'default' is a keyword |
I think the stack looks fine. My one suggestion would be a general one in commit logs to s/pci/PCI/ and s/ids/IDs/. I'd be tempted to upper-case LPC as well as it is an acronym (https://en.wikipedia.org/wiki/Low_Pin_Count).