Page MenuHomeFreeBSD

bhyve: add config option to modify lpc ids
ClosedPublic

Authored by corvink on Jan 22 2021, 10:59 AM.
Tags
Referenced Files
Unknown Object (File)
Fri, Oct 4, 11:33 AM
Unknown Object (File)
Wed, Oct 2, 1:16 AM
Unknown Object (File)
Wed, Oct 2, 12:04 AM
Unknown Object (File)
Tue, Oct 1, 5:01 PM
Unknown Object (File)
Tue, Oct 1, 3:23 PM
Unknown Object (File)
Mon, Sep 30, 4:54 PM
Unknown Object (File)
Sun, Sep 29, 9:50 AM
Unknown Object (File)
Fri, Sep 27, 8:10 AM

Details

Summary
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.

Diff Detail

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

Event Timeline

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

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.

corvink edited the summary of this revision. (Show Details)

Address feedback

usr.sbin/bhyve/pci_lpc.c
94

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

538

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
547

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).

corvink 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.

corvink added inline comments.
usr.sbin/bhyve/pci_lpc.c
547

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
506

Should we also check the device class and subclass?

527

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?

corvink marked an inline comment as done.
  • rebase on main
  • start sentences on new line in bhyve_config
  • check lpc class and subclass
corvink 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
506

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.

527
/*
 * 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.

corvink marked 5 inline comments as done.
  • Rebase on 14.0-CURRENT

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
504
510
514

or "prove"

manu added inline comments.
sys/dev/pci/pcireg.h
1105 ↗(On Diff #103825)

@bapt made a script convert the pci vendor file to an C header, I just need to commit that and patch the different files that uses those.

bz added inline comments.
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
533

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.

In D28280#813137, @c.koehne_beckhoff.com wrote:

Thanks for your feedback. I'm going to update this patch when @manu finished his vendor id work.

Thanks for the reminder, I'll add that to my todo list for this week.

  • 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
corvink retitled this revision from bhyve: set lpc IDs to physical values to bhyve: add config option to modify lpc ids.Feb 1 2023, 1:43 PM
corvink edited the summary of this revision. (Show Details)

oops, updated to fast:

  • fix build

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
464–465
480

Could maybe s/def/default/. :)

corvink added inline comments.
usr.sbin/bhyve/pci_lpc.c
480

error: invalid parameter name: 'default' is a keyword

corvink marked an inline comment as done.
  • unify config keys with hostbridge

@markj @jhb Can you take a look at this patch stack? Thanks.

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).

This revision was not accepted when it landed; it landed in state Needs Review.Mar 27 2023, 8:11 AM
This revision was automatically updated to reflect the committed changes.