bhyve: add config option to modify lpc ids

[corvink]

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.

[markj]

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]

Address feedback

[markj]

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]

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]

• 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]

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.

[markj]

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]

• rebase on main
• start sentences on new line in bhyve_config
• check lpc class and subclass

[corvink]

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]

• Rebase on 14.0-CURRENT

[markj]

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]

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]

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!

[jhb]

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.

[corvink]

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

[manu]

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.

[corvink]

• 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]

oops, updated to fast:

• fix build

[jhb]

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]

usr.sbin/bhyve/pci_lpc.c

480

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

[corvink]

• unify config keys with hostbridge

[corvink]

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

[jhb]

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