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.

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

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.

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

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

New sentences should start on new lines.

Should we also check the device class and subclass?

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

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

It's already merged. See D33770.

It's already merged. See D33770.

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.

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

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

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.

same below

What happens on AMD systems? I can't quickly check, do they have an Intel-compatible LPC device?

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.

or "prove"

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

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!

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.

Could maybe s/def/default/. :)

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